Re: [PATCH v2] generic/508: fix to check inode creation time feature on scratch mountpoint

[Chao Yu <yuchao0@huawei.com>]

On 2018/10/25 10:16, Dave Chinner wrote:
> On Wed, Oct 24, 2018 at 03:24:33PM +0800, Chao Yu wrote:
>> _require_btime() just check inode creation time feature on TEST_DIR
>> mountpoint, but generic/508 needs to do that check on SCRATCH_MNT
>> mountpoint. Let's add _require_scratch_btime() for that, meanwhile
>> moving the check behind scratch_mkfs/scratch_moun.
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> v2:
>> As Dave Chinner suggested:
>> - introduce _require_scratch_btime() to check inode creation time
>> feature in scratch mountpoint, adjust generic/508 to use it.
>> - relocate the check behind scratch_mkfs/scratch_mount.
>>  common/rc         | 8 ++++++++
>>  tests/generic/508 | 2 +-
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/rc b/common/rc
>> index b4987a9cd7f7..111ba5410506 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -3851,6 +3851,14 @@ _require_btime()
>>  	rm -f $TEST_DIR/test_creation_time
>>  }
>>  
>> +_require_scratch_btime()
>> +{
>> +	$XFS_IO_PROG -f $SCRATCH_MNT/test_creation_time -c "statx -v" \
>> +		| grep btime >>$seqres.full 2>&1 || \
>> +		_notrun "inode creation time not supported by this filesystem"
>> +	rm -f $SCRATCH_MNT/test_creation_time
>> +}
>> +
>>  init_rc
>>  
>>  ################################################################################
>> diff --git a/tests/generic/508 b/tests/generic/508
>> index b869b3a9c260..f1cda52fa44c 100755
>> --- a/tests/generic/508
>> +++ b/tests/generic/508
>> @@ -46,7 +46,6 @@ _supported_os Linux
>>  _require_test_lsattr
>>  _require_statx
>>  _require_xfs_io_command "statx" "-v"
>> -_require_btime
>>  
>>  _require_scratch
>>  _require_scratch_shutdown
>> @@ -59,6 +58,7 @@ testfile=$SCRATCH_MNT/testfile
>>  do_check()
>>  {
>>  	_scratch_mount
>> +	_require_scratch_btime
> 
> There is no need to check it every time the scratch device is
> mounted. Do the require checks up front befor the test starts
> after _scratch_mkfs has been run.

Oh, sorry, actually, I should do this check with more clean way.

Taking other existing _require_* function which needs scratch device being
mounted as an example, it may look more clean that adding mkfs/mount/umount
into _require_scratch_btime instead of coupling codes with main flow. Like:

_require_scratch_btime()
{
	_require_scratch
	_scratch_mkfs > /dev/null 2>&1
	_scratch_mount

	$XFS_IO_PROG -f $SCRATCH_MNT/test_creation_time -c "statx -v" \
		| grep btime >>$seqres.full 2>&1 || \
		_notrun "inode creation time not supported by this filesystem"
	rm -f $SCRATCH_MNT/test_creation_time

	_scratch_unmount
}

How do you think of this?

Thanks,

> 
> But, oh, what an inconsistent mess these scratch device require
> statements are.  Some just check the scratch device. Some mkfs the
> scratch device, mount it and then run tests. Others require that the
> scratch device is already made and mounted. It doesn't look like
> there's any consistency here, so it's no wonder test writers are
> getting this stuff wrong....
> 
> Cheers,
> 
> Dave.
>