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

[PATCH 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

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 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..c015c4f879c1 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;
 	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 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 c015c4f879c1..b53107d5a84e 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 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 b53107d5a84e..84b57c8fba3d 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 1/3] btrfs-progs: use btrfs_key for btrfs_check_node() and btrfs_check_leaf()

[David Sterba]

On Thu, Sep 02, 2021 at 09:08:41PM +0800, Qu Wenruo wrote:
> 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>

This fails on fsck/001 test

    [TEST/fsck]   001-bad-file-extent-bytenr
failed to restore image ./default_case.img
basename: missing operand
Try 'basename --help' for more information.
failed: /labs/dsterba/gits/btrfs-progs/btrfs check --repair --force
make: *** [Makefile:413: test-fsck] Error 1

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

[Qu Wenruo]

On 2021/9/3 下午10:02, David Sterba wrote:
> On Thu, Sep 02, 2021 at 09:08:41PM +0800, Qu Wenruo wrote:
>> 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>
>
> This fails on fsck/001 test
>
>      [TEST/fsck]   001-bad-file-extent-bytenr
> failed to restore image ./default_case.img
> basename: missing operand
> Try 'basename --help' for more information.
> failed: /labs/dsterba/gits/btrfs-progs/btrfs check --repair --force
> make: *** [Makefile:413: test-fsck] Error 1
>

Not reproducible locally.

And of course, for such lowlevel change I would run all tests on it.

Mind to share the base?

I'm basing all my patches on the following commit:

commit 06bae7076265242f118471ce8c4340dcc5e3f555 (david/devel)
Author: Josef Bacik <josef@toxicpanda.com>
Date:   Mon Aug 23 15:23:13 2021 -0400

     btrfs-progs: tests: add image with an invalid super bytes_used

And I also re-checked the test on this patch, it also passes locally.

Thanks,
Qu

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

[Qu Wenruo]

On 2021/9/4 上午6:23, Qu Wenruo wrote:
> 
> 
> On 2021/9/3 下午10:02, David Sterba wrote:
>> On Thu, Sep 02, 2021 at 09:08:41PM +0800, Qu Wenruo wrote:
>>> 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>
>>
>> This fails on fsck/001 test
>>
>>      [TEST/fsck]   001-bad-file-extent-bytenr
>> failed to restore image ./default_case.img
>> basename: missing operand
>> Try 'basename --help' for more information.
>> failed: /labs/dsterba/gits/btrfs-progs/btrfs check --repair --force
>> make: *** [Makefile:413: test-fsck] Error 1
>>
> 
> Not reproducible locally.
> 
> And of course, for such lowlevel change I would run all tests on it.
> 
> Mind to share the base?
> 
> I'm basing all my patches on the following commit:
> 
> commit 06bae7076265242f118471ce8c4340dcc5e3f555 (david/devel)
> Author: Josef Bacik <josef@toxicpanda.com>
> Date:   Mon Aug 23 15:23:13 2021 -0400
> 
>      btrfs-progs: tests: add image with an invalid super bytes_used
> 
> And I also re-checked the test on this patch, it also passes locally.
> 
> Thanks,
> Qu
> 
Based on the latest devel branch head:

Author: David Sterba <dsterba@suse.com>
Date:   Fri Aug 27 15:34:57 2021 +0200

     btrfs-progs: tests: add mkfs test for raid0/1 and raid10/2

Fsck still passes with all my patches applied, but only with the first 
patch it fails.

So there seems to be something conflicting, will update the patchset if 
it's my fault.

Thanks,
Qu

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

[Qu Wenruo]

On 2021/9/2 下午9:08, Qu Wenruo wrote:
> 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..c015c4f879c1 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)29368320
>   		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;29368320
>   	}
>   	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;

This is the cause of fsck tests failure.

@parent_key_ptr is not initialized, but I'm also wondering why compiler 
is not slapping a big warning onto my face.

Will update the patchset and even try to figure out why compiler is not 
helping me in this case.

Thanks,
Qu
>   	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,
> 

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

[Qu Wenruo]

On 2021/9/4 上午8:56, Qu Wenruo wrote:
> 
> 
> On 2021/9/2 下午9:08, Qu Wenruo wrote:
>> 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..c015c4f879c1 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)29368320
>>           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;29368320
>>       }
>>       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;
> 
> This is the cause of fsck tests failure.
> 
> @parent_key_ptr is not initialized, but I'm also wondering why compiler 
> is not slapping a big warning onto my face.

Not sure why but neither clang 12.0.1 nor 11.1.0 gives me any warning on 
the uninitialized pointer, even if -Wuninitialized is specified.

Any idea/suggestion to detect such uninitialized pointer?

Thanks,
Qu
> 
> Will update the patchset and even try to figure out why compiler is not 
> helping me in this case.
> 
> Thanks,
> Qu
>>       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,
>>
> 

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

[David Sterba]

On Sat, Sep 04, 2021 at 01:06:10PM +0800, Qu Wenruo wrote:
> >> -            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;
> > 
> > This is the cause of fsck tests failure.
> > 
> > @parent_key_ptr is not initialized, but I'm also wondering why compiler 
> > is not slapping a big warning onto my face.
> 
> Not sure why but neither clang 12.0.1 nor 11.1.0 gives me any warning on 
> the uninitialized pointer, even if -Wuninitialized is specified.
> 
> Any idea/suggestion to detect such uninitialized pointer?

My best guess is that the stack variables are implicitly initialized to
zero and there are checks inside the functions, and this somehow tells
the compiler not to report it as uninitialized. I'm running 11.1.1 and
don't see any warning with our without -Wuninitialized.