Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/7] Abort cleanup fixes
@ 2018-11-21 19:05 Josef Bacik
  2018-11-21 19:05 ` [PATCH 1/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delayed_ref_lock Josef Bacik
                   ` (8 more replies)
  0 siblings, 9 replies; 15+ messages in thread
From: Josef Bacik @ 2018-11-21 19:05 UTC (permalink / raw)
  To: linux-btrfs, kernel-team

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delayed_ref_lock
  2018-11-21 19:05 [PATCH 0/7] Abort cleanup fixes Josef Bacik
@ 2018-11-21 19:05 ` Josef Bacik
  2018-11-21 19:05 ` [PATCH 2/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delete_ref_head Josef Bacik
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Josef Bacik @ 2018-11-21 19:05 UTC (permalink / raw)
  To: linux-btrfs, kernel-team

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delete_ref_head
  2018-11-21 19:05 [PATCH 0/7] Abort cleanup fixes Josef Bacik
  2018-11-21 19:05 ` [PATCH 1/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delayed_ref_lock Josef Bacik
@ 2018-11-21 19:05 ` Josef Bacik
  2018-11-21 19:05 ` [PATCH 3/7] btrfs: handle delayed ref head accounting cleanup in abort Josef Bacik
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Josef Bacik @ 2018-11-21 19:05 UTC (permalink / raw)
  To: linux-btrfs, kernel-team

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/7] btrfs: handle delayed ref head accounting cleanup in abort
  2018-11-21 19:05 [PATCH 0/7] Abort cleanup fixes Josef Bacik
  2018-11-21 19:05 ` [PATCH 1/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delayed_ref_lock Josef Bacik
  2018-11-21 19:05 ` [PATCH 2/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delete_ref_head Josef Bacik
@ 2018-11-21 19:05 ` Josef Bacik
  2018-11-21 19:05 ` [PATCH 4/7] btrfs: call btrfs_create_pending_block_groups unconditionally Josef Bacik
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Josef Bacik @ 2018-11-21 19:05 UTC (permalink / raw)
  To: linux-btrfs, kernel-team

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 4/7] btrfs: call btrfs_create_pending_block_groups unconditionally
  2018-11-21 19:05 [PATCH 0/7] Abort cleanup fixes Josef Bacik
                   ` (2 preceding siblings ...)
  2018-11-21 19:05 ` [PATCH 3/7] btrfs: handle delayed ref head accounting cleanup in abort Josef Bacik
@ 2018-11-21 19:05 ` Josef Bacik
  2018-11-21 19:05 ` [PATCH 5/7] btrfs: just delete pending bgs if we are aborted Josef Bacik
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Josef Bacik @ 2018-11-21 19:05 UTC (permalink / raw)
  To: linux-btrfs, kernel-team

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 5/7] btrfs: just delete pending bgs if we are aborted
  2018-11-21 19:05 [PATCH 0/7] Abort cleanup fixes Josef Bacik
                   ` (3 preceding siblings ...)
  2018-11-21 19:05 ` [PATCH 4/7] btrfs: call btrfs_create_pending_block_groups unconditionally Josef Bacik
@ 2018-11-21 19:05 ` Josef Bacik
  2019-01-16 14:59   ` David Sterba
  2018-11-21 19:05 ` [PATCH 6/7] btrfs: cleanup pending bgs on transaction abort Josef Bacik
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 15+ messages in thread
From: Josef Bacik @ 2018-11-21 19:05 UTC (permalink / raw)
  To: linux-btrfs, kernel-team

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 6/7] btrfs: cleanup pending bgs on transaction abort
  2018-11-21 19:05 [PATCH 0/7] Abort cleanup fixes Josef Bacik
                   ` (4 preceding siblings ...)
  2018-11-21 19:05 ` [PATCH 5/7] btrfs: just delete pending bgs if we are aborted Josef Bacik
@ 2018-11-21 19:05 ` Josef Bacik
  2018-11-21 19:05 ` [PATCH 7/7] btrfs: wait on ordered extents on abort cleanup Josef Bacik
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Josef Bacik @ 2018-11-21 19:05 UTC (permalink / raw)
  To: linux-btrfs, kernel-team

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 7/7] btrfs: wait on ordered extents on abort cleanup
  2018-11-21 19:05 [PATCH 0/7] Abort cleanup fixes Josef Bacik
                   ` (5 preceding siblings ...)
  2018-11-21 19:05 ` [PATCH 6/7] btrfs: cleanup pending bgs on transaction abort Josef Bacik
