[PATCH v3 0/9] btrfs-progs: make check handle invalid bg items

[PATCH v3 0/9] btrfs-progs: make check handle invalid bg items

[Josef Bacik]

v2->v3:
- Rebased onto devel.
- Fixed the lowmem checks to work with Qu's test for subpage.
- Adjust the titles to indicate the area where the patch is for.
- Make btrfs_next_leaf() return an error instead of doing check_block in lowmem.
- Drop the corrupt-block patche for super blocks.

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 (9):
  btrfs-progs: tests: fix running lowmem checks
  btrfs-progs: check blocks in btrfs_next_sibling_block
  btrfs-progs: check: propagate fs root errors in lowmem mode
  btrfs-progs: check: propagate extent item errors in lowmem mode
  btrfs-progs: do not double add unaligned extent records
  btrfs-progs: check: detect and fix problems with super_bytes_used
  btrfs-progs: check: detect issues with btrfs_super_used in lowmem
    check
  btrfs-progs: test: add a test image with a corrupt block group item
  btrfs-progs: tests: add a test image with an invalid super bytes_used

 check/main.c                                  |  35 ++++++++++++++++++
 check/mode-lowmem.c                           |  16 +++++++-
 check/mode-lowmem.h                           |   1 +
 kernel-shared/ctree.c                         |   7 ++++
 tests/common                                  |   6 +--
 .../.lowmem_repairable                        |   0
 .../.lowmem_repairable                        |   0
 .../default.img.xz                            | Bin 0 -> 1060 bytes
 8 files changed, 59 insertions(+), 6 deletions(-)
 create mode 100644 tests/fsck-tests/050-invalid-block-group-used/.lowmem_repairable
 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 v3 1/9] btrfs-progs: tests: fix running lowmem checks

[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 | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/common b/tests/common
index 94ebf7a6..bcab11f3 100644
--- a/tests/common
+++ b/tests/common
@@ -428,10 +428,8 @@ check_image()
 
 	image=$1
 	echo "testing image $(basename $image)" >> "$RESULTS"
-	"$TOP/btrfs" check "$image" &> "$tmp_output"
-	ret=$?
-	cat "$tmp_output" >> "$RESULTS"
-	[ "$ret" -eq 0 ] && _fail "btrfs check should have detected corruption"
+	run_mustfail_stdout "btrfs check should have detected corruption" \
+		"$TOP/btrfs" check "$image" &> "$tmp_output"
 
 	# Also make sure no subpage related warnings
 	check_test_results "$tmp_output" "$testname"
-- 
2.26.3

[PATCH v3 2/9] btrfs-progs: check blocks in btrfs_next_sibling_block

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

This occurs because we allow fsck to raw-read blocks even if they fail
basic sanity checks, because we want the opportunity to repair the
blocks.  However this means corrupt blocks are sitting in cache marked
as uptodate.  btrfs_search_slot() handles this by doing a check_block()
on every block we add to the path, so that anything that is doing a
search gets a proper -EIO.

btrfs_next_sibling_block() needs a similar check.  With this fix we now
return -EIO on btrfs_next_leaf() properly and we no longer infinite loop
on 007 with lowmem.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 kernel-shared/ctree.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel-shared/ctree.c b/kernel-shared/ctree.c
index 518718de..0845cc60 100644
--- a/kernel-shared/ctree.c
+++ b/kernel-shared/ctree.c
@@ -3125,6 +3125,13 @@ int btrfs_next_sibling_tree_block(struct btrfs_fs_info *fs_info,
 		free_extent_buffer(c);
 		path->nodes[level] = next;
 		path->slots[level] = 0;
+		/*
+		 * Fsck will happily load corrupt blocks in order to fix them,
+		 * so we need an extra check just to make sure this block isn't
+		 * marked uptodate but invalid.
+		 */
+		if (check_block(fs_info, path, level))
+			return -EIO;
 		if (level == path->lowest_level)
 			break;
 		if (path->reada)
-- 
2.26.3

[PATCH v3 3/9] btrfs-progs: check: 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.

Reviewed-by: Qu Wenruo <wqu@suse.com>
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 888894e8..507873ce 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -5195,7 +5195,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 v3 4/9] btrfs-progs: check: 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.

Reviewed-by: Qu Wenruo <wqu@suse.com>
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 507873ce..cb8e3ab8 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -4381,6 +4381,7 @@ next:
 		goto next;
 	}
 
+	err |= tmp_err;
 	ptr_offset += btrfs_extent_inline_ref_size(type);
 	goto next;
 
-- 
2.26.3

[PATCH v3 5/9] 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 81b6650f..6f77b5ff 100644
--- a/check/main.c
+++ b/check/main.c
@@ -7928,6 +7928,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;
 
@@ -7943,6 +7945,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 v3 6/9] btrfs-progs: 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.

Reviewed-by: Qu Wenruo <wqu@suse.com>
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 6f77b5ff..19d1c153 100644
--- a/check/main.c
+++ b/check/main.c
@@ -8660,12 +8660,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,
@@ -8675,6 +8677,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 v3 7/9] btrfs-progs: check: detect issues with 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.

Reviewed-by: Qu Wenruo <wqu@suse.com>
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 cb8e3ab8..a1ad9606 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)
@@ -3645,6 +3646,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",
@@ -5547,6 +5550,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)
@@ -5559,7 +5570,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 v3 8/9] btrfs-progs: test: 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>
---
 tests/fsck-tests/050-invalid-block-group-used/.lowmem_repairable | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/fsck-tests/050-invalid-block-group-used/.lowmem_repairable

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
-- 
2.26.3

[PATCH v3 9/9] btrfs-progs: tests: 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 v3 2/9] btrfs-progs: check blocks in btrfs_next_sibling_block

[David Sterba]

On Mon, Aug 23, 2021 at 03:23:06PM -0400, 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

BTW, short test references should be eg. fsck/007, here' it's clear from
the context that it's for check/fsck.

Re: [PATCH v3 8/9] btrfs-progs: test: add a test image with a corrupt block group item

[David Sterba]

On Mon, Aug 23, 2021 at 03:23:12PM -0400, Josef Bacik wrote:
> This image has a broken used field of a block group item to validate
> fsck does the correct thing.

Should there be a new image or are you just adding the
.lowmem_repairable file to trigger lowmem repair?

Re: [PATCH v3 8/9] btrfs-progs: test: add a test image with a corrupt block group item

[David Sterba]

On Tue, Aug 24, 2021 at 07:00:20PM +0200, David Sterba wrote:
> On Mon, Aug 23, 2021 at 03:23:12PM -0400, Josef Bacik wrote:
> > This image has a broken used field of a block group item to validate
> > fsck does the correct thing.
> 
> Should there be a new image or are you just adding the
> .lowmem_repairable file to trigger lowmem repair?

Merged with patch from v2 that contained the image.