[PATCH v2 00/12] btrfs-progs: make check handle invalid bg items

[PATCH v2 00/12] btrfs-progs: make check handle invalid bg items

[Josef Bacik]

v1->v2:
- Discovered that we also don't check bytes_super in the superblock, add that
  checking and repair ability since it's coupled with the block group used
  repair.
- Discovered that we haven't actually been setting --mode=lowmem for the initial
  image check if we do make test-check-lowmem, we only do it after the repair.
  Fixed this.
- Now that we're properly testing error detection in all of the test cases, I
  found 3 problems with the --mode=lowmem mode, one infinite loop and two places
  we weren't properly propagating the error code up to the user.
- My super repair thing tripped a case where we wouldn't clean up properly for
  unaligned extent records, fixed this as well.
- Add another test image for the corrupted super bytes.
- Realize that you need a special .lowmem_repairable file in order for the
  lowmem repair code to run against images, so did that for both testcases.

--- Original email ---
Hello,

While writing code for extent tree v2 I noticed that I was generating a fs with
an invalid block group ->used value.  However fsck wasn't catching this, because
we don't actuall check the used value of the block group items in normal mode.
lowmem mode does this properly thankfully, so this only needs to be added to the
normal fsck mode.

I've added code to btrfs-corrupt-block to generate the corrupt image I need for
the test case.  Then of course the actual patch to detect and fix the problem.
Thanks,

Josef

Josef Bacik (12):
  btrfs-progs: fix running lowmem check tests
  btrfs-progs: do not infinite loop on corrupt keys with lowmem mode
  btrfs-progs: propagate fs root errors in lowmem mode
  btrfs-progs: propagate extent item errors in lowmem mode
  btrfs-progs: do not double add unaligned extent records
  btrfs-progs: add the ability to corrupt block group items
  btrfs-progs: add the ability to corrupt fields of the super block
  btrfs-progs: make check detect and fix invalid used for block groups
  btrfs-progs: make check detect and fix problems with super_bytes_used
  btrfs-progs: check btrfs_super_used in lowmem check
  btrfs-progs: add a test image with a corrupt block group item
  btrfs-progs: add a test image with an invalid super bytes_used

 btrfs-corrupt-block.c                         | 172 +++++++++++++++++-
 check/common.h                                |   5 +
 check/main.c                                  | 124 ++++++++++++-
 check/mode-lowmem.c                           |  25 ++-
 check/mode-lowmem.h                           |   1 +
 tests/common                                  |   5 +-
 .../.lowmem_repairable                        |   0
 .../default.img.xz                            | Bin 0 -> 1036 bytes
 .../.lowmem_repairable                        |   0
 .../default.img.xz                            | Bin 0 -> 1060 bytes
 10 files changed, 322 insertions(+), 10 deletions(-)
 create mode 100644 tests/fsck-tests/050-invalid-block-group-used/.lowmem_repairable
 create mode 100644 tests/fsck-tests/050-invalid-block-group-used/default.img.xz
 create mode 100644 tests/fsck-tests/051-invalid-super-bytes-used/.lowmem_repairable
 create mode 100644 tests/fsck-tests/051-invalid-super-bytes-used/default.img.xz

-- 
2.26.3

[PATCH v2 01/12] btrfs-progs: fix running lowmem check tests

[Josef Bacik]

When I added the invalid super image I saw that the lowmem tests were
passing, despite not having the detection code yet.  Turns out this is
because we weren't using a run command helper which does the proper
expansion and adds the --mode=lowmem option.  Fix this to use the proper
handler, and now the lowmem test fails properly without my patch to add
this support to the lowmem mode.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 tests/common | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/common b/tests/common
index 805a447c..5b255689 100644
--- a/tests/common
+++ b/tests/common
@@ -425,9 +425,8 @@ check_image()
 
 	image=$1
 	echo "testing image $(basename $image)" >> "$RESULTS"
-	"$TOP/btrfs" check "$image" >> "$RESULTS" 2>&1
-	[ $? -eq 0 ] && _fail "btrfs check should have detected corruption"
-
+	run_mustfail "btrfs check should have detected corruption" \
+		"$TOP/btrfs" check "$image"
 	run_check "$TOP/btrfs" check --repair --force "$image"
 	run_check "$TOP/btrfs" check "$image"
 }
-- 
2.26.3

[PATCH v2 02/12] btrfs-progs: do not infinite loop on corrupt keys with lowmem mode

[Josef Bacik]

By enabling the lowmem checks properly I uncovered the case where test
007 will infinite loop at the detection stage.  This is because when
checking the inode item we will just btrfs_next_item(), and because we
ignore check tree block failures at read time we don't get an -EIO from
btrfs_next_leaf.  Generally what check usually does is validate the
leaves/nodes as we hit them, but in this case we're not doing that.  Fix
this by checking the leaf if we move to the next one and if it fails
bail.  This allows us to pass the 007 test with lowmem.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 check/mode-lowmem.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 323e66bc..7fc7d467 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -2675,6 +2675,15 @@ static int check_inode_item(struct btrfs_root *root, struct btrfs_path *path)
 	while (1) {
 		btrfs_item_key_to_cpu(path->nodes[0], &last_key, path->slots[0]);
 		ret = btrfs_next_item(root, path);
+
+		/*
+		 * New leaf, we need to check it and see if it's valid, if not
+		 * we need to bail otherwise we could end up stuck.
+		 */
+		if (path->slots[0] == 0 &&
+		    btrfs_check_leaf(gfs_info, NULL, path->nodes[0]))
+			ret = -EIO;
+
 		if (ret < 0) {
 			/* out will fill 'err' rusing current statistics */
 			goto out;
-- 
2.26.3

[PATCH v2 03/12] btrfs-progs: propagate fs root errors in lowmem mode

[Josef Bacik]

We have a check that will return an error only if ret < 0, but we return
the lowmem specific errors which are all > 0.  Fix this by simply
checking if (ret).  This allows test 010 to pass with lowmem properly.

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

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 7fc7d467..d278c927 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -5204,7 +5204,7 @@ static int check_btrfs_root(struct btrfs_root *root, int check_all)
 		 * missing we will skip it forever.
 		 */
 		ret = check_fs_first_inode(root);
-		if (ret < 0)
+		if (ret)
 			return FATAL_ERROR;
 	}
 
-- 
2.26.3

[PATCH v2 04/12] btrfs-progs: propagate extent item errors in lowmem mode

[Josef Bacik]

Test 044 was failing with lowmem because it was not bubbling up the
error to the user.  This is because we try to allow repair the
opportunity to clear the error, however if repair isn't set we simply do
not add the temporary error to the main error return variable.  Fix this
by adding the tmp_err to err before moving on to the next item.

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

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index d278c927..14815519 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -4390,6 +4390,7 @@ next:
 		goto next;
 	}
 
+	err |= tmp_err;
 	ptr_offset += btrfs_extent_inline_ref_size(type);
 	goto next;
 
-- 
2.26.3

[PATCH v2 05/12] btrfs-progs: do not double add unaligned extent records

[Josef Bacik]

