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

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

[Xiao Yang]

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')

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
 fi
 
 $here/src/seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE > $seqres.full 2>&1 ||
-- 
1.8.3.1

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

[Eryu Guan]

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

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

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

[Xiao Yang]

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

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

[Eryu Guan]

On Wed, Jul 19, 2017 at 10:35:01AM +0800, Xiao Yang wrote:
> 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.

OK, I see why the test was *not* _notrun on 3.5 kernel now (without
seek_data/hole support added to ext4, but with SEEK_DATA/SEEK_HOLE flags
recognized by vfs, which was introduced by 982d81658 in v3.1-rc1),
because ext4 had the "default behavior" support, and that means "just
return i_size for SEEK_HOLE and will return the same offset for
SEEK_DATA".

> File system does not support unwritten extents.

This means the file system doesn't treat unwritten extents as a hole,
but data, which has nothing to do with SEEK_DATA/HOLE support status.

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

So this is wrong, IMO.

Thanks,
Eryu

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

[Xiao Yang]

On 2017/07/19 17:56, Eryu Guan wrote:
> On Wed, Jul 19, 2017 at 10:35:01AM +0800, Xiao Yang wrote:
>> 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.
> OK, I see why the test was *not* _notrun on 3.5 kernel now (without
> seek_data/hole support added to ext4, but with SEEK_DATA/SEEK_HOLE flags
> recognized by vfs, which was introduced by 982d81658 in v3.1-rc1),
> because ext4 had the "default behavior" support, and that means "just
> return i_size for SEEK_HOLE and will return the same offset for
> SEEK_DATA".
>
>> File system does not support unwritten extents.
> This means the file system doesn't treat unwritten extents as a hole,
> but data, which has nothing to do with SEEK_DATA/HOLE support status.
Hi Eryu,

Thanks for your detailed explanation, i got it.

Could you tell me whether generic/448 should be fixed or not?

Thanks,
Xiao Yang
>> _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.
> So this is wrong, IMO.
>
> Thanks,
> Eryu
>
>
> .
>

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

[Eryu Guan]

On Wed, Jul 19, 2017 at 06:38:01PM +0800, Xiao Yang wrote:
> On 2017/07/19 17:56, Eryu Guan wrote:
> > On Wed, Jul 19, 2017 at 10:35:01AM +0800, Xiao Yang wrote:
> > > 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.
> > OK, I see why the test was *not* _notrun on 3.5 kernel now (without
> > seek_data/hole support added to ext4, but with SEEK_DATA/SEEK_HOLE flags
> > recognized by vfs, which was introduced by 982d81658 in v3.1-rc1),
> > because ext4 had the "default behavior" support, and that means "just
> > return i_size for SEEK_HOLE and will return the same offset for
> > SEEK_DATA".
> > 
> > > File system does not support unwritten extents.
> > This means the file system doesn't treat unwritten extents as a hole,
> > but data, which has nothing to do with SEEK_DATA/HOLE support status.
> Hi Eryu,
> 
> Thanks for your detailed explanation, i got it.
> 
> Could you tell me whether generic/448 should be fixed or not?

IMHO, as the pure "sysfs entry not found" issue, it's really a low
priority bug to fix, as the bug only exists when vfs recognizes
SEEK_DATA/HOLE flag && ext4 has no extent zeroout tunable in sysfs, so
only 3.1-3.6 kernels suffer from this issue, but they're really old
kernels.

OTOH, factoring out a helper like what I suggested in my first reply
seems reasonable to me, so we don't repeat the same pattern in multiple
tests and we also get the bug fixed. If you send a patch to do this I'm
happy to review it :)

Thanks,
Eryu

[PATCH] common/rc: factor out _ext4_disable_extent_zeroout() helper

[Xiao Yang]

1) This pattern is repeated in several seek_data/hole tests
   (e.g. generic/285, generic/436, generic/445 generic/448)
   and generic/009.  A common _ext4_disable_extent_zeroout()
   helper could be added and applied by generic/009 and
   _require_seek_data_hole().

2) On some old kernels(e.g. v3.1-v3.6), when vfs recognizes
   SEEK_DATA/HOLE flag && ext4 has no extent zeroout tunable
   in sysfs, these cases may trigger "sysfs entry not found"
   issue.  We can add check if extent_max_zeroout_kb exists
   on ext4 filesystem.
   The extent_max_zeroout_kb is introduced by:
   '67a5da564f97 ("ext4: make the zero-out chunk size tunable")'

