[PATCH 0/5] btrfs-progs: check: original and lowmem mode fix

[PATCH 0/5] btrfs-progs: check: original and lowmem mode fix

[Su Yue]

The head three patches is independent from later two.
Include:
1) Let check_extent_refs() return error instead of 0 if something wrong
   is found in original mode.
2) repair_root_items() should be called before any repair. Put it
   before do_check_chunks_and_extents();
3) Error status should not be modified if repair_root_items() succeeded
   in original repair.

The late two patches are about invalid type in extent_inline_ref.

Only original mode applied 001-003 patches can handle the case in the
last patch. So I gather those 5 patches together.


Su Yue (5):
  btrfs-progs: check: return value of check_extent_refs()
  btrfs-progs: check: call repair_root_items() before any repair
  btrfs-progs: check: error or return value of repair_root_items()
  btrfs-progs: check: check extent_inline_ref in lowmem
  btrfs-progs: fsck-tests: 027/bad_extent_inline_ref_type

 cmds-check.c                                       |  83 +++++++++++++++------
 .../bad-extent-inline-ref-type.raw.xz              | Bin 0 -> 17144 bytes
 2 files changed, 62 insertions(+), 21 deletions(-)
 create mode 100644 tests/fsck-tests/027-bad-extent-inline-ref-type/bad-extent-inline-ref-type.raw.xz

-- 
2.14.1

[PATCH 1/5] btrfs-progs: check: return value of check_extent_refs()

[Su Yue]

In original check mode(without option "--repair"), check_extent_refs()
always returns 0.

Add a variable @error to record status while checking extents.
At the end of check_extent_refs(), let it return -EIO if @error is
nonzero.

