* [PATCH 1/2] generic/{436,445}: check falloc support
@ 2019-02-26 14:09 Amir Goldstein
2019-02-26 14:09 ` [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole Amir Goldstein
2019-03-02 15:26 ` [PATCH 1/2] generic/{436,445}: check falloc support Eryu Guan
0 siblings, 2 replies; 6+ messages in thread
From: Amir Goldstein @ 2019-02-26 14:09 UTC (permalink / raw)
To: Eryu Guan; +Cc: Miklos Szeredi, Eddie Horng, linux-unionfs, fstests
The sanity test case in those tests (i.e. 13..17)
are all skipped in fs with no falloc support, but the tests
are reported to pass.
For example, from 445.full:
File system supports the default behavior.
File system does not support fallocate.
Allocation size: 4096
17. Test file with unwritten extents, data-hole-data inside page
Test skipped as fs doesn't support unwritten extents.
Explicitly check for falloc support before running those tests
so they would be properly reported as skipped.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
tests/generic/436 | 2 ++
tests/generic/445 | 2 ++
tests/generic/group | 4 ++--
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/tests/generic/436 b/tests/generic/436
index 3ed22c96..a3ef70e5 100755
--- a/tests/generic/436
+++ b/tests/generic/436
@@ -24,6 +24,8 @@ _supported_os Linux
_require_test
_require_seek_data_hole
+# All the seek sanity test cases here do falloc
+_require_xfs_io_command "falloc"
BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
diff --git a/tests/generic/445 b/tests/generic/445
index e697cca0..694913ae 100755
--- a/tests/generic/445
+++ b/tests/generic/445
@@ -24,6 +24,8 @@ _supported_os Linux
_require_test
_require_seek_data_hole
+# All the seek sanity test cases here do falloc
+_require_xfs_io_command "falloc"
BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
diff --git a/tests/generic/group b/tests/generic/group
index 31011ac8..15227b67 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -438,7 +438,7 @@
433 auto quick copy
434 auto quick copy
435 auto encrypt
-436 auto quick rw seek
+436 auto quick rw seek prealloc
437 auto quick dax
438 auto
439 auto quick punch
@@ -447,7 +447,7 @@
442 blockdev
443 auto quick rw
444 auto quick acl
-445 auto quick rw seek
+445 auto quick rw seek prealloc
446 auto quick rw punch
447 auto clone
448 auto quick rw seek
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole 2019-02-26 14:09 [PATCH 1/2] generic/{436,445}: check falloc support Amir Goldstein @ 2019-02-26 14:09 ` Amir Goldstein 2019-03-02 15:25 ` Eryu Guan 2019-03-02 15:26 ` [PATCH 1/2] generic/{436,445}: check falloc support Eryu Guan 1 sibling, 1 reply; 6+ messages in thread From: Amir Goldstein @ 2019-02-26 14:09 UTC (permalink / raw) To: Eryu Guan; +Cc: Miklos Szeredi, Eddie Horng, linux-unionfs, fstests Added a test case to seek_sanity_test and a test to run it. When checking for SEEK_HOLE support, abort if filesystem supports punching holes that SEEK_HOLE will not find, because this configuration doesn't make any sense. This kind of senless behavior was introduced to overlayfs in v4.19 with stacked file operations, because overlay fallocate op is stacked and llseek op is not stacked. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Eryu, After this change, the generic/seek group tests will start failing with overlayfs on upstream kernel. We had missing coverage of SEEK_HOLE, so we missed a regression in kernel v4.19 when overlayfs SEEK_HOLE stopped finding punched holes (or any holes for that matter). Thanks, Amir. src/seek_sanity_test.c | 63 +++++++++++++++++++++++++++++++++++++++--- tests/generic/999 | 44 +++++++++++++++++++++++++++++ tests/generic/999.out | 1 + tests/generic/group | 1 + 4 files changed, 105 insertions(+), 4 deletions(-) create mode 100755 tests/generic/999 create mode 100644 tests/generic/999.out diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c index e9938d1b..a98bae03 100644 --- a/src/seek_sanity_test.c +++ b/src/seek_sanity_test.c @@ -16,6 +16,7 @@ #include <unistd.h> #include <stdlib.h> #include <assert.h> +#include "global.h" #ifndef SEEK_DATA #define SEEK_DATA 3 @@ -25,6 +26,7 @@ static blksize_t alloc_size; int default_behavior = 0; int unwritten_extents = 0; +int punch_hole = 0; char *base_file_path; static void get_file_system(int fd) @@ -117,8 +119,9 @@ static int do_fallocate(int fd, off_t offset, off_t length, int mode) ret = fallocate(fd, mode, offset, length); if (ret) - fprintf(stderr, " ERROR %d: Failed to preallocate " - "space to %ld bytes\n", errno, (long) length); + fprintf(stderr, " ERROR %d: Failed to %s of %ld bytes\n", + errno, (mode & FALLOC_FL_PUNCH_HOLE) ? "punch hole" : + "preallocate space", (long) length); return ret; } @@ -261,6 +264,47 @@ out: return ret; } +/* + * Make sure hole size is properly reported when punched in the middle of a file + */ +static int test21(int fd, int testnum) +{ + char *buf = NULL; + int bufsz, filsz; + int ret = 0; + + if (!punch_hole) { + fprintf(stdout, "Test skipped as fs doesn't support punch hole.\n"); + goto out; + } + + bufsz = alloc_size * 3; + buf = do_malloc(bufsz); + if (!buf) { + ret = -1; + goto out; + } + memset(buf, 'a', bufsz); + + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + filsz = bufsz; + ret = do_fallocate(fd, alloc_size, alloc_size, + FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); + if (ret < 0) + goto out; + + ret += do_lseek(testnum, 1, fd, filsz, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 2, fd, filsz, SEEK_HOLE, 0, alloc_size); + ret += do_lseek(testnum, 3, fd, filsz, SEEK_DATA, alloc_size, alloc_size * 2); +out: + if (buf) + free(buf); + return ret; +} + /* * Make sure hole size is properly reported when starting in the middle of a * hole in ext? doubly indirect tree @@ -1049,6 +1093,7 @@ struct testrec seek_tests[] = { { 18, test18, "Test file with negative SEEK_{HOLE,DATA} offsets" }, { 19, test19, "Test file SEEK_DATA from middle of a large hole" }, { 20, test20, "Test file SEEK_DATA from middle of a huge hole" }, + { 21, test21, "Test file SEEK_HOLE that was created by PUNCH_HOLE" }, }; static int run_test(struct testrec *tr) @@ -1120,15 +1165,25 @@ static int test_basic_support(void) } ftruncate(fd, 0); - if (fallocate(fd, 0, 0, alloc_size) == -1) { + if (fallocate(fd, 0, 0, alloc_size * 2) == -1) { if (errno == EOPNOTSUPP) - fprintf(stderr, "File system does not support fallocate."); + fprintf(stderr, "File system does not support fallocate.\n"); else { fprintf(stderr, "ERROR %d: Failed to preallocate " "space to %ld bytes. Aborting.\n", errno, (long) alloc_size); ret = -1; } goto out; + } else if (fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, + 0, alloc_size) == -1) { + fprintf(stderr, "File system does not support punch hole.\n"); + } else if (default_behavior) { + fprintf(stderr, "File system supports punch hole, but does not support " + "finding holes with SEEK_HOLE. Aborting.\n"); + ret = -1; + goto out; + } else { + punch_hole = 1; } pos = lseek(fd, 0, SEEK_DATA); diff --git a/tests/generic/999 b/tests/generic/999 new file mode 100755 index 00000000..b52f6985 --- /dev/null +++ b/tests/generic/999 @@ -0,0 +1,44 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2019, CTERA Networks. All Rights Reserved. +# +# FS QA Test No. 999 +# +# Check that SEEK_HOLE can find a punched hole. +# +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common/rc + +_supported_fs generic +_supported_os Linux + +_require_test +_require_seek_data_hole +_require_xfs_io_command "fpunch" + +base_test_file=$TEST_DIR/seek_sanity_testfile.$seq + +_require_test_program "seek_sanity_test" + +_cleanup() +{ + cd / + rm -f $tmp.* + rm -f $base_test_file* +} + +$here/src/seek_sanity_test -s 21 -e 21 $base_test_file > $seqres.full 2>&1 || + _fail "seek sanity check failed!" + +# success, all done +status=0 +exit diff --git a/tests/generic/999.out b/tests/generic/999.out new file mode 100644 index 00000000..7fbc6768 --- /dev/null +++ b/tests/generic/999.out @@ -0,0 +1 @@ +QA output created by 999 diff --git a/tests/generic/group b/tests/generic/group index 15227b67..14ac9b2c 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -534,3 +534,4 @@ 529 auto quick attr 530 auto quick unlink 531 auto quick unlink +999 auto quick punch seek -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole 2019-02-26 14:09 ` [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole Amir Goldstein @ 2019-03-02 15:25 ` Eryu Guan 2019-03-02 16:09 ` Amir Goldstein 0 siblings, 1 reply; 6+ messages in thread From: Eryu Guan @ 2019-03-02 15:25 UTC (permalink / raw) To: Amir Goldstein; +Cc: Miklos Szeredi, Eddie Horng, linux-unionfs, fstests On Tue, Feb 26, 2019 at 04:09:02PM +0200, Amir Goldstein wrote: > Added a test case to seek_sanity_test and a test to run it. > > When checking for SEEK_HOLE support, abort if filesystem > supports punching holes that SEEK_HOLE will not find, because > this configuration doesn't make any sense. Hmm, I don't think it's a good idea to call it a bug if the filesystem decides to support punch hole but not SEEK_HOLE (non-default behavior). It's not a ideal to support only punch hole but not SEEK_HOLE, as you mentioned that only hugetlbfs supports this strange combination, but there's no standard to forbid such combination. IMHO, punch hole and SEEK_HOLE are totally two independent features, filesystems are free to support any or both of them. > > This kind of senless behavior was introduced to overlayfs > in v4.19 with stacked file operations, because overlay fallocate > op is stacked and llseek op is not stacked. And I think this is an overlay-specific bug. I'd suggest comparing SEEK_DATA/SEEK_HOLE results from underlying filesystem and from overlayfs, to make sure the two results are the same, which means if underlying fs supports SEEK_HOLE overlayfs calls llseek op from underlying fs too. (Perhaps using xfs_io's seek command.) > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Eryu, > > After this change, the generic/seek group tests will start > failing with overlayfs on upstream kernel. > > We had missing coverage of SEEK_HOLE, so we missed a regression > in kernel v4.19 when overlayfs SEEK_HOLE stopped finding punched > holes (or any holes for that matter). So I suggest two new tests, one overlay-specific test to cover the regression, and one generic test to cover seek holes after punch hole (as this one, but don't fail if punch_hole == true && SEEK_HOLE == false). Thanks, Eryu > > Thanks, > Amir. > > src/seek_sanity_test.c | 63 +++++++++++++++++++++++++++++++++++++++--- > tests/generic/999 | 44 +++++++++++++++++++++++++++++ > tests/generic/999.out | 1 + > tests/generic/group | 1 + > 4 files changed, 105 insertions(+), 4 deletions(-) > create mode 100755 tests/generic/999 > create mode 100644 tests/generic/999.out > > diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c > index e9938d1b..a98bae03 100644 > --- a/src/seek_sanity_test.c > +++ b/src/seek_sanity_test.c > @@ -16,6 +16,7 @@ > #include <unistd.h> > #include <stdlib.h> > #include <assert.h> > +#include "global.h" > > #ifndef SEEK_DATA > #define SEEK_DATA 3 > @@ -25,6 +26,7 @@ > static blksize_t alloc_size; > int default_behavior = 0; > int unwritten_extents = 0; > +int punch_hole = 0; > char *base_file_path; > > static void get_file_system(int fd) > @@ -117,8 +119,9 @@ static int do_fallocate(int fd, off_t offset, off_t length, int mode) > > ret = fallocate(fd, mode, offset, length); > if (ret) > - fprintf(stderr, " ERROR %d: Failed to preallocate " > - "space to %ld bytes\n", errno, (long) length); > + fprintf(stderr, " ERROR %d: Failed to %s of %ld bytes\n", > + errno, (mode & FALLOC_FL_PUNCH_HOLE) ? "punch hole" : > + "preallocate space", (long) length); > > return ret; > } > @@ -261,6 +264,47 @@ out: > return ret; > } > > +/* > + * Make sure hole size is properly reported when punched in the middle of a file > + */ > +static int test21(int fd, int testnum) > +{ > + char *buf = NULL; > + int bufsz, filsz; > + int ret = 0; > + > + if (!punch_hole) { > + fprintf(stdout, "Test skipped as fs doesn't support punch hole.\n"); > + goto out; > + } > + > + bufsz = alloc_size * 3; > + buf = do_malloc(bufsz); > + if (!buf) { > + ret = -1; > + goto out; > + } > + memset(buf, 'a', bufsz); > + > + ret = do_pwrite(fd, buf, bufsz, 0); > + if (ret) > + goto out; > + > + filsz = bufsz; > + ret = do_fallocate(fd, alloc_size, alloc_size, > + FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE); > + if (ret < 0) > + goto out; > + > + ret += do_lseek(testnum, 1, fd, filsz, SEEK_DATA, 0, 0); > + ret += do_lseek(testnum, 2, fd, filsz, SEEK_HOLE, 0, alloc_size); > + ret += do_lseek(testnum, 3, fd, filsz, SEEK_DATA, alloc_size, alloc_size * 2); > +out: > + if (buf) > + free(buf); > + return ret; > +} > + > /* > * Make sure hole size is properly reported when starting in the middle of a > * hole in ext? doubly indirect tree > @@ -1049,6 +1093,7 @@ struct testrec seek_tests[] = { > { 18, test18, "Test file with negative SEEK_{HOLE,DATA} offsets" }, > { 19, test19, "Test file SEEK_DATA from middle of a large hole" }, > { 20, test20, "Test file SEEK_DATA from middle of a huge hole" }, > + { 21, test21, "Test file SEEK_HOLE that was created by PUNCH_HOLE" }, > }; > > static int run_test(struct testrec *tr) > @@ -1120,15 +1165,25 @@ static int test_basic_support(void) > } > > ftruncate(fd, 0); > - if (fallocate(fd, 0, 0, alloc_size) == -1) { > + if (fallocate(fd, 0, 0, alloc_size * 2) == -1) { > if (errno == EOPNOTSUPP) > - fprintf(stderr, "File system does not support fallocate."); > + fprintf(stderr, "File system does not support fallocate.\n"); > else { > fprintf(stderr, "ERROR %d: Failed to preallocate " > "space to %ld bytes. Aborting.\n", errno, (long) alloc_size); > ret = -1; > } > goto out; > + } else if (fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, > + 0, alloc_size) == -1) { > + fprintf(stderr, "File system does not support punch hole.\n"); > + } else if (default_behavior) { > + fprintf(stderr, "File system supports punch hole, but does not support " > + "finding holes with SEEK_HOLE. Aborting.\n"); > + ret = -1; > + goto out; > + } else { > + punch_hole = 1; > } > > pos = lseek(fd, 0, SEEK_DATA); > diff --git a/tests/generic/999 b/tests/generic/999 > new file mode 100755 > index 00000000..b52f6985 > --- /dev/null > +++ b/tests/generic/999 > @@ -0,0 +1,44 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2019, CTERA Networks. All Rights Reserved. > +# > +# FS QA Test No. 999 > +# > +# Check that SEEK_HOLE can find a punched hole. > +# > +seq=`basename $0` > +seqres=$RESULT_DIR/$seq > +echo "QA output created by $seq" > + > +here=`pwd` > +tmp=/tmp/$$ > +status=1 # failure is the default! > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +# get standard environment, filters and checks > +. ./common/rc > + > +_supported_fs generic > +_supported_os Linux > + > +_require_test > +_require_seek_data_hole > +_require_xfs_io_command "fpunch" > + > +base_test_file=$TEST_DIR/seek_sanity_testfile.$seq > + > +_require_test_program "seek_sanity_test" > + > +_cleanup() > +{ > + cd / > + rm -f $tmp.* > + rm -f $base_test_file* > +} > + > +$here/src/seek_sanity_test -s 21 -e 21 $base_test_file > $seqres.full 2>&1 || > + _fail "seek sanity check failed!" > + > +# success, all done > +status=0 > +exit > diff --git a/tests/generic/999.out b/tests/generic/999.out > new file mode 100644 > index 00000000..7fbc6768 > --- /dev/null > +++ b/tests/generic/999.out > @@ -0,0 +1 @@ > +QA output created by 999 > diff --git a/tests/generic/group b/tests/generic/group > index 15227b67..14ac9b2c 100644 > --- a/tests/generic/group > +++ b/tests/generic/group > @@ -534,3 +534,4 @@ > 529 auto quick attr > 530 auto quick unlink > 531 auto quick unlink > +999 auto quick punch seek > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole 2019-03-02 15:25 ` Eryu Guan @ 2019-03-02 16:09 ` Amir Goldstein 2019-03-02 16:30 ` Eryu Guan 0 siblings, 1 reply; 6+ messages in thread From: Amir Goldstein @ 2019-03-02 16:09 UTC (permalink / raw) To: Eryu Guan; +Cc: Miklos Szeredi, Eddie Horng, overlayfs, fstests On Sat, Mar 2, 2019 at 5:25 PM Eryu Guan <guaneryu@gmail.com> wrote: > > On Tue, Feb 26, 2019 at 04:09:02PM +0200, Amir Goldstein wrote: > > Added a test case to seek_sanity_test and a test to run it. > > > > When checking for SEEK_HOLE support, abort if filesystem > > supports punching holes that SEEK_HOLE will not find, because > > this configuration doesn't make any sense. > > Hmm, I don't think it's a good idea to call it a bug if the filesystem > decides to support punch hole but not SEEK_HOLE (non-default behavior). > It's not a ideal to support only punch hole but not SEEK_HOLE, as you > mentioned that only hugetlbfs supports this strange combination, but > there's no standard to forbid such combination. IMHO, punch hole and > SEEK_HOLE are totally two independent features, filesystems are free to > support any or both of them. > > > > > This kind of senless behavior was introduced to overlayfs > > in v4.19 with stacked file operations, because overlay fallocate > > op is stacked and llseek op is not stacked. > > And I think this is an overlay-specific bug. I'd suggest comparing > SEEK_DATA/SEEK_HOLE results from underlying filesystem and from > overlayfs, to make sure the two results are the same, which means if > underlying fs supports SEEK_HOLE overlayfs calls llseek op from > underlying fs too. (Perhaps using xfs_io's seek command.) > I understand where your arguments are coming from, but from my perspective, what happened in overlayfs could happen in any filesystem. A regression that causes filesystem to revert to "default" behavior would go unnoticed with current set of xfstest seek sanity tests. So while the bug was overlayfs specific, the lack of test coverage is generic. So I thought to myself, well how can the test suite know if a filesystem is supposed to SEEK_HOLE? and I found a property that could be used as a very strong indication that filesystem is expected to support SEEK_HOLE. IMO, the question whether or not this is a standard is way less interesting than the question - what are the odds that a filesystem wants to be tested in xfstests does not follow this rule? I do not know the answer to this question, but if you think the odds are low, then I believe it is worth using this heuristic to improve test coverage. Another option is to whitelist/blacklist xfstests supported filesystems that support "proper" SEEK_HOLE, so the sanity tests will actually verify a single expected behavior, instead of verifying one of two possible expected behaviors. Let me know what you think. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Eryu, > > > > After this change, the generic/seek group tests will start > > failing with overlayfs on upstream kernel. > > > > We had missing coverage of SEEK_HOLE, so we missed a regression > > in kernel v4.19 when overlayfs SEEK_HOLE stopped finding punched > > holes (or any holes for that matter). > > So I suggest two new tests, one overlay-specific test to cover the > regression, and one generic test to cover seek holes after punch hole > (as this one, but don't fail if punch_hole == true && SEEK_HOLE == > false). > OK, this is what I'll do, but as I write above, this will result in suboptimal generic test coverage. - Add flag seek_sanity_test -f that fails if default_behavior is detected. - Use this flag for requirements of new generic PUNCH+SEEK test: _require_seek_data_hole -f _require_xfs_io_command "fpunch" - For overlayfs specific test, the test will not _require_seek_data_hole -f but it will instead check real SEEK_HOLE support of base fs and then will test seek of punched hole with -f flag. Thanks, Amir. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole 2019-03-02 16:09 ` Amir Goldstein @ 2019-03-02 16:30 ` Eryu Guan 0 siblings, 0 replies; 6+ messages in thread From: Eryu Guan @ 2019-03-02 16:30 UTC (permalink / raw) To: Amir Goldstein; +Cc: Miklos Szeredi, Eddie Horng, overlayfs, fstests On Sat, Mar 02, 2019 at 06:09:47PM +0200, Amir Goldstein wrote: > On Sat, Mar 2, 2019 at 5:25 PM Eryu Guan <guaneryu@gmail.com> wrote: > > > > On Tue, Feb 26, 2019 at 04:09:02PM +0200, Amir Goldstein wrote: > > > Added a test case to seek_sanity_test and a test to run it. > > > > > > When checking for SEEK_HOLE support, abort if filesystem > > > supports punching holes that SEEK_HOLE will not find, because > > > this configuration doesn't make any sense. > > > > Hmm, I don't think it's a good idea to call it a bug if the filesystem > > decides to support punch hole but not SEEK_HOLE (non-default behavior). > > It's not a ideal to support only punch hole but not SEEK_HOLE, as you > > mentioned that only hugetlbfs supports this strange combination, but > > there's no standard to forbid such combination. IMHO, punch hole and > > SEEK_HOLE are totally two independent features, filesystems are free to > > support any or both of them. > > > > > > > > This kind of senless behavior was introduced to overlayfs > > > in v4.19 with stacked file operations, because overlay fallocate > > > op is stacked and llseek op is not stacked. > > > > And I think this is an overlay-specific bug. I'd suggest comparing > > SEEK_DATA/SEEK_HOLE results from underlying filesystem and from > > overlayfs, to make sure the two results are the same, which means if > > underlying fs supports SEEK_HOLE overlayfs calls llseek op from > > underlying fs too. (Perhaps using xfs_io's seek command.) > > > > I understand where your arguments are coming from, but from my > perspective, what happened in overlayfs could happen in any filesystem. > A regression that causes filesystem to revert to "default" behavior would > go unnoticed with current set of xfstest seek sanity tests. > > So while the bug was overlayfs specific, the lack of test coverage is > generic. So I thought to myself, well how can the test suite know if > a filesystem is supposed to SEEK_HOLE? and I found a property > that could be used as a very strong indication that filesystem is expected > to support SEEK_HOLE. > > IMO, the question whether or not this is a standard is way less > interesting than the question - what are the odds that a filesystem > wants to be tested in xfstests does not follow this rule? > I do not know the answer to this question, but if you think the odds > are low, then I believe it is worth using this heuristic to improve > test coverage. > > Another option is to whitelist/blacklist xfstests supported filesystems > that support "proper" SEEK_HOLE, so the sanity tests will actually > verify a single expected behavior, instead of verifying one of two > possible expected behaviors. > Let me know what you think. I'd rather go with the whitelist/blacklist option. Perhaps a wrapper function around seek_sanity_test and automatically add the proposed "-f" argument if $FSTYP is in that list. > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > > > > Eryu, > > > > > > After this change, the generic/seek group tests will start > > > failing with overlayfs on upstream kernel. > > > > > > We had missing coverage of SEEK_HOLE, so we missed a regression > > > in kernel v4.19 when overlayfs SEEK_HOLE stopped finding punched > > > holes (or any holes for that matter). > > > > So I suggest two new tests, one overlay-specific test to cover the > > regression, and one generic test to cover seek holes after punch hole > > (as this one, but don't fail if punch_hole == true && SEEK_HOLE == > > false). > > > > OK, this is what I'll do, but as I write above, this will result in suboptimal > generic test coverage. > > - Add flag seek_sanity_test -f that fails if default_behavior is detected. > - Use this flag for requirements of new generic PUNCH+SEEK test: > _require_seek_data_hole -f > _require_xfs_io_command "fpunch" > - For overlayfs specific test, the test will not _require_seek_data_hole -f > but it will instead check real SEEK_HOLE support of base fs and then will > test seek of punched hole with -f flag. Yeah, this looks sane to me. Thanks! Eryu ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] generic/{436,445}: check falloc support 2019-02-26 14:09 [PATCH 1/2] generic/{436,445}: check falloc support Amir Goldstein 2019-02-26 14:09 ` [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole Amir Goldstein @ 2019-03-02 15:26 ` Eryu Guan 1 sibling, 0 replies; 6+ messages in thread From: Eryu Guan @ 2019-03-02 15:26 UTC (permalink / raw) To: Amir Goldstein; +Cc: Miklos Szeredi, Eddie Horng, linux-unionfs, fstests On Tue, Feb 26, 2019 at 04:09:01PM +0200, Amir Goldstein wrote: > The sanity test case in those tests (i.e. 13..17) > are all skipped in fs with no falloc support, but the tests > are reported to pass. > > For example, from 445.full: > > File system supports the default behavior. > File system does not support fallocate. > Allocation size: 4096 > 17. Test file with unwritten extents, data-hole-data inside page > Test skipped as fs doesn't support unwritten extents. > > Explicitly check for falloc support before running those tests > so they would be properly reported as skipped. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> This patch looks fine, I'm taking it for this week's update. Thanks, Eryu ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-02 16:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-26 14:09 [PATCH 1/2] generic/{436,445}: check falloc support Amir Goldstein
2019-02-26 14:09 ` [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole Amir Goldstein
2019-03-02 15:25 ` Eryu Guan
2019-03-02 16:09 ` Amir Goldstein
2019-03-02 16:30 ` Eryu Guan
2019-03-02 15:26 ` [PATCH 1/2] generic/{436,445}: check falloc support Eryu Guan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.