[PATCH 0/7] Abort cleanup fixes

[PATCH 0/7] Abort cleanup fixes

[Josef Bacik]

A new xfstests that really hammers on transaction aborts (generic/495 I think?)
uncovered a lot of random issues.  Some of these were introduced with the new
delayed refs rsv patches, others were just exposed by them, such as the pending
bg stuff.  With these patches in place I stopped getting all the random
leftovers and WARN_ON()'s when running whichever xfstest that was and things are
much smoother now.  Thanks,

Josef

[PATCH 1/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delayed_ref_lock

[Josef Bacik]

We have this open coded in btrfs_destroy_delayed_refs, use the helper
instead.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index beaa58e742b5..f062fb0487cd 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4197,16 +4197,9 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
 
 		head = rb_entry(node, struct btrfs_delayed_ref_head,
 				href_node);
-		if (!mutex_trylock(&head->mutex)) {
-			refcount_inc(&head->refs);
-			spin_unlock(&delayed_refs->lock);
-
-			mutex_lock(&head->mutex);
-			mutex_unlock(&head->mutex);
-			btrfs_put_delayed_ref_head(head);
-			spin_lock(&delayed_refs->lock);
+		if (btrfs_delayed_ref_lock(delayed_refs, head))
 			continue;
-		}
+
 		spin_lock(&head->lock);
 		while ((n = rb_first_cached(&head->ref_tree)) != NULL) {
 			ref = rb_entry(n, struct btrfs_delayed_ref_node,
-- 
2.14.3

[PATCH 2/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delete_ref_head

[Josef Bacik]

Instead of open coding this stuff use the helper instead.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f062fb0487cd..7d02748cf3f6 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4215,12 +4215,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
 		if (head->must_insert_reserved)
 			pin_bytes = true;
 		btrfs_free_delayed_extent_op(head->extent_op);
-		delayed_refs->num_heads--;
-		if (head->processing == 0)
-			delayed_refs->num_heads_ready--;
-		atomic_dec(&delayed_refs->num_entries);
-		rb_erase_cached(&head->href_node, &delayed_refs->href_root);
-		RB_CLEAR_NODE(&head->href_node);
+		btrfs_delete_ref_head(delayed_refs, head);
 		spin_unlock(&head->lock);
 		spin_unlock(&delayed_refs->lock);
 		mutex_unlock(&head->mutex);
-- 
2.14.3

[PATCH 3/7] btrfs: handle delayed ref head accounting cleanup in abort

[Josef Bacik]

We weren't doing any of the accounting cleanup when we aborted
transactions.  Fix this by making cleanup_ref_head_accounting global and
calling it from the abort code, this fixes the issue where our
accounting was all wrong after the fs aborts.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.h       |  5 +++++
 fs/btrfs/disk-io.c     |  1 +
 fs/btrfs/extent-tree.c | 13 ++++++-------
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 8ccc5019172b..709de7471d86 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -35,6 +35,7 @@
 struct btrfs_trans_handle;
 struct btrfs_transaction;
 struct btrfs_pending_snapshot;
+struct btrfs_delayed_ref_root;
 extern struct kmem_cache *btrfs_trans_handle_cachep;
 extern struct kmem_cache *btrfs_bit_radix_cachep;
 extern struct kmem_cache *btrfs_path_cachep;
@@ -2643,6 +2644,10 @@ int btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
 			   unsigned long count);
 int btrfs_async_run_delayed_refs(struct btrfs_fs_info *fs_info,
 				 unsigned long count, u64 transid, int wait);
+void
+btrfs_cleanup_ref_head_accounting(struct btrfs_fs_info *fs_info,
+				  struct btrfs_delayed_ref_root *delayed_refs,
+				  struct btrfs_delayed_ref_head *head);
 int btrfs_lookup_data_extent(struct btrfs_fs_info *fs_info, u64 start, u64 len);
 int btrfs_lookup_extent_info(struct btrfs_trans_handle *trans,
 			     struct btrfs_fs_info *fs_info, u64 bytenr,
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 7d02748cf3f6..8e7926c91e35 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4223,6 +4223,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
 		if (pin_bytes)
 			btrfs_pin_extent(fs_info, head->bytenr,
 					 head->num_bytes, 1);
+		btrfs_cleanup_ref_head_accounting(fs_info, delayed_refs, head);
 		btrfs_put_delayed_ref_head(head);
 		cond_resched();
 		spin_lock(&delayed_refs->lock);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index e31980d451c2..a0f8880ee5dd 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2457,12 +2457,11 @@ static int run_and_cleanup_extent_op(struct btrfs_trans_handle *trans,
 	return ret ? ret : 1;
 }
 
-static void cleanup_ref_head_accounting(struct btrfs_trans_handle *trans,
-					struct btrfs_delayed_ref_head *head)
+void
+btrfs_cleanup_ref_head_accounting(struct btrfs_fs_info *fs_info,
+				  struct btrfs_delayed_ref_root *delayed_refs,
+				  struct btrfs_delayed_ref_head *head)
 {
-	struct btrfs_fs_info *fs_info = trans->fs_info;
-	struct btrfs_delayed_ref_root *delayed_refs =
-		&trans->transaction->delayed_refs;
 	int nr_items = 1;
 
 	if (head->total_ref_mod < 0) {
@@ -2540,7 +2539,7 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
 		}
 	}
 
-	cleanup_ref_head_accounting(trans, head);
+	btrfs_cleanup_ref_head_accounting(fs_info, delayed_refs, head);
 
 	trace_run_delayed_ref_head(fs_info, head, 0);
 	btrfs_delayed_ref_unlock(head);
@@ -7218,7 +7217,7 @@ static noinline int check_ref_cleanup(struct btrfs_trans_handle *trans,
 	if (head->must_insert_reserved)
 		ret = 1;
 
-	cleanup_ref_head_accounting(trans, head);
+	btrfs_cleanup_ref_head_accounting(trans->fs_info, delayed_refs, head);
 	mutex_unlock(&head->mutex);
 	btrfs_put_delayed_ref_head(head);
 	return ret;
-- 
2.14.3

[PATCH 4/7] btrfs: call btrfs_create_pending_block_groups unconditionally

[Josef Bacik]

The first thing we do is loop through the list, this

if (!list_empty())
	btrfs_create_pending_block_groups();

thing is just wasted space.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent-tree.c | 3 +--
 fs/btrfs/transaction.c | 6 ++----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a0f8880ee5dd..b9b829c8825c 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3004,8 +3004,7 @@ int btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
 	}
 
 	if (run_all) {
-		if (!list_empty(&trans->new_bgs))
-			btrfs_create_pending_block_groups(trans);
+		btrfs_create_pending_block_groups(trans);
 
 		spin_lock(&delayed_refs->lock);
 		node = rb_first_cached(&delayed_refs->href_root);
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index a4682a696fb6..826a15a07fce 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -845,8 +845,7 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
 	btrfs_trans_release_metadata(trans);
 	trans->block_rsv = NULL;
 
-	if (!list_empty(&trans->new_bgs))
-		btrfs_create_pending_block_groups(trans);
+	btrfs_create_pending_block_groups(trans);
 
 	btrfs_trans_release_chunk_metadata(trans);
 
@@ -1937,8 +1936,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	cur_trans->delayed_refs.flushing = 1;
 	smp_wmb();
 
-	if (!list_empty(&trans->new_bgs))
-		btrfs_create_pending_block_groups(trans);
+	btrfs_create_pending_block_groups(trans);
 
 	ret = btrfs_run_delayed_refs(trans, 0);
 	if (ret) {
-- 
2.14.3

[PATCH 5/7] btrfs: just delete pending bgs if we are aborted

[Josef Bacik]

We still need to do all of the accounting cleanup for pending block
groups if we abort.  So set the ret to trans->aborted so if we aborted
the cleanup happens and everybody is happy.

Reviewed-by: Omar Sandoval <osandov@fb.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent-tree.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index b9b829c8825c..90423b6749b7 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -10500,11 +10500,17 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
 	struct btrfs_root *extent_root = fs_info->extent_root;
 	struct btrfs_block_group_item item;
 	struct btrfs_key key;
-	int ret = 0;
+	int ret;
 
 	if (!trans->can_flush_pending_bgs)
 		return;
 
+	/*
+	 * If we aborted the transaction with pending bg's we need to just
+	 * cleanup the list and carry on.
+	 */
+	ret = trans->aborted;
+
 	while (!list_empty(&trans->new_bgs)) {
 		block_group = list_first_entry(&trans->new_bgs,
 					       struct btrfs_block_group_cache,
-- 
2.14.3

[PATCH 6/7] btrfs: cleanup pending bgs on transaction abort

[Josef Bacik]

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>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/transaction.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 826a15a07fce..3c1be9db897c 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -2269,6 +2269,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	btrfs_scrub_continue(fs_info);
 cleanup_transaction:
 	btrfs_trans_release_metadata(trans);
+	/* This cleans up the pending block groups list properly. */
+	if (!trans->aborted)
+		trans->aborted = ret;
+	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 7/7] btrfs: wait on ordered extents on abort cleanup

[Josef Bacik]

If we flip read-only before we initiate writeback on all dirty pages for
ordered extents we've created then we'll have ordered extents left over
on umount, which results in all sorts of bad things happening.  Fix this
by making sure we wait on ordered extents if we have to do the aborted
transaction cleanup stuff.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8e7926c91e35..c5918ff8241b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4171,6 +4171,14 @@ static void btrfs_destroy_all_ordered_extents(struct btrfs_fs_info *fs_info)
 		spin_lock(&fs_info->ordered_root_lock);
 	}
 	spin_unlock(&fs_info->ordered_root_lock);
+
+	/*
+	 * We need this here because if we've been flipped read-only we won't
+	 * get sync() from the umount, so we need to make sure any ordered
+	 * extents that haven't had their dirty pages IO start writeout yet
+	 * actually get run and error out properly.
+	 */
+	btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1);
 }
 
 static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
-- 
2.14.3

Re: [PATCH 0/7] Abort cleanup fixes

[David Sterba]

On Wed, Nov 21, 2018 at 02:05:38PM -0500, Josef Bacik wrote:
> A new xfstests that really hammers on transaction aborts (generic/495 I think?)

The test number is 475 and it would be really useful to have a sample of
the stacktraces next to the patches that fix the problems. I was seeing
random failures in 475 but took me a while to find this patchset that
fixed them.

I'm queuing the fixes without cleanups to 5.0-rc.

Re: [PATCH 5/7] btrfs: just delete pending bgs if we are aborted

[David Sterba]

On Wed, Nov 21, 2018 at 02:05:43PM -0500, Josef Bacik wrote:
> We still need to do all of the accounting cleanup for pending block
> groups if we abort.  So set the ret to trans->aborted so if we aborted
> the cleanup happens and everybody is happy.
> 
> Reviewed-by: Omar Sandoval <osandov@fb.com>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent-tree.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index b9b829c8825c..90423b6749b7 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -10500,11 +10500,17 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
>  	struct btrfs_root *extent_root = fs_info->extent_root;
>  	struct btrfs_block_group_item item;
>  	struct btrfs_key key;
> -	int ret = 0;
> +	int ret;
>  
>  	if (!trans->can_flush_pending_bgs)
>  		return;
>  
> +	/*
> +	 * If we aborted the transaction with pending bg's we need to just
> +	 * cleanup the list and carry on.
> +	 */
> +	ret = trans->aborted;

The cleanup is suitable for a separate helper that does only

while (!list_empty(&trans->new_bgs)) {
	list_del_init(&block_group->bg_list);
	btrfs_delayed_refs_rsv_release(fs_info, 1);
}

and does not rely on the transaction->abort in a function with 'create'
in it's name.

The related part is in a separate patch that ab-uses the fact that
setting ->abort will trigger the cleanup.

https://patchwork.kernel.org/patch/10693081/ will then simply call the
halper instead of

+	/* This cleans up the pending block groups list properly. */
+	if (!trans->aborted)
+		trans->aborted = ret;
+	btrfs_create_pending_block_groups(trans);

Setting aborted to an error code anywhere else than
__btrfs_abort_transaction does not sound right as it misses the whole
report.

Re: [PATCH 5/7] btrfs: just delete pending bgs if we are aborted

[David Sterba]

On Wed, Jan 16, 2019 at 03:59:20PM +0100, David Sterba wrote:
> On Wed, Nov 21, 2018 at 02:05:43PM -0500, Josef Bacik wrote:
> > We still need to do all of the accounting cleanup for pending block
> > groups if we abort.  So set the ret to trans->aborted so if we aborted
> > the cleanup happens and everybody is happy.
> > 
> > Reviewed-by: Omar Sandoval <osandov@fb.com>
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/extent-tree.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index b9b829c8825c..90423b6749b7 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -10500,11 +10500,17 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
> >  	struct btrfs_root *extent_root = fs_info->extent_root;
> >  	struct btrfs_block_group_item item;
> >  	struct btrfs_key key;
> > -	int ret = 0;
> > +	int ret;
> >  
> >  	if (!trans->can_flush_pending_bgs)
> >  		return;
> >  
> > +	/*
> > +	 * If we aborted the transaction with pending bg's we need to just
> > +	 * cleanup the list and carry on.
> > +	 */
> > +	ret = trans->aborted;
> 
> The cleanup is suitable for a separate helper that does only
> 
> while (!list_empty(&trans->new_bgs)) {
> 	list_del_init(&block_group->bg_list);
> 	btrfs_delayed_refs_rsv_release(fs_info, 1);
> }
> 
> and does not rely on the transaction->abort in a function with 'create'
> in it's name.
> 
> The related part is in a separate patch that ab-uses the fact that
> setting ->abort will trigger the cleanup.
> 
> https://patchwork.kernel.org/patch/10693081/ will then simply call the
> halper instead of
> 
> +	/* This cleans up the pending block groups list properly. */
> +	if (!trans->aborted)
> +		trans->aborted = ret;
> +	btrfs_create_pending_block_groups(trans);
> 
> Setting aborted to an error code anywhere else than
> __btrfs_abort_transaction does not sound right as it misses the whole
> report.

Like this:

--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1901,6 +1901,20 @@ static inline void btrfs_wait_delalloc_flush(struct btrfs_fs_info *fs_info)
                btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1);
 }

+static void btrfs_cleanup_pending_block_groups(struct btrfs_trans_handle *trans)
+{
+       struct btrfs_fs_info *fs_info = trans->fs_info;
+       struct btrfs_block_group_cache *block_group;
+
+       while (!list_empty(&trans->new_bgs)) {
+               block_group = list_first_entry(&trans->new_bgs,
+                                              struct btrfs_block_group_cache,
+                                              bg_list);
+               btrfs_delayed_refs_rsv_release(fs_info, 1);
+               list_del_init(&block_group->bg_list);
+       }
+}
+
 int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 {
        struct btrfs_fs_info *fs_info = trans->fs_info;
@@ -2270,6 +2284,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
        btrfs_scrub_continue(fs_info);
 cleanup_transaction:
        btrfs_trans_release_metadata(trans);
+       btrfs_cleanup_pending_block_groups(trans);
        btrfs_trans_release_chunk_metadata(trans);
        trans->block_rsv = NULL;
        btrfs_warn(fs_info, "Skipping commit of aborted transaction.");
---

The call to btrfs_trans_release_chunk_metadata is not duplicated now as it
would happen twice in you version (within btrfs_create_pending_block_groups and
in transaction commit).

Re: [PATCH 5/7] btrfs: just delete pending bgs if we are aborted

[David Sterba]

On Wed, Jan 16, 2019 at 04:22:55PM +0100, David Sterba wrote:
> On Wed, Jan 16, 2019 at 03:59:20PM +0100, David Sterba wrote:
> > On Wed, Nov 21, 2018 at 02:05:43PM -0500, Josef Bacik wrote:
> > > We still need to do all of the accounting cleanup for pending block
> > > groups if we abort.  So set the ret to trans->aborted so if we aborted
> > > the cleanup happens and everybody is happy.
> > > 
> > > Reviewed-by: Omar Sandoval <osandov@fb.com>
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > > ---
> > >  fs/btrfs/extent-tree.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > > index b9b829c8825c..90423b6749b7 100644
> > > --- a/fs/btrfs/extent-tree.c
> > > +++ b/fs/btrfs/extent-tree.c
> > > @@ -10500,11 +10500,17 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
> > >  	struct btrfs_root *extent_root = fs_info->extent_root;
> > >  	struct btrfs_block_group_item item;
> > >  	struct btrfs_key key;
> > > -	int ret = 0;
> > > +	int ret;
> > >  
> > >  	if (!trans->can_flush_pending_bgs)
> > >  		return;
> > >  
> > > +	/*
> > > +	 * If we aborted the transaction with pending bg's we need to just
> > > +	 * cleanup the list and carry on.
> > > +	 */
> > > +	ret = trans->aborted;
> > 
> > The cleanup is suitable for a separate helper that does only
> > 
> > while (!list_empty(&trans->new_bgs)) {
> > 	list_del_init(&block_group->bg_list);
> > 	btrfs_delayed_refs_rsv_release(fs_info, 1);
> > }
> > 
> > and does not rely on the transaction->abort in a function with 'create'
> > in it's name.
> > 
> > The related part is in a separate patch that ab-uses the fact that
> > setting ->abort will trigger the cleanup.
> > 
> > https://patchwork.kernel.org/patch/10693081/ will then simply call the
> > halper instead of
> > 
> > +	/* This cleans up the pending block groups list properly. */
> > +	if (!trans->aborted)
> > +		trans->aborted = ret;
> > +	btrfs_create_pending_block_groups(trans);
> > 
> > Setting aborted to an error code anywhere else than
> > __btrfs_abort_transaction does not sound right as it misses the whole
> > report.
> 
> Like this:
> 
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1901,6 +1901,20 @@ static inline void btrfs_wait_delalloc_flush(struct btrfs_fs_info *fs_info)
>                 btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1);
>  }
> 
> +static void btrfs_cleanup_pending_block_groups(struct btrfs_trans_handle *trans)
> +{
> +       struct btrfs_fs_info *fs_info = trans->fs_info;
> +       struct btrfs_block_group_cache *block_group;
> +
> +       while (!list_empty(&trans->new_bgs)) {
> +               block_group = list_first_entry(&trans->new_bgs,
> +                                              struct btrfs_block_group_cache,
> +                                              bg_list);
> +               btrfs_delayed_refs_rsv_release(fs_info, 1);
> +               list_del_init(&block_group->bg_list);
> +       }
> +}
> +
>  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  {
>         struct btrfs_fs_info *fs_info = trans->fs_info;
> @@ -2270,6 +2284,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>         btrfs_scrub_continue(fs_info);
>  cleanup_transaction:
>         btrfs_trans_release_metadata(trans);
> +       btrfs_cleanup_pending_block_groups(trans);
>         btrfs_trans_release_chunk_metadata(trans);
>         trans->block_rsv = NULL;
>         btrfs_warn(fs_info, "Skipping commit of aborted transaction.");
> ---
> 
> The call to btrfs_trans_release_chunk_metadata is not duplicated now as it
> would happen twice in you version (within btrfs_create_pending_block_groups and
> in transaction commit).

FYI, this passed the 475 test (with accounting warnings that are
possibly fixed by the other patches) + no blowups in the following
tests.

Re: [PATCH 5/7] btrfs: just delete pending bgs if we are aborted

[Josef Bacik]

On Thu, Jan 17, 2019 at 03:56:32PM +0100, David Sterba wrote:
> On Wed, Jan 16, 2019 at 04:22:55PM +0100, David Sterba wrote:
> > On Wed, Jan 16, 2019 at 03:59:20PM +0100, David Sterba wrote:
> > > On Wed, Nov 21, 2018 at 02:05:43PM -0500, Josef Bacik wrote:
> > > > We still need to do all of the accounting cleanup for pending block
> > > > groups if we abort.  So set the ret to trans->aborted so if we aborted
> > > > the cleanup happens and everybody is happy.
> > > > 
> > > > Reviewed-by: Omar Sandoval <osandov@fb.com>
> > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > > > ---
> > > >  fs/btrfs/extent-tree.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > > > index b9b829c8825c..90423b6749b7 100644
> > > > --- a/fs/btrfs/extent-tree.c
> > > > +++ b/fs/btrfs/extent-tree.c
> > > > @@ -10500,11 +10500,17 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
> > > >  	struct btrfs_root *extent_root = fs_info->extent_root;
> > > >  	struct btrfs_block_group_item item;
> > > >  	struct btrfs_key key;
> > > > -	int ret = 0;
> > > > +	int ret;
> > > >  
> > > >  	if (!trans->can_flush_pending_bgs)
> > > >  		return;
> > > >  
> > > > +	/*
> > > > +	 * If we aborted the transaction with pending bg's we need to just
> > > > +	 * cleanup the list and carry on.
> > > > +	 */
> > > > +	ret = trans->aborted;
> > > 
> > > The cleanup is suitable for a separate helper that does only
> > > 
> > > while (!list_empty(&trans->new_bgs)) {
> > > 	list_del_init(&block_group->bg_list);
> > > 	btrfs_delayed_refs_rsv_release(fs_info, 1);
> > > }
> > > 
> > > and does not rely on the transaction->abort in a function with 'create'
> > > in it's name.
> > > 
> > > The related part is in a separate patch that ab-uses the fact that
> > > setting ->abort will trigger the cleanup.
> > > 
> > > https://patchwork.kernel.org/patch/10693081/ will then simply call the
> > > halper instead of
> > > 
> > > +	/* This cleans up the pending block groups list properly. */
> > > +	if (!trans->aborted)
> > > +		trans->aborted = ret;
> > > +	btrfs_create_pending_block_groups(trans);
> > > 
> > > Setting aborted to an error code anywhere else than
> > > __btrfs_abort_transaction does not sound right as it misses the whole
> > > report.
> > 
> > Like this:
> > 
> > --- a/fs/btrfs/transaction.c
> > +++ b/fs/btrfs/transaction.c
> > @@ -1901,6 +1901,20 @@ static inline void btrfs_wait_delalloc_flush(struct btrfs_fs_info *fs_info)
> >                 btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1);
> >  }
> > 
> > +static void btrfs_cleanup_pending_block_groups(struct btrfs_trans_handle *trans)
> > +{
> > +       struct btrfs_fs_info *fs_info = trans->fs_info;
> > +       struct btrfs_block_group_cache *block_group;
> > +
> > +       while (!list_empty(&trans->new_bgs)) {
> > +               block_group = list_first_entry(&trans->new_bgs,
> > +                                              struct btrfs_block_group_cache,
> > +                                              bg_list);
> > +               btrfs_delayed_refs_rsv_release(fs_info, 1);
> > +               list_del_init(&block_group->bg_list);
> > +       }
> > +}
> > +
> >  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
> >  {
> >         struct btrfs_fs_info *fs_info = trans->fs_info;
> > @@ -2270,6 +2284,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
> >         btrfs_scrub_continue(fs_info);
> >  cleanup_transaction:
> >         btrfs_trans_release_metadata(trans);
> > +       btrfs_cleanup_pending_block_groups(trans);
> >         btrfs_trans_release_chunk_metadata(trans);
> >         trans->block_rsv = NULL;
> >         btrfs_warn(fs_info, "Skipping commit of aborted transaction.");
> > ---
> > 
> > The call to btrfs_trans_release_chunk_metadata is not duplicated now as it
> > would happen twice in you version (within btrfs_create_pending_block_groups and
> > in transaction commit).
> 
> FYI, this passed the 475 test (with accounting warnings that are
> possibly fixed by the other patches) + no blowups in the following
> tests.

I'm fine with it if you want to throw it in there.  I'm still trying to get my
fix for the balance thing through xfstests.  Thanks,

Josef

Re: [PATCH 5/7] btrfs: just delete pending bgs if we are aborted

[Nikolay Borisov]

On 17.01.19 г. 16:56 ч., David Sterba wrote:
> On Wed, Jan 16, 2019 at 04:22:55PM +0100, David Sterba wrote:
>> On Wed, Jan 16, 2019 at 03:59:20PM +0100, David Sterba wrote:
>>> On Wed, Nov 21, 2018 at 02:05:43PM -0500, Josef Bacik wrote:
>>>> We still need to do all of the accounting cleanup for pending block
>>>> groups if we abort.  So set the ret to trans->aborted so if we aborted
>>>> the cleanup happens and everybody is happy.
>>>>
>>>> Reviewed-by: Omar Sandoval <osandov@fb.com>
>>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>>> ---
>>>>  fs/btrfs/extent-tree.c | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>> index b9b829c8825c..90423b6749b7 100644
>>>> --- a/fs/btrfs/extent-tree.c
>>>> +++ b/fs/btrfs/extent-tree.c
>>>> @@ -10500,11 +10500,17 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
>>>>  	struct btrfs_root *extent_root = fs_info->extent_root;
>>>>  	struct btrfs_block_group_item item;
>>>>  	struct btrfs_key key;
>>>> -	int ret = 0;
>>>> +	int ret;
>>>>  
>>>>  	if (!trans->can_flush_pending_bgs)
>>>>  		return;
>>>>  
>>>> +	/*
>>>> +	 * If we aborted the transaction with pending bg's we need to just
>>>> +	 * cleanup the list and carry on.
>>>> +	 */
>>>> +	ret = trans->aborted;
>>>
>>> The cleanup is suitable for a separate helper that does only
>>>
>>> while (!list_empty(&trans->new_bgs)) {
>>> 	list_del_init(&block_group->bg_list);
>>> 	btrfs_delayed_refs_rsv_release(fs_info, 1);
>>> }
>>>
>>> and does not rely on the transaction->abort in a function with 'create'
>>> in it's name.
>>>
>>> The related part is in a separate patch that ab-uses the fact that
>>> setting ->abort will trigger the cleanup.
>>>
>>> https://patchwork.kernel.org/patch/10693081/ will then simply call the
>>> halper instead of
>>>
>>> +	/* This cleans up the pending block groups list properly. */
>>> +	if (!trans->aborted)
>>> +		trans->aborted = ret;
>>> +	btrfs_create_pending_block_groups(trans);
>>>
>>> Setting aborted to an error code anywhere else than
>>> __btrfs_abort_transaction does not sound right as it misses the whole
>>> report.
>>
>> Like this:
>>
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -1901,6 +1901,20 @@ static inline void btrfs_wait_delalloc_flush(struct btrfs_fs_info *fs_info)
>>                 btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1);
>>  }
>>
>> +static void btrfs_cleanup_pending_block_groups(struct btrfs_trans_handle *trans)
>> +{
>> +       struct btrfs_fs_info *fs_info = trans->fs_info;
>> +       struct btrfs_block_group_cache *block_group;
>> +
>> +       while (!list_empty(&trans->new_bgs)) {
>> +               block_group = list_first_entry(&trans->new_bgs,
>> +                                              struct btrfs_block_group_cache,
>> +                                              bg_list);
>> +               btrfs_delayed_refs_rsv_release(fs_info, 1);
>> +               list_del_init(&block_group->bg_list);
>> +       }
>> +}
>> +
>>  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>>  {
>>         struct btrfs_fs_info *fs_info = trans->fs_info;
>> @@ -2270,6 +2284,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>>         btrfs_scrub_continue(fs_info);
>>  cleanup_transaction:
>>         btrfs_trans_release_metadata(trans);
>> +       btrfs_cleanup_pending_block_groups(trans);
>>         btrfs_trans_release_chunk_metadata(trans);
>>         trans->block_rsv = NULL;
>>         btrfs_warn(fs_info, "Skipping commit of aborted transaction.");
>> ---
>>
>> The call to btrfs_trans_release_chunk_metadata is not duplicated now as it
>> would happen twice in you version (within btrfs_create_pending_block_groups and
>> in transaction commit).
> 
> FYI, this passed the 475 test (with accounting warnings that are
> possibly fixed by the other patches) + no blowups in the following
> tests.

For what is worth 475 doesn't always fail for me - without those
patches. So make sure you have given it a good bashing before deeming it
good.

> 

Re: [PATCH 0/7] Abort cleanup fixes

[David Sterba]

On Wed, Nov 21, 2018 at 02:05:38PM -0500, Josef Bacik wrote:
> A new xfstests that really hammers on transaction aborts (generic/495 I think?)
> uncovered a lot of random issues.  Some of these were introduced with the new
> delayed refs rsv patches, others were just exposed by them, such as the pending
> bg stuff.  With these patches in place I stopped getting all the random
> leftovers and WARN_ON()'s when running whichever xfstest that was and things are
> much smoother now.  Thanks,

FYI

in 5.0-rc:

- btrfs: wait on ordered extents on abort cleanup
- btrfs: handle delayed ref head accounting cleanup in abort

in misc-next:

- btrfs: make btrfs_destroy_delayed_refs use btrfs_delete_ref_head
- btrfs: make btrfs_destroy_delayed_refs use btrfs_delayed_ref_lock
- btrfs: call btrfs_create_pending_block_groups unconditionally

reworked as https://patchwork.kernel.org/patch/10784039/ :

- btrfs: cleanup pending bgs on transaction abort
- btrfs: just delete pending bgs if we are aborted