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

David Sterba
2025-08-22T13:01:34Z

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.