The repair cycle in the main check will drop all of our cache and loop
through again to make sure everything is still good to go.
Unfortunately we record our unaligned extent records on a per-root list
so they can be retrieved when we're checking the fs roots.  This isn't
straightforward to clean up, so instead simply check our current list of
unaligned extent records when we are adding a new one to make sure we're
not duplicating our efforts.  This makes us able to pass 001 with my
super bytes_used fix applied.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 check/main.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/check/main.c b/check/main.c
index a8851815..3f6db8f8 100644
--- a/check/main.c
+++ b/check/main.c
@@ -7892,6 +7892,8 @@ static int record_unaligned_extent_rec(struct extent_record *rec)
 
 	rbtree_postorder_for_each_entry_safe(back, tmp,
 					     &rec->backref_tree, node) {
+		bool skip = false;
+
 		if (back->full_backref || !back->is_data)
 			continue;
 
@@ -7907,6 +7909,24 @@ static int record_unaligned_extent_rec(struct extent_record *rec)
 		if (IS_ERR_OR_NULL(dest_root))
 			continue;
 
+		/*
+		 * If we repaired something and restarted we could potentially
+		 * try to add this unaligned record multiple times, so check
+		 * before we add a new one.
+		 */
+		list_for_each_entry(urec, &dest_root->unaligned_extent_recs,
+				    list) {
+			if (urec->objectid == dest_root->objectid &&
+			    urec->owner == dback->owner &&
+			    urec->bytenr == rec->start) {
+				skip = true;
+				break;
+			}
+		}
+
+		if (skip)
+			continue;
+
 		urec = malloc(sizeof(struct unaligned_extent_rec_t));
 		if (!urec)
 			return -ENOMEM;
-- 
2.26.3

[PATCH v2 06/12] btrfs-progs: add the ability to corrupt block group items

[Josef Bacik]

While doing the extent tree v2 stuff I noticed that fsck doesn't detect
an invalid ->used value on the block group item in the normal mode.  To
build a test case for this I need the ability to corrupt block group
items.  This allows us to corrupt the various fields of a block group.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 btrfs-corrupt-block.c | 108 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 107 insertions(+), 1 deletion(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 77bdc810..80622f29 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -348,6 +348,24 @@ enum btrfs_key_field {
 	BTRFS_KEY_BAD,
 };
 
+enum btrfs_block_group_field {
+	BTRFS_BLOCK_GROUP_ITEM_USED,
+	BTRFS_BLOCK_GROUP_ITEM_FLAGS,
+	BTRFS_BLOCK_GROUP_ITEM_CHUNK_OBJECTID,
+	BTRFS_BLOCK_GROUP_ITEM_BAD,
+};
+
+static enum btrfs_block_group_field convert_block_group_field(char *field)
+{
+	if (!strncmp(field, "used", FIELD_BUF_LEN))
+		return BTRFS_BLOCK_GROUP_ITEM_USED;
+	if (!strncmp(field, "flags", FIELD_BUF_LEN))
+		return BTRFS_BLOCK_GROUP_ITEM_FLAGS;
+	if (!strncmp(field, "chunk_objectid", FIELD_BUF_LEN))
+		return BTRFS_BLOCK_GROUP_ITEM_CHUNK_OBJECTID;
+	return BTRFS_BLOCK_GROUP_ITEM_BAD;
+}
+
 static enum btrfs_inode_field convert_inode_field(char *field)
 {
 	if (!strncmp(field, "isize", FIELD_BUF_LEN))
@@ -442,6 +460,83 @@ static u8 generate_u8(u8 orig)
 	return ret;
 }
 
+static int corrupt_block_group(struct btrfs_root *root, u64 bg, char *field)
+{
+	struct btrfs_trans_handle *trans;
+	struct btrfs_path *path;
+	struct btrfs_block_group_item *bgi;
+	struct btrfs_key key;
+	enum btrfs_block_group_field corrupt_field;
+	u64 orig, bogus;
+	int ret = 0;
+
+	root = root->fs_info->extent_root;
+
+	corrupt_field = convert_block_group_field(field);
+	if (corrupt_field == BTRFS_BLOCK_GROUP_ITEM_BAD) {
+		fprintf(stderr, "Invalid field %s\n", field);
+		return -EINVAL;
+	}
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
+	trans = btrfs_start_transaction(root, 1);
+	if (IS_ERR(trans)) {
+		btrfs_free_path(path);
+		fprintf(stderr, "Couldn't start transaction %ld\n",
+			PTR_ERR(trans));
+		return PTR_ERR(trans);
+	}
+
+	key.objectid = bg;
+	key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
+	key.offset = 0;
+
+	ret = btrfs_search_slot(trans, root, &key, path, 0, 1);
+	if (ret < 0) {
+		fprintf(stderr, "Error searching for bg %llu %d\n", bg, ret);
+		goto out;
+	}
+
+	ret = 0;
+	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
+	if (key.type != BTRFS_BLOCK_GROUP_ITEM_KEY) {
+		fprintf(stderr, "Couldn't find the bg %llu\n", bg);
+		goto out;
+	}
+
+	bgi = btrfs_item_ptr(path->nodes[0], path->slots[0],
+			     struct btrfs_block_group_item);
+	switch (corrupt_field) {
+	case BTRFS_BLOCK_GROUP_ITEM_USED:
+		orig = btrfs_block_group_used(path->nodes[0], bgi);
+		bogus = generate_u64(orig);
+		btrfs_set_block_group_used(path->nodes[0], bgi, bogus);
+		break;
+	case BTRFS_BLOCK_GROUP_ITEM_CHUNK_OBJECTID:
+		orig = btrfs_block_group_chunk_objectid(path->nodes[0], bgi);
+		bogus = generate_u64(orig);
+		btrfs_set_block_group_chunk_objectid(path->nodes[0], bgi,
+						     bogus);
+		break;
+	case BTRFS_BLOCK_GROUP_ITEM_FLAGS:
+		orig = btrfs_block_group_flags(path->nodes[0], bgi);
+		bogus = generate_u64(orig);
+		btrfs_set_block_group_flags(path->nodes[0], bgi, bogus);
+		break;
+	default:
+		ret = -EINVAL;
+		goto out;
+	}
+	btrfs_mark_buffer_dirty(path->nodes[0]);
+out:
+	btrfs_commit_transaction(trans, root);
+	btrfs_free_path(path);
+	return ret;
+}
+
 static int corrupt_key(struct btrfs_root *root, struct btrfs_key *key,
 		       char *field)
 {
@@ -1150,6 +1245,7 @@ int main(int argc, char **argv)
 	u64 file_extent = (u64)-1;
 	u64 root_objectid = 0;
 	u64 csum_bytenr = 0;
+	u64 block_group = 0;
 	char field[FIELD_BUF_LEN];
 
 	field[0] = '\0';
@@ -1177,11 +1273,12 @@ int main(int argc, char **argv)
 			{ "delete", no_argument, NULL, 'd'},
 			{ "root", no_argument, NULL, 'r'},
 			{ "csum", required_argument, NULL, 'C'},
+			{ "block-group", required_argument, NULL, 'B'},
 			{ "help", no_argument, NULL, GETOPT_VAL_HELP},
 			{ NULL, 0, NULL, 0 }
 		};
 
-		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:D:d:r:C:",
+		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:D:d:r:C:B:",
 				long_options, NULL);
 		if (c < 0)
 			break;
@@ -1244,6 +1341,9 @@ int main(int argc, char **argv)
 			case 'C':
 				csum_bytenr = arg_strtou64(optarg);
 				break;
+			case 'B':
+				block_group = arg_strtou64(optarg);
+				break;
 			case GETOPT_VAL_HELP:
 			default:
 				print_usage(c != GETOPT_VAL_HELP);
@@ -1385,6 +1485,12 @@ int main(int argc, char **argv)
 		ret = corrupt_key(target_root, &key, field);
 		goto out_close;
 	}
+	if (block_group) {
+		if (*field == 0)
+			print_usage(1);
+		ret = corrupt_block_group(root, block_group, field);
+		goto out_close;
+	}
 	/*
 	 * If we made it here and we have extent set then we didn't specify
 	 * inode and we're screwed.
-- 
2.26.3

[PATCH v2 07/12] btrfs-progs: add the ability to corrupt fields of the super block

[Josef Bacik]

Doing the extent tree v2 work I generated an invalid super block with
the wrong bytes_used set, and only noticed because it affected the block
groups as well.  Neither modes of fsck check for a valid bytes_used, so
add the ability to corrupt this field so I can generate a testcase for
fsck.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 btrfs-corrupt-block.c | 66 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 80622f29..7e576897 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -355,6 +355,24 @@ enum btrfs_block_group_field {
 	BTRFS_BLOCK_GROUP_ITEM_BAD,
 };
 
+enum btrfs_super_field {
+	BTRFS_SUPER_FIELD_FLAGS,
+	BTRFS_SUPER_FIELD_TOTAL_BYTES,
+	BTRFS_SUPER_FIELD_BYTES_USED,
+	BTRFS_SUPER_FIELD_BAD,
+};
+
+static enum btrfs_super_field convert_super_field(char *field)
+{
+	if (!strncmp(field, "flags", FIELD_BUF_LEN))
+		return BTRFS_SUPER_FIELD_FLAGS;
+	if (!strncmp(field, "total_bytes", FIELD_BUF_LEN))
+		return BTRFS_SUPER_FIELD_TOTAL_BYTES;
+	if (!strncmp(field, "bytes_used", FIELD_BUF_LEN))
+		return BTRFS_SUPER_FIELD_BYTES_USED;
+	return BTRFS_SUPER_FIELD_BAD;
+}
+
 static enum btrfs_block_group_field convert_block_group_field(char *field)
 {
 	if (!strncmp(field, "used", FIELD_BUF_LEN))
@@ -460,6 +478,41 @@ static u8 generate_u8(u8 orig)
 	return ret;
 }
 
+static int corrupt_super_block(struct btrfs_root *root, char *field)
+{
+	struct btrfs_fs_info *fs_info = root->fs_info;
+	enum btrfs_super_field corrupt_field;
+	u64 orig, bogus;
+
+	corrupt_field = convert_super_field(field);
+	if (corrupt_field == BTRFS_SUPER_FIELD_BAD) {
+		fprintf(stderr, "Invalid field %s\n", field);
+		return -EINVAL;
+	}
+
+	switch (corrupt_field) {
+	case BTRFS_SUPER_FIELD_FLAGS:
+		orig = btrfs_super_flags(fs_info->super_copy);
+		bogus = generate_u64(orig);
+		btrfs_set_super_flags(fs_info->super_copy, bogus);
+		break;
+	case BTRFS_SUPER_FIELD_TOTAL_BYTES:
+		orig = btrfs_super_total_bytes(fs_info->super_copy);
+		bogus = generate_u64(orig);
+		btrfs_set_super_total_bytes(fs_info->super_copy, bogus);
+		break;
+	case BTRFS_SUPER_FIELD_BYTES_USED:
+		orig = btrfs_super_bytes_used(fs_info->super_copy);
+		bogus = generate_u64(orig);
+		btrfs_set_super_bytes_used(fs_info->super_copy, bogus);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return write_all_supers(fs_info);
+}
+
 static int corrupt_block_group(struct btrfs_root *root, u64 bg, char *field)
 {
 	struct btrfs_trans_handle *trans;
@@ -1240,6 +1293,7 @@ int main(int argc, char **argv)
 	int corrupt_di = 0;
 	int delete = 0;
 	int should_corrupt_key = 0;
+	int corrupt_super = 0;
 	u64 metadata_block = 0;
 	u64 inode = 0;
 	u64 file_extent = (u64)-1;
@@ -1274,11 +1328,12 @@ int main(int argc, char **argv)
 			{ "root", no_argument, NULL, 'r'},
 			{ "csum", required_argument, NULL, 'C'},
 			{ "block-group", required_argument, NULL, 'B'},
+			{ "super", no_argument, NULL, 's'},
 			{ "help", no_argument, NULL, GETOPT_VAL_HELP},
 			{ NULL, 0, NULL, 0 }
 		};
 
-		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:D:d:r:C:B:",
+		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:D:d:r:C:B:s",
 				long_options, NULL);
 		if (c < 0)
 			break;
@@ -1344,6 +1399,9 @@ int main(int argc, char **argv)
 			case 'B':
 				block_group = arg_strtou64(optarg);
 				break;
+			case 's':
+				corrupt_super = 1;
+				break;
 			case GETOPT_VAL_HELP:
 			default:
 				print_usage(c != GETOPT_VAL_HELP);
@@ -1491,6 +1549,12 @@ int main(int argc, char **argv)
 		ret = corrupt_block_group(root, block_group, field);
 		goto out_close;
 	}
+	if (corrupt_super) {
+		if (*field == 0)
+			print_usage(1);
+		ret = corrupt_super_block(root, field);
+		goto out_close;
+	}
 	/*
 	 * If we made it here and we have extent set then we didn't specify
 	 * inode and we're screwed.
-- 
2.26.3

[PATCH v2 08/12] btrfs-progs: make check detect and fix invalid used for block groups

[Josef Bacik]

The lowmem mode validates the used field of the block group item, but
the normal mode does not.  Fix this by keeping a running tally of what
we think the used value for the block group should be, and then if it
mismatches report an error and fix the problem if we have repair set.
We have to keep track of pending extents because we process leaves as we
see them, so it could be much later in the process that we find the
block group item to associate the extents with.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 check/common.h |  5 +++
 check/main.c   | 89 +++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/check/common.h b/check/common.h
index e72379a0..ba4e291e 100644
--- a/check/common.h
+++ b/check/common.h
@@ -37,10 +37,14 @@ struct block_group_record {
 	u64 offset;
 
 	u64 flags;
+
+	u64 disk_used;
+	u64 actual_used;
 };
 
 struct block_group_tree {
 	struct cache_tree tree;
+	struct extent_io_tree pending_extents;
 	struct list_head block_groups;
 };
 
@@ -141,6 +145,7 @@ u64 calc_stripe_length(u64 type, u64 length, int num_stripes);
 static inline void block_group_tree_init(struct block_group_tree *tree)
 {
 	cache_tree_init(&tree->tree);
+	extent_io_tree_init(&tree->pending_extents);
 	INIT_LIST_HEAD(&tree->block_groups);
 }
 
diff --git a/check/main.c b/check/main.c
index 3f6db8f8..af9e0ff3 100644
--- a/check/main.c
+++ b/check/main.c
@@ -5083,9 +5083,27 @@ static void free_block_group_record(struct cache_extent *cache)
 
 void free_block_group_tree(struct block_group_tree *tree)
 {
+	extent_io_tree_cleanup(&tree->pending_extents);
 	cache_tree_free_extents(&tree->tree, free_block_group_record);
 }
 
+static void update_block_group_used(struct block_group_tree *tree,
+				    u64 bytenr, u64 num_bytes)
+{
+	struct cache_extent *bg_item;
+	struct block_group_record *bg_rec;
+
+	bg_item = lookup_cache_extent(&tree->tree, bytenr, num_bytes);
+	if (!bg_item) {
+		set_extent_dirty(&tree->pending_extents, bytenr,
+				 bytenr + num_bytes - 1);
+		return;
+	}
+	bg_rec = container_of(bg_item, struct block_group_record,
+			      cache);
+	bg_rec->actual_used += num_bytes;
+}
+
 int insert_device_extent_record(struct device_extent_tree *tree,
 				struct device_extent_record *de_rec)
 {
@@ -5270,6 +5288,7 @@ btrfs_new_block_group_record(struct extent_buffer *leaf, struct btrfs_key *key,
 
 	ptr = btrfs_item_ptr(leaf, slot, struct btrfs_block_group_item);
 	rec->flags = btrfs_block_group_flags(leaf, ptr);
+	rec->disk_used = btrfs_block_group_used(leaf, ptr);
 
 	INIT_LIST_HEAD(&rec->list);
 
@@ -5281,6 +5300,7 @@ static int process_block_group_item(struct block_group_tree *block_group_cache,
 				    struct extent_buffer *eb, int slot)
 {
 	struct block_group_record *rec;
+	u64 start, end;
 	int ret = 0;
 
 	rec = btrfs_new_block_group_record(eb, key, slot);
@@ -5289,6 +5309,22 @@ static int process_block_group_item(struct block_group_tree *block_group_cache,
 		fprintf(stderr, "Block Group[%llu, %llu] existed.\n",
 			rec->objectid, rec->offset);
 		free(rec);
+		return ret;
+	}
+
+	while (!find_first_extent_bit(&block_group_cache->pending_extents,
+				      rec->objectid, &start, &end,
+				      EXTENT_DIRTY)) {
+		u64 len;
+
+		if (start >= rec->objectid + rec->offset)
+			break;
+		start = max(start, rec->objectid);
+		len = min(end - start + 1,
+			  rec->objectid + rec->offset - start);
+		rec->actual_used += len;
+		clear_extent_dirty(&block_group_cache->pending_extents, start,
+				   start + len - 1);
 	}
 
 	return ret;
@@ -5352,6 +5388,7 @@ process_device_extent_item(struct device_extent_tree *dev_extent_cache,
 
 static int process_extent_item(struct btrfs_root *root,
 			       struct cache_tree *extent_cache,
+			       struct block_group_tree *block_group_cache,
 			       struct extent_buffer *eb, int slot)
 {
 	struct btrfs_extent_item *ei;
@@ -5380,6 +5417,8 @@ static int process_extent_item(struct btrfs_root *root,
 		num_bytes = key.offset;
 	}
 
+	update_block_group_used(block_group_cache, key.objectid, num_bytes);
+
 	if (!IS_ALIGNED(key.objectid, gfs_info->sectorsize)) {
 		error("ignoring invalid extent, bytenr %llu is not aligned to %u",
 		      key.objectid, gfs_info->sectorsize);
@@ -6348,13 +6387,13 @@ static int run_next_block(struct btrfs_root *root,
 				continue;
 			}
 			if (key.type == BTRFS_EXTENT_ITEM_KEY) {
-				process_extent_item(root, extent_cache, buf,
-						    i);
+				process_extent_item(root, extent_cache,
+						    block_group_cache, buf, i);
 				continue;
 			}
 			if (key.type == BTRFS_METADATA_ITEM_KEY) {
-				process_extent_item(root, extent_cache, buf,
-						    i);
+				process_extent_item(root, extent_cache,
+						    block_group_cache, buf, i);
 				continue;
 			}
 			if (key.type == BTRFS_EXTENT_CSUM_KEY) {
@@ -8619,6 +8658,41 @@ static int deal_root_from_list(struct list_head *list,
 	return ret;
 }
 
+static int check_block_groups(struct block_group_tree *bg_cache)
+{
+	struct btrfs_trans_handle *trans;
+	struct cache_extent *item;
+	struct block_group_record *bg_rec;
+	int ret = 0;
+
+	for (item = first_cache_extent(&bg_cache->tree); item;
+	     item = next_cache_extent(item)) {
+		bg_rec = container_of(item, struct block_group_record,
+				      cache);
+		if (bg_rec->disk_used == bg_rec->actual_used)
+			continue;
+		fprintf(stderr,
+			"block group[%llu %llu] used %llu but extent items used %llu\n",
+			bg_rec->objectid, bg_rec->offset, bg_rec->disk_used,
+			bg_rec->actual_used);
+		ret = -1;
+	}
+
+	if (!repair || !ret)
+		return ret;
+
+	trans = btrfs_start_transaction(gfs_info->extent_root, 1);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		fprintf(stderr, "Failed to start a transaction\n");
+		return ret;
+	}
+
+	ret = btrfs_fix_block_accounting(trans);
+	btrfs_commit_transaction(trans, gfs_info->extent_root);
+	return ret ? ret : -EAGAIN;
+}
+
 /**
  * parse_tree_roots - Go over all roots in the tree root and add each one to
  *		      a list.
@@ -8910,6 +8984,13 @@ again:
 		goto out;
 	}
 
+	ret = check_block_groups(&block_group_cache);
+	if (ret) {
+		if (ret == -EAGAIN)
+			goto loop;
+		goto out;
+	}
+
 	ret = check_devices(&dev_cache, &dev_extent_cache);
 	if (ret && err)
 		ret = err;
-- 
2.26.3

[PATCH v2 09/12] btrfs-progs: make check detect and fix problems with super_bytes_used

[Josef Bacik]

We do not detect problems with our bytes_used counter in the super
block.  Thankfully the same method to fix block groups is used to re-set
the value in the super block, so simply add some extra code to validate
the bytes_used field and then piggy back on the repair code for block
groups.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 check/main.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/check/main.c b/check/main.c
index af9e0ff3..b1b1b866 100644
--- a/check/main.c
+++ b/check/main.c
@@ -8663,12 +8663,14 @@ static int check_block_groups(struct block_group_tree *bg_cache)
 	struct btrfs_trans_handle *trans;
 	struct cache_extent *item;
 	struct block_group_record *bg_rec;
+	u64 used = 0;
 	int ret = 0;
 
 	for (item = first_cache_extent(&bg_cache->tree); item;
 	     item = next_cache_extent(item)) {
 		bg_rec = container_of(item, struct block_group_record,
 				      cache);
+		used += bg_rec->actual_used;
 		if (bg_rec->disk_used == bg_rec->actual_used)
 			continue;
 		fprintf(stderr,
@@ -8678,6 +8680,19 @@ static int check_block_groups(struct block_group_tree *bg_cache)
 		ret = -1;
 	}
 
+	/*
+	 * We check the super bytes_used here because it's the sum of all block
+	 * groups used, and the repair actually happens in
+	 * btrfs_fix_block_accounting, so we can kill both birds with the same
+	 * stone here.
+	 */
+	if (used != btrfs_super_bytes_used(gfs_info->super_copy)) {
+		fprintf(stderr,
+			"super bytes used %llu mismatches actual used %llu\n",
+			btrfs_super_bytes_used(gfs_info->super_copy), used);
+		ret = -1;
+	}
+
 	if (!repair || !ret)
 		return ret;
 
-- 
2.26.3

[PATCH v2 10/12] btrfs-progs: check btrfs_super_used in lowmem check

[Josef Bacik]

We can already fix this problem with the block accounting code, we just
need to keep track of how much we should have used on the file system,
and then check it against the bytes_super.  The repair just piggy backs
on the block group used repair.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 check/mode-lowmem.c | 13 ++++++++++++-
 check/mode-lowmem.h |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 14815519..dacc5445 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -28,6 +28,7 @@
 #include "check/mode-lowmem.h"
 
 static u64 last_allocated_chunk;
+static u64 total_used = 0;
 
 static int calc_extent_flag(struct btrfs_root *root, struct extent_buffer *eb,
 			    u64 *flags_ret)
@@ -3654,6 +3655,8 @@ next:
 out:
 	btrfs_release_path(&path);
 
+	total_used += used;
+
 	if (total != used) {
 		error(
 		"block group[%llu %llu] used %llu but extent items used %llu",
@@ -5556,6 +5559,14 @@ next:
 	}
 out:
 
+	if (total_used != btrfs_super_bytes_used(gfs_info->super_copy)) {
+		fprintf(stderr,
+			"super bytes used %llu mismatches actual used %llu\n",
+			btrfs_super_bytes_used(gfs_info->super_copy),
+			total_used);
+		err |= SUPER_BYTES_USED_ERROR;
+	}
+
 	if (repair) {
 		ret = end_avoid_extents_overwrite();
 		if (ret < 0)
@@ -5568,7 +5579,7 @@ out:
 		if (ret)
 			err |= ret;
 		else
-			err &= ~BG_ACCOUNTING_ERROR;
+			err &= ~(BG_ACCOUNTING_ERROR|SUPER_BYTES_USED_ERROR);
 	}
 
 	btrfs_release_path(&path);
diff --git a/check/mode-lowmem.h b/check/mode-lowmem.h
index da9f8600..0bcc338b 100644
--- a/check/mode-lowmem.h
+++ b/check/mode-lowmem.h
@@ -48,6 +48,7 @@
 #define DIR_ITEM_HASH_MISMATCH	(1<<24) /* Dir item hash mismatch */
 #define INODE_MODE_ERROR	(1<<25) /* Bad inode mode */
 #define INVALID_GENERATION	(1<<26)	/* Generation is too new */
+#define SUPER_BYTES_USED_ERROR	(1<<27)	/* Super bytes_used is invalid */
 
 /*
  * Error bit for low memory mode check.
-- 
2.26.3

[PATCH v2 11/12] btrfs-progs: add a test image with a corrupt block group item

[Josef Bacik]

This image has a broken used field of a block group item to validate
fsck does the correct thing.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 .../.lowmem_repairable                           |   0
 .../050-invalid-block-group-used/default.img.xz  | Bin 0 -> 1036 bytes
 2 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/fsck-tests/050-invalid-block-group-used/.lowmem_repairable
 create mode 100644 tests/fsck-tests/050-invalid-block-group-used/default.img.xz

diff --git a/tests/fsck-tests/050-invalid-block-group-used/.lowmem_repairable b/tests/fsck-tests/050-invalid-block-group-used/.lowmem_repairable
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/fsck-tests/050-invalid-block-group-used/default.img.xz b/tests/fsck-tests/050-invalid-block-group-used/default.img.xz
new file mode 100644
index 0000000000000000000000000000000000000000..6425ba16349395416fc1918ce4c386059d618409
GIT binary patch
literal 1036
zcmV+n1oQj-H+ooF000E$*0e?f03iVu0001VFXf}+6aNFrT>wRyj;C3^v%$$4d1rE0
zjjaF1$8Jv*pMMbE@{&83eAipquzP5y*Z^dMFaaah{5${uN^u50JUj3Gv&TKFetZNb
zeGi-h_Sc-xdQ?TDr}!X-75pO<Qv3dfP)udQ#2BMlhKV!<)h0wx#0gBFB3oujWhIL&
zL%Q_)XZGuT<s*G=df*h(28)|6jezRgn^fw3f_X9cr>3s7;Q#Cs@eB}(-tDiTrOkuL
zr-`Q{!4@d7$vYR${O40b$KD!sxV##0^*`8ePjSki5__Mw0>a{sam+nk%_Taf1QUIl
zCPC+p!>ol%J~2Y3anuU4qT<_;R2!C0z*;OS_X<C(1PY_piT;4cC;rb`fiOWnUh;Y5
z+VpHb7TtwJG9h(H?eIvX{dw_9_d^y!4q2*1)lacoD~cQb2zYFYf$yU6F73JuP-a_~
z^nRPN>lH|4cwp*YYCcEjpgm>gLEL94KJ)NIDw(fAxBE)HBeuH?SFG)P2n*qCcX@dj
zl6!cB_HK~tM6u@FF~?28<QXd$N2j((D}P0X*4u&8MljFY;=CO4qv}A|Yfk0*J1L<0
zvtlIvVt#RxD#E>(XL*Vme;}!4%~G!$l6uxm(%@F-`~K9;P=VVx6iDWsw2&F)MC!ne
z{yU2uSdn?OJQ<Lt*^Y0*dZ}@9Mbh>k99D$H$r|?eQ%u-&&dW^Q%ogbbG<!ht<u}AO
zPK!nm?T;rPEW*}JR&_-PG_1)0l`D=?gQ%=PFA7tIi*Hc#g_0U1=2f8&+0+M2vL0@Y
zRepZ8%3sj#@uRLS03@G+a$5t}CT2~i>`T74qDMIaXhLRLP+<sRw5KZtAG@&9m`Ls0
zS5b66v3%FLAinwOLU5f}@`Y!nIlBhSV}9k)|B|7ydoH$5f8x-VX7s|n=;9N;KXH?H
z@kBA19X?ijYFlm44FX3(6!04T9(Yd!8hL5JWd}`cJ{=UOnZVNi<^S&%@5!isxf&15
z3`Qf*v}A6h?C9<3y1jv7oiCsGu3lhGk{+-4VN_WAkphoN6}JCC>j+OmK|vlc6+Q+8
z>i9;tP|5yL$Hg>IZhAK@v(3YzO&IxH_W<yblu5-O+ic3l3LzTL=eJ!H;1FmLNMh9z
zP}b#vH2+}dMT<T6wQ}~GA0mWw5XZl*-GXoYp{R!3<B4{}7<uJK<0J_L%nX=dW8jjl
zJo6(aM0F6xZK|Og`Pbn0U-IJOVOIUa)MDdsCv%q+fGEK_y4SF$G)B_i)~Dk);s!z~
zd&otC2e}ecZOkpbFV;}uV=M0f0000E#S;ff2MZnm0p$mPs0aWTvKPg%#Ao{g00000
G1X)_aZ2)fo

literal 0
HcmV?d00001

-- 
2.26.3

[PATCH v2 12/12] btrfs-progs: add a test image with an invalid super bytes_used

[Josef Bacik]

This is used to validate the detection and correction code in both fsck
modes for an invalid bytes_used value in the super block.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 .../.lowmem_repairable                           |   0
 .../051-invalid-super-bytes-used/default.img.xz  | Bin 0 -> 1060 bytes
 2 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/fsck-tests/051-invalid-super-bytes-used/.lowmem_repairable
 create mode 100644 tests/fsck-tests/051-invalid-super-bytes-used/default.img.xz

diff --git a/tests/fsck-tests/051-invalid-super-bytes-used/.lowmem_repairable b/tests/fsck-tests/051-invalid-super-bytes-used/.lowmem_repairable
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/fsck-tests/051-invalid-super-bytes-used/default.img.xz b/tests/fsck-tests/051-invalid-super-bytes-used/default.img.xz
new file mode 100644
index 0000000000000000000000000000000000000000..20d6af898690038ba5e3f7f3852f699413a7f15a
GIT binary patch
literal 1060
zcmV+<1l#-lH+ooF000E$*0e?f03iVu0001VFXf}+6aNF`T>wRyj;C3^v%$$4d1ocf
zjjaF1$8Jv*pMMm%#9U8wBSY$t`hql!7fWN#!<KoG7QbgI5Zz59AG36Vz1Z(#I0~Ln
z3JKK~H5jCU<u;c6IpGR%0p(Y1WzO5tieFMrVv97@7Gm*yDDf8GPy!w=xShqKM$Hbm
zNcld}wm#`Gky6*aiFfO+#g4Pry^cWVoFmH0VBeS~uQB0-tE>9)m~LUu{3h{cCV}n!
zW)j69{DZ;^4fwk&(@SrTPYl1&-d)dlKfc5eTWtvaaT3+KYdkYPq^__%7Cr|^=lv8x
z%X#@fQz>K*kIy&~JE`|+LnwNXr>^@~gg`$X!^enj{MM+Qk;qkdaYSe0zikAnG$AyN
z%hIp4{EhUpHVZf~sgUl}ybGx$gBffDVBF7(%IweseM7z#2OYAV_+^+V+j+fA%W}fc
ztW$!wvbbD7mmZTXE=T>9l4%pam5o<OA~-y2ZDRM-QL~v6TDXKleUF?ALnGlA7~&lR
z0q!@;zV*Y$Qh)gnX(oJR*Iu0}O>Wqfy31;MWh?)Wit!p!yl!SKcNBMb`cc>fa<5G6
zCPLe%*Vw+j@@8j4Sn<j~apvG>hM2ja(Ycw(;E=od)VY^PtFDz(Y?dy_HfQ}3Y20gS
z-t+TT?)DNjw`uj9E#Xdmutgz=9)f=XJ){7(wKS`QI)KkMJGX^;Mw}5CVh3&e-|Nz8
z#xJ^>@ol)mT8Mg(u=fuV%zGVUY0JGnTk<f>2tUD57AmS(<o(nfm6zE)U|?D+{k~s}
z5`lUF<7(8yv^0iUH*Ps&US#EKgBpJ#)F~Np^2gMoB5@yIdz(nik(sj?*J7sWVnw1s
zorCvu>Cx%M<Y6H_Q~HoHzM208Z&CJ<3W!YP>D0RE-&Wlc){tTY21u}MNMj3C?bZOg
zYNo{!+@KC%>W3*ihg`;{_lWQvsRXSfgLOq#)6xKI6_u~N5hsk{o<;;yEi>!k>sTgc
z$BdTv^NvrZ$?Z8&jqFAaE4bR>22AD#3*mR8U$!asT12H?EQuLPCrIcH7el(xev3hL
zQAZ|(`U@M8)fHnVK^&S4EMN{O5{g9PF}@wqoi=P39rIae4i8Y~TSurLWEZ6Sxx~s~
zXWtlT7;2?3<S;PdEkVsK4-X>Z=0eg@s@X`Pb0)`FgKF=kq5e?)|3dK)AYPa%)=75r
zJ1rQgvK#p3?1^k+W2E1T-3mMF>o8h_=GipTN`KdfpZ&&w+3F%(bk;sr{ITgm@F<x-
z-=1Yn$&27By)uiajG+wuCz_6dz;K{fS0x7)SmmNq{N)3a2BtnE463(n5vKrsu3*jw
e*LMW~0e}dAs0aWryM7<B#Ao{g000001X)^oc?S^y

literal 0
HcmV?d00001

-- 
2.26.3

Re: [PATCH v2 01/12] btrfs-progs: fix running lowmem check tests

[Qu Wenruo]

On 2021/8/19 上午5:33, Josef Bacik wrote:
> When I added the invalid super image I saw that the lowmem tests were
> passing, despite not having the detection code yet.  Turns out this is
> because we weren't using a run command helper which does the proper
> expansion and adds the --mode=lowmem option.  Fix this to use the proper
> handler, and now the lowmem test fails properly without my patch to add
> this support to the lowmem mode.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   tests/common | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tests/common b/tests/common
> index 805a447c..5b255689 100644
> --- a/tests/common
> +++ b/tests/common
> @@ -425,9 +425,8 @@ check_image()
>
>   	image=$1
>   	echo "testing image $(basename $image)" >> "$RESULTS"
> -	"$TOP/btrfs" check "$image" >> "$RESULTS" 2>&1
> -	[ $? -eq 0 ] && _fail "btrfs check should have detected corruption"
> -
> +	run_mustfail "btrfs check should have detected corruption" \
> +		"$TOP/btrfs" check "$image"
>   	run_check "$TOP/btrfs" check --repair --force "$image"
>   	run_check "$TOP/btrfs" check "$image"
>   }
>

Re: [PATCH v2 02/12] btrfs-progs: do not infinite loop on corrupt keys with lowmem mode

[Qu Wenruo]

On 2021/8/19 上午5:33, Josef Bacik wrote:
> By enabling the lowmem checks properly I uncovered the case where test
> 007 will infinite loop at the detection stage.  This is because when
> checking the inode item we will just btrfs_next_item(), and because we
> ignore check tree block failures at read time we don't get an -EIO from
> btrfs_next_leaf.  Generally what check usually does is validate the
> leaves/nodes as we hit them, but in this case we're not doing that.  Fix
> this by checking the leaf if we move to the next one and if it fails
> bail.  This allows us to pass the 007 test with lowmem.

Doesn't this mean btrfs_next_item() is not doing what it should do?

Normally we would expect btrfs_next_item() to return -EIO other than
manually checking the returned leaf.

Thanks,
Qu
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>   check/mode-lowmem.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 323e66bc..7fc7d467 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -2675,6 +2675,15 @@ static int check_inode_item(struct btrfs_root *root, struct btrfs_path *path)
>   	while (1) {
>   		btrfs_item_key_to_cpu(path->nodes[0], &last_key, path->slots[0]);
>   		ret = btrfs_next_item(root, path);
> +
> +		/*
> +		 * New leaf, we need to check it and see if it's valid, if not
> +		 * we need to bail otherwise we could end up stuck.
> +		 */
> +		if (path->slots[0] == 0 &&
> +		    btrfs_check_leaf(gfs_info, NULL, path->nodes[0]))
> +			ret = -EIO;
> +
>   		if (ret < 0) {
>   			/* out will fill 'err' rusing current statistics */
>   			goto out;
>

Re: [PATCH v2 03/12] btrfs-progs: propagate fs root errors in lowmem mode

[Qu Wenruo]

On 2021/8/19 上午5:33, Josef Bacik wrote:
> We have a check that will return an error only if ret < 0, but we return
> the lowmem specific errors which are all > 0.  Fix this by simply
> checking if (ret).  This allows test 010 to pass with lowmem properly.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   check/mode-lowmem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 7fc7d467..d278c927 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -5204,7 +5204,7 @@ static int check_btrfs_root(struct btrfs_root *root, int check_all)
>   		 * missing we will skip it forever.
>   		 */
>   		ret = check_fs_first_inode(root);
> -		if (ret < 0)
> +		if (ret)
>   			return FATAL_ERROR;
>   	}
>
>

Re: [PATCH v2 04/12] btrfs-progs: propagate extent item errors in lowmem mode

[Qu Wenruo]

On 2021/8/19 上午5:33, Josef Bacik wrote:
> Test 044 was failing with lowmem because it was not bubbling up the
> error to the user.  This is because we try to allow repair the
> opportunity to clear the error, however if repair isn't set we simply do
> not add the temporary error to the main error return variable.  Fix this
> by adding the tmp_err to err before moving on to the next item.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   check/mode-lowmem.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index d278c927..14815519 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -4390,6 +4390,7 @@ next:
>   		goto next;
>   	}
>
> +	err |= tmp_err;
>   	ptr_offset += btrfs_extent_inline_ref_size(type);
>   	goto next;
>
>

