[PATCH v2 0/3] btrfs-progs: error messages enhancement for bad tree blocks

[PATCH v2 0/3] btrfs-progs: error messages enhancement for bad tree blocks

[Qu Wenruo]

When handling a corrupted btrfs image, there are tons of meaningless
error messages from btrfs-check:

  incorrect offsets 8492 3707786077

Above error message is meaningless, it doesn't contain which tree block
is causing the problem, just some random expected values with corrupted
values.

On the other hand, btrfs kernel tree-checker has way better error
messages, and even more checks than the counterpart in btrfs-progs.

So let's just backport the superior tree-checker code to btrfs-progs,
with some btrfs-progs specific (but minor) modifications.

Now the error message would look more sane (although a little too long):

  corrupt leaf: root=2 block=72164753408 slot=109, unexpected item end, have 3707786077 expect 8492

Changelog:
v2:
- Fix an uninitialized pointer which leads to fsck test failure

Qu Wenruo (3):
  btrfs-progs: use btrfs_key for btrfs_check_node() and
    btrfs_check_leaf()
  btrfs-progs: backport btrfs_check_leaf() from kernel
  btrfs-progs: backport btrfs_check_node() from kernel

 check/main.c          |   9 +-
 check/mode-original.h |   2 +-
 kernel-shared/ctree.c | 246 ++++++++++++++++++++++++++----------------
 kernel-shared/ctree.h |   5 +-
 4 files changed, 163 insertions(+), 99 deletions(-)

-- 
2.33.0

[PATCH v2 1/3] btrfs-progs: use btrfs_key for btrfs_check_node() and btrfs_check_leaf()

[Qu Wenruo]

In kernel space we hardly use btrfs_disk_key, unless for very lowlevel
code.

There is no need to intentionally use btrfs_disk_key in btrfs-progs
either.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c          |  9 +++---
 check/mode-original.h |  2 +-
 kernel-shared/ctree.c | 64 +++++++++++++++++++++++--------------------
 kernel-shared/ctree.h |  4 +--
 4 files changed, 42 insertions(+), 37 deletions(-)

