On Tue 13-02-24 16:38:08, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> Liam says that, unlike with xarray, once the RCU read lock is
> released ma_state is not safe to re-use for the next mas_find() call.
> But the RCU read lock has to be released on each loop iteration so
> that dput() can be called safely.
>
> Thus we are forced to walk the offset tree with fresh state for each
> directory entry. mt_find() can do this for us, though it might be a
> little less efficient than maintaining ma_state locally.
>
> Since offset_iterate_dir() doesn't build ma_state locally any more,
> there's no longer a strong need for offset_find_next(). Clean up by
> rolling these two helpers together.
>
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
Well, in general I think even xas_next_entry() is not safe to use how
offset_find_next() was using it. Once you drop rcu_read_lock(),
xas->xa_node could go stale. But since you're holding inode->i_rwsem when
using offset_find_next() you should be protected from concurrent
modifications of the mapping (whatever the underlying data structure is) -
that's what makes xas_next_entry() safe AFAIU. Isn't that enough for the
maple tree? Am I missing something?
Honza
> ---
> fs/libfs.c | 39 +++++++++++++++++----------------------
> 1 file changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index f073e9aeb2bf..6e01fde1cf95 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -436,23 +436,6 @@ static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
> return vfs_setpos(file, offset, MAX_LFS_FILESIZE);
> }
>
> -static struct dentry *offset_find_next(struct ma_state *mas)
> -{
> - struct dentry *child, *found = NULL;
> -
> - rcu_read_lock();
> - child = mas_find(mas, ULONG_MAX);
> - if (!child)
> - goto out;
> - spin_lock(&child->d_lock);
> - if (simple_positive(child))
> - found = dget_dlock(child);
> - spin_unlock(&child->d_lock);
> -out:
> - rcu_read_unlock();
> - return found;
> -}
> -
> static bool offset_dir_emit(struct dir_context *ctx, struct dentry *dentry)
> {
> unsigned long offset = dentry2offset(dentry);
> @@ -465,13 +448,22 @@ static bool offset_dir_emit(struct dir_context *ctx, struct dentry *dentry)
> static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
> {
> struct offset_ctx *octx = inode->i_op->get_offset_ctx(inode);
> - MA_STATE(mas, &octx->mt, ctx->pos, ctx->pos);
> - struct dentry *dentry;
> + struct dentry *dentry, *found;
> + unsigned long offset;
>
> + offset = ctx->pos;
> while (true) {
> - dentry = offset_find_next(&mas);
> + found = mt_find(&octx->mt, &offset, ULONG_MAX);
> + if (!found)
> + goto out_noent;
> +
> + dentry = NULL;
> + spin_lock(&found->d_lock);
> + if (simple_positive(found))
> + dentry = dget_dlock(found);
> + spin_unlock(&found->d_lock);
> if (!dentry)
> - return ERR_PTR(-ENOENT);
> + goto out_noent;
>
> if (!offset_dir_emit(ctx, dentry)) {
> dput(dentry);
> @@ -479,9 +471,12 @@ static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
> }
>
> dput(dentry);
> - ctx->pos = mas.index + 1;
> + ctx->pos = offset;
> }
> return NULL;
> +
> +out_noent:
> + return ERR_PTR(-ENOENT);
> }
>
> /**
>
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
