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

[Christoph Hellwig <hch@lst.de>]

On Thu, Nov 21, 2024 at 08:04:15AM -0800, Darrick J. Wong 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.
> 
> Yes, that's my recollection too -- performing a logwrite replay of an
> old mark means that you can end up with blocks with the correct fs uuid
> but an LSN that's higher than anything in the log.  Recovery will then
> skip the block replay, which is not correct.
> 
> I suppose we could fix log recovery to treat incoming block LSNs that
> are higher than the log head as if there were no block contents at all.
> OTOH going backwards in time isn't usually a concern...right?

It's probably the best we can do.  Recover as far as everything validated
and then give up.

> 
> > Therefore the reason for using dm-thin was that it was an easy way to
> > provide predictable behavior to the test program, where discards punch
> > out blocks that subsequently return zeroes.
> 
> Yep.  The test needs to reset the block device to a zeroed state.
> Discards get us there quickly, but only if discard_zeroes_data==1.
> Hence bolting dm-thinp (where this is guaranteed) onto the logwrites
> tests.

discard_zeroes_data was unfortunately always broken as no standard
gives you any such guarantee.  The best you get is a guarantee that
it returns zeroes if it actually deallocated the block, but if it
deallocates a given block or not is a black box.

> 
> > 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.
> 
> Not sure -- I think BLKZEROOUT will cause allocations and real disk
> writes if we're not careful.

If the device reports a queue/write_zeroes_max_bytes it supports a hardware
offload.  That could still write zeroes to the media if the device
is stupid enough, but hopefully not many are.