Re: [PATCH 12/29] btrfs: btrfs_write_marked_extents rename werr to ret err to ret2

Anand Jain
2024-04-16T03:18:29Z

On 3/20/24 05:25, Qu Wenruo wrote:
>
>
> 在 2024/3/20 01:25, Anand Jain 写道:
>> Rename the function's local variable werr to ret and err to ret2, then
>> move ret2 to the local variable of the while loop. Drop the
>> initialization
>> there since it's immediately assigned below.
>
> I love the local return value inside the loop, but I still think we can
> do further improvement.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/transaction.c | 22 +++++++++++-----------
>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index feffff91c6fe..167893457b58 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -1119,8 +1119,7 @@ int btrfs_end_transaction_throttle(struct
>> btrfs_trans_handle *trans)
>>   int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info,
>>                      struct extent_io_tree *dirty_pages, int mark)
>>   {
>> -    int err = 0;
>> -    int werr = 0;
>> +    int ret = 0;
>
> Can we rename it to something like global_ret or whatever can indicate
> that variable is our final result?

@ret is the actual function return variable and is consistent with doc:

https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#code

Thanks, Anand

>
>
>>       struct address_space *mapping = fs_info->btree_inode->i_mapping;
>>       struct extent_state *cached_state = NULL;
>>       u64 start = 0;
>> @@ -1128,9 +1127,10 @@ int btrfs_write_marked_extents(struct
>> btrfs_fs_info *fs_info,
>>
>>       while (find_first_extent_bit(dirty_pages, start, &start, &end,
>>                        mark, &cached_state)) {
>> +        int ret2;
>
> And @ret inside the loop.
>
>>           bool wait_writeback = false;
>>
>> -        err = convert_extent_bit(dirty_pages, start, end,
>> +        ret2 = convert_extent_bit(dirty_pages, start, end,
>>                        EXTENT_NEED_WAIT,
>>                        mark, &cached_state);
>>           /*
>> @@ -1146,22 +1146,22 @@ int btrfs_write_marked_extents(struct
>> btrfs_fs_info *fs_info,
>>            * We cleanup any entries left in the io tree when committing
>>            * the transaction (through extent_io_tree_release()).
>>            */
>> -        if (err == -ENOMEM) {
>> -            err = 0;
>> +        if (ret2 == -ENOMEM) {
>> +            ret2 = 0;
>>               wait_writeback = true;
>>           }
>> -        if (!err)
>> -            err = filemap_fdatawrite_range(mapping, start, end);
>> -        if (err)
>> -            werr = err;
>> +        if (!ret2)
>> +            ret2 = filemap_fdatawrite_range(mapping, start, end);
>> +        if (ret2)
>> +            ret = ret2;
>>           else if (wait_writeback)
>> -            werr = filemap_fdatawait_range(mapping, start, end);
>> +            ret = filemap_fdatawait_range(mapping, start, end);
>
> This does not follow the regular update sequence, we always update the
> local one @ret2/@err, but here we directly update the @ret/@werr.
>
> Just for the sake of consistency, can we only use @ret2 for all the
> functions calls?
>
>>           free_extent_state(cached_state);
>>           cached_state = NULL;
>>           cond_resched();
>>           start = end + 1;
>
> Another thing is, we can move most of the writeback code (aka code
> inside the loop) into a helper function, and make things much simpler at
> least for the caller.
>
> {
>     int global_ret = 0;
>
>        while (find_first_extent_bit(dirty_pages, start, &start, &end,
>                         mark, &cached_state)) {
>         int ret;
>
>         ret = writeback_range();
>         if (ret < 0)
>             global_ret = ret;
>         cond_resched();
>         start = end + 1
>     }
>     return global_ret;
> }
>
> Thanks,
> Qu
>>       }
>> -    return werr;
>> +    return ret;
>>   }
>>
>>   /*