[PATCH v2 0/4] fstests/fsx: test coverage for eof zeroing

[PATCH v2 0/4] fstests/fsx: test coverage for eof zeroing

[Brian Foster]

Hi all,

Here's v2 of the patches for improved test coverage (via fsx) for EOF
related zeroing. The most notable change is that the discussion on v1
uncovered that some of the existing fsx behavior was wrong wrt to
simulated ops, so patch 1 is factored out as a standalone bug fix to
address that.

Brian

v2:
- Factored out patch 1 to fix simulation mode.
- Use MAP_FAILED, don't inject data for simulated ops.
- Rebase to latest master and renumber test.
- Use run_fsx and -S 0 by default (timestamp seed).
v1: https://lore.kernel.org/fstests/20240822144422.188462-1-bfoster@redhat.com/

Brian Foster (4):
  fsx: don't skip file size and buf updates on simulated ops
  fsx: factor out a file size update helper
  fsx: support eof page pollution for eof zeroing test coverage
  generic: test to run fsx eof pollution

 ltp/fsx.c             | 134 ++++++++++++++++++++++++++++++++----------
 tests/generic/363     |  23 ++++++++
 tests/generic/363.out |   2 +
 3 files changed, 127 insertions(+), 32 deletions(-)
 create mode 100755 tests/generic/363
 create mode 100644 tests/generic/363.out

-- 
2.45.0

[PATCH v2 1/4] fsx: don't skip file size and buf updates on simulated ops

[Brian Foster]

fsx supports the ability to skip through a certain number of
operations of a given command sequence before beginning full
operation. The way this works is by tracking the operation count,
simulating minimal side effects of skipped operations in-memory, and
then finally writing out the in-memory state to the target file when
full operation begins.

Several fallocate() related operations don't correctly track
in-memory state when simulated, however. For example, consider an
ops file with the following two operations:

  zero_range 0x0 0x1000 0x0
  read 0x0 0x1000 0x0

... and an fsx run like so:

  fsx -d -b 2 --replay-ops=<opsfile> <file>

This simulates the zero_range operation, but fails to track the file
extension that occurs as a side effect such that the subsequent read
doesn't occur as expected:

  Will begin at operation 2
  skipping zero size read

The read is skipped in this case because the file size is zero.  The
proper behavior, and what is consistent with other size changing
operations, is to make the appropriate in-core changes before
checking whether an operation is simulated so the end result of
those changes can be reflected on-disk for eventual non-simulated
operations. This results in expected behavior with the same ops file
and test command:

  Will begin at operation 2
  2 read  0x0 thru        0xfff   (0x1000 bytes)

Update zero, copy and clone range to do the file size and EOF change
related zeroing before checking against the simulated ops count.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 ltp/fsx.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index 2dc59b06..c5727cff 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -1247,6 +1247,17 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
 	log4(OP_ZERO_RANGE, offset, length,
 	     keep_size ? FL_KEEP_SIZE : FL_NONE);
 
+	if (!keep_size && end_offset > file_size) {
+		/*
+		 * If there's a gap between the old file size and the offset of
+		 * the zero range operation, fill the gap with zeroes.
+		 */
+		if (offset > file_size)
+			memset(good_buf + file_size, '\0', offset - file_size);
+
+		file_size = end_offset;
+	}
+
 	if (testcalls <= simulatedopcount)
 		return;
 
@@ -1263,17 +1274,6 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
 	}
 
 	memset(good_buf + offset, '\0', length);
-
-	if (!keep_size && end_offset > file_size) {
-		/*
-		 * If there's a gap between the old file size and the offset of
-		 * the zero range operation, fill the gap with zeroes.
-		 */
-		if (offset > file_size)
-			memset(good_buf + file_size, '\0', offset - file_size);
-
-		file_size = end_offset;
-	}
 }
 
 #else
@@ -1538,6 +1538,11 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
 
 	log5(OP_CLONE_RANGE, offset, length, dest, FL_NONE);
 
+	if (dest > file_size)
+		memset(good_buf + file_size, '\0', dest - file_size);
+	if (dest + length > file_size)
+		file_size = dest + length;
+
 	if (testcalls <= simulatedopcount)
 		return;
 
@@ -1556,10 +1561,6 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
 	}
 
 	memcpy(good_buf + dest, good_buf + offset, length);
-	if (dest > file_size)
-		memset(good_buf + file_size, '\0', dest - file_size);
-	if (dest + length > file_size)
-		file_size = dest + length;
 }
 
 #else
@@ -1756,6 +1757,11 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
 
 	log5(OP_COPY_RANGE, offset, length, dest, FL_NONE);
 
+	if (dest > file_size)
+		memset(good_buf + file_size, '\0', dest - file_size);
+	if (dest + length > file_size)
+		file_size = dest + length;
+
 	if (testcalls <= simulatedopcount)
 		return;
 
@@ -1792,10 +1798,6 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
 	}
 
 	memcpy(good_buf + dest, good_buf + offset, length);
-	if (dest > file_size)
-		memset(good_buf + file_size, '\0', dest - file_size);
-	if (dest + length > file_size)
-		file_size = dest + length;
 }
 
 #else
-- 
2.45.0

[PATCH v2 2/4] fsx: factor out a file size update helper

[Brian Foster]

In preparation for support for eof page pollution, factor out a file
size update helper. This updates the internally tracked file size
based on the upcoming operation and zeroes the appropriate range in
the good buffer for extending operations.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 ltp/fsx.c | 49 +++++++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index c5727cff..4a9be0e1 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -983,6 +983,17 @@ gendata(char *original_buf, char *good_buf, unsigned offset, unsigned size)
 	}
 }
 
