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

Sun YangKai
2025-08-22T13:08:43Z

> 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