Re: [PATCH v3 03/11] btrfs: introduce new members for extent_map

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

在 2024/5/24 02:23, Filipe Manana 写道:
[...]
>> @@ -832,10 +897,11 @@ void btrfs_drop_extent_map_range(struct btrfs_inode *inode, u64 start, u64 end,
>>                                          split->orig_start = em->orig_start;
>>                                  }
>>                          } else {
>> +                               split->disk_num_bytes = 0;
>> +                               split->offset = 0;
>>                                  split->ram_bytes = split->len;
>>                                  split->orig_start = split->start;
>>                                  split->block_len = 0;
>> -                               split->disk_num_bytes = 0;
>
> Why move the assignment of ->disk_num_bytes ?
> This is sort of distracting, doing unnecessary changes.

It's to group the newer members together, and to follow the new trend to
put them in disk_bytenr disk_num_bytes offset ram_bytes order.

I know with structures, there is really no need to keep any order
between the member assignment, but with fixed ordering, it would be
better in the long run.

And unfortunately the cost is that, the first patch doing the
re-ordering the members would be harder to review.

>
>>                          }
>>
>>                          if (extent_map_in_tree(em)) {
>> @@ -989,10 +1055,12 @@ int split_extent_map(struct btrfs_inode *inode, u64 start, u64 len, u64 pre,
>>          /* First, replace the em with a new extent_map starting from * em->start */
>>          split_pre->start = em->start;
>>          split_pre->len = pre;
>> +       split_pre->disk_bytenr = new_logical;
>
> We are already setting disk_bytenr to the same value a few lines below.'

Sorry, I didn't see any location touching disk_bytenr, either inside the
patch, nor else where, especially the disk_bytenr is a new member.

>
>> +       split_pre->disk_num_bytes = split_pre->len;
>> +       split_pre->offset = 0;
>>          split_pre->orig_start = split_pre->start;
>>          split_pre->block_start = new_logical;
>>          split_pre->block_len = split_pre->len;
>> -       split_pre->disk_num_bytes = split_pre->block_len;
>
> Here, where slit_pre->block_len has the same value as split->pre_len.
> This sort of apparently accidental change makes it harder to review.

Again, to keep a consistent order of members.

>
>>          split_pre->ram_bytes = split_pre->len;
>>          split_pre->flags = flags;
>>          split_pre->generation = em->generation;
>> @@ -1007,10 +1075,12 @@ int split_extent_map(struct btrfs_inode *inode, u64 start, u64 len, u64 pre,
>>          /* Insert the middle extent_map. */
>>          split_mid->start = em->start + pre;
>>          split_mid->len = em->len - pre;
>> +       split_mid->disk_bytenr = em->block_start + pre;
>
> Same here.
>
>> +       split_mid->disk_num_bytes = split_mid->len;
>> +       split_mid->offset = 0;
>>          split_mid->orig_start = split_mid->start;
>>          split_mid->block_start = em->block_start + pre;
>>          split_mid->block_len = split_mid->len;
>> -       split_mid->disk_num_bytes = split_mid->block_len;
>
> Which relates to this.
>
> Otherwise it looks fine, and could be fixed up when cherry picked to for-next.

So although it's indeed harder to review, we would have a very
consistent order when assigning those members.

Thankfully this is only a one time pain, there should be no more member
order related problems.

Thanks,
Qu

>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>
> Thanks.