[PATCH 0/6] Several bugfixes

[PATCH 0/6] Several bugfixes

[Liu Bo]

Patch 1-5 can make balance process bail out gracefully after we get into
readonly by aborting transaction.

Patch 6 addresses a hang when we have two or more trans handle aborted
and committed themselves.

Liu Bo (6):
  Btrfs: check for NULL pointer in updating reloc roots
  Btrfs: build up error handling for merge_reloc_roots
  Btrfs: free all recorded tree blocks on error
  Btrfs: do not BUG_ON in prepare_to_reloc
  Btrfs: do not BUG_ON on aborted situation
  Btrfs: avoid deadlock on transaction waiting list

 fs/btrfs/relocation.c  |   74 +++++++++++++++++++++++++++++++++++++-----------
 fs/btrfs/transaction.c |    7 ++++
 fs/btrfs/volumes.c     |    9 ++++-
 3 files changed, 71 insertions(+), 19 deletions(-)

-- 
1.7.7

[PATCH 1/6] Btrfs: check for NULL pointer in updating reloc roots

[Liu Bo]

Add a check for NULL pointer to avoid invalid reference.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/relocation.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 300e09a..d9be73b 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1269,6 +1269,8 @@ static int __update_reloc_root(struct btrfs_root *root, int del)
 	}
 	spin_unlock(&rc->reloc_root_tree.lock);
 
+	if (!node)
+		return 0;
 	BUG_ON((struct btrfs_root *)node->data != root);
 
 	if (!del) {
-- 
1.7.7

[PATCH 2/6] Btrfs: build up error handling for merge_reloc_roots

[Liu Bo]

We first use btrfs_std_error hook to replace with BUG_ON, and we
also need to cleanup what is left, including reloc roots rbtree
and reloc roots list.
Here we use a helper function to cleanup both rbtree and list, and
since this function can also be used in the balance recover path,
we also make the change as well to keep code simple.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/relocation.c |   47 +++++++++++++++++++++++++++++++++++------------
 1 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index d9be73b..8608afb 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2240,13 +2240,28 @@ again:
 }
 
 static noinline_for_stack
+void free_reloc_roots(struct list_head *list)
+{
+	struct btrfs_root *reloc_root;
+
+	while (!list_empty(list)) {
+		reloc_root = list_entry(list->next, struct btrfs_root,
+					root_list);
+		__update_reloc_root(reloc_root, 1);
+		free_extent_buffer(reloc_root->node);
+		free_extent_buffer(reloc_root->commit_root);
+		kfree(reloc_root);
+	}
+}
+
+static noinline_for_stack
 int merge_reloc_roots(struct reloc_control *rc)
 {
 	struct btrfs_root *root;
 	struct btrfs_root *reloc_root;
 	LIST_HEAD(reloc_roots);
 	int found = 0;
-	int ret;
+	int ret = 0;
 again:
 	root = rc->extent_root;
 
@@ -2272,20 +2287,33 @@ again:
 			BUG_ON(root->reloc_root != reloc_root);
 
 			ret = merge_reloc_root(rc, root);
-			BUG_ON(ret);
+			if (ret)
+				goto out;
 		} else {
 			list_del_init(&reloc_root->root_list);
 		}
 		ret = btrfs_drop_snapshot(reloc_root, rc->block_rsv, 0, 1);
-		BUG_ON(ret < 0);
+		if (ret < 0) {
+			if (list_empty(&reloc_root->root_list))
+				list_add_tail(&reloc_root->root_list,
+					      &reloc_roots);
+			goto out;
+		}
 	}
 
 	if (found) {
 		found = 0;
 		goto again;
 	}
+out:
+	if (ret) {
+		btrfs_std_error(root->fs_info, ret);
+		if (!list_empty(&reloc_roots))
+			free_reloc_roots(&reloc_roots);
+	}
+
 	BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root));
-	return 0;
+	return ret;
 }
 
 static void free_block_list(struct rb_root *blocks)
