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

[Brian Foster <bfoster@redhat.com>]

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
> > 
> > 
>