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

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

On 2021/8/6 上午1:28, Marcos Paulo de Souza wrote:
> 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.

Then the parameter @offset makes no sense.

Since we have exact key, it's really intuitional to think we're
searching for a specific key, and under most cases we are.

>
> 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?

For such case, the caller doesn't have the full key, but are looking for
a key to meet certain key *range* requirement.

I believe that needs a new interface.

IMHO, we need a generic way to search for a key range (or doing
iteration for a key range), and a subset of above interface to just
search for a specific key (thus with much simpler interface).

Thanks,
Qu

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