Re: [PATCH 2/2] hfsplus: do not end readdir at i_size after an unlink

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Thu, 2026-11-12 at 06:17 -0500, Hengyu Liang wrote:
> Commit 4b0496432844 ("hfsplus: rework hfsplus_readdir() logic")
> stopped
> moving the position of open directories back when an entry is
> deleted.
> 
> However, hfsplus_readdir() still ends when ctx->pos reaches i_size,
> and
> i_size drops on every delete. As of now, a program that reads a
> directory and deletes what it read sees the directory end early and
> leaves files behind.
> 
> The issue can be reproduced with BusyBox rm on a new hfsplus volume:
> 
>     mkdir /mnt/d
>     (cd /mnt/d && for i in $(seq 1500); do : > f$i; done)
>     rm -r /mnt/d

Frankly speaking, I don't quite follow what is the issue of the fix. Do
you mean that we create and delete folder in one thread and execute the
readdir in another one? Or do you mean that application has several
threads and one thread do readdir but another one delete file by file?
How this deletion is executed? I mean which files are deleted?

Even if we delete files in folder then i_size is dropping gradually. If
we traversing the folder's content and ctx->pos >= inode->i_size, then
it sounds that we already showed the existing content. Do you mean that
if we deleted the range that we already processed, then we miss the
rest of undeleted items?

> 
> Before commit 4b0496432844 ("hfsplus: rework hfsplus_readdir()
> logic"),
> the directory is removed. After that commit, the result is:
> 
>     rm: can't remove '/mnt/d': Directory not empty
> 
> and 304 files are left.
> 
> This patch will end a walk that continues from the saved key where
> the
> records of the directory end, and keep the i_size check for a walk
> that
> starts from a position.
> 
> Fixes: 4b0496432844 ("hfsplus: rework hfsplus_readdir() logic")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
> ---
> Application impact (Ubuntu 26.04 user space): a folder with 1500
> files
> on an HFS+ volume is moved to the trash, then the trash is emptied
> with
> "gio trash --empty" (the call behind "Empty Trash" in GNOME Files,
> done
> by gvfsd-trash 1.60). On v7.1, all files are removed. On v7.2, the
> trash
> looks empty, but 478 files stay in .Trash-0/expunged and their space
> is
> not freed.
> 
>  fs/hfsplus/dir.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
> index b3a1193a491f..81fe8a3e07c9 100644
> --- a/fs/hfsplus/dir.c
> +++ b/fs/hfsplus/dir.c
> @@ -135,10 +135,16 @@ static int hfsplus_readdir(struct file *file,
> struct dir_context *ctx)
>  	char *strbuf;
>  	hfsplus_cat_entry entry;
>  	struct hfs_find_data fd;
> -	struct hfsplus_readdir_data *rd;
> +	struct hfsplus_readdir_data *rd = file->private_data;
> +	/*
> +	 * i_size follows the valence and drops on unlink, so it
> says nothing
> +	 * about a walk that continues from a saved key. That walk
> ends where
> +	 * the records of this directory end.
> +	 */

I really dislike the comment in the variable declaration section.

> +	bool resume = rd && rd->pos == file->f_pos;

Is it possible that rd is NULL here?

I don't really follow how the resume variable can help at all.
Probably, I am missing the case. Could you please explain the issue at
first?

Thanks,
Slava.

>  	u16 type;
>  
> -	if (file->f_pos >= inode->i_size)
> +	if (!resume && file->f_pos >= inode->i_size)
>  		return 0;
>  
>  	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> @@ -183,10 +189,9 @@ static int hfsplus_readdir(struct file *file,
> struct dir_context *ctx)
>  			goto out;
>  		ctx->pos = 2;
>  	}
> -	if (ctx->pos >= inode->i_size)
> +	if (!resume && ctx->pos >= inode->i_size)
>  		goto out;
> -	rd = file->private_data;
> -	if (rd && rd->pos == ctx->pos) {
> +	if (resume) {
>  		memcpy(fd.search_key, &rd->key, sizeof(struct
> hfsplus_cat_key));
>  		err = hfs_brec_find(&fd, hfs_find_rec_by_key);
>  		if (err == -ENOENT)
> @@ -195,9 +200,11 @@ static int hfsplus_readdir(struct file *file,
> struct dir_context *ctx)
>  		err = hfs_brec_goto(&fd, ctx->pos - 1);
>  	}
>  	if (err)
> -		goto out;
> +		goto end_walk;
>  	for (;;) {
>  		if (be32_to_cpu(fd.key->cat.parent) != inode->i_ino)
> {
> +			if (resume)
> +				goto out;
>  			pr_err("walked past end of dir\n");
>  			err = -EIO;
>  			goto out;
> @@ -263,11 +270,11 @@ static int hfsplus_readdir(struct file *file,
> struct dir_context *ctx)
>  		}
>  next:
>  		ctx->pos++;
> -		if (ctx->pos >= inode->i_size)
> +		if (!resume && ctx->pos >= inode->i_size)
>  			goto out;
>  		err = hfs_brec_goto(&fd, 1);
>  		if (err)
> -			goto out;
> +			goto end_walk;
>  	}
>  	if (!rd) {
>  		rd = kmalloc_obj(struct hfsplus_readdir_data);
> @@ -279,6 +286,10 @@ static int hfsplus_readdir(struct file *file,
> struct dir_context *ctx)
>  	}
>  	rd->pos = ctx->pos;
>  	memcpy(&rd->key, fd.key, sizeof(struct hfsplus_cat_key));
> +end_walk:
> +	/* This directory can hold the last records of the catalog.
> */
> +	if (resume && err == -ENOENT)
> +		err = 0;
>  out:
>  	kfree(strbuf);
>  	hfs_find_exit(&fd);