[PATCH 0/7] Error handling fixes

[PATCH 0/7] Error handling fixes

[Josef Bacik]

Hello,

For a short period of time our btrfs backport had 947a629988f1 ("btrfs: move
tree block parentness check into validate_extent_buffer()") without the
associated fix, which resulted in a lot of hilarity.

One of the things that popped was a WARN_ON(ret == 1) in __btrfs_free_extent
where we didn't find the bytenr we were looking for.  This was troubling, as it
appeared that we were losing the EIO and returning 1 from btrfs_search_slot.

I rigged up my error injection stress test with
btrfs_check_leaf/btrfs_check_node with balance (as this was the path that we saw
the error).  This of course uncovered a few other unrelated things, but
eventually I reproduced what we saw in production.  Thankfully it was not that
we were eating the -EIO and returning 1 instead, however the actual problem is
worse.  We do not handle the errors properly in snapshot delete (which also gets
used by reloation), and then we do not abort the transaction when we hit errors
in this path, which leads to the file system being corrupted and eventually
triggers the above WARN_ON().

With these fixes in place my stress testing was running overnight without
tripping over any other leaks, corruptions, or panics.  Previously I wasn't able
to run for longer than a couple of minutes without falling over.  Thanks,

Josef

Josef Bacik (7):
  btrfs: use btrfs_handle_fs_error in btrfs_fill_super
  btrfs: replace BUG_ON(level == 0) with ASSERT(level)
  btrfs: handle errors from btrfs_read_node_slot in split
  btrfs: iput on orphan cleanup failure
  btrfs: drop root refs properly when orphan cleanup fails
  btrfs: handle errors in walk_down_tree properly
  btrfs: abort the transaction if we get an error during snapshot drop

 fs/btrfs/ctree.c       | 55 +++++++++++++++++++++---------------------
 fs/btrfs/disk-io.c     |  4 +--
 fs/btrfs/extent-tree.c | 10 +++++---
 fs/btrfs/inode.c       |  5 +++-
 fs/btrfs/super.c       |  1 +
 5 files changed, 40 insertions(+), 35 deletions(-)

-- 
2.26.3

[PATCH 1/7] btrfs: use btrfs_handle_fs_error in btrfs_fill_super

[Josef Bacik]

While trying to track down a lost EIO problem I hit the following
assertion while doing my error injection testing

BTRFS warning (device nvme1n1): transaction 1609 (with 180224 dirty metadata bytes) is not committed
assertion failed: !found, in fs/btrfs/disk-io.c:4456
------------[ cut here ]------------
kernel BUG at fs/btrfs/messages.h:169!
invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
CPU: 0 PID: 1445 Comm: mount Tainted: G        W          6.2.0-rc5+ #3
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.1-2.fc37 04/01/2014
RIP: 0010:btrfs_assertfail.constprop.0+0x18/0x1a
RSP: 0018:ffffb95fc3b0bc68 EFLAGS: 00010286
RAX: 0000000000000034 RBX: ffff9941c2ac2000 RCX: 0000000000000000
RDX: 0000000000000001 RSI: ffffffffb6741f7d RDI: 00000000ffffffff
RBP: ffff9941c2ac2428 R08: 0000000000000000 R09: ffffb95fc3b0bb38
R10: 0000000000000003 R11: ffffffffb71438a8 R12: ffff9941c2ac2428
R13: ffff9941c2ac2450 R14: ffff9941c2ac2450 R15: 000000000002c000
FS:  00007fcea2d07800(0000) GS:ffff9941fbc00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f00cc7c83a8 CR3: 000000010c686000 CR4: 0000000000350ef0
Call Trace:
 <TASK>
 close_ctree+0x426/0x48f
 btrfs_mount_root.cold+0x7e/0xee
 ? legacy_parse_param+0x2b/0x220
 legacy_get_tree+0x2b/0x50
 vfs_get_tree+0x29/0xc0
 vfs_kern_mount.part.0+0x73/0xb0
 btrfs_mount+0x11d/0x3d0
 ? legacy_parse_param+0x2b/0x220
 legacy_get_tree+0x2b/0x50
 vfs_get_tree+0x29/0xc0
 path_mount+0x438/0xa40
 __x64_sys_mount+0xe9/0x130
 do_syscall_64+0x3e/0x90
 entry_SYSCALL_64_after_hwframe+0x72/0xdc

This is because the error injection did an EIO for the root inode lookup
and we simply jumped to closing the ctree.  However because we didn't
mark the file system as having an error we skipped all of the broken
transaction cleanup stuff, and thus triggered this ASSERT().  Fix this
by calling btrfs_handle_fs_error() in this case so we have the error set
on the file system.

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

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 581845bc206a..d8885966e801 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1158,6 +1158,7 @@ static int btrfs_fill_super(struct super_block *sb,
 	inode = btrfs_iget(sb, BTRFS_FIRST_FREE_OBJECTID, fs_info->fs_root);
 	if (IS_ERR(inode)) {
 		err = PTR_ERR(inode);
+		btrfs_handle_fs_error(fs_info, err, NULL);
 		goto fail_close;
 	}
 
-- 
2.26.3

Re: [PATCH 1/7] btrfs: use btrfs_handle_fs_error in btrfs_fill_super

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 2/7] btrfs: replace BUG_ON(level == 0) with ASSERT(level)

[Josef Bacik]

In btrfs_read_node_slot() we have a BUG_ON() that can be converted to an
ASSERT().

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

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 118440857f33..484d750df4c6 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -964,7 +964,7 @@ struct extent_buffer *btrfs_read_node_slot(struct extent_buffer *parent,
 	if (slot < 0 || slot >= btrfs_header_nritems(parent))
 		return ERR_PTR(-ENOENT);
 
-	BUG_ON(level == 0);
+	ASSERT(level);
 
 	check.level = level - 1;
 	check.transid = btrfs_node_ptr_generation(parent, slot);
-- 
2.26.3

Re: [PATCH 2/7] btrfs: replace BUG_ON(level == 0) with ASSERT(level)

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 3/7] btrfs: handle errors from btrfs_read_node_slot in split

[Josef Bacik]

While investigating a problem with error injection I tripped over
curious behavior in the node/leaf splitting code.  If we get an EIO when
trying to read either the left or right leaf/node for splitting we'll
simply treat the node as if it were full and continue on.  The end
result of this isn't too bad, we simply end up allocating a block when
we may have pushed items into the adjacent blocks.  However this does
essentially allow us to continue to modify a file system that we've
gotten errors on, either from a bad disk or csum mismatch or other
corruption.  This isn't particularly safe, so instead handle these
btrfs_read_node_slot() usages differently.  We allow you to pass in any
slot, the idea being that we save some code if the slot number is
outside of the range of the parent.  This means we treat all errors the
same, when in reality we only want to ignore -ENOENT.

Fix this by changing how we call btrfs_read_node_slot(), which is to
only call it for slots we know are valid.  This way if we get an error
back from reading the block we can properly pass the error up the chain.
This was validated with the error injection testing I was doing.

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

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 484d750df4c6..ed042b541326 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1069,11 +1069,14 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 	    BTRFS_NODEPTRS_PER_BLOCK(fs_info) / 4)
 		return 0;
 
