| Message ID | 1500889747-12149-1-git-send-email-yangx.jy@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > > > -- 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
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 >> >> >> > > . > -- 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
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) 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(-)