Re: [PATCH 2/2] btrfs: more trivial BTRFS_PATH_AUTO_FREE conversions

[Sun YangKai <sunk67188@gmail.com>]

> On Thu, Aug 21, 2025 at 09:12:24PM +0800, Sun YangKai wrote:
> > Trival pattern for the auto freeing with goto -> return convertions
> > if possible. No other function cleanup.
> 
> Not all the changes match the trivial pattern described in the patches.
> 
> > Signed-off-by: Sun YangKai <sunk67188@gmail.com>
> > ---
> > 
> >  fs/btrfs/raid-stripe-tree.c |  16 +-
> >  fs/btrfs/ref-verify.c       |   3 +-
> >  fs/btrfs/reflink.c          |   3 +-
> >  fs/btrfs/relocation.c       |  66 +++-----
> >  fs/btrfs/root-tree.c        |  49 +++---
> >  fs/btrfs/scrub.c            |  11 +-
> >  fs/btrfs/send.c             | 307 +++++++++++++-----------------------
> >  fs/btrfs/super.c            |   9 +-
> >  fs/btrfs/tree-log.c         | 124 +++++----------
> >  9 files changed, 204 insertions(+), 384 deletions(-)
> > 
> > diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> > index cab0b291088c..231107cafab2 100644
> > --- a/fs/btrfs/raid-stripe-tree.c
> > +++ b/fs/btrfs/raid-stripe-tree.c
> > @@ -67,7 +67,7 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle
> > *trans, u64 start, u64 le> 
> >  {
> >  
> >  	struct btrfs_fs_info *fs_info = trans->fs_info;
> >  	struct btrfs_root *stripe_root = fs_info->stripe_root;
> > 
> > -	struct btrfs_path *path;
> > +	BTRFS_PATH_AUTO_FREE(path);
> > 
> >  	struct btrfs_key key;
> >  	struct extent_buffer *leaf;
> >  	u64 found_start;
> > 
> > @@ -260,7 +260,6 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle
> > *trans, u64 start, u64 le> 
> >  		btrfs_release_path(path);
> >  	
> >  	}
> > 
> > -	btrfs_free_path(path);
> > 
> >  	return ret;
> >  
> >  }
> 
> As an example, this is the pattern, just declare, allocate and free at
> the end.
> 
> > @@ -376,7 +374,7 @@ int btrfs_get_raid_extent_offset(struct btrfs_fs_info
> > *fs_info,> 
> >  	struct btrfs_stripe_extent *stripe_extent;
> >  	struct btrfs_key stripe_key;
> >  	struct btrfs_key found_key;
> > 
> > -	struct btrfs_path *path;
> > +	BTRFS_PATH_AUTO_FREE(path);
> > 
> >  	struct extent_buffer *leaf;
> >  	const u64 end = logical + *length;
> >  	int num_stripes;
> > 
> > @@ -402,7 +400,7 @@ int btrfs_get_raid_extent_offset(struct btrfs_fs_info
> > *fs_info,> 
> >  	ret = btrfs_search_slot(NULL, stripe_root, &stripe_key, path, 0, 0);
> >  	if (ret < 0)
> > 
> > -		goto free_path;
> > +		return ret;
> > 
> >  	if (ret) {
> >  	
> >  		if (path->slots[0] != 0)
> >  		
> >  			path->slots[0]--;
> > 
> > @@ -459,8 +457,7 @@ int btrfs_get_raid_extent_offset(struct btrfs_fs_info
> > *fs_info,> 
> >  		trace_btrfs_get_raid_extent_offset(fs_info, logical, *length,
> >  		
> >  						   stripe->physical, devid);
> > 
> > -		ret = 0;
> > -		goto free_path;
> > +		return 0;
> > 
> >  	}
> >  	
> >  	/* If we're here, we haven't found the requested devid in the stripe. 
*/
> > 
> > @@ -474,8 +471,5 @@ int btrfs_get_raid_extent_offset(struct btrfs_fs_info
> > *fs_info,> 
> >  			  logical, logical + *length, stripe->dev->devid,
> >  			  btrfs_bg_type_to_raid_name(map_type));
> >  	
> >  	}
> > 
> > -free_path:
> > -	btrfs_free_path(path);
> > -
> > 
> >  	return ret;
> >  
> >  }
> 
> This is also still trivial with the goto/return conversion as long as
> there's not additional logic around that.
> 
> > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> > index 7256f6748c8f..8b08d6e4cb2b 100644
> > --- a/fs/btrfs/relocation.c
> > +++ b/fs/btrfs/relocation.c
> > @@ -409,7 +409,7 @@ static noinline_for_stack struct btrfs_backref_node
> > *build_backref_tree(> 
> >  	struct btrfs_backref_iter *iter;
> >  	struct btrfs_backref_cache *cache = &rc->backref_cache;
> >  	/* For searching parent of TREE_BLOCK_REF */
> > 
> > -	struct btrfs_path *path;
> > +	BTRFS_PATH_AUTO_FREE(path);
> > 
> >  	struct btrfs_backref_node *cur;
> >  	struct btrfs_backref_node *node = NULL;
> >  	struct btrfs_backref_edge *edge;
> > 
> > @@ -461,7 +461,6 @@ static noinline_for_stack struct btrfs_backref_node
> > *build_backref_tree(> 
> >  out:
> >  	btrfs_free_path(iter->path);
> >  	kfree(iter);
> > 
> > -	btrfs_free_path(path);
> 
> In this case it depends, there are a few more statements after the
> freeing which means the path is still allocated until the function ends.
> If the statements are something quick, like in this case then it's still
> OK and considered trivial.
> 
> >  	if (ret) {
> >  	
> >  		btrfs_backref_error_cleanup(cache, node);
> >  		return ERR_PTR(ret);
> > 
> > @@ -1661,8 +1651,6 @@ static noinline_for_stack int
> > merge_reloc_root(struct reloc_control *rc,> 
> >  	btrfs_tree_unlock(leaf);
> >  	free_extent_buffer(leaf);
> >  
> >  out:
> > -	btrfs_free_path(path);
> 
> Similar to the previous one but is not trivial, due to calls to
> insert_dirty_subvol(), btrfs_end_transaction_throttle(),
> btrfs_btree_balance_dirty() and invalidate_extent_cache(). Please have a
> look what the functions do.
> 
> > -
> > 
> >  	if (ret == 0) {
> >  	
> >  		ret = insert_dirty_subvol(trans, rc, root);
> >  		if (ret)
> > 
> > @@ -4080,7 +4058,7 @@ int btrfs_recover_relocation(struct btrfs_fs_info
> > *fs_info)> 
> >  	struct btrfs_key key;
> >  	struct btrfs_root *fs_root;
> >  	struct btrfs_root *reloc_root;
> > 
> > -	struct btrfs_path *path;
> > +	BTRFS_PATH_AUTO_FREE(path);
> > 
> >  	struct extent_buffer *leaf;
> >  	struct reloc_control *rc = NULL;
> >  	struct btrfs_trans_handle *trans;
> > 
> > @@ -4229,8 +4207,6 @@ int btrfs_recover_relocation(struct btrfs_fs_info
> > *fs_info)> 
> >  out:
> >  	free_reloc_roots(&reloc_roots);
> > 
> > -	btrfs_free_path(path);
> 
> Same non-trivial case, there's btrfs_orphan_cleanup() after that.
> 
> > -
> > 
> >  	if (ret == 0) {
> >  	
> >  		/* cleanup orphan inode in data relocation tree */
> >  		fs_root = btrfs_grab_root(fs_info->data_reloc_root);
> 
> Please split the patch to parts that have the described trivial changes,
> and then one patch per function in case it's not trivial and needs some
> adjustments.
> 
> The freeing followed by other code can be still converted to auto
> cleaning but there must be an explicit path = NULL after the free.

Got it. Thank you for your patience and kind reply. I'll go over those changes 
again.

Best regards,
Sun YangKai