Re: [PATCH 7/7] btrfs: ioctl: Simplify btrfs_ioctl_get_subvol_info

[Marcos Paulo de Souza <mpdesouza@suse.de>]

On Thu, 2021-08-05 at 20:13 +0800, Anand Jain wrote:
> On 05/08/2021 02:48, Marcos Paulo de Souza wrote:
> > By using btrfs_find_item we can simplify the code.
> 
>   Yep. I like the idea.
> 
> > Also, remove the
> > -ENOENT error condition, since it'll never hit. If find_item
> > returns 0,
> > it means it found the desired objectid and type, so it won't reach
> > the -ENOENT
> > condition.
> > 
> > No functional changes.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
>   Looks good to me.
> 
>   Reviewed-by: Anand Jain <anand.jain@oracle.com>

Sorry, I believe that I did a mistake here.

See bellow.

> 
> Thanks, Anand
> 
> > ---
> >   fs/btrfs/ioctl.c | 56 +++++++++++++++++++----------------------
> > -------
> >   1 file changed, 22 insertions(+), 34 deletions(-)
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index d09eaa83b5d2..2c57bea16c92 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -2685,6 +2685,7 @@ static int btrfs_ioctl_get_subvol_info(struct
> > file *file, void __user *argp)
> >   	unsigned long item_off;
> >   	unsigned long item_len;
> >   	struct inode *inode;
> > +	u64 treeid;
> >   	int slot;
> >   	int ret = 0;
> >   
> > @@ -2702,15 +2703,15 @@ static int
> > btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
> >   	fs_info = BTRFS_I(inode)->root->fs_info;
> >   
> >   	/* Get root_item of inode's subvolume */
> > -	key.objectid = BTRFS_I(inode)->root->root_key.objectid;
> > -	root = btrfs_get_fs_root(fs_info, key.objectid, true);
> > +	treeid = BTRFS_I(inode)->root->root_key.objectid;
> > +	root = btrfs_get_fs_root(fs_info, treeid, true);
> >   	if (IS_ERR(root)) {
> >   		ret = PTR_ERR(root);
> >   		goto out_free;
> >   	}
> >   	root_item = &root->root_item;
> >   
> > -	subvol_info->treeid = key.objectid;
> > +	subvol_info->treeid = treeid;
> >   
> >   	subvol_info->generation = btrfs_root_generation(root_item);
> >   	subvol_info->flags = btrfs_root_flags(root_item);
> > @@ -2737,44 +2738,31 @@ static int
> > btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
> >   	subvol_info->rtime.sec = btrfs_stack_timespec_sec(&root_item-
> > >rtime);
> >   	subvol_info->rtime.nsec = btrfs_stack_timespec_nsec(&root_item-
> > >rtime);
> >   
> > -	if (key.objectid != BTRFS_FS_TREE_OBJECTID) {
> > +	if (treeid != BTRFS_FS_TREE_OBJECTID) {
> >   		/* Search root tree for ROOT_BACKREF of this subvolume
> > */
> > -		key.type = BTRFS_ROOT_BACKREF_KEY;
> > -		key.offset = 0;
> > -		ret = btrfs_search_slot(NULL, fs_info->tree_root, &key,
> > path, 0, 0);
> > +		ret = btrfs_find_item(fs_info->tree_root, path, treeid,
> > +					BTRFS_ROOT_BACKREF_KEY, 0,
> > &key);
> >   		if (ret < 0) {
> >   			goto out;
> > -		} else if (path->slots[0] >=
> > -			   btrfs_header_nritems(path->nodes[0])) {
> > -			ret = btrfs_next_leaf(fs_info->tree_root,
> > path);
> > -			if (ret < 0) {
> > -				goto out;
> > -			} else if (ret > 0) {
> > -				ret = -EUCLEAN;
> > -				goto out;
> > -			}

btrfs_next_leaf returns > 0 if it there aren't any other leaf. So, in
this case, it can find another leaf.

> > +		} else if (ret > 0) {
> > +			ret = -EUCLEAN;
> > +			goto out;
> >   		}

This is wrong, since btrfs_find_item can return 1 is next_leaf returned
something different OR if the objectid or type aren't the same.

In this case, the -ENOENT path can be reached, since there can be more
leaves but with different objectid and type.

In this case, with the following change, we would report such situation
wrongly as -EUCLEAN.

I'll rework this patch in the v2 of this patchset.

> >   
> >   		leaf = path->nodes[0];
> >   		slot = path->slots[0];
> > -		btrfs_item_key_to_cpu(leaf, &key, slot);
> > -		if (key.objectid == subvol_info->treeid &&
> > -		    key.type == BTRFS_ROOT_BACKREF_KEY) {
> > -			subvol_info->parent_id = key.offset;
> > -
> > -			rref = btrfs_item_ptr(leaf, slot, struct
> > btrfs_root_ref);
> > -			subvol_info->dirid = btrfs_root_ref_dirid(leaf,
> > rref);
> > -
> > -			item_off = btrfs_item_ptr_offset(leaf, slot)
> > -					+ sizeof(struct
> > btrfs_root_ref);
> > -			item_len = btrfs_item_size_nr(leaf, slot)
> > -					- sizeof(struct
> > btrfs_root_ref);
> > -			read_extent_buffer(leaf, subvol_info->name,
> > -					   item_off, item_len);
> > -		} else {
> > -			ret = -ENOENT;
> > -			goto out;
> > -		}
> > +
> > +		subvol_info->parent_id = key.offset;
> > +
> > +		rref = btrfs_item_ptr(leaf, slot, struct
> > btrfs_root_ref);
> > +		subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
> > +
> > +		item_off = btrfs_item_ptr_offset(leaf, slot)
> > +				+ sizeof(struct btrfs_root_ref);
> > +		item_len = btrfs_item_size_nr(leaf, slot)
> > +				- sizeof(struct btrfs_root_ref);
> > +		read_extent_buffer(leaf, subvol_info->name,
> > +				   item_off, item_len);
> >   	}
> >   
> >   	if (copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
> >