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

Christoph Hellwig
2024-11-22T12:34:45Z

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.