Re: [PATCH 7/8] btrfs: relocation: use on-stack iterator in build_backref_tree

[Filipe Manana <fdmanana@kernel.org>]

On Fri, Sep 22, 2023 at 2:26 PM David Sterba <dsterba@suse.com> wrote:
>
> build_backref_tree() is called in a loop by relocate_tree_blocks()
> for each relocated block. The iterator is allocated and freed repeatedly
> while we could simply use an on-stack variable to avoid the allocation
> and remove one more failure case. The stack grows by 48 bytes.
>
> This was the only use of btrfs_backref_iter_alloc() so it's changed to
> be an initializer and btrfs_backref_iter_free() can be removed
> completely.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/backref.c    | 26 ++++++++++----------------
>  fs/btrfs/backref.h    | 11 ++---------
>  fs/btrfs/relocation.c | 12 ++++++------
>  3 files changed, 18 insertions(+), 31 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 0dc91bf654b5..691b20b47065 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -2828,26 +2828,20 @@ void free_ipath(struct inode_fs_paths *ipath)
>         kfree(ipath);
>  }
>
> -struct btrfs_backref_iter *btrfs_backref_iter_alloc(struct btrfs_fs_info *fs_info)
> +int btrfs_backref_iter_init(struct btrfs_fs_info *fs_info,
> +                           struct btrfs_backref_iter *iter)
>  {
> -       struct btrfs_backref_iter *ret;
> -
> -       ret = kzalloc(sizeof(*ret), GFP_NOFS);
> -       if (!ret)
> -               return NULL;
> -
> -       ret->path = btrfs_alloc_path();
> -       if (!ret->path) {
> -               kfree(ret);
> -               return NULL;
> -       }
> +       memset(iter, 0, sizeof(struct btrfs_backref_iter));
> +       iter->path = btrfs_alloc_path();

So this is breaking misc-next, as paths are leaking here, easily
visible after "rmmod btrfs":

[ 2265.115295] =============================================================================
[ 2265.115938] BUG btrfs_path (Not tainted): Objects remaining in
btrfs_path on __kmem_cache_shutdown()
[ 2265.116615] -----------------------------------------------------------------------------

[ 2265.117614] Slab 0x00000000dbb6fd30 objects=36 used=3
fp=0x000000001768ab21
flags=0x17fffc000000800(slab|node=0|zone=2|lastcpupid=0x1ffff)
[ 2265.118423] CPU: 1 PID: 402761 Comm: rmmod Not tainted
6.6.0-rc3-btrfs-next-139+ #1
[ 2265.118440] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.16.2-0-gea1b7a073390-prebuilt.qemu.org 04/01/2014
[ 2265.118457] Call Trace:
[ 2265.118483]  <TASK>
[ 2265.118491]  dump_stack_lvl+0x44/0x60
[ 2265.118521]  slab_err+0xb6/0xf0
[ 2265.118547]  __kmem_cache_shutdown+0x15f/0x2f0
[ 2265.118565]  kmem_cache_destroy+0x4c/0x170
[ 2265.118588]  exit_btrfs_fs+0x24/0x40 [btrfs]
[ 2265.119121]  __x64_sys_delete_module+0x193/0x290
[ 2265.119137]  ? exit_to_user_mode_prepare+0x3d/0x170
[ 2265.119154]  do_syscall_64+0x38/0x90
[ 2265.119169]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
[ 2265.119185] RIP: 0033:0x7f55ec127997
[ 2265.119199] Code: 73 01 c3 48 8b 0d 81 94 0c 00 f7 d8 64 89 01 48
83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 b0 00 00
00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 51 94 0c 00 f7 d8 64 89
01 48
[ 2265.119210] RSP: 002b:00007ffe06412d98 EFLAGS: 00000206 ORIG_RAX:
00000000000000b0
[ 2265.119225] RAX: ffffffffffffffda RBX: 00005589627f26f0 RCX: 00007f55ec127997
[ 2265.119234] RDX: 0000000000000000 RSI: 0000000000000800 RDI: 00005589627f2758
[ 2265.119241] RBP: 0000000000000000 R08: 1999999999999999 R09: 0000000000000000
[ 2265.119249] R10: 00007f55ec19aac0 R11: 0000000000000206 R12: 00007ffe06412fe0
[ 2265.119257] R13: 00007ffe064133da R14: 00005589627f22a0 R15: 00007ffe06412fe8
[ 2265.119276]  </TASK>
[ 2265.119281] Disabling lock debugging due to kernel taint
[ 2265.119289] Object 0x0000000062d6ea78 @offset=784
[ 2265.120073] Object 0x0000000042bd66e6 @offset=1904
[ 2265.120712] Object 0x00000000603962f0 @offset=2240
[ 2265.121397] =============================================================================
[ 2265.122021] BUG btrfs_path (Tainted: G    B             ): Objects
remaining in btrfs_path on __kmem_cache_shutdown()
(...)

I get thousands and thousands of these messages after running fstests
and doing "rmmod btrfs".

The problem here is the code is reusing the iterator, and every time
allocating a new path without freeing the previous one.
It could simply avoid path allocation and reuse it.

Thanks.



> +       if (!iter->path)
> +               return -ENOMEM;
>
>         /* Current backref iterator only supports iteration in commit root */
> -       ret->path->search_commit_root = 1;
> -       ret->path->skip_locking = 1;
> -       ret->fs_info = fs_info;
> +       iter->path->search_commit_root = 1;
> +       iter->path->skip_locking = 1;
> +       iter->fs_info = fs_info;
>
> -       return ret;
> +       return 0;
>  }
>
>  int btrfs_backref_iter_start(struct btrfs_backref_iter *iter, u64 bytenr)
> diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
> index 83a9a34e948e..24fabbd2a80a 100644
> --- a/fs/btrfs/backref.h
> +++ b/fs/btrfs/backref.h
> @@ -269,15 +269,8 @@ struct btrfs_backref_iter {
>         u32 end_ptr;
>  };
>
> -struct btrfs_backref_iter *btrfs_backref_iter_alloc(struct btrfs_fs_info *fs_info);
> -
> -static inline void btrfs_backref_iter_free(struct btrfs_backref_iter *iter)
> -{
> -       if (!iter)
> -               return;
> -       btrfs_free_path(iter->path);
> -       kfree(iter);
> -}
> +int btrfs_backref_iter_init(struct btrfs_fs_info *fs_info,
> +                           struct btrfs_backref_iter *iter);
>
>  static inline struct extent_buffer *btrfs_backref_get_eb(
>                 struct btrfs_backref_iter *iter)
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index d1dcbb15baa7..6a31e73c3df7 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -464,7 +464,7 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
>                         struct reloc_control *rc, struct btrfs_key *node_key,
>                         int level, u64 bytenr)
>  {
> -       struct btrfs_backref_iter *iter;
> +       struct btrfs_backref_iter iter;
>         struct btrfs_backref_cache *cache = &rc->backref_cache;
>         /* For searching parent of TREE_BLOCK_REF */
>         struct btrfs_path *path;
> @@ -474,9 +474,9 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
>         int ret;
>         int err = 0;
>
> -       iter = btrfs_backref_iter_alloc(rc->extent_root->fs_info);
> -       if (!iter)
> -               return ERR_PTR(-ENOMEM);
> +       ret = btrfs_backref_iter_init(rc->extent_root->fs_info, &iter);
> +       if (ret < 0)
> +               return ERR_PTR(ret);
>         path = btrfs_alloc_path();
>         if (!path) {
>                 err = -ENOMEM;
> @@ -494,7 +494,7 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
>
>         /* Breadth-first search to build backref cache */
>         do {
> -               ret = btrfs_backref_add_tree_node(cache, path, iter, node_key,
> +               ret = btrfs_backref_add_tree_node(cache, path, &iter, node_key,
>                                                   cur);
>                 if (ret < 0) {
>                         err = ret;
> @@ -522,7 +522,7 @@ static noinline_for_stack struct btrfs_backref_node *build_backref_tree(
>         if (handle_useless_nodes(rc, node))
>                 node = NULL;
>  out:
> -       btrfs_backref_iter_free(iter);
> +       btrfs_backref_iter_release(&iter);
>         btrfs_free_path(path);
>         if (err) {
>                 btrfs_backref_error_cleanup(cache, node);
> --
> 2.41.0
>