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

[Brian Foster <bfoster@redhat.com>]

On Thu, Nov 21, 2024 at 02:12:39PM +0100, Christoph Hellwig wrote:
> On Thu, Nov 21, 2024 at 07:28:09AM -0500, Brian Foster wrote:
> > IIRC it was to accommodate the test program, which presumably used
> > discard for efficiency reasons because it did a lot of context switching
> > to different point-in-time variations of the fs. If the discard didn't
> > actually zero the range (depending on the underlying test dev), then at
> > least on XFS, we'd see odd recovery issues and whatnot from the fs going
> > forward/back in time.
> 
> But the fundamental problem is that discard does not actually zero
> data.  It sometimes might, but also often doesn't because it's not
> part of the contract.  That's why my patch two switch to the zeroing
> ioctl is the right thing.  Note that this doesn't mean explicitly
> writing zeroes, it still uses zeroing offloads available in the drivers.
> 

Ok. I get that wrt discard.. I'm saying that's why we inserted dm-thin
into the stack, because it had predictable behavior that fell in line
with this particular design quirk of the higher level test program.

I've no issue with changing it so long as we maintain effectiveness of
the test. I'm not familiar enough with the zeroing offloads in the
drivers you mention to know whether that means this test can still run
quickly in the general case, or only with select hardware..?

> > I don't recall all the specifics, but I thought part of the reason for
> > using discard over explicit zeroing was the latter made the test
> > impractically slow. I could be misremembering, but if you want to change
> > it I'd suggest to at least verify runtimes on some of the preexisting
> > logwrites tests as well.
> 
> 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.

Brian