| Message ID | 20200417153615.23832-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] Btrfs: fix memory leak of transaction when deleting unused block group | expand |
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.
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 >
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.
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; }