| Message ID | 20210804184854.10696-8-mpdesouza@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Use btrfs_find_item whenever possible | expand |
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> 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; > - } > + } else if (ret > 0) { > + ret = -EUCLEAN; > + goto out; > } > > 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))) >
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))) > >
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; - } + } else if (ret > 0) { + ret = -EUCLEAN; + goto out; } 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)))
By using btrfs_find_item we can simplify the code. 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> --- fs/btrfs/ioctl.c | 56 +++++++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 34 deletions(-)