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

[Josef Bacik <josef@toxicpanda.com>]

On Tue, Mar 19, 2024 at 08:25:20PM +0530, Anand Jain wrote:
> 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.
> 
> 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;
>  	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;
>  		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);

Ok so this is a correct conversion, but we'll lose "ret" here.  Can you follow
up with a different series to fix this?  I think we just say

free_extent_state(cached_state);
if (ret)
	break;

otherwise this patch looks fine, you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef