[30/35] btrfs: cleanup pending bgs on transaction abort
diff mbox series

Message ID 20180830174225.2200-31-josef@toxicpanda.com
State New
Headers show
Series
  • My current patch queue
Related show

Commit Message

Josef Bacik Aug. 30, 2018, 5:42 p.m. UTC
We may abort the transaction during a commit and not have a chance to
run the pending bgs stuff, which will leave block groups on our list and
cause us accounting issues and leaked memory.  Fix this by running the
pending bgs when we cleanup a transaction.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/transaction.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Nikolay Borisov Aug. 31, 2018, 7:48 a.m. UTC | #1
On 30.08.2018 20:42, Josef Bacik wrote:
> We may abort the transaction during a commit and not have a chance to
> run the pending bgs stuff, which will leave block groups on our list and
> cause us accounting issues and leaked memory.  Fix this by running the
> pending bgs when we cleanup a transaction.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/transaction.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 89d14f135837..0f39a0d302d3 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -2273,6 +2273,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  	btrfs_scrub_continue(fs_info);
>  cleanup_transaction:
>  	btrfs_trans_release_metadata(trans);
> +	btrfs_create_pending_block_groups(trans);

And now you've basically hi-jacked btrfs_create_pending_block_groups to
just act as "delete all bg" in case transaction is aborted. Considering
this and the previous patch I'd rather you replace them with a single
one which introduces a new function delete_pending_bgs or whatever and
use that. This will be more explicit and self-documenting.

>  	btrfs_trans_release_chunk_metadata(trans);
>  	trans->block_rsv = NULL;
>  	btrfs_warn(fs_info, "Skipping commit of aborted transaction.");
>
Josef Bacik Aug. 31, 2018, 2:07 p.m. UTC | #2
On Fri, Aug 31, 2018 at 10:48:58AM +0300, Nikolay Borisov wrote:
> 
> 
> On 30.08.2018 20:42, Josef Bacik wrote:
> > We may abort the transaction during a commit and not have a chance to
> > run the pending bgs stuff, which will leave block groups on our list and
> > cause us accounting issues and leaked memory.  Fix this by running the
> > pending bgs when we cleanup a transaction.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/transaction.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > index 89d14f135837..0f39a0d302d3 100644
> > --- a/fs/btrfs/transaction.c
> > +++ b/fs/btrfs/transaction.c
> > @@ -2273,6 +2273,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
> >  	btrfs_scrub_continue(fs_info);
> >  cleanup_transaction:
> >  	btrfs_trans_release_metadata(trans);
> > +	btrfs_create_pending_block_groups(trans);
> 
> And now you've basically hi-jacked btrfs_create_pending_block_groups to
> just act as "delete all bg" in case transaction is aborted. Considering
> this and the previous patch I'd rather you replace them with a single
> one which introduces a new function delete_pending_bgs or whatever and
> use that. This will be more explicit and self-documenting.
> 

I haven't hi-jacked it and I'm not adding another helper when we already have
code that does the right thing.  Remember if we abort a transaction in a path
that doesn't commit we still end up calling btrfs_create_pending_block_groups()
in btrfs_end_transaction which does the cleanup there, I'm not adding a bunch of
new code for a case that's easily handled with the previous fix and works the
same way in other paths.  Thanks,

Josef
Omar Sandoval Sept. 1, 2018, 12:34 a.m. UTC | #3
On Thu, Aug 30, 2018 at 01:42:20PM -0400, Josef Bacik wrote:
> We may abort the transaction during a commit and not have a chance to
> run the pending bgs stuff, which will leave block groups on our list and
> cause us accounting issues and leaked memory.  Fix this by running the
> pending bgs when we cleanup a transaction.

Reviewed-by: Omar Sandoval <osandov@fb.com>

Again, I think it's fine to reuse the same function as long as there's a
comment here.

> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/transaction.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 89d14f135837..0f39a0d302d3 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -2273,6 +2273,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  	btrfs_scrub_continue(fs_info);
>  cleanup_transaction:
>  	btrfs_trans_release_metadata(trans);
> +	btrfs_create_pending_block_groups(trans);
>  	btrfs_trans_release_chunk_metadata(trans);
>  	trans->block_rsv = NULL;
>  	btrfs_warn(fs_info, "Skipping commit of aborted transaction.");
> -- 
> 2.14.3
>

Patch
diff mbox series

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 89d14f135837..0f39a0d302d3 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -2273,6 +2273,7 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	btrfs_scrub_continue(fs_info);
 cleanup_transaction:
 	btrfs_trans_release_metadata(trans);
+	btrfs_create_pending_block_groups(trans);
 	btrfs_trans_release_chunk_metadata(trans);
 	trans->block_rsv = NULL;
 	btrfs_warn(fs_info, "Skipping commit of aborted transaction.");