Re: [PATCH 2/2] generic/530: only use xfs-specific mkfs options when testing on xfs

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Dec 20, 2024 at 07:49:28AM +1100, Dave Chinner wrote:
> On Thu, Dec 19, 2024 at 08:32:06AM -0800, Darrick J. Wong wrote:
> > On Thu, Dec 19, 2024 at 06:16:26PM +0800, Zorro Lang wrote:
> > > On Wed, Dec 18, 2024 at 09:37:53PM -0800, Christoph Hellwig wrote:
> > > > On Thu, Dec 19, 2024 at 09:26:06AM +1100, Dave Chinner wrote:
> > > > > On Tue, Dec 17, 2024 at 12:11:07AM -0800, Christoph Hellwig wrote:
> > > > > > On Sun, Dec 15, 2024 at 12:12:42AM -0500, Theodore Ts'o wrote:
> > > > > > > +if [ $FSTYP = "xfs" ]; then
> > > > > > > +    _scratch_mkfs "-l size=256m" >> $seqres.full 2>&1
> > > > > > > +else
> > > > > > > +    _scratch_mkfs >> $seqres.full 2>&1
> > > > > > > +fi
> > > > > > 
> > > > > > We really need to document why generic tests have file system specific
> > > > > > hacks.  And yes, that's a request for Dave who originally added it
> > > > > > without any explanation and not Ted.
> > > > > 
> > > > > Creating and then unlinking 50000 files is journal space bound
> > > > > when using default 64MB logs on small test filesystems. Increasing
> > > > > the journal size to 256MB halved the runtime of this test.
> > > > 
> > > > Please explain this in thet test.  And you probably also want to
> > > > ensure that you don't force the log smaller than 256 either, otherwise
> > > > people in 10 or 20 years will wonder why this test forces logs to
> > > > be so small.
> > > 
> > > I'll help to add this comment when I merge this patch, if Dave hope to
> > > keep "-l size=256m" for xfs.
> > 
> > What happens if someone runs fstests with a 128M external log device?
> 
> It fails, then drops MKFS_OPTIONS.  The simple solution for this is
> to simply to use a larger external log device....
> 
> > Is this one of those cases where _scratch_mkfs notices the mkfs failure
> > and formats without MKFS_OPTIONS? 
> 
> More than likely.
> 
> > And if that's true, what about my
> > test configs that set MKFS_OPTIONS to test new non-default features?
> 
> Changing existing infrastructure behaviour to better suit *your*
> test environments is *your* responsibility to address, not mine.
> I don't care if MKFS_OPTIONS get dropped in occasional tests, it's
> more important to me that the tests run fast so I can iterate
> my modify-build-test development cycle faster.

I don't agree that your personal development velocity is more important
than silent loss of code coverage of everyone who happens to be testing
external log devices.  That at least covers myself, our QA department,
and everyone who uses the prepackaged configurations in kdevops and
xfstests-bld.

> It should be trivial for you to address, though. Add a function
> like:
> 
> _scratch_mkfs_try_option "-l size=256M" 

Or instead:

# Pushing 50000 unlinked inode inactivations through a small xfs log can
# result in bottlenecks on the log grant heads, so try to make the log
# larger to reduce runtime.
if [ $FSTYP = xfs ] && [ -z "$SCRATCH_LOGDEV ]; then
	_scratch_mkfs -l size=256M
else
	_scratch_mkfs
fi

> which has the opposite fallback behaviour of dropping the test
> supplied option and using MKFS_OPTIONS instead, and I'll use it for
> all these "test go faster" modifications that we badly need to
> address...
> 
> FWIW, this would also get rid of the need for the FSTYP checks in
> the test, too, because passing "-l size=256M" will fail on btrfs,
> ext4, etc and then they fall back to the specific test config...

Huh?  And what happens when btrfs want their own try option?

> So, provide me with the infrastructure that makes stuff like this
> work properly in *your test environment*, and I'll use it
> appropriately.

Dave.

You're the one who made the changes to generic/53*.  It worked before.

Like any other code change, it's /your/ responsibility to try not to
cause regressions for everyone else using the same code.  If other
people have problems with a change, it's longstanding community
expectation that the author should help resolve the problem.

It is /not/ standard practice to lecture the affected people about how
they should embark on a treewide change to fix a problem that didn't
exist previously.  The maintainer reverts the offending commit and the
author redrafts the patch.  You're well aware of this, because you've
said as much about other patches (e.g. iomap zeroing cleanups).

It is not everyone else's (Ted, Christoph, Zorro) role to clean up after
you.

I do not appreciate the condescending behavior you have shown towards me
this year and last.  Next year I will take more steps to insulate my
team from you and other community members as needed because I don't want
them to continue to deal with such conduct[1][2].

--D

[1] https://lore.kernel.org/fstests/Z0pBKWBlXLBxwK3D@dread.disaster.area/
[2] https://lore.kernel.org/linux-xfs/Z1FQdYEXLR5BoOE-@redhat.com/

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