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@xxxxxxxxxxxxxxxxxxx/ > > [2] https://lore.kernel.org/linux-xfs/Z1FQdYEXLR5BoOE-@xxxxxxxxxx/ > > > > > -Dave. > > > -- > > > Dave Chinner > > > david@xxxxxxxxxxxxx > >