-	left = btrfs_read_node_slot(parent, pslot - 1);
-	if (IS_ERR(left))
-		left = NULL;
+	if (pslot) {
+		left = btrfs_read_node_slot(parent, pslot - 1);
+		if (IS_ERR(left)) {
+			ret = PTR_ERR(left);
+			left = NULL;
+			goto enospc;
+		}
 
-	if (left) {
 		__btrfs_tree_lock(left, BTRFS_NESTING_LEFT);
 		wret = btrfs_cow_block(trans, root, left,
 				       parent, pslot - 1, &left,
@@ -1084,11 +1087,14 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 		}
 	}
 
-	right = btrfs_read_node_slot(parent, pslot + 1);
-	if (IS_ERR(right))
-		right = NULL;
+	if (pslot + 1 < btrfs_header_nritems(parent)) {
+		right = btrfs_read_node_slot(parent, pslot + 1);
+		if (IS_ERR(right)) {
+			ret = PTR_ERR(right);
+			right = NULL;
+			goto enospc;
+		}
 
-	if (right) {
 		__btrfs_tree_lock(right, BTRFS_NESTING_RIGHT);
 		wret = btrfs_cow_block(trans, root, right,
 				       parent, pslot + 1, &right,
@@ -1245,14 +1251,14 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans,
 	if (!parent)
 		return 1;
 
-	left = btrfs_read_node_slot(parent, pslot - 1);
-	if (IS_ERR(left))
-		left = NULL;
-
 	/* first, try to make some room in the middle buffer */
-	if (left) {
+	if (pslot) {
 		u32 left_nr;
 
+		left = btrfs_read_node_slot(parent, pslot - 1);
+		if (IS_ERR(left))
+			return PTR_ERR(left);
+
 		__btrfs_tree_lock(left, BTRFS_NESTING_LEFT);
 
 		left_nr = btrfs_header_nritems(left);
@@ -1297,16 +1303,17 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans,
 		btrfs_tree_unlock(left);
 		free_extent_buffer(left);
 	}
-	right = btrfs_read_node_slot(parent, pslot + 1);
-	if (IS_ERR(right))
-		right = NULL;
 
 	/*
 	 * then try to empty the right most buffer into the middle
 	 */
-	if (right) {
+	if (pslot + 1 < btrfs_header_nritems(parent)) {
 		u32 right_nr;
 
+		right = btrfs_read_node_slot(parent, pslot + 1);
+		if (IS_ERR(right))
+			return PTR_ERR(right);
+
 		__btrfs_tree_lock(right, BTRFS_NESTING_RIGHT);
 
 		right_nr = btrfs_header_nritems(right);
@@ -3203,12 +3210,8 @@ static int push_leaf_right(struct btrfs_trans_handle *trans, struct btrfs_root
 	btrfs_assert_tree_write_locked(path->nodes[1]);
 
 	right = btrfs_read_node_slot(upper, slot + 1);
-	/*
-	 * slot + 1 is not valid or we fail to read the right node,
-	 * no big deal, just return.
-	 */
 	if (IS_ERR(right))
-		return 1;
+		return PTR_ERR(right);
 
 	__btrfs_tree_lock(right, BTRFS_NESTING_RIGHT);
 
@@ -3422,12 +3425,8 @@ static int push_leaf_left(struct btrfs_trans_handle *trans, struct btrfs_root
 	btrfs_assert_tree_write_locked(path->nodes[1]);
 
 	left = btrfs_read_node_slot(path->nodes[1], slot - 1);
-	/*
-	 * slot - 1 is not valid or we fail to read the left node,
-	 * no big deal, just return.
-	 */
 	if (IS_ERR(left))
-		return 1;
+		return PTR_ERR(left);
 
 	__btrfs_tree_lock(left, BTRFS_NESTING_LEFT);
 
-- 
2.26.3

[PATCH 4/7] btrfs: iput on orphan cleanup failure

[Josef Bacik]

We missed a couple of iput()'s in the orphan cleanup failure paths, add
them so we don't get refcount errors.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2fd518afc4f3..09a3f7836400 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3691,6 +3691,7 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
 			trans = btrfs_start_transaction(root, 1);
 			if (IS_ERR(trans)) {
 				ret = PTR_ERR(trans);
+				iput(inode);
 				goto out;
 			}
 			btrfs_debug(fs_info, "auto deleting %Lu",
@@ -3698,8 +3699,10 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
 			ret = btrfs_del_orphan_item(trans, root,
 						    found_key.objectid);
 			btrfs_end_transaction(trans);
-			if (ret)
+			if (ret) {
+				iput(inode);
 				goto out;
+			}
 			continue;
 		}
 
-- 
2.26.3

[PATCH 5/7] btrfs: drop root refs properly when orphan cleanup fails

[Josef Bacik]

When we mount the file system we do something like this

while (1) {
	lookup fs roots;

	for (i = 0; i < num_roots; i++) {
		ret = btrfs_orphan_cleanup(roots[i]);
		if (ret)
			break;
		btrfs_put_root(roots[i]);
	}
}

for (; i < num_roots; i++)
	btrfs_put_root(roots[i]);

As you can see if we break in that inner loop we just go back to the
outer loop and lose the fact that we have to drop references on the
remaining roots we looked up.  Fix this by making an out label and
jumping to that on error so we don't leak a reference to the roots we
looked up.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b9a02163a891..c828bd987c23 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4380,12 +4380,12 @@ int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
 			root_objectid = gang[i]->root_key.objectid;
 			err = btrfs_orphan_cleanup(gang[i]);
 			if (err)
-				break;
+				goto out;
 			btrfs_put_root(gang[i]);
 		}
 		root_objectid++;
 	}
-
+out:
 	/* release the uncleaned roots due to error */
 	for (; i < ret; i++) {
 		if (gang[i])
-- 
2.26.3

Re: [PATCH 5/7] btrfs: drop root refs properly when orphan cleanup fails

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 6/7] btrfs: handle errors in walk_down_tree properly

[Josef Bacik]

We can get errors in walk_down_proc as we try and lookup extent info for
the snapshot dropping to act on.  However if we get an error we simply
return 1 which indicates we're done with walking down, which will lead
us to improperly continue with the snapshot drop with the incorrect
information.  Instead break if we get any error from walk_down_proc or
do_walk_down, and handle the case of ret == 1 by returning 0, otherwise
return the ret value that we have.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 824c657f59e8..30720ea94a82 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5509,11 +5509,11 @@ static noinline int walk_down_tree(struct btrfs_trans_handle *trans,
 {
 	int level = wc->level;
 	int lookup_info = 1;
-	int ret;
+	int ret = 0;
 
 	while (level >= 0) {
 		ret = walk_down_proc(trans, root, path, wc, lookup_info);
-		if (ret > 0)
+		if (ret)
 			break;
 
 		if (level == 0)
@@ -5528,10 +5528,10 @@ static noinline int walk_down_tree(struct btrfs_trans_handle *trans,
 			path->slots[level]++;
 			continue;
 		} else if (ret < 0)
-			return ret;
+			break;
 		level = wc->level;
 	}
-	return 0;
+	return (ret == 1) ? 0 : ret;
 }
 
 static noinline int walk_up_tree(struct btrfs_trans_handle *trans,
-- 
2.26.3

[PATCH 7/7] btrfs: abort the transaction if we get an error during snapshot drop

[Josef Bacik]

We were seeing weird errors when we were testing our btrfs backports
before we had the incorrect level check fix.  These errors appeared to
be improper error handling, but error injection testing uncovered that
the errors were a result of corruption that occurred from improper error
handling during snapshot delete.

With snapshot delete if we encounter any errors during walk_down or
walk_up we'll simply return an error, we won't abort the transaction.
This is problematic because we will be dropping references for nodes and
leaves along the way, and if we fail in the middle we will leave the
file system corrupt because we don't know where we left off in the drop.

Fix this by making sure we abort if we hit any errors during the walk
down or walk up operations, as we have no idea what operations could
have been left half done at this point.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 30720ea94a82..6b6c59e6805c 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5708,12 +5708,14 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc)
 
 		ret = walk_down_tree(trans, root, path, wc);
 		if (ret < 0) {
+			btrfs_abort_transaction(trans, ret);
 			err = ret;
 			break;
 		}
 
 		ret = walk_up_tree(trans, root, path, wc, BTRFS_MAX_LEVEL);
 		if (ret < 0) {
+			btrfs_abort_transaction(trans, ret);
 			err = ret;
 			break;
 		}
-- 
2.26.3

Re: [PATCH 0/7] Error handling fixes

[David Sterba]

On Tue, Feb 07, 2023 at 11:57:18AM -0500, Josef Bacik wrote:
> Hello,
> 
> For a short period of time our btrfs backport had 947a629988f1 ("btrfs: move
> tree block parentness check into validate_extent_buffer()") without the
> associated fix, which resulted in a lot of hilarity.
> 
> One of the things that popped was a WARN_ON(ret == 1) in __btrfs_free_extent
> where we didn't find the bytenr we were looking for.  This was troubling, as it
> appeared that we were losing the EIO and returning 1 from btrfs_search_slot.
> 
> I rigged up my error injection stress test with
> btrfs_check_leaf/btrfs_check_node with balance (as this was the path that we saw
> the error).  This of course uncovered a few other unrelated things, but
> eventually I reproduced what we saw in production.  Thankfully it was not that
> we were eating the -EIO and returning 1 instead, however the actual problem is
> worse.  We do not handle the errors properly in snapshot delete (which also gets
> used by reloation), and then we do not abort the transaction when we hit errors
> in this path, which leads to the file system being corrupted and eventually
> triggers the above WARN_ON().
> 
> With these fixes in place my stress testing was running overnight without
> tripping over any other leaks, corruptions, or panics.  Previously I wasn't able
> to run for longer than a couple of minutes without falling over.  Thanks,
> 
> Josef
> 
> Josef Bacik (7):
>   btrfs: use btrfs_handle_fs_error in btrfs_fill_super
>   btrfs: replace BUG_ON(level == 0) with ASSERT(level)
>   btrfs: handle errors from btrfs_read_node_slot in split
>   btrfs: iput on orphan cleanup failure
>   btrfs: drop root refs properly when orphan cleanup fails
>   btrfs: handle errors in walk_down_tree properly
>   btrfs: abort the transaction if we get an error during snapshot drop

Added to for-next as topic branch, will be moved to misc-next once the
6.3 pull request is out. Thanks.

Re: [PATCH 0/7] Error handling fixes

[David Sterba]

On Wed, Feb 15, 2023 at 08:26:39PM +0100, David Sterba wrote:
> On Tue, Feb 07, 2023 at 11:57:18AM -0500, Josef Bacik wrote:
> > Hello,
> > 
> > For a short period of time our btrfs backport had 947a629988f1 ("btrfs: move
> > tree block parentness check into validate_extent_buffer()") without the
> > associated fix, which resulted in a lot of hilarity.
> > 
> > One of the things that popped was a WARN_ON(ret == 1) in __btrfs_free_extent
> > where we didn't find the bytenr we were looking for.  This was troubling, as it
> > appeared that we were losing the EIO and returning 1 from btrfs_search_slot.
> > 
> > I rigged up my error injection stress test with
> > btrfs_check_leaf/btrfs_check_node with balance (as this was the path that we saw
> > the error).  This of course uncovered a few other unrelated things, but
> > eventually I reproduced what we saw in production.  Thankfully it was not that
> > we were eating the -EIO and returning 1 instead, however the actual problem is
> > worse.  We do not handle the errors properly in snapshot delete (which also gets
> > used by reloation), and then we do not abort the transaction when we hit errors
> > in this path, which leads to the file system being corrupted and eventually
> > triggers the above WARN_ON().
> > 
> > With these fixes in place my stress testing was running overnight without
> > tripping over any other leaks, corruptions, or panics.  Previously I wasn't able
> > to run for longer than a couple of minutes without falling over.  Thanks,
> > 
> > Josef
> > 
> > Josef Bacik (7):
> >   btrfs: use btrfs_handle_fs_error in btrfs_fill_super
> >   btrfs: replace BUG_ON(level == 0) with ASSERT(level)
> >   btrfs: handle errors from btrfs_read_node_slot in split
> >   btrfs: iput on orphan cleanup failure
> >   btrfs: drop root refs properly when orphan cleanup fails
> >   btrfs: handle errors in walk_down_tree properly
> >   btrfs: abort the transaction if we get an error during snapshot drop
> 
> Added to for-next as topic branch, will be moved to misc-next once the
> 6.3 pull request is out. Thanks.

Moved to misc-next.