btrfs-progs: Free bad extent buffer as soon as possible

Commit Message

Qu Wenruo March 6, 2019, 1:47 a.m. UTC

[BUG]
For the new multiple -b parameter supporting, we could hit this bug on a
16K node sized btrfs:
  $ ./btrfs inspect dump-tree -b 1024 -b 2048 -b 4096 -b 8192 zimg
  btrfs-progs v4.20.2
  ERROR: tree block bytenr 1024 is not aligned to sectorsize 4096
  ERROR: tree block bytenr 2048 is not aligned to sectorsize 4096
  Couldn't map the block 4096
  Invalid mapping for 4096-20480, got 13631488-22020096
  Couldn't map the block 4096
  bad tree block 4096, bytenr mismatch, want=4096, have=0
  ERROR: failed to read tree block 4096
  extent_io.c:665: free_extent_buffer_internal: BUG_ON `eb->refs < 0`
  triggered, value 1
  ./btrfs[0x426e57]
  ./btrfs(free_extent_buffer+0xe)[0x427701]
  ./btrfs(alloc_extent_buffer+0x3f)[0x427872]
  ./btrfs(btrfs_find_create_tree_block+0xf)[0x415b3c]
  ./btrfs(read_tree_block+0x5c)[0x4171b5]
  ./btrfs(cmd_inspect_dump_tree+0x587)[0x46fb75]
  ./btrfs(handle_command_group+0x44)[0x40df89]
  ./btrfs(cmd_inspect+0x15)[0x44b569]
  ./btrfs(main+0x8b)[0x40e032]
  /lib64/libc.so.6(__libc_start_main+0xeb)[0x7f2001a54b7b]
  ./btrfs(_start+0x2a)[0x40dd1a]
  Aborted (core dumped)

This is not only limited to multiple ins dump-tree -b parameter support,
but also to possible overlapping bad tree blocks.

[CAUSE]
Btrfs delay extent freeing to improve performance.

However for the "-b 4096 -b 8192" case, the first -b 4096 will cause an
extent buffer start=4096 len=16384 refs=0 in the cached extent tree.

Then the incoming -b 8192 will hit the cache and reuse the cached extent
buffer.
And since the cached extent buffer doesn't match the bytenr, its refs
won't get increased, and we're going to free that eb again.

Since the bad cached eb already has a ref number 0, calling
free_extent_buffer() on it again will trigger the assert.

[FIX]
So for bad extent buffer we failed to read, just delete them
immediately.
This will free them from extent buffer cache, so later extent buffer
allocation will not hit the stale one, and prevent the bug from
happening.

Reported-by: David Sterba <[e-mail]>
Signed-off-by: Qu Wenruo <[e-mail]>
---
To David:

I'm not sure if this patch is suitbale to be merged with ins dump-tree
multiple -b parameter support.
As it looks to be the cause of serveral reported 'refs < 0' bug.
It's pretty lucky that multiple -b parameters could trigger it reliably.

Anyway your perdiction of small fix is very correct.

Thanks,
Qu
---
 disk-io.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Patch

diff --git a/disk-io.c b/disk-io.c
index 797b9b79ea3c..509e242b5614 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -392,7 +392,11 @@  struct extent_buffer* read_tree_block(struct btrfs_fs_info *fs_info, u64 bytenr,
 			continue;
 		}
 	}
-	free_extent_buffer(eb);
+	/*
+	 * We failed to read this tree block, it be should deleted right now
+	 * to avoid stale cache populate the cache.
+	 */
+	free_extent_buffer_nocache(eb);
 	return ERR_PTR(ret);
 }