[PATCH 0/2] Fix recent regressions from for_next

[PATCH 0/2] Fix recent regressions from for_next

[Theodore Ts'o]

I don't think I saw an announcement for the most recent for_next
update, but I just happened to pull it and try it out, and found the
following regressions.  Fortunately they were easy fixes.

Theodore Ts'o (2):
  generic/135: don't try to rm $SCRATCH_MNT/*
  generic/530: only use xfs-specific mkfs options when testing on xfs

 tests/generic/135 | 2 --
 tests/generic/530 | 6 +++++-
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.45.2

[PATCH 1/2] generic/135: don't try to rm $SCRATCH_MNT/*

[Theodore Ts'o]

This fixes a regression for ext4 introduced by commit 32e14cb90b88
("fstests: don't use directory stacks"), which replaced a number of
files at the top-level of the scratch file system:

    async_file sync_file direct_file trunc_file

with "rm $SCRATCH_MNT/*".  This causes a test failure on ext4 file
systems caused by a failed attempt to unlink the lost+found directory.
The thing, is these files are all super small (4k or 16k) and the
scratch file system is going to get reformatted before it gets used
again.  So just dropping the delete is the simplest way to solve the
problem.

Fixes: 32e14cb90b88 ("fstests: don't use directory stacks")
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/135 | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tests/generic/135 b/tests/generic/135
index 392fee6fc..f74e144f3 100755
--- a/tests/generic/135
+++ b/tests/generic/135
@@ -40,7 +40,5 @@ od -Ad -x $SCRATCH_MNT/sync_file
 od -Ad -x $SCRATCH_MNT/direct_file
 od -Ad -x $SCRATCH_MNT/trunc_file
 
-rm -f $SCRATCH_MNT/*
-
 status=0
 exit 
-- 
2.45.2

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

[Theodore Ts'o]

This fixes a regression introduced by commit 000813899afb ("fstests:
scale some tests for high CPU count sanity") where this test
unconditionally tried to use the mkfs option "-l size=256m" which
would break when testing any file sytem other than xfs.

Fix this the same way commit 000813899afb dealt with this for
generic/531; so this was just an oversight.

Fixes: 000813899afb ("fstests: scale some tests for high CPU count sanity")
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/530 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/generic/530 b/tests/generic/530
index 18256b870..06d7532f0 100755
--- a/tests/generic/530
+++ b/tests/generic/530
@@ -22,7 +22,11 @@ _require_scratch_shutdown
 _require_metadata_journaling
 _require_test_program "t_open_tmpfiles"
 
-_scratch_mkfs "-l size=256m" >> $seqres.full 2>&1
+if [ $FSTYP = "xfs" ]; then
+    _scratch_mkfs "-l size=256m" >> $seqres.full 2>&1
+else
+    _scratch_mkfs >> $seqres.full 2>&1
+fi
 _scratch_mount
 
 # Set ULIMIT_NOFILE to min(file-max / 2, 50000 files per LOAD_FACTOR)
-- 
2.45.2

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

[Christoph Hellwig]

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.

Re: [PATCH 1/2] generic/135: don't try to rm $SCRATCH_MNT/*

[Christoph Hellwig]

On Sun, Dec 15, 2024 at 12:12:41AM -0500, Theodore Ts'o wrote:
> This fixes a regression for ext4 introduced by commit 32e14cb90b88
> ("fstests: don't use directory stacks"), which replaced a number of
> files at the top-level of the scratch file system:
> 
>     async_file sync_file direct_file trunc_file
> 
> with "rm $SCRATCH_MNT/*".  This causes a test failure on ext4 file
> systems caused by a failed attempt to unlink the lost+found directory.
> The thing, is these files are all super small (4k or 16k) and the
> scratch file system is going to get reformatted before it gets used
> again.  So just dropping the delete is the simplest way to solve the
> problem.
> 
> Fixes: 32e14cb90b88 ("fstests: don't use directory stacks")
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Dave Chinner]

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.

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

Re: [PATCH 0/2] Fix recent regressions from for_next

[Dave Chinner]

On Sun, Dec 15, 2024 at 12:12:40AM -0500, Theodore Ts'o wrote:
> I don't think I saw an announcement for the most recent for_next
> update, but I just happened to pull it and try it out, and found the
> following regressions.  Fortunately they were easy fixes.
> 
> Theodore Ts'o (2):
>   generic/135: don't try to rm $SCRATCH_MNT/*
>   generic/530: only use xfs-specific mkfs options when testing on xfs
> 
>  tests/generic/135 | 2 --
>  tests/generic/530 | 6 +++++-
>  2 files changed, 5 insertions(+), 3 deletions(-)

Fixes look good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

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

[Christoph Hellwig]

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.

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

[Zorro Lang]

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.

Thanks,
Zorro

> 

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

[Darrick J. Wong]

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?
Is this one of those cases where _scratch_mkfs notices the mkfs failure
and formats without MKFS_OPTIONS?  And if that's true, what about my
test configs that set MKFS_OPTIONS to test new non-default features?

--D

> Thanks,
> Zorro
> 
> > 
> 
> 

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

[Dave Chinner]

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.

It should be trivial for you to address, though. Add a function
like:

_scratch_mkfs_try_option "-l size=256M" 

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

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

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

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

[Darrick J. Wong]

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

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

[Eric Sandeen]

<snip the latest grousing back and forth>

Gentlemen, It would go a really long way if everyone could keep a
few things in mind as we work together in this public space.

1) Act in good faith
2) Assume good intent
3) Don't escalate conflict
4) Own your mistakes

That's basic stuff. Could we maybe give that a shot for a while,
and see if we can reduce this misery a bit?

(If you took any of the above personally, please don't. I'm just
reflecting on what I think are some longstanding patterns, here.)

Everybody's working hard, everybody's stressed, everybody makes
mistakes. I have no desire to go point by point through some
of the many recent blow-ups, but in nearly every one of them I
think things would have gone better if at any time during the back
and forth, people had stopped to reflect on the points above
before firing off a reply.

Above isn't perfect and perhaps others have thoughts on basic
guidelines for communicating without conflict, but maybe it's a
start?

-Eric

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

[Zorro Lang]

On Thu, Dec 19, 2024 at 02:53:31PM -0800, Darrick J. Wong wrote:
> 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

Maybe better to use "_has_logdev", due to there's "$USE_EXTERNAL".
I can change this patch as:

  # 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" ] && _has_logdev; then
  	_scratch_mkfs -l size=256M
  else
  	_scratch_mkfs
  fi

when I merge it, if you all agree :)

Thanks,
Zorro

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

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

[Zorro Lang]

On Sun, Dec 22, 2024 at 08:22:46PM +0800, Zorro Lang wrote:
> On Thu, Dec 19, 2024 at 02:53:31PM -0800, Darrick J. Wong wrote:
> > 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
> 
> Maybe better to use "_has_logdev", due to there's "$USE_EXTERNAL".
> I can change this patch as:
> 
>   # 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" ] && _has_logdev; then

Sorry, should be "! _has_logdev".

>   	_scratch_mkfs -l size=256M
>   else
>   	_scratch_mkfs
>   fi
> 
> when I merge it, if you all agree :)
> 
> Thanks,
> Zorro
> 
> > 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
> >