@ 2018-11-21 19:05 ` Josef Bacik
  2019-01-14 11:55 ` [PATCH 0/7] Abort cleanup fixes David Sterba
  2019-01-28 22:06 ` David Sterba
  8 siblings, 0 replies; 15+ messages in thread
From: Josef Bacik @ 2018-11-21 19:05 UTC (permalink / raw)
  To: linux-btrfs, kernel-team

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/7] Abort cleanup fixes
  2018-11-21 19:05 [PATCH 0/7] Abort cleanup fixes Josef Bacik
                   ` (6 preceding siblings ...)
  2018-11-21 19:05 ` [PATCH 7/7] btrfs: wait on ordered extents on abort cleanup Josef Bacik
@ 2019-01-14 11:55 ` David Sterba
  2019-01-28 22:06 ` David Sterba
  8 siblings, 0 replies; 15+ messages in thread
From: David Sterba @ 2019-01-14 11:55 UTC (permalink / raw)
  To: Josef Bacik; +Cc: linux-btrfs, kernel-team

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/7] btrfs: just delete pending bgs if we are aborted
  2018-11-21 19:05 ` [PATCH 5/7] btrfs: just delete pending bgs if we are aborted Josef Bacik
@ 2019-01-16 14:59   ` David Sterba
  2019-01-16 15:22     ` David Sterba
  0 siblings, 1 reply; 15+ messages in thread
From: David Sterba @ 2019-01-16 14:59 UTC (permalink / raw)
  To: Josef Bacik; +Cc: linux-btrfs, kernel-team

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/7] btrfs: just delete pending bgs if we are aborted
  2019-01-16 14:59   ` David Sterba
@ 2019-01-16 15:22     ` David Sterba
  2019-01-17 14:56       ` David Sterba
  0 siblings, 1 reply; 15+ messages in thread
From: David Sterba @ 2019-01-16 15:22 UTC (permalink / raw)
  To: dsterba, Josef Bacik, linux-btrfs, kernel-team

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).

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/7] btrfs: just delete pending bgs if we are aborted
  2019-01-16 15:22     ` David Sterba
@ 2019-01-17 14:56       ` David Sterba
  2019-01-17 14:58         ` Josef Bacik
  2019-01-17 15:40         ` Nikolay Borisov
  0 siblings, 2 replies; 15+ messages in thread
From: David Sterba @ 2019-01-17 14:56 UTC (permalink / raw)
  To: dsterba, Josef Bacik, linux-btrfs, kernel-team

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/7] btrfs: just delete pending bgs if we are aborted
  2019-01-17 14:56       ` David Sterba
@ 2019-01-17 14:58         ` Josef Bacik
  2019-01-17 15:40         ` Nikolay Borisov
  1 sibling, 0 replies; 15+ messages in thread
From: Josef Bacik @ 2019-01-17 14:58 UTC (permalink / raw)
  To: David Sterba; +Cc: Josef Bacik, linux-btrfs, kernel-team

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/7] btrfs: just delete pending bgs if we are aborted
  2019-01-17 14:56       ` David Sterba
  2019-01-17 14:58         ` Josef Bacik
@ 2019-01-17 15:40         ` Nikolay Borisov
  1 sibling, 0 replies; 15+ messages in thread
From: Nikolay Borisov @ 2019-01-17 15:40 UTC (permalink / raw)
  To: dsterba, Josef Bacik, linux-btrfs, kernel-team



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.

> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/7] Abort cleanup fixes
  2018-11-21 19:05 [PATCH 0/7] Abort cleanup fixes Josef Bacik
                   ` (7 preceding siblings ...)
  2019-01-14 11:55 ` [PATCH 0/7] Abort cleanup fixes David Sterba
@ 2019-01-28 22:06 ` David Sterba
  8 siblings, 0 replies; 15+ messages in thread
From: David Sterba @ 2019-01-28 22:06 UTC (permalink / raw)
  To: Josef Bacik; +Cc: linux-btrfs, kernel-team

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-21 19:05 [PATCH 0/7] Abort cleanup fixes Josef Bacik
2018-11-21 19:05 ` [PATCH 1/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delayed_ref_lock Josef Bacik
2018-11-21 19:05 ` [PATCH 2/7] btrfs: make btrfs_destroy_delayed_refs use btrfs_delete_ref_head Josef Bacik
2018-11-21 19:05 ` [PATCH 3/7] btrfs: handle delayed ref head accounting cleanup in abort Josef Bacik
2018-11-21 19:05 ` [PATCH 4/7] btrfs: call btrfs_create_pending_block_groups unconditionally Josef Bacik
2018-11-21 19:05 ` [PATCH 5/7] btrfs: just delete pending bgs if we are aborted Josef Bacik
2019-01-16 14:59   ` David Sterba
2019-01-16 15:22     ` David Sterba
2019-01-17 14:56       ` David Sterba
2019-01-17 14:58         ` Josef Bacik
2019-01-17 15:40         ` Nikolay Borisov
2018-11-21 19:05 ` [PATCH 6/7] btrfs: cleanup pending bgs on transaction abort Josef Bacik
2018-11-21 19:05 ` [PATCH 7/7] btrfs: wait on ordered extents on abort cleanup Josef Bacik
2019-01-14 11:55 ` [PATCH 0/7] Abort cleanup fixes David Sterba
2019-01-28 22:06 ` David Sterba

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox