diff mbox series

[3/3] generic: test to run fsx eof pollution

Message ID 20240822144422.188462-4-bfoster@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series fstests/fsx: test coverage for eof zeroing | expand

Commit Message

Brian Foster Aug. 22, 2024, 2:44 p.m. UTC
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/362     | 27 +++++++++++++++++++++++++++
 tests/generic/362.out |  2 ++
 2 files changed, 29 insertions(+)
 create mode 100755 tests/generic/362
 create mode 100644 tests/generic/362.out

Comments

Darrick J. Wong Aug. 22, 2024, 8:54 p.m. UTC | #1
On Thu, Aug 22, 2024 at 10:44:22AM -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>
> ---
>  tests/generic/362     | 27 +++++++++++++++++++++++++++
>  tests/generic/362.out |  2 ++
>  2 files changed, 29 insertions(+)
>  create mode 100755 tests/generic/362
>  create mode 100644 tests/generic/362.out
> 
> diff --git a/tests/generic/362 b/tests/generic/362
> new file mode 100755
> index 00000000..30870cd0
> --- /dev/null
> +++ b/tests/generic/362
> @@ -0,0 +1,27 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 Red Hat, Inc.  All Rights Reserved.
> +#
> +# FSQA Test No. 362
> +#
> +# 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
> +
> +FSX_FILE_SIZE=262144
> +# on failure, replace -q with -d to see post-eof writes in the dump output
> +FSX_ARGS="-q -l $FSX_FILE_SIZE -e 1 -N 100000"
> +
> +_require_test
> +
> +# currently only xfs performs enough zeroing to satisfy fsx
> +_supported_fs xfs

Should get rid of this. ;)

> +ltp/fsx $FSX_ARGS $FSX_AVOID $TEST_DIR/fsx.$seq > $tmp.output 2>&1

I wonder, is there a reason not to use run_fsx from common/rc?

Otherwise this looks ok to me.

--D

> +cat $tmp.output
> +
> +status=0
> +exit
> diff --git a/tests/generic/362.out b/tests/generic/362.out
> new file mode 100644
> index 00000000..7af6b96a
> --- /dev/null
> +++ b/tests/generic/362.out
> @@ -0,0 +1,2 @@
> +QA output created by 362
> +All 100000 operations completed A-OK!
> -- 
> 2.45.0
> 
>
Brian Foster Aug. 26, 2024, 2:07 p.m. UTC | #2
On Thu, Aug 22, 2024 at 01:54:30PM -0700, Darrick J. Wong wrote:
> On Thu, Aug 22, 2024 at 10:44:22AM -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>
> > ---
> >  tests/generic/362     | 27 +++++++++++++++++++++++++++
> >  tests/generic/362.out |  2 ++
> >  2 files changed, 29 insertions(+)
> >  create mode 100755 tests/generic/362
> >  create mode 100644 tests/generic/362.out
> > 
> > diff --git a/tests/generic/362 b/tests/generic/362
> > new file mode 100755
> > index 00000000..30870cd0
> > --- /dev/null
> > +++ b/tests/generic/362
> > @@ -0,0 +1,27 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2024 Red Hat, Inc.  All Rights Reserved.
> > +#
> > +# FSQA Test No. 362
> > +#
> > +# 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
> > +
> > +FSX_FILE_SIZE=262144
> > +# on failure, replace -q with -d to see post-eof writes in the dump output
> > +FSX_ARGS="-q -l $FSX_FILE_SIZE -e 1 -N 100000"
> > +
> > +_require_test
> > +
> > +# currently only xfs performs enough zeroing to satisfy fsx
> > +_supported_fs xfs
> 
> Should get rid of this. ;)
> 

Agreed. But just to be clear, I was planning to leave this in for now so
we don't have to go and fix the various other filesystem issues
(assuming various maintainers agree) as a gate to fixing iomap zeroing
and having decent test coverage. I'm planning to look into the other
failures once I get through the iomap revalidation thing to avoid the
flush on XFS.

> > +ltp/fsx $FSX_ARGS $FSX_AVOID $TEST_DIR/fsx.$seq > $tmp.output 2>&1
> 
> I wonder, is there a reason not to use run_fsx from common/rc?
> 

At first glance the only thing that stands out to me is run_fsx
hardcodes the file to $TEST_DIR/junk instead of using and leaving around
a test-specific file. I find that ever so slightly annoying, but not
enough to hardcode the fsx command in the test, so I'll make that
change.

My initial hope was to be able to just turn on the eof pollution thing
by default and rely on existing fstests to (which I've confirmed do)
detect the iomap zeroing problems, but the other non-iomap fs failures
mean we can't do that without being disruptive and hence the need for
the custom fstest. If we can get to the point of completely removing the
_supported_fs check above, then perhaps we could also revisit changing
the fsx default and removing this test.

> Otherwise this looks ok to me.
> 

Thanks for the comments.

Brian

> --D
> 
> > +cat $tmp.output
> > +
> > +status=0
> > +exit
> > diff --git a/tests/generic/362.out b/tests/generic/362.out
> > new file mode 100644
> > index 00000000..7af6b96a
> > --- /dev/null
> > +++ b/tests/generic/362.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 362
> > +All 100000 operations completed A-OK!
> > -- 
> > 2.45.0
> > 
> > 
>
diff mbox series

Patch

diff --git a/tests/generic/362 b/tests/generic/362
new file mode 100755
index 00000000..30870cd0
--- /dev/null
+++ b/tests/generic/362
@@ -0,0 +1,27 @@ 
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2024 Red Hat, Inc.  All Rights Reserved.
+#
+# FSQA Test No. 362
+#
+# 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
+
+FSX_FILE_SIZE=262144
+# on failure, replace -q with -d to see post-eof writes in the dump output
+FSX_ARGS="-q -l $FSX_FILE_SIZE -e 1 -N 100000"
+
+_require_test
+
+# currently only xfs performs enough zeroing to satisfy fsx
+_supported_fs xfs
+
+ltp/fsx $FSX_ARGS $FSX_AVOID $TEST_DIR/fsx.$seq > $tmp.output 2>&1
+cat $tmp.output
+
+status=0
+exit
diff --git a/tests/generic/362.out b/tests/generic/362.out
new file mode 100644
index 00000000..7af6b96a
--- /dev/null
+++ b/tests/generic/362.out
@@ -0,0 +1,2 @@ 
+QA output created by 362
+All 100000 operations completed A-OK!