Example:
$ btrfs check bad-extent-inline-ref-type.raw
Checking filesystem on bad-extent-inline-ref-type.raw
UUID: 1942d6fe-617b-4499-9982-cc8ffae5447f
checking extents
corrupt extent record: key 29360128 169 16384
ref mismatch on [29360128 16384] extent item 0, found 1
Backref 29360128 parent 5 root 5 not found in extent tree
backpointer mismatch on [29360128 16384]
bad extent [29360128, 29376512), type mismatch with chunk
checking free space cache
checking fs roots
checking csums
checking root refs
found 114688 bytes used, no error found
total csum bytes: 0
total tree bytes: 114688
total fs tree bytes: 32768
total extent tree bytes: 16384
btree space waste bytes: 109471
file data blocks allocated: 0
 referenced 0

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 cmds-check.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/cmds-check.c b/cmds-check.c
index 8aa136df..93b47194 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -10664,6 +10664,7 @@ static int check_extent_refs(struct btrfs_root *root,
 	struct cache_extent *cache;
 	int ret = 0;
 	int had_dups = 0;
+	int error = 0;
 
 	if (repair) {
 		/*
@@ -10807,6 +10808,7 @@ static int check_extent_refs(struct btrfs_root *root,
 			cur_err = 1;
 		}
 
+		error = cur_err;
 		remove_cache_extent(extent_cache, cache);
 		free_all_extent_backrefs(rec);
 		if (!init_extent_tree && repair && (!cur_err || fix))
@@ -10839,7 +10841,10 @@ repair_abort:
 		}
 		return ret;
 	}
-	return 0;
+
+	if (error)
+		error = -EIO;
+	return error;
 }
 
 u64 calc_stripe_length(u64 type, u64 length, int num_stripes)
-- 
2.14.1

[PATCH 2/5] btrfs-progs: check: call repair_root_items() before any repair

[Su Yue]

The Annotation of repair_root_items():
"This must be run before any other repair code - not doing it so,
makes other repair code delete or modify backrefs in the extent tree
for example, which will result in an inconsistent fs after repairing
the root items."

However, the rule was broken by commit
"6896ab801d47419fa06d886bae4bf31d0287ced1" which intends to fix
failure of test-fsck/013.
The correct way is to skip calling repair_root_item() when
init_extent_tree is nonzero.

Now put repair_root_items() before do_check_chunks_and_extents() and
not call repair_root_items() if do init_extent_tree.
Then test-fsck/013 works well.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 cmds-check.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 93b47194..3e2f9faa 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -14644,32 +14644,35 @@ int cmd_check(int argc, char **argv)
 		goto close_out;
 	}
 
+	if (!init_extent_tree) {
+		ret = repair_root_items(info);
+		err |= !!ret;
+		if (ret < 0) {
+			error("failed to repair root items: %s",
+			      strerror(-ret));
+			goto close_out;
+		}
+		if (repair) {
+			fprintf(stderr, "Fixed %d roots.\n", ret);
+			ret = 0;
+		} else if (ret > 0) {
+			fprintf(stderr,
+				"Found %d roots with an outdated root item.\n",
+				ret);
+			fprintf(stderr,
+				"Please run a filesystem check with the option --repair to fix them.\n");
+			ret = 1;
+			err |= !!ret;
+			goto close_out;
+		}
+	}
+
 	ret = do_check_chunks_and_extents(info);
 	err |= !!ret;
 	if (ret)
 		error(
 		"errors found in extent allocation tree or chunk allocation");
 
-	ret = repair_root_items(info);
-	err |= !!ret;
-	if (ret < 0) {
-		error("failed to repair root items: %s", strerror(-ret));
-		goto close_out;
-	}
-	if (repair) {
-		fprintf(stderr, "Fixed %d roots.\n", ret);
-		ret = 0;
-	} else if (ret > 0) {
-		fprintf(stderr,
-		       "Found %d roots with an outdated root item.\n",
-		       ret);
-		fprintf(stderr,
-			"Please run a filesystem check with the option --repair to fix them.\n");
-		ret = 1;
-		err |= !!ret;
-		goto close_out;
-	}
-
 	if (!ctx.progress_enabled) {
 		if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
 			fprintf(stderr, "checking free space tree\n");
-- 
2.14.1

[PATCH 3/5] btrfs-progs: check: error or return value of repair_root_items()

[Su Yue]

Return value of repair_root_items():
<0 on error
=0 does nothing
>0 if repair is enable, N roots is repaired;
   else N roots is corrupted.

In the repair mode, there should be no error if return value is bigger
than 0.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 cmds-check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds-check.c b/cmds-check.c
index 3e2f9faa..adc4a934 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -14646,8 +14646,8 @@ int cmd_check(int argc, char **argv)
 
 	if (!init_extent_tree) {
 		ret = repair_root_items(info);
-		err |= !!ret;
 		if (ret < 0) {
+			err = !!ret;
 			error("failed to repair root items: %s",
 			      strerror(-ret));
 			goto close_out;
-- 
2.14.1

[PATCH 4/5] btrfs-progs: check: check extent_inline_ref in lowmem

[Su Yue]

Lowmem check does not skip invalid type in extent_inline_ref then
calls btrfs_extent_inline_ref_size(type) which causes crash.

Error:
$ btrfs check --mode=lowmem  /tmp/data_small
Checking filesystem on /tmp/data_small
UUID: ee205d69-8724-4aa2-a4f5-bc8558a62169
checking extents
ERROR: extent[20971520 16384] backref type mismatch, missing bit: 2
ERROR: extent[20971520 16384] backref generation mismatch,
wanted: 7, have: 0
ERROR: extent[20971520 16384] is referred by other roots than 3
ctree.h:1754: btrfs_extent_inline_ref_size: BUG_ON `1` triggered,
value 1
btrfs(+0x543db)[0x55fabc2ab3db]
btrfs(+0x587f7)[0x55fabc2af7f7]
btrfs(+0x5fa44)[0x55fabc2b6a44]
btrfs(cmd_check+0x194a)[0x55fabc2bd717]
btrfs(main+0x88)[0x55fabc2682e0]
/usr/lib/libc.so.6(__libc_start_main+0xea)[0x7f021c3824ca]
btrfs(_start+0x2a)[0x55fabc267e7a]
[1] 5188 abort (core dumped)  btrfs check --mode=lowmem /tmp/data_small

Solve it by introducing check_extent_inline_ref() to check type.
If the checker returns a nonzero value, we should not check the
corrupted extent item anymore.

Suggested-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 cmds-check.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/cmds-check.c b/cmds-check.c
index adc4a934..aa054a19 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -11415,6 +11415,29 @@ loop:
 	goto again;
 }
 
+static int check_extent_inline_ref(struct extent_buffer *eb,
+		   struct btrfs_key *key, struct btrfs_extent_inline_ref *iref)
+{
+	int ret;
+	u8 type = btrfs_extent_inline_ref_type(eb, iref);
+
+	switch (type) {
+	case BTRFS_TREE_BLOCK_REF_KEY:
+	case BTRFS_EXTENT_DATA_REF_KEY:
+	case BTRFS_SHARED_BLOCK_REF_KEY:
+	case BTRFS_SHARED_DATA_REF_KEY:
+		ret = 0;
+		break;
+	default:
+		error("extent[%llu %u %llu] has unknown ref type: %d",
+		      key->objectid, key->type, key->offset, type);
+		ret = UNKNOWN_TYPE;
+		break;
+	}
+
+	return ret;
+}
+
 /*
  * Check backrefs of a tree block given by @bytenr or @eb.
  *
@@ -11549,6 +11572,11 @@ static int check_tree_block_ref(struct btrfs_root *root,
 		type = btrfs_extent_inline_ref_type(leaf, iref);
 		offset = btrfs_extent_inline_ref_offset(leaf, iref);
 
+		ret = check_extent_inline_ref(leaf, &key, iref);
+		if (ret) {
+			err |= ret;
+			break;
+		}
 		if (type == BTRFS_TREE_BLOCK_REF_KEY) {
 			if (offset == root->objectid)
 				found_ref = 1;
@@ -11826,6 +11854,11 @@ static int check_extent_data_item(struct btrfs_root *root,
 		type = btrfs_extent_inline_ref_type(leaf, iref);
 		dref = (struct btrfs_extent_data_ref *)(&iref->offset);
 
+		ret = check_extent_inline_ref(leaf, &dbref_key, iref);
+		if (ret) {
+			err |= ret;
+			break;
+		}
 		if (type == BTRFS_EXTENT_DATA_REF_KEY) {
 			ref_root = btrfs_extent_data_ref_root(leaf, dref);
 			if (ref_root == root->objectid)
-- 
2.14.1

[PATCH 5/5] btrfs-progs: fsck-tests: 027/bad_extent_inline_ref_type

[Su Yue]

This case is for avoiding crash in lowmem check mode.
Field type of extent_inline_ref in an extent is corrupted.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 .../bad-extent-inline-ref-type.raw.xz                   | Bin 0 -> 17144 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/fsck-tests/027-bad-extent-inline-ref-type/bad-extent-inline-ref-type.raw.xz

diff --git a/tests/fsck-tests/027-bad-extent-inline-ref-type/bad-extent-inline-ref-type.raw.xz b/tests/fsck-tests/027-bad-extent-inline-ref-type/bad-extent-inline-ref-type.raw.xz
new file mode 100644
index 0000000000000000000000000000000000000000..09d820196b11a88557b4cb92b5b3533e80eb5d14
GIT binary patch
literal 17144
zcmeI4XHb*r7RM8$7l9xGOAEv(0Rd4&qzY(|q7Z_#pwa~u0trZw7O4RQsTV~7DJmdU
zK$>&~O(2L$k&aYpE@go=A<Mlxb7yDw&aUj$o!y!DTP9zgne+b7@0|Dlob%F-`B_+j
zK#Ws8<wypQ1Xu(F0`X8R$aFe^(fK?Ggr(8x*-z=Z8mIYj$8Awn2~tzp;Vher6Jj^s
z$Uj$60~?eEzKn`GBcILHAN1fu42%~&YAN{KD{9<(qcSOo^K#>`r=Ej#de0ut2}XJ-
z`*Lkj47tM~I~gIlfavCeyjo5A&Me9fe-{2xqU-FB`+}|aYc?tSUUK4I+*--pn!_!0
z(d6_>zh7c+RV5n38}0T9h4!>K6Yow(aRx;mZkK;xgq&8c>AVzlf&H=K{ICR9C8$o<
zq^%PvfVW|jNFs4$**?$WXpNf2;4}&7XkE#%voO64ym9o(zEXxbUxK4TZe~MWYpv4t
zCiuOZ)((X(RpFzrPlctVV%=PpvhDUsJ2_3n&^fsih_MHy+e*V>hHMtDmNJpb?q;8n
z@%qt0ND;l&neL&Il_I#cT7nPz=N{7$tGP6tlX9!(ohUKs@;S$r-o0X414EqB^9@tp
z9f%7-H4#MvA1yK=j5iUO@tiB12&&}A*wf^CpKBWQw4;tmGNF=3*W&u4$Ge@mo=&pa
zz;R3VFY=F3nK)odEmhdg_oeBnGVHjQ`cg`}PnO^eC%bn!58jRWT&Hnwzk#i>;BrI%
z!pH+N<9=L8om37vuLdk7KWZc|mTw})B}=%T2|2YtXk0Xz5*<iZ5{9x4&42jBAu{%a
zUI6Q<LYK2zG)J~;?T#HYEEFUSVIjie6smY)>#@CCTm(zeSnv00?LC2-D)S731G@F>
zYooTxODz&ur#+Jo2?$?@Xxa&#@}lY$)?Cd9vVgQh&5I0lQB^j<qWxm{SuM%YN*kX1
zANWblB*ogxhf4!JkRGfsuq&UrqLEleeUZPycNtG_D!93)KZISJmm-C|PEL&ql#X9M
z;I%H)$eem6mDX4M?iP0|9KEp?ucW|~Te664azCTX`8u9AztBdsrnB^bptWWAtdogF
z{p_{JmJ^d0Cf3ci_l4BYr~<IurS*mkkD6yE0$iudU2XjLqOfoA&QmC)iLdV4cVrSy
zu81xEF|U0gzQFzZL7Gall|j~#F%3e9`cVqo`L|^UIavqoxm?i4Z59sj*^87pSE(|B
zM|?$exXM046qM%oa)<8b=i_EQSFm($;b|wOUMG-qSawUuwn&_jr{!b0VxWqXk|g`w
zEH|BZp~Beq+O+>uA%uPWmW`5vi(#A?=1ID?<r<wNQ$m94>RV36vO)#j-e}Py=d;5u
zigqJ*ZCb6fnB+~L+2FX@Rd+I8$~QoqIBap572f}<cWA=q`KMFoT8nSp(Ymre8`duA
z{0KH#xwJV2yPo5NJ>fWs1Hmi#xjR3|sJz!dhvJ*fMvot4Q7!b!$a*E<Ap=oL`7r)U
zZus!Gd<Z5oo*iNssP^U%T73iIz@}WbB%DF#0n32q@#u%NZ!CddpRO8AlD;}pq57%y
zncT&EZVdw`?OS}i={$)W9zK>E93AvzDI|9u)qnW&okMbFjd5WsA%sT9Kc&8}bu@}w
zvn+!i%y1v1QCOS8s@`rZ621@eTvL;}cCe`FV}RY@O36&ek74uq&$2X5#ns`}258Rc
zp5nf8D)!WFP^Wr}g?f%^2TI68ep=aF4VreqrX^Th;ktd>%jA@x)xMhBOq8Tj{kG*w
z`1@doJ><Z6vaMT9$VF-B%mY^pHC0#&Jcua}T<FOa$Xy;h+Dh=c1BXgC2;B}X=}V%p
zJAc4hnI~UOQ&402UZ#hM#06ci3xuVu$oJ}`S8%SIpQ1symh;;R%qfcZ)=qYreA)u9
zkXD?LD%qIjGL{FLp&i<$vga4@l=kp<1N5v0<O_dF-nuNCWFld>vbn-%1Jof5i9DC8
zfx1sVA#iYp#P4Br#qm=#k9o@JjB5k*t(!5XR&Eee%!Lc$VNUJS_+5iEF^cJFP^fb$
zmIuqo!1}MlYCKVCH_AN?wQ+Nz6)uS57~F2Ej5|Rt$FBdH?S=tMTX{7mST3PKenfv!
zGg^xY<BcB-@fB&!cX6UdGa_{#U!Hx9v$1&#G1UpRk2|`j8Bqi=bpPu>_O+z+Cm8z0
zN6(lA19`m@r2L%M00_UB!9c)vShRqE0Rj8po$(h_{SVqAKw*Hw{tAVagR?;OLFDto
zr()X{KWO;(86BI^w?{-CYcmz~qf*>F&0<QbJj(B?=bTpN>4$r#M_0Zcj>7RK&BqfZ
zr5emCtGm4RBH#FgAdW8PS)Ld$B(+qjE>4!Yd6kZ)ynAv***HcH4R+L$g%T%<XGxU}
z1;ioiFxhVErI+O963#@Q6AFhEcuCV%MY57y>X&+9!T0{(m(#D6g5R>I`-ANN<<aM9
zBE7(C-45O;0dxZB{43D80zM3SQ&k@t!cdN_PiQ0}J@u?xUCR=tGut~^7#A~!bG1p4
zn%1DWuCvAarII|v)#LV!eG@zGpF+H)SN=W#$^ex8)=>Uh3I^E!r`Z3DVG}eW*r#??
z&e{&LJU`(_jMAdq6=%jXxj4M`oXKD%_B-Wh@uSgUoE&}l*fVjZeNZph_Zr2W_n4t~
z-sn8<SlQR5<yu_gX0z+Kc%t~maO=FR!$W31deh6`zBH>_-l6JUN2axwcn6y0Z-qZf
zs+1?cL+ezVBHxcYGE{|L@FQ5DWOZxja<fhOu|JA-r(jw)8vNNC9Ly8eQ1}68F7#@)
z$vcL%L9z#qOQ2KusjpVRn(HCGJN$U;y-$e-#nspAL3Ihi@F3d5qA_TyAQv-MD%ZM0
z<B^{T6#D9+s!v0E#&arZ^}BHq>(6DgZ~f6F0r9DYFZ_-$J-!*%pmYC+{Az}kJ&~7S
zV$xtaY@eH<pSdqFF+?i#36swaDn&NWijuT?GAG3Y@dHfdFw4SBg^jI+?`;oTPvfyQ
zmzXDq3KY!EAJoDWUIz(wU)e+&ooTU>F6RM}H$uF_uhK1hhvWky8dtM0H>IoFALZu1
z%-j+uD%uH^cF_=b6Ur2F+M3^vrh23WxgEjVJngJc5_lV)sCrcPc5bG8E8&DnC+(K0
zFcT)Az@3H7-nsvVNAD0`x39#ZJ-{;Ie)q=eloA_PoS=eCucCS2mD&i2bBm+bBlV0c
zn~&)wAi(<K5jf-77n5F86dw#qJDm*Fr~h12h(=T>0u>3UNI*sUo88I*?mgh%|9a{G
zXcBigrU3*D2-ps4`!8L`FB}8{6b2{^P}sNY9)XnHp`QT+?4K4eC$I)+&c!rFe3NO8
zBhA4g!fXmsVy%{DP&#1b(<CFZo)YBZXNYseJu?{@Yv1yfsCaZF&Mq{-3T2l7IPP|G
zM*9H+V+SSZXNNd%`|f5gy`Pemb9MCz3(_6eDCo%vi0Z$y#J`>9J^m1m2ZXj`p#eSy
z;9~$j#@}k-A4o|cC4rO#QgVA`>bGsqKV)eMU>LwKfMEc`0EYeXvhvruqxc(Giw8mT
zo=rl`@wG{S6}G*G{yR@r7OXw>Z@XeUV!$%=@9klSgVG<w5Q~$uCQ!ubVCg&%GictK
t|4794_bYVAh_9ku=e$8+N*FjeIQa1Pe{tTEMSBEP5AE9i4Pvyk`Uwf&p78(x

literal 0
HcmV?d00001

-- 
2.14.1

Re: [PATCH 2/5] btrfs-progs: check: call repair_root_items() before any repair

[Qu Wenruo]

On 2017年09月27日 14:34, Su Yue wrote:
> The Annotation of repair_root_items():
> "This must be run before any other repair code - not doing it so,
> makes other repair code delete or modify backrefs in the extent tree
> for example, which will result in an inconsistent fs after repairing
> the root items."
> 
> However, the rule was broken by commit
> "6896ab801d47419fa06d886bae4bf31d0287ced1" which intends to fix

Wrong commit.

commit 6896ab801d47419fa06d886bae4bf31d0287ced1
Author: Qu Wenruo <quwenruo.btrfs@gmx.com>
Date:   Mon Sep 11 15:36:12 2017 +0900

     btrfs-progs: docs: mkfs: Add extra condition for rootdir option

This commit has no way affect btrfs check.

Please use first 12 characters and one line summary to describe the commit.
And of course, use correct commit.

Thanks,
Qu


> failure of test-fsck/013.
> The correct way is to skip calling repair_root_item() when
> init_extent_tree is nonzero.
> 
> Now put repair_root_items() before do_check_chunks_and_extents() and
> not call repair_root_items() if do init_extent_tree.
> Then test-fsck/013 works well.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>   cmds-check.c | 43 +++++++++++++++++++++++--------------------
>   1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/cmds-check.c b/cmds-check.c
> index 93b47194..3e2f9faa 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -14644,32 +14644,35 @@ int cmd_check(int argc, char **argv)
>   		goto close_out;
>   	}
>   
> +	if (!init_extent_tree) {
> +		ret = repair_root_items(info);
> +		err |= !!ret;
> +		if (ret < 0) {
> +			error("failed to repair root items: %s",
> +			      strerror(-ret));
> +			goto close_out;
> +		}
> +		if (repair) {
> +			fprintf(stderr, "Fixed %d roots.\n", ret);
> +			ret = 0;
> +		} else if (ret > 0) {
> +			fprintf(stderr,
> +				"Found %d roots with an outdated root item.\n",
> +				ret);
> +			fprintf(stderr,
> +				"Please run a filesystem check with the option --repair to fix them.\n");
> +			ret = 1;
> +			err |= !!ret;
> +			goto close_out;
> +		}
> +	}
> +
>   	ret = do_check_chunks_and_extents(info);
>   	err |= !!ret;
>   	if (ret)
>   		error(
>   		"errors found in extent allocation tree or chunk allocation");
>   
> -	ret = repair_root_items(info);
> -	err |= !!ret;
> -	if (ret < 0) {
> -		error("failed to repair root items: %s", strerror(-ret));
> -		goto close_out;
> -	}
> -	if (repair) {
> -		fprintf(stderr, "Fixed %d roots.\n", ret);
> -		ret = 0;
> -	} else if (ret > 0) {
> -		fprintf(stderr,
> -		       "Found %d roots with an outdated root item.\n",
> -		       ret);
> -		fprintf(stderr,
> -			"Please run a filesystem check with the option --repair to fix them.\n");
> -		ret = 1;
> -		err |= !!ret;
> -		goto close_out;
> -	}
> -
>   	if (!ctx.progress_enabled) {
>   		if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
>   			fprintf(stderr, "checking free space tree\n");
> 

[PATCH v2 2/5] btrfs-progs: check: call repair_root_items() before any repair

[Su Yue]

The Annotation of repair_root_items():
"This must be run before any other repair code - not doing it so,
makes other repair code delete or modify backrefs in the extent tree
for example, which will result in an inconsistent fs after repairing
the root items."

However, the rule was broken by commit 1f728b1a514f.
The commit intends to fix failure of test-fsck/013 so it moves
repair_root_items() after check_extents_and_chunks().

The correct way is to skip calling repair_root_item() when
init_extent_tree is nonzero.
Now put repair_root_items() before do_check_chunks_and_extents() and
not call repair_root_items() if do init_extent_tree.
Then test-fsck/013 works well.

---
Changelog:
v2:
   Correct commit number reported. (Suggested by Qu Wenruo)

   Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 cmds-check.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 93b47194..3e2f9faa 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -14644,32 +14644,35 @@ int cmd_check(int argc, char **argv)
 		goto close_out;
 	}
 
+	if (!init_extent_tree) {
+		ret = repair_root_items(info);
+		err |= !!ret;
+		if (ret < 0) {
+			error("failed to repair root items: %s",
+			      strerror(-ret));
+			goto close_out;
+		}
+		if (repair) {
+			fprintf(stderr, "Fixed %d roots.\n", ret);
+			ret = 0;
+		} else if (ret > 0) {
+			fprintf(stderr,
+				"Found %d roots with an outdated root item.\n",
+				ret);
+			fprintf(stderr,
+				"Please run a filesystem check with the option --repair to fix them.\n");
+			ret = 1;
+			err |= !!ret;
+			goto close_out;
+		}
+	}
+
 	ret = do_check_chunks_and_extents(info);
 	err |= !!ret;
 	if (ret)
 		error(
 		"errors found in extent allocation tree or chunk allocation");
 
-	ret = repair_root_items(info);
-	err |= !!ret;
-	if (ret < 0) {
-		error("failed to repair root items: %s", strerror(-ret));
-		goto close_out;
-	}
-	if (repair) {
-		fprintf(stderr, "Fixed %d roots.\n", ret);
-		ret = 0;
-	} else if (ret > 0) {
-		fprintf(stderr,
-		       "Found %d roots with an outdated root item.\n",
-		       ret);
-		fprintf(stderr,
-			"Please run a filesystem check with the option --repair to fix them.\n");
-		ret = 1;
-		err |= !!ret;
-		goto close_out;
-	}
-
 	if (!ctx.progress_enabled) {
 		if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
 			fprintf(stderr, "checking free space tree\n");
-- 
2.14.1

[PATCH v3 2/5] btrfs-progs: check: call repair_root_items() before any repair

[Su Yue]

The Annotation of repair_root_items():
"This must be run before any other repair code - not doing it so,
makes other repair code delete or modify backrefs in the extent tree
for example, which will result in an inconsistent fs after repairing
the root items."

However, the rule was broken by commit 1f728b1a514f ("Btrfs-progs,
fsck: move root items repair after root rebuilding").
The commit intends to fix failure of test-fsck/013 so it moves
repair_root_items() after check_extents_and_chunks().

The correct way is to skip calling repair_root_item() when
init_extent_tree is nonzero.
Now put repair_root_items() before do_check_chunks_and_extents() and
not call repair_root_items() if do init_extent_tree.
Then test-fsck/013 works well.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
Changelog:
v2:
   Correct commit number reported. (Suggested by Qu Wenruo)
v3:
   Change commit format. (Suggested by Qu Wenruo)
   
 cmds-check.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 93b47194..3e2f9faa 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -14644,32 +14644,35 @@ int cmd_check(int argc, char **argv)
 		goto close_out;
 	}
 
+	if (!init_extent_tree) {
+		ret = repair_root_items(info);
+		err |= !!ret;
+		if (ret < 0) {
+			error("failed to repair root items: %s",
+			      strerror(-ret));
+			goto close_out;
+		}
+		if (repair) {
+			fprintf(stderr, "Fixed %d roots.\n", ret);
+			ret = 0;
+		} else if (ret > 0) {
+			fprintf(stderr,
+				"Found %d roots with an outdated root item.\n",
+				ret);
+			fprintf(stderr,
+				"Please run a filesystem check with the option --repair to fix them.\n");
+			ret = 1;
+			err |= !!ret;
+			goto close_out;
+		}
+	}
+
 	ret = do_check_chunks_and_extents(info);
 	err |= !!ret;
 	if (ret)
 		error(
 		"errors found in extent allocation tree or chunk allocation");
 
-	ret = repair_root_items(info);
-	err |= !!ret;
-	if (ret < 0) {
-		error("failed to repair root items: %s", strerror(-ret));
-		goto close_out;
-	}
-	if (repair) {
-		fprintf(stderr, "Fixed %d roots.\n", ret);
-		ret = 0;
-	} else if (ret > 0) {
-		fprintf(stderr,
-		       "Found %d roots with an outdated root item.\n",
-		       ret);
-		fprintf(stderr,
-			"Please run a filesystem check with the option --repair to fix them.\n");
-		ret = 1;
-		err |= !!ret;
-		goto close_out;
-	}
-
 	if (!ctx.progress_enabled) {
 		if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
 			fprintf(stderr, "checking free space tree\n");
-- 
2.14.1

Re: [PATCH 1/5] btrfs-progs: check: return value of check_extent_refs()

[David Sterba]

On Wed, Sep 27, 2017 at 02:34:36PM +0800, Su Yue wrote:
> In original check mode(without option "--repair"), check_extent_refs()
> always returns 0.
> 
> Add a variable @error to record status while checking extents.
> At the end of check_extent_refs(), let it return -EIO if @error is
> nonzero.
> 
> Example:
> $ btrfs check bad-extent-inline-ref-type.raw
> Checking filesystem on bad-extent-inline-ref-type.raw
> UUID: 1942d6fe-617b-4499-9982-cc8ffae5447f
> checking extents
> corrupt extent record: key 29360128 169 16384
> ref mismatch on [29360128 16384] extent item 0, found 1
> Backref 29360128 parent 5 root 5 not found in extent tree
> backpointer mismatch on [29360128 16384]
> bad extent [29360128, 29376512), type mismatch with chunk
> checking free space cache
> checking fs roots
> checking csums
> checking root refs
> found 114688 bytes used, no error found
> total csum bytes: 0
> total tree bytes: 114688
> total fs tree bytes: 32768
> total extent tree bytes: 16384
> btree space waste bytes: 109471
> file data blocks allocated: 0
>  referenced 0
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>

With this patch applied, the test fsck/006 fails, is that intentional?

Log from the last failing testcase:

checking extents
ref mismatch on [15474688 905216] extent item 1, found 4
Data backref 15474688 parent 31817728 owner 0 offset 0 num_refs 0 not found in extent tree
Incorrect local backref count on 15474688 parent 31817728 owner 0 offset 0 found 1 wanted 0 back 0x12d02f0
Data backref 15474688 parent 30883840 owner 0 offset 0 num_refs 0 not found in extent tree
Incorrect local backref count on 15474688 parent 30883840 owner 0 offset 0 found 1 wanted 0 back 0x12cd710
Data backref 15474688 parent 31326208 owner 0 offset 0 num_refs 0 not found in extent tree
Incorrect local backref count on 15474688 parent 31326208 owner 0 offset 0 found 1 wanted 0 back 0x12d2b80
backpointer mismatch on [15474688 905216]
ERROR: errors found in extent allocation tree or chunk allocation
checking free space cache
checking fs roots
checking csums
checking root refs
Checking filesystem on test.img
UUID: 3857c23d-4219-4600-a636-ac7707dc4ff3
cache and super generation don't match, space cache will be invalidated
found 6291456 bytes used, error(s) found
total csum bytes: 660
total tree bytes: 786432
total fs tree bytes: 688128
total extent tree bytes: 16384
btree space waste bytes: 459860
file data blocks allocated: 35536896
 referenced 25890816
failed: /mnt/big/dsterba/backup-labs-subv/gits/btrfs-progs/btrfs check test.img

I haven't tried with the other patches applied, so it might get actually fixed.
Even in that case, I'd rather keep the branches bisectable.

Re: [PATCH 1/5] btrfs-progs: check: return value of check_extent_refs()

[Su Yue]

On 10/06/2017 01:46 AM, David Sterba wrote:
> On Wed, Sep 27, 2017 at 02:34:36PM +0800, Su Yue wrote:
>> In original check mode(without option "--repair"), check_extent_refs()
>> always returns 0.
>>
>> Add a variable @error to record status while checking extents.
>> At the end of check_extent_refs(), let it return -EIO if @error is
>> nonzero.
>>
>> Example:
>> $ btrfs check bad-extent-inline-ref-type.raw
>> Checking filesystem on bad-extent-inline-ref-type.raw
>> UUID: 1942d6fe-617b-4499-9982-cc8ffae5447f
>> checking extents
>> corrupt extent record: key 29360128 169 16384
>> ref mismatch on [29360128 16384] extent item 0, found 1
>> Backref 29360128 parent 5 root 5 not found in extent tree
>> backpointer mismatch on [29360128 16384]
>> bad extent [29360128, 29376512), type mismatch with chunk
>> checking free space cache
>> checking fs roots
>> checking csums
>> checking root refs
>> found 114688 bytes used, no error found
>> total csum bytes: 0
>> total tree bytes: 114688
>> total fs tree bytes: 32768
>> total extent tree bytes: 16384
>> btree space waste bytes: 109471
>> file data blocks allocated: 0
>>   referenced 0
>>
>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> 
> With this patch applied, the test fsck/006 fails, is that intentional?
> 
Yes, it's expected. This patch just let btrfs-progs return a correct
value.

> Log from the last failing testcase:
> 
> checking extents
> ref mismatch on [15474688 905216] extent item 1, found 4
> Data backref 15474688 parent 31817728 owner 0 offset 0 num_refs 0 not found in extent tree
> Incorrect local backref count on 15474688 parent 31817728 owner 0 offset 0 found 1 wanted 0 back 0x12d02f0
> Data backref 15474688 parent 30883840 owner 0 offset 0 num_refs 0 not found in extent tree
> Incorrect local backref count on 15474688 parent 30883840 owner 0 offset 0 found 1 wanted 0 back 0x12cd710
> Data backref 15474688 parent 31326208 owner 0 offset 0 num_refs 0 not found in extent tree
> Incorrect local backref count on 15474688 parent 31326208 owner 0 offset 0 found 1 wanted 0 back 0x12d2b80
> backpointer mismatch on [15474688 905216]
> ERROR: errors found in extent allocation tree or chunk allocation
> checking free space cache
> checking fs roots
> checking csums
> checking root refs
> Checking filesystem on test.img
> UUID: 3857c23d-4219-4600-a636-ac7707dc4ff3
> cache and super generation don't match, space cache will be invalidated
> found 6291456 bytes used, error(s) found
> total csum bytes: 660
> total tree bytes: 786432
> total fs tree bytes: 688128
> total extent tree bytes: 16384
> btree space waste bytes: 459860
> file data blocks allocated: 35536896
>   referenced 25890816
> failed: /mnt/big/dsterba/backup-labs-subv/gits/btrfs-progs/btrfs check test.img
> 
The log shows that there are errors in the extent-tree but no error
value returned.

And the abormal output was caused by commit 1f728b1a514f ("Btrfs-
progs,fsck: move root items repair after root rebuilding").

> I haven't tried with the other patches applied, so it might get actually fixed.
> Even in that case, I'd rather keep the branches bisectable.
> 
It is described and fixed by the second and third patch in the
patchset.

Thanks,
Su
> 

Re: [PATCH 1/5] btrfs-progs: check: return value of check_extent_refs()

[Su Yue]

On 10/06/2017 01:46 AM, David Sterba wrote:
> On Wed, Sep 27, 2017 at 02:34:36PM +0800, Su Yue wrote:
>> In original check mode(without option "--repair"), check_extent_refs()
>> always returns 0.
>>
>> Add a variable @error to record status while checking extents.
>> At the end of check_extent_refs(), let it return -EIO if @error is
>> nonzero.
>>
>> Example:
>> $ btrfs check bad-extent-inline-ref-type.raw
>> Checking filesystem on bad-extent-inline-ref-type.raw
>> UUID: 1942d6fe-617b-4499-9982-cc8ffae5447f
>> checking extents
>> corrupt extent record: key 29360128 169 16384
>> ref mismatch on [29360128 16384] extent item 0, found 1
>> Backref 29360128 parent 5 root 5 not found in extent tree
>> backpointer mismatch on [29360128 16384]
>> bad extent [29360128, 29376512), type mismatch with chunk
>> checking free space cache
>> checking fs roots
>> checking csums
>> checking root refs
>> found 114688 bytes used, no error found
>> total csum bytes: 0
>> total tree bytes: 114688
>> total fs tree bytes: 32768
>> total extent tree bytes: 16384
>> btree space waste bytes: 109471
>> file data blocks allocated: 0
>>   referenced 0
>>
>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> 
> With this patch applied, the test fsck/006 fails, is that intentional?
> 
> Log from the last failing testcase:
> 
> checking extents
> ref mismatch on [15474688 905216] extent item 1, found 4
> Data backref 15474688 parent 31817728 owner 0 offset 0 num_refs 0 not found in extent tree
> Incorrect local backref count on 15474688 parent 31817728 owner 0 offset 0 found 1 wanted 0 back 0x12d02f0
> Data backref 15474688 parent 30883840 owner 0 offset 0 num_refs 0 not found in extent tree
> Incorrect local backref count on 15474688 parent 30883840 owner 0 offset 0 found 1 wanted 0 back 0x12cd710
> Data backref 15474688 parent 31326208 owner 0 offset 0 num_refs 0 not found in extent tree
> Incorrect local backref count on 15474688 parent 31326208 owner 0 offset 0 found 1 wanted 0 back 0x12d2b80
> backpointer mismatch on [15474688 905216]
> ERROR: errors found in extent allocation tree or chunk allocation
> checking free space cache
> checking fs roots
> checking csums
> checking root refs
> Checking filesystem on test.img
> UUID: 3857c23d-4219-4600-a636-ac7707dc4ff3
> cache and super generation don't match, space cache will be invalidated
> found 6291456 bytes used, error(s) found
> total csum bytes: 660
> total tree bytes: 786432
> total fs tree bytes: 688128
> total extent tree bytes: 16384
> btree space waste bytes: 459860
> file data blocks allocated: 35536896
>   referenced 25890816
> failed: /mnt/big/dsterba/backup-labs-subv/gits/btrfs-progs/btrfs check test.img
> 
> I haven't tried with the other patches applied, so it might get actually fixed.
> Even in that case, I'd rather keep the branches bisectable.
> 
Understood, I will do it in next version.
> 

Re: [PATCH 1/5] btrfs-progs: check: return value of check_extent_refs()

[David Sterba]

On Fri, Oct 06, 2017 at 09:38:05AM +0800, Su Yue wrote:
> 
> 
> On 10/06/2017 01:46 AM, David Sterba wrote:
> > On Wed, Sep 27, 2017 at 02:34:36PM +0800, Su Yue wrote:
> >> In original check mode(without option "--repair"), check_extent_refs()
> >> always returns 0.
> >>
> >> Add a variable @error to record status while checking extents.
> >> At the end of check_extent_refs(), let it return -EIO if @error is
> >> nonzero.
> >>
> >> Example:
> >> $ btrfs check bad-extent-inline-ref-type.raw
> >> Checking filesystem on bad-extent-inline-ref-type.raw
> >> UUID: 1942d6fe-617b-4499-9982-cc8ffae5447f
> >> checking extents
> >> corrupt extent record: key 29360128 169 16384
> >> ref mismatch on [29360128 16384] extent item 0, found 1
> >> Backref 29360128 parent 5 root 5 not found in extent tree
> >> backpointer mismatch on [29360128 16384]
> >> bad extent [29360128, 29376512), type mismatch with chunk
> >> checking free space cache
> >> checking fs roots
> >> checking csums
> >> checking root refs
> >> found 114688 bytes used, no error found
> >> total csum bytes: 0
> >> total tree bytes: 114688
> >> total fs tree bytes: 32768
> >> total extent tree bytes: 16384
> >> btree space waste bytes: 109471
> >> file data blocks allocated: 0
> >>   referenced 0
> >>
> >> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> > 
> > With this patch applied, the test fsck/006 fails, is that intentional?
> > 
> Yes, it's expected. This patch just let btrfs-progs return a correct
> value.

Ok thanks. I'll add a note about that to the changelog, this kind of
information is important when we knowingly break bisectability.

Re: [PATCH 0/5] btrfs-progs: check: original and lowmem mode fix

[David Sterba]

On Wed, Sep 27, 2017 at 02:34:35PM +0800, Su Yue wrote:
> The head three patches is independent from later two.
> Include:
> 1) Let check_extent_refs() return error instead of 0 if something wrong
>    is found in original mode.
> 2) repair_root_items() should be called before any repair. Put it
>    before do_check_chunks_and_extents();
> 3) Error status should not be modified if repair_root_items() succeeded
>    in original repair.
> 
> The late two patches are about invalid type in extent_inline_ref.
> 
> Only original mode applied 001-003 patches can handle the case in the
> last patch. So I gather those 5 patches together.
> 
> 
> Su Yue (5):
>   btrfs-progs: check: return value of check_extent_refs()
>   btrfs-progs: check: call repair_root_items() before any repair
>   btrfs-progs: check: error or return value of repair_root_items()
>   btrfs-progs: check: check extent_inline_ref in lowmem
>   btrfs-progs: fsck-tests: 027/bad_extent_inline_ref_type

1-5 applied, with minor adjustments. Thanks.