[PATCH 1/2] btrfs-progs: check: Make btrfs check return error for qgroup mismatch

[PATCH 1/2] btrfs-progs: check: Make btrfs check return error for qgroup mismatch

[Qu Wenruo]

Current btrfs-check will check qgroup consistency, but even when it
finds something wrong, the return value is still 0.

Fix it by allowing report_qgroups() to return int to indicate qgroup
mismatch, and also add extra logical to return no error if qgroup repair
is successful.

Without this patch, fstests can't detect qgroup corruption by its fsck
alone.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c    | 10 ++++++----
 qgroup-verify.c | 20 +++++++++++++++++---
 qgroup-verify.h |  2 +-
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/check/main.c b/check/main.c
index c4a1801fb0ef..d49b4326915e 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9467,6 +9467,7 @@ int cmd_check(int argc, char **argv)
 	int clear_space_cache = 0;
 	int qgroup_report = 0;
 	int qgroups_repaired = 0;
+	int qgroup_report_ret;
 	unsigned ctree_flags = OPEN_CTREE_EXCLUSIVE;
 	int force = 0;
 
@@ -9709,7 +9710,7 @@ int cmd_check(int argc, char **argv)
 		ret = qgroup_verify_all(info);
 		err |= !!ret;
 		if (ret == 0)
-			report_qgroups(1);
+			err |= !!report_qgroups(1);
 		goto close_out;
 	}
 	if (subvolid) {
@@ -9894,13 +9895,14 @@ int cmd_check(int argc, char **argv)
 			error("failed to check quota groups");
 			goto out;
 		}
-		report_qgroups(0);
+		qgroup_report_ret = report_qgroups(0);
 		ret = repair_qgroups(info, &qgroups_repaired);
-		err |= !!ret;
-		if (err) {
+		if (ret) {
 			error("failed to repair quota groups");
 			goto out;
 		}
+		if (qgroup_report_ret && (!qgroups_repaired || ret))
+			err |= qgroup_report_ret;
 		ret = 0;
 	}
 
diff --git a/qgroup-verify.c b/qgroup-verify.c
index 571b4d4f7171..4deb9879fb35 100644
--- a/qgroup-verify.c
+++ b/qgroup-verify.c
@@ -1298,10 +1298,19 @@ static int report_qgroup_difference(struct qgroup_count *count, int verbose)
 	return is_different;
 }
 
-void report_qgroups(int all)
+/*
+ * Report qgroups errors
+ * Return 0 if nothing wrong.
+ * Return <0 if any qgroup is inconsistent.
+ *
+ * @all:	if set, all qgroup will be checked and reported even already
+ * 		inconsistent or under rescan.
+ */
+int report_qgroups(int all)
 {
 	struct rb_node *node;
 	struct qgroup_count *c;
+	bool found_err = false;
 
 	if (!repair && counts.rescan_running) {
 		if (all) {
@@ -1310,7 +1319,7 @@ void report_qgroups(int all)
 		} else {
 			printf(
 	"Qgroup rescan is running, qgroups will not be printed.\n");
-			return;
+			return 0;
 		}
 	}
 	if (counts.qgroup_inconsist && !counts.rescan_running)
@@ -1319,11 +1328,16 @@ void report_qgroups(int all)
 	while (node) {
 		c = rb_entry(node, struct qgroup_count, rb_node);
 
-		if (report_qgroup_difference(c, all))
+		if (report_qgroup_difference(c, all)) {
 			list_add_tail(&c->bad_list, &bad_qgroups);
+			found_err = true;
+		}
 
 		node = rb_next(node);
 	}
+	if (found_err)
+		return -EUCLEAN;
+	return 0;
 }
 
 void free_qgroup_counts(void)
diff --git a/qgroup-verify.h b/qgroup-verify.h
index d7d83a46ed5a..14d36bbf81d1 100644
--- a/qgroup-verify.h
+++ b/qgroup-verify.h
@@ -23,7 +23,7 @@
 #include "ctree.h"
 
 int qgroup_verify_all(struct btrfs_fs_info *info);
-void report_qgroups(int all);
+int report_qgroups(int all);
 int repair_qgroups(struct btrfs_fs_info *info, int *repaired);
 
 int print_extent_state(struct btrfs_fs_info *info, u64 subvol);
-- 
2.17.0