@@ -4266,14 +4294,9 @@ int btrfs_recover_relocation(struct btrfs_root *root)
 out_free:
 	kfree(rc);
 out:
-	while (!list_empty(&reloc_roots)) {
-		reloc_root = list_entry(reloc_roots.next,
-					struct btrfs_root, root_list);
-		list_del(&reloc_root->root_list);
-		free_extent_buffer(reloc_root->node);
-		free_extent_buffer(reloc_root->commit_root);
-		kfree(reloc_root);
-	}
+	if (!list_empty(&reloc_roots))
+		free_reloc_roots(&reloc_roots);
+
 	btrfs_free_path(path);
 
 	if (err == 0) {
-- 
1.7.7

Re: [PATCH 2/6] Btrfs: build up error handling for merge_reloc_roots

[David Sterba]

On Tue, Mar 05, 2013 at 12:25:37AM +0800, Liu Bo wrote:
> We first use btrfs_std_error hook to replace with BUG_ON, and we
> also need to cleanup what is left, including reloc roots rbtree
> and reloc roots list.
> Here we use a helper function to cleanup both rbtree and list, and
> since this function can also be used in the balance recover path,
> we also make the change as well to keep code simple.

I've noticed that return value from merge_reloc_roots is never checked
in the callers. Did you verify that this is ok?

For example when called from btrfs_recover_relocation, is it possible
that the reloc-to-be-recovered is still left unprocessed but the
filesystem is going to be silently mounted read-write?

The mount and remount callpaths check for recover_relocation errors and
do not proceed otherwise. In RO mount, it's not called.

So, either merge_reloc_roots callers should catch the errors or there
are none by design (ie. the whole reloc operation is restartable).

david

Re: [PATCH 2/6] Btrfs: build up error handling for merge_reloc_roots

[Liu Bo]

On Mon, Mar 18, 2013 at 03:16:12PM +0100, David Sterba wrote:
> On Tue, Mar 05, 2013 at 12:25:37AM +0800, Liu Bo wrote:
> > We first use btrfs_std_error hook to replace with BUG_ON, and we
> > also need to cleanup what is left, including reloc roots rbtree
> > and reloc roots list.
> > Here we use a helper function to cleanup both rbtree and list, and
> > since this function can also be used in the balance recover path,
> > we also make the change as well to keep code simple.
> 
> I've noticed that return value from merge_reloc_roots is never checked
> in the callers. Did you verify that this is ok?

Yeah, it's fine.

Actually we set fs to RO once we get error here, as we have recorded a balance
item and , balance can start over again the next time.

> 
> For example when called from btrfs_recover_relocation, is it possible
> that the reloc-to-be-recovered is still left unprocessed but the
> filesystem is going to be silently mounted read-write?

hmm, I'm not able to picture such a case...

> 
> The mount and remount callpaths check for recover_relocation errors and
> do not proceed otherwise. In RO mount, it's not called.
> 
> So, either merge_reloc_roots callers should catch the errors or there
> are none by design (ie. the whole reloc operation is restartable).
> 

it's desigend to be always ready for a crash or a power off.

thanks,
liubo

Re: [PATCH 2/6] Btrfs: build up error handling for merge_reloc_roots

[David Sterba]

On Tue, Mar 19, 2013 at 08:16:41PM +0800, Liu Bo wrote:
> On Mon, Mar 18, 2013 at 03:16:12PM +0100, David Sterba wrote:
> > I've noticed that return value from merge_reloc_roots is never checked
> > in the callers. Did you verify that this is ok?
> 
> Yeah, it's fine.

Then it's ok to change return value to 'void' so it does not look like
an unhandled errorcode.

> Actually we set fs to RO once we get error here, as we have recorded a balance
> item and , balance can start over again the next time.

Yeah, if we get there via transaction abort. My background motivation is
to implement a (much) more responsive balance cancel. This is different
from the poweroff "cancel", because it does not keep any in-memory
state.

david

[PATCH 3/6] Btrfs: free all recorded tree blocks on error

[Liu Bo]

We've missed the 'free blocks' part on ENOMEM error.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/relocation.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 8608afb..8cae3d7 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2848,8 +2848,10 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 	int err = 0;
 
 	path = btrfs_alloc_path();
-	if (!path)
-		return -ENOMEM;
+	if (!path) {
+		err = -ENOMEM;
+		goto out_path;
+	}
 
 	rb_node = rb_first(blocks);
 	while (rb_node) {
@@ -2888,10 +2890,11 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 		rb_node = rb_next(rb_node);
 	}
 out:
-	free_block_list(blocks);
 	err = finish_pending_nodes(trans, rc, path, err);
 
 	btrfs_free_path(path);
+out_path:
+	free_block_list(blocks);
 	return err;
 }
 
