[PATCH v2 00/42] Cleanup error handling in relocation

[PATCH v2 00/42] Cleanup error handling in relocation

[Josef Bacik]

v1->v2:
- fixed a bug where I accidentally dropped reading flags in relocate_block_group
  when I dropped the extra checks that we handle in the tree checker.

--- Original message ---
Hello,

Relocation is the last place that is not able to handle errors at all, which
results in all sorts of lovely panics if you encounter corruptions or IO errors.
I'm going to start cleaning up relocation, but before I move code around I want
the error handling to be somewhat sane, so I'm not changing behavior and error
handling at the same time.

These patches are purely about error handling, there is no behavior changing
other than returning errors up the chain properly.  There is a lot of room for
follow up cleanups, which will happen next.  However I wanted to get this series
done today and out so we could get it merged ASAP, and then the follow up
cleanups can happen later as they are less important and less critical.

The only exception to the above is the patch to add the error injection sites
for btrfs_cow_block and btrfs_search_slot, and a lockdep fix that I discovered
while running my tests, those are the first two patches in the series.

I tested this with my error injection stress test, where I keep track of all
stack traces that have been tested and only inject errors when we have a new
stack trace, which means I should have covered all of the various error
conditions.  With this patchset I'm no longer panicing while stressing the error
conditions.  Thanks,

Josef

Josef Bacik (42):
  btrfs: allow error injection for btrfs_search_slot and btrfs_cow_block
  btrfs: fix lockdep splat in btrfs_recover_relocation
  btrfs: convert some BUG_ON()'s to ASSERT()'s in do_relocation
  btrfs: convert BUG_ON()'s in relocate_tree_block
  btrfs: return an error from btrfs_record_root_in_trans
  btrfs: handle errors from select_reloc_root()
  btrfs: convert BUG_ON()'s in select_reloc_root() to proper errors
  btrfs: check record_root_in_trans related failures in
    select_reloc_root
  btrfs: do proper error handling in record_reloc_root_in_trans
  btrfs: handle btrfs_record_root_in_trans failure in
    btrfs_rename_exchange
  btrfs: handle btrfs_record_root_in_trans failure in btrfs_rename
  btrfs: handle btrfs_record_root_in_trans failure in
    btrfs_delete_subvolume
  btrfs: handle btrfs_record_root_in_trans failure in
    btrfs_recover_log_trees
  btrfs: handle btrfs_record_root_in_trans failure in create_subvol
  btrfs: btrfs: handle btrfs_record_root_in_trans failure in
    relocate_tree_block
  btrfs: handle btrfs_record_root_in_trans failure in start_transaction
  btrfs: handle record_root_in_trans failure in qgroup_account_snapshot
  btrfs: handle record_root_in_trans failure in
    btrfs_record_root_in_trans
  btrfs: handle record_root_in_trans failure in create_pending_snapshot
  btrfs: do not panic in __add_reloc_root
  btrfs: have proper error handling in btrfs_init_reloc_root
  btrfs: do proper error handling in create_reloc_root
  btrfs: handle btrfs_update_reloc_root failure in commit_fs_roots
  btrfs: change insert_dirty_subvol to return errors
  btrfs: handle btrfs_update_reloc_root failure in insert_dirty_subvol
  btrfs: handle btrfs_update_reloc_root failure in prepare_to_merge
  btrfs: do proper error handling in btrfs_update_reloc_root
  btrfs: convert logic BUG_ON()'s in replace_path to ASSERT()'s
  btrfs: handle initial btrfs_cow_block error in replace_path
  btrfs: handle the loop btrfs_cow_block error in replace_path
  btrfs: handle btrfs_search_slot failure in replace_path
  btrfs: handle errors in reference count manipulation in replace_path
  btrfs: handle extent reference errors in do_relocation
  btrfs: check for BTRFS_BLOCK_FLAG_FULL_BACKREF being set improperly
  btrfs: remove the extent item sanity checks in relocate_block_group
  btrfs: do proper error handling in create_reloc_inode
  btrfs: handle __add_reloc_root failure in btrfs_recover_relocation
  btrfs: handle __add_reloc_root failure in btrfs_reloc_post_snapshot
  btrfs: cleanup error handling in prepare_to_merge
  btrfs: handle extent corruption with select_one_root properly
  btrfs: do proper error handling in merge_reloc_roots
  btrfs: check return value of btrfs_commit_transaction in relocation

 fs/btrfs/ctree.c        |   2 +
 fs/btrfs/inode.c        |  21 ++-
 fs/btrfs/ioctl.c        |   6 +-
 fs/btrfs/relocation.c   | 373 ++++++++++++++++++++++++++++++----------
 fs/btrfs/transaction.c  |  37 ++--
 fs/btrfs/tree-checker.c |   5 +
 fs/btrfs/tree-log.c     |   8 +-
 fs/btrfs/volumes.c      |   2 +
 8 files changed, 343 insertions(+), 111 deletions(-)

-- 
2.26.2

[PATCH v2 01/42] btrfs: allow error injection for btrfs_search_slot and btrfs_cow_block

[Josef Bacik]

The following patches are going to address error handling in relocation,
in order to test those patches I need to be able to inject errors in
btrfs_search_slot and btrfs_cow_block, as we call both of these pretty
often in different cases during relocation.

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

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index f2c3d29b6bc4..8e551b237ee0 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1494,6 +1494,7 @@ noinline int btrfs_cow_block(struct btrfs_trans_handle *trans,
 
 	return ret;
 }
+ALLOW_ERROR_INJECTION(btrfs_cow_block, ERRNO);
 
 /*
  * helper function for defrag to decide if two blocks pointed to by a
@@ -2817,6 +2818,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		btrfs_release_path(p);
 	return ret;
 }
+ALLOW_ERROR_INJECTION(btrfs_search_slot, ERRNO);
 
 /*
  * Like btrfs_search_slot, this looks for a key in the given tree. It uses the
-- 
2.26.2

[PATCH v2 02/42] btrfs: fix lockdep splat in btrfs_recover_relocation

[Josef Bacik]

While testing the error paths of relocation I hit the following lockdep
splat

======================================================
WARNING: possible circular locking dependency detected
5.10.0-rc2-btrfs-next-71 #1 Not tainted
------------------------------------------------------
find/324157 is trying to acquire lock:
ffff8ebc48d293a0 (btrfs-tree-01#2/3){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]

but task is already holding lock:
ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (btrfs-tree-00){++++}-{3:3}:
       lock_acquire+0xd8/0x490
       down_write_nested+0x44/0x120
       __btrfs_tree_lock+0x27/0x120 [btrfs]
       btrfs_search_slot+0x2a3/0xc50 [btrfs]
       btrfs_insert_empty_items+0x58/0xa0 [btrfs]
       insert_with_overflow+0x44/0x110 [btrfs]
       btrfs_insert_xattr_item+0xb8/0x1d0 [btrfs]
       btrfs_setxattr+0xd6/0x4c0 [btrfs]
       btrfs_setxattr_trans+0x68/0x100 [btrfs]
       __vfs_setxattr+0x66/0x80
       __vfs_setxattr_noperm+0x70/0x200
       vfs_setxattr+0x6b/0x120
       setxattr+0x125/0x240
       path_setxattr+0xba/0xd0
       __x64_sys_setxattr+0x27/0x30
       do_syscall_64+0x33/0x80
       entry_SYSCALL_64_after_hwframe+0x44/0xa9

-> #0 (btrfs-tree-01#2/3){++++}-{3:3}:
       check_prev_add+0x91/0xc60
       __lock_acquire+0x1689/0x3130
       lock_acquire+0xd8/0x490
       down_read_nested+0x45/0x220
       __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
       btrfs_next_old_leaf+0x27d/0x580 [btrfs]
       btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
       iterate_dir+0x170/0x1c0
       __x64_sys_getdents64+0x83/0x140
       do_syscall_64+0x33/0x80
       entry_SYSCALL_64_after_hwframe+0x44/0xa9

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(btrfs-tree-00);
                               lock(btrfs-tree-01#2/3);
                               lock(btrfs-tree-00);
  lock(btrfs-tree-01#2/3);

 *** DEADLOCK ***

5 locks held by find/324157:
 #0: ffff8ebc502c6e00 (&f->f_pos_lock){+.+.}-{3:3}, at: __fdget_pos+0x4d/0x60
 #1: ffff8eb97f689980 (&type->i_mutex_dir_key#10){++++}-{3:3}, at: iterate_dir+0x52/0x1c0
 #2: ffff8ebaec00ca58 (btrfs-tree-02#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
 #3: ffff8eb98f986f78 (btrfs-tree-01#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
 #4: ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]

stack backtrace:
CPU: 2 PID: 324157 Comm: find Not tainted 5.10.0-rc2-btrfs-next-71 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
Call Trace:
 dump_stack+0x8d/0xb5
 check_noncircular+0xff/0x110
 ? mark_lock.part.0+0x468/0xe90
 check_prev_add+0x91/0xc60
 __lock_acquire+0x1689/0x3130
 ? kvm_clock_read+0x14/0x30
 ? kvm_sched_clock_read+0x5/0x10
 lock_acquire+0xd8/0x490
 ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
 down_read_nested+0x45/0x220
 ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
 __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
 btrfs_next_old_leaf+0x27d/0x580 [btrfs]
 btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
 iterate_dir+0x170/0x1c0
 __x64_sys_getdents64+0x83/0x140
 ? filldir+0x1d0/0x1d0
 do_syscall_64+0x33/0x80
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

This is thankfully straightforward to fix, simply release the path
before we setup the reloc_ctl.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bb1aa96e1233..ece8bb62fcc1 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4283,6 +4283,8 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
 		btrfs_warn(fs_info,
 	"balance: cannot set exclusive op status, resume manually");
 
+	btrfs_release_path(path);
+
 	mutex_lock(&fs_info->balance_mutex);
 	BUG_ON(fs_info->balance_ctl);
 	spin_lock(&fs_info->balance_lock);
-- 
2.26.2

[PATCH v2 03/42] btrfs: convert some BUG_ON()'s to ASSERT()'s in do_relocation

[Josef Bacik]

A few of these are checking for correctness, and won't be triggered by
corrupted file systems, so convert them to ASSERT() instead of BUG_ON()
and add a comment explaining their existence.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index c5774a8e6ff7..d4cf982c78ff 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2194,7 +2194,11 @@ static int do_relocation(struct btrfs_trans_handle *trans,
 	int ret;
 	int err = 0;
 
-	BUG_ON(lowest && node->eb);
+	/*
+	 * If we are lowest then this is the first time we're processing this
+	 * block, and thus shouldn't have an eb associated with it yet.
+	 */
+	ASSERT(!lowest || !node->eb);
 
 	path->lowest_level = node->level + 1;
 	rc->backref_cache.path[node->level] = node;
