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

[Amir Goldstein <amir73il@gmail.com>]

On Sat, Aug 29, 2020 at 9:47 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Aug 27, 2020 at 02:35:07PM -0400, Brian Foster wrote:
> > OTOH, perhaps the thinp behavior could be internal, but conditional
> > based on XFS. It's not really clear to me if this problem is more of an
> > XFS phenomenon or just that XFS happens to have some unique recovery
> > checking logic that explicitly detects it. It seems more like the
> > latter, but I don't know enough about ext4 or btrfs to say..
>
> The way I understand the tests (and Josefs mail seems to confirm that)
> is that it uses discards to ensure data disappears.  Unfortunately
> that's only how discard sometimes work, but not all the time.
>

I think we are confusing two slightly different uses of discard in those tests.
One use case is that dm-logwrites records discards in test workloads and then
needs to replay them to simulate the sequence of IO event up to a crash point.

I think that use case is less interesting, because as Christoph points out,
discard is not reliable, so I think we should get rid of " -o discard"
in the tests -
it did not catch any issues that I know of.

But there is another discard in those tests issued by _log_writes_mkfs
(at least it does for xfs and ext4). This discard has the by product of
making sure that replay from the start to point in time, first wipes all
stale data from the replay block device.

Of course the problems we encounter is that it does not wipe all stale
data when not running on dm-thinp, which is why I suggested to always
use dm-thinp for replay device, but I can live perfectly well with Brian's
v1 patches where both workload and replay are done on dm-thinp.

Josef had two variants for those tests, one doing "replay from start"
for every checkpoint and utilizing this discard-as-wipe behavior
and one flavor that used dm-thinp to take snapshots and replay
from snapshot T to the next mark.

I remember someone tried converting some of the tests to the snapshot
flavor, but it turned out to be slower, so we left it as is (always replay from
the start).

In conclusion, I *think* the correct fix for the failing tests is:
1. Use dm-thinp for all those tests (as v1 does)
2. In _log_writes_replay_log{,_range}() start by explicitly
    wiping dm-thinp, either with with hole punch command or by
    re-creating the new thinp volume, whichever is faster.
    instead of relying on the replay of discard operation recorded
    from mkfs that sort of kind of worked by mistake.

Thanks,
Amir.