Re: [PATCH 1/4] btrfs: cleanup the bytenr usage inside btrfs_extent_item_to_extent_map()

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

在 2024/6/25 19:49, Filipe Manana 写道:
> On Tue, Jun 25, 2024 at 6:08 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> [PROBLEMS]
>
> I wouldn't call this "problems", there are no bugs here or anything harmful.
>
>> Before commit 85de2be7129c ("btrfs: remove extent_map::block_start
>> member"), we utilized @bytenr variable inside
>> btrfs_extent_item_to_extent_map() to calculate block_start.
>>
>> But that commit removed block_start completely, we have no need to
>> advance @bytenr at all.
>>
>> Furthermore with recent enhanced btrfs-progs check for ram_bytes
>> mimsatch, it turns out that for truncated ordered extents, their
>
> mimsatch -> mismatch
>
>> ram_bytes can be smaller than disk_num_bytes.
>>
>> [ENHANCEMENT]
>> Thankfully all above problems are not really going to affect end users,
>> fix them by:
>>
>> - Declare @bytenr only inside the if branch and make it const
>>    So we can remove the unnecessary advance of @bytenr.
>>
>> - Manually override extent_map::ram_bytes using disk_num_bytes
>>    This is for non-compressed regular/preallocated extents.
>
> I don't see anything in the patch changing ram_bytes.
> Perhaps this is from an early patch version never submitted, or from
> some other patch?

My bad, when re-editing the commit message, I got confused with later
patches.

I will remove the ram_bytes related part.

Thanks,
Qu
>
> The code itself looks good.
> Thanks.
>
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/file-item.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
>> index 55703c833f3d..2cc61c792ee6 100644
>> --- a/fs/btrfs/file-item.c
>> +++ b/fs/btrfs/file-item.c
>> @@ -1281,7 +1281,6 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
>>          const int slot = path->slots[0];
>>          struct btrfs_key key;
>>          u64 extent_start;
>> -       u64 bytenr;
>>          u8 type = btrfs_file_extent_type(leaf, fi);
>>          int compress_type = btrfs_file_extent_compression(leaf, fi);
>>
>> @@ -1291,22 +1290,22 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
>>          em->generation = btrfs_file_extent_generation(leaf, fi);
>>          if (type == BTRFS_FILE_EXTENT_REG ||
>>              type == BTRFS_FILE_EXTENT_PREALLOC) {
>> +               const u64 disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
>> +
>>                  em->start = extent_start;
>>                  em->len = btrfs_file_extent_end(path) - extent_start;
>> -               bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
>> -               if (bytenr == 0) {
>> +               if (disk_bytenr == 0) {
>>                          em->disk_bytenr = EXTENT_MAP_HOLE;
>>                          em->disk_num_bytes = 0;
>>                          em->offset = 0;
>>                          return;
>>                  }
>> -               em->disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
>> +               em->disk_bytenr = disk_bytenr;
>>                  em->disk_num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi);
>>                  em->offset = btrfs_file_extent_offset(leaf, fi);
>>                  if (compress_type != BTRFS_COMPRESS_NONE) {
>>                          extent_map_set_compression(em, compress_type);
>>                  } else {
>> -                       bytenr += btrfs_file_extent_offset(leaf, fi);
>>                          if (type == BTRFS_FILE_EXTENT_PREALLOC)
>>                                  em->flags |= EXTENT_FLAG_PREALLOC;
>>                  }
>> --
>> 2.45.2
>>
>>
>