+/*
+ * Helper to update the tracked file size. If the offset begins beyond current
+ * EOF, zero the range from EOF to offset in the good buffer.
+ */
+void
+update_file_size(unsigned offset, unsigned size)
+{
+	if (offset > file_size)
+		memset(good_buf + file_size, '\0', offset - file_size);
+	file_size = offset + size;
+}
 
 void
 dowrite(unsigned offset, unsigned size)
@@ -1003,10 +1014,8 @@ dowrite(unsigned offset, unsigned size)
 	log4(OP_WRITE, offset, size, FL_NONE);
 
 	gendata(original_buf, good_buf, offset, size);
-	if (file_size < offset + size) {
-		if (file_size < offset)
-			memset(good_buf + file_size, '\0', offset - file_size);
-		file_size = offset + size;
+	if (offset + size > file_size) {
+		update_file_size(offset, size);
 		if (lite) {
 			warn("Lite file size bug in fsx!");
 			report_failure(149);
@@ -1070,10 +1079,8 @@ domapwrite(unsigned offset, unsigned size)
 	log4(OP_MAPWRITE, offset, size, FL_NONE);
 
 	gendata(original_buf, good_buf, offset, size);
-	if (file_size < offset + size) {
-		if (file_size < offset)
-			memset(good_buf + file_size, '\0', offset - file_size);
-		file_size = offset + size;
+	if (offset + size > file_size) {
+		update_file_size(offset, size);
 		if (lite) {
 			warn("Lite file size bug in fsx!");
 			report_failure(200);
@@ -1136,9 +1143,7 @@ dotruncate(unsigned size)
 
 	log4(OP_TRUNCATE, 0, size, FL_NONE);
 
-	if (size > file_size)
-		memset(good_buf + file_size, '\0', size - file_size);
-	file_size = size;
+	update_file_size(size, 0);
 
 	if (testcalls <= simulatedopcount)
 		return;
@@ -1247,16 +1252,8 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
 	log4(OP_ZERO_RANGE, offset, length,
 	     keep_size ? FL_KEEP_SIZE : FL_NONE);
 
-	if (!keep_size && end_offset > file_size) {
-		/*
-		 * If there's a gap between the old file size and the offset of
-		 * the zero range operation, fill the gap with zeroes.
-		 */
-		if (offset > file_size)
-			memset(good_buf + file_size, '\0', offset - file_size);
-
-		file_size = end_offset;
-	}
+	if (!keep_size && end_offset > file_size)
+		update_file_size(offset, length);
 
 	if (testcalls <= simulatedopcount)
 		return;
@@ -1538,10 +1535,8 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
 
 	log5(OP_CLONE_RANGE, offset, length, dest, FL_NONE);
 
-	if (dest > file_size)
-		memset(good_buf + file_size, '\0', dest - file_size);
 	if (dest + length > file_size)
-		file_size = dest + length;
+		update_file_size(dest, length);
 
 	if (testcalls <= simulatedopcount)
 		return;
@@ -1757,10 +1752,8 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
 
 	log5(OP_COPY_RANGE, offset, length, dest, FL_NONE);
 
-	if (dest > file_size)
-		memset(good_buf + file_size, '\0', dest - file_size);
 	if (dest + length > file_size)
-		file_size = dest + length;
+		update_file_size(dest, length);
 
 	if (testcalls <= simulatedopcount)
 		return;
@@ -1848,7 +1841,7 @@ do_preallocate(unsigned offset, unsigned length, int keep_size)
 
 	if (end_offset > file_size) {
 		memset(good_buf + file_size, '\0', end_offset - file_size);
-		file_size = end_offset;
+		update_file_size(offset, length);
 	}
 
 	if (testcalls <= simulatedopcount)
-- 
2.45.0

[PATCH v2 3/4] fsx: support eof page pollution for eof zeroing test coverage

[Brian Foster]

File ranges that are newly exposed via size changing operations are
expected to return zeroes until written to. This behavior tends to
be difficult to regression test as failures can be racy and
transient. fsx is probably the best tool for this type of test
coverage, but uncovering issues can require running for a
significantly longer period of time than is typically invoked
through fstests tests. As a result, these types of regressions tend
to go unnoticed for an unfortunate amount of time.

To facilitate uncovering these problems more quickly, implement an
eof pollution mode in fsx that opportunistically injects post-eof
data prior to operations that change file size. Since data injection
occurs immediately before the size changing operation, it can be
used to detect problems in partial eof page/block zeroing associated
with each relevant operation.

The implementation takes advantage of the fact that mapped writes
can place data beyond eof so long as the page starts within eof. The
main reason for the isolated per-operation approach (vs. something
like allowing mapped writes to write beyond eof, for example) is to
accommodate the fact that writeback zeroes post-eof data on the eof
page. The current approach is therefore not necessarily guaranteed
to detect all problems, but provides more generic and broad test
coverage than the alternative of testing explicit command sequences
and doesn't require significant changes to how fsx works. If this
proves useful long term, further enhancements can be considered that
might facilitate the presence of post-eof data across operations.

Enable the feature with the -e command line option. It is disabled
by default because zeroing behavior is inconsistent across
filesystems. This can also be revisited in the future if zeroing
behavior is refined for the major filesystems that rely on fstests
for regression testing.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 ltp/fsx.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 2 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index 4a9be0e1..1ba1bf65 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -178,6 +178,7 @@ int	dedupe_range_calls = 1;		/* -B flag disables */
 int	copy_range_calls = 1;		/* -E flag disables */
 int	exchange_range_calls = 1;	/* -0 flag disables */
 int	integrity = 0;			/* -i flag */
+int	pollute_eof = 0;		/* -e flag */
 int	fsxgoodfd = 0;
 int	o_direct;			/* -Z */
 int	aio = 0;
@@ -983,6 +984,63 @@ gendata(char *original_buf, char *good_buf, unsigned offset, unsigned size)
 	}
 }
 
+/*
+ * Pollute the EOF page with data beyond EOF prior to size change operations.
+ * This provides additional test coverage for partial EOF block/page zeroing.
+ * If the upcoming operation does not correctly zero, incorrect file data will
+ * be detected.
+ */
+void
+pollute_eofpage(unsigned int maxoff)
+{
+	unsigned offset = file_size;
+	unsigned pg_offset;
+	unsigned write_size;
+	char    *p;
+
+	/*
+	 * Nothing to do if pollution disabled or we're simulating. Simulation
+	 * only tracks file size updates and skips syscalls, so we don't want to
+	 * inject file data that won't be zeroed.
+	 */
+	if (!pollute_eof || testcalls <= simulatedopcount)
+		return;
+
+	/* write up to specified max or the end of the eof page */
+	pg_offset = offset & mmap_mask;
+	write_size = MIN(PAGE_SIZE - pg_offset, maxoff - offset);
+
+	if (!pg_offset)
+		return;
+
+	if (!quiet &&
+	    ((progressinterval && testcalls % progressinterval == 0) ||
+	    (debug &&
+	     (monitorstart == -1 ||
+	     (offset + write_size > monitorstart &&
+	      (monitorend == -1 || offset <= monitorend)))))) {
+		prt("%lld pollute_eof\t0x%x thru\t0x%x\t(0x%x bytes)\n",
+			testcalls, offset, offset + write_size - 1, write_size);
+	}
+
+	if ((p = (char *)mmap(0, PAGE_SIZE, PROT_READ | PROT_WRITE,
+			      MAP_FILE | MAP_SHARED, fd,
+			      (off_t)(offset - pg_offset))) == MAP_FAILED) {
+		prterr("pollute_eofpage: mmap");
+		return;
+	}
+
+	/*
+	 * Write to a range just past EOF of the test file. Do not update the
+	 * good buffer because the upcoming operation is expected to zero this
+	 * range of the file.
+	 */
+	gendata(original_buf, p, pg_offset, write_size);
+
+	if (munmap(p, PAGE_SIZE) != 0)
+		prterr("pollute_eofpage: munmap");
+}
+
 /*
  * Helper to update the tracked file size. If the offset begins beyond current
  * EOF, zero the range from EOF to offset in the good buffer.
@@ -990,8 +1048,10 @@ gendata(char *original_buf, char *good_buf, unsigned offset, unsigned size)
 void
 update_file_size(unsigned offset, unsigned size)
 {
-	if (offset > file_size)
+	if (offset > file_size) {
+		pollute_eofpage(offset + size);
 		memset(good_buf + file_size, '\0', offset - file_size);
+	}
 	file_size = offset + size;
 }
 
@@ -1143,6 +1203,9 @@ dotruncate(unsigned size)
 
 	log4(OP_TRUNCATE, 0, size, FL_NONE);
 
+	/* pollute the current EOF before a truncate down */
+	if (size < file_size)
+		pollute_eofpage(maxfilelen);
 	update_file_size(size, 0);
 
 	if (testcalls <= simulatedopcount)
@@ -1305,6 +1368,9 @@ do_collapse_range(unsigned offset, unsigned length)
 
 	log4(OP_COLLAPSE_RANGE, offset, length, FL_NONE);
 
+	/* pollute current eof before collapse truncates down */
+	pollute_eofpage(maxfilelen);
+
 	if (testcalls <= simulatedopcount)
 		return;
 
@@ -1356,6 +1422,9 @@ do_insert_range(unsigned offset, unsigned length)
 
 	log4(OP_INSERT_RANGE, offset, length, FL_NONE);
 
+	/* pollute current eof before insert truncates up */
+	pollute_eofpage(maxfilelen);
+
 	if (testcalls <= simulatedopcount)
 		return;
 
@@ -2385,6 +2454,7 @@ usage(void)
 	-b opnum: beginning operation number (default 1)\n\
 	-c P: 1 in P chance of file close+open at each op (default infinity)\n\
 	-d: debug output for all operations\n\
+	-e: pollute post-eof on size changes (default 0)\n\
 	-f: flush and invalidate cache after I/O\n\
 	-g X: write character X instead of random generated data\n\
 	-i logdev: do integrity testing, logdev is the dm log writes device\n\
@@ -2783,7 +2853,7 @@ main(int argc, char **argv)
 	setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
 
 	while ((ch = getopt_long(argc, argv,
-				 "0b:c:dfg:i:j:kl:m:no:p:qr:s:t:w:xyABD:EFJKHzCILN:OP:RS:UWXZ",
+				 "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:w:xyABD:EFJKHzCILN:OP:RS:UWXZ",
 				 longopts, NULL)) != EOF)
 		switch (ch) {
 		case 'b':
@@ -2805,6 +2875,11 @@ main(int argc, char **argv)
 		case 'd':
 			debug = 1;
 			break;
+		case 'e':
+			pollute_eof = getnum(optarg, &endp);
+			if (pollute_eof < 0 || pollute_eof > 1)
+				usage();
+			break;
 		case 'f':
 			flush = 1;
 			break;
-- 
2.45.0

[PATCH v2 4/4] generic: test to run fsx eof pollution

[Brian Foster]

Filesystem regressions related to partial page zeroing can go
unnoticed for a decent amount of time. A recent example is the issue
of iomap zero range not handling dirty pagecache over unwritten
extents, which leads to wrong behavior on certain file extending
operations (i.e. truncate, write extension, etc.).

fsx does occasionally uncover these sorts of problems, but failures
can be rare and/or require longer running tests outside what is
typically run via full fstests regression runs. fsx now supports a
mode that injects post-eof data in order to explicitly test partial
eof zeroing behavior. This uncovers certain problems more quickly
and applies coverage more broadly across size changing operations.

Add a new test that runs an fsx instance (modeled after generic/127)
with eof pollution mode enabled. While the test is generic, it is
currently limited to XFS as that is currently the only known major
fs that does enough zeroing to satisfy the strict semantics expected
by fsx. The long term goal is to uncover and fix issues so more
filesystems can enable this test.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 tests/generic/363     | 23 +++++++++++++++++++++++
 tests/generic/363.out |  2 ++
 2 files changed, 25 insertions(+)
 create mode 100755 tests/generic/363
 create mode 100644 tests/generic/363.out

diff --git a/tests/generic/363 b/tests/generic/363
new file mode 100755
index 00000000..477c111c
--- /dev/null
+++ b/tests/generic/363
@@ -0,0 +1,23 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2024 Red Hat, Inc.  All Rights Reserved.
+#
+# FSQA Test No. 363
+#
+# Run fsx with EOF pollution enabled. This provides test coverage for partial
+# EOF page/block zeroing for operations that change file size.
+#
+
+. ./common/preamble
+_begin_fstest rw auto
+
+_require_test
+
+# currently only xfs performs enough zeroing to satisfy fsx
+_supported_fs xfs
+
+# on failure, replace -q with -d to see post-eof writes in the dump output
+run_fsx "-q -S 0 -e 1 -N 100000"
+
+status=0
+exit
diff --git a/tests/generic/363.out b/tests/generic/363.out
new file mode 100644
index 00000000..3d219cd0
--- /dev/null
+++ b/tests/generic/363.out
@@ -0,0 +1,2 @@
+QA output created by 363
+fsx -q -S 0 -e 1 -N 100000
-- 
2.45.0

Re: [PATCH v2 1/4] fsx: don't skip file size and buf updates on simulated ops

[Darrick J. Wong]

On Wed, Aug 28, 2024 at 02:15:31PM -0400, Brian Foster wrote:
> fsx supports the ability to skip through a certain number of
> operations of a given command sequence before beginning full
> operation. The way this works is by tracking the operation count,
> simulating minimal side effects of skipped operations in-memory, and
> then finally writing out the in-memory state to the target file when
> full operation begins.
> 
> Several fallocate() related operations don't correctly track
> in-memory state when simulated, however. For example, consider an
> ops file with the following two operations:
> 
>   zero_range 0x0 0x1000 0x0
>   read 0x0 0x1000 0x0
> 
> ... and an fsx run like so:
> 
>   fsx -d -b 2 --replay-ops=<opsfile> <file>
> 
> This simulates the zero_range operation, but fails to track the file
> extension that occurs as a side effect such that the subsequent read
> doesn't occur as expected:
> 
>   Will begin at operation 2
>   skipping zero size read
> 
> The read is skipped in this case because the file size is zero.  The
> proper behavior, and what is consistent with other size changing
> operations, is to make the appropriate in-core changes before
> checking whether an operation is simulated so the end result of
> those changes can be reflected on-disk for eventual non-simulated
> operations. This results in expected behavior with the same ops file
> and test command:
> 
>   Will begin at operation 2
>   2 read  0x0 thru        0xfff   (0x1000 bytes)
> 
> Update zero, copy and clone range to do the file size and EOF change
> related zeroing before checking against the simulated ops count.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Oh wow, I really got that wrong. :(

Well, thank you for uncovering that error;
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


> ---
>  ltp/fsx.c | 40 +++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 2dc59b06..c5727cff 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -1247,6 +1247,17 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
>  	log4(OP_ZERO_RANGE, offset, length,
>  	     keep_size ? FL_KEEP_SIZE : FL_NONE);
>  
> +	if (!keep_size && end_offset > file_size) {
> +		/*
> +		 * If there's a gap between the old file size and the offset of
> +		 * the zero range operation, fill the gap with zeroes.
> +		 */
> +		if (offset > file_size)
> +			memset(good_buf + file_size, '\0', offset - file_size);
> +
> +		file_size = end_offset;
> +	}
> +
>  	if (testcalls <= simulatedopcount)
>  		return;
>  
> @@ -1263,17 +1274,6 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
>  	}
>  
>  	memset(good_buf + offset, '\0', length);
> -
> -	if (!keep_size && end_offset > file_size) {
> -		/*
> -		 * If there's a gap between the old file size and the offset of
> -		 * the zero range operation, fill the gap with zeroes.
> -		 */
> -		if (offset > file_size)
> -			memset(good_buf + file_size, '\0', offset - file_size);
> -
> -		file_size = end_offset;
> -	}
>  }
>  
>  #else
> @@ -1538,6 +1538,11 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
>  
>  	log5(OP_CLONE_RANGE, offset, length, dest, FL_NONE);
>  
> +	if (dest > file_size)
> +		memset(good_buf + file_size, '\0', dest - file_size);
> +	if (dest + length > file_size)
> +		file_size = dest + length;
> +
>  	if (testcalls <= simulatedopcount)
>  		return;
>  
> @@ -1556,10 +1561,6 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
>  	}
>  
>  	memcpy(good_buf + dest, good_buf + offset, length);
> -	if (dest > file_size)
> -		memset(good_buf + file_size, '\0', dest - file_size);
> -	if (dest + length > file_size)
> -		file_size = dest + length;
>  }
>  
>  #else
> @@ -1756,6 +1757,11 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
>  
>  	log5(OP_COPY_RANGE, offset, length, dest, FL_NONE);
>  
> +	if (dest > file_size)
> +		memset(good_buf + file_size, '\0', dest - file_size);
> +	if (dest + length > file_size)
> +		file_size = dest + length;
> +
>  	if (testcalls <= simulatedopcount)
>  		return;
>  
> @@ -1792,10 +1798,6 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
>  	}
>  
>  	memcpy(good_buf + dest, good_buf + offset, length);
> -	if (dest > file_size)
> -		memset(good_buf + file_size, '\0', dest - file_size);
> -	if (dest + length > file_size)
> -		file_size = dest + length;
>  }
>  
>  #else
> -- 
> 2.45.0
> 
> 

Re: [PATCH v2 2/4] fsx: factor out a file size update helper

[Darrick J. Wong]

On Wed, Aug 28, 2024 at 02:15:32PM -0400, Brian Foster wrote:
> In preparation for support for eof page pollution, factor out a file
> size update helper. This updates the internally tracked file size
> based on the upcoming operation and zeroes the appropriate range in
> the good buffer for extending operations.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks like a straightforward hoist now.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  ltp/fsx.c | 49 +++++++++++++++++++++----------------------------
>  1 file changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index c5727cff..4a9be0e1 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -983,6 +983,17 @@ gendata(char *original_buf, char *good_buf, unsigned offset, unsigned size)
>  	}
>  }
>  
> +/*
> + * Helper to update the tracked file size. If the offset begins beyond current
> + * EOF, zero the range from EOF to offset in the good buffer.
> + */
> +void
> +update_file_size(unsigned offset, unsigned size)
> +{
> +	if (offset > file_size)
> +		memset(good_buf + file_size, '\0', offset - file_size);
> +	file_size = offset + size;
> +}
>  
>  void
>  dowrite(unsigned offset, unsigned size)
> @@ -1003,10 +1014,8 @@ dowrite(unsigned offset, unsigned size)
>  	log4(OP_WRITE, offset, size, FL_NONE);
>  
>  	gendata(original_buf, good_buf, offset, size);
> -	if (file_size < offset + size) {
> -		if (file_size < offset)
> -			memset(good_buf + file_size, '\0', offset - file_size);
> -		file_size = offset + size;
> +	if (offset + size > file_size) {
> +		update_file_size(offset, size);
>  		if (lite) {
>  			warn("Lite file size bug in fsx!");
>  			report_failure(149);
> @@ -1070,10 +1079,8 @@ domapwrite(unsigned offset, unsigned size)
>  	log4(OP_MAPWRITE, offset, size, FL_NONE);
>  
>  	gendata(original_buf, good_buf, offset, size);
> -	if (file_size < offset + size) {
> -		if (file_size < offset)
> -			memset(good_buf + file_size, '\0', offset - file_size);
> -		file_size = offset + size;
> +	if (offset + size > file_size) {
> +		update_file_size(offset, size);
>  		if (lite) {
>  			warn("Lite file size bug in fsx!");
>  			report_failure(200);
> @@ -1136,9 +1143,7 @@ dotruncate(unsigned size)
>  
>  	log4(OP_TRUNCATE, 0, size, FL_NONE);
>  
> -	if (size > file_size)
> -		memset(good_buf + file_size, '\0', size - file_size);
> -	file_size = size;
> +	update_file_size(size, 0);
>  
>  	if (testcalls <= simulatedopcount)
>  		return;
> @@ -1247,16 +1252,8 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
>  	log4(OP_ZERO_RANGE, offset, length,
>  	     keep_size ? FL_KEEP_SIZE : FL_NONE);
>  
> -	if (!keep_size && end_offset > file_size) {
> -		/*
> -		 * If there's a gap between the old file size and the offset of
> -		 * the zero range operation, fill the gap with zeroes.
> -		 */
> -		if (offset > file_size)
> -			memset(good_buf + file_size, '\0', offset - file_size);
> -
> -		file_size = end_offset;
> -	}
> +	if (!keep_size && end_offset > file_size)
> +		update_file_size(offset, length);
>  
>  	if (testcalls <= simulatedopcount)
>  		return;
> @@ -1538,10 +1535,8 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
>  
>  	log5(OP_CLONE_RANGE, offset, length, dest, FL_NONE);
>  
> -	if (dest > file_size)
> -		memset(good_buf + file_size, '\0', dest - file_size);
>  	if (dest + length > file_size)
> -		file_size = dest + length;
> +		update_file_size(dest, length);
>  
>  	if (testcalls <= simulatedopcount)
>  		return;
> @@ -1757,10 +1752,8 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
>  
>  	log5(OP_COPY_RANGE, offset, length, dest, FL_NONE);
>  
> -	if (dest > file_size)
> -		memset(good_buf + file_size, '\0', dest - file_size);
>  	if (dest + length > file_size)
> -		file_size = dest + length;
> +		update_file_size(dest, length);
>  
>  	if (testcalls <= simulatedopcount)
>  		return;
> @@ -1848,7 +1841,7 @@ do_preallocate(unsigned offset, unsigned length, int keep_size)
>  
>  	if (end_offset > file_size) {
>  		memset(good_buf + file_size, '\0', end_offset - file_size);
> -		file_size = end_offset;
> +		update_file_size(offset, length);
>  	}
>  
>  	if (testcalls <= simulatedopcount)
> -- 
> 2.45.0
> 
> 

Re: [PATCH v2 3/4] fsx: support eof page pollution for eof zeroing test coverage

[Darrick J. Wong]

On Wed, Aug 28, 2024 at 02:15:33PM -0400, Brian Foster wrote:
> File ranges that are newly exposed via size changing operations are
> expected to return zeroes until written to. This behavior tends to
> be difficult to regression test as failures can be racy and
> transient. fsx is probably the best tool for this type of test
> coverage, but uncovering issues can require running for a
> significantly longer period of time than is typically invoked
> through fstests tests. As a result, these types of regressions tend
> to go unnoticed for an unfortunate amount of time.
> 
> To facilitate uncovering these problems more quickly, implement an
> eof pollution mode in fsx that opportunistically injects post-eof
> data prior to operations that change file size. Since data injection
> occurs immediately before the size changing operation, it can be
> used to detect problems in partial eof page/block zeroing associated
> with each relevant operation.
> 
> The implementation takes advantage of the fact that mapped writes
> can place data beyond eof so long as the page starts within eof. The
> main reason for the isolated per-operation approach (vs. something
> like allowing mapped writes to write beyond eof, for example) is to
> accommodate the fact that writeback zeroes post-eof data on the eof
> page. The current approach is therefore not necessarily guaranteed
> to detect all problems, but provides more generic and broad test
> coverage than the alternative of testing explicit command sequences
> and doesn't require significant changes to how fsx works. If this
> proves useful long term, further enhancements can be considered that
> might facilitate the presence of post-eof data across operations.
> 
> Enable the feature with the -e command line option. It is disabled
> by default because zeroing behavior is inconsistent across
> filesystems. This can also be revisited in the future if zeroing
> behavior is refined for the major filesystems that rely on fstests
> for regression testing.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

I wonder what insanity this is going to shake loose, so thanks for doing
putting this in a stressor program. :)

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  ltp/fsx.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 4a9be0e1..1ba1bf65 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -178,6 +178,7 @@ int	dedupe_range_calls = 1;		/* -B flag disables */
>  int	copy_range_calls = 1;		/* -E flag disables */
>  int	exchange_range_calls = 1;	/* -0 flag disables */
>  int	integrity = 0;			/* -i flag */
> +int	pollute_eof = 0;		/* -e flag */
>  int	fsxgoodfd = 0;
>  int	o_direct;			/* -Z */
>  int	aio = 0;
> @@ -983,6 +984,63 @@ gendata(char *original_buf, char *good_buf, unsigned offset, unsigned size)
>  	}
>  }
>  
> +/*
> + * Pollute the EOF page with data beyond EOF prior to size change operations.
> + * This provides additional test coverage for partial EOF block/page zeroing.
> + * If the upcoming operation does not correctly zero, incorrect file data will
> + * be detected.
> + */
> +void
> +pollute_eofpage(unsigned int maxoff)
> +{
> +	unsigned offset = file_size;
> +	unsigned pg_offset;
> +	unsigned write_size;
> +	char    *p;
> +
> +	/*
> +	 * Nothing to do if pollution disabled or we're simulating. Simulation
> +	 * only tracks file size updates and skips syscalls, so we don't want to
> +	 * inject file data that won't be zeroed.
> +	 */
> +	if (!pollute_eof || testcalls <= simulatedopcount)
> +		return;
> +
> +	/* write up to specified max or the end of the eof page */
> +	pg_offset = offset & mmap_mask;
> +	write_size = MIN(PAGE_SIZE - pg_offset, maxoff - offset);
> +
> +	if (!pg_offset)
> +		return;
> +
> +	if (!quiet &&
> +	    ((progressinterval && testcalls % progressinterval == 0) ||
> +	    (debug &&
> +	     (monitorstart == -1 ||
> +	     (offset + write_size > monitorstart &&
> +	      (monitorend == -1 || offset <= monitorend)))))) {
> +		prt("%lld pollute_eof\t0x%x thru\t0x%x\t(0x%x bytes)\n",
> +			testcalls, offset, offset + write_size - 1, write_size);
> +	}
> +
> +	if ((p = (char *)mmap(0, PAGE_SIZE, PROT_READ | PROT_WRITE,
> +			      MAP_FILE | MAP_SHARED, fd,
> +			      (off_t)(offset - pg_offset))) == MAP_FAILED) {
> +		prterr("pollute_eofpage: mmap");
> +		return;
> +	}
> +
> +	/*
> +	 * Write to a range just past EOF of the test file. Do not update the
> +	 * good buffer because the upcoming operation is expected to zero this
> +	 * range of the file.
> +	 */
> +	gendata(original_buf, p, pg_offset, write_size);
> +
> +	if (munmap(p, PAGE_SIZE) != 0)
> +		prterr("pollute_eofpage: munmap");
> +}
> +
>  /*
>   * Helper to update the tracked file size. If the offset begins beyond current
>   * EOF, zero the range from EOF to offset in the good buffer.
> @@ -990,8 +1048,10 @@ gendata(char *original_buf, char *good_buf, unsigned offset, unsigned size)
>  void
>  update_file_size(unsigned offset, unsigned size)
>  {
> -	if (offset > file_size)
> +	if (offset > file_size) {
> +		pollute_eofpage(offset + size);
>  		memset(good_buf + file_size, '\0', offset - file_size);
> +	}
>  	file_size = offset + size;
>  }
>  
> @@ -1143,6 +1203,9 @@ dotruncate(unsigned size)
>  
>  	log4(OP_TRUNCATE, 0, size, FL_NONE);
>  
> +	/* pollute the current EOF before a truncate down */
> +	if (size < file_size)
> +		pollute_eofpage(maxfilelen);
>  	update_file_size(size, 0);
>  
>  	if (testcalls <= simulatedopcount)
> @@ -1305,6 +1368,9 @@ do_collapse_range(unsigned offset, unsigned length)
>  
>  	log4(OP_COLLAPSE_RANGE, offset, length, FL_NONE);
>  
> +	/* pollute current eof before collapse truncates down */
> +	pollute_eofpage(maxfilelen);
> +
>  	if (testcalls <= simulatedopcount)
>  		return;
>  
> @@ -1356,6 +1422,9 @@ do_insert_range(unsigned offset, unsigned length)
>  
>  	log4(OP_INSERT_RANGE, offset, length, FL_NONE);
>  
> +	/* pollute current eof before insert truncates up */
> +	pollute_eofpage(maxfilelen);
> +
>  	if (testcalls <= simulatedopcount)
>  		return;
>  
> @@ -2385,6 +2454,7 @@ usage(void)
>  	-b opnum: beginning operation number (default 1)\n\
>  	-c P: 1 in P chance of file close+open at each op (default infinity)\n\
>  	-d: debug output for all operations\n\
> +	-e: pollute post-eof on size changes (default 0)\n\
>  	-f: flush and invalidate cache after I/O\n\
>  	-g X: write character X instead of random generated data\n\
>  	-i logdev: do integrity testing, logdev is the dm log writes device\n\
> @@ -2783,7 +2853,7 @@ main(int argc, char **argv)
>  	setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
>  
>  	while ((ch = getopt_long(argc, argv,
> -				 "0b:c:dfg:i:j:kl:m:no:p:qr:s:t:w:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> +				 "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:w:xyABD:EFJKHzCILN:OP:RS:UWXZ",
>  				 longopts, NULL)) != EOF)
>  		switch (ch) {
>  		case 'b':
> @@ -2805,6 +2875,11 @@ main(int argc, char **argv)
>  		case 'd':
>  			debug = 1;
>  			break;
> +		case 'e':
> +			pollute_eof = getnum(optarg, &endp);
> +			if (pollute_eof < 0 || pollute_eof > 1)
> +				usage();
> +			break;
>  		case 'f':
>  			flush = 1;
>  			break;
> -- 
> 2.45.0
> 
> 

Re: [PATCH v2 4/4] generic: test to run fsx eof pollution

[Darrick J. Wong]

On Wed, Aug 28, 2024 at 02:15:34PM -0400, Brian Foster wrote:
> Filesystem regressions related to partial page zeroing can go
> unnoticed for a decent amount of time. A recent example is the issue
> of iomap zero range not handling dirty pagecache over unwritten
> extents, which leads to wrong behavior on certain file extending
> operations (i.e. truncate, write extension, etc.).
> 
> fsx does occasionally uncover these sorts of problems, but failures
> can be rare and/or require longer running tests outside what is
> typically run via full fstests regression runs. fsx now supports a
> mode that injects post-eof data in order to explicitly test partial
> eof zeroing behavior. This uncovers certain problems more quickly
> and applies coverage more broadly across size changing operations.
> 
> Add a new test that runs an fsx instance (modeled after generic/127)
> with eof pollution mode enabled. While the test is generic, it is
> currently limited to XFS as that is currently the only known major
> fs that does enough zeroing to satisfy the strict semantics expected
> by fsx. The long term goal is to uncover and fix issues so more
> filesystems can enable this test.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good!
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  tests/generic/363     | 23 +++++++++++++++++++++++
>  tests/generic/363.out |  2 ++
>  2 files changed, 25 insertions(+)
>  create mode 100755 tests/generic/363
>  create mode 100644 tests/generic/363.out
> 
> diff --git a/tests/generic/363 b/tests/generic/363
> new file mode 100755
> index 00000000..477c111c
> --- /dev/null
> +++ b/tests/generic/363
> @@ -0,0 +1,23 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 Red Hat, Inc.  All Rights Reserved.
> +#
> +# FSQA Test No. 363
> +#
> +# Run fsx with EOF pollution enabled. This provides test coverage for partial
> +# EOF page/block zeroing for operations that change file size.
> +#
> +
> +. ./common/preamble
> +_begin_fstest rw auto
> +
> +_require_test
> +
> +# currently only xfs performs enough zeroing to satisfy fsx
> +_supported_fs xfs
> +
> +# on failure, replace -q with -d to see post-eof writes in the dump output
> +run_fsx "-q -S 0 -e 1 -N 100000"
> +
> +status=0
> +exit
> diff --git a/tests/generic/363.out b/tests/generic/363.out
> new file mode 100644
> index 00000000..3d219cd0
> --- /dev/null
> +++ b/tests/generic/363.out
> @@ -0,0 +1,2 @@
> +QA output created by 363
> +fsx -q -S 0 -e 1 -N 100000
> -- 
> 2.45.0
> 
> 

Re: [PATCH v2 1/4] fsx: don't skip file size and buf updates on simulated ops

[Brian Foster]

On Wed, Aug 28, 2024 at 06:27:16PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 28, 2024 at 02:15:31PM -0400, Brian Foster wrote:
> > fsx supports the ability to skip through a certain number of
> > operations of a given command sequence before beginning full
> > operation. The way this works is by tracking the operation count,
> > simulating minimal side effects of skipped operations in-memory, and
> > then finally writing out the in-memory state to the target file when
> > full operation begins.
> > 
> > Several fallocate() related operations don't correctly track
> > in-memory state when simulated, however. For example, consider an
> > ops file with the following two operations:
> > 
> >   zero_range 0x0 0x1000 0x0
> >   read 0x0 0x1000 0x0
> > 
> > ... and an fsx run like so:
> > 
> >   fsx -d -b 2 --replay-ops=<opsfile> <file>
> > 
> > This simulates the zero_range operation, but fails to track the file
> > extension that occurs as a side effect such that the subsequent read
> > doesn't occur as expected:
> > 
> >   Will begin at operation 2
> >   skipping zero size read
> > 
> > The read is skipped in this case because the file size is zero.  The
> > proper behavior, and what is consistent with other size changing
> > operations, is to make the appropriate in-core changes before
> > checking whether an operation is simulated so the end result of
> > those changes can be reflected on-disk for eventual non-simulated
> > operations. This results in expected behavior with the same ops file
> > and test command:
> > 
> >   Will begin at operation 2
> >   2 read  0x0 thru        0xfff   (0x1000 bytes)
> > 
> > Update zero, copy and clone range to do the file size and EOF change
> > related zeroing before checking against the simulated ops count.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> 
> Oh wow, I really got that wrong. :(
> 

Eh, so did I. ;) That the code was mostly right was pretty much just
luck. Thanks for the thoughtful reviews.

Brian

> Well, thank you for uncovering that error;
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
> 
> > ---
> >  ltp/fsx.c | 40 +++++++++++++++++++++-------------------
> >  1 file changed, 21 insertions(+), 19 deletions(-)
> > 
> > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > index 2dc59b06..c5727cff 100644
> > --- a/ltp/fsx.c
> > +++ b/ltp/fsx.c
> > @@ -1247,6 +1247,17 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
> >  	log4(OP_ZERO_RANGE, offset, length,
> >  	     keep_size ? FL_KEEP_SIZE : FL_NONE);
> >  
> > +	if (!keep_size && end_offset > file_size) {
> > +		/*
> > +		 * If there's a gap between the old file size and the offset of
> > +		 * the zero range operation, fill the gap with zeroes.
> > +		 */
> > +		if (offset > file_size)
> > +			memset(good_buf + file_size, '\0', offset - file_size);
> > +
> > +		file_size = end_offset;
> > +	}
> > +
> >  	if (testcalls <= simulatedopcount)
> >  		return;
> >  
> > @@ -1263,17 +1274,6 @@ do_zero_range(unsigned offset, unsigned length, int keep_size)
> >  	}
> >  
> >  	memset(good_buf + offset, '\0', length);
> > -
> > -	if (!keep_size && end_offset > file_size) {
> > -		/*
> > -		 * If there's a gap between the old file size and the offset of
> > -		 * the zero range operation, fill the gap with zeroes.
> > -		 */
> > -		if (offset > file_size)
> > -			memset(good_buf + file_size, '\0', offset - file_size);
> > -
> > -		file_size = end_offset;
> > -	}
> >  }
> >  
> >  #else
> > @@ -1538,6 +1538,11 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
> >  
> >  	log5(OP_CLONE_RANGE, offset, length, dest, FL_NONE);
> >  
> > +	if (dest > file_size)
> > +		memset(good_buf + file_size, '\0', dest - file_size);
> > +	if (dest + length > file_size)
> > +		file_size = dest + length;
> > +
> >  	if (testcalls <= simulatedopcount)
> >  		return;
> >  
> > @@ -1556,10 +1561,6 @@ do_clone_range(unsigned offset, unsigned length, unsigned dest)
> >  	}
> >  
> >  	memcpy(good_buf + dest, good_buf + offset, length);
> > -	if (dest > file_size)
> > -		memset(good_buf + file_size, '\0', dest - file_size);
> > -	if (dest + length > file_size)
> > -		file_size = dest + length;
> >  }
> >  
> >  #else
> > @@ -1756,6 +1757,11 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
> >  
> >  	log5(OP_COPY_RANGE, offset, length, dest, FL_NONE);
> >  
> > +	if (dest > file_size)
> > +		memset(good_buf + file_size, '\0', dest - file_size);
> > +	if (dest + length > file_size)
> > +		file_size = dest + length;
> > +
> >  	if (testcalls <= simulatedopcount)
> >  		return;
> >  
> > @@ -1792,10 +1798,6 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest)
> >  	}
> >  
> >  	memcpy(good_buf + dest, good_buf + offset, length);
> > -	if (dest > file_size)
> > -		memset(good_buf + file_size, '\0', dest - file_size);
> > -	if (dest + length > file_size)
> > -		file_size = dest + length;
> >  }
> >  
> >  #else
> > -- 
> > 2.45.0
> > 
> > 
> 

