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

[Eryu Guan <eguan@redhat.com>]

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