Re: [PATCH v2 08/12] btrfs-progs: make check detect and fix invalid used for block groups

[Qu Wenruo]

On 2021/8/19 上午5:33, Josef Bacik wrote:
> The lowmem mode validates the used field of the block group item, but
> the normal mode does not.  Fix this by keeping a running tally of what
> we think the used value for the block group should be, and then if it
> mismatches report an error and fix the problem if we have repair set.
> We have to keep track of pending extents because we process leaves as we
> see them, so it could be much later in the process that we find the
> block group item to associate the extents with.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>   check/common.h |  5 +++
>   check/main.c   | 89 +++++++++++++++++++++++++++++++++++++++++++++++---
>   2 files changed, 90 insertions(+), 4 deletions(-)
>
> diff --git a/check/common.h b/check/common.h
> index e72379a0..ba4e291e 100644
> --- a/check/common.h
> +++ b/check/common.h
> @@ -37,10 +37,14 @@ struct block_group_record {
>   	u64 offset;
>
>   	u64 flags;
> +
> +	u64 disk_used;
> +	u64 actual_used;
>   };
>
>   struct block_group_tree {
>   	struct cache_tree tree;
> +	struct extent_io_tree pending_extents;
>   	struct list_head block_groups;
>   };
>
> @@ -141,6 +145,7 @@ u64 calc_stripe_length(u64 type, u64 length, int num_stripes);
>   static inline void block_group_tree_init(struct block_group_tree *tree)
>   {
>   	cache_tree_init(&tree->tree);
> +	extent_io_tree_init(&tree->pending_extents);
>   	INIT_LIST_HEAD(&tree->block_groups);
>   }
>
> diff --git a/check/main.c b/check/main.c
> index 3f6db8f8..af9e0ff3 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -5083,9 +5083,27 @@ static void free_block_group_record(struct cache_extent *cache)
>
>   void free_block_group_tree(struct block_group_tree *tree)
>   {
> +	extent_io_tree_cleanup(&tree->pending_extents);
>   	cache_tree_free_extents(&tree->tree, free_block_group_record);
>   }
>
> +static void update_block_group_used(struct block_group_tree *tree,
> +				    u64 bytenr, u64 num_bytes)
> +{
> +	struct cache_extent *bg_item;
> +	struct block_group_record *bg_rec;
> +
> +	bg_item = lookup_cache_extent(&tree->tree, bytenr, num_bytes);
> +	if (!bg_item) {
> +		set_extent_dirty(&tree->pending_extents, bytenr,
> +				 bytenr + num_bytes - 1);
> +		return;
> +	}

I guess this is to handle cases where the extent item shows up before we
reached the block group item.

So we set the pending_extents range dirty, and waiting for the block
group item to show up.

But I'm not sure if we really need to handle them like this.

Can't we just set the range dirty and call it a day, then check the tree
to calculate the actual used space for each block group item after we
iterated the whole extent tree?

Thanks,
Qu

> +	bg_rec = container_of(bg_item, struct block_group_record,
> +			      cache);
> +	bg_rec->actual_used += num_bytes;
> +}
> +
>   int insert_device_extent_record(struct device_extent_tree *tree,
>   				struct device_extent_record *de_rec)
>   {
> @@ -5270,6 +5288,7 @@ btrfs_new_block_group_record(struct extent_buffer *leaf, struct btrfs_key *key,
>
>   	ptr = btrfs_item_ptr(leaf, slot, struct btrfs_block_group_item);
>   	rec->flags = btrfs_block_group_flags(leaf, ptr);
> +	rec->disk_used = btrfs_block_group_used(leaf, ptr);
>
>   	INIT_LIST_HEAD(&rec->list);
>
> @@ -5281,6 +5300,7 @@ static int process_block_group_item(struct block_group_tree *block_group_cache,
>   				    struct extent_buffer *eb, int slot)
>   {
>   	struct block_group_record *rec;
> +	u64 start, end;
>   	int ret = 0;
>
>   	rec = btrfs_new_block_group_record(eb, key, slot);
> @@ -5289,6 +5309,22 @@ static int process_block_group_item(struct block_group_tree *block_group_cache,
>   		fprintf(stderr, "Block Group[%llu, %llu] existed.\n",
>   			rec->objectid, rec->offset);
>   		free(rec);
> +		return ret;
> +	}
> +
> +	while (!find_first_extent_bit(&block_group_cache->pending_extents,
> +				      rec->objectid, &start, &end,
> +				      EXTENT_DIRTY)) {
> +		u64 len;
> +
> +		if (start >= rec->objectid + rec->offset)
> +			break;
> +		start = max(start, rec->objectid);
> +		len = min(end - start + 1,
> +			  rec->objectid + rec->offset - start);
> +		rec->actual_used += len;
> +		clear_extent_dirty(&block_group_cache->pending_extents, start,
> +				   start + len - 1);
>   	}
>
>   	return ret;
> @@ -5352,6 +5388,7 @@ process_device_extent_item(struct device_extent_tree *dev_extent_cache,
>
>   static int process_extent_item(struct btrfs_root *root,
>   			       struct cache_tree *extent_cache,
> +			       struct block_group_tree *block_group_cache,
>   			       struct extent_buffer *eb, int slot)
>   {
>   	struct btrfs_extent_item *ei;
> @@ -5380,6 +5417,8 @@ static int process_extent_item(struct btrfs_root *root,
>   		num_bytes = key.offset;
>   	}
>
> +	update_block_group_used(block_group_cache, key.objectid, num_bytes);
> +
>   	if (!IS_ALIGNED(key.objectid, gfs_info->sectorsize)) {
>   		error("ignoring invalid extent, bytenr %llu is not aligned to %u",
>   		      key.objectid, gfs_info->sectorsize);
> @@ -6348,13 +6387,13 @@ static int run_next_block(struct btrfs_root *root,
>   				continue;
>   			}
>   			if (key.type == BTRFS_EXTENT_ITEM_KEY) {
> -				process_extent_item(root, extent_cache, buf,
> -						    i);
> +				process_extent_item(root, extent_cache,
> +						    block_group_cache, buf, i);
>   				continue;
>   			}
>   			if (key.type == BTRFS_METADATA_ITEM_KEY) {
> -				process_extent_item(root, extent_cache, buf,
> -						    i);
> +				process_extent_item(root, extent_cache,
> +						    block_group_cache, buf, i);
>   				continue;
>   			}
>   			if (key.type == BTRFS_EXTENT_CSUM_KEY) {
> @@ -8619,6 +8658,41 @@ static int deal_root_from_list(struct list_head *list,
>   	return ret;
>   }
>
> +static int check_block_groups(struct block_group_tree *bg_cache)
> +{
> +	struct btrfs_trans_handle *trans;
> +	struct cache_extent *item;
> +	struct block_group_record *bg_rec;
> +	int ret = 0;
> +
> +	for (item = first_cache_extent(&bg_cache->tree); item;
> +	     item = next_cache_extent(item)) {
> +		bg_rec = container_of(item, struct block_group_record,
> +				      cache);
> +		if (bg_rec->disk_used == bg_rec->actual_used)
> +			continue;
> +		fprintf(stderr,
> +			"block group[%llu %llu] used %llu but extent items used %llu\n",
> +			bg_rec->objectid, bg_rec->offset, bg_rec->disk_used,
> +			bg_rec->actual_used);
> +		ret = -1;
> +	}
> +
> +	if (!repair || !ret)
> +		return ret;
> +
> +	trans = btrfs_start_transaction(gfs_info->extent_root, 1);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		fprintf(stderr, "Failed to start a transaction\n");
> +		return ret;
> +	}
> +
> +	ret = btrfs_fix_block_accounting(trans);
> +	btrfs_commit_transaction(trans, gfs_info->extent_root);
> +	return ret ? ret : -EAGAIN;
> +}
> +
>   /**
>    * parse_tree_roots - Go over all roots in the tree root and add each one to
>    *		      a list.
> @@ -8910,6 +8984,13 @@ again:
>   		goto out;
>   	}
>
> +	ret = check_block_groups(&block_group_cache);
> +	if (ret) {
> +		if (ret == -EAGAIN)
> +			goto loop;
> +		goto out;
> +	}
> +
>   	ret = check_devices(&dev_cache, &dev_extent_cache);
>   	if (ret && err)
>   		ret = err;
>

Re: [PATCH v2 09/12] btrfs-progs: make check detect and fix problems with super_bytes_used

[Qu Wenruo]

On 2021/8/19 上午5:33, Josef Bacik wrote:
> We do not detect problems with our bytes_used counter in the super
> block.  Thankfully the same method to fix block groups is used to re-set
> the value in the super block, so simply add some extra code to validate
> the bytes_used field and then piggy back on the repair code for block
> groups.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   check/main.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/check/main.c b/check/main.c
> index af9e0ff3..b1b1b866 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -8663,12 +8663,14 @@ static int check_block_groups(struct block_group_tree *bg_cache)
>   	struct btrfs_trans_handle *trans;
>   	struct cache_extent *item;
>   	struct block_group_record *bg_rec;
> +	u64 used = 0;
>   	int ret = 0;
>
>   	for (item = first_cache_extent(&bg_cache->tree); item;
>   	     item = next_cache_extent(item)) {
>   		bg_rec = container_of(item, struct block_group_record,
>   				      cache);
> +		used += bg_rec->actual_used;
>   		if (bg_rec->disk_used == bg_rec->actual_used)
>   			continue;
>   		fprintf(stderr,
> @@ -8678,6 +8680,19 @@ static int check_block_groups(struct block_group_tree *bg_cache)
>   		ret = -1;
>   	}
>
> +	/*
> +	 * We check the super bytes_used here because it's the sum of all block
> +	 * groups used, and the repair actually happens in
> +	 * btrfs_fix_block_accounting, so we can kill both birds with the same
> +	 * stone here.
> +	 */
> +	if (used != btrfs_super_bytes_used(gfs_info->super_copy)) {
> +		fprintf(stderr,
> +			"super bytes used %llu mismatches actual used %llu\n",
> +			btrfs_super_bytes_used(gfs_info->super_copy), used);
> +		ret = -1;
> +	}
> +
>   	if (!repair || !ret)
>   		return ret;
>
>

Re: [PATCH v2 10/12] btrfs-progs: check btrfs_super_used in lowmem check

[Qu Wenruo]

On 2021/8/19 上午5:33, Josef Bacik wrote:
> We can already fix this problem with the block accounting code, we just
> need to keep track of how much we should have used on the file system,
> and then check it against the bytes_super.  The repair just piggy backs
> on the block group used repair.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

One unrelated concern inlined below.

> ---
>   check/mode-lowmem.c | 13 ++++++++++++-
>   check/mode-lowmem.h |  1 +
>   2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 14815519..dacc5445 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -28,6 +28,7 @@
>   #include "check/mode-lowmem.h"
>
>   static u64 last_allocated_chunk;
> +static u64 total_used = 0;
>
>   static int calc_extent_flag(struct btrfs_root *root, struct extent_buffer *eb,
>   			    u64 *flags_ret)
> @@ -3654,6 +3655,8 @@ next:
>   out:
>   	btrfs_release_path(&path);
>
> +	total_used += used;
> +
>   	if (total != used) {
>   		error(
>   		"block group[%llu %llu] used %llu but extent items used %llu",
> @@ -5556,6 +5559,14 @@ next:
>   	}
>   out:
>
> +	if (total_used != btrfs_super_bytes_used(gfs_info->super_copy)) {
> +		fprintf(stderr,
> +			"super bytes used %llu mismatches actual used %llu\n",
> +			btrfs_super_bytes_used(gfs_info->super_copy),
> +			total_used);
> +		err |= SUPER_BYTES_USED_ERROR;
> +	}
> +
>   	if (repair) {
>   		ret = end_avoid_extents_overwrite();
>   		if (ret < 0)
> @@ -5568,7 +5579,7 @@ out:
>   		if (ret)
>   			err |= ret;
>   		else
> -			err &= ~BG_ACCOUNTING_ERROR;
> +			err &= ~(BG_ACCOUNTING_ERROR|SUPER_BYTES_USED_ERROR);
>   	}
>
>   	btrfs_release_path(&path);
> diff --git a/check/mode-lowmem.h b/check/mode-lowmem.h
> index da9f8600..0bcc338b 100644
> --- a/check/mode-lowmem.h
> +++ b/check/mode-lowmem.h
> @@ -48,6 +48,7 @@
>   #define DIR_ITEM_HASH_MISMATCH	(1<<24) /* Dir item hash mismatch */
>   #define INODE_MODE_ERROR	(1<<25) /* Bad inode mode */
>   #define INVALID_GENERATION	(1<<26)	/* Generation is too new */
> +#define SUPER_BYTES_USED_ERROR	(1<<27)	/* Super bytes_used is invalid */

We're reaching U32 limit now...

Thanks,
Qu
>
>   /*
>    * Error bit for low memory mode check.
>

Re: [PATCH v2 01/12] btrfs-progs: fix running lowmem check tests

[David Sterba]

On Wed, Aug 18, 2021 at 05:33:13PM -0400, Josef Bacik wrote:
> When I added the invalid super image I saw that the lowmem tests were
> passing, despite not having the detection code yet.  Turns out this is
> because we weren't using a run command helper which does the proper
> expansion and adds the --mode=lowmem option.  Fix this to use the proper
> handler, and now the lowmem test fails properly without my patch to add
> this support to the lowmem mode.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  tests/common | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/common b/tests/common
> index 805a447c..5b255689 100644
> --- a/tests/common
> +++ b/tests/common
> @@ -425,9 +425,8 @@ check_image()
>  
>  	image=$1
>  	echo "testing image $(basename $image)" >> "$RESULTS"
> -	"$TOP/btrfs" check "$image" >> "$RESULTS" 2>&1
> -	[ $? -eq 0 ] && _fail "btrfs check should have detected corruption"
> -
> +	run_mustfail "btrfs check should have detected corruption" \
> +		"$TOP/btrfs" check "$image"

This seems correct but Qu sent a patch that processes the output looking
for some specific error messages so I've applied his version
("btrfs-progs: tests: also check subpage warning for check_image cases")

>  	run_check "$TOP/btrfs" check --repair --force "$image"
>  	run_check "$TOP/btrfs" check "$image"
>  }
> -- 
> 2.26.3

Re: [PATCH v2 07/12] btrfs-progs: add the ability to corrupt fields of the super block

[David Sterba]

On Wed, Aug 18, 2021 at 05:33:19PM -0400, Josef Bacik wrote:
> Doing the extent tree v2 work I generated an invalid super block with
> the wrong bytes_used set, and only noticed because it affected the block
> groups as well.  Neither modes of fsck check for a valid bytes_used, so
> add the ability to corrupt this field so I can generate a testcase for
> fsck.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  btrfs-corrupt-block.c | 66 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
> index 80622f29..7e576897 100644
> --- a/btrfs-corrupt-block.c
> +++ b/btrfs-corrupt-block.c
> @@ -355,6 +355,24 @@ enum btrfs_block_group_field {
>  	BTRFS_BLOCK_GROUP_ITEM_BAD,
>  };
>  
> +enum btrfs_super_field {
> +	BTRFS_SUPER_FIELD_FLAGS,
> +	BTRFS_SUPER_FIELD_TOTAL_BYTES,
> +	BTRFS_SUPER_FIELD_BYTES_USED,
> +	BTRFS_SUPER_FIELD_BAD,
> +};
> +
> +static enum btrfs_super_field convert_super_field(char *field)
> +{
> +	if (!strncmp(field, "flags", FIELD_BUF_LEN))
> +		return BTRFS_SUPER_FIELD_FLAGS;
> +	if (!strncmp(field, "total_bytes", FIELD_BUF_LEN))
> +		return BTRFS_SUPER_FIELD_TOTAL_BYTES;
> +	if (!strncmp(field, "bytes_used", FIELD_BUF_LEN))
> +		return BTRFS_SUPER_FIELD_BYTES_USED;
> +	return BTRFS_SUPER_FIELD_BAD;

There's a more feature-complete utility to modify super block fields
btrfs-sb-mod, this would be duplicated in corrupt-block.

Re: [PATCH v2 02/12] btrfs-progs: do not infinite loop on corrupt keys with lowmem mode

[David Sterba]

On Thu, Aug 19, 2021 at 01:42:39PM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/8/19 上午5:33, Josef Bacik wrote:
> > By enabling the lowmem checks properly I uncovered the case where test
> > 007 will infinite loop at the detection stage.  This is because when
> > checking the inode item we will just btrfs_next_item(), and because we
> > ignore check tree block failures at read time we don't get an -EIO from
> > btrfs_next_leaf.  Generally what check usually does is validate the
> > leaves/nodes as we hit them, but in this case we're not doing that.  Fix
> > this by checking the leaf if we move to the next one and if it fails
> > bail.  This allows us to pass the 007 test with lowmem.
> 
> Doesn't this mean btrfs_next_item() is not doing what it should do?
> 
> Normally we would expect btrfs_next_item() to return -EIO other than
> manually checking the returned leaf.

That's an interesting point, I think we rely on that behaviour
elsewhere too.

Re: [PATCH v2 00/12] btrfs-progs: make check handle invalid bg items

[David Sterba]

On Wed, Aug 18, 2021 at 05:33:12PM -0400, Josef Bacik wrote:
> v1->v2:
> - Discovered that we also don't check bytes_super in the superblock, add that
>   checking and repair ability since it's coupled with the block group used
>   repair.
> - Discovered that we haven't actually been setting --mode=lowmem for the initial
>   image check if we do make test-check-lowmem, we only do it after the repair.
>   Fixed this.
> - Now that we're properly testing error detection in all of the test cases, I
>   found 3 problems with the --mode=lowmem mode, one infinite loop and two places
>   we weren't properly propagating the error code up to the user.
> - My super repair thing tripped a case where we wouldn't clean up properly for
>   unaligned extent records, fixed this as well.
> - Add another test image for the corrupted super bytes.
> - Realize that you need a special .lowmem_repairable file in order for the
>   lowmem repair code to run against images, so did that for both testcases.
> 
> --- Original email ---
> Hello,
> 
> While writing code for extent tree v2 I noticed that I was generating a fs with
> an invalid block group ->used value.  However fsck wasn't catching this, because
> we don't actuall check the used value of the block group items in normal mode.
> lowmem mode does this properly thankfully, so this only needs to be added to the
> normal fsck mode.
> 
> I've added code to btrfs-corrupt-block to generate the corrupt image I need for
> the test case.  Then of course the actual patch to detect and fix the problem.
> Thanks,
> 
> Josef
> 
> Josef Bacik (12):
>   btrfs-progs: fix running lowmem check tests
>   btrfs-progs: do not infinite loop on corrupt keys with lowmem mode
>   btrfs-progs: propagate fs root errors in lowmem mode
>   btrfs-progs: propagate extent item errors in lowmem mode
>   btrfs-progs: do not double add unaligned extent records
>   btrfs-progs: add the ability to corrupt block group items
>   btrfs-progs: add the ability to corrupt fields of the super block
>   btrfs-progs: make check detect and fix invalid used for block groups
>   btrfs-progs: make check detect and fix problems with super_bytes_used
>   btrfs-progs: check btrfs_super_used in lowmem check
>   btrfs-progs: add a test image with a corrupt block group item
>   btrfs-progs: add a test image with an invalid super bytes_used

There are some comments or question so I haven't merged the patches yet
(not merged: 2, 5, 8, 9, 11, 12). Please have a look, rebase on top of
devel should cleanly drop the merged patches from the series.

Re: [PATCH v2 02/12] btrfs-progs: do not infinite loop on corrupt keys with lowmem mode

[Josef Bacik]

On 8/23/21 11:04 AM, David Sterba wrote:
> On Thu, Aug 19, 2021 at 01:42:39PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2021/8/19 上午5:33, Josef Bacik wrote:
>>> By enabling the lowmem checks properly I uncovered the case where test
>>> 007 will infinite loop at the detection stage.  This is because when
>>> checking the inode item we will just btrfs_next_item(), and because we
>>> ignore check tree block failures at read time we don't get an -EIO from
>>> btrfs_next_leaf.  Generally what check usually does is validate the
>>> leaves/nodes as we hit them, but in this case we're not doing that.  Fix
>>> this by checking the leaf if we move to the next one and if it fails
>>> bail.  This allows us to pass the 007 test with lowmem.
>>
>> Doesn't this mean btrfs_next_item() is not doing what it should do?
>>
>> Normally we would expect btrfs_next_item() to return -EIO other than
>> manually checking the returned leaf.
> 
> That's an interesting point, I think we rely on that behaviour
> elsewhere too.
> 

This is the result of how we deal with corrupt blocks.  We will happily 
read corrupt blocks with check, because we expect check to do it's own 
btrfs_check_node/btrfs_check_leaf().  The problem here is that if the 
block is corrupt it's still in cache, and so btrfs_next_leaf() will 
return it because the buffer is marked uptodate.

It looks like search does the extra check_block() specifically to catch 
this case, so I'll fix next_leaf to do the same thing as well.  Thanks,

Josef

Re: [PATCH v2 02/12] btrfs-progs: do not infinite loop on corrupt keys with lowmem mode

[Qu Wenruo]

On 2021/8/24 上午2:44, Josef Bacik wrote:
> On 8/23/21 11:04 AM, David Sterba wrote:
>> On Thu, Aug 19, 2021 at 01:42:39PM +0800, Qu Wenruo wrote:
>>>
>>>
>>> On 2021/8/19 上午5:33, Josef Bacik wrote:
>>>> By enabling the lowmem checks properly I uncovered the case where test
>>>> 007 will infinite loop at the detection stage.  This is because when
>>>> checking the inode item we will just btrfs_next_item(), and because we
>>>> ignore check tree block failures at read time we don't get an -EIO from
>>>> btrfs_next_leaf.  Generally what check usually does is validate the
>>>> leaves/nodes as we hit them, but in this case we're not doing that.
>>>> Fix
>>>> this by checking the leaf if we move to the next one and if it fails
>>>> bail.  This allows us to pass the 007 test with lowmem.
>>>
>>> Doesn't this mean btrfs_next_item() is not doing what it should do?
>>>
>>> Normally we would expect btrfs_next_item() to return -EIO other than
>>> manually checking the returned leaf.
>>
>> That's an interesting point, I think we rely on that behaviour
>> elsewhere too.
>>
>
> This is the result of how we deal with corrupt blocks.  We will happily
> read corrupt blocks with check, because we expect check to do it's own
> btrfs_check_node/btrfs_check_leaf().  The problem here is that if the
> block is corrupt it's still in cache, and so btrfs_next_leaf() will
> return it because the buffer is marked uptodate.

Shouldn't we prevent the corrupted block from entering the cache?

>
> It looks like search does the extra check_block() specifically to catch
> this case, so I'll fix next_leaf to do the same thing as well.  Thanks,

OK for now I think it's fine to have the extra check.

It won't cause any harm even if we solved the cache problem in the future.

Thanks,
Qu

>
> Josef