Re: [PATCH 06/13] btrfs-progs: btrfs_item_size_nr/btrfs_item_offset_nr everywhere

[Nikolay Borisov <nborisov@suse.com>]

On 9.03.22 г. 13:45 ч., Nikolay Borisov wrote:
> 
> 
> On 23.02.22 г. 0:26 ч., Josef Bacik wrote:
>> We have this pattern in a lot of places
>>
>>     item = btrfs_item_nr(slot);
>>     btrfs_item_size(leaf, item);
>>     btrfs_item_offset(leaf, item);
>>
>> when we could simply use
>>
>>     btrfs_item_size_nr(leaf, slot);
>>     btrfs_item_offset_nr(leaf, slot);
>>
>> Fix all callers of btrfs_item_size() and btrfs_item_offset() to use the
>> _nr variation of the helpers.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   check/main.c               |  8 ++++----
>>   image/main.c               |  2 +-
>>   kernel-shared/backref.c    |  4 +---
>>   kernel-shared/ctree.c      | 24 ++++++++++++------------
>>   kernel-shared/dir-item.c   |  6 ++----
>>   kernel-shared/inode-item.c |  4 +---
>>   kernel-shared/print-tree.c |  6 +++---
>>   7 files changed, 24 insertions(+), 30 deletions(-)
>>
>> diff --git a/check/main.c b/check/main.c
>> index e9ac94cc..76eb8d54 100644
>> --- a/check/main.c
>> +++ b/check/main.c
>> @@ -4222,10 +4222,10 @@ static int swap_values(struct btrfs_root 
>> *root, struct btrfs_path *path,
>>           item2 = btrfs_item_nr(slot + 1);
>>           btrfs_item_key_to_cpu(buf, &k1, slot);
>>           btrfs_item_key_to_cpu(buf, &k2, slot + 1);
>> -        item1_offset = btrfs_item_offset(buf, item1);
>> -        item2_offset = btrfs_item_offset(buf, item2);
>> -        item1_size = btrfs_item_size(buf, item1);
>> -        item2_size = btrfs_item_size(buf, item2);
>> +        item1_offset = btrfs_item_offset_nr(buf, slot);
>> +        item2_offset = btrfs_item_offset_nr(buf, slot + 1);
>> +        item1_size = btrfs_item_size_nr(buf, slot);
>> +        item2_size = btrfs_item_size_nr(buf, slot + 1);
>>           item1_data = malloc(item1_size);
>>           if (!item1_data)
>> diff --git a/image/main.c b/image/main.c
>> index 3125163d..e953d981 100644
>> --- a/image/main.c
>> +++ b/image/main.c
>> @@ -1239,7 +1239,7 @@ static void truncate_item(struct extent_buffer 
>> *eb, int slot, u32 new_size)
>>       for (i = slot; i < nritems; i++) {
>>           u32 ioff;
>>           item = btrfs_item_nr(i);
>> -        ioff = btrfs_item_offset(eb, item);
>> +        ioff = btrfs_item_offset_nr(eb, i);
>>           btrfs_set_item_offset(eb, item, ioff + size_diff);
>>       }
>> diff --git a/kernel-shared/backref.c b/kernel-shared/backref.c
>> index f1a638ed..327599b7 100644
>> --- a/kernel-shared/backref.c
>> +++ b/kernel-shared/backref.c
>> @@ -1417,7 +1417,6 @@ static int iterate_inode_refs(u64 inum, struct 
>> btrfs_root *fs_root,
>>       u64 parent = 0;
>>       int found = 0;
>>       struct extent_buffer *eb;
>> -    struct btrfs_item *item;
>>       struct btrfs_inode_ref *iref;
>>       struct btrfs_key found_key;
>> @@ -1442,10 +1441,9 @@ static int iterate_inode_refs(u64 inum, struct 
>> btrfs_root *fs_root,
>>           extent_buffer_get(eb);
>>           btrfs_release_path(path);
>> -        item = btrfs_item_nr(slot);
>>           iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
>> -        for (cur = 0; cur < btrfs_item_size(eb, item); cur += len) {
>> +        for (cur = 0; cur < btrfs_item_size_nr(eb, slot); cur += len) {
>>               name_len = btrfs_inode_ref_name_len(eb, iref);
>>               /* path must be released before calling iterate()! */
>>               pr_debug("following ref at offset %u for inode %llu in "
>> diff --git a/kernel-shared/ctree.c b/kernel-shared/ctree.c
>> index 10b22b2c..fc661aeb 100644
>> --- a/kernel-shared/ctree.c
>> +++ b/kernel-shared/ctree.c
>> @@ -2041,7 +2041,7 @@ static int push_leaf_right(struct 
>> btrfs_trans_handle *trans, struct btrfs_root
>>           if (path->slots[0] == i)
>>               push_space += data_size + sizeof(*item);
> 
> item is no longer used in this while so the assignment done above this 
> if can also be removed. Heck, I'd even go as far as moving the variable 
> declaration inside the for() loop further down and convert all the 
> sizeof(*item) calls to either sizeof(struct btrfs_item) or make a const 
> size_t item_size = sizeof(struct btrfs_item) atop the function and use 
> that.
>> -        this_item_size = btrfs_item_size(left, item);
>> +        this_item_size = btrfs_item_size_nr(left, i);
>>           if (this_item_size + sizeof(*item) + push_space > free_space)
>>               break;
>>           push_items++;
>> @@ -2092,7 +2092,7 @@ static int push_leaf_right(struct 
>> btrfs_trans_handle *trans, struct btrfs_root
>>       push_space = BTRFS_LEAF_DATA_SIZE(root->fs_info);
>>       for (i = 0; i < right_nritems; i++) {
>>           item = btrfs_item_nr(i);
> 
> Then this line could be turned into:
> struct btrfs_item item = btrfs_item_nr(i);
> 
>> -        push_space -= btrfs_item_size(right, item);
>> +        push_space -= btrfs_item_size_nr(right, i);
>>           btrfs_set_item_offset(right, item, push_space);
> 
> And item would be confined to the single site here.
> 
>>       }
>> @@ -2187,7 +2187,7 @@ static int push_leaf_left(struct 
>> btrfs_trans_handle *trans, struct btrfs_root
>>           if (path->slots[0] == i)
>>               push_space += data_size + sizeof(*item);
>> -        this_item_size = btrfs_item_size(right, item);
>> +        this_item_size = btrfs_item_size_nr(right, i);
>>           if (this_item_size + sizeof(*item) + push_space > free_space)
>>               break;
>> @@ -2224,7 +2224,7 @@ static int push_leaf_left(struct 
>> btrfs_trans_handle *trans, struct btrfs_root
>>           u32 ioff;
>>           item = btrfs_item_nr(i);
>> -        ioff = btrfs_item_offset(left, item);
>> +        ioff = btrfs_item_offset_nr(left, i);
>>           btrfs_set_item_offset(left, item,
>>                 ioff - (BTRFS_LEAF_DATA_SIZE(root->fs_info) -
>>                     old_left_item_size));
>> @@ -2256,7 +2256,7 @@ static int push_leaf_left(struct 
>> btrfs_trans_handle *trans, struct btrfs_root
>>       push_space = BTRFS_LEAF_DATA_SIZE(root->fs_info);
>>       for (i = 0; i < right_nritems; i++) {
>>           item = btrfs_item_nr(i);
>> -        push_space = push_space - btrfs_item_size(right, item);
>> +        push_space = push_space - btrfs_item_size_nr(right, i);
>>           btrfs_set_item_offset(right, item, push_space);
>>       }
> 
> nit: Same strategy can be applied to push_leaf_left as the one in 
> pusth_leaf_right
> 
> <snip>
> 
>> @@ -3029,7 +3029,7 @@ int btrfs_del_items(struct btrfs_trans_handle 
>> *trans, struct btrfs_root *root,
>>               u32 ioff;
>>               item = btrfs_item_nr(i);
> 
> nit: btrfs_item is used only in this loop so why not move it to reduce 
> the variable's scope.
> 
>> -            ioff = btrfs_item_offset(leaf, item);
>> +            ioff = btrfs_item_offset_nr(leaf, i);
>>               btrfs_set_item_offset(leaf, item, ioff + dsize);
>>           }
> 
> <snip>
> 

Oh right that's effectively implemented by the following patch so 
disregard this one ;)