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

Qu Wenruo
2024-03-19T21:25:12Z

在 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?

> 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;
> }
>
> /*