[PATCH v2 0/3] fstests: Fix order of _require_scratch* and _require_dm_target

[PATCH v2 0/3] fstests: Fix order of _require_scratch* and _require_dm_target

[Shin'ichiro Kawasaki]

Test cases with dm_target shall call _require_scratch* before
_require_dm_target. However, three test cases generic/628, generic/629 and
btrfs/146 do not follow this rule and result in unexpected failures. The first
patch in this series fixes the failures in generic/628 and generic/629. The
second patch fixes it in btrfs/146. The last patch improves _require_dm_target
to prevent the failure in the future test cases to be added.

The failure was found with generic/628 and generic/629. I checked all existing
test cases and found btrfs/146 also has the issue.

Changes from v1:
* Changed fix approach according to comment on the list
* Added fix for btrfs/146 as the 2nd patch
* Separated improvement in _require_dm_target as the last patch

Shin'ichiro Kawasaki (3):
  generic/{628,629}: Fix order of _require_scratch* and
    _require_dm_target
  btrfs/146: Add _require_scratch_dev_pool before _require_dm_target
  common/rc: Check call order of _require_dm_target and
    _require_scratch*

 common/rc         | 3 +++
 tests/btrfs/146   | 1 +
 tests/generic/628 | 2 +-
 tests/generic/629 | 2 +-
 4 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] generic/{628,629}: Fix order of _require_scratch* and _require_dm_target

