Re: [PATCH] Btrfs: fix BUG_ON in btrfs_mark_buffer_dirty

Hi Jeff,

On Wed, Sep 07, 2016 at 10:25:54AM -0400, Jeff Mahoney wrote:

On 9/6/16 5:51 PM, Liu Bo wrote:

quoted

Hi Filipe,

On Mon, Sep 05, 2016 at 04:28:09PM +0100, Filipe Manana wrote:

quoted

On Fri, Sep 2, 2016 at 8:35 PM, Liu Bo [off-list ref] wrote:

quoted

This can only happen with CONFIG_BTRFS_FS_CHECK_INTEGRITY=y.

Commit 1ba98d0 ("Btrfs: detect corruption when non-root leaf has zero item")
assumes that a leaf is its root when leaf->bytenr == btrfs_root_bytenr(root),
however, we should not use btrfs_root_bytenr(root) since it's mainly got
updated during committing transaction.  So the check can fail when doing
COW on this leaf while it is a root.

This changes to use "if (leaf == btrfs_root_node(root))" instead, just like
how we check whether leaf is a root in __btrfs_cow_block().

Reported-by: Jeff Mahoney <redacted>
Signed-off-by: Liu Bo <redacted>

Hi Bo,

Even with this patch applied against latest branch for-linus-4.8, at
least on a build with CONFIG_BTRFS_FS_CHECK_INTEGRITY=y,
the issue still happens for me when running fsstress with balance in parallel:

Thanks for the report.

This panic shows that we can have non-root btree leaf with 0 nritems during
split_leaf(), but a btrfs_search_slot which calls split_leaf() like this is
inserting an item, and while we set @right's nritems to 0, we also assign @disk_key
associated with @right in the parent node, so I think we're actually having
 nritem 0 temporarily and we can remove this btrfs_mark_buffer_dirty() like the
following quick patch.

Thanks,

-liubo

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index d1c56c9..5e5ceb5 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c

@@ -4341,7 +4341,11 @@ again:
 			if (path->slots[1] == 0)
 				fixup_low_keys(fs_info, path, &disk_key, 1);
 		}
-		btrfs_mark_buffer_dirty(right);
+		/*
+		 * We create a new leaf 'right' for the required ins_len and
+		 * we'll do btrfs_mark_buffer_dirty() on this leaf after copying
+		 * the content of ins_len to 'right'.
+		 */
 		return ret;
 	}
 

I think you're on the right track here.  I need to see if I still have
the code lying around, but when I was debugging the btrfs_rename issue
that ended up being a compiler bug, I hooked into check_leaf and ran
into similar issues.  We mark the buffer dirty before it's in a
consistent state.

That's right.

One thing that I'm not 100% sure is that if there is a
chance that this metadata leaf gets flushed by writeback throttle code
and we panic after it so that we get a 'nritem 0 non-root' leaf, no?

But anyway, even if we have it, we'd know it by the newly added check
in check_leaf.

Thanks,

-liubo