Re: [PATCH 01/12] generic/757: fix various bugs in this test

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

On Fri, Nov 22, 2024 at 11:33:24AM -0500, Brian Foster wrote:
> On Fri, Nov 22, 2024 at 08:13:47AM -0800, Darrick J. Wong wrote:
> > On Fri, Nov 22, 2024 at 08:49:42AM -0500, Brian Foster wrote:
> > > On Fri, Nov 22, 2024 at 01:31:33PM +0100, Christoph Hellwig wrote:
> > > > On Thu, Nov 21, 2024 at 09:11:56AM -0500, Brian Foster wrote:
> > > > > > I'm all for speeding up tests.  But relying on a unspecified side effect
> > > > > > of an operation and then requiring a driver that implements that side
> > > > > > effect without documenting that isn't really good practice.
> > > > > > 
> > > > > 
> > > > > It's a hack to facilitate test coverage. It would obviously need to be
> > > > > revisited if behavior changed sufficiently to break the test.
> > > > > 
> > > > > I'm not really sure what you're asking for wrt documentation. A quick
> > > > > scan of the git history shows the first such commit is 65cc9a235919
> > > > > ("generic/482: use thin volume as data device"), the commit log for
> > > > > which seems to explain the reasoning.
> > > > 
> > > > A comment on _log_writes_init that it must only be used by dm-thin
> > > > because it relies on the undocumented behavior that dm-trim zeroes
> > > > all blocks discarded.
> > > > 
> > > > Or even better my moving the dm-think setup boilerplate into the log
> > > > writes helpers, so that it gets done automatically.
> > > > 
> > > 
> > > A related idea might be to incorporate your BLKZEROOUT fix so the core
> > > tool is fundamentally correct, but then wrap the existing discard
> > > behavior in a param or something that the dm-thin oriented tests can
> > > pass to enable it as a fast zero hack/optimization.
> > > 
> > > But that all seems reasonable to me either way. I'm not sure that's
> > > something I would have fully abstracted into the logwrites stuff
> > > initially, but here we are ~5 years later and it seems pretty much every
> > > additional logwrites test has wanted the same treatment. If whoever
> > > wants to convert this newer test over wants to start by refactoring
> > > things that way, that sounds like a welcome cleanup to me.
> > 
> > Ugh, I just want to fix this stupid test and move on with the bugfixes,
> > not refactor every logwrites user in the codebase just to reduce one
> > test's runtime from hours to 90s.
> > 
> > It's not as simple as making the logwrites init function set up thinp on
> > its own, because there's at least one test out there (generic/470) that
> > takes care of its own discarding, and then there's whatever the strange
> > stuff that the tests/btrfs/ users do -- it looks fairly simple, but I
> > don't really want to go digging into that just to make sure I didn't
> > break their testing.
> > 
> > I'll send what I have currently, which adds a warning about running
> > logwrites on a device that supports discard but isn't thinp... in
> > addition to fixing the xfs log recovery thing, and in addition to fixing
> > the loop duration.
> > 
> > I guess I can add yet another patch to switch the replay program to use
> > BLKDISCARD if the _init function thinks it's ok, but seriously... you
> > guys need to send start sending patches implementing the new
> > functionality that you suggest.
> > 
> 
> Sorry, I should have been more clear. I certainly don't insist on it as
> an immediate change or to gatekeep the current patch. I'm just acking
> the idea, and I think it's perfectly fair to say "more time consuming
> than I have time for right now" if you planned to just fixup the test
> itself. I may get to it opportunistically someday, or if hch cares
> enough about it he's certainly capable of picking it up sooner.
> 
> For future reference, I'm generally not trying to tell people what to do
> with their patches or force work on people, etc. I realize we have a
> tendency to do that. I don't like it either. It would be nice if we had
> a clearer way to express/discuss an idea without implying it as a
> demand.

/me suggests

"Here's something that we ought to do, though as a separate patchset:
clean up all the $fubar to be $less_fubar.  In the meantime this patch
is good enough for now."

--D

> Brian
> 
> > --D
> > 
> > > Brian
> > > 
> > > 
> > 
>