3) Declare several vars as local in _require_seek_data_hole().

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 common/rc         | 29 ++++++++++++++++++++++-------
 tests/generic/009 |  7 +------
 tests/generic/285 |  6 ------
 tests/generic/436 |  6 ------
 tests/generic/445 |  6 ------
 tests/generic/448 |  6 ------
 6 files changed, 23 insertions(+), 37 deletions(-)

diff --git a/common/rc b/common/rc
index bd989bb..b36a9bf 100644
--- a/common/rc
+++ b/common/rc
@@ -2304,16 +2304,31 @@ _require_fail_make_request()
  not found. Seems that CONFIG_FAIL_MAKE_REQUEST kernel config option not enabled"
 }
 
-#
+# Disable extent zeroing for ext4 on the given device
+_ext4_disable_extent_zeroout()
+{
+	local dev=${1:-$TEST_DEV}
+	local sdev=`_short_dev $dev`
+
+	if [ "$FSTYP" == "ext4" ]; then
+		[ -f /sys/fs/ext4/$sdev/extent_max_zeroout_kb ] && \
+			echo 0 >/sys/fs/ext4/$sdev/extent_max_zeroout_kb
+	fi
+}
+
 # Check if the file system supports seek_data/hole
-#
 _require_seek_data_hole()
 {
-    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"
+	local dev=${1:-$TEST_DEV}
+	local testfile=$TEST_DIR/$$.seek
+	local 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
+	_ext4_disable_extent_zeroout $dev
 }
 
 _require_runas()
diff --git a/tests/generic/009 b/tests/generic/009
index 5902afd..a7ca060 100755
--- a/tests/generic/009
+++ b/tests/generic/009
@@ -48,15 +48,10 @@ _require_xfs_io_command "fzero"
 _require_xfs_io_command "fiemap"
 _require_xfs_io_command "falloc"
 _require_test
+_ext4_disable_extent_zeroout
 
 testfile=$TEST_DIR/009.$$
 
-# 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
-fi
-
 # When PAGE_SIZE > 4096 xfs extent layout is different so it would not match
 # the output.
 if [ "$FSTYP" = "xfs" ]; then
diff --git a/tests/generic/285 b/tests/generic/285
index 16e70b1..3f7d298 100755
--- a/tests/generic/285
+++ b/tests/generic/285
@@ -47,12 +47,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
 
 _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
-fi
-
 _cleanup()
 {
 	eval "rm -f $BASE_TEST_FILE.*"
diff --git a/tests/generic/436 b/tests/generic/436
index bcd6ddc..6cda008 100755
--- a/tests/generic/436
+++ b/tests/generic/436
@@ -44,12 +44,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
 
 _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
-fi
-
 _cleanup()
 {
 	rm -f $tmp.* $BASE_TEST_FILE.*
diff --git a/tests/generic/445 b/tests/generic/445
index 81dd916..323a0ca 100755
--- a/tests/generic/445
+++ b/tests/generic/445
@@ -44,12 +44,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
 
 _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
-fi
-
 _cleanup()
 {
 	rm -f $tmp.* $BASE_TEST_FILE.*
diff --git a/tests/generic/448 b/tests/generic/448
index 87b99d7..22656f6 100755
--- a/tests/generic/448
+++ b/tests/generic/448
@@ -48,12 +48,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
 
 _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
-fi
-
 $here/src/seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE > $seqres.full 2>&1 ||
 	_fail "seek sanity check failed!"
 
-- 
1.8.3.1

Re: [PATCH] common/rc: factor out _ext4_disable_extent_zeroout() helper

[Eryu Guan]

On Mon, Jul 24, 2017 at 05:49:07PM +0800, Xiao Yang wrote:
> 1) This pattern is repeated in several seek_data/hole tests
>    (e.g. generic/285, generic/436, generic/445 generic/448)
>    and generic/009.  A common _ext4_disable_extent_zeroout()
>    helper could be added and applied by generic/009 and
>    _require_seek_data_hole().
> 
> 2) On some old kernels(e.g. v3.1-v3.6), when vfs recognizes
>    SEEK_DATA/HOLE flag && ext4 has no extent zeroout tunable
>    in sysfs, these cases may trigger "sysfs entry not found"
>    issue.  We can add check if extent_max_zeroout_kb exists
>    on ext4 filesystem.
>    The extent_max_zeroout_kb is introduced by:
>    '67a5da564f97 ("ext4: make the zero-out chunk size tunable")'
> 
> 3) Declare several vars as local in _require_seek_data_hole().
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>

Thanks for doing this!