@@ -2287,7 +2291,11 @@ static int do_relocation(struct btrfs_trans_handle *trans,
 				err = ret;
 				goto next;
 			}
-			BUG_ON(node->eb != eb);
+			/*
+			 * We've just cow'ed this block, it should have updated
+			 * the correct backref node entry.
+			 */
+			ASSERT(node->eb == eb);
 		} else {
 			btrfs_set_node_blockptr(upper->eb, slot,
 						node->eb->start);
@@ -2323,7 +2331,12 @@ static int do_relocation(struct btrfs_trans_handle *trans,
 	}
 
 	path->lowest_level = 0;
-	BUG_ON(err == -ENOSPC);
+
+	/*
+	 * We should have allocated all of our space in the block rsv and thus
+	 * shouldn't ENOSPC.
+	 */
+	ASSERT(err != -ENOSPC);
 	return err;
 }
 
-- 
2.26.2

[PATCH v2 04/42] btrfs: convert BUG_ON()'s in relocate_tree_block

[Josef Bacik]

We have a couple of BUG_ON()'s in relocate_tree_block() that can be
tripped if we have file system corruption.  Convert these to ASSERT()'s
so developers still get yelled at when they break the backref code, but
error out nicely for users so the whole box doesn't go down.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index d4cf982c78ff..3a5e89c82fa5 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2475,8 +2475,28 @@ static int relocate_tree_block(struct btrfs_trans_handle *trans,
 
 	if (root) {
 		if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) {
-			BUG_ON(node->new_bytenr);
-			BUG_ON(!list_empty(&node->list));
+			/*
+			 * This block was the root block of a root, and this is
+			 * the first time we're processing the block and thus it
+			 * should not have had the ->new_bytenr modified and
+			 * should have not been included on the changed list.
+			 *
+			 * However in the case of corruption we could have
+			 * multiple refs pointing to the same block improperly,
+			 * and thus we would trip over these checks.  ASSERT()
+			 * for the developer case, because it could indicate a
+			 * bug in the backref code, however error out for a
+			 * normal user in the case of corruption.
+			 */
+			ASSERT(node->new_bytenr == 0);
+			ASSERT(list_empty(&node->list));
+			if (node->new_bytenr || !list_empty(&node->list)) {
+				btrfs_err(root->fs_info,
+				  "bytenr %llu has improper references to it",
+					  node->bytenr);
+				ret = -EUCLEAN;
+				goto out;
+			}
 			btrfs_record_root_in_trans(trans, root);
 			root = root->reloc_root;
 			node->new_bytenr = root->node->start;
-- 
2.26.2

[PATCH v2 05/42] btrfs: return an error from btrfs_record_root_in_trans

[Josef Bacik]

We can create a reloc root when we record the root in the trans, which
can fail for all sorts of different reasons.  Propagate this error up
the chain of callers.  Future patches will fix the callers of
btrfs_record_root_in_trans() to handle the error.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 0e4063651047..fab26241fb2e 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -403,6 +403,7 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
 			       int force)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
+	int ret = 0;
 
 	if ((test_bit(BTRFS_ROOT_SHAREABLE, &root->state) &&
 	    root->last_trans < trans->transid) || force) {
@@ -451,11 +452,11 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
 		 * lock.  smp_wmb() makes sure that all the writes above are
 		 * done before we pop in the zero below
 		 */
-		btrfs_init_reloc_root(trans, root);
+		ret = btrfs_init_reloc_root(trans, root);
 		smp_mb__before_atomic();
 		clear_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state);
 	}
-	return 0;
+	return ret;
 }
 
 
-- 
2.26.2

[PATCH v2 06/42] btrfs: handle errors from select_reloc_root()

[Josef Bacik]

Currently select_reloc_root() doesn't return an error, but followup
patches will make it possible for it to return an error.  We do have
proper error recovery in do_relocation however, so handle the
possibility of select_reloc_root() having an error properly instead of
BUG_ON(!root).  I've also adjusted select_reloc_root() to return
ERR_PTR(-ENOENT) if we don't find a root, instead of NULL, to make the
error case easier to deal with.  I've replaced the BUG_ON(!root) with an
ASSERT(ret != -ENOENT), as this indicates we messed up the backref
walking code, but could indicate corruption so we do not want to have a
BUG_ON() here.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 3a5e89c82fa5..89e9253846cc 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2037,7 +2037,7 @@ struct btrfs_root *select_reloc_root(struct btrfs_trans_handle *trans,
 			break;
 	}
 	if (!root)
-		return NULL;
+		return ERR_PTR(-ENOENT);
 
 	next = node;
 	/* setup backref node path for btrfs_reloc_cow_block */
