[PATCH 1/2] Btrfs: fix memory leak of transaction when deleting unused block group

[PATCH 1/2] Btrfs: fix memory leak of transaction when deleting unused block group

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When cleaning pinned extents right before deleting an unused block group,
we check if there's still a previous transaction running and if so we
increment its reference count before using it for cleaning pinned ranges
in its pinned extents iotree. However we ended up never decrementing the
reference count after using the transaction, resulting in a memory leak.

Fix it by decrementing the reference count.

Fixes: fe119a6eeb6705 ("btrfs: switch to per-transaction pinned extents")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 47f66c6a7d7f..93c180ffcb80 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1288,11 +1288,15 @@ static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
 	if (ret)
 		goto err;
 	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
+	if (prev_trans)
+		refcount_dec(&prev_trans->use_count);
 
 	return true;
 
 err:
 	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
+	if (prev_trans)
+		refcount_dec(&prev_trans->use_count);
 	btrfs_dec_block_group_ro(bg);
 	return false;
 }
-- 
2.11.0

Re: [PATCH 1/2] Btrfs: fix memory leak of transaction when deleting unused block group

[David Sterba]

On Fri, Apr 17, 2020 at 03:40:12PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When cleaning pinned extents right before deleting an unused block group,
> we check if there's still a previous transaction running and if so we
> increment its reference count before using it for cleaning pinned ranges
> in its pinned extents iotree. However we ended up never decrementing the
> reference count after using the transaction, resulting in a memory leak.
> 
> Fix it by decrementing the reference count.
> 
> Fixes: fe119a6eeb6705 ("btrfs: switch to per-transaction pinned extents")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.

Re: [PATCH 1/2] Btrfs: fix memory leak of transaction when deleting unused block group

[Nikolay Borisov]

On 17.04.20 г. 17:40 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When cleaning pinned extents right before deleting an unused block group,
> we check if there's still a previous transaction running and if so we
> increment its reference count before using it for cleaning pinned ranges
> in its pinned extents iotree. However we ended up never decrementing the
> reference count after using the transaction, resulting in a memory leak.
> 
> Fix it by decrementing the reference count.
> 
> Fixes: fe119a6eeb6705 ("btrfs: switch to per-transaction pinned extents")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 1/2] Btrfs: fix memory leak of transaction when deleting unused block group

[Filipe Manana]

On Fri, Apr 17, 2020 at 4:23 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Fri, Apr 17, 2020 at 03:40:12PM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > When cleaning pinned extents right before deleting an unused block group,
> > we check if there's still a previous transaction running and if so we
> > increment its reference count before using it for cleaning pinned ranges
> > in its pinned extents iotree. However we ended up never decrementing the
> > reference count after using the transaction, resulting in a memory leak.
> >
> > Fix it by decrementing the reference count.
> >
> > Fixes: fe119a6eeb6705 ("btrfs: switch to per-transaction pinned extents")
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
>
> Added to misc-next, thanks.

This is actually wrong, I'll send a v2 soon.

[PATCH v2 1/2] Btrfs: fix memory leak of transaction when deleting unused block group

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When cleaning pinned extents right before deleting an unused block group,
we check if there's still a previous transaction running and if so we
increment its reference count before using it for cleaning pinned ranges
in its pinned extents iotree. However we ended up never decrementing the
reference count after using the transaction, resulting in a memory leak.

Fix it by decrementing the reference count.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---

V2: Use btrfs_put_transaction() and not refcount_dec(), otherwise we are
    not really releasing the memory used by the transaction in case its
    refcount is 1. Stupid mistake.

 fs/btrfs/block-group.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 47f66c6a7d7f..af9e9a008724 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1288,11 +1288,15 @@ static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
 	if (ret)
 		goto err;
 	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
+	if (prev_trans)
+		btrfs_put_transaction(prev_trans);
 
 	return true;
 
 err:
 	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
+	if (prev_trans)
+		btrfs_put_transaction(prev_trans);
 	btrfs_dec_block_group_ro(bg);
 	return false;
 }
-- 
2.11.0

Re: [PATCH v2 1/2] Btrfs: fix memory leak of transaction when deleting unused block group

[David Sterba]

On Fri, Apr 17, 2020 at 04:36:15PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When cleaning pinned extents right before deleting an unused block group,
> we check if there's still a previous transaction running and if so we
> increment its reference count before using it for cleaning pinned ranges
> in its pinned extents iotree. However we ended up never decrementing the
> reference count after using the transaction, resulting in a memory leak.
> 
> Fix it by decrementing the reference count.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
> 
> V2: Use btrfs_put_transaction() and not refcount_dec(), otherwise we are
>     not really releasing the memory used by the transaction in case its
>     refcount is 1. Stupid mistake.

I missed it too. As Nik pointed out on IRC, the API should be consistent
so that for put there's a get, or refcount_inc/refcount_dec. In this
case there's non-trivial work on the put side, so the wrappers make
sense.

A good example may be btrfs_get_block_group/btrfs_put_block_group, same
as for the transaction.

Trivial wrappers around plain _inc/_dec wouldn't be so great, but in
case there's eg refcount_dec_and_test + kfree, then I think this can be
considered a good pattern.

Re: [PATCH v2 1/2] Btrfs: fix memory leak of transaction when deleting unused block group

[Filipe Manana]

On Fri, Apr 17, 2020 at 4:38 PM <fdmanana@kernel.org> wrote:
>
> From: Filipe Manana <fdmanana@suse.com>
>
> When cleaning pinned extents right before deleting an unused block group,
> we check if there's still a previous transaction running and if so we
> increment its reference count before using it for cleaning pinned ranges
> in its pinned extents iotree. However we ended up never decrementing the
> reference count after using the transaction, resulting in a memory leak.
>
> Fix it by decrementing the reference count.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Fixes: fe119a6eeb6705 ("btrfs: switch to per-transaction pinned extents")

And missed that in v2, but had it in v1.

> ---
>
> V2: Use btrfs_put_transaction() and not refcount_dec(), otherwise we are
>     not really releasing the memory used by the transaction in case its
>     refcount is 1. Stupid mistake.
>
>  fs/btrfs/block-group.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 47f66c6a7d7f..af9e9a008724 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1288,11 +1288,15 @@ static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
>         if (ret)
>                 goto err;
>         mutex_unlock(&fs_info->unused_bg_unpin_mutex);
> +       if (prev_trans)
> +               btrfs_put_transaction(prev_trans);
>
>         return true;
>
>  err:
>         mutex_unlock(&fs_info->unused_bg_unpin_mutex);
> +       if (prev_trans)
> +               btrfs_put_transaction(prev_trans);
>         btrfs_dec_block_group_ro(bg);
>         return false;
>  }
> --
> 2.11.0
>

Re: [PATCH v2 1/2] Btrfs: fix memory leak of transaction when deleting unused block group

[David Sterba]

On Tue, Apr 21, 2020 at 09:05:33AM +0100, Filipe Manana wrote:
> On Fri, Apr 17, 2020 at 4:38 PM <fdmanana@kernel.org> wrote:
> >
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > When cleaning pinned extents right before deleting an unused block group,
> > we check if there's still a previous transaction running and if so we
> > increment its reference count before using it for cleaning pinned ranges
> > in its pinned extents iotree. However we ended up never decrementing the
> > reference count after using the transaction, resulting in a memory leak.
> >
> > Fix it by decrementing the reference count.
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> 
> Fixes: fe119a6eeb6705 ("btrfs: switch to per-transaction pinned extents")
> 
> And missed that in v2, but had it in v1.

Patch updated, thanks.