> ---
>  common/rc         | 29 ++++++++++++++++++++++-------
>  tests/generic/009 |  7 +------
>  tests/generic/285 |  6 ------
>  tests/generic/436 |  6 ------
>  tests/generic/445 |  6 ------
>  tests/generic/448 |  6 ------
>  6 files changed, 23 insertions(+), 37 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index bd989bb..b36a9bf 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2304,16 +2304,31 @@ _require_fail_make_request()
>   not found. Seems that CONFIG_FAIL_MAKE_REQUEST kernel config option not enabled"
>  }
>  
> -#
> +# Disable extent zeroing for ext4 on the given device
> +_ext4_disable_extent_zeroout()
> +{
> +	local dev=${1:-$TEST_DEV}
> +	local sdev=`_short_dev $dev`
> +
> +	if [ "$FSTYP" == "ext4" ]; then
> +		[ -f /sys/fs/ext4/$sdev/extent_max_zeroout_kb ] && \
> +			echo 0 >/sys/fs/ext4/$sdev/extent_max_zeroout_kb
> +	fi

I'd like the caller to do this FSTYP check if we name the function with
a leading _ext4, it seems redundant to check FSTYP again when we know it
only works for ext4.

Or we can make it a function serves all FSTYP using a case switch, and
ext4 is the only filesystem that needs action. But I'm not sure if it's
necessary for other filesystems at all, even in the future, so I
suggested _ext4_disable_extent_zeroout in the first place.

Thanks,
Eryu

> +}
> +
>  # Check if the file system supports seek_data/hole
> -#
>  _require_seek_data_hole()
>  {
> -    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"
> +	local dev=${1:-$TEST_DEV}
> +	local testfile=$TEST_DIR/$$.seek
> +	local 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
> +	_ext4_disable_extent_zeroout $dev
>  }
>  
>  _require_runas()
> diff --git a/tests/generic/009 b/tests/generic/009
> index 5902afd..a7ca060 100755
> --- a/tests/generic/009
> +++ b/tests/generic/009
> @@ -48,15 +48,10 @@ _require_xfs_io_command "fzero"
>  _require_xfs_io_command "fiemap"
>  _require_xfs_io_command "falloc"
>  _require_test
> +_ext4_disable_extent_zeroout
>  
>  testfile=$TEST_DIR/009.$$
>  
> -# 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
> -fi
> -
>  # When PAGE_SIZE > 4096 xfs extent layout is different so it would not match
>  # the output.
>  if [ "$FSTYP" = "xfs" ]; then
> diff --git a/tests/generic/285 b/tests/generic/285
> index 16e70b1..3f7d298 100755
> --- a/tests/generic/285
> +++ b/tests/generic/285
> @@ -47,12 +47,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
>  
>  _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
> -fi
> -
>  _cleanup()
>  {
>  	eval "rm -f $BASE_TEST_FILE.*"
> diff --git a/tests/generic/436 b/tests/generic/436
> index bcd6ddc..6cda008 100755
> --- a/tests/generic/436
> +++ b/tests/generic/436
> @@ -44,12 +44,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
>  
>  _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
> -fi
> -
>  _cleanup()
>  {
>  	rm -f $tmp.* $BASE_TEST_FILE.*
> diff --git a/tests/generic/445 b/tests/generic/445
> index 81dd916..323a0ca 100755
> --- a/tests/generic/445
> +++ b/tests/generic/445
> @@ -44,12 +44,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
>  
>  _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
> -fi
> -
>  _cleanup()
>  {
>  	rm -f $tmp.* $BASE_TEST_FILE.*
> diff --git a/tests/generic/448 b/tests/generic/448
> index 87b99d7..22656f6 100755
> --- a/tests/generic/448
> +++ b/tests/generic/448
> @@ -48,12 +48,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
>  
>  _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
> -fi
> -
>  $here/src/seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE > $seqres.full 2>&1 ||
>  	_fail "seek sanity check failed!"
>  
> -- 
> 1.8.3.1
> 
> 
> 

Re: [PATCH] common/rc: factor out _ext4_disable_extent_zeroout() helper

[Xiao Yang]

On 2017/07/24 18:04, Eryu Guan wrote:
> On Mon, Jul 24, 2017 at 05:49:07PM +0800, Xiao Yang wrote:
>> 1) This pattern is repeated in several seek_data/hole tests
>>     (e.g. generic/285, generic/436, generic/445 generic/448)
>>     and generic/009.  A common _ext4_disable_extent_zeroout()
>>     helper could be added and applied by generic/009 and
>>     _require_seek_data_hole().
>>
>> 2) On some old kernels(e.g. v3.1-v3.6), when vfs recognizes
>>     SEEK_DATA/HOLE flag&&  ext4 has no extent zeroout tunable
>>     in sysfs, these cases may trigger "sysfs entry not found"
>>     issue.  We can add check if extent_max_zeroout_kb exists
>>     on ext4 filesystem.
>>     The extent_max_zeroout_kb is introduced by:
>>     '67a5da564f97 ("ext4: make the zero-out chunk size tunable")'
>>
>> 3) Declare several vars as local in _require_seek_data_hole().
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> Thanks for doing this!
>
>> ---
>>   common/rc         | 29 ++++++++++++++++++++++-------
>>   tests/generic/009 |  7 +------
>>   tests/generic/285 |  6 ------
>>   tests/generic/436 |  6 ------
>>   tests/generic/445 |  6 ------
>>   tests/generic/448 |  6 ------
>>   6 files changed, 23 insertions(+), 37 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index bd989bb..b36a9bf 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -2304,16 +2304,31 @@ _require_fail_make_request()
>>    not found. Seems that CONFIG_FAIL_MAKE_REQUEST kernel config option not enabled"
>>   }
>>
>> -#
>> +# Disable extent zeroing for ext4 on the given device
>> +_ext4_disable_extent_zeroout()
>> +{
>> +	local dev=${1:-$TEST_DEV}
>> +	local sdev=`_short_dev $dev`
>> +
>> +	if [ "$FSTYP" == "ext4" ]; then
>> +		[ -f /sys/fs/ext4/$sdev/extent_max_zeroout_kb ]&&  \
>> +			echo 0>/sys/fs/ext4/$sdev/extent_max_zeroout_kb
>> +	fi
> I'd like the caller to do this FSTYP check if we name the function with
> a leading _ext4, it seems redundant to check FSTYP again when we know it
> only works for ext4.
Hi Eryu,

I prefer that only ext4 could call _ext4_disable_extent_zeroout.
I will send v2 patch as your above comment.  Thanks a lot.

Thanks,
Xiao Yang
> Or we can make it a function serves all FSTYP using a case switch, and
> ext4 is the only filesystem that needs action. But I'm not sure if it's
> necessary for other filesystems at all, even in the future, so I
> suggested _ext4_disable_extent_zeroout in the first place.
>
> Thanks,
> Eryu
>
>> +}
>> +
>>   # Check if the file system supports seek_data/hole
>> -#
>>   _require_seek_data_hole()
>>   {
>> -    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"
>> +	local dev=${1:-$TEST_DEV}
>> +	local testfile=$TEST_DIR/$$.seek
>> +	local 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
>> +	_ext4_disable_extent_zeroout $dev
>>   }
>>
>>   _require_runas()
>> diff --git a/tests/generic/009 b/tests/generic/009
>> index 5902afd..a7ca060 100755
>> --- a/tests/generic/009
>> +++ b/tests/generic/009
>> @@ -48,15 +48,10 @@ _require_xfs_io_command "fzero"
>>   _require_xfs_io_command "fiemap"
>>   _require_xfs_io_command "falloc"
>>   _require_test
>> +_ext4_disable_extent_zeroout
>>
>>   testfile=$TEST_DIR/009.$$
>>
>> -# 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
>> -fi
>> -
>>   # When PAGE_SIZE>  4096 xfs extent layout is different so it would not match
>>   # the output.
>>   if [ "$FSTYP" = "xfs" ]; then
>> diff --git a/tests/generic/285 b/tests/generic/285
>> index 16e70b1..3f7d298 100755
>> --- a/tests/generic/285
>> +++ b/tests/generic/285
>> @@ -47,12 +47,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
>>
>>   _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
>> -fi
>> -
>>   _cleanup()
>>   {
>>   	eval "rm -f $BASE_TEST_FILE.*"
>> diff --git a/tests/generic/436 b/tests/generic/436
>> index bcd6ddc..6cda008 100755
>> --- a/tests/generic/436
>> +++ b/tests/generic/436
>> @@ -44,12 +44,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
>>
>>   _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
>> -fi
>> -
>>   _cleanup()
>>   {
>>   	rm -f $tmp.* $BASE_TEST_FILE.*
>> diff --git a/tests/generic/445 b/tests/generic/445
>> index 81dd916..323a0ca 100755
>> --- a/tests/generic/445
>> +++ b/tests/generic/445
>> @@ -44,12 +44,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
>>
>>   _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
>> -fi
>> -
>>   _cleanup()
>>   {
>>   	rm -f $tmp.* $BASE_TEST_FILE.*
>> diff --git a/tests/generic/448 b/tests/generic/448
>> index 87b99d7..22656f6 100755
>> --- a/tests/generic/448
>> +++ b/tests/generic/448
>> @@ -48,12 +48,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
>>
>>   _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
>> -fi
>> -
>>   $here/src/seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE>  $seqres.full 2>&1 ||
>>   	_fail "seek sanity check failed!"
>>
>> -- 
>> 1.8.3.1
>>
>>
>>
>
> .
>

[PATCH v2] common/rc: factor out _ext4_disable_extent_zeroout() helper

[Xiao Yang]

1) This pattern is repeated in several seek_data/hole tests
   (e.g. generic/285, generic/436, generic/445 generic/448)
   and generic/009.  A common _ext4_disable_extent_zeroout()
   helper could be added and applied by generic/009 and
   _require_seek_data_hole().

2) On some old kernels(e.g. v3.1-v3.6), when vfs recognizes
   SEEK_DATA/HOLE flag && ext4 has no extent zeroout tunable
   in sysfs, these cases may trigger "sysfs entry not found"
   issue.  We can add check if extent_max_zeroout_kb exists
   on ext4 filesystem.
   The extent_max_zeroout_kb is introduced by:
   '67a5da564f97 ("ext4: make the zero-out chunk size tunable")'

3) Declare several vars as local in _require_seek_data_hole().

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 common/rc         | 29 ++++++++++++++++++++++-------
 tests/generic/009 |  3 +--
 tests/generic/285 |  6 ------
 tests/generic/436 |  6 ------
 tests/generic/445 |  6 ------
 tests/generic/448 |  6 ------
 6 files changed, 23 insertions(+), 33 deletions(-)

