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

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

[Chao Yu]

_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
 
 	touch $testfile
 
-- 
2.18.0.rc1

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

[Dave Chinner]

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.

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

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

[Chao Yu]

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