@@ -2209,7 +2209,18 @@ static int do_relocation(struct btrfs_trans_handle *trans,
 
 		upper = edge->node[UPPER];
 		root = select_reloc_root(trans, rc, upper, edges);
-		BUG_ON(!root);
+		if (IS_ERR(root)) {
+			err = PTR_ERR(root);
+
+			/*
+			 * This can happen if there's fs corruption, but if we
+			 * have ASSERT()'s on then we're developers and we
+			 * likely made a logic mistake in the backref code, so
+			 * check for this error condition.
+			 */
+			ASSERT(err != -ENOENT);
+			goto next;
+		}
 
 		if (upper->eb && !upper->locked) {
 			if (!lowest) {
-- 
2.26.2

[PATCH v2 07/42] btrfs: convert BUG_ON()'s in select_reloc_root() to proper errors

[Josef Bacik]

We have several BUG_ON()'s in select_reloc_root() that can be tripped if
you have extent tree corruption.  Convert these to ASSERT()'s, because
if we hit it during testing it really is bad, or could indicate a
problem with the backref walking code.

However if users hit these problems it generally indicates corruption,
I've hit a few machines in the fleet that trip over these with clearly
corrupted extent trees, so be nice and spit out an error message and
return an error instead of bringing the whole box down.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 51 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 89e9253846cc..78bed3c4d635 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2006,8 +2006,35 @@ struct btrfs_root *select_reloc_root(struct btrfs_trans_handle *trans,
 		cond_resched();
 		next = walk_up_backref(next, edges, &index);
 		root = next->root;
-		BUG_ON(!root);
-		BUG_ON(!test_bit(BTRFS_ROOT_SHAREABLE, &root->state));
+
+		/*
+		 * If there is no root, then our references for this block are
+		 * incomplete, as we should be able to walk all the way up to a
+		 * block that is owned by a root.
+		 *
+		 * This path is only for SHAREABLE roots, so if we come upon a
+		 * non-SHAREABLE root then we have backrefs that resolve
+		 * improperly.
+		 *
+		 * Both of these cases indicate file system corruption, or a bug
+		 * in the backref walking code.  The ASSERT() is to make sure
+		 * developers get bitten as soon as possible, proper error
+		 * handling is for users who may have corrupt file systems.
+		 */
+		ASSERT(root);
+		ASSERT(test_bit(BTRFS_ROOT_SHAREABLE, &root->state));
+		if (!root) {
+			btrfs_err(trans->fs_info,
+		"bytenr %llu doesn't have a backref path ending in a root",
+				  node->bytenr);
+			return ERR_PTR(-EUCLEAN);
+		}
+		if (!test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) {
+			btrfs_err(trans->fs_info,
+"bytenr %llu has multiple refs with one ending in a non shareable root",
+				  node->bytenr);
+			return ERR_PTR(-EUCLEAN);
+		}
 
 		if (root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) {
 			record_reloc_root_in_trans(trans, root);
@@ -2018,8 +2045,24 @@ struct btrfs_root *select_reloc_root(struct btrfs_trans_handle *trans,
 		root = root->reloc_root;
 
 		if (next->new_bytenr != root->node->start) {
-			BUG_ON(next->new_bytenr);
-			BUG_ON(!list_empty(&next->list));
+			/*
+			 * We just created the reloc root, so we shouldn't have
+			 * ->new_bytenr set and this shouldn't be in the changed
+			 *  list.  If it is then we have multiple roots pointing
+			 *  at the same bytenr, or we've made a mistake in the
+			 *  backref walking code.  ASSERT() for developers,
+			 *  error out for users, as it indicates corruption or a
+			 *  bad bug.
+			 */
+			ASSERT(next->new_bytenr == 0);
+			ASSERT(list_empty(&next->list));
+			if (next->new_bytenr || !list_empty(&next->list)) {
+				btrfs_err(trans->fs_info,
+"bytenr %llu possibly has multiple roots pointing at the same bytenr %llu",
+					  node->bytenr, next->bytenr);
+				return ERR_PTR(-EUCLEAN);
+			}
+
 			next->new_bytenr = root->node->start;
 			btrfs_put_root(next->root);
 			next->root = btrfs_grab_root(root);
-- 
2.26.2

[PATCH v2 08/42] btrfs: check record_root_in_trans related failures in select_reloc_root

[Josef Bacik]

We will record the fs root or the reloc root in the trans in
select_reloc_root.  These will actually return errors in the following
patches, so check their return value here and return it up the stack.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 78bed3c4d635..5a470d3d91a4 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2000,6 +2000,7 @@ struct btrfs_root *select_reloc_root(struct btrfs_trans_handle *trans,
 	struct btrfs_backref_node *next;
 	struct btrfs_root *root;
 	int index = 0;
+	int ret;
 
 	next = node;
 	while (1) {
@@ -2037,11 +2038,15 @@ struct btrfs_root *select_reloc_root(struct btrfs_trans_handle *trans,
 		}
 
 		if (root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) {
-			record_reloc_root_in_trans(trans, root);
+			ret = record_reloc_root_in_trans(trans, root);
+			if (ret)
+				return ERR_PTR(ret);
 			break;
 		}
 
-		btrfs_record_root_in_trans(trans, root);
+		ret = btrfs_record_root_in_trans(trans, root);
+		if (ret)
+			return ERR_PTR(ret);
 		root = root->reloc_root;
 
 		if (next->new_bytenr != root->node->start) {
-- 
2.26.2

[PATCH v2 09/42] btrfs: do proper error handling in record_reloc_root_in_trans

[Josef Bacik]

Generally speaking this shouldn't ever fail, the corresponding fs root
for the reloc root will already be in memory, so we won't get -ENOMEM
here.

However if there is no corresponding root for the reloc root then we
could get -ENOMEM when we try to allocate it or we could get -ENOENT
when we look it up and see that it doesn't exist.

Convert these BUG_ON()'s into ASSERT()'s + proper error handling for the
case of corruption.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 5a470d3d91a4..4397a8a448f4 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1983,8 +1983,30 @@ static int record_reloc_root_in_trans(struct btrfs_trans_handle *trans,
 		return 0;
 
 	root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset, false);
-	BUG_ON(IS_ERR(root));
-	BUG_ON(root->reloc_root != reloc_root);
+
+	/*
+	 * This should succeed, since we can't have a reloc root without having
+	 * already looked up the actual root and created the reloc root for this
+	 * root.
+	 *
+	 * However if there's some sort of corruption where we have a ref to a
+	 * reloc root without a corresponding root this could return -ENOENT.
+	 *
+	 * The ASSERT()'s are to catch this case in testing, because it could
+	 * indicate a bug, but for non-developers it indicates corruption and we
+	 * should error out.
+	 */
+	ASSERT(!IS_ERR(root));
+	ASSERT(root->reloc_root == reloc_root);
+	if (IS_ERR(root))
+		return PTR_ERR(root);
+	if (root->reloc_root != reloc_root) {
+		btrfs_err(fs_info,
+			  "root %llu has two reloc roots associated with it",
+			  reloc_root->root_key.offset);
+		btrfs_put_root(root);
+		return -EUCLEAN;
+	}
 	ret = btrfs_record_root_in_trans(trans, root);
 	btrfs_put_root(root);
 
-- 
2.26.2

[PATCH v2 10/42] btrfs: handle btrfs_record_root_in_trans failure in btrfs_rename_exchange

[Josef Bacik]

btrfs_record_root_in_trans will return errors in the future, so handle
the error properly in btrfs_rename_exchange.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c790cff41be5..e64c6a98ad54 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8883,8 +8883,11 @@ static int btrfs_rename_exchange(struct inode *old_dir,
 		goto out_notrans;
 	}
 
-	if (dest != root)
-		btrfs_record_root_in_trans(trans, dest);
+	if (dest != root) {
+		ret = btrfs_record_root_in_trans(trans, dest);
+		if (ret)
+			goto out_fail;
+	}
 
 	/*
 	 * We need to find a free sequence number both in the source and
-- 
2.26.2

[PATCH v2 11/42] btrfs: handle btrfs_record_root_in_trans failure in btrfs_rename

[Josef Bacik]

btrfs_record_root_in_trans will return errors in the future, so handle
the error properly in btrfs_rename.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e64c6a98ad54..ce6602a80324 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9191,8 +9191,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		goto out_notrans;
 	}
 
-	if (dest != root)
-		btrfs_record_root_in_trans(trans, dest);
+	if (dest != root) {
+		ret = btrfs_record_root_in_trans(trans, dest);
+		if (ret)
+			goto out_fail;
+	}
 
 	ret = btrfs_set_inode_index(BTRFS_I(new_dir), &index);
 	if (ret)
-- 
2.26.2

[PATCH v2 12/42] btrfs: handle btrfs_record_root_in_trans failure in btrfs_delete_subvolume

[Josef Bacik]

btrfs_record_root_in_trans will return errors in the future, so handle
the error properly in btrfs_delete_subvolume.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ce6602a80324..9cc8b810f4fe 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4146,7 +4146,12 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
 		goto out_end_trans;
 	}
 
-	btrfs_record_root_in_trans(trans, dest);
+	ret = btrfs_record_root_in_trans(trans, dest);
+	if (ret) {
+		err = ret;
+		btrfs_abort_transaction(trans, ret);
+		goto out_end_trans;
+	}
 
 	memset(&dest->root_item.drop_progress, 0,
 		sizeof(dest->root_item.drop_progress));
-- 
2.26.2

[PATCH v2 13/42] btrfs: handle btrfs_record_root_in_trans failure in btrfs_recover_log_trees

[Josef Bacik]

btrfs_record_root_in_trans will return errors in the future, so handle
the error properly in btrfs_recover_log_trees.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/tree-log.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 955c9a36cfeb..1ad77e2399f7 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -6276,8 +6276,12 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 		}
 
 		wc.replay_dest->log_root = log;
-		btrfs_record_root_in_trans(trans, wc.replay_dest);
-		ret = walk_log_tree(trans, log, &wc);
+		ret = btrfs_record_root_in_trans(trans, wc.replay_dest);
+		if (ret)
+			btrfs_handle_fs_error(fs_info, ret,
+				"Couldn't record the root in the transaction.");
+		else
+			ret = walk_log_tree(trans, log, &wc);
 
 		if (!ret && wc.stage == LOG_WALK_REPLAY_ALL) {
 			ret = fixup_inode_link_counts(trans, wc.replay_dest,
-- 
2.26.2

[PATCH v2 14/42] btrfs: handle btrfs_record_root_in_trans failure in create_subvol

[Josef Bacik]

btrfs_record_root_in_trans will return errors in the future, so handle
the error properly in create_subvol.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ioctl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a5dc7cc5d705..da9026a487d2 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -702,7 +702,11 @@ static noinline int create_subvol(struct inode *dir,
 	/* Freeing will be done in btrfs_put_root() of new_root */
 	anon_dev = 0;
 
-	btrfs_record_root_in_trans(trans, new_root);
+	ret = btrfs_record_root_in_trans(trans, new_root);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		goto fail;
+	}
 
 	ret = btrfs_create_subvol_root(trans, new_root, root, new_dirid);
 	btrfs_put_root(new_root);
-- 
2.26.2

[PATCH v2 15/42] btrfs: btrfs: handle btrfs_record_root_in_trans failure in relocate_tree_block

[Josef Bacik]

btrfs_record_root_in_trans will return errors in the future, so handle
the error properly in relocate_tree_block.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 4397a8a448f4..6f7bbbd76102 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2578,7 +2578,9 @@ static int relocate_tree_block(struct btrfs_trans_handle *trans,
 				ret = -EUCLEAN;
 				goto out;
 			}
-			btrfs_record_root_in_trans(trans, root);
+			ret = btrfs_record_root_in_trans(trans, root);
+			if (ret)
+				goto out;
 			root = root->reloc_root;
 			node->new_bytenr = root->node->start;
 			btrfs_put_root(node->root);
-- 
2.26.2

[PATCH v2 16/42] btrfs: handle btrfs_record_root_in_trans failure in start_transaction

[Josef Bacik]

btrfs_record_root_in_trans will return errors in the future, so handle
the error properly in start_transaction.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index fab26241fb2e..f7fd013ecc2a 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -737,7 +737,11 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
 	 * Thus it need to be called after current->journal_info initialized,
 	 * or we can deadlock.
 	 */
-	btrfs_record_root_in_trans(h, root);
+	ret = btrfs_record_root_in_trans(h, root);
+	if (ret) {
+		btrfs_end_transaction(h);
+		return ERR_PTR(ret);
+	}
 
 	return h;
 
-- 
2.26.2

[PATCH v2 17/42] btrfs: handle record_root_in_trans failure in qgroup_account_snapshot

[Josef Bacik]

record_root_in_trans can fail currently, so handle this failure
properly.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index f7fd013ecc2a..ad12a13d0412 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1440,7 +1440,9 @@ static int qgroup_account_snapshot(struct btrfs_trans_handle *trans,
 	 * recorded root will never be updated again, causing an outdated root
 	 * item.
 	 */
-	record_root_in_trans(trans, src, 1);
+	ret = record_root_in_trans(trans, src, 1);
+	if (ret)
+		return ret;
 
 	/*
 	 * We are going to commit transaction, see btrfs_commit_transaction()
@@ -1492,7 +1494,7 @@ static int qgroup_account_snapshot(struct btrfs_trans_handle *trans,
 	 * insert_dir_item()
 	 */
 	if (!ret)
-		record_root_in_trans(trans, parent, 1);
+		ret = record_root_in_trans(trans, parent, 1);
 	return ret;
 }
 
-- 
2.26.2

[PATCH v2 18/42] btrfs: handle record_root_in_trans failure in btrfs_record_root_in_trans

[Josef Bacik]

record_root_in_trans can fail currently, handle this failure properly.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index ad12a13d0412..d0f130172622 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -483,6 +483,7 @@ int btrfs_record_root_in_trans(struct btrfs_trans_handle *trans,
 			       struct btrfs_root *root)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
+	int ret;
 
 	if (!test_bit(BTRFS_ROOT_SHAREABLE, &root->state))
 		return 0;
@@ -497,10 +498,10 @@ int btrfs_record_root_in_trans(struct btrfs_trans_handle *trans,
 		return 0;
 
 	mutex_lock(&fs_info->reloc_mutex);
-	record_root_in_trans(trans, root, 0);
+	ret = record_root_in_trans(trans, root, 0);
 	mutex_unlock(&fs_info->reloc_mutex);
 
-	return 0;
+	return ret;
 }
 
 static inline int is_transaction_blocked(struct btrfs_transaction *trans)
-- 
2.26.2

[PATCH v2 19/42] btrfs: handle record_root_in_trans failure in create_pending_snapshot

[Josef Bacik]

record_root_in_trans can currently fail, so handle this failure
properly.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index d0f130172622..0aa6d8ddad21 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1572,8 +1572,9 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 	dentry = pending->dentry;
 	parent_inode = pending->dir;
 	parent_root = BTRFS_I(parent_inode)->root;
-	record_root_in_trans(trans, parent_root, 0);
-
+	ret = record_root_in_trans(trans, parent_root, 0);
+	if (ret)
+		goto fail;
 	cur_time = current_time(parent_inode);
 
 	/*
@@ -1609,7 +1610,11 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 		goto fail;
 	}
 
-	record_root_in_trans(trans, root, 0);
+	ret = record_root_in_trans(trans, root, 0);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		goto fail;
+	}
 	btrfs_set_root_last_snapshot(&root->root_item, trans->transid);
 	memcpy(new_root_item, &root->root_item, sizeof(*new_root_item));
 	btrfs_check_and_init_root_item(new_root_item);
-- 
2.26.2

[PATCH v2 20/42] btrfs: do not panic in __add_reloc_root

[Josef Bacik]

If we have a duplicate entry for a reloc root then we could have fs
corruption that resulted in a double allocation.  This shouldn't happen
generally so leave an ASSERT() for this case, but return an error
instead of panicing in the normal user case.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 6f7bbbd76102..63f42aa43fa3 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -637,10 +637,12 @@ static int __must_check __add_reloc_root(struct btrfs_root *root)
 	rb_node = rb_simple_insert(&rc->reloc_root_tree.rb_root,
 				   node->bytenr, &node->rb_node);
 	spin_unlock(&rc->reloc_root_tree.lock);
+	ASSERT(rb_node == NULL);
 	if (rb_node) {
-		btrfs_panic(fs_info, -EEXIST,
+		btrfs_err(fs_info,
 			    "Duplicate root found for start=%llu while inserting into relocation tree",
 			    node->bytenr);
+		return -EEXIST;
 	}
 
 	list_add_tail(&root->root_list, &rc->reloc_roots);
-- 
2.26.2

[PATCH v2 21/42] btrfs: have proper error handling in btrfs_init_reloc_root

[Josef Bacik]

create_reloc_root will return errors in the future, and __add_reloc_root
can return -ENOMEM or -EEXIST, so handle these errors properly.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 63f42aa43fa3..4afba27419f0 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -860,9 +860,14 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans,
 	reloc_root = create_reloc_root(trans, root, root->root_key.objectid);
 	if (clear_rsv)
 		trans->block_rsv = rsv;
+	if (IS_ERR(reloc_root))
+		return PTR_ERR(reloc_root);
 
 	ret = __add_reloc_root(reloc_root);
-	BUG_ON(ret < 0);
+	if (ret) {
+		btrfs_put_root(reloc_root);
+		return ret;
+	}
 	root->reloc_root = btrfs_grab_root(reloc_root);
 	return 0;
 }
-- 
2.26.2

[PATCH v2 22/42] btrfs: do proper error handling in create_reloc_root

[Josef Bacik]

We do memory allocations here, read blocks from disk, all sorts of
operations that could easily fail at any given point.  Instead of
panicing the box, simply return the error back up the chain, all callers
at this point have proper error handling.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 4afba27419f0..2ae1a3816ce5 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -737,10 +737,11 @@ static struct btrfs_root *create_reloc_root(struct btrfs_trans_handle *trans,
 	struct extent_buffer *eb;
 	struct btrfs_root_item *root_item;
 	struct btrfs_key root_key;
-	int ret;
+	int ret = 0;
 
 	root_item = kmalloc(sizeof(*root_item), GFP_NOFS);
-	BUG_ON(!root_item);
+	if (!root_item)
+		return ERR_PTR(-ENOMEM);
 
 	root_key.objectid = BTRFS_TREE_RELOC_OBJECTID;
 	root_key.type = BTRFS_ROOT_ITEM_KEY;
@@ -752,7 +753,9 @@ static struct btrfs_root *create_reloc_root(struct btrfs_trans_handle *trans,
 		/* called by btrfs_init_reloc_root */
 		ret = btrfs_copy_root(trans, root, root->commit_root, &eb,
 				      BTRFS_TREE_RELOC_OBJECTID);
-		BUG_ON(ret);
+		if (ret)
+			goto fail;
+
 		/*
 		 * Set the last_snapshot field to the generation of the commit
 		 * root - like this ctree.c:btrfs_block_can_be_shared() behaves
@@ -773,7 +776,8 @@ static struct btrfs_root *create_reloc_root(struct btrfs_trans_handle *trans,
 		 */
 		ret = btrfs_copy_root(trans, root, root->node, &eb,
 				      BTRFS_TREE_RELOC_OBJECTID);
-		BUG_ON(ret);
+		if (ret)
+			goto fail;
 	}
 
 	memcpy(root_item, &root->root_item, sizeof(*root_item));
@@ -793,14 +797,20 @@ static struct btrfs_root *create_reloc_root(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_insert_root(trans, fs_info->tree_root,
 				&root_key, root_item);
-	BUG_ON(ret);
+	if (ret)
+		goto fail;
+
 	kfree(root_item);
 
 	reloc_root = btrfs_read_tree_root(fs_info->tree_root, &root_key);
-	BUG_ON(IS_ERR(reloc_root));
+	if (IS_ERR(reloc_root))
+		return reloc_root;
 	set_bit(BTRFS_ROOT_SHAREABLE, &reloc_root->state);
 	reloc_root->last_trans = trans->transid;
 	return reloc_root;
+fail:
+	kfree(root_item);
+	return ERR_PTR(ret);
 }
 
 /*
-- 
2.26.2

[PATCH v2 23/42] btrfs: handle btrfs_update_reloc_root failure in commit_fs_roots

[Josef Bacik]

btrfs_update_reloc_root will will return errors in the future, so handle
the error properly in commit_fs_roots.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 0aa6d8ddad21..1dac76b7ea96 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1346,7 +1346,9 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
 			spin_unlock(&fs_info->fs_roots_radix_lock);
 
 			btrfs_free_log(trans, root);
-			btrfs_update_reloc_root(trans, root);
+			err = btrfs_update_reloc_root(trans, root);
+			if (err)
+				return err;
 
 			btrfs_save_ino_cache(root, trans);
 
-- 
2.26.2

[PATCH v2 24/42] btrfs: change insert_dirty_subvol to return errors

[Josef Bacik]

This will be able to return errors in the future, so change it to return
an error and handle the error appropriately.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 2ae1a3816ce5..e3f73ec1476c 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1556,9 +1556,9 @@ static int find_next_key(struct btrfs_path *path, int level,
 /*
  * Insert current subvolume into reloc_control::dirty_subvol_roots
  */
-static void insert_dirty_subvol(struct btrfs_trans_handle *trans,
-				struct reloc_control *rc,
-				struct btrfs_root *root)
+static int insert_dirty_subvol(struct btrfs_trans_handle *trans,
+			       struct reloc_control *rc,
+			       struct btrfs_root *root)
 {
 	struct btrfs_root *reloc_root = root->reloc_root;
 	struct btrfs_root_item *reloc_root_item;
@@ -1578,6 +1578,7 @@ static void insert_dirty_subvol(struct btrfs_trans_handle *trans,
 		btrfs_grab_root(root);
 		list_add_tail(&root->reloc_dirty_list, &rc->dirty_subvol_roots);
 	}
+	return 0;
 }
 
 static int clean_dirty_subvols(struct reloc_control *rc)
@@ -1789,8 +1790,13 @@ static noinline_for_stack int merge_reloc_root(struct reloc_control *rc,
 out:
 	btrfs_free_path(path);
 
-	if (err == 0)
-		insert_dirty_subvol(trans, rc, root);
+	if (err == 0) {
+		ret = insert_dirty_subvol(trans, rc, root);
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			err = ret;
+		}
+	}
 
 	if (trans)
 		btrfs_end_transaction_throttle(trans);
-- 
2.26.2

[PATCH v2 25/42] btrfs: handle btrfs_update_reloc_root failure in insert_dirty_subvol

[Josef Bacik]

btrfs_update_reloc_root will will return errors in the future, so handle
the error properly in insert_dirty_subvol.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index e3f73ec1476c..16deb9e3f764 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1562,6 +1562,7 @@ static int insert_dirty_subvol(struct btrfs_trans_handle *trans,
 {
 	struct btrfs_root *reloc_root = root->reloc_root;
 	struct btrfs_root_item *reloc_root_item;
+	int ret;
 
 	/* @root must be a subvolume tree root with a valid reloc tree */
 	ASSERT(root->root_key.objectid != BTRFS_TREE_RELOC_OBJECTID);
@@ -1572,7 +1573,9 @@ static int insert_dirty_subvol(struct btrfs_trans_handle *trans,
 		sizeof(reloc_root_item->drop_progress));
 	btrfs_set_root_drop_level(reloc_root_item, 0);
 	btrfs_set_root_refs(reloc_root_item, 0);
-	btrfs_update_reloc_root(trans, root);
+	ret = btrfs_update_reloc_root(trans, root);
+	if (ret)
+		return ret;
 
 	if (list_empty(&root->reloc_dirty_list)) {
 		btrfs_grab_root(root);
-- 
2.26.2

[PATCH v2 26/42] btrfs: handle btrfs_update_reloc_root failure in prepare_to_merge

[Josef Bacik]

btrfs_update_reloc_root will will return errors in the future, so handle
an error properly in prepare_to_merge.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 16deb9e3f764..75272ef03486 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1872,10 +1872,21 @@ int prepare_to_merge(struct reloc_control *rc, int err)
 		 */
 		if (!err)
 			btrfs_set_root_refs(&reloc_root->root_item, 1);
-		btrfs_update_reloc_root(trans, root);
+		ret = btrfs_update_reloc_root(trans, root);
 
+		/*
+		 * Even if we have an error we need this reloc root back on our
+		 * list so we can clean up properly.
+		 */
 		list_add(&reloc_root->root_list, &reloc_roots);
 		btrfs_put_root(root);
+
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			if (!err)
+				err = ret;
+			break;
+		}
 	}
 
 	list_splice(&reloc_roots, &rc->reloc_roots);
-- 
2.26.2

[PATCH v2 27/42] btrfs: do proper error handling in btrfs_update_reloc_root

[Josef Bacik]

We call btrfs_update_root in btrfs_update_reloc_root, which can fail for
all sorts of reasons, including IO errors.  Instead of panicing the box
lets return the error, now that all callers properly handle those
errors.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 75272ef03486..ec6228de3ff6 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -894,7 +894,7 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
 	int ret;
 
 	if (!have_reloc_root(root))
-		goto out;
+		return 0;
 
 	reloc_root = root->reloc_root;
 	root_item = &reloc_root->root_item;
@@ -927,10 +927,8 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
 
 	ret = btrfs_update_root(trans, fs_info->tree_root,
 				&reloc_root->root_key, root_item);
-	BUG_ON(ret);
 	btrfs_put_root(reloc_root);
-out:
-	return 0;
+	return ret;
 }
 
 /*
-- 
2.26.2

[PATCH v2 28/42] btrfs: convert logic BUG_ON()'s in replace_path to ASSERT()'s

[Josef Bacik]

A few BUG_ON()'s in replace_path are purely to keep us from making
logical mistakes, so replace them with ASSERT()'s.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index ec6228de3ff6..bb393fa29087 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1202,8 +1202,8 @@ int replace_path(struct btrfs_trans_handle *trans, struct reloc_control *rc,
 	int ret;
 	int slot;
 
-	BUG_ON(src->root_key.objectid != BTRFS_TREE_RELOC_OBJECTID);
-	BUG_ON(dest->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID);
+	ASSERT(src->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID);
+	ASSERT(dest->root_key.objectid != BTRFS_TREE_RELOC_OBJECTID);
 
 	last_snapshot = btrfs_root_last_snapshot(&src->root_item);
 again:
@@ -1234,7 +1234,7 @@ int replace_path(struct btrfs_trans_handle *trans, struct reloc_control *rc,
 	parent = eb;
 	while (1) {
 		level = btrfs_header_level(parent);
-		BUG_ON(level < lowest_level);
+		ASSERT(level >= lowest_level);
 
 		ret = btrfs_bin_search(parent, &key, &slot);
 		if (ret < 0)
-- 
2.26.2

[PATCH v2 29/42] btrfs: handle initial btrfs_cow_block error in replace_path

[Josef Bacik]

If we error out cow'ing the root node when doing a replace_path then we
simply unlock and free the buffer and return the error.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index bb393fa29087..32e183b1d958 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1222,7 +1222,11 @@ int replace_path(struct btrfs_trans_handle *trans, struct reloc_control *rc,
 	if (cow) {
 		ret = btrfs_cow_block(trans, dest, eb, NULL, 0, &eb,
 				      BTRFS_NESTING_COW);
-		BUG_ON(ret);
+		if (ret) {
+			btrfs_tree_unlock(eb);
+			free_extent_buffer(eb);
+			return ret;
+		}
 	}
 
 	if (next_key) {
-- 
2.26.2

[PATCH v2 30/42] btrfs: handle the loop btrfs_cow_block error in replace_path

[Josef Bacik]

As we loop through the path to replace it, we will have to cow each node
we hit on the path down to the lowest_level.  If this fails we simply
unlock and free the block and break from the loop.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 32e183b1d958..3e788b1249d3 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1286,7 +1286,11 @@ int replace_path(struct btrfs_trans_handle *trans, struct reloc_control *rc,
 				ret = btrfs_cow_block(trans, dest, eb, parent,
 						      slot, &eb,
 						      BTRFS_NESTING_COW);
-				BUG_ON(ret);
+				if (ret) {
+					btrfs_tree_unlock(eb);
+					free_extent_buffer(eb);
+					break;
+				}
 			}
 
 			btrfs_tree_unlock(parent);
-- 
2.26.2

[PATCH v2 31/42] btrfs: handle btrfs_search_slot failure in replace_path

[Josef Bacik]

This can fail for any number of reasons, why bring the whole box down
with it?

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 3e788b1249d3..7c7dda11f2aa 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1314,7 +1314,8 @@ int replace_path(struct btrfs_trans_handle *trans, struct reloc_control *rc,
 		path->lowest_level = level;
 		ret = btrfs_search_slot(trans, src, &key, path, 0, 1);
 		path->lowest_level = 0;
-		BUG_ON(ret);
+		if (ret)
+			break;
 
 		/*
 		 * Info qgroup to trace both subtrees.
-- 
2.26.2

[PATCH v2 32/42] btrfs: handle errors in reference count manipulation in replace_path

[Josef Bacik]

If any of the reference count manipulation stuff fails in replace_path
we need to abort the transaction, as we've modified the blocks already.
We can simply break at this point and everything will be cleaned up.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 7c7dda11f2aa..74d52fc457c7 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1355,27 +1355,39 @@ int replace_path(struct btrfs_trans_handle *trans, struct reloc_control *rc,
 		ref.skip_qgroup = true;
 		btrfs_init_tree_ref(&ref, level - 1, src->root_key.objectid);
 		ret = btrfs_inc_extent_ref(trans, &ref);
-		BUG_ON(ret);
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			break;
+		}
 		btrfs_init_generic_ref(&ref, BTRFS_ADD_DELAYED_REF, new_bytenr,
 				       blocksize, 0);
 		ref.skip_qgroup = true;
 		btrfs_init_tree_ref(&ref, level - 1, dest->root_key.objectid);
 		ret = btrfs_inc_extent_ref(trans, &ref);
-		BUG_ON(ret);
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			break;
+		}
 
 		btrfs_init_generic_ref(&ref, BTRFS_DROP_DELAYED_REF, new_bytenr,
 				       blocksize, path->nodes[level]->start);
 		btrfs_init_tree_ref(&ref, level - 1, src->root_key.objectid);
 		ref.skip_qgroup = true;
 		ret = btrfs_free_extent(trans, &ref);
-		BUG_ON(ret);
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			break;
+		}
 
 		btrfs_init_generic_ref(&ref, BTRFS_DROP_DELAYED_REF, old_bytenr,
 				       blocksize, 0);
 		btrfs_init_tree_ref(&ref, level - 1, dest->root_key.objectid);
 		ref.skip_qgroup = true;
 		ret = btrfs_free_extent(trans, &ref);
-		BUG_ON(ret);
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			break;
+		}
 
 		btrfs_unlock_up_safe(path, 0);
 
-- 
2.26.2

[PATCH v2 33/42] btrfs: handle extent reference errors in do_relocation

[Josef Bacik]

We can already deal with errors appropriately from do_relocation, simply
handle any errors that come from changing the refs at this point
cleanly.  We have to abort the transaction if we fail here as we've
modified metadata at this point.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 74d52fc457c7..bc63c4bb4057 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2447,10 +2447,12 @@ static int do_relocation(struct btrfs_trans_handle *trans,
 			btrfs_init_tree_ref(&ref, node->level,
 					    btrfs_header_owner(upper->eb));
 			ret = btrfs_inc_extent_ref(trans, &ref);
-			BUG_ON(ret);
-
-			ret = btrfs_drop_subtree(trans, root, eb, upper->eb);
-			BUG_ON(ret);
+			if (!ret)
+				btrfs_drop_subtree(trans, root, eb, upper->eb);
+			if (ret) {
+				btrfs_abort_transaction(trans, ret);
+				err = ret;
+			}
 		}
 next:
 		if (!upper->pending)
-- 
2.26.2

[PATCH v2 34/42] btrfs: check for BTRFS_BLOCK_FLAG_FULL_BACKREF being set improperly

[Josef Bacik]

We need to validate that a data extent item does not have the
FULL_BACKREF flag set on it's flags.

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

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index d8af62d9f98b..df39ad294aa2 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -1272,6 +1272,11 @@ static int check_extent_item(struct extent_buffer *leaf,
 				   key->offset, fs_info->sectorsize);
 			return -EUCLEAN;
 		}
+		if (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
+			extent_err(leaf, slot,
+			"invalid extent flag, data has full backref set");
+			return -EUCLEAN;
+		}
 	}
 	ptr = (unsigned long)(struct btrfs_extent_item *)(ei + 1);
 
-- 
2.26.2

[PATCH v2 35/42] btrfs: remove the extent item sanity checks in relocate_block_group

[Josef Bacik]

These checks are all taken care of for us by the tree checker code.

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

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index bc63c4bb4057..8a9c946302cd 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3337,20 +3337,6 @@ static void unset_reloc_control(struct reloc_control *rc)
 	mutex_unlock(&fs_info->reloc_mutex);
 }
 
-static int check_extent_flags(u64 flags)
-{
-	if ((flags & BTRFS_EXTENT_FLAG_DATA) &&
-	    (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK))
-		return 1;
-	if (!(flags & BTRFS_EXTENT_FLAG_DATA) &&
-	    !(flags & BTRFS_EXTENT_FLAG_TREE_BLOCK))
-		return 1;
-	if ((flags & BTRFS_EXTENT_FLAG_DATA) &&
-	    (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF))
-		return 1;
-	return 0;
-}
-
 static noinline_for_stack
 int prepare_to_relocate(struct reloc_control *rc)
 {
@@ -3402,7 +3388,6 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 	struct btrfs_path *path;
 	struct btrfs_extent_item *ei;
 	u64 flags;
-	u32 item_size;
 	int ret;
 	int err = 0;
 	int progress = 0;
@@ -3451,19 +3436,7 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 
 		ei = btrfs_item_ptr(path->nodes[0], path->slots[0],
 				    struct btrfs_extent_item);
-		item_size = btrfs_item_size_nr(path->nodes[0], path->slots[0]);
-		if (item_size >= sizeof(*ei)) {
-			flags = btrfs_extent_flags(path->nodes[0], ei);
-			ret = check_extent_flags(flags);
-			BUG_ON(ret);
-		} else if (unlikely(item_size == sizeof(struct btrfs_extent_item_v0))) {
-			err = -EINVAL;
-			btrfs_print_v0_err(trans->fs_info);
-			btrfs_abort_transaction(trans, err);
-			break;
-		} else {
-			BUG();
-		}
+		flags = btrfs_extent_flags(path->nodes[0], ei);
 
 		if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK) {
 			ret = add_tree_block(rc, &key, path, &blocks);
-- 
2.26.2

[PATCH v2 36/42] btrfs: do proper error handling in create_reloc_inode

[Josef Bacik]

We already handle some errors in this function, and the callers do the
correct error handling, so clean up the rest of the function to do the
appropriate error handling.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 8a9c946302cd..c4b6eef70072 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3601,10 +3601,15 @@ struct inode *create_reloc_inode(struct btrfs_fs_info *fs_info,
 		goto out;
 
 	err = __insert_orphan_inode(trans, root, objectid);
-	BUG_ON(err);
+	if (err)
+		goto out;
 
 	inode = btrfs_iget(fs_info->sb, objectid, root);
-	BUG_ON(IS_ERR(inode));
+	if (IS_ERR(inode)) {
+		err = PTR_ERR(inode);
+		inode = NULL;
+		goto out;
+	}
 	BTRFS_I(inode)->index_cnt = group->start;
 
 	err = btrfs_orphan_add(trans, BTRFS_I(inode));
-- 
2.26.2

[PATCH v2 37/42] btrfs: handle __add_reloc_root failure in btrfs_recover_relocation

[Josef Bacik]

We can already handle errors appropriately from this function, deal with
an error coming from __add_reloc_root appropriately.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index c4b6eef70072..e2994fb15f2d 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3951,7 +3951,12 @@ int btrfs_recover_relocation(struct btrfs_root *root)
 		}
 
 		err = __add_reloc_root(reloc_root);
-		BUG_ON(err < 0); /* -ENOMEM or logic error */
+		if (err) {
+			list_add_tail(&reloc_root->root_list, &reloc_roots);
+			btrfs_put_root(fs_root);
+			btrfs_end_transaction(trans);
+			goto out_unset;
+		}
 		fs_root->reloc_root = btrfs_grab_root(reloc_root);
 		btrfs_put_root(fs_root);
 	}
-- 
2.26.2

[PATCH v2 38/42] btrfs: handle __add_reloc_root failure in btrfs_reloc_post_snapshot

[Josef Bacik]

If we fail to add the reloc root, drop it and return the error.  All
callers of this function already handle errors appropriately.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index e2994fb15f2d..01dbcdc86cf6 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4171,7 +4171,10 @@ int btrfs_reloc_post_snapshot(struct btrfs_trans_handle *trans,
 		return PTR_ERR(reloc_root);
 
 	ret = __add_reloc_root(reloc_root);
-	BUG_ON(ret < 0);
+	if (ret) {
+		btrfs_put_root(reloc_root);
+		return ret;
+	}
 	new_root->reloc_root = btrfs_grab_root(reloc_root);
 
 	if (rc->create_reloc_tree)
-- 
2.26.2

[PATCH v2 39/42] btrfs: cleanup error handling in prepare_to_merge

[Josef Bacik]

This probably can't happen even with a corrupt file system, because we
would have failed much earlier on than here.  However there's no reason
we can't just check and bail out as appropriate, so do that and convert
the correctness BUG_ON() to an ASSERT().

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 01dbcdc86cf6..9baff1a60ce3 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1882,8 +1882,14 @@ int prepare_to_merge(struct reloc_control *rc, int err)
 
 		root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset,
 				false);
-		BUG_ON(IS_ERR(root));
-		BUG_ON(root->reloc_root != reloc_root);
+		if (IS_ERR(root)) {
+			list_add(&reloc_root->root_list, &reloc_roots);
+			btrfs_abort_transaction(trans, (int)PTR_ERR(root));
+			if (!err)
+				err = PTR_ERR(root);
+			break;
+		}
+		ASSERT(root->reloc_root == reloc_root);
 
 		/*
 		 * set reference count to 1, so btrfs_recover_relocation
-- 
2.26.2

[PATCH v2 40/42] btrfs: handle extent corruption with select_one_root properly

[Josef Bacik]

In corruption cases we could have paths from a block up to no root at
all, and thus we'll BUG_ON(!root) in select_one_root.  Handle this by
adding an ASSERT() for developers, and returning an error for normal
users.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 9baff1a60ce3..12b4955f2ab2 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2205,7 +2205,16 @@ struct btrfs_root *select_one_root(struct btrfs_backref_node *node)
 		cond_resched();
 		next = walk_up_backref(next, edges, &index);
 		root = next->root;
-		BUG_ON(!root);
+
+		/*
+		 * This can occur if we have incomplete extent refs leading all
+		 * the way up a particular path, in this case return -EUCLEAN.
+		 * However leave as an ASSERT() for developers, because it could
+		 * indicate a bug in the backref code.
+		 */
+		ASSERT(root);
+		if (!root)
+			return ERR_PTR(-EUCLEAN);
 
 		/* No other choice for non-shareable tree */
 		if (!test_bit(BTRFS_ROOT_SHAREABLE, &root->state))
@@ -2613,8 +2622,12 @@ static int relocate_tree_block(struct btrfs_trans_handle *trans,
 
 	BUG_ON(node->processed);
 	root = select_one_root(node);
-	if (root == ERR_PTR(-ENOENT)) {
-		update_processed_blocks(rc, node);
+	if (IS_ERR(root)) {
+		ret = PTR_ERR(root);
+		if (ret == -ENOENT) {
+			ret = 0;
+			update_processed_blocks(rc, node);
+		}
 		goto out;
 	}
 
-- 
2.26.2

[PATCH v2 41/42] btrfs: do proper error handling in merge_reloc_roots

[Josef Bacik]

We have a BUG_ON() if we get an error back from btrfs_get_fs_root().
This honestly should never fail, as at this point we have a solid
coordination of fs root to reloc root, and these roots will all be in
memory.  But in the name of killing BUG_ON()'s remove this one and
handle the error properly.  Change the remaining BUG_ON() to an
ASSERT().

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 12b4955f2ab2..97c1d967b528 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1961,9 +1961,18 @@ void merge_reloc_roots(struct reloc_control *rc)
 
 		root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset,
 					 false);
+		if (IS_ERR(root)) {
+			/*
+			 * This likely won't happen, since we would have failed
+			 * at a higher level.  However for correctness sake
+			 * handle the error anyway.
+			 */
+			ret = PTR_ERR(root);
+			goto out;
+		}
+
 		if (btrfs_root_refs(&reloc_root->root_item) > 0) {
-			BUG_ON(IS_ERR(root));
-			BUG_ON(root->reloc_root != reloc_root);
+			ASSERT(root->reloc_root == reloc_root);
 			ret = merge_reloc_root(rc, root);
 			btrfs_put_root(root);
 			if (ret) {
-- 
2.26.2

[PATCH v2 42/42] btrfs: check return value of btrfs_commit_transaction in relocation

[Josef Bacik]

There's a few places where we don't check the return value of
btrfs_commit_transaction in relocation.c.  Thankfully all these places
have straightforward error handling, so simply change all of the sites
at once.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 97c1d967b528..8b17827de608 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1917,7 +1917,7 @@ int prepare_to_merge(struct reloc_control *rc, int err)
 	list_splice(&reloc_roots, &rc->reloc_roots);
 
 	if (!err)
-		btrfs_commit_transaction(trans);
+		err = btrfs_commit_transaction(trans);
 	else
 		btrfs_end_transaction(trans);
 	return err;
@@ -3403,8 +3403,7 @@ int prepare_to_relocate(struct reloc_control *rc)
 		 */
 		return PTR_ERR(trans);
 	}
-	btrfs_commit_transaction(trans);
-	return 0;
+	return btrfs_commit_transaction(trans);
 }
 
 static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
@@ -3563,7 +3562,9 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 		err = PTR_ERR(trans);
 		goto out_free;
 	}
-	btrfs_commit_transaction(trans);
+	ret = btrfs_commit_transaction(trans);
+	if (ret && !err)
+		err = ret;
 out_free:
 	ret = clean_dirty_subvols(rc);
 	if (ret < 0 && !err)
-- 
2.26.2

Re: [PATCH v2 02/42] btrfs: fix lockdep splat in btrfs_recover_relocation

[Nikolay Borisov]

On 13.11.20 г. 18:22 ч., Josef Bacik wrote:
> While testing the error paths of relocation I hit the following lockdep
> splat
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.10.0-rc2-btrfs-next-71 #1 Not tainted
> ------------------------------------------------------
> find/324157 is trying to acquire lock:
> ffff8ebc48d293a0 (btrfs-tree-01#2/3){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
> 
> but task is already holding lock:
> ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
> 
> which lock already depends on the new lock.
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (btrfs-tree-00){++++}-{3:3}:
>        lock_acquire+0xd8/0x490
>        down_write_nested+0x44/0x120
>        __btrfs_tree_lock+0x27/0x120 [btrfs]
>        btrfs_search_slot+0x2a3/0xc50 [btrfs]
>        btrfs_insert_empty_items+0x58/0xa0 [btrfs]
>        insert_with_overflow+0x44/0x110 [btrfs]
>        btrfs_insert_xattr_item+0xb8/0x1d0 [btrfs]
>        btrfs_setxattr+0xd6/0x4c0 [btrfs]
>        btrfs_setxattr_trans+0x68/0x100 [btrfs]
>        __vfs_setxattr+0x66/0x80
>        __vfs_setxattr_noperm+0x70/0x200
>        vfs_setxattr+0x6b/0x120
>        setxattr+0x125/0x240
>        path_setxattr+0xba/0xd0
>        __x64_sys_setxattr+0x27/0x30
>        do_syscall_64+0x33/0x80
>        entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> -> #0 (btrfs-tree-01#2/3){++++}-{3:3}:
>        check_prev_add+0x91/0xc60
>        __lock_acquire+0x1689/0x3130
>        lock_acquire+0xd8/0x490
>        down_read_nested+0x45/0x220
>        __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>        btrfs_next_old_leaf+0x27d/0x580 [btrfs]
>        btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
>        iterate_dir+0x170/0x1c0
>        __x64_sys_getdents64+0x83/0x140
>        do_syscall_64+0x33/0x80
>        entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> other info that might help us debug this:
> 
>  Possible unsafe locking scenario:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(btrfs-tree-00);
>                                lock(btrfs-tree-01#2/3);
>                                lock(btrfs-tree-00);
>   lock(btrfs-tree-01#2/3);
> 
>  *** DEADLOCK ***
> 
> 5 locks held by find/324157:
>  #0: ffff8ebc502c6e00 (&f->f_pos_lock){+.+.}-{3:3}, at: __fdget_pos+0x4d/0x60
>  #1: ffff8eb97f689980 (&type->i_mutex_dir_key#10){++++}-{3:3}, at: iterate_dir+0x52/0x1c0
>  #2: ffff8ebaec00ca58 (btrfs-tree-02#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  #3: ffff8eb98f986f78 (btrfs-tree-01#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  #4: ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
> 
> stack backtrace:
> CPU: 2 PID: 324157 Comm: find Not tainted 5.10.0-rc2-btrfs-next-71 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> Call Trace:
>  dump_stack+0x8d/0xb5
>  check_noncircular+0xff/0x110
>  ? mark_lock.part.0+0x468/0xe90
>  check_prev_add+0x91/0xc60
>  __lock_acquire+0x1689/0x3130
>  ? kvm_clock_read+0x14/0x30
>  ? kvm_sched_clock_read+0x5/0x10
>  lock_acquire+0xd8/0x490
>  ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  down_read_nested+0x45/0x220
>  ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  btrfs_next_old_leaf+0x27d/0x580 [btrfs]
>  btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
>  iterate_dir+0x170/0x1c0
>  __x64_sys_getdents64+0x83/0x140
>  ? filldir+0x1d0/0x1d0
>  do_syscall_64+0x33/0x80
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> This is thankfully straightforward to fix, simply release the path
> before we setup the reloc_ctl.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

So you are changing btrfs_recover_balance yet nowhere in the stack
traces provided does this functino persist, instead the problem seems to
be due to the way btrfs_real_readdir does its locking. I'm confused.

> ---
>  fs/btrfs/volumes.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb1aa96e1233..ece8bb62fcc1 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4283,6 +4283,8 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
>  		btrfs_warn(fs_info,
>  	"balance: cannot set exclusive op status, resume manually");
>  
> +	btrfs_release_path(path);
> +
>  	mutex_lock(&fs_info->balance_mutex);
>  	BUG_ON(fs_info->balance_ctl);
>  	spin_lock(&fs_info->balance_lock);
> 

Re: [PATCH v2 05/42] btrfs: return an error from btrfs_record_root_in_trans

[Nikolay Borisov]

On 13.11.20 г. 18:22 ч., Josef Bacik wrote:
> We can create a reloc root when we record the root in the trans, which
> can fail for all sorts of different reasons.  Propagate this error up

It seems it can only fail due to a single reason and that being -ENOMEM
in __add_reloc_root (see below), because create_reloc_root BUGS on error.

> the chain of callers.  Future patches will fix the callers of
> btrfs_record_root_in_trans() to handle the error.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/transaction.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 0e4063651047..fab26241fb2e 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -403,6 +403,7 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
>  			       int force)
>  {
>  	struct btrfs_fs_info *fs_info = root->fs_info;
> +	int ret = 0;
>  
>  	if ((test_bit(BTRFS_ROOT_SHAREABLE, &root->state) &&
>  	    root->last_trans < trans->transid) || force) {
> @@ -451,11 +452,11 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
>  		 * lock.  smp_wmb() makes sure that all the writes above are
>  		 * done before we pop in the zero below
>  		 */
> -		btrfs_init_reloc_root(trans, root);
> +		ret = btrfs_init_reloc_root(trans, root);

In order for this value to have any effect btrfs_init_reloc_root ought
to also be changed because it either always returns 0 or BUG_ON() on
-ENOMEM from __ad_reloc_root.

<snip>

Re: [PATCH v2 13/42] btrfs: handle btrfs_record_root_in_trans failure in btrfs_recover_log_trees

[Nikolay Borisov]

On 13.11.20 г. 18:23 ч., Josef Bacik wrote:
> btrfs_record_root_in_trans will return errors in the future, so handle
> the error properly in btrfs_recover_log_trees.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/tree-log.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 955c9a36cfeb..1ad77e2399f7 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -6276,8 +6276,12 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>  		}
>  
>  		wc.replay_dest->log_root = log;
> -		btrfs_record_root_in_trans(trans, wc.replay_dest);
> -		ret = walk_log_tree(trans, log, &wc);
> +		ret = btrfs_record_root_in_trans(trans, wc.replay_dest);
> +		if (ret)
> +			btrfs_handle_fs_error(fs_info, ret,
> +				"Couldn't record the root in the transaction.");
> +		else
> +			ret = walk_log_tree(trans, log, &wc);

After handle_fs_error the filesystem is in RO state so in case of error
simply call the function and goto error?


>  
>  		if (!ret && wc.stage == LOG_WALK_REPLAY_ALL) {
>  			ret = fixup_inode_link_counts(trans, wc.replay_dest,
> 

Re: [PATCH v2 14/42] btrfs: handle btrfs_record_root_in_trans failure in create_subvol

[Nikolay Borisov]

On 13.11.20 г. 18:23 ч., Josef Bacik wrote:
> btrfs_record_root_in_trans will return errors in the future, so handle
> the error properly in create_subvol.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/ioctl.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index a5dc7cc5d705..da9026a487d2 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -702,7 +702,11 @@ static noinline int create_subvol(struct inode *dir,
>  	/* Freeing will be done in btrfs_put_root() of new_root */
>  	anon_dev = 0;
>  
> -	btrfs_record_root_in_trans(trans, new_root);
> +	ret = btrfs_record_root_in_trans(trans, new_root);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		goto fail;
> +	}

I think create_subvol is broken w.r.t handling of anon_bdev when an
error occurs since it's not being freed in the "goto fail" case.

>  
>  	ret = btrfs_create_subvol_root(trans, new_root, root, new_dirid);
>  	btrfs_put_root(new_root);
> 

Re: [PATCH v2 20/42] btrfs: do not panic in __add_reloc_root

[Nikolay Borisov]

On 13.11.20 г. 18:23 ч., Josef Bacik wrote:
> If we have a duplicate entry for a reloc root then we could have fs
> corruption that resulted in a double allocation.  This shouldn't happen
> generally so leave an ASSERT() for this case, but return an error
> instead of panicing in the normal user case

nit: panicing => panicking - codespell caught it :)

Re: [PATCH v2 05/42] btrfs: return an error from btrfs_record_root_in_trans

[Nikolay Borisov]

On 24.11.20 г. 13:02 ч., Nikolay Borisov wrote:
> effect btrfs_init_reloc_root ought
> to also be changed because it either always returns 0 or BUG_ON() on
> -ENOMEM from __ad_reloc_root.

Ok, you do the necessary changes in patch 21, so disregard this.

Re: [PATCH v2 33/42] btrfs: handle extent reference errors in do_relocation

[Nikolay Borisov]

On 13.11.20 г. 18:23 ч., Josef Bacik wrote:
> We can already deal with errors appropriately from do_relocation, simply
> handle any errors that come from changing the refs at this point
> cleanly.  We have to abort the transaction if we fail here as we've
> modified metadata at this point.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/relocation.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 74d52fc457c7..bc63c4bb4057 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -2447,10 +2447,12 @@ static int do_relocation(struct btrfs_trans_handle *trans,
>  			btrfs_init_tree_ref(&ref, node->level,
>  					    btrfs_header_owner(upper->eb));
>  			ret = btrfs_inc_extent_ref(trans, &ref);
> -			BUG_ON(ret);
> -
> -			ret = btrfs_drop_subtree(trans, root, eb, upper->eb);
> -			BUG_ON(ret);
> +			if (!ret)
> +				btrfs_drop_subtree(trans, root, eb, upper->eb);
> +			if (ret) {
> +				btrfs_abort_transaction(trans, ret);
> +				err = ret;
> +			}

nit: I'd prefer this: 

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index bc63c4bb4057..ce063c83d337 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2447,12 +2447,12 @@ static int do_relocation(struct btrfs_trans_handle *trans,
                        btrfs_init_tree_ref(&ref, node->level,
                                            btrfs_header_owner(upper->eb));
                        ret = btrfs_inc_extent_ref(trans, &ref);
-                       if (!ret)
-                               btrfs_drop_subtree(trans, root, eb, upper->eb);
                        if (ret) {
                                btrfs_abort_transaction(trans, ret);
                                err = ret;
+                               goto next;
                        }
+                       btrfs_drop_subtree(trans, root, eb, upper->eb);

One less conditional statement to worry about

>  		}
>  next:
>  		if (!upper->pending)
> 

Re: [PATCH v2 37/42] btrfs: handle __add_reloc_root failure in btrfs_recover_relocation

[Nikolay Borisov]

On 13.11.20 г. 18:23 ч., Josef Bacik wrote:
> We can already handle errors appropriately from this function, deal with
> an error coming from __add_reloc_root appropriately.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/relocation.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index c4b6eef70072..e2994fb15f2d 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -3951,7 +3951,12 @@ int btrfs_recover_relocation(struct btrfs_root *root)
>  		}
>  
>  		err = __add_reloc_root(reloc_root);
> -		BUG_ON(err < 0); /* -ENOMEM or logic error */
> +		if (err) {
> +			list_add_tail(&reloc_root->root_list, &reloc_roots);
> +			btrfs_put_root(fs_root);

Do you need to do the the put_root, since
free_reloc_roots->__del_reloc_root->if (!list_empty(&root->root_list))
will set put_ref to true and put another reference?

> +			btrfs_end_transaction(trans);
> +			goto out_unset;
> +		}
>  		fs_root->reloc_root = btrfs_grab_root(reloc_root);
>  		btrfs_put_root(fs_root);
>  	}
> 

Re: [PATCH v2 02/42] btrfs: fix lockdep splat in btrfs_recover_relocation

[Filipe Manana]

On Fri, Nov 13, 2020 at 4:25 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> While testing the error paths of relocation I hit the following lockdep
> splat

The lockdep splat has a kernel named exactly like mine: *-btrfs-next-71 :)

>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.10.0-rc2-btrfs-next-71 #1 Not tainted
> ------------------------------------------------------
> find/324157 is trying to acquire lock:
> ffff8ebc48d293a0 (btrfs-tree-01#2/3){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>
> but task is already holding lock:
> ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (btrfs-tree-00){++++}-{3:3}:
>        lock_acquire+0xd8/0x490
>        down_write_nested+0x44/0x120
>        __btrfs_tree_lock+0x27/0x120 [btrfs]
>        btrfs_search_slot+0x2a3/0xc50 [btrfs]
>        btrfs_insert_empty_items+0x58/0xa0 [btrfs]
>        insert_with_overflow+0x44/0x110 [btrfs]
>        btrfs_insert_xattr_item+0xb8/0x1d0 [btrfs]
>        btrfs_setxattr+0xd6/0x4c0 [btrfs]
>        btrfs_setxattr_trans+0x68/0x100 [btrfs]
>        __vfs_setxattr+0x66/0x80
>        __vfs_setxattr_noperm+0x70/0x200
>        vfs_setxattr+0x6b/0x120
>        setxattr+0x125/0x240
>        path_setxattr+0xba/0xd0
>        __x64_sys_setxattr+0x27/0x30
>        do_syscall_64+0x33/0x80
>        entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> -> #0 (btrfs-tree-01#2/3){++++}-{3:3}:
>        check_prev_add+0x91/0xc60
>        __lock_acquire+0x1689/0x3130
>        lock_acquire+0xd8/0x490
>        down_read_nested+0x45/0x220
>        __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>        btrfs_next_old_leaf+0x27d/0x580 [btrfs]
>        btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
>        iterate_dir+0x170/0x1c0
>        __x64_sys_getdents64+0x83/0x140
>        do_syscall_64+0x33/0x80
>        entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> other info that might help us debug this:
>
>  Possible unsafe locking scenario:
>
>        CPU0                    CPU1
>        ----                    ----
>   lock(btrfs-tree-00);
>                                lock(btrfs-tree-01#2/3);
>                                lock(btrfs-tree-00);
>   lock(btrfs-tree-01#2/3);
>
>  *** DEADLOCK ***
>
> 5 locks held by find/324157:
>  #0: ffff8ebc502c6e00 (&f->f_pos_lock){+.+.}-{3:3}, at: __fdget_pos+0x4d/0x60
>  #1: ffff8eb97f689980 (&type->i_mutex_dir_key#10){++++}-{3:3}, at: iterate_dir+0x52/0x1c0
>  #2: ffff8ebaec00ca58 (btrfs-tree-02#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  #3: ffff8eb98f986f78 (btrfs-tree-01#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  #4: ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>
> stack backtrace:
> CPU: 2 PID: 324157 Comm: find Not tainted 5.10.0-rc2-btrfs-next-71 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> Call Trace:
>  dump_stack+0x8d/0xb5
>  check_noncircular+0xff/0x110
>  ? mark_lock.part.0+0x468/0xe90
>  check_prev_add+0x91/0xc60
>  __lock_acquire+0x1689/0x3130
>  ? kvm_clock_read+0x14/0x30
>  ? kvm_sched_clock_read+0x5/0x10
>  lock_acquire+0xd8/0x490
>  ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  down_read_nested+0x45/0x220
>  ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  btrfs_next_old_leaf+0x27d/0x580 [btrfs]
>  btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
>  iterate_dir+0x170/0x1c0
>  __x64_sys_getdents64+0x83/0x140
>  ? filldir+0x1d0/0x1d0
>  do_syscall_64+0x33/0x80
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> This is thankfully straightforward to fix, simply release the path
> before we setup the reloc_ctl.

Ok, so that splat is exactly what I reported not long ago and is
already fixed by:

https://lore.kernel.org/linux-btrfs/36b861f262858990f84eda72da6bb2e6762c41b7.1604697895.git.josef@toxicpanda.com/#r

Which is the splat that happened in one of my test boxes.

So, have you pasted the wrong splat?
Did it happen with any existing test case from fstests, if so, which?
That one I reported was with btrfs/187 (worth mentioning in the
changelog).

Thanks.

>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/volumes.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb1aa96e1233..ece8bb62fcc1 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4283,6 +4283,8 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
>                 btrfs_warn(fs_info,
>         "balance: cannot set exclusive op status, resume manually");
>
> +       btrfs_release_path(path);
> +
>         mutex_lock(&fs_info->balance_mutex);
>         BUG_ON(fs_info->balance_ctl);
>         spin_lock(&fs_info->balance_lock);
> --
> 2.26.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2 02/42] btrfs: fix lockdep splat in btrfs_recover_relocation

[Josef Bacik]

On 11/24/20 11:56 AM, Filipe Manana wrote:
> On Fri, Nov 13, 2020 at 4:25 PM Josef Bacik <josef@toxicpanda.com> wrote:
>>
>> While testing the error paths of relocation I hit the following lockdep
>> splat
> 
> The lockdep splat has a kernel named exactly like mine: *-btrfs-next-71 :)
> 
>>
>> ======================================================
>> WARNING: possible circular locking dependency detected
>> 5.10.0-rc2-btrfs-next-71 #1 Not tainted
>> ------------------------------------------------------
>> find/324157 is trying to acquire lock:
>> ffff8ebc48d293a0 (btrfs-tree-01#2/3){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>>
>> but task is already holding lock:
>> ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>>
>> which lock already depends on the new lock.
>>
>> the existing dependency chain (in reverse order) is:
>>
>> -> #1 (btrfs-tree-00){++++}-{3:3}:
>>         lock_acquire+0xd8/0x490
>>         down_write_nested+0x44/0x120
>>         __btrfs_tree_lock+0x27/0x120 [btrfs]
>>         btrfs_search_slot+0x2a3/0xc50 [btrfs]
>>         btrfs_insert_empty_items+0x58/0xa0 [btrfs]
>>         insert_with_overflow+0x44/0x110 [btrfs]
>>         btrfs_insert_xattr_item+0xb8/0x1d0 [btrfs]
>>         btrfs_setxattr+0xd6/0x4c0 [btrfs]
>>         btrfs_setxattr_trans+0x68/0x100 [btrfs]
>>         __vfs_setxattr+0x66/0x80
>>         __vfs_setxattr_noperm+0x70/0x200
>>         vfs_setxattr+0x6b/0x120
>>         setxattr+0x125/0x240
>>         path_setxattr+0xba/0xd0
>>         __x64_sys_setxattr+0x27/0x30
>>         do_syscall_64+0x33/0x80
>>         entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> -> #0 (btrfs-tree-01#2/3){++++}-{3:3}:
>>         check_prev_add+0x91/0xc60
>>         __lock_acquire+0x1689/0x3130
>>         lock_acquire+0xd8/0x490
>>         down_read_nested+0x45/0x220
>>         __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>>         btrfs_next_old_leaf+0x27d/0x580 [btrfs]
>>         btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
>>         iterate_dir+0x170/0x1c0
>>         __x64_sys_getdents64+0x83/0x140
>>         do_syscall_64+0x33/0x80
>>         entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> other info that might help us debug this:
>>
>>   Possible unsafe locking scenario:
>>
>>         CPU0                    CPU1
>>         ----                    ----
>>    lock(btrfs-tree-00);
>>                                 lock(btrfs-tree-01#2/3);
>>                                 lock(btrfs-tree-00);
>>    lock(btrfs-tree-01#2/3);
>>
>>   *** DEADLOCK ***
>>
>> 5 locks held by find/324157:
>>   #0: ffff8ebc502c6e00 (&f->f_pos_lock){+.+.}-{3:3}, at: __fdget_pos+0x4d/0x60
>>   #1: ffff8eb97f689980 (&type->i_mutex_dir_key#10){++++}-{3:3}, at: iterate_dir+0x52/0x1c0
>>   #2: ffff8ebaec00ca58 (btrfs-tree-02#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>>   #3: ffff8eb98f986f78 (btrfs-tree-01#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>>   #4: ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>>
>> stack backtrace:
>> CPU: 2 PID: 324157 Comm: find Not tainted 5.10.0-rc2-btrfs-next-71 #1
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
>> Call Trace:
>>   dump_stack+0x8d/0xb5
>>   check_noncircular+0xff/0x110
>>   ? mark_lock.part.0+0x468/0xe90
>>   check_prev_add+0x91/0xc60
>>   __lock_acquire+0x1689/0x3130
>>   ? kvm_clock_read+0x14/0x30
>>   ? kvm_sched_clock_read+0x5/0x10
>>   lock_acquire+0xd8/0x490
>>   ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>>   down_read_nested+0x45/0x220
>>   ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>>   __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>>   btrfs_next_old_leaf+0x27d/0x580 [btrfs]
>>   btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
>>   iterate_dir+0x170/0x1c0
>>   __x64_sys_getdents64+0x83/0x140
>>   ? filldir+0x1d0/0x1d0
>>   do_syscall_64+0x33/0x80
>>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> This is thankfully straightforward to fix, simply release the path
>> before we setup the reloc_ctl.
> 
> Ok, so that splat is exactly what I reported not long ago and is
> already fixed by:
> 
> https://lore.kernel.org/linux-btrfs/36b861f262858990f84eda72da6bb2e6762c41b7.1604697895.git.josef@toxicpanda.com/#r
> 
> Which is the splat that happened in one of my test boxes.
> 
> So, have you pasted the wrong splat?
> Did it happen with any existing test case from fstests, if so, which?
> That one I reported was with btrfs/187 (worth mentioning in the
> changelog).

Lol I pasted the wrong splat, I forgot to scp it from the vm that had 
the problem, so I just pulled the last one I had fixed.  It didn't 
happen with an xfstests testcase, it happened while doing error 
injection testing.  I'll try and reproduce so I get the real splat, my 
bad.  Thanks,

Josef

Re: [PATCH v2 13/42] btrfs: handle btrfs_record_root_in_trans failure in btrfs_recover_log_trees

[Josef Bacik]

On 11/24/20 7:37 AM, Nikolay Borisov wrote:
> 
> 
> On 13.11.20 г. 18:23 ч., Josef Bacik wrote:
>> btrfs_record_root_in_trans will return errors in the future, so handle
>> the error properly in btrfs_recover_log_trees.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/tree-log.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>> index 955c9a36cfeb..1ad77e2399f7 100644
>> --- a/fs/btrfs/tree-log.c
>> +++ b/fs/btrfs/tree-log.c
>> @@ -6276,8 +6276,12 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>>   		}
>>   
>>   		wc.replay_dest->log_root = log;
>> -		btrfs_record_root_in_trans(trans, wc.replay_dest);
>> -		ret = walk_log_tree(trans, log, &wc);
>> +		ret = btrfs_record_root_in_trans(trans, wc.replay_dest);
>> +		if (ret)
>> +			btrfs_handle_fs_error(fs_info, ret,
>> +				"Couldn't record the root in the transaction.");
>> +		else
>> +			ret = walk_log_tree(trans, log, &wc);
> 
> After handle_fs_error the filesystem is in RO state so in case of error
> simply call the function and goto error?

We are holding a ref on the destination root, so we have to do this in order to 
make sure the cleanup is done properly.  I'll note this in the commit log.  Thanks,

Josef

Re: [PATCH v2 14/42] btrfs: handle btrfs_record_root_in_trans failure in create_subvol

[Josef Bacik]

On 11/24/20 7:42 AM, Nikolay Borisov wrote:
> 
> 
> On 13.11.20 г. 18:23 ч., Josef Bacik wrote:
>> btrfs_record_root_in_trans will return errors in the future, so handle
>> the error properly in create_subvol.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/ioctl.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index a5dc7cc5d705..da9026a487d2 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -702,7 +702,11 @@ static noinline int create_subvol(struct inode *dir,
>>   	/* Freeing will be done in btrfs_put_root() of new_root */
>>   	anon_dev = 0;
>>   
>> -	btrfs_record_root_in_trans(trans, new_root);
>> +	ret = btrfs_record_root_in_trans(trans, new_root);
>> +	if (ret) {
>> +		btrfs_abort_transaction(trans, ret);
>> +		goto fail;
>> +	}
> 
> I think create_subvol is broken w.r.t handling of anon_bdev when an
> error occurs since it's not being freed in the "goto fail" case.
> 

The comment above addresses this, it'll be cleared at btrfs_put_root() time. 
Thanks,

Josef

Re: [PATCH v2 37/42] btrfs: handle __add_reloc_root failure in btrfs_recover_relocation

[Josef Bacik]

On 11/24/20 8:26 AM, Nikolay Borisov wrote:
> 
> 
> On 13.11.20 г. 18:23 ч., Josef Bacik wrote:
>> We can already handle errors appropriately from this function, deal with
>> an error coming from __add_reloc_root appropriately.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/relocation.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index c4b6eef70072..e2994fb15f2d 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -3951,7 +3951,12 @@ int btrfs_recover_relocation(struct btrfs_root *root)
>>   		}
>>   
>>   		err = __add_reloc_root(reloc_root);
>> -		BUG_ON(err < 0); /* -ENOMEM or logic error */
>> +		if (err) {
>> +			list_add_tail(&reloc_root->root_list, &reloc_roots);
>> +			btrfs_put_root(fs_root);
> 
> Do you need to do the the put_root, since
> free_reloc_roots->__del_reloc_root->if (!list_empty(&root->root_list))
> will set put_ref to true and put another reference?

Yes this is for the corresponding fs_root, not the reloc root.  Thanks,

Josef