[PATCH 2/2] btrfs-progs: fsck-tests: Add test case to ensure btrfs check return error for corrupted qgroups

[Qu Wenruo]

Since the test case uses run_mustfail(), which is pretty easy pass due to
other unexpected problems, so here an extra run_check() is added to
ensure we don't only report qgroup error, but also fix it without
problem.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../032-corrupted-qgroup/qgroup_corrupted.img | Bin 0 -> 3072 bytes
 tests/fsck-tests/032-corrupted-qgroup/test.sh |  19 ++++++++++++++++++
 2 files changed, 19 insertions(+)
 create mode 100644 tests/fsck-tests/032-corrupted-qgroup/qgroup_corrupted.img
 create mode 100755 tests/fsck-tests/032-corrupted-qgroup/test.sh

diff --git a/tests/fsck-tests/032-corrupted-qgroup/qgroup_corrupted.img b/tests/fsck-tests/032-corrupted-qgroup/qgroup_corrupted.img
new file mode 100644
index 0000000000000000000000000000000000000000..a19a303534d419fa7660615d0dcf25ad314e7b20
GIT binary patch
literal 3072
zcmeH|XHe7G8peZ25!j#v5fFhzB*@YhrCfzT5TpxB?*gl=6d?pJod8jKRaQZyh2D$Q
z00FWJ5wi${wviS<iGZ}w4dlO)xjQ<}-0%Crcg~#m_ntGSJkOk&lj!~!Id{Pk2l`*I
zf6gq-_8In%2DQvS*C5bwW<SX6fujM?5R-?Gu$#o>F(!{Nx#rkW@jbpf@ZEubu>(PG
z+t1`!_C#_>vtjeXh^tM(!>}_(EkZ(hLV|+$xg@jwAZTiSBPHnOjm#KU8*y_mOOjd=
zf8?u6Q5RLp#LqvkV&!KgCu=2~j)lj+Qc)~$=pO9mqMi`f@niL0qlbY_HTI;g6L${j
z2OH-~#`RPvI)!>7ky7<g9-g|!DUj6DK}%`Ox)9zV^~8mE?X5zn+JghOD%w_^3x5*&
z7e%h?cxgelG76|@?18eKtBGUKX5%bhg?i(Y+fmDU8C`KQsbiPjB_}W$czd!Rc9_Kk
zl|Q)%T9iRhkWkjF11&7%ZSm~O#W$5@G**GNLKprtPEgT~%TmzWm5F-87iRpu8~$F`
z7R^5$lgE<{((cv4ixcGWCX?C6oZ14Gr8(JD^IoK!4vf_%_{g*SDY`Uds9-&^9yDH{
z0~`jc8r0k{Q$)Y@p>2TTS`7^FR4r^tHLBcPVt2bLQ}xr7^*}S=uY?VX$jS@dovQbG
z>Cdszl66J4VfzoyMD>y@GZMphHbSN5Zk@eS+vT~1gbY3J^!Y!}#jsqmHm>tc`0wP}
z+!9#fqc%u#_B2Y_d#!DQhEQ`n4xlP-$}J`G6iE+E=9LCeO{|`(TcwpM{WttDJk(k3
zl(1l54Yu4T1NfBz1Ngie?N1$fpkc@g=Lb>9$wP4o$xwZ{auO$>^?5F?iys>sxPy*+
z!Pi0dWkG0udzl>pwBAX{k_7Y<kISu5$`rny>wBH<)6Y~jd5q}+qQawTg&o*{i5%Om
zj#fK6TMeJiXoT1{wQvA=FH=3RB5-^8{`mWyu~jR7*(n=ig4!(`-Ww~U+G+!T8uOwv
z>@}I+#ux38n*6bb#p_N#?`K_q+S<NFze=nwUCDd7K|VmO2ERB23{U_N)aI*nj+;E5
zJPpj`=2WgVsTksh(+N*|hvqD)g=VzkFZ(e`5bh@p@E!tdV{8USNAkN2wOs`{C~LM9
zk`-q)cxVo(_E>Uc!P*AV9f|<Au@GvyF};7_Fx~h*Y_{wmW|k*GBpULtObs@$6*{v^
z_h<7BP;Ui$S!N}V`|U3+*4xxN1&};A5AO`6(){k_g(csZk#}B|qKc_jQ@0SowIbi#
zDAEMiVj8o&5U~TK_wN7^?pSl_TxVQX&yVW-SM(~^&na+yv*NEp^D=8*##yN@Vp$sD
zw|2m85jE|TPG#h4TxCiE<IVU98x}?Eg{q1|KL~IaXuElOPv^%EaqG+SDNz2C`5?|p
zn-s)$3c{KWFTmC8S$IfFJX!l~(IA34IdM-jeh4f6qxtqy*qPcNIEIlmq`(v<2O&o2
zyzp7v9O{{qeflb<(c>~9rJU;a$pa=DK(ZjwR*h#&A4bPjauK<e2BNB$ZYd(HaBn!E
z5~!cUAZ4{nAHGrXPr1%9<lyJPF}^%6+9bzlBQV0dsw<lhN@gC{F-!p>o91ha+0Uw`
zH10rLMF<;Ek4Hv#bhyA^u&`wq9&G>Eu&MNPw6)HNE|krIORn|9i0&Xf%?Aw@aCAZ|
z+)pTgBM>JiNGV|RN{eD0ylz~^^rm{>;WgyLj+3)=p|?~TeMTcAboF+KJXzsVu+_DN
zkA1v9y;*N4^-bB$Yt>{}ycdw@890v;!Ok=Grg!~5Y_A;DzvqV3_F9B!<W#QC<&`<N
z-fsT1U1vUkdz8tu!?Oo>R49eC)%S6SSYE92N)n}w>&K;x(ow~t73MCg+azw%ZT$Xz
zpl@kna=UX~KYi^L3?{Zk*MIFoXVKK}qj=_t9mwx_ue;TX{o~S?oPXk-db0d5qH?@i
z<n9ck=N-7!AMX^fOm{~5^Een?kcqk5{Ls}1foaw?L{a2bl)Nw2H1AAH%K1F1@Rlgf
z#n?*grX)BEoof#Zr)BL+R)mDMSKKYvozLz=weKmcNM*;Wq6wHDUN7Z|q)kNdVGO4H
zqdsy0Pih<9Wp6t>)$bYGGAdF$fCUy7@4j0I4hdxZnosj14o~<m4x}F{`*I)|T`+Ig
zO`!jGps{mw-zFh2(aCF4#L{!#e%V;^q;Y2V_I*8f%@~}E5!q>&{i<%-rJUWrOyZTT
zQ!|wp(0C}MRpM2a*3BiA%x1i{VrFBwEc^Xa6S15DWaTeLd`<a~O!5v=cz^zuE&Oto
zxC9s9b8#syx98mlnW3Y{pjC+^2iAh9*VV301YNkJfMtgd+{V2^#I*raY*8NG#9rja
z=yI4jh`8h&)zVv))jN`J{)~w3*oRR&T=eYLiO1V@zdr6O)`B9Is?uw(DH1<fu}61}
M0{wko&HnrGZzc7KcK`qY

literal 0
HcmV?d00001

diff --git a/tests/fsck-tests/032-corrupted-qgroup/test.sh b/tests/fsck-tests/032-corrupted-qgroup/test.sh
new file mode 100755
index 000000000000..ccecfeac3ca3
--- /dev/null
+++ b/tests/fsck-tests/032-corrupted-qgroup/test.sh
@@ -0,0 +1,19 @@
+#!/bin/bash
+# To check if btrfs check can handle valid orphan items.
+# Orphan item is a marker for deleted inodes that were open at the time of
+# deletion.  # Orphan inode/root will is not referenced and will have an orphan
+# item, which should not be reported as error.
+
+source "$TEST_TOP/common"
+
+check_prereq btrfs
+
+check_image() {
+	run_mustfail "btrfs check failed to detect qgroup corruption" \
+		     "$TOP/btrfs" check "$1"
+	# Above command can fail due to other bugs, so add extra safe net to
+	# ensure we can fix qgroup without problem.
+	run_check "$TOP/btrfs" check --repair "$1"
+}
+
+check_all_images
-- 
2.17.0

[PATCH 1/2] btrfs-progs: check: Make btrfs check return error for qgroup mismatch

[Qu Wenruo]

Current btrfs-check will check qgroup consistency, but even when it
finds something wrong, the return value is still 0.

Fix it by allowing report_qgroups() to return int to indicate qgroup
mismatch, and also add extra logical to return no error if qgroup repair
is successful.

Without this patch, fstests can't detect qgroup corruption by its fsck
alone.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c    | 10 ++++++----
 qgroup-verify.c | 20 +++++++++++++++++---
 qgroup-verify.h |  2 +-
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/check/main.c b/check/main.c
index c4a1801fb0ef..d49b4326915e 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9467,6 +9467,7 @@ int cmd_check(int argc, char **argv)
 	int clear_space_cache = 0;
 	int qgroup_report = 0;
 	int qgroups_repaired = 0;
+	int qgroup_report_ret;
 	unsigned ctree_flags = OPEN_CTREE_EXCLUSIVE;
 	int force = 0;
 
@@ -9709,7 +9710,7 @@ int cmd_check(int argc, char **argv)
 		ret = qgroup_verify_all(info);
 		err |= !!ret;
 		if (ret == 0)
-			report_qgroups(1);
+			err |= !!report_qgroups(1);
 		goto close_out;
 	}
 	if (subvolid) {
@@ -9894,13 +9895,14 @@ int cmd_check(int argc, char **argv)
 			error("failed to check quota groups");
 			goto out;
 		}
-		report_qgroups(0);
+		qgroup_report_ret = report_qgroups(0);
 		ret = repair_qgroups(info, &qgroups_repaired);
-		err |= !!ret;
-		if (err) {
+		if (ret) {
 			error("failed to repair quota groups");
 			goto out;
 		}
+		if (qgroup_report_ret && (!qgroups_repaired || ret))
+			err |= qgroup_report_ret;
 		ret = 0;
 	}
 
diff --git a/qgroup-verify.c b/qgroup-verify.c
index 571b4d4f7171..4deb9879fb35 100644
--- a/qgroup-verify.c
+++ b/qgroup-verify.c
@@ -1298,10 +1298,19 @@ static int report_qgroup_difference(struct qgroup_count *count, int verbose)
 	return is_different;
 }
 
-void report_qgroups(int all)
+/*
+ * Report qgroups errors
+ * Return 0 if nothing wrong.
+ * Return <0 if any qgroup is inconsistent.
+ *
+ * @all:	if set, all qgroup will be checked and reported even already
+ * 		inconsistent or under rescan.
+ */
+int report_qgroups(int all)
 {
 	struct rb_node *node;
 	struct qgroup_count *c;
+	bool found_err = false;
 
 	if (!repair && counts.rescan_running) {
 		if (all) {
@@ -1310,7 +1319,7 @@ void report_qgroups(int all)
 		} else {
 			printf(
 	"Qgroup rescan is running, qgroups will not be printed.\n");
-			return;
+			return 0;
 		}
 	}
 	if (counts.qgroup_inconsist && !counts.rescan_running)
@@ -1319,11 +1328,16 @@ void report_qgroups(int all)
 	while (node) {
 		c = rb_entry(node, struct qgroup_count, rb_node);
 
-		if (report_qgroup_difference(c, all))
+		if (report_qgroup_difference(c, all)) {
 			list_add_tail(&c->bad_list, &bad_qgroups);
+			found_err = true;
+		}
 
 		node = rb_next(node);
 	}
+	if (found_err)
+		return -EUCLEAN;
+	return 0;
 }
 
 void free_qgroup_counts(void)
diff --git a/qgroup-verify.h b/qgroup-verify.h
index d7d83a46ed5a..14d36bbf81d1 100644
--- a/qgroup-verify.h
+++ b/qgroup-verify.h
@@ -23,7 +23,7 @@
 #include "ctree.h"
 
 int qgroup_verify_all(struct btrfs_fs_info *info);
-void report_qgroups(int all);
+int report_qgroups(int all);
 int repair_qgroups(struct btrfs_fs_info *info, int *repaired);
 
 int print_extent_state(struct btrfs_fs_info *info, u64 subvol);
-- 
2.17.0

Re: [PATCH 1/2] btrfs-progs: check: Make btrfs check return error for qgroup mismatch

[David Sterba]

On Mon, Apr 30, 2018 at 02:16:59PM +0800, Qu Wenruo wrote:
> Current btrfs-check will check qgroup consistency, but even when it
> finds something wrong, the return value is still 0.
> 
> Fix it by allowing report_qgroups() to return int to indicate qgroup
> mismatch, and also add extra logical to return no error if qgroup repair
> is successful.
> 
> Without this patch, fstests can't detect qgroup corruption by its fsck
> alone.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Applied, thanks. Please send the 0/N (cover letter) mail even for
2-patch series.

Re: [PATCH 1/2] btrfs-progs: check: Make btrfs check return error for qgroup mismatch

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1033 bytes --]



On 2018年05月09日 01:45, David Sterba wrote:
> On Mon, Apr 30, 2018 at 02:16:59PM +0800, Qu Wenruo wrote:
>> Current btrfs-check will check qgroup consistency, but even when it
>> finds something wrong, the return value is still 0.
>>
>> Fix it by allowing report_qgroups() to return int to indicate qgroup
>> mismatch, and also add extra logical to return no error if qgroup repair
>> is successful.
>>
>> Without this patch, fstests can't detect qgroup corruption by its fsck
>> alone.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Applied, thanks. Please send the 0/N (cover letter) mail even for
> 2-patch series.

Any advice on the necessity of the cover letter?

Is cover letter highly recommended for any patches which have any
dependency like the 2nd test case?

Thanks,
Qu

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] btrfs-progs: check: Make btrfs check return error for qgroup mismatch

[David Sterba]

On Thu, May 10, 2018 at 09:43:09AM +0800, Qu Wenruo wrote:
> 
> 
> On 2018年05月09日 01:45, David Sterba wrote:
> > On Mon, Apr 30, 2018 at 02:16:59PM +0800, Qu Wenruo wrote:
> >> Current btrfs-check will check qgroup consistency, but even when it
> >> finds something wrong, the return value is still 0.
> >>
> >> Fix it by allowing report_qgroups() to return int to indicate qgroup
> >> mismatch, and also add extra logical to return no error if qgroup repair
> >> is successful.
> >>
> >> Without this patch, fstests can't detect qgroup corruption by its fsck
> >> alone.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> > 
> > Applied, thanks. Please send the 0/N (cover letter) mail even for
> > 2-patch series.
> 
> Any advice on the necessity of the cover letter?
> 
> Is cover letter highly recommended for any patches which have any
> dependency like the 2nd test case?

The cover letter is a place where you can get feedback on the whole
patchset, individual patches get the specific comments to the code.

A patchset makes a logical chunk of patches so the cover letter also
serves as the high-level overview of the changes before I or others even
start looking further.

I've seen other maintainers to prefer cover letters, but as this is not
documented and not generally appreciated by developers, it needs to be
stated. Developers who post patches frequently could help to improve the
patch flow process, but lack of cover letter is not a big deal
otherwise.

Re: [PATCH 1/2] btrfs-progs: check: Make btrfs check return error for qgroup mismatch

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1789 bytes --]



On 2018年05月10日 19:45, David Sterba wrote:
> On Thu, May 10, 2018 at 09:43:09AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018年05月09日 01:45, David Sterba wrote:
>>> On Mon, Apr 30, 2018 at 02:16:59PM +0800, Qu Wenruo wrote:
>>>> Current btrfs-check will check qgroup consistency, but even when it
>>>> finds something wrong, the return value is still 0.
>>>>
>>>> Fix it by allowing report_qgroups() to return int to indicate qgroup
>>>> mismatch, and also add extra logical to return no error if qgroup repair
>>>> is successful.
>>>>
>>>> Without this patch, fstests can't detect qgroup corruption by its fsck
>>>> alone.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>
>>> Applied, thanks. Please send the 0/N (cover letter) mail even for
>>> 2-patch series.
>>
>> Any advice on the necessity of the cover letter?
>>
>> Is cover letter highly recommended for any patches which have any
>> dependency like the 2nd test case?
> 
> The cover letter is a place where you can get feedback on the whole
> patchset, individual patches get the specific comments to the code.
> 
> A patchset makes a logical chunk of patches so the cover letter also
> serves as the high-level overview of the changes before I or others even
> start looking further.
> 
> I've seen other maintainers to prefer cover letters, but as this is not
> documented and not generally appreciated by developers, it needs to be
> stated. Developers who post patches frequently could help to improve the
> patch flow process, but lack of cover letter is not a big deal
> otherwise.

Much appreciated about the explain on this topic.

Personally I'm completely fine for cover letters, so I would take this
method for later logically related patches.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]