[Shin'ichiro Kawasaki]

Test cases with dm_target shall call _require_scratch* before
_require_dm_target to ensure that valid SCRATCH_DEV is available for
dm_target. However, the test cases generic/628 and generic/629 call
_require_dm_target before _require_scratch*, then unexpected failure was
reported when SCRATCH_DEV is not specified. Fix the order of function
calls.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/generic/628 | 2 +-
 tests/generic/629 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/generic/628 b/tests/generic/628
index 2ccbbbdc..7dc6dfcd 100755
--- a/tests/generic/628
+++ b/tests/generic/628
@@ -29,8 +29,8 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs generic
-_require_dm_target error
 _require_scratch_reflink
+_require_dm_target error
 _require_xfs_io_command "chattr" "s"
 _require_cp_reflink
 
diff --git a/tests/generic/629 b/tests/generic/629
index abfa90ab..f501555e 100755
--- a/tests/generic/629
+++ b/tests/generic/629
@@ -28,10 +28,10 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs generic
+_require_scratch
 _require_dm_target error
 _require_xfs_io_command "chattr" "s"
 _require_xfs_io_command "copy_range"
-_require_scratch
 
 # Format filesystem and set up quota limits
 _scratch_mkfs > $seqres.full
-- 
2.31.1

[PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target

[Shin'ichiro Kawasaki]

The test case btrfs/146 calls _require_dm_target which depends on
SCRATCH_DEV. The test case assumes that valid devices are set in
SCRATCH_DEV_POOL, and one of the devices is propagated to SCRATCH_DEV.
However, when SCRATCH_DEV_POOL is not set, valid value is not propagated
to SCRATCH_DEV and _require_dm_target causes unexpected test case
failure. To avoid the failure, add _require_scratch_dev_pool call before
_require_dm_target call to detect invalid SCRATCH_DEV_POOL beforehand.

Of note is that the test case replaces SCRATCH_DEV_POOL value internally
and calls _require_scratch_dev_pool for the replaced SCRATCH_DEV_POOL.
With this fix, the test case will call _require_scratch_dev_pool twice
for the original SCRATCH_DEV_POOL and the replaced SCRATCH_DEV_POOL.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/btrfs/146 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/btrfs/146 b/tests/btrfs/146
index 64c3513f..eee1a286 100755
--- a/tests/btrfs/146
+++ b/tests/btrfs/146
@@ -26,6 +26,7 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs btrfs
+_require_scratch_dev_pool
 _require_dm_target error
 _require_test_program fsync-err
 _require_test_program dmerror
-- 
2.31.1

[PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch*

[Shin'ichiro Kawasaki]

When SCRATCH_DEV is not set and the test case does not call
_require_scratch* before _require_dm_target, _require_block_device
called from _require_dm_target fails to evaluate SCRATCH_DEV and
results in the test case failure. This failure reason is not described
in the error message and it takes some time to catch.

To catch the failure reason easier, check SCRATCH_DEV in
_require_dm_target. If SCRATCH_DEV is not set, fail the test case
and print message which requests to fix call order of _require_scratch*
and _require_dm_target. This improvement follows what _scratch_shutdown
does for _require_scratch_shutdown.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 common/rc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/rc b/common/rc
index dda5da06..cbec8aaa 100644
--- a/common/rc
+++ b/common/rc
@@ -1971,6 +1971,9 @@ _require_dm_target()
 
 	# require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF
 	# behaviour
+	if [ -z "$SCRATCH_DEV" ]; then
+		_fail "_require_dm_target: call _require_scratch* first in test"
+	fi
 	_require_block_device $SCRATCH_DEV
 	_require_sane_bdev_flush $SCRATCH_DEV
 	_require_command "$DMSETUP_PROG" dmsetup
-- 
2.31.1

Re: [PATCH v2 1/3] generic/{628,629}: Fix order of _require_scratch* and _require_dm_target

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch*

[Dave Chinner]

On Wed, Sep 08, 2021 at 05:37:15PM +0900, Shin'ichiro Kawasaki wrote:
> When SCRATCH_DEV is not set and the test case does not call
> _require_scratch* before _require_dm_target, _require_block_device
> called from _require_dm_target fails to evaluate SCRATCH_DEV and
> results in the test case failure. This failure reason is not described
> in the error message and it takes some time to catch.

You should quote the actual failure message here so we have some
idea of whether the message that was emitted was appropriate or not
without having to go know how the test failed...

> To catch the failure reason easier, check SCRATCH_DEV in
> _require_dm_target. If SCRATCH_DEV is not set, fail the test case
> and print message which requests to fix call order of _require_scratch*
> and _require_dm_target. This improvement follows what _scratch_shutdown
> does for _require_scratch_shutdown.

Also, you don't need to describe the change in the commit message -
the patch does that. The first paragraph is all that is needed here
as it describes why you want to make the change.

> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  common/rc | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index dda5da06..cbec8aaa 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1971,6 +1971,9 @@ _require_dm_target()
>  
>  	# require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF
>  	# behaviour
> +	if [ -z "$SCRATCH_DEV" ]; then
> +		_fail "_require_dm_target: call _require_scratch* first in test"
> +	fi
>  	_require_block_device $SCRATCH_DEV
>  	_require_sane_bdev_flush $SCRATCH_DEV
>  	_require_command "$DMSETUP_PROG" dmsetup

That's a notrun case, not a fail.

Also, we report the error that has occurred, not how to resolve the
problem. That's because we might change behaviour in future and now
the error message tells people to do something that is
wrong/non-existent. As such, I think the premise this change is based
on is not really valid - people running fstests are assumed to have
a level of knowledge sufficient to trace a failing test and
determine what went wrong from the error reported. i.e. the error
message should state what the problem was, not describe a potential
solution.

Also, this is not the place to check if SCRATCH_DEV is set. The
check for a NULL device should be in _require_block_device(). Oh,
wait, it already is:

_require_block_device()
{
	if [ -z "$1" ]; then
		echo "Usage: _require_block_device <dev>" 1>&2
		exit 1
	fi
....
}

And that's the error message the test emitted that you didn't
understand, right?

If so, the change here should really be to _require_block_device().
i.e.

	if [ -z "$1" ]; then
		_notrun "test requires a block device to be specified"
	fi

A quick scan shows a bunch of similar _requires checks that do
similar things with poor error messages and 'exit 1' (e.g.
_require_local_device()). _requires rules should call _notrun if the
test should not run because of incorrect setup, not 'exit 1'.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch*

[Shinichiro Kawasaki]

On Sep 10, 2021 / 10:48, Dave Chinner wrote:
> On Wed, Sep 08, 2021 at 05:37:15PM +0900, Shin'ichiro Kawasaki wrote:
> > When SCRATCH_DEV is not set and the test case does not call
> > _require_scratch* before _require_dm_target, _require_block_device
> > called from _require_dm_target fails to evaluate SCRATCH_DEV and
> > results in the test case failure. This failure reason is not described
> > in the error message and it takes some time to catch.
> 
> You should quote the actual failure message here so we have some
> idea of whether the message that was emitted was appropriate or not
> without having to go know how the test failed...

Sorry about the lack of the infomration. As you found below, the meesage was
"Usage: _require_block_device <dev>".

> 
> > To catch the failure reason easier, check SCRATCH_DEV in
> > _require_dm_target. If SCRATCH_DEV is not set, fail the test case
> > and print message which requests to fix call order of _require_scratch*
> > and _require_dm_target. This improvement follows what _scratch_shutdown
> > does for _require_scratch_shutdown.
> 
> Also, you don't need to describe the change in the commit message -
> the patch does that. The first paragraph is all that is needed here
> as it describes why you want to make the change.

I see. I will write "why" in the commit message, not "what". (In the past, I
was advised to write "what" the patch does, but I think this guide is valid
only when the change is complicated).

> 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >  common/rc | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/common/rc b/common/rc
> > index dda5da06..cbec8aaa 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -1971,6 +1971,9 @@ _require_dm_target()
> >  
> >  	# require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF
> >  	# behaviour
> > +	if [ -z "$SCRATCH_DEV" ]; then
> > +		_fail "_require_dm_target: call _require_scratch* first in test"
> > +	fi
> >  	_require_block_device $SCRATCH_DEV
> >  	_require_sane_bdev_flush $SCRATCH_DEV
> >  	_require_command "$DMSETUP_PROG" dmsetup
> 
> That's a notrun case, not a fail.
> 
> Also, we report the error that has occurred, not how to resolve the
> problem. That's because we might change behaviour in future and now
> the error message tells people to do something that is
> wrong/non-existent. As such, I think the premise this change is based
> on is not really valid - people running fstests are assumed to have
> a level of knowledge sufficient to trace a failing test and
> determine what went wrong from the error reported. i.e. the error
> message should state what the problem was, not describe a potential
> solution.

Thank you for the comment. These are the points I missed. At least I was
able to catch the cause, so the improvement I suggested is not a big
improvement.

> 
> Also, this is not the place to check if SCRATCH_DEV is set. The
> check for a NULL device should be in _require_block_device(). Oh,
> wait, it already is:
> 
> _require_block_device()
> {
> 	if [ -z "$1" ]; then
> 		echo "Usage: _require_block_device <dev>" 1>&2
> 		exit 1
> 	fi
> ....
> }
> 
> And that's the error message the test emitted that you didn't
> understand, right?

Right :)

> 
> If so, the change here should really be to _require_block_device().
> i.e.
> 
> 	if [ -z "$1" ]; then
> 		_notrun "test requires a block device to be specified"
> 	fi
> 
> A quick scan shows a bunch of similar _requires checks that do
> similar things with poor error messages and 'exit 1' (e.g.
> _require_local_device()). _requires rules should call _notrun if the
> test should not run because of incorrect setup, not 'exit 1'.

Thank you for your thoughts. I walked through _require_* bash functions in
common/, and listed 20 functions below, which call 'exit 1', _fail, or
'return 1' for its argument check failure:

--- list start ---

common/rc

  _require_scratch_size
  _require_scratch_size_nocheck
  _require_command *
  _require_block_device *
  _require_local_device *
  _require_zoned_device *
  _require_non_zoned_device *
  _require_scratch_ext4_feature
  _require_xfs_io_command
  _require_fio
  _require_batched_discard *
  _require_chattr
  _require_fs_sysfs
  _require_scratch_feature

common/btrfs

  _require_btrfs_mkfs_feature
  _require_btrfs_fs_feature

common/xfs

  _require_xfs_db_command
  _require_xfs_spaceman_command

common/encrypt

  _require_encryption_policy_support (checks arguments passed from _require_scratch_encryption)

common/rnameat2

  _require_renameat2

--- list end ---

Many of the functions above check arguments not for incorrect setup, but for
call in test cases with invalid arguments. 6 functions of them with * in the
list check arguments for the incorrect setups, such as DEBUGFS_PROG,
SCRATCH_DEV or SCRATCH_MNT. So I suggest to modify these functions to improve
error messages and call "_notrun". What do you think about this?

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch*

[Eryu Guan]

On Fri, Sep 10, 2021 at 06:34:05AM +0000, Shinichiro Kawasaki wrote:
> On Sep 10, 2021 / 10:48, Dave Chinner wrote:
> > On Wed, Sep 08, 2021 at 05:37:15PM +0900, Shin'ichiro Kawasaki wrote:
> > > When SCRATCH_DEV is not set and the test case does not call
> > > _require_scratch* before _require_dm_target, _require_block_device
> > > called from _require_dm_target fails to evaluate SCRATCH_DEV and
> > > results in the test case failure. This failure reason is not described
> > > in the error message and it takes some time to catch.
> > 
> > You should quote the actual failure message here so we have some
> > idea of whether the message that was emitted was appropriate or not
> > without having to go know how the test failed...
> 
> Sorry about the lack of the infomration. As you found below, the meesage was
> "Usage: _require_block_device <dev>".
> 
> > 
> > > To catch the failure reason easier, check SCRATCH_DEV in
> > > _require_dm_target. If SCRATCH_DEV is not set, fail the test case
> > > and print message which requests to fix call order of _require_scratch*
> > > and _require_dm_target. This improvement follows what _scratch_shutdown
> > > does for _require_scratch_shutdown.
> > 
> > Also, you don't need to describe the change in the commit message -
> > the patch does that. The first paragraph is all that is needed here
> > as it describes why you want to make the change.
> 
> I see. I will write "why" in the commit message, not "what". (In the past, I
> was advised to write "what" the patch does, but I think this guide is valid
> only when the change is complicated).
> 
> > 
> > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > ---
> > >  common/rc | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index dda5da06..cbec8aaa 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -1971,6 +1971,9 @@ _require_dm_target()
> > >  
> > >  	# require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF
> > >  	# behaviour
> > > +	if [ -z "$SCRATCH_DEV" ]; then
> > > +		_fail "_require_dm_target: call _require_scratch* first in test"
> > > +	fi
> > >  	_require_block_device $SCRATCH_DEV
> > >  	_require_sane_bdev_flush $SCRATCH_DEV
> > >  	_require_command "$DMSETUP_PROG" dmsetup
> > 
> > That's a notrun case, not a fail.
> > 
> > Also, we report the error that has occurred, not how to resolve the
> > problem. That's because we might change behaviour in future and now
> > the error message tells people to do something that is
> > wrong/non-existent. As such, I think the premise this change is based
> > on is not really valid - people running fstests are assumed to have
> > a level of knowledge sufficient to trace a failing test and
> > determine what went wrong from the error reported. i.e. the error
> > message should state what the problem was, not describe a potential
> > solution.
> 
> Thank you for the comment. These are the points I missed. At least I was
> able to catch the cause, so the improvement I suggested is not a big
> improvement.
> 
> > 
> > Also, this is not the place to check if SCRATCH_DEV is set. The
> > check for a NULL device should be in _require_block_device(). Oh,
> > wait, it already is:
> > 
> > _require_block_device()
> > {
> > 	if [ -z "$1" ]; then
> > 		echo "Usage: _require_block_device <dev>" 1>&2
> > 		exit 1
> > 	fi
> > ....
> > }
> > 
> > And that's the error message the test emitted that you didn't
> > understand, right?
> 
> Right :)
> 
> > 
> > If so, the change here should really be to _require_block_device().
> > i.e.
> > 
> > 	if [ -z "$1" ]; then
> > 		_notrun "test requires a block device to be specified"
> > 	fi
> > 
> > A quick scan shows a bunch of similar _requires checks that do
> > similar things with poor error messages and 'exit 1' (e.g.
> > _require_local_device()). _requires rules should call _notrun if the
> > test should not run because of incorrect setup, not 'exit 1'.
> 
> Thank you for your thoughts. I walked through _require_* bash functions in
> common/, and listed 20 functions below, which call 'exit 1', _fail, or
> 'return 1' for its argument check failure:
> 
> --- list start ---
> 
> common/rc
> 
>   _require_scratch_size
>   _require_scratch_size_nocheck
>   _require_command *
>   _require_block_device *
>   _require_local_device *
>   _require_zoned_device *
>   _require_non_zoned_device *
>   _require_scratch_ext4_feature
>   _require_xfs_io_command
>   _require_fio
>   _require_batched_discard *
>   _require_chattr
>   _require_fs_sysfs
>   _require_scratch_feature
> 
> common/btrfs
> 
>   _require_btrfs_mkfs_feature
>   _require_btrfs_fs_feature
> 
> common/xfs
> 
>   _require_xfs_db_command
>   _require_xfs_spaceman_command
> 
> common/encrypt
> 
>   _require_encryption_policy_support (checks arguments passed from _require_scratch_encryption)
> 
> common/rnameat2
> 
>   _require_renameat2
> 
> --- list end ---
> 
> Many of the functions above check arguments not for incorrect setup, but for
> call in test cases with invalid arguments. 6 functions of them with * in the
> list check arguments for the incorrect setups, such as DEBUGFS_PROG,
> SCRATCH_DEV or SCRATCH_MNT. So I suggest to modify these functions to improve
> error messages and call "_notrun". What do you think about this?

IMO the _fail calls in above _require* rules are indicating function
usage errors, which are bugs in the test code. While _notrun indicates a
required condition is not met for this test.

Thanks,
Eryu

P.S. I've applied the first two patches, thanks for the fix!

> 
> -- 
> Best Regards,
> Shin'ichiro Kawasaki

Re: [PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target

[Eryu Guan]

On Wed, Sep 08, 2021 at 05:37:14PM +0900, Shin'ichiro Kawasaki wrote:
> The test case btrfs/146 calls _require_dm_target which depends on
> SCRATCH_DEV. The test case assumes that valid devices are set in
> SCRATCH_DEV_POOL, and one of the devices is propagated to SCRATCH_DEV.
> However, when SCRATCH_DEV_POOL is not set, valid value is not propagated
> to SCRATCH_DEV and _require_dm_target causes unexpected test case
> failure. To avoid the failure, add _require_scratch_dev_pool call before
> _require_dm_target call to detect invalid SCRATCH_DEV_POOL beforehand.
> 
> Of note is that the test case replaces SCRATCH_DEV_POOL value internally
> and calls _require_scratch_dev_pool for the replaced SCRATCH_DEV_POOL.
> With this fix, the test case will call _require_scratch_dev_pool twice
> for the original SCRATCH_DEV_POOL and the replaced SCRATCH_DEV_POOL.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  tests/btrfs/146 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/btrfs/146 b/tests/btrfs/146
> index 64c3513f..eee1a286 100755
> --- a/tests/btrfs/146
> +++ b/tests/btrfs/146
> @@ -26,6 +26,7 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs btrfs
> +_require_scratch_dev_pool

There's no need for the test to call _require_scratch_dev_pool twice, it
doesn't do any setup work, just to make sure there's scratch pool
defined. Move both _require_scratch and _require_scratch_dev_pool up
here should be fine. I've fixed it on commit.

Thanks,
Eryu

>  _require_dm_target error
>  _require_test_program fsync-err
>  _require_test_program dmerror
> -- 
> 2.31.1

Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch*

[Shinichiro Kawasaki]

On Sep 12, 2021 / 17:17, Eryu Guan wrote:
> On Fri, Sep 10, 2021 at 06:34:05AM +0000, Shinichiro Kawasaki wrote:
> > On Sep 10, 2021 / 10:48, Dave Chinner wrote:
> > > On Wed, Sep 08, 2021 at 05:37:15PM +0900, Shin'ichiro Kawasaki wrote:
> > > > When SCRATCH_DEV is not set and the test case does not call
> > > > _require_scratch* before _require_dm_target, _require_block_device
> > > > called from _require_dm_target fails to evaluate SCRATCH_DEV and
> > > > results in the test case failure. This failure reason is not described
> > > > in the error message and it takes some time to catch.
> > > 
> > > You should quote the actual failure message here so we have some
> > > idea of whether the message that was emitted was appropriate or not
> > > without having to go know how the test failed...
> > 
> > Sorry about the lack of the infomration. As you found below, the meesage was
> > "Usage: _require_block_device <dev>".
> > 
> > > 
> > > > To catch the failure reason easier, check SCRATCH_DEV in
> > > > _require_dm_target. If SCRATCH_DEV is not set, fail the test case
> > > > and print message which requests to fix call order of _require_scratch*
> > > > and _require_dm_target. This improvement follows what _scratch_shutdown
> > > > does for _require_scratch_shutdown.
> > > 
> > > Also, you don't need to describe the change in the commit message -
> > > the patch does that. The first paragraph is all that is needed here
> > > as it describes why you want to make the change.
> > 
> > I see. I will write "why" in the commit message, not "what". (In the past, I
> > was advised to write "what" the patch does, but I think this guide is valid
> > only when the change is complicated).
> > 
> > > 
> > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > ---
> > > >  common/rc | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/common/rc b/common/rc
> > > > index dda5da06..cbec8aaa 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -1971,6 +1971,9 @@ _require_dm_target()
> > > >  
> > > >  	# require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF
> > > >  	# behaviour
> > > > +	if [ -z "$SCRATCH_DEV" ]; then
> > > > +		_fail "_require_dm_target: call _require_scratch* first in test"
> > > > +	fi
> > > >  	_require_block_device $SCRATCH_DEV
> > > >  	_require_sane_bdev_flush $SCRATCH_DEV
> > > >  	_require_command "$DMSETUP_PROG" dmsetup
> > > 
> > > That's a notrun case, not a fail.
> > > 
> > > Also, we report the error that has occurred, not how to resolve the
> > > problem. That's because we might change behaviour in future and now
> > > the error message tells people to do something that is
> > > wrong/non-existent. As such, I think the premise this change is based
> > > on is not really valid - people running fstests are assumed to have
> > > a level of knowledge sufficient to trace a failing test and
> > > determine what went wrong from the error reported. i.e. the error
> > > message should state what the problem was, not describe a potential
> > > solution.
> > 
> > Thank you for the comment. These are the points I missed. At least I was
> > able to catch the cause, so the improvement I suggested is not a big
> > improvement.
> > 
> > > 
> > > Also, this is not the place to check if SCRATCH_DEV is set. The
> > > check for a NULL device should be in _require_block_device(). Oh,
> > > wait, it already is:
> > > 
> > > _require_block_device()
> > > {
> > > 	if [ -z "$1" ]; then
> > > 		echo "Usage: _require_block_device <dev>" 1>&2
> > > 		exit 1
> > > 	fi
> > > ....
> > > }
> > > 
> > > And that's the error message the test emitted that you didn't
> > > understand, right?
> > 
> > Right :)
> > 
> > > 
> > > If so, the change here should really be to _require_block_device().
> > > i.e.
> > > 
> > > 	if [ -z "$1" ]; then
> > > 		_notrun "test requires a block device to be specified"
> > > 	fi
> > > 
> > > A quick scan shows a bunch of similar _requires checks that do
> > > similar things with poor error messages and 'exit 1' (e.g.
> > > _require_local_device()). _requires rules should call _notrun if the
> > > test should not run because of incorrect setup, not 'exit 1'.
> > 
> > Thank you for your thoughts. I walked through _require_* bash functions in
> > common/, and listed 20 functions below, which call 'exit 1', _fail, or
> > 'return 1' for its argument check failure:
> > 
> > --- list start ---
> > 
> > common/rc
> > 
> >   _require_scratch_size
> >   _require_scratch_size_nocheck
> >   _require_command *
> >   _require_block_device *
> >   _require_local_device *
> >   _require_zoned_device *
> >   _require_non_zoned_device *
> >   _require_scratch_ext4_feature
> >   _require_xfs_io_command
> >   _require_fio
> >   _require_batched_discard *
> >   _require_chattr
> >   _require_fs_sysfs
> >   _require_scratch_feature
> > 
> > common/btrfs
> > 
> >   _require_btrfs_mkfs_feature
> >   _require_btrfs_fs_feature
> > 
> > common/xfs
> > 
> >   _require_xfs_db_command
> >   _require_xfs_spaceman_command
> > 
> > common/encrypt
> > 
> >   _require_encryption_policy_support (checks arguments passed from _require_scratch_encryption)
> > 
> > common/rnameat2
> > 
> >   _require_renameat2
> > 
> > --- list end ---
> > 
> > Many of the functions above check arguments not for incorrect setup, but for
> > call in test cases with invalid arguments. 6 functions of them with * in the
> > list check arguments for the incorrect setups, such as DEBUGFS_PROG,
> > SCRATCH_DEV or SCRATCH_MNT. So I suggest to modify these functions to improve
> > error messages and call "_notrun". What do you think about this?
> 
> IMO the _fail calls in above _require* rules are indicating function
> usage errors, which are bugs in the test code. While _notrun indicates a
> required condition is not met for this test.

I see. I think the _require* rules with "exit 1" also indicates the usage errors
and the bugs. As Dave pointed out, it is assumed the fstests users have enough
skill to identify the bug, then this improvement I suggested don't have much
value. I withdraw this suggestion. Dave and Eryu, thank you for the comments.

> 
> Thanks,
> Eryu
> 
> P.S. I've applied the first two patches, thanks for the fix!

Thanks!

-- 
Best Regards,
Shin'ichiro Kawasaki