On Wed 31-07-24 12:38:35, yangerkun wrote:
> After we switch tmpfs dir operations from simple_dir_operations to
> simple_offset_dir_operations, every rename happened will fill new dentry
> to dest dir's maple tree(&SHMEM_I(inode)->dir_offsets->mt) with a free
> key starting with octx->newx_offset, and then set newx_offset equals to
> free key + 1. This will lead to infinite readdir combine with rename
> happened at the same time, which fail generic/736 in xfstests(detail show
> as below).
>
> 1. create 5000 files(1 2 3...) under one dir
> 2. call readdir(man 3 readdir) once, and get one entry
> 3. rename(entry, "TEMPFILE"), then rename("TEMPFILE", entry)
> 4. loop 2~3, until readdir return nothing or we loop too many
> times(tmpfs break test with the second condition)
>
> We choose the same logic what commit 9b378f6ad48cf ("btrfs: fix infinite
> directory reads") to fix it, record the last_index when we open dir, and
> do not emit the entry which index >= last_index. The file->private_data
> now used in offset dir can use directly to do this, and we also update
> the last_index when we llseek the dir file.
The patch looks good! Just I'm not sure about the llseek part. As far as I
understand it was added due to this sentence in the standard:
"If a file is removed from or added to the directory after the most recent
call to opendir() or rewinddir(), whether a subsequent call to readdir()
returns an entry for that file is unspecified."
So if the offset used in offset_dir_llseek() is 0, then we should update
last_index. But otherwise I'd leave it alone because IMHO it would do more
harm than good.
Honza
>
> Fixes: a2e459555c5f ("shmem: stable directory offsets")
> Signed-off-by: yangerkun <yangerkun@xxxxxxxxxx>
> ---
> fs/libfs.c | 34 +++++++++++++++++++++++-----------
> 1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 8aa34870449f..38b306738c00 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -450,6 +450,14 @@ void simple_offset_destroy(struct offset_ctx *octx)
> mtree_destroy(&octx->mt);
> }
>
> +static int offset_dir_open(struct inode *inode, struct file *file)
> +{
> + struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
> +
> + file->private_data = (void *)ctx->next_offset;
> + return 0;
> +}
> +
> /**
> * offset_dir_llseek - Advance the read position of a directory descriptor
> * @file: an open directory whose position is to be updated
> @@ -463,6 +471,9 @@ void simple_offset_destroy(struct offset_ctx *octx)
> */
> static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
> {
> + struct inode *inode = file->f_inode;
> + struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
> +
> switch (whence) {
> case SEEK_CUR:
> offset += file->f_pos;
> @@ -476,7 +487,7 @@ static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
> }
>
> /* In this case, ->private_data is protected by f_pos_lock */
> - file->private_data = NULL;
> + file->private_data = (void *)ctx->next_offset;
> return vfs_setpos(file, offset, LONG_MAX);
> }
>
> @@ -507,7 +518,7 @@ static bool offset_dir_emit(struct dir_context *ctx, struct dentry *dentry)
> inode->i_ino, fs_umode_to_dtype(inode->i_mode));
> }
>
> -static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
> +static void offset_iterate_dir(struct inode *inode, struct dir_context *ctx, long last_index)
> {
> struct offset_ctx *octx = inode->i_op->get_offset_ctx(inode);
> struct dentry *dentry;
> @@ -515,17 +526,21 @@ static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
> while (true) {
> dentry = offset_find_next(octx, ctx->pos);
> if (!dentry)
> - return ERR_PTR(-ENOENT);
> + return;
> +
> + if (dentry2offset(dentry) >= last_index) {
> + dput(dentry);
> + return;
> + }
>
> if (!offset_dir_emit(ctx, dentry)) {
> dput(dentry);
> - break;
> + return;
> }
>
> ctx->pos = dentry2offset(dentry) + 1;
> dput(dentry);
> }
> - return NULL;
> }
>
> /**
> @@ -552,22 +567,19 @@ static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
> static int offset_readdir(struct file *file, struct dir_context *ctx)
> {
> struct dentry *dir = file->f_path.dentry;
> + long last_index = (long)file->private_data;
>
> lockdep_assert_held(&d_inode(dir)->i_rwsem);
>
> if (!dir_emit_dots(file, ctx))
> return 0;
>
> - /* In this case, ->private_data is protected by f_pos_lock */
> - if (ctx->pos == DIR_OFFSET_MIN)
> - file->private_data = NULL;
> - else if (file->private_data == ERR_PTR(-ENOENT))
> - return 0;
> - file->private_data = offset_iterate_dir(d_inode(dir), ctx);
> + offset_iterate_dir(d_inode(dir), ctx, last_index);
> return 0;
> }
>
> const struct file_operations simple_offset_dir_operations = {
> + .open = offset_dir_open,
> .llseek = offset_dir_llseek,
> .iterate_shared = offset_readdir,
> .read = generic_read_dir,
> --
> 2.39.2
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
