Re: [PATCH v2 11/11] btrfs: cleanup duplicated parameters related to btrfs_create_dio_extent()

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

在 2024/5/21 02:18, Filipe Manana 写道:
> On Fri, May 3, 2024 at 7:03 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> The following 3 parameters can be cleaned up using btrfs_file_extent
>> structure:
>>
>> - len
>>    btrfs_file_extent::num_bytes
>>
>> - orig_block_len
>>    btrfs_file_extent::disk_num_bytes
>>
>> - ram_bytes
>>    btrfs_file_extent::ram_bytes
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/inode.c | 22 ++++++++--------------
>>   1 file changed, 8 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index a95dc2333972..09974c86d3d1 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -6969,11 +6969,8 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>>   static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
>>                                                    struct btrfs_dio_data *dio_data,
>>                                                    const u64 start,
>> -                                                 const u64 len,
>> -                                                 const u64 orig_block_len,
>> -                                                 const u64 ram_bytes,
>> -                                                 const int type,
>> -                                                 struct btrfs_file_extent *file_extent)
>> +                                                 struct btrfs_file_extent *file_extent,
>> +                                                 const int type)
>>   {
>>          struct extent_map *em = NULL;
>>          struct btrfs_ordered_extent *ordered;
>> @@ -6991,7 +6988,7 @@ static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
>>                  if (em) {
>>                          free_extent_map(em);
>>                          btrfs_drop_extent_map_range(inode, start,
>> -                                                   start + len - 1, false);
>> +                                       start + file_extent->num_bytes - 1, false);
>>                  }
>>                  em = ERR_CAST(ordered);
>>          } else {
>> @@ -7034,10 +7031,9 @@ static struct extent_map *btrfs_new_extent_direct(struct btrfs_inode *inode,
>>          file_extent.ram_bytes = ins.offset;
>>          file_extent.offset = 0;
>>          file_extent.compression = BTRFS_COMPRESS_NONE;
>> -       em = btrfs_create_dio_extent(inode, dio_data, start, ins.offset,
>> -                                    ins.offset,
>> -                                    ins.offset, BTRFS_ORDERED_REGULAR,
>> -                                    &file_extent);
>> +       em = btrfs_create_dio_extent(inode, dio_data, start,
>> +                                    &file_extent,
>> +                                    BTRFS_ORDERED_REGULAR);
>
> As we're changing this, we can leave this in a single line as it fits.
>
>>          btrfs_dec_block_group_reservations(fs_info, ins.objectid);
>>          if (IS_ERR(em))
>>                  btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset,
>> @@ -7404,10 +7400,8 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
>>                  }
>>                  space_reserved = true;
>>
>> -               em2 = btrfs_create_dio_extent(BTRFS_I(inode), dio_data, start, len,
>> -                                             file_extent.disk_num_bytes,
>> -                                             file_extent.ram_bytes, type,
>> -                                             &file_extent);
>> +               em2 = btrfs_create_dio_extent(BTRFS_I(inode), dio_data, start,
>> +                                             &file_extent, type);
>
> Same here.

Just a small question related to the single line one.

The parameter @start with its tailing ',' is already at 80 chars,
do we still need to follow the old 80 chars width recommendation?

With previous several patches, I re-checked the lines, some can indeed
be improved a little, but some BTRFS_ORDERED_* flags can not be merged
without exceeding the 80 chars limits.

Thanks,
Qu
>
> The rest looks good, thanks.
>
>>                  btrfs_dec_nocow_writers(bg);
>>                  if (type == BTRFS_ORDERED_PREALLOC) {
>>                          free_extent_map(em);
>> --
>> 2.45.0
>>
>>
>