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