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

Message ID 20161129073303.14776-2-quwenruo@cn.fujitsu.com
State Not Applicable
Headers show

Commit Message

Qu Wenruo Nov. 29, 2016, 7:32 a.m. UTC
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(+)

Comments

Eryu Guan Nov. 29, 2016, 8:16 a.m. UTC | #1
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
Qu Wenruo Nov. 29, 2016, 8:44 a.m. UTC | #2
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
Dave Chinner Nov. 29, 2016, 9:01 p.m. UTC | #3
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.
Qu Wenruo Nov. 30, 2016, 12:56 a.m. UTC | #4
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
Dave Chinner Nov. 30, 2016, 1:23 a.m. UTC | #5
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.

Patch
diff mbox

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()
 {