diff --git a/common/rc b/common/rc
index bd989bb..49887cf 100644
--- a/common/rc
+++ b/common/rc
@@ -2304,16 +2304,31 @@ _require_fail_make_request()
  not found. Seems that CONFIG_FAIL_MAKE_REQUEST kernel config option not enabled"
 }
 
-#
+# 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
+}
+
 # Check if the file system supports seek_data/hole
-#
 _require_seek_data_hole()
 {
-    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"
+	local dev=${1:-$TEST_DEV}
+	local testfile=$TEST_DIR/$$.seek
+	local 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
 }
 
 _require_runas()
diff --git a/tests/generic/009 b/tests/generic/009
index 5902afd..797def5 100755
--- a/tests/generic/009
+++ b/tests/generic/009
@@ -53,8 +53,7 @@ testfile=$TEST_DIR/009.$$
 
 # 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
+	_ext4_disable_extent_zeroout
 fi
 
 # When PAGE_SIZE > 4096 xfs extent layout is different so it would not match
diff --git a/tests/generic/285 b/tests/generic/285
index 16e70b1..3f7d298 100755
--- a/tests/generic/285
+++ b/tests/generic/285
@@ -47,12 +47,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
 
 _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
-fi
-
 _cleanup()
 {
 	eval "rm -f $BASE_TEST_FILE.*"
diff --git a/tests/generic/436 b/tests/generic/436
index bcd6ddc..6cda008 100755
--- a/tests/generic/436
+++ b/tests/generic/436
@@ -44,12 +44,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
 
 _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
-fi
-
 _cleanup()
 {
 	rm -f $tmp.* $BASE_TEST_FILE.*
diff --git a/tests/generic/445 b/tests/generic/445
index 81dd916..323a0ca 100755
--- a/tests/generic/445
+++ b/tests/generic/445
@@ -44,12 +44,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
 
 _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
-fi
-
 _cleanup()
 {
 	rm -f $tmp.* $BASE_TEST_FILE.*
diff --git a/tests/generic/448 b/tests/generic/448
index 87b99d7..22656f6 100755
--- a/tests/generic/448
+++ b/tests/generic/448
@@ -48,12 +48,6 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
 
 _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
-fi
-
 $here/src/seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE > $seqres.full 2>&1 ||
 	_fail "seek sanity check failed!"
 
-- 
1.8.3.1

[PATCH] common/rc: update _require_seek_data_hole()

[Xiao Yang]

_require_seek_data_hole() could not skip tests if filesystem does not support
fallocate or unwritten extent.  We could update _require_seek_data_hole() to
catch both of them.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 common/rc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index fa1314c..b064f53 100644
--- a/common/rc
+++ b/common/rc
@@ -2297,7 +2297,7 @@ _require_seek_data_hole()
     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" && \
+    echo $testseek | grep -q "not support" && \
         _notrun "File system does not support llseek(2) SEEK_DATA/HOLE"
 }
 
-- 
1.8.3.1

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

[Xiao Yang]

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.)
>
>> 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?
Hi Eryu,

I am sorry for missing this suggestion, it sounds better to me. :-)
I will rewrite this patch as you said.  Thanks a lot.

Thanks,
Xiao Yang.
> 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
>
> .
>