| Message ID | 20161129073303.14776-2-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Nov 29, 2016 at 03:32:54PM +0800, Qu Wenruo wrote: > Old btrfs qgroup test cases uses fix golden output numbers, which limits > the coverage since they can't handle mount options like compress or > inode_map, and cause false alert. > > Introduce _btrfs_check_scratch_qgroup() function to check qgroup > correctness using "btrfs check --qgroup-report" function, which will > follow the way kernel handle qgroup and are proved very reliable. > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > --- > common/rc | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/common/rc b/common/rc > index 8c99306..35d2d56 100644 > --- a/common/rc > +++ b/common/rc > @@ -3018,6 +3018,25 @@ _require_deletable_scratch_dev_pool() > done > } > > +# We check if "btrfs check" support to check qgroup correctness > +# Old fixed golden output can cover case like compress and inode_map > +# mount options, which limits the coverage > +_require_btrfs_check_qgroup() > +{ > + _require_command "$BTRFS_UTIL_PROG" btrfs > + output=$($BTRFS_UTIL_PROG check --help | grep "qgroup-report") > + if [ -z "$output" ]; then No need to save command output to a var, you can check the return value of btrfs, e.g. if ! $BTRFS_UTIL_PROG check --help | grep -q "qgroup-report"; then _notrun "..." fi The output is only needed when we need to do further operations on it, e.g. in _require_xfs_io_command. > + _notrun "$BTRFS_UTIL_PROG too old (must support 'check --qgroup-report')" > + fi > +} > + > +_btrfs_check_scratch_qgroup() > +{ > + _require_btrfs_check_qgroup This _require belongs to each test that calls _btrfs_check_scratch_qgroup. And I think you can put the introduction of this helper and the conversions to use this helper all together in one patch (so we know why it's introduced and how it gets used from one patch), and all adding extra qgroup check updates to another patch. Thanks, Eryu > + $BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 |\ > + grep -E "Counts for qgroup.*are different" > +} > + > # We check for btrfs and (optionally) features of the btrfs command > _require_btrfs() > { > -- > 2.7.4 > > > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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
At 11/29/2016 04:16 PM, Eryu Guan wrote: > On Tue, Nov 29, 2016 at 03:32:54PM +0800, Qu Wenruo wrote: >> Old btrfs qgroup test cases uses fix golden output numbers, which limits >> the coverage since they can't handle mount options like compress or >> inode_map, and cause false alert. >> >> Introduce _btrfs_check_scratch_qgroup() function to check qgroup >> correctness using "btrfs check --qgroup-report" function, which will >> follow the way kernel handle qgroup and are proved very reliable. >> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >> --- >> common/rc | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/common/rc b/common/rc >> index 8c99306..35d2d56 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -3018,6 +3018,25 @@ _require_deletable_scratch_dev_pool() >> done >> } >> >> +# We check if "btrfs check" support to check qgroup correctness >> +# Old fixed golden output can cover case like compress and inode_map >> +# mount options, which limits the coverage >> +_require_btrfs_check_qgroup() >> +{ >> + _require_command "$BTRFS_UTIL_PROG" btrfs >> + output=$($BTRFS_UTIL_PROG check --help | grep "qgroup-report") >> + if [ -z "$output" ]; then > > No need to save command output to a var, you can check the return value > of btrfs, e.g. > > if ! $BTRFS_UTIL_PROG check --help | grep -q "qgroup-report"; then > _notrun "..." > fi Thanks for the advice. I'm not familiar with bash gramma, like the difference of [] or without []. So I choose the tried and true method. Anyway I'll change it in next version. > > The output is only needed when we need to do further operations on it, > e.g. in _require_xfs_io_command. > >> + _notrun "$BTRFS_UTIL_PROG too old (must support 'check --qgroup-report')" >> + fi >> +} >> + >> +_btrfs_check_scratch_qgroup() >> +{ >> + _require_btrfs_check_qgroup > > This _require belongs to each test that calls > _btrfs_check_scratch_qgroup. Yes, I was originally planning to do it, but it seems that some other common function also uses this method. So I choose this to reduce the modification. I'm OK to change it in next version. > > And I think you can put the introduction of this helper and the > conversions to use this helper all together in one patch (so we know why > it's introduced and how it gets used from one patch), and all adding > extra qgroup check updates to another patch. The idea is allow further review for each test case modification. Since some modification is easy to be wrong. Like forget to add echo "Silence is golden" while removing the only output. If all the modification are proved to be good, I can fold them all into one patch. Thanks, Qu > > Thanks, > Eryu > >> + $BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 |\ >> + grep -E "Counts for qgroup.*are different" >> +} >> + >> # We check for btrfs and (optionally) features of the btrfs command >> _require_btrfs() >> { >> -- >> 2.7.4 >> >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe fstests" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- 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
On Tue, Nov 29, 2016 at 03:32:54PM +0800, Qu Wenruo wrote: > Old btrfs qgroup test cases uses fix golden output numbers, which limits > the coverage since they can't handle mount options like compress or > inode_map, and cause false alert. > > Introduce _btrfs_check_scratch_qgroup() function to check qgroup > correctness using "btrfs check --qgroup-report" function, which will > follow the way kernel handle qgroup and are proved very reliable. > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > --- > common/rc | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/common/rc b/common/rc > index 8c99306..35d2d56 100644 > --- a/common/rc > +++ b/common/rc > @@ -3018,6 +3018,25 @@ _require_deletable_scratch_dev_pool() > done > } > > +# We check if "btrfs check" support to check qgroup correctness > +# Old fixed golden output can cover case like compress and inode_map > +# mount options, which limits the coverage > +_require_btrfs_check_qgroup() > +{ > + _require_command "$BTRFS_UTIL_PROG" btrfs > + output=$($BTRFS_UTIL_PROG check --help | grep "qgroup-report") > + if [ -z "$output" ]; then > + _notrun "$BTRFS_UTIL_PROG too old (must support 'check --qgroup-report')" > + fi > +} Why wouldn't this just set a global variable that you then check in _check_scratch_fs and run the _btrfs_check_scratch_qgroup() call then? What about all the tests that currently run without this functionality being present? They will now notrun rather than use the golden output match - this seems like a regression to me, especially for distro QE testing older kernel/progs combinations... > + > +_btrfs_check_scratch_qgroup() > +{ > + _require_btrfs_check_qgroup This needs to go in the test itself before the test is run, not get hidden in a function call at the end of the test. Cheers, Dave.
At 11/30/2016 05:01 AM, Dave Chinner wrote: > On Tue, Nov 29, 2016 at 03:32:54PM +0800, Qu Wenruo wrote: >> Old btrfs qgroup test cases uses fix golden output numbers, which limits >> the coverage since they can't handle mount options like compress or >> inode_map, and cause false alert. >> >> Introduce _btrfs_check_scratch_qgroup() function to check qgroup >> correctness using "btrfs check --qgroup-report" function, which will >> follow the way kernel handle qgroup and are proved very reliable. >> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >> --- >> common/rc | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/common/rc b/common/rc >> index 8c99306..35d2d56 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -3018,6 +3018,25 @@ _require_deletable_scratch_dev_pool() >> done >> } >> >> +# We check if "btrfs check" support to check qgroup correctness >> +# Old fixed golden output can cover case like compress and inode_map >> +# mount options, which limits the coverage >> +_require_btrfs_check_qgroup() >> +{ >> + _require_command "$BTRFS_UTIL_PROG" btrfs >> + output=$($BTRFS_UTIL_PROG check --help | grep "qgroup-report") >> + if [ -z "$output" ]; then >> + _notrun "$BTRFS_UTIL_PROG too old (must support 'check --qgroup-report')" >> + fi >> +} > > Why wouldn't this just set a global variable that you then > check in _check_scratch_fs and run the _btrfs_check_scratch_qgroup() > call then? The problem is, "btrfs check --qgroup-report" will do force report, even for case like qgroup rescan still running. Some test, like btrfs/114 which tests rescan, false report will cause problem. So here I choose the manually checking other than always do it at _check_scratch_fs(). > > What about all the tests that currently run without this > functionality being present? They will now notrun rather than use > the golden output match - this seems like a regression to me, > especially for distro QE testing older kernel/progs combinations... In fact, the support of qgroup-report is introduced much earlier. It's about v3.14. For other fs, old tool combination would be OK, but for fs like btrfs, I don't think that's sane. Although I could exclude these fixed golden output in next version for now, until we have a good agreement on the behavior. BTW, currently btrfs qgroup test cases are already using "btrfs check --qgroup-report" in newer test cases. Even without checking for the support of --qgroup-report option. So it should already cause a lot of problem for any btrfs-progs earlier than v3.14. But at least I didn't see such report in fstests ML. Thanks, Qu > >> + >> +_btrfs_check_scratch_qgroup() >> +{ >> + _require_btrfs_check_qgroup > > This needs to go in the test itself before the test is run, > not get hidden in a function call at the end of the test. > > Cheers, > > Dave. > -- 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
On Wed, Nov 30, 2016 at 08:56:03AM +0800, Qu Wenruo wrote: > > > At 11/30/2016 05:01 AM, Dave Chinner wrote: > >On Tue, Nov 29, 2016 at 03:32:54PM +0800, Qu Wenruo wrote: > >>Old btrfs qgroup test cases uses fix golden output numbers, which limits > >>the coverage since they can't handle mount options like compress or > >>inode_map, and cause false alert. > >> > >>Introduce _btrfs_check_scratch_qgroup() function to check qgroup > >>correctness using "btrfs check --qgroup-report" function, which will > >>follow the way kernel handle qgroup and are proved very reliable. > >> > >>Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > >>--- > >> common/rc | 19 +++++++++++++++++++ > >> 1 file changed, 19 insertions(+) > >> > >>diff --git a/common/rc b/common/rc > >>index 8c99306..35d2d56 100644 > >>--- a/common/rc > >>+++ b/common/rc > >>@@ -3018,6 +3018,25 @@ _require_deletable_scratch_dev_pool() > >> done > >> } > >> > >>+# We check if "btrfs check" support to check qgroup correctness > >>+# Old fixed golden output can cover case like compress and inode_map > >>+# mount options, which limits the coverage > >>+_require_btrfs_check_qgroup() > >>+{ > >>+ _require_command "$BTRFS_UTIL_PROG" btrfs > >>+ output=$($BTRFS_UTIL_PROG check --help | grep "qgroup-report") > >>+ if [ -z "$output" ]; then > >>+ _notrun "$BTRFS_UTIL_PROG too old (must support 'check --qgroup-report')" > >>+ fi > >>+} > > > >Why wouldn't this just set a global variable that you then > >check in _check_scratch_fs and run the _btrfs_check_scratch_qgroup() > >call then? > > The problem is, "btrfs check --qgroup-report" will do force report, > even for case like qgroup rescan still running. > > Some test, like btrfs/114 which tests rescan, false report will > cause problem. So for those specific tests, you aren't going to be running "btrfs check --qgroup-report", right? In which case, those tests should not call _require_btrfs_check_qgroup(), and then _check_scratch_fs() will not run the quota check. i.e. there will be no difference to the current behaviour. > So here I choose the manually checking other than always do it at > _check_scratch_fs(). I don't see what the problem you are avoiding is. Either it is safe to run the quota check or it isn't, and triggering it to run in _check_scratch_fs() via a _requires rule makes no difference to that. Cheers, Dave.
diff --git a/common/rc b/common/rc index 8c99306..35d2d56 100644 --- a/common/rc +++ b/common/rc @@ -3018,6 +3018,25 @@ _require_deletable_scratch_dev_pool() done } +# We check if "btrfs check" support to check qgroup correctness +# Old fixed golden output can cover case like compress and inode_map +# mount options, which limits the coverage +_require_btrfs_check_qgroup() +{ + _require_command "$BTRFS_UTIL_PROG" btrfs + output=$($BTRFS_UTIL_PROG check --help | grep "qgroup-report") + if [ -z "$output" ]; then + _notrun "$BTRFS_UTIL_PROG too old (must support 'check --qgroup-report')" + fi +} + +_btrfs_check_scratch_qgroup() +{ + _require_btrfs_check_qgroup + $BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 |\ + grep -E "Counts for qgroup.*are different" +} + # We check for btrfs and (optionally) features of the btrfs command _require_btrfs() {
Old btrfs qgroup test cases uses fix golden output numbers, which limits the coverage since they can't handle mount options like compress or inode_map, and cause false alert. Introduce _btrfs_check_scratch_qgroup() function to check qgroup correctness using "btrfs check --qgroup-report" function, which will follow the way kernel handle qgroup and are proved very reliable. Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- common/rc | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)