[PATCH 1/2] generic/{436,445}: check falloc support

[PATCH 1/2] generic/{436,445}: check falloc support

[Amir Goldstein]

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

[PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole

[Amir Goldstein]

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

Re: [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole

[Eryu Guan]

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
> 

Re: [PATCH 1/2] generic/{436,445}: check falloc support

[Eryu Guan]

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

Re: [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole

[Amir Goldstein]

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.

Re: [PATCH 2/2] generic: Test that SEEK_HOLE can find a punched hole

[Eryu Guan]

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