-- 
1.7.7

[PATCH 4/6] Btrfs: do not BUG_ON in prepare_to_reloc

[Liu Bo]

We can bail out from here gracefully instead of a cold BUG_ON.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/relocation.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 8cae3d7..c45c833 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3731,7 +3731,15 @@ int prepare_to_relocate(struct reloc_control *rc)
 	set_reloc_control(rc);
 
 	trans = btrfs_join_transaction(rc->extent_root);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		unset_reloc_control(rc);
+		/*
+		 * extent tree is not a ref_cow tree and has no reloc_root to
+		 * cleanup.  And callers are responsible to free the above
+		 * block rsv.
+		 */
+		return PTR_ERR(trans);
+	}
 	btrfs_commit_transaction(trans, rc->extent_root);
 	return 0;
 }
-- 
1.7.7

[PATCH 5/6] Btrfs: do not BUG_ON on aborted situation

[Liu Bo]

Btrfs balance can easily hit BUG_ON in these places, but we want
to it bail out gracefully after we force the whole filesystem to
readonly.  So we use btrfs_std_error hook in place of BUG_ON.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/relocation.c |    6 +++++-
 fs/btrfs/volumes.c    |    9 +++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index c45c833..9fd63e9 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3771,7 +3771,11 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 	while (1) {
 		progress++;
 		trans = btrfs_start_transaction(rc->extent_root, 0);
-		BUG_ON(IS_ERR(trans));
+		if (IS_ERR(trans)) {
+			err = PTR_ERR(trans);
+			trans = NULL;
+			break;
+		}
 restart:
 		if (update_backref_cache(trans, &rc->backref_cache)) {
 			btrfs_end_transaction(trans, rc->extent_root);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5349e17..0a1559f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2357,7 +2357,11 @@ static int btrfs_relocate_chunk(struct btrfs_root *root,
 		return ret;
 
 	trans = btrfs_start_transaction(root, 0);
-	BUG_ON(IS_ERR(trans));
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		btrfs_std_error(root->fs_info, ret);
+		return ret;
+	}
 
 	lock_chunks(root);
 
@@ -3024,7 +3028,8 @@ static void __cancel_balance(struct btrfs_fs_info *fs_info)
 
 	unset_balance_control(fs_info);
 	ret = del_balance_item(fs_info->tree_root);
-	BUG_ON(ret);
+	if (ret)
+		btrfs_std_error(fs_info, ret);
 
 	atomic_set(&fs_info->mutually_exclusive_operation_running, 0);
 }
-- 
1.7.7

[PATCH 6/6] Btrfs: avoid deadlock on transaction waiting list

[Liu Bo]

Only let one trans handle to wait for other handles, otherwise we
will get ABBA issues.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/transaction.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 4caa1fa..7cc38ed 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1394,6 +1394,13 @@ static void cleanup_transaction(struct btrfs_trans_handle *trans,
 	btrfs_abort_transaction(trans, root, err);
 
 	spin_lock(&root->fs_info->trans_lock);
+
+	if (list_empty(&cur_trans->list)) {
+		spin_unlock(&root->fs_info->trans_lock);
+		btrfs_end_transaction(trans, root);
+		return;
+	}
+
 	list_del_init(&cur_trans->list);
 	if (cur_trans == root->fs_info->running_transaction) {
 		root->fs_info->trans_no_join = 1;
-- 
1.7.7