On Thu, Jun 13, 2024 at 05:04:47PM +1000, Dave Chinner wrote: > On Wed, Jun 12, 2024 at 10:47:50AM -0700, Darrick J. Wong wrote: > > The actual defect here was an overzealous inode verifier, which was > > fixed in a separate patch. This patch adds some transaction precommit > > functions for CONFIG_XFS_DEBUG=y mode so that we can detect these kinds > > of transient errors at transaction commit time, where it's much easier > > to find the root cause. > > Ok, I can see the value in this for very strict integrity checking, > but I don't think that XONFIG_XFS_DEBUG context is right > for this level of checking. > > Think of the difference using xfs_assert_ilocked() with > CONFIG_XFS_DEBUG vs iusing CONFIG_PROVE_LOCKING to enable lockdep. > Lockdep checks a lot more about lock usage than our debug build > asserts and so may find deep, subtle issues that our asserts won't > find. However, that extra capability comes at a huge cost for > relatively little extra gain, and so most of the time people work > without CONFIG_PROVE_LOCKING enabled. A test run here or there, and > then when the code developement is done, but it's not used all the > time on every little change that is developed and tested. > > In comparison, I can't remember the last time I did any testing with > CONFIG_XFS_DEBUG disabled. Even all my performance regression > testing is run with CONFIG_XFS_DEBUG=y, and a change like this one > would make any sort of load testing on debug kernels far to costly > and so all that testing would get done with debugging turned off. > That's a significant loss, IMO, because we'd lose more validation > from people turning CONFIG_XFS_DEBUG off than we'd gain from the > rare occasions this new commit verifier infrastructure would catch > a real bug. > > Hence I think this should be pushed into a separate debug config > sub-option. Make it something we can easily turn on with > KASAN and lockdep when we our periodic costly extensive validation > test runs. Do you want a CONFIG_XFS_DEBUG_EXPENSIVE=y guard, then? Some of the bmbt scanning debug things might qualify for that too. --D > -Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx >
