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