diff --git a/check/main.c b/check/main.c
index a27efe56eec6..ff1ccade3967 100644
--- a/check/main.c
+++ b/check/main.c
@@ -4162,7 +4162,6 @@ static int record_bad_block_io(struct cache_tree *extent_cache,
 {
 	struct extent_record *rec;
 	struct cache_extent *cache;
-	struct btrfs_key key;
 
 	cache = lookup_cache_extent(extent_cache, start, len);
 	if (!cache)
@@ -4172,8 +4171,8 @@ static int record_bad_block_io(struct cache_tree *extent_cache,
 	if (!is_extent_tree_record(rec))
 		return 0;
 
-	btrfs_disk_key_to_cpu(&key, &rec->parent_key);
-	return btrfs_add_corrupt_extent_record(gfs_info, &key, start, len, 0);
+	return btrfs_add_corrupt_extent_record(gfs_info, &rec->parent_key,
+					       start, len, 0);
 }
 
 static int swap_values(struct btrfs_root *root, struct btrfs_path *path,
@@ -6567,7 +6566,9 @@ static int run_next_block(struct btrfs_root *root,
 			}
 
 			memset(&tmpl, 0, sizeof(tmpl));
-			btrfs_cpu_key_to_disk(&tmpl.parent_key, &key);
+			tmpl.parent_key.objectid = key.objectid;
+			tmpl.parent_key.type = key.type;
+			tmpl.parent_key.offset = key.offset;
 			tmpl.parent_generation =
 				btrfs_node_ptr_generation(buf, i);
 			tmpl.start = ptr;
diff --git a/check/mode-original.h b/check/mode-original.h
index eed16d92d0db..cf06917c47dc 100644
--- a/check/mode-original.h
+++ b/check/mode-original.h
@@ -79,7 +79,7 @@ struct extent_record {
 	struct rb_root backref_tree;
 	struct list_head list;
 	struct cache_extent cache;
-	struct btrfs_disk_key parent_key;
+	struct btrfs_key parent_key;
 	u64 start;
 	u64 max_size;
 	u64 nr;
diff --git a/kernel-shared/ctree.c b/kernel-shared/ctree.c
index 0845cc6091d4..cf023b0831bc 100644
--- a/kernel-shared/ctree.c
+++ b/kernel-shared/ctree.c
@@ -568,11 +568,10 @@ static inline unsigned int leaf_data_end(const struct btrfs_fs_info *fs_info,
 
 enum btrfs_tree_block_status
 btrfs_check_node(struct btrfs_fs_info *fs_info,
-		 struct btrfs_disk_key *parent_key, struct extent_buffer *buf)
+		 struct btrfs_key *parent_key, struct extent_buffer *buf)
 {
 	int i;
-	struct btrfs_key cpukey;
-	struct btrfs_disk_key key;
+	struct btrfs_key key;
 	u32 nritems = btrfs_header_nritems(buf);
 	enum btrfs_tree_block_status ret = BTRFS_TREE_BLOCK_INVALID_NRITEMS;
 
@@ -581,25 +580,27 @@ btrfs_check_node(struct btrfs_fs_info *fs_info,
 
 	ret = BTRFS_TREE_BLOCK_INVALID_PARENT_KEY;
 	if (parent_key && parent_key->type) {
-		btrfs_node_key(buf, &key, 0);
+		btrfs_node_key_to_cpu(buf, &key, 0);
 		if (memcmp(parent_key, &key, sizeof(key)))
 			goto fail;
 	}
 	ret = BTRFS_TREE_BLOCK_BAD_KEY_ORDER;
 	for (i = 0; nritems > 1 && i < nritems - 2; i++) {
-		btrfs_node_key(buf, &key, i);
-		btrfs_node_key_to_cpu(buf, &cpukey, i + 1);
-		if (btrfs_comp_keys(&key, &cpukey) >= 0)
+		struct btrfs_key next_key;
+
+		btrfs_node_key_to_cpu(buf, &key, i);
+		btrfs_node_key_to_cpu(buf, &next_key, i + 1);
+		if (btrfs_comp_cpu_keys(&key, &next_key) >= 0)
 			goto fail;
 	}
 	return BTRFS_TREE_BLOCK_CLEAN;
 fail:
 	if (btrfs_header_owner(buf) == BTRFS_EXTENT_TREE_OBJECTID) {
 		if (parent_key)
-			btrfs_disk_key_to_cpu(&cpukey, parent_key);
+			memcpy(&key, parent_key, sizeof(struct btrfs_key));
 		else
-			btrfs_node_key_to_cpu(buf, &cpukey, 0);
-		btrfs_add_corrupt_extent_record(fs_info, &cpukey,
+			btrfs_node_key_to_cpu(buf, &key, 0);
+		btrfs_add_corrupt_extent_record(fs_info, &key,
 						buf->start, buf->len,
 						btrfs_header_level(buf));
 	}
@@ -608,11 +609,10 @@ fail:
 
 enum btrfs_tree_block_status
 btrfs_check_leaf(struct btrfs_fs_info *fs_info,
-		 struct btrfs_disk_key *parent_key, struct extent_buffer *buf)
+		 struct btrfs_key *parent_key, struct extent_buffer *buf)
 {
 	int i;
-	struct btrfs_key cpukey;
-	struct btrfs_disk_key key;
+	struct btrfs_key key;
 	u32 nritems = btrfs_header_nritems(buf);
 	enum btrfs_tree_block_status ret = BTRFS_TREE_BLOCK_INVALID_NRITEMS;
 
@@ -639,7 +639,7 @@ btrfs_check_leaf(struct btrfs_fs_info *fs_info,
 	if (nritems == 0)
 		return BTRFS_TREE_BLOCK_CLEAN;
 
-	btrfs_item_key(buf, &key, 0);
+	btrfs_item_key_to_cpu(buf, &key, 0);
 	if (parent_key && parent_key->type &&
 	    memcmp(parent_key, &key, sizeof(key))) {
 		ret = BTRFS_TREE_BLOCK_INVALID_PARENT_KEY;
@@ -648,9 +648,12 @@ btrfs_check_leaf(struct btrfs_fs_info *fs_info,
 		goto fail;
 	}
 	for (i = 0; nritems > 1 && i < nritems - 1; i++) {
-		btrfs_item_key(buf, &key, i);
-		btrfs_item_key_to_cpu(buf, &cpukey, i + 1);
-		if (btrfs_comp_keys(&key, &cpukey) >= 0) {
+		struct btrfs_key next_key;
+
+		btrfs_item_key_to_cpu(buf, &key, i);
+		btrfs_item_key_to_cpu(buf, &next_key, i + 1);
+
+		if (btrfs_comp_cpu_keys(&key, &next_key) >= 0) {
 			ret = BTRFS_TREE_BLOCK_BAD_KEY_ORDER;
 			fprintf(stderr, "bad key ordering %d %d\n", i, i+1);
 			goto fail;
@@ -676,8 +679,10 @@ btrfs_check_leaf(struct btrfs_fs_info *fs_info,
 	for (i = 0; i < nritems; i++) {
 		if (btrfs_item_end_nr(buf, i) >
 				BTRFS_LEAF_DATA_SIZE(fs_info)) {
-			btrfs_item_key(buf, &key, 0);
-			btrfs_print_key(&key);
+			struct btrfs_disk_key disk_key;
+
+			btrfs_item_key(buf, &disk_key, 0);
+			btrfs_print_key(&disk_key);
 			fflush(stdout);
 			ret = BTRFS_TREE_BLOCK_INVALID_OFFSETS;
 			fprintf(stderr, "slot end outside of leaf %llu > %llu\n",
@@ -692,11 +697,11 @@ btrfs_check_leaf(struct btrfs_fs_info *fs_info,
 fail:
 	if (btrfs_header_owner(buf) == BTRFS_EXTENT_TREE_OBJECTID) {
 		if (parent_key)
-			btrfs_disk_key_to_cpu(&cpukey, parent_key);
+			memcpy(&key, parent_key, sizeof(struct btrfs_key));
 		else
-			btrfs_item_key_to_cpu(buf, &cpukey, 0);
+			btrfs_item_key_to_cpu(buf, &key, 0);
 
-		btrfs_add_corrupt_extent_record(fs_info, &cpukey,
+		btrfs_add_corrupt_extent_record(fs_info, &key,
 						buf->start, buf->len, 0);
 	}
 	return ret;
@@ -705,22 +710,21 @@ fail:
 static int noinline check_block(struct btrfs_fs_info *fs_info,
 				struct btrfs_path *path, int level)
 {
-	struct btrfs_disk_key key;
-	struct btrfs_disk_key *key_ptr = NULL;
-	struct extent_buffer *parent;
+	struct btrfs_key key;
+	struct btrfs_key *parent_key_ptr = NULL;
 	enum btrfs_tree_block_status ret;
 
 	if (path->skip_check_block)
 		return 0;
 	if (path->nodes[level + 1]) {
-		parent = path->nodes[level + 1];
-		btrfs_node_key(parent, &key, path->slots[level + 1]);
-		key_ptr = &key;
+		btrfs_node_key_to_cpu(path->nodes[level + 1], &key,
+				     path->slots[level + 1]);
+		parent_key_ptr = &key;
 	}
 	if (level == 0)
-		ret = btrfs_check_leaf(fs_info, key_ptr, path->nodes[0]);
+		ret = btrfs_check_leaf(fs_info, parent_key_ptr, path->nodes[0]);
 	else
-		ret = btrfs_check_node(fs_info, key_ptr, path->nodes[level]);
+		ret = btrfs_check_node(fs_info, parent_key_ptr, path->nodes[level]);
 	if (ret == BTRFS_TREE_BLOCK_CLEAN)
 		return 0;
 	return -EIO;
diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h
index 3cca60323e3d..5ed8e3e373fa 100644
--- a/kernel-shared/ctree.h
+++ b/kernel-shared/ctree.h
@@ -2637,10 +2637,10 @@ int btrfs_del_ptr(struct btrfs_root *root, struct btrfs_path *path,
 		int level, int slot);
 enum btrfs_tree_block_status
 btrfs_check_node(struct btrfs_fs_info *fs_info,
-		 struct btrfs_disk_key *parent_key, struct extent_buffer *buf);
+		 struct btrfs_key *parent_key, struct extent_buffer *buf);
 enum btrfs_tree_block_status
 btrfs_check_leaf(struct btrfs_fs_info *fs_info,
-		 struct btrfs_disk_key *parent_key, struct extent_buffer *buf);
+		 struct btrfs_key *parent_key, struct extent_buffer *buf);
 void reada_for_search(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 		      int level, int slot, u64 objectid);
 struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info,
-- 
2.33.0

[PATCH v2 2/3] btrfs-progs: backport btrfs_check_leaf() from kernel

[Qu Wenruo]

Currently btrfs_check_leaf() provides almost meaningless messages for
things like invalid item offset:

  incorrect offsets 8492 3707786077

While kernel tree-checker is doing a way better job, so it's wise to
backport btrfs_check_leaf() from kernel.

There are some modification needed:

- New generic_err() helper

- Remove unlikely() macro

- Remove empty essential tree check
  Mkfs still needs to create empty essential trees.

- Using BTRFS_TREE_BLOCK_* return value
  Original mode check still relies on them to do certain repair.

- No need for btrfs_fs_info
  We no longer needs fsid output, thus no need for btrfs_fs_info.

- No item contents check

- Still using the fail: label for btrfs-progs specific error handling

The new output looks like:

  corrupt leaf: root=2 block=72164753408 slot=109, unexpected item end, have 3707786077 expect 8492

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/ctree.c | 152 +++++++++++++++++++++++++-----------------
 1 file changed, 90 insertions(+), 62 deletions(-)

diff --git a/kernel-shared/ctree.c b/kernel-shared/ctree.c
index cf023b0831bc..4f1c34d8020d 100644
--- a/kernel-shared/ctree.c
+++ b/kernel-shared/ctree.c
@@ -566,6 +566,20 @@ static inline unsigned int leaf_data_end(const struct btrfs_fs_info *fs_info,
 	return btrfs_item_offset_nr(leaf, nr - 1);
 }
 
+static void generic_err(const struct extent_buffer *buf, int slot,
+			const char *fmt, ...)
+{
+	va_list args;
+
+	fprintf(stderr, "corrupt %s: root=%lld block=%llu slot=%d, ",
+		btrfs_header_level(buf) == 0 ? "leaf": "node",
+		btrfs_header_owner(buf), btrfs_header_bytenr(buf), slot);
+	va_start(args, fmt);
+	vfprintf(stderr, fmt, args);
+	va_end(args);
+	fprintf(stderr, "\n");
+}
+
 enum btrfs_tree_block_status
 btrfs_check_node(struct btrfs_fs_info *fs_info,
 		 struct btrfs_key *parent_key, struct extent_buffer *buf)
@@ -609,100 +623,114 @@ fail:
 
 enum btrfs_tree_block_status
 btrfs_check_leaf(struct btrfs_fs_info *fs_info,
-		 struct btrfs_key *parent_key, struct extent_buffer *buf)
+		 struct btrfs_key *parent_key, struct extent_buffer *leaf)
 {
-	int i;
+	/* No valid key type is 0, so all key should be larger than this key */
+	struct btrfs_key prev_key = {0, 0, 0};
 	struct btrfs_key key;
-	u32 nritems = btrfs_header_nritems(buf);
-	enum btrfs_tree_block_status ret = BTRFS_TREE_BLOCK_INVALID_NRITEMS;
-
-	if (nritems * sizeof(struct btrfs_item) > buf->len)  {
-		fprintf(stderr, "invalid number of items %llu\n",
-			(unsigned long long)buf->start);
-		goto fail;
-	}
+	u32 nritems = btrfs_header_nritems(leaf);
+	int slot;
+	int ret;
 
-	if (btrfs_header_level(buf) != 0) {
+	if (btrfs_header_level(leaf) != 0) {
+		generic_err(leaf, 0,
+			"invalid level for leaf, have %d expect 0",
+			btrfs_header_level(leaf));
 		ret = BTRFS_TREE_BLOCK_INVALID_LEVEL;
-		fprintf(stderr, "leaf is not a leaf %llu\n",
-		       (unsigned long long)btrfs_header_bytenr(buf));
-		goto fail;
-	}
-	if (btrfs_leaf_free_space(buf) < 0) {
-		ret = BTRFS_TREE_BLOCK_INVALID_FREE_SPACE;
-		fprintf(stderr, "leaf free space incorrect %llu %d\n",
-			(unsigned long long)btrfs_header_bytenr(buf),
-			btrfs_leaf_free_space(buf));
 		goto fail;
 	}
 
 	if (nritems == 0)
-		return BTRFS_TREE_BLOCK_CLEAN;
-
-	btrfs_item_key_to_cpu(buf, &key, 0);
-	if (parent_key && parent_key->type &&
-	    memcmp(parent_key, &key, sizeof(key))) {
-		ret = BTRFS_TREE_BLOCK_INVALID_PARENT_KEY;
-		fprintf(stderr, "leaf parent key incorrect %llu\n",
-		       (unsigned long long)btrfs_header_bytenr(buf));
-		goto fail;
-	}
-	for (i = 0; nritems > 1 && i < nritems - 1; i++) {
-		struct btrfs_key next_key;
-
-		btrfs_item_key_to_cpu(buf, &key, i);
-		btrfs_item_key_to_cpu(buf, &next_key, i + 1);
+		return 0;
 
-		if (btrfs_comp_cpu_keys(&key, &next_key) >= 0) {
+	/*
+	 * Check the following things to make sure this is a good leaf, and
+	 * leaf users won't need to bother with similar sanity checks:
+	 *
+	 * 1) key ordering
+	 * 2) item offset and size
+	 *    No overlap, no hole, all inside the leaf.
+	 * 3) item content
+	 *    If possible, do comprehensive sanity check.
+	 *    NOTE: All checks must only rely on the item data itself.
+	 */
+	for (slot = 0; slot < nritems; slot++) {
+		u32 item_end_expected;
+
+		btrfs_item_key_to_cpu(leaf, &key, slot);
+
+		/* Make sure the keys are in the right order */
+		if (btrfs_comp_cpu_keys(&prev_key, &key) >= 0) {
+			generic_err(leaf, slot,
+	"bad key order, prev (%llu %u %llu) current (%llu %u %llu)",
+				prev_key.objectid, prev_key.type,
+				prev_key.offset, key.objectid, key.type,
+				key.offset);
 			ret = BTRFS_TREE_BLOCK_BAD_KEY_ORDER;
-			fprintf(stderr, "bad key ordering %d %d\n", i, i+1);
 			goto fail;
 		}
-		if (btrfs_item_offset_nr(buf, i) !=
-			btrfs_item_end_nr(buf, i + 1)) {
+
+		/*
+		 * Make sure the offset and ends are right, remember that the
+		 * item data starts at the end of the leaf and grows towards the
+		 * front.
+		 */
+		if (slot == 0)
+			item_end_expected = BTRFS_LEAF_DATA_SIZE(fs_info);
+		else
+			item_end_expected = btrfs_item_offset_nr(leaf,
+								 slot - 1);
+		if (btrfs_item_end_nr(leaf, slot) != item_end_expected) {
+			generic_err(leaf, slot,
+				"unexpected item end, have %u expect %u",
+				btrfs_item_end_nr(leaf, slot),
+				item_end_expected);
 			ret = BTRFS_TREE_BLOCK_INVALID_OFFSETS;
-			fprintf(stderr, "incorrect offsets %u %u\n",
-				btrfs_item_offset_nr(buf, i),
-				btrfs_item_end_nr(buf, i + 1));
 			goto fail;
 		}
-		if (i == 0 && btrfs_item_end_nr(buf, i) !=
+
+		/*
+		 * Check to make sure that we don't point outside of the leaf,
+		 * just in case all the items are consistent to each other, but
+		 * all point outside of the leaf.
+		 */
+		if (btrfs_item_end_nr(leaf, slot) >
 		    BTRFS_LEAF_DATA_SIZE(fs_info)) {
+			generic_err(leaf, slot,
+			"slot end outside of leaf, have %u expect range [0, %u]",
+				btrfs_item_end_nr(leaf, slot),
+				BTRFS_LEAF_DATA_SIZE(fs_info));
 			ret = BTRFS_TREE_BLOCK_INVALID_OFFSETS;
-			fprintf(stderr, "bad item end %u wanted %u\n",
-				btrfs_item_end_nr(buf, i),
-				(unsigned)BTRFS_LEAF_DATA_SIZE(fs_info));
 			goto fail;
 		}
-	}
 
-	for (i = 0; i < nritems; i++) {
-		if (btrfs_item_end_nr(buf, i) >
-				BTRFS_LEAF_DATA_SIZE(fs_info)) {
-			struct btrfs_disk_key disk_key;
-
-			btrfs_item_key(buf, &disk_key, 0);
-			btrfs_print_key(&disk_key);
-			fflush(stdout);
+		/* Also check if the item pointer overlaps with btrfs item. */
+		if (btrfs_item_ptr_offset(leaf, slot) <
+		    btrfs_item_nr_offset(slot) + sizeof(struct btrfs_item)) {
+			generic_err(leaf, slot,
+		"slot overlaps with its data, item end %lu data start %lu",
+				btrfs_item_nr_offset(slot) +
+				sizeof(struct btrfs_item),
+				btrfs_item_ptr_offset(leaf, slot));
 			ret = BTRFS_TREE_BLOCK_INVALID_OFFSETS;
-			fprintf(stderr, "slot end outside of leaf %llu > %llu\n",
-				(unsigned long long)btrfs_item_end_nr(buf, i),
-				(unsigned long long)BTRFS_LEAF_DATA_SIZE(
-					fs_info));
 			goto fail;
 		}
+
+		prev_key.objectid = key.objectid;
+		prev_key.type = key.type;
+		prev_key.offset = key.offset;
 	}
 
 	return BTRFS_TREE_BLOCK_CLEAN;
 fail:
-	if (btrfs_header_owner(buf) == BTRFS_EXTENT_TREE_OBJECTID) {
+	if (btrfs_header_owner(leaf) == BTRFS_EXTENT_TREE_OBJECTID) {
 		if (parent_key)
 			memcpy(&key, parent_key, sizeof(struct btrfs_key));
 		else
-			btrfs_item_key_to_cpu(buf, &key, 0);
+			btrfs_item_key_to_cpu(leaf, &key, 0);
 
 		btrfs_add_corrupt_extent_record(fs_info, &key,
-						buf->start, buf->len, 0);
+						leaf->start, leaf->len, 0);
 	}
 	return ret;
 }
-- 
2.33.0

[PATCH v2 3/3] btrfs-progs: backport btrfs_check_node() from kernel

[Qu Wenruo]

The btrfs_check_node() has far less meaningful error message compared to
kernel counterpart, and it even lacks certain checks like level check.

Backport btrfs_check_node() to btrfs-progs to not only unify the code
but greatly improve the readability of the error messages.

Extra modification includes:

- No fs_info needed
  As we don't need to output fsid.

- Remove unlikely() macro

- Extra BTRFS_TREE_BLOCK_* error type

- Btrfs-progs specific error handling
  To record the corrupted tree blocks.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/ctree.c | 70 ++++++++++++++++++++++++++++++-------------
 kernel-shared/ctree.h |  1 +
 2 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/kernel-shared/ctree.c b/kernel-shared/ctree.c
index 4f1c34d8020d..8f9e4b7ca8bd 100644
--- a/kernel-shared/ctree.c
+++ b/kernel-shared/ctree.c
@@ -582,41 +582,71 @@ static void generic_err(const struct extent_buffer *buf, int slot,
 
 enum btrfs_tree_block_status
 btrfs_check_node(struct btrfs_fs_info *fs_info,
-		 struct btrfs_key *parent_key, struct extent_buffer *buf)
+		 struct btrfs_key *parent_key, struct extent_buffer *node)
 {
-	int i;
-	struct btrfs_key key;
-	u32 nritems = btrfs_header_nritems(buf);
+	unsigned long nr = btrfs_header_nritems(node);
+	struct btrfs_key key, next_key;
+	int slot;
+	int level = btrfs_header_level(node);
+	u64 bytenr;
 	enum btrfs_tree_block_status ret = BTRFS_TREE_BLOCK_INVALID_NRITEMS;
 
-	if (nritems == 0 || nritems > BTRFS_NODEPTRS_PER_BLOCK(fs_info))
+	if (level <= 0 || level >= BTRFS_MAX_LEVEL) {
+		generic_err(node, 0,
+			"invalid level for node, have %d expect [1, %d]",
+			level, BTRFS_MAX_LEVEL - 1);
+		ret = BTRFS_TREE_BLOCK_INVALID_LEVEL;
 		goto fail;
+	}
+	if (nr == 0 || nr > BTRFS_NODEPTRS_PER_BLOCK(fs_info)) {
+		generic_err(node, 0,
+"corrupt node: root=%llu block=%llu, nritems too %s, have %lu expect range [1,%u]",
+			   btrfs_header_owner(node), node->start,
+			   nr == 0 ? "small" : "large", nr,
+			   BTRFS_NODEPTRS_PER_BLOCK(fs_info));
+		ret = BTRFS_TREE_BLOCK_INVALID_NRITEMS;
+		goto fail;
+	}
 
-	ret = BTRFS_TREE_BLOCK_INVALID_PARENT_KEY;
-	if (parent_key && parent_key->type) {
-		btrfs_node_key_to_cpu(buf, &key, 0);
-		if (memcmp(parent_key, &key, sizeof(key)))
+	for (slot = 0; slot < nr - 1; slot++) {
+		bytenr = btrfs_node_blockptr(node, slot);
+		btrfs_node_key_to_cpu(node, &key, slot);
+		btrfs_node_key_to_cpu(node, &next_key, slot + 1);
+
+		if (!bytenr) {
+			generic_err(node, slot,
+				"invalid NULL node pointer");
+			ret = BTRFS_TREE_BLOCK_INVALID_BLOCKPTR;
 			goto fail;
-	}
-	ret = BTRFS_TREE_BLOCK_BAD_KEY_ORDER;
-	for (i = 0; nritems > 1 && i < nritems - 2; i++) {
-		struct btrfs_key next_key;
+		}
+		if (!IS_ALIGNED(bytenr, fs_info->sectorsize)) {
+			generic_err(node, slot,
+			"unaligned pointer, have %llu should be aligned to %u",
+				bytenr, fs_info->sectorsize);
+			ret = BTRFS_TREE_BLOCK_INVALID_BLOCKPTR;
+			goto fail;
+		}
 
-		btrfs_node_key_to_cpu(buf, &key, i);
-		btrfs_node_key_to_cpu(buf, &next_key, i + 1);
-		if (btrfs_comp_cpu_keys(&key, &next_key) >= 0)
+		if (btrfs_comp_cpu_keys(&key, &next_key) >= 0) {
+			generic_err(node, slot,
+	"bad key order, current (%llu %u %llu) next (%llu %u %llu)",
+				key.objectid, key.type, key.offset,
+				next_key.objectid, next_key.type,
+				next_key.offset);
+			ret = BTRFS_TREE_BLOCK_BAD_KEY_ORDER;
 			goto fail;
+		}
 	}
 	return BTRFS_TREE_BLOCK_CLEAN;
 fail:
-	if (btrfs_header_owner(buf) == BTRFS_EXTENT_TREE_OBJECTID) {
+	if (btrfs_header_owner(node) == BTRFS_EXTENT_TREE_OBJECTID) {
 		if (parent_key)
 			memcpy(&key, parent_key, sizeof(struct btrfs_key));
 		else
-			btrfs_node_key_to_cpu(buf, &key, 0);
+			btrfs_node_key_to_cpu(node, &key, 0);
 		btrfs_add_corrupt_extent_record(fs_info, &key,
-						buf->start, buf->len,
-						btrfs_header_level(buf));
+						node->start, node->len,
+						btrfs_header_level(node));
 	}
 	return ret;
 }
diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h
index 5ed8e3e373fa..529f8bce2f50 100644
--- a/kernel-shared/ctree.h
+++ b/kernel-shared/ctree.h
@@ -697,6 +697,7 @@ enum btrfs_tree_block_status {
 	BTRFS_TREE_BLOCK_INVALID_LEVEL,
 	BTRFS_TREE_BLOCK_INVALID_FREE_SPACE,
 	BTRFS_TREE_BLOCK_INVALID_OFFSETS,
+	BTRFS_TREE_BLOCK_INVALID_BLOCKPTR,
 };
 
 struct btrfs_inode_item {
-- 
2.33.0

Re: [PATCH v2 0/3] btrfs-progs: error messages enhancement for bad tree blocks

[David Sterba]

On Sat, Sep 04, 2021 at 09:31:28AM +0800, Qu Wenruo wrote:
> When handling a corrupted btrfs image, there are tons of meaningless
> error messages from btrfs-check:
> 
>   incorrect offsets 8492 3707786077
> 
> Above error message is meaningless, it doesn't contain which tree block
> is causing the problem, just some random expected values with corrupted
> values.
> 
> On the other hand, btrfs kernel tree-checker has way better error
> messages, and even more checks than the counterpart in btrfs-progs.
> 
> So let's just backport the superior tree-checker code to btrfs-progs,
> with some btrfs-progs specific (but minor) modifications.
> 
> Now the error message would look more sane (although a little too long):
> 
>   corrupt leaf: root=2 block=72164753408 slot=109, unexpected item end, have 3707786077 expect 8492
> 
> Changelog:
> v2:
> - Fix an uninitialized pointer which leads to fsck test failure
> 
> Qu Wenruo (3):
>   btrfs-progs: use btrfs_key for btrfs_check_node() and
>     btrfs_check_leaf()
>   btrfs-progs: backport btrfs_check_leaf() from kernel
>   btrfs-progs: backport btrfs_check_node() from kernel

Added to devel, thanks.