[PATCH 00/10] Enhance btrfs qgroup test group coverage to support more mount options

[PATCH 00/10] Enhance btrfs qgroup test group coverage to support more mount options

[Qu Wenruo]

Old test cases in btrfs qgroup test group use fixed golden output.
This limits the coverage, since mount option like compress and
inode_cache and populate output easily.

On the other hand, "btrfs check" has support for checking qgroup
correctness at least from 3 kernel release before.
And that function is proved to be stable and helped to exposed quite a
lot of qgroup bugs.

So this patchset will use "btrfs check --qgroup-report" to replace fixed
golden output, and add extra check for some existing test cases.

This will reduce false alerts and improve the coverage.
In fact, with these patches and "-o inode_cache" mount option, we have
already exposed a new qgroup bug.

Qu Wenruo (10):
  fstests: common: Introduce function to check qgroup correctness
  fstests: btrfs/017: Use new _btrfs_check_scratch_qgroup function
  fstests: btrfs/022: Add extra qgroup verification after each work
  fstests: btrfs/028: Use new wrapped _btrfs_check_scratch_qgroup
    function
  fstests: btrfs/042: Add extra qgroup verification
  fstests: btrfs/091: Use _btrfs_check_scratch_qgroup other than fixed  
      golden output
  fstests: btrfs/099: Add extra verification for qgroup
  fstests: btrfs/104: Use _btrfs_check_scratch_qgroup to replace open   
     codes
  fstests: btrfs/122: Use _btrfs_check_scratch_qgroup to replace open   
     code
  fstests: btrfs/123: Use _btrfs_check_scratch_qgroup to replace open   
     code

 common/rc           | 19 +++++++++++++++++++
 tests/btrfs/017     |  4 ++--
 tests/btrfs/017.out |  2 --
 tests/btrfs/022     |  5 +++++
 tests/btrfs/028     |  4 ++--
 tests/btrfs/042     |  3 +++
 tests/btrfs/091     |  5 ++---
 tests/btrfs/091.out |  4 ----
 tests/btrfs/099     |  3 +++
 tests/btrfs/104     | 11 ++---------
 tests/btrfs/122     |  9 ++-------
 tests/btrfs/123     |  4 +---
 12 files changed, 41 insertions(+), 32 deletions(-)

-- 
2.7.4

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

[Qu Wenruo]

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
+}
+
+_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()
 {
-- 
2.7.4

[PATCH 02/10] fstests: btrfs/017: Use new _btrfs_check_scratch_qgroup function

[Qu Wenruo]

Now this test case can handle inode_map mount option and expose error
for that mount option.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/btrfs/017     | 4 ++--
 tests/btrfs/017.out | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/btrfs/017 b/tests/btrfs/017
index 3f409d3..356cef5 100755
--- a/tests/btrfs/017
+++ b/tests/btrfs/017
@@ -86,8 +86,8 @@ rm -fr $SCRATCH_MNT/snap/foo*
 
 sync
 
-units=`_btrfs_qgroup_units`
-$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}'
+_scratch_unmount
+_btrfs_check_scratch_qgroup
 
 # success, all done
 status=0
diff --git a/tests/btrfs/017.out b/tests/btrfs/017.out
index 503eb88..bbc8eb4 100644
--- a/tests/btrfs/017.out
+++ b/tests/btrfs/017.out
@@ -1,4 +1,2 @@
 QA output created by 017
 Blocks modified: [0 - 1]
-65536 65536
-65536 65536
-- 
2.7.4

[PATCH 03/10] fstests: btrfs/022: Add extra qgroup verification after each work

[Qu Wenruo]

The old code is using rescan to verify the number.

It will never hurt to add extra qgroup verification using btrfsck.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/btrfs/022 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/btrfs/022 b/tests/btrfs/022
index 56d4f3d..d1ac921 100755
--- a/tests/btrfs/022
+++ b/tests/btrfs/022
@@ -125,20 +125,25 @@ _scratch_mkfs > /dev/null 2>&1
 _scratch_mount
 _basic_test
 _scratch_unmount
+_btrfs_check_scratch_qgroup
 
 _scratch_mkfs > /dev/null 2>&1
 _scratch_mount
 _rescan_test
 _scratch_unmount
+_btrfs_check_scratch_qgroup
 
 _scratch_mkfs > /dev/null 2>&1
 _scratch_mount
 _limit_test_exceed
 _scratch_unmount
+_btrfs_check_scratch_qgroup
 
 _scratch_mkfs > /dev/null 2>&1
 _scratch_mount
 _limit_test_noexceed
+_scratch_unmount
+_btrfs_check_scratch_qgroup
 
 # success, all done
 echo "Silence is golden"
-- 
2.7.4

[PATCH 04/10] fstests: btrfs/028: Use new wrapped _btrfs_check_scratch_qgroup function

[Qu Wenruo]

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/btrfs/028 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/btrfs/028 b/tests/btrfs/028
index 1425609..c4e99c6 100755
--- a/tests/btrfs/028
+++ b/tests/btrfs/028
@@ -87,8 +87,8 @@ _run_btrfs_util_prog filesystem sync $SCRATCH_MNT
 _scratch_unmount
 
 # generate a qgroup report and look for inconsistent groups
-$BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 | \
-	grep -E "Counts for qgroup.*are different"
+_btrfs_check_scratch_qgroup
+
 echo "Silence is golden"
 status=0
 
-- 
2.7.4

