| Message ID | 20180830174225.2200-31-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | My current patch queue | expand |
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."); >
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
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 >
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.");
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(+)