Re: [PATCH 01/10] fstests: common: Introduce function to check qgroup correctness

[Dave Chinner <david@fromorbit.com>]

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.
-- 
Dave Chinner
david@fromorbit.com