Re: [PATCH] xfs: new EOF fragmentation tests

[Brian Foster <bfoster@redhat.com>]

On Wed, Feb 13, 2019 at 08:04:22AM +1100, Dave Chinner wrote:
> On Tue, Feb 12, 2019 at 09:03:30AM -0500, Brian Foster wrote:
> > On Mon, Feb 11, 2019 at 12:36:47PM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > These tests create substantial file fragmentation as a result of
> > > application actions that defeat post-EOF preallocation
> > > optimisations. They are intended to replicate known vectors for
> > > these problems, and provide a check that the fragmentation levels
> > > have been controlled. The mitigations we make may not completely
> > > remove fragmentation (e.g. they may demonstrate speculative delalloc
> > > related extent size growth) so the checks don't assume we'll end up
> > > with perfect layouts and hence check for an exceptable level of
> > > fragmentation rather than none.
> .....
> > > +workfile=$SCRATCH_MNT/file
> > > +nfiles=8
> > > +wsize=4096
> > > +wcnt=1000
> > > +
> > > +write_sync_file()
> > > +{
> > > +	idx=$1
> > > +
> > > +	for ((cnt=0; cnt<$wcnt; cnt++)); do
> > > +		$XFS_IO_PROG -f -s -c "pwrite $((cnt * wsize)) $wsize" $workfile.$idx
> > > +	done
> > > +}
> > > +
> > > +rm -f $workfile*
> > > +for ((n=0; n<$nfiles; n++)); do
> > > +	write_sync_file $n > /dev/null 2>&1 &
> > > +done
> > > +wait
> > > +sync
> > 
> > I'm not a huge fan of seeing these global syncs sprinkled around tests
> > where they might not be needed. Are these here for a reason? If so, could
> > they be replaced with an xfs_io -c syncfs or some such?
> 
> sync is there to ensure that everything is stable and I don't need
> to care about using sync flags for counting extents.
> 
> I could use syncfs (either binary or xfs_io) but that's not
> supported on all the OS's that fstests runs on. And I don't think
> using sync really matters - if "sync" causes problems for fstests
> that using syncfs fixes, then we've got bigger issues to worry
> about.
> 

It's not a matter of sync causing problems, I just think it's a kludge
to sync the entire system. syncfs by definition is localized to a target
fs.

Having taken a closer look, xfs_io unconditionally uses the fiemap sync
flag so this is an instance where we don't need either.

> > > +
> > > +for ((n=0; n<$nfiles; n++)); do
> > > +	count=$(_count_extents $workfile.$n)
> > > +	# Acceptible extent count range is 1-40
> > > +	_within_tolerance "file.$n extent count" $count 21 19 -v
> > > +done
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > ...
> > > diff --git a/tests/xfs/502 b/tests/xfs/502
> > > new file mode 100755
> > > index 00000000..9f314a3d
> > > --- /dev/null
> > > +++ b/tests/xfs/502
> > > @@ -0,0 +1,81 @@
> > ...
> > > +
> > > +# Write multiple files in parallel using O_DIRECT writes w/ extent size hints.
> > > +# Aim is to interleave allocations to fragment the files. O_DIRECT writes defeat
> > > +# the open/write/close heuristics in xfs_release() that prevent EOF block
> > > +# removal, so this should fragment badly. Typical problematic behaviour shows
> > > +# per-file extent counts of ~1000 (worst case) whilst fixed behaviour typically
> > > +# shows extent counts in the low single digits (almost best case)
> > > +#
> > 
> > This is essentially equivalent to the previous test, right? E.g.,
> 
> IMO, no. This is a different IO pattern that demonstrates a
> fragmentation problem, regardless of the underlying cause. We may
> fix the buffered write problem (e.g. via enabling delalloc on extent
> size hints) but that leaves the direct IO variant unchanged. Hence,
> if we drop this test, we'll never realise we still have a direct IO
> variant we have to ensure we don't break.
> 
> IOWs, I'm attempting to characterise and exercise the userspace IO
> patterns that cause fragmentation problems rather than exercise a
> specific bug in the filesystem implementation.
> 
> > path as direct I/O with an extent size hint in current XFS. Of course
> > that may not always be so, but I wonder if these two could be combined.
> 
> That's kinda my point - these IO patterns haven't always (and won't
> always) exercised the same code paths and allocation behaviours.
> Hence we nee dto exercise them both, even if they currently both
> expose the same underlying problem.
> 

Ok, but I wasn't suggesting to not test any of these codepaths. I wanted
to make sure the duplication of (current) behavior across the separate
tests was intentional, for one. I'm also noting that the difference
between this test and the previous is the xfs_io '-d' flag, and thus
wondering why we wouldn't combine that into a single test (that tests
extsz hint fragmentation on buffered and direct I/O).

Brian

> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com