| Message ID | 20241215051242.3340572-3-tytso@mit.edu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix recent regressions from for_next | expand |
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.
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.
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.
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 >
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 > > > > >
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.
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
<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
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)
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(-)