[PATCH 05/10] fstests: btrfs/042: Add extra qgroup verification

[Qu Wenruo]

Use newly introduced _btrfs_check_scratch_qgroup() to double check if
qgroup numbers are correct.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/btrfs/042 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/btrfs/042 b/tests/btrfs/042
index 498ccc9..003b7af 100755
--- a/tests/btrfs/042
+++ b/tests/btrfs/042
@@ -89,6 +89,9 @@ if [ $total_written -gt $LIMIT_SIZE ];then
 	_fail "total written should be less than $LIMIT_SIZE"
 fi
 
+_scratch_unmount
+_btrfs_check_scratch_qgroup
+
 # success, all done
 echo "Silence is golden"
 status=0
-- 
2.7.4

[PATCH 06/10] fstests: btrfs/091: Use _btrfs_check_scratch_qgroup other than fixed golden output

[Qu Wenruo]

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/btrfs/091     | 5 ++---
 tests/btrfs/091.out | 4 ----
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/tests/btrfs/091 b/tests/btrfs/091
index e3c43c7..ab4ff4e 100755
--- a/tests/btrfs/091
+++ b/tests/btrfs/091
@@ -94,9 +94,8 @@ cp --reflink $SCRATCH_MNT/subv1/file1 $SCRATCH_MNT/subv3/file1
 # in commit tree. So need to sync to update the qgroup commit tree.
 sync
 
-units=`_btrfs_qgroup_units`
-$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $SED_PROG -n '/[0-9]/p' | \
-	$AWK_PROG '{print $2" "$3}'
+_scratch_unmount
+_btrfs_check_scratch_qgroup
 
 # success, all done
 status=0
diff --git a/tests/btrfs/091.out b/tests/btrfs/091.out
index 68fe2df..f2981b5 100644
--- a/tests/btrfs/091.out
+++ b/tests/btrfs/091.out
@@ -1,7 +1,3 @@
 QA output created by 091
 wrote 262144/262144 bytes at offset 0
 XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-65536 65536
-327680 65536
-327680 65536
-327680 65536
-- 
2.7.4

[PATCH 07/10] fstests: btrfs/099: Add extra verification for qgroup

[Qu Wenruo]

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/btrfs/099 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/btrfs/099 b/tests/btrfs/099
index 70f07b5..9cc9a3d 100755
--- a/tests/btrfs/099
+++ b/tests/btrfs/099
@@ -82,6 +82,9 @@ sync
 $XFS_IO_PROG -f -c "pwrite -b $BLOCKSIZE 0 $(($FILESIZE - $BLOCKSIZE))" \
 	$SCRATCH_MNT/foo | _filter_xfs_io
 
+_scratch_unmount
+_btrfs_check_scratch_qgroup
+
 # success, all done
 status=0
 exit
-- 
2.7.4

[PATCH 08/10] fstests: btrfs/104: Use _btrfs_check_scratch_qgroup to replace open codes

[Qu Wenruo]

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/btrfs/104 | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/tests/btrfs/104 b/tests/btrfs/104
index 6afaa02..a5d2070 100755
--- a/tests/btrfs/104
+++ b/tests/btrfs/104
@@ -152,14 +152,7 @@ _run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap1
 sleep 45
 
 _scratch_unmount
-
-# generate a qgroup report and look for inconsistent groups
-#  - don't use _run_btrfs_util_prog here as it captures the output and
-#    we need to grep it.
-$BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 | \
-			grep -E -q "Counts for qgroup.*are different"
-if [ $? -ne 0 ]; then
-	status=0
-fi
+_btrfs_check_scratch_qgroup
+status=0
 
 exit
-- 
2.7.4

[PATCH 09/10] fstests: btrfs/122: Use _btrfs_check_scratch_qgroup to replace open code

[Qu Wenruo]

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/btrfs/122 | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/tests/btrfs/122 b/tests/btrfs/122
index 82252ab..899ede5 100755
--- a/tests/btrfs/122
+++ b/tests/btrfs/122
@@ -77,12 +77,7 @@ _run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT "$SCRATCH_MNT/snaps/snap1"
 _run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT "$SCRATCH_MNT/snaps/snap2"
 
 _scratch_unmount
-
-# generate a qgroup report and look for inconsistent groups
-$BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 | \
-			grep -q -E "Counts for qgroup.*are different"
-if [ $? -ne 0 ]; then
-	status=0
-fi
+_btrfs_check_scratch_qgroup
+status=0
 
 exit
-- 
2.7.4

[PATCH 10/10] fstests: btrfs/123: Use _btrfs_check_scratch_qgroup to replace open code

[Qu Wenruo]

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/btrfs/123 | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/btrfs/123 b/tests/btrfs/123
index e89d541..36d24c4 100755
--- a/tests/btrfs/123
+++ b/tests/btrfs/123
@@ -76,9 +76,7 @@ _run_btrfs_util_prog quota rescan -w $SCRATCH_MNT
 _run_btrfs_util_prog balance start -d $SCRATCH_MNT
 
 _scratch_unmount
-# generate a qgroup report and look for inconsistent groups
-$BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 | \
-		grep -E "Counts for qgroup.*are different"
+_btrfs_check_scratch_qgroup
 
 # success, all done
 status=0
-- 
2.7.4

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

[Eryu Guan]

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

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

[Qu Wenruo]

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

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

[Dave Chinner]

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

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

[Qu Wenruo]

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

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

[Dave Chinner]

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