Re: [PATCH v2 10/11] btrfs: btrfs_drop_snapshot optimize return variable

[David Sterba <dsterba@suse.cz>]

On Thu, Apr 18, 2024 at 03:08:42PM +0800, Anand Jain wrote:
> Drop the variable 'err', reuse the variable 'ret' by reinitializing it to
> zero where necessary.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: handle return error better, no need of original 'ret'. (Josef).
> 
>  fs/btrfs/extent-tree.c | 48 +++++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 78dc94a97e35..17aa45b906bb 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -5833,8 +5833,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  	struct btrfs_root_item *root_item = &root->root_item;
>  	struct walk_control *wc;
>  	struct btrfs_key key;
> -	int err = 0;
> -	int ret;
> +	int ret = 0;
>  	int level;
>  	bool root_dropped = false;
>  	bool unfinished_drop = false;
> @@ -5843,14 +5842,14 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  
>  	path = btrfs_alloc_path();
>  	if (!path) {
> -		err = -ENOMEM;
> +		ret = -ENOMEM;
>  		goto out;
>  	}
>  
>  	wc = kzalloc(sizeof(*wc), GFP_NOFS);
>  	if (!wc) {
>  		btrfs_free_path(path);
> -		err = -ENOMEM;
> +		ret = -ENOMEM;
>  		goto out;
>  	}
>  
> @@ -5863,12 +5862,12 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  	else
>  		trans = btrfs_start_transaction(tree_root, 0);
>  	if (IS_ERR(trans)) {
> -		err = PTR_ERR(trans);
> +		ret = PTR_ERR(trans);
>  		goto out_free;
>  	}
>  
> -	err = btrfs_run_delayed_items(trans);
> -	if (err)
> +	ret = btrfs_run_delayed_items(trans);
> +	if (ret)
>  		goto out_end_trans;
>  
>  	/*
> @@ -5899,11 +5898,11 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  		path->lowest_level = level;
>  		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
>  		path->lowest_level = 0;
> -		if (ret < 0) {
> -			err = ret;
> +		if (ret < 0)
>  			goto out_end_trans;
> -		}
> +
>  		WARN_ON(ret > 0);
> +		ret = 0;
>  
>  		/*
>  		 * unlock our path, this is safe because only this
> @@ -5916,14 +5915,17 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  			btrfs_tree_lock(path->nodes[level]);
>  			path->locks[level] = BTRFS_WRITE_LOCK;
>  
> +			/*
> +			 * btrfs_lookup_extent_info() returns 0 for success,
> +			 * or < 0 for error.
> +			 */
>  			ret = btrfs_lookup_extent_info(trans, fs_info,
>  						path->nodes[level]->start,
>  						level, 1, &wc->refs[level],
>  						&wc->flags[level], NULL);
> -			if (ret < 0) {
> -				err = ret;
> +			if (ret < 0)
>  				goto out_end_trans;
> -			}
> +
>  			BUG_ON(wc->refs[level] == 0);
>  
>  			if (level == btrfs_root_drop_level(root_item))
> @@ -5949,19 +5951,18 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  		ret = walk_down_tree(trans, root, path, wc);
>  		if (ret < 0) {
>  			btrfs_abort_transaction(trans, ret);
> -			err = ret;
>  			break;
>  		}
>  
>  		ret = walk_up_tree(trans, root, path, wc, BTRFS_MAX_LEVEL);
>  		if (ret < 0) {
>  			btrfs_abort_transaction(trans, ret);
> -			err = ret;
>  			break;
>  		}
>  
>  		if (ret > 0) {
>  			BUG_ON(wc->stage != DROP_REFERENCE);
> +			ret = 0;
>  			break;
>  		}
>  
> @@ -5983,7 +5984,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  						root_item);
>  			if (ret) {
>  				btrfs_abort_transaction(trans, ret);
> -				err = ret;
>  				goto out_end_trans;
>  			}
>  
> @@ -5994,7 +5994,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  			if (!for_reloc && btrfs_need_cleaner_sleep(fs_info)) {
>  				btrfs_debug(fs_info,
>  					    "drop snapshot early exit");
> -				err = -EAGAIN;
> +				ret = -EAGAIN;
>  				goto out_free;
>  			}
>  
> @@ -6008,19 +6008,18 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  			else
>  				trans = btrfs_start_transaction(tree_root, 0);
>  			if (IS_ERR(trans)) {
> -				err = PTR_ERR(trans);
> +				ret = PTR_ERR(trans);
>  				goto out_free;
>  			}
>  		}
>  	}
>  	btrfs_release_path(path);
> -	if (err)
> +	if (ret)
>  		goto out_end_trans;
>  
>  	ret = btrfs_del_root(trans, &root->root_key);
>  	if (ret) {
>  		btrfs_abort_transaction(trans, ret);
> -		err = ret;
>  		goto out_end_trans;
>  	}
>  
> @@ -6029,10 +6028,11 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  				      NULL, NULL);
>  		if (ret < 0) {
>  			btrfs_abort_transaction(trans, ret);
> -			err = ret;
>  			goto out_end_trans;
>  		} else if (ret > 0) {
> -			/* if we fail to delete the orphan item this time
> +			ret = 0;
> +			/*
> +			 * if we fail to delete the orphan item this time

If you change comments please also fix the formatting or spelling, in
this case first letter is uppercase.

>  			 * around, it'll get picked up the next time.
>  			 *
>  			 * The most common failure here is just -ENOENT.
> @@ -6067,7 +6067,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  	 * We were an unfinished drop root, check to see if there are any
>  	 * pending, and if not clear and wake up any waiters.
>  	 */
> -	if (!err && unfinished_drop)
> +	if (!ret && unfinished_drop)
>  		btrfs_maybe_wake_unfinished_drop(fs_info);
>  
>  	/*
> @@ -6079,7 +6079,7 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
>  	 */
>  	if (!for_reloc && !root_dropped)
>  		btrfs_add_dead_root(root);
> -	return err;
> +	return ret;
>  }
>  
>  /*
> -- 
> 2.41.0
>