* [PATCH v3 1/2] fstests: Add more sanity to seek_sanity_test
@ 2019-04-03 5:59 Amir Goldstein
2019-04-03 5:59 ` [PATCH v3 2/2] generic: Test that SEEK_HOLE can find a punched hole Amir Goldstein
2019-04-03 16:04 ` [PATCH v3 1/2] fstests: Add more sanity to seek_sanity_test Darrick J. Wong
0 siblings, 2 replies; 4+ messages in thread
From: Amir Goldstein @ 2019-04-03 5:59 UTC (permalink / raw)
To: Eryu Guan; +Cc: Miklos Szeredi, Eddie Horng, linux-unionfs, fstests
seek_sanity_test checks for one of several SEEK_DATA/HOLE
behaviors and allows for the default behavior of filesystems,
where SEEK_HOLE always returns EOF.
This means that if filesystem has a regression in finding
holes, the sanity test won't catch it. And indeed this regression
happened in overlayfs on kernel v4.19 and went unnoticed.
To improve test coverage, add a flag -f to seek_sanity_test to
indicate that the default behavior is not acceptable.
Whitelist all filesystem types that are expected to detect holes
and use wrapper when invoking seek_sanity_test to add the -f flag
to those filesystems.
Overlayfs inherits expected behavior from base fs type.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
Eryu,
Sorry for the delay.
Changes per your v2 review.
Amir.
Changes from v2:
- Rename helper to _fstyp_has_non_default_seek_data_hole
- Clarify documentation and error message
- Skip new test (seek to punched hole) on default_behavior
common/rc | 47 ++++++++++++++++++++++++++++++++++++++++++
src/seek_sanity_test.c | 14 +++++++++++--
tests/generic/285 | 2 +-
tests/generic/436 | 2 +-
tests/generic/445 | 2 +-
tests/generic/448 | 2 +-
tests/generic/490 | 2 +-
7 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/common/rc b/common/rc
index 1c42515f..7b292321 100644
--- a/common/rc
+++ b/common/rc
@@ -2298,6 +2298,53 @@ _ext4_disable_extent_zeroout()
echo 0 >/sys/fs/ext4/$sdev/extent_max_zeroout_kb
}
+# The default behavior of SEEK_HOLE is to always return EOF.
+# Filesystems that implement non-default behavior return the offset
+# of holes with SEEK_HOLE. There is no way to query the filesystem
+# of which behavior it is implementing.
+# We use this whitelist FSTYP, to set expectation and avoid silent
+# regression of filesystem seek hole behavior.
+#
+# Return 0 for true
+_fstyp_has_non_default_seek_data_hole()
+{
+ if [ -z $1 ]; then
+ local fstyp=$FSTYP
+ else
+ local fstyp=$1
+ fi
+
+ case "$fstyp" in
+ btrfs|ext4|xfs|ceph|cifs|f2fs|gfs2|nfs*|ocfs2|tmpfs)
+ return 0
+ ;;
+ overlay)
+ if [ ! -z $OVL_BASE_FSTYP -a $OVL_BASE_FSTYP != "overlay" ]; then
+ _fstyp_has_non_default_seek_data_hole $OVL_BASE_FSTYP
+ return $?
+ else
+ # Assume that base fs has default behavior
+ return 1
+ fi
+ ;;
+ *)
+ # by default fstyp has default SEEK_HOLE behavior;
+ # if your fs has non-default behavior, add it to whitelist above!
+ return 1
+ ;;
+ esac
+}
+
+# Run seek sanity test with predefined expectation for SEEK_DATA/HOLE behavior
+_run_seek_sanity_test()
+{
+ local testseekargs
+ if _fstyp_has_non_default_seek_data_hole; then
+ testseekargs+="-f"
+ fi
+ $here/src/seek_sanity_test $testseekargs $*
+}
+
# Check if the file system supports seek_data/hole
_require_seek_data_hole()
{
diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
index e9938d1b..a6bda0b1 100644
--- a/src/seek_sanity_test.c
+++ b/src/seek_sanity_test.c
@@ -23,6 +23,7 @@
#endif
static blksize_t alloc_size;
+int allow_default_behavior = 1;
int default_behavior = 0;
int unwritten_extents = 0;
char *base_file_path;
@@ -1119,6 +1120,12 @@ static int test_basic_support(void)
fprintf(stderr, "File system supports the default behavior.\n");
}
+ if (default_behavior && !allow_default_behavior) {
+ fprintf(stderr, "Default behavior is not allowed. Aborting.\n");
+ ret = -1;
+ goto out;
+ }
+
ftruncate(fd, 0);
if (fallocate(fd, 0, 0, alloc_size) == -1) {
if (errno == EOPNOTSUPP)
@@ -1148,7 +1155,7 @@ out:
void usage(char *cmd)
{
- fprintf(stdout, "Usage: %s [-t] [-s <starttest>] [-e <endtest>] base_file_path\n", cmd);
+ fprintf(stdout, "Usage: %s [-tf] [-s <starttest>] [-e <endtest>] base_file_path\n", cmd);
exit(1);
}
@@ -1169,11 +1176,14 @@ int main(int argc, char **argv)
teststart = 1;
testend = 12;
- while ((opt = getopt(argc, argv, "ts:e:")) != -1) {
+ while ((opt = getopt(argc, argv, "tfs:e:")) != -1) {
switch (opt) {
case 't':
check_support++;
break;
+ case 'f':
+ allow_default_behavior = 0;
+ break;
case 's':
teststart = strtol(optarg, NULL, 10);
if (teststart <= 0 || teststart > numtests) {
diff --git a/tests/generic/285 b/tests/generic/285
index b53b3acc..20e89e5b 100755
--- a/tests/generic/285
+++ b/tests/generic/285
@@ -37,7 +37,7 @@ _cleanup()
eval "rm -f $BASE_TEST_FILE.*"
}
-$here/src/seek_sanity_test $BASE_TEST_FILE > $seqres.full 2>&1 ||
+_run_seek_sanity_test $BASE_TEST_FILE > $seqres.full 2>&1 ||
_fail "seek sanity check failed!"
# success, all done
diff --git a/tests/generic/436 b/tests/generic/436
index a3ef70e5..ada60b2e 100755
--- a/tests/generic/436
+++ b/tests/generic/436
@@ -36,7 +36,7 @@ _cleanup()
rm -f $tmp.* $BASE_TEST_FILE.*
}
-$here/src/seek_sanity_test -s 13 -e 16 $BASE_TEST_FILE > $seqres.full 2>&1 ||
+_run_seek_sanity_test -s 13 -e 16 $BASE_TEST_FILE > $seqres.full 2>&1 ||
_fail "seek sanity check failed!"
# success, all done
diff --git a/tests/generic/445 b/tests/generic/445
index 694913ae..248f321b 100755
--- a/tests/generic/445
+++ b/tests/generic/445
@@ -36,7 +36,7 @@ _cleanup()
rm -f $tmp.* $BASE_TEST_FILE.*
}
-$here/src/seek_sanity_test -s 17 -e 17 $BASE_TEST_FILE > $seqres.full 2>&1 ||
+_run_seek_sanity_test -s 17 -e 17 $BASE_TEST_FILE > $seqres.full 2>&1 ||
_fail "seek sanity check failed!"
# success, all done
diff --git a/tests/generic/448 b/tests/generic/448
index d3ec5528..dada499b 100755
--- a/tests/generic/448
+++ b/tests/generic/448
@@ -34,7 +34,7 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
_require_test_program "seek_sanity_test"
-$here/src/seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE > $seqres.full 2>&1 ||
+_run_seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE > $seqres.full 2>&1 ||
_fail "seek sanity check failed!"
_check_dmesg
diff --git a/tests/generic/490 b/tests/generic/490
index 330c2b60..07b08939 100755
--- a/tests/generic/490
+++ b/tests/generic/490
@@ -39,7 +39,7 @@ _cleanup()
rm -f $base_test_file*
}
-$here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
+_run_seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
_fail "seek sanity check failed!"
# success, all done
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3 2/2] generic: Test that SEEK_HOLE can find a punched hole
2019-04-03 5:59 [PATCH v3 1/2] fstests: Add more sanity to seek_sanity_test Amir Goldstein
@ 2019-04-03 5:59 ` Amir Goldstein
2019-04-03 16:04 ` [PATCH v3 1/2] fstests: Add more sanity to seek_sanity_test Darrick J. Wong
1 sibling, 0 replies; 4+ messages in thread
From: Amir Goldstein @ 2019-04-03 5:59 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.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
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 a6bda0b1..30e996e2 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
@@ -26,6 +27,7 @@ static blksize_t alloc_size;
int allow_default_behavior = 1;
int default_behavior = 0;
int unwritten_extents = 0;
+int punch_hole = 0;
char *base_file_path;
static void get_file_system(int fd)
@@ -118,8 +120,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;
}
@@ -262,6 +265,52 @@ 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;
+ }
+
+ if (default_behavior) {
+ fprintf(stdout, "Test skipped as fs doesn't support seeking a punched 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
@@ -1050,6 +1099,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)
@@ -1127,15 +1177,20 @@ 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 {
+ 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..d8565041
--- /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*
+}
+
+_run_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 c63035ce..f85e416c 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -540,3 +540,4 @@
535 auto quick log
536 auto quick rw shutdown
537 auto quick trim
+999 auto quick punch seek
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 1/2] fstests: Add more sanity to seek_sanity_test
2019-04-03 5:59 [PATCH v3 1/2] fstests: Add more sanity to seek_sanity_test Amir Goldstein
2019-04-03 5:59 ` [PATCH v3 2/2] generic: Test that SEEK_HOLE can find a punched hole Amir Goldstein
@ 2019-04-03 16:04 ` Darrick J. Wong
2019-04-03 16:22 ` Amir Goldstein
1 sibling, 1 reply; 4+ messages in thread
From: Darrick J. Wong @ 2019-04-03 16:04 UTC (permalink / raw)
To: Amir Goldstein
Cc: Eryu Guan, Miklos Szeredi, Eddie Horng, linux-unionfs, fstests
On Wed, Apr 03, 2019 at 08:59:25AM +0300, Amir Goldstein wrote:
> seek_sanity_test checks for one of several SEEK_DATA/HOLE
> behaviors and allows for the default behavior of filesystems,
> where SEEK_HOLE always returns EOF.
>
> This means that if filesystem has a regression in finding
> holes, the sanity test won't catch it. And indeed this regression
> happened in overlayfs on kernel v4.19 and went unnoticed.
>
> To improve test coverage, add a flag -f to seek_sanity_test to
> indicate that the default behavior is not acceptable.
> Whitelist all filesystem types that are expected to detect holes
> and use wrapper when invoking seek_sanity_test to add the -f flag
> to those filesystems.
>
> Overlayfs inherits expected behavior from base fs type.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Eryu,
>
> Sorry for the delay.
> Changes per your v2 review.
>
> Amir.
>
> Changes from v2:
> - Rename helper to _fstyp_has_non_default_seek_data_hole
> - Clarify documentation and error message
> - Skip new test (seek to punched hole) on default_behavior
>
>
> common/rc | 47 ++++++++++++++++++++++++++++++++++++++++++
> src/seek_sanity_test.c | 14 +++++++++++--
> tests/generic/285 | 2 +-
> tests/generic/436 | 2 +-
> tests/generic/445 | 2 +-
> tests/generic/448 | 2 +-
> tests/generic/490 | 2 +-
> 7 files changed, 64 insertions(+), 7 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index 1c42515f..7b292321 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2298,6 +2298,53 @@ _ext4_disable_extent_zeroout()
> echo 0 >/sys/fs/ext4/$sdev/extent_max_zeroout_kb
> }
>
> +# The default behavior of SEEK_HOLE is to always return EOF.
I assume this statement applies even when there /is/ a hole in the file?
As in, "Some filesystems return EOF even when there are holes", right?
I wonder if there's a good way to fix SEEK_HOLE for those filesystems.
> +# Filesystems that implement non-default behavior return the offset
...except for the magic filesystems below that actually return holes?
I suppose that works, since even the default behavior tells no
falsehoods w.r.t. where might there data in a file.
> +# of holes with SEEK_HOLE. There is no way to query the filesystem
> +# of which behavior it is implementing.
> +# We use this whitelist FSTYP, to set expectation and avoid silent
> +# regression of filesystem seek hole behavior.
> +#
> +# Return 0 for true
> +_fstyp_has_non_default_seek_data_hole()
> +{
> + if [ -z $1 ]; then
> + local fstyp=$FSTYP
> + else
> + local fstyp=$1
> + fi
> +
> + case "$fstyp" in
> + btrfs|ext4|xfs|ceph|cifs|f2fs|gfs2|nfs*|ocfs2|tmpfs)
Not a fan of whitelists, but I don't see a better way to do this.
Acked-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> + return 0
> + ;;
> + overlay)
> + if [ ! -z $OVL_BASE_FSTYP -a $OVL_BASE_FSTYP != "overlay" ]; then
> + _fstyp_has_non_default_seek_data_hole $OVL_BASE_FSTYP
> + return $?
> + else
> + # Assume that base fs has default behavior
> + return 1
> + fi
> + ;;
> + *)
> + # by default fstyp has default SEEK_HOLE behavior;
> + # if your fs has non-default behavior, add it to whitelist above!
> + return 1
> + ;;
> + esac
> +}
> +
> +# Run seek sanity test with predefined expectation for SEEK_DATA/HOLE behavior
> +_run_seek_sanity_test()
> +{
> + local testseekargs
> + if _fstyp_has_non_default_seek_data_hole; then
> + testseekargs+="-f"
> + fi
> + $here/src/seek_sanity_test $testseekargs $*
> +}
> +
> # Check if the file system supports seek_data/hole
> _require_seek_data_hole()
> {
> diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
> index e9938d1b..a6bda0b1 100644
> --- a/src/seek_sanity_test.c
> +++ b/src/seek_sanity_test.c
> @@ -23,6 +23,7 @@
> #endif
>
> static blksize_t alloc_size;
> +int allow_default_behavior = 1;
> int default_behavior = 0;
> int unwritten_extents = 0;
> char *base_file_path;
> @@ -1119,6 +1120,12 @@ static int test_basic_support(void)
> fprintf(stderr, "File system supports the default behavior.\n");
> }
>
> + if (default_behavior && !allow_default_behavior) {
> + fprintf(stderr, "Default behavior is not allowed. Aborting.\n");
> + ret = -1;
> + goto out;
> + }
> +
> ftruncate(fd, 0);
> if (fallocate(fd, 0, 0, alloc_size) == -1) {
> if (errno == EOPNOTSUPP)
> @@ -1148,7 +1155,7 @@ out:
>
> void usage(char *cmd)
> {
> - fprintf(stdout, "Usage: %s [-t] [-s <starttest>] [-e <endtest>] base_file_path\n", cmd);
> + fprintf(stdout, "Usage: %s [-tf] [-s <starttest>] [-e <endtest>] base_file_path\n", cmd);
> exit(1);
> }
>
> @@ -1169,11 +1176,14 @@ int main(int argc, char **argv)
> teststart = 1;
> testend = 12;
>
> - while ((opt = getopt(argc, argv, "ts:e:")) != -1) {
> + while ((opt = getopt(argc, argv, "tfs:e:")) != -1) {
> switch (opt) {
> case 't':
> check_support++;
> break;
> + case 'f':
> + allow_default_behavior = 0;
> + break;
> case 's':
> teststart = strtol(optarg, NULL, 10);
> if (teststart <= 0 || teststart > numtests) {
> diff --git a/tests/generic/285 b/tests/generic/285
> index b53b3acc..20e89e5b 100755
> --- a/tests/generic/285
> +++ b/tests/generic/285
> @@ -37,7 +37,7 @@ _cleanup()
> eval "rm -f $BASE_TEST_FILE.*"
> }
>
> -$here/src/seek_sanity_test $BASE_TEST_FILE > $seqres.full 2>&1 ||
> +_run_seek_sanity_test $BASE_TEST_FILE > $seqres.full 2>&1 ||
> _fail "seek sanity check failed!"
>
> # success, all done
> diff --git a/tests/generic/436 b/tests/generic/436
> index a3ef70e5..ada60b2e 100755
> --- a/tests/generic/436
> +++ b/tests/generic/436
> @@ -36,7 +36,7 @@ _cleanup()
> rm -f $tmp.* $BASE_TEST_FILE.*
> }
>
> -$here/src/seek_sanity_test -s 13 -e 16 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> +_run_seek_sanity_test -s 13 -e 16 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> _fail "seek sanity check failed!"
>
> # success, all done
> diff --git a/tests/generic/445 b/tests/generic/445
> index 694913ae..248f321b 100755
> --- a/tests/generic/445
> +++ b/tests/generic/445
> @@ -36,7 +36,7 @@ _cleanup()
> rm -f $tmp.* $BASE_TEST_FILE.*
> }
>
> -$here/src/seek_sanity_test -s 17 -e 17 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> +_run_seek_sanity_test -s 17 -e 17 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> _fail "seek sanity check failed!"
>
> # success, all done
> diff --git a/tests/generic/448 b/tests/generic/448
> index d3ec5528..dada499b 100755
> --- a/tests/generic/448
> +++ b/tests/generic/448
> @@ -34,7 +34,7 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
>
> _require_test_program "seek_sanity_test"
>
> -$here/src/seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> +_run_seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> _fail "seek sanity check failed!"
>
> _check_dmesg
> diff --git a/tests/generic/490 b/tests/generic/490
> index 330c2b60..07b08939 100755
> --- a/tests/generic/490
> +++ b/tests/generic/490
> @@ -39,7 +39,7 @@ _cleanup()
> rm -f $base_test_file*
> }
>
> -$here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> +_run_seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> _fail "seek sanity check failed!"
>
> # success, all done
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 1/2] fstests: Add more sanity to seek_sanity_test
2019-04-03 16:04 ` [PATCH v3 1/2] fstests: Add more sanity to seek_sanity_test Darrick J. Wong
@ 2019-04-03 16:22 ` Amir Goldstein
0 siblings, 0 replies; 4+ messages in thread
From: Amir Goldstein @ 2019-04-03 16:22 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Eryu Guan, Miklos Szeredi, Eddie Horng, overlayfs, fstests
On Wed, Apr 3, 2019 at 7:06 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Wed, Apr 03, 2019 at 08:59:25AM +0300, Amir Goldstein wrote:
> > seek_sanity_test checks for one of several SEEK_DATA/HOLE
> > behaviors and allows for the default behavior of filesystems,
> > where SEEK_HOLE always returns EOF.
> >
> > This means that if filesystem has a regression in finding
> > holes, the sanity test won't catch it. And indeed this regression
> > happened in overlayfs on kernel v4.19 and went unnoticed.
> >
> > To improve test coverage, add a flag -f to seek_sanity_test to
> > indicate that the default behavior is not acceptable.
> > Whitelist all filesystem types that are expected to detect holes
> > and use wrapper when invoking seek_sanity_test to add the -f flag
> > to those filesystems.
> >
> > Overlayfs inherits expected behavior from base fs type.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Eryu,
> >
> > Sorry for the delay.
> > Changes per your v2 review.
> >
> > Amir.
> >
> > Changes from v2:
> > - Rename helper to _fstyp_has_non_default_seek_data_hole
> > - Clarify documentation and error message
> > - Skip new test (seek to punched hole) on default_behavior
> >
> >
> > common/rc | 47 ++++++++++++++++++++++++++++++++++++++++++
> > src/seek_sanity_test.c | 14 +++++++++++--
> > tests/generic/285 | 2 +-
> > tests/generic/436 | 2 +-
> > tests/generic/445 | 2 +-
> > tests/generic/448 | 2 +-
> > tests/generic/490 | 2 +-
> > 7 files changed, 64 insertions(+), 7 deletions(-)
> >
> > diff --git a/common/rc b/common/rc
> > index 1c42515f..7b292321 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2298,6 +2298,53 @@ _ext4_disable_extent_zeroout()
> > echo 0 >/sys/fs/ext4/$sdev/extent_max_zeroout_kb
> > }
> >
> > +# The default behavior of SEEK_HOLE is to always return EOF.
>
> I assume this statement applies even when there /is/ a hole in the file?
>
> As in, "Some filesystems return EOF even when there are holes", right?
Right.
>
> I wonder if there's a good way to fix SEEK_HOLE for those filesystems.
>
> > +# Filesystems that implement non-default behavior return the offset
>
> ...except for the magic filesystems below that actually return holes?
Yes.
>
> I suppose that works, since even the default behavior tells no
> falsehoods w.r.t. where might there data in a file.
>
> > +# of holes with SEEK_HOLE. There is no way to query the filesystem
> > +# of which behavior it is implementing.
> > +# We use this whitelist FSTYP, to set expectation and avoid silent
> > +# regression of filesystem seek hole behavior.
> > +#
> > +# Return 0 for true
> > +_fstyp_has_non_default_seek_data_hole()
> > +{
> > + if [ -z $1 ]; then
> > + local fstyp=$FSTYP
> > + else
> > + local fstyp=$1
> > + fi
> > +
> > + case "$fstyp" in
> > + btrfs|ext4|xfs|ceph|cifs|f2fs|gfs2|nfs*|ocfs2|tmpfs)
>
> Not a fan of whitelists, but I don't see a better way to do this.
>
> Acked-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> --D
>
> > + return 0
> > + ;;
> > + overlay)
> > + if [ ! -z $OVL_BASE_FSTYP -a $OVL_BASE_FSTYP != "overlay" ]; then
> > + _fstyp_has_non_default_seek_data_hole $OVL_BASE_FSTYP
> > + return $?
> > + else
> > + # Assume that base fs has default behavior
> > + return 1
> > + fi
> > + ;;
> > + *)
> > + # by default fstyp has default SEEK_HOLE behavior;
> > + # if your fs has non-default behavior, add it to whitelist above!
> > + return 1
> > + ;;
> > + esac
> > +}
> > +
> > +# Run seek sanity test with predefined expectation for SEEK_DATA/HOLE behavior
> > +_run_seek_sanity_test()
> > +{
> > + local testseekargs
> > + if _fstyp_has_non_default_seek_data_hole; then
> > + testseekargs+="-f"
> > + fi
> > + $here/src/seek_sanity_test $testseekargs $*
> > +}
> > +
> > # Check if the file system supports seek_data/hole
> > _require_seek_data_hole()
> > {
> > diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
> > index e9938d1b..a6bda0b1 100644
> > --- a/src/seek_sanity_test.c
> > +++ b/src/seek_sanity_test.c
> > @@ -23,6 +23,7 @@
> > #endif
> >
> > static blksize_t alloc_size;
> > +int allow_default_behavior = 1;
> > int default_behavior = 0;
> > int unwritten_extents = 0;
> > char *base_file_path;
> > @@ -1119,6 +1120,12 @@ static int test_basic_support(void)
> > fprintf(stderr, "File system supports the default behavior.\n");
> > }
> >
> > + if (default_behavior && !allow_default_behavior) {
> > + fprintf(stderr, "Default behavior is not allowed. Aborting.\n");
> > + ret = -1;
> > + goto out;
> > + }
> > +
> > ftruncate(fd, 0);
> > if (fallocate(fd, 0, 0, alloc_size) == -1) {
> > if (errno == EOPNOTSUPP)
> > @@ -1148,7 +1155,7 @@ out:
> >
> > void usage(char *cmd)
> > {
> > - fprintf(stdout, "Usage: %s [-t] [-s <starttest>] [-e <endtest>] base_file_path\n", cmd);
> > + fprintf(stdout, "Usage: %s [-tf] [-s <starttest>] [-e <endtest>] base_file_path\n", cmd);
> > exit(1);
> > }
> >
> > @@ -1169,11 +1176,14 @@ int main(int argc, char **argv)
> > teststart = 1;
> > testend = 12;
> >
> > - while ((opt = getopt(argc, argv, "ts:e:")) != -1) {
> > + while ((opt = getopt(argc, argv, "tfs:e:")) != -1) {
> > switch (opt) {
> > case 't':
> > check_support++;
> > break;
> > + case 'f':
> > + allow_default_behavior = 0;
> > + break;
> > case 's':
> > teststart = strtol(optarg, NULL, 10);
> > if (teststart <= 0 || teststart > numtests) {
> > diff --git a/tests/generic/285 b/tests/generic/285
> > index b53b3acc..20e89e5b 100755
> > --- a/tests/generic/285
> > +++ b/tests/generic/285
> > @@ -37,7 +37,7 @@ _cleanup()
> > eval "rm -f $BASE_TEST_FILE.*"
> > }
> >
> > -$here/src/seek_sanity_test $BASE_TEST_FILE > $seqres.full 2>&1 ||
> > +_run_seek_sanity_test $BASE_TEST_FILE > $seqres.full 2>&1 ||
> > _fail "seek sanity check failed!"
> >
> > # success, all done
> > diff --git a/tests/generic/436 b/tests/generic/436
> > index a3ef70e5..ada60b2e 100755
> > --- a/tests/generic/436
> > +++ b/tests/generic/436
> > @@ -36,7 +36,7 @@ _cleanup()
> > rm -f $tmp.* $BASE_TEST_FILE.*
> > }
> >
> > -$here/src/seek_sanity_test -s 13 -e 16 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> > +_run_seek_sanity_test -s 13 -e 16 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> > _fail "seek sanity check failed!"
> >
> > # success, all done
> > diff --git a/tests/generic/445 b/tests/generic/445
> > index 694913ae..248f321b 100755
> > --- a/tests/generic/445
> > +++ b/tests/generic/445
> > @@ -36,7 +36,7 @@ _cleanup()
> > rm -f $tmp.* $BASE_TEST_FILE.*
> > }
> >
> > -$here/src/seek_sanity_test -s 17 -e 17 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> > +_run_seek_sanity_test -s 17 -e 17 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> > _fail "seek sanity check failed!"
> >
> > # success, all done
> > diff --git a/tests/generic/448 b/tests/generic/448
> > index d3ec5528..dada499b 100755
> > --- a/tests/generic/448
> > +++ b/tests/generic/448
> > @@ -34,7 +34,7 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
> >
> > _require_test_program "seek_sanity_test"
> >
> > -$here/src/seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> > +_run_seek_sanity_test -s 18 -e 18 $BASE_TEST_FILE > $seqres.full 2>&1 ||
> > _fail "seek sanity check failed!"
> >
> > _check_dmesg
> > diff --git a/tests/generic/490 b/tests/generic/490
> > index 330c2b60..07b08939 100755
> > --- a/tests/generic/490
> > +++ b/tests/generic/490
> > @@ -39,7 +39,7 @@ _cleanup()
> > rm -f $base_test_file*
> > }
> >
> > -$here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> > +_run_seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> > _fail "seek sanity check failed!"
> >
> > # success, all done
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-04-03 16:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-03 5:59 [PATCH v3 1/2] fstests: Add more sanity to seek_sanity_test Amir Goldstein
2019-04-03 5:59 ` [PATCH v3 2/2] generic: Test that SEEK_HOLE can find a punched hole Amir Goldstein
2019-04-03 16:04 ` [PATCH v3 1/2] fstests: Add more sanity to seek_sanity_test Darrick J. Wong
2019-04-03 16:22 ` Amir Goldstein
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.