Re: [PATCH v2 0/4] fstests/fsx: test coverage for eof zeroing

[Zorro Lang]

On Wed, Aug 28, 2024 at 02:15:30PM -0400, Brian Foster wrote:
> Hi all,
> 
> Here's v2 of the patches for improved test coverage (via fsx) for EOF
> related zeroing. The most notable change is that the discussion on v1
> uncovered that some of the existing fsx behavior was wrong wrt to
> simulated ops, so patch 1 is factored out as a standalone bug fix to
> address that.
> 
> Brian
> 
> v2:
> - Factored out patch 1 to fix simulation mode.
> - Use MAP_FAILED, don't inject data for simulated ops.
> - Rebase to latest master and renumber test.
> - Use run_fsx and -S 0 by default (timestamp seed).
> v1: https://lore.kernel.org/fstests/20240822144422.188462-1-bfoster@redhat.com/
> 
> Brian Foster (4):
>   fsx: don't skip file size and buf updates on simulated ops
>   fsx: factor out a file size update helper
>   fsx: support eof page pollution for eof zeroing test coverage
>   generic: test to run fsx eof pollution

Thanks for updating the fsx.c, fsstress and fsx are important test program in
fstests, it's always warm welcome improving them :) I'll merge this patchset
after a basic regression test.

Thanks,
Zorro

> 
>  ltp/fsx.c             | 134 ++++++++++++++++++++++++++++++++----------
>  tests/generic/363     |  23 ++++++++
>  tests/generic/363.out |   2 +
>  3 files changed, 127 insertions(+), 32 deletions(-)
>  create mode 100755 tests/generic/363
>  create mode 100644 tests/generic/363.out
> 
> -- 
> 2.45.0
> 
>