Re: [PATCH] generic/448: don't disable extent zeroing if extent_max_zeroout_kb isn't supported

[Xiao Yang <yangx.jy@cn.fujitsu.com>]

On 2017/07/18 21:18, Eryu Guan wrote:
> On Tue, Jul 18, 2017 at 04:27:50PM +0800, Xiao Yang wrote:
>> On some old kernel(e.g. v3.5), this case fails because it can not create
>> extent_max_zeroout_kb file, as below:
>>    Silence is golden
>>   +./tests/generic/448: line 54: /sys/fs/ext4/sda5/extent_max_zeroout_kb: No such file or directory
>>    seek sanity check failed!
>>
>> The extent_max_zeroout_kb file is introduced by:
>>    67a5da564f97('ext4: make the zero-out chunk size tunable')
> This is available since v3.7-rc1 kernel
> $ git describe --contains 67a5da564f97
> v3.7-rc1~91^2~63
>
> But ext4 SEEK_DATA/SEEK_HOLE support was added in v3.8-rc1
> $ git describe --contains c8c0df241cc2
> v3.8-rc1~89^2~40
>
> IMO, you shouldn't hit this error at all, because test won't pass
> _require_seek_data_hole rule. (Unless you're testing some customized
> kernel with seek_data/hole backported but not that ext4 sysfs entry.)
Hi Eryu

Thanks for your explanation.

I test it in v3.5 kernel without seek_data/hole backported, please see 
the following message:
[root@localhost xfstests]# uname -r
3.5.0
[root@localhost xfstests]# src/seek_sanity_test -t 
/mnt/xfstests/test/testfile 2>&1
File system magic#: 0xef53
Allocation size: 4096
File system supports the default behavior.
File system does not support unwritten extents.

_require_seek_data_hole could not catch "File system does not support" 
if ext4 SEEK_DATA/SEEK_HOLE
support was not added.

I think we should update  _require_seek_data_hole to catch both "Kernel 
does not support" and
"File system does not support".   This case could be skipped when 
meetting either of them.

Thanks,
Xiao Yang
>> We should only disable extent zeroing when extent_max_zeroout_kb is supported.
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   tests/generic/448 | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/generic/448 b/tests/generic/448
>> index 87b99d7..3e92742 100755
>> --- a/tests/generic/448
>> +++ b/tests/generic/448
>> @@ -51,7 +51,8 @@ _require_test_program "seek_sanity_test"
>>   # Disable extent zeroing for ext4 as that change where holes are created
>>   if [ "$FSTYP" = "ext4" ]; then
>>   	DEV=`_short_dev $TEST_DEV`
>> -	echo 0>/sys/fs/ext4/$DEV/extent_max_zeroout_kb
>> +	[ -f /sys/fs/ext4/$DEV/extent_max_zeroout_kb ] \
>> +	&&  echo 0>/sys/fs/ext4/$DEV/extent_max_zeroout_kb
> But I'm OK with adding this check, but not in this test only, this
> pattern is repeated in several seek_data/hole tests, e.g.
> generic/285, generic/436, generic/445 generic/448, and generic/009
> (which is not a seek_data/hole test).
>
> So I'm wondering if this hunk of code can be made into a helper and
> embed it into _require_seek_data_hole. e.g. (completely untested)
>
> # Disable extent zeroing for ext4 on the given device
> _ext4_disable_extent_zeroout()
> {
> 	local dev=${1:-$TEST_DEV}
> 	local sdev=`_short_dev $dev`
>
> 	[ -f /sys/fs/ext4/$sdev/extent_max_zeroout_kb ] \
> 	&&  echo 0>/sys/fs/ext4/$sdev/extent_max_zeroout_kb
> }
>
> And _require_seek_data_hole
>
> _require_seek_data_hole()
> {
> 	local dev=${1:-$TEST_DEV}
> 	local testfile=$TEST_DIR/$$.seek
>
> 	testseek=`$here/src/seek_sanity_test -t $testfile 2>&1`
> 	rm -f $testfile&>/dev/null
> 	echo $testseek | grep -q "Kernel does not support"&&  \
> 	    _notrun "File system does not support llseek(2) SEEK_DATA/HOLE"
> 	# Disable extent zeroing for ext4 as that change where holes are
> 	# created
> 	if [ "$FSTYP" == "ext4" ]; then
> 		_ext4_disable_extent_zeroout $dev
> 	fi
> }
>
> And update generic/009 accordingly to use this new helper.
>
> What do you and/or other people think?
>
> Thanks,
> Eryu
>
>>   fi
>>
>>   $here/src/seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE>  $seqres.full 2>&1 ||
>> -- 
>> 1.8.3.1
>>
>>
>>
>> --
>> 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
>
> .
>