Re: [PATCH 1/7] btrfs: Reorder btrfs_find_item arguments

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

On Thu, 2021-08-05 at 10:16 +0800, Qu Wenruo wrote:
> 
> On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote:
> > It's more natural do use objectid, type and offset, in this order,
> > when
> > dealing with btrfs keys.
> 
> I'm completely fine with this part.
> 
> > No functional changes.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >   fs/btrfs/backref.c | 9 ++++-----
> >   fs/btrfs/ctree.c   | 2 +-
> >   fs/btrfs/ctree.h   | 2 +-
> >   3 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> > index f735b8798ba1..9e92faaafa02 100644
> > --- a/fs/btrfs/backref.c
> > +++ b/fs/btrfs/backref.c
> > @@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root
> > *fs_root, struct btrfs_path *path,
> >   				btrfs_tree_read_unlock(eb);
> >   			free_extent_buffer(eb);
> >   		}
> > -		ret = btrfs_find_item(fs_root, path, parent, 0,
> > -				BTRFS_INODE_REF_KEY, &found_key);
> > +		ret = btrfs_find_item(fs_root, path, parent,
> > BTRFS_INODE_REF_KEY,
> > +					0, &found_key);
> >   		if (ret > 0)
> >   			ret = -ENOENT;
> >   		if (ret)
> > @@ -2063,9 +2063,8 @@ static int iterate_inode_refs(u64 inum,
> > struct btrfs_root *fs_root,
> >   	struct btrfs_key found_key;
> > 
> >   	while (!ret) {
> > -		ret = btrfs_find_item(fs_root, path, inum,
> > -				parent ? parent + 1 : 0,
> > BTRFS_INODE_REF_KEY,
> > -				&found_key);
> > +		ret = btrfs_find_item(fs_root, path, inum,
> > BTRFS_INODE_REF_KEY,
> > +				parent ? parent + 1 : 0, &found_key);
> > 
> >   		if (ret < 0)
> >   			break;
> > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> > index 84627cbd5b5b..c0002ec9c025 100644
> > --- a/fs/btrfs/ctree.c
> > +++ b/fs/btrfs/ctree.c
> > @@ -1528,7 +1528,7 @@ setup_nodes_for_search(struct
> > btrfs_trans_handle *trans,
> >   }
> > 
> >   int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path
> > *path,
> > -		u64 iobjectid, u64 ioff, u8 key_type,
> > +		u64 iobjectid, u8 key_type, u64 ioff,
> >   		struct btrfs_key *found_key)
> 
> But the @found_key part makes me wonder.
> 
> Is it really needed?
> 
> The caller has @path and return value. If we return 0, we know it's
> an
> exact match, no need to grab the key.
> If we return 1, caller can easily grab the key using @path (if they
> really need).
> 
> So can we also remove @found_key parameter, and add some comment on
> the
> function?

I believe that the function name is misleading. Maybe we can adjust it
to something like btrfs_find_item_offset, since it validates if the
found item has the same objectid and type of the searched key.

This is very common for a lot of the callers, which expect to receive
the same objectid and type, and each caller validate the offset as
required. Maybe we can add a comment and change the function name to
reflect all aspects of how it works. What do you think?

Thanks,
  Marcos

> 
> Thanks,
> Qu
> 
> >   {
> >   	int ret;
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index a898257ad2b5..0a971e98f5f9 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -2858,7 +2858,7 @@ int btrfs_duplicate_item(struct
> > btrfs_trans_handle *trans,
> >   			 struct btrfs_path *path,
> >   			 const struct btrfs_key *new_key);
> >   int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path
> > *path,
> > -		u64 inum, u64 ioff, u8 key_type, struct btrfs_key
> > *found_key);
> > +		u64 inum, u8 key_type, u64 ioff, struct btrfs_key
> > *found_key);
> >   int btrfs_search_slot(struct btrfs_trans_handle *trans, struct
> > btrfs_root *root,
> >   		      const struct btrfs_key *key, struct btrfs_path
> > *p,
> >   		      int ins_len, int cow);
> >