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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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?

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);
>