Re: [PATCH 1/4] generic: require discard zero behavior for dmlogwrites on XFS

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Aug 27, 2020 at 10:02 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Aug 27, 2020 at 09:58:09AM +0300, Amir Goldstein wrote:
> > I imagine that ext4 could also be burned by this.
> > Do we have a reason to limit this requirement to xfs?
> > I prefer to make it generic.
>
> The whole idea that discard zeroes data is just completely broken.
> Discard is a hint that is explicitly allowed to do nothing.

I figured you'd say something like that :)
but since we are talking about dm-thin as a solution for predictable
behavior at the moment and this sanity check helps avoiding adding
new tests that can fail to some extent, is the proposed bandaid good enough
to keep those tests alive until a better solution is proposed?

Another concern that I have is that perhaps adding dm-thin to the fsx tests
would change timing of io in a subtle way and hide the original bugs that they
caught:

47c7d0b19502 ("xfs: fix incorrect log_flushed on fsync")
3af423b03435 ("xfs: evict CoW fork extents when performing finsert/fcollapse")
51e3ae81ec58 ("ext4: fix interaction between i_size, fallocate, and
delalloc after a crash")

Because at the time I (re)wrote Josef's fsx tests, they flushed out the bugs
on spinning rust much more frequently than on SSD.

So Brian, if you could verify that the fsx tests still catch the original bugs
by reverting the fixes or running with an old kernel and probably running
on a slow disk that would be great.

I know it is not a simple request, but I just don't know how else to trust
these changes. I guess a quick way for you to avoid the hassle is to add
_require_discard_zeroes (patch #1) and drop the rest.

Thanks,
Amir.