diff mbox series

[v2] generic: disable dmlogwrites tests on XFS

Message ID 20200827145329.435398-1-bfoster@redhat.com
State New
Headers show
Series [v2] generic: disable dmlogwrites tests on XFS | expand

Commit Message

Brian Foster Aug. 27, 2020, 2:53 p.m. UTC
Several generic fstests use dm-log-writes to test the filesystem for
consistency at various crash recovery points. dm-log-writes and the
associated replay mechanism rely on discard to clear stale blocks
when moving to various points in time of the fs. If the storage
doesn't provide discard zeroing or the discard requests exceed the
hardcoded maximum (128MB) of the fallback solution to physically
write zeroes, stale blocks are left around in the target fs. This
causes issues on XFS if recovery observes metadata from a future
version of an fs that has been replayed to an older point in time.
This corrupts the filesystem and leads to spurious test failures
that are nontrivial to diagnose.

Disable the generic dmlogwrites tests on XFS for the time being.
This is intended to be a temporary change until a solution is found
that allows these tests to predictably clear stale data while still
allowing them to run in a reasonable amount of time.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---

v2:
- Drop all dmthinp changes. Unconditionally disable tests on XFS.
v1: https://lore.kernel.org/fstests/20200826143815.360002-2-bfoster@redhat.com/

 common/dmlogwrites | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Christoph Hellwig Aug. 29, 2020, 6:48 a.m. UTC | #1
On Thu, Aug 27, 2020 at 10:53:29AM -0400, Brian Foster wrote:
> Several generic fstests use dm-log-writes to test the filesystem for
> consistency at various crash recovery points. dm-log-writes and the
> associated replay mechanism rely on discard to clear stale blocks
> when moving to various points in time of the fs. If the storage
> doesn't provide discard zeroing or the discard requests exceed the
> hardcoded maximum (128MB) of the fallback solution to physically
> write zeroes, stale blocks are left around in the target fs. This
> causes issues on XFS if recovery observes metadata from a future
> version of an fs that has been replayed to an older point in time.
> This corrupts the filesystem and leads to spurious test failures
> that are nontrivial to diagnose.
> 
> Disable the generic dmlogwrites tests on XFS for the time being.
> This is intended to be a temporary change until a solution is found
> that allows these tests to predictably clear stale data while still
> allowing them to run in a reasonable amount of time.

As said in the other discussion I don't think this is correct.  The
intent of the tests is to ensure the data can't be read.  You just
happen to trigger over that with XFS, but it also means that tests
don't work correctly on other file systems in that configuration.
Brian Foster Aug. 31, 2020, 1:37 p.m. UTC | #2
On Sat, Aug 29, 2020 at 07:48:50AM +0100, Christoph Hellwig wrote:
> On Thu, Aug 27, 2020 at 10:53:29AM -0400, Brian Foster wrote:
> > Several generic fstests use dm-log-writes to test the filesystem for
> > consistency at various crash recovery points. dm-log-writes and the
> > associated replay mechanism rely on discard to clear stale blocks
> > when moving to various points in time of the fs. If the storage
> > doesn't provide discard zeroing or the discard requests exceed the
> > hardcoded maximum (128MB) of the fallback solution to physically
> > write zeroes, stale blocks are left around in the target fs. This
> > causes issues on XFS if recovery observes metadata from a future
> > version of an fs that has been replayed to an older point in time.
> > This corrupts the filesystem and leads to spurious test failures
> > that are nontrivial to diagnose.
> > 
> > Disable the generic dmlogwrites tests on XFS for the time being.
> > This is intended to be a temporary change until a solution is found
> > that allows these tests to predictably clear stale data while still
> > allowing them to run in a reasonable amount of time.
> 
> As said in the other discussion I don't think this is correct.  The
> intent of the tests is to ensure the data can't be read.  You just
> happen to trigger over that with XFS, but it also means that tests
> don't work correctly on other file systems in that configuration.
> 

Yes, but the goal of this patch is not to completely fix the dmlogwrites
infrastructure and set of tests. The goal is to disable a subset of
tests that are known to produce spurious corruptions on XFS until that
issue can be addressed, so it doesn't result in continued bug reports in
the meantime. I don't run these tests routinely on other fs', so it's
not really my place to decide that the tradeoff between this problem and
the ability of the test to reproduce legitimate bugs justifies disabling
the test on those configs.

Brian
Christoph Hellwig Aug. 31, 2020, 4:02 p.m. UTC | #3
On Mon, Aug 31, 2020 at 09:37:32AM -0400, Brian Foster wrote:
> Yes, but the goal of this patch is not to completely fix the dmlogwrites
> infrastructure and set of tests. The goal is to disable a subset of
> tests that are known to produce spurious corruptions on XFS until that
> issue can be addressed, so it doesn't result in continued bug reports in
> the meantime. I don't run these tests routinely on other fs', so it's
> not really my place to decide that the tradeoff between this problem and
> the ability of the test to reproduce legitimate bugs justifies disabling
> the test on those configs.

So my problem is that XFS here is the messenger - this could screw up
every other file system just as much.  So if we just want to disable
the tests for now we should do it for all file systems, not just for
the one that found the problem with the test.
Amir Goldstein Sept. 1, 2020, 6:25 a.m. UTC | #4
On Mon, Aug 31, 2020 at 4:37 PM Brian Foster <bfoster@redhat.com> wrote:
>
> On Sat, Aug 29, 2020 at 07:48:50AM +0100, Christoph Hellwig wrote:
> > On Thu, Aug 27, 2020 at 10:53:29AM -0400, Brian Foster wrote:
> > > Several generic fstests use dm-log-writes to test the filesystem for
> > > consistency at various crash recovery points. dm-log-writes and the
> > > associated replay mechanism rely on discard to clear stale blocks
> > > when moving to various points in time of the fs. If the storage
> > > doesn't provide discard zeroing or the discard requests exceed the
> > > hardcoded maximum (128MB) of the fallback solution to physically
> > > write zeroes, stale blocks are left around in the target fs. This
> > > causes issues on XFS if recovery observes metadata from a future
> > > version of an fs that has been replayed to an older point in time.
> > > This corrupts the filesystem and leads to spurious test failures
> > > that are nontrivial to diagnose.
> > >
> > > Disable the generic dmlogwrites tests on XFS for the time being.
> > > This is intended to be a temporary change until a solution is found
> > > that allows these tests to predictably clear stale data while still
> > > allowing them to run in a reasonable amount of time.
> >
> > As said in the other discussion I don't think this is correct.  The
> > intent of the tests is to ensure the data can't be read.  You just
> > happen to trigger over that with XFS, but it also means that tests
> > don't work correctly on other file systems in that configuration.
> >
>
> Yes, but the goal of this patch is not to completely fix the dmlogwrites
> infrastructure and set of tests. The goal is to disable a subset of
> tests that are known to produce spurious corruptions on XFS until that
> issue can be addressed, so it doesn't result in continued bug reports in
> the meantime. I don't run these tests routinely on other fs', so it's
> not really my place to decide that the tradeoff between this problem and
> the ability of the test to reproduce legitimate bugs justifies disabling
> the test on those configs.
>

Brian,

Let's not take this course please.
Please post patches v1 2/4-4/4 without patch v1 1/4
The only objection was to patch 1/4 and it is not strictly needed
to solve the problem you care about.

I had a *concern* about pacthes 2-4, but I can live with that
concern and it is certainly preferred to disabling the tests.

I can follow up with fixing the dmlogwrites common helpers
later when I get the time, so they do not rely on discard for
correctness of replay.

As I wrote, all it takes is to issue an explicit zero/punch command
in the beginning of replay halpers. Just need to find the command
that works correctly and most efficiently with thinp.

If you have the time to do that (since I believe you already tested
some commands) that would be great. Otherwise, I'll do that later.

Thanks,
Amir.
Brian Foster Sept. 1, 2020, 12:31 p.m. UTC | #5
On Tue, Sep 01, 2020 at 09:25:42AM +0300, Amir Goldstein wrote:
> On Mon, Aug 31, 2020 at 4:37 PM Brian Foster <bfoster@redhat.com> wrote:
> >
> > On Sat, Aug 29, 2020 at 07:48:50AM +0100, Christoph Hellwig wrote:
> > > On Thu, Aug 27, 2020 at 10:53:29AM -0400, Brian Foster wrote:
> > > > Several generic fstests use dm-log-writes to test the filesystem for
> > > > consistency at various crash recovery points. dm-log-writes and the
> > > > associated replay mechanism rely on discard to clear stale blocks
> > > > when moving to various points in time of the fs. If the storage
> > > > doesn't provide discard zeroing or the discard requests exceed the
> > > > hardcoded maximum (128MB) of the fallback solution to physically
> > > > write zeroes, stale blocks are left around in the target fs. This
> > > > causes issues on XFS if recovery observes metadata from a future
> > > > version of an fs that has been replayed to an older point in time.
> > > > This corrupts the filesystem and leads to spurious test failures
> > > > that are nontrivial to diagnose.
> > > >
> > > > Disable the generic dmlogwrites tests on XFS for the time being.
> > > > This is intended to be a temporary change until a solution is found
> > > > that allows these tests to predictably clear stale data while still
> > > > allowing them to run in a reasonable amount of time.
> > >
> > > As said in the other discussion I don't think this is correct.  The
> > > intent of the tests is to ensure the data can't be read.  You just
> > > happen to trigger over that with XFS, but it also means that tests
> > > don't work correctly on other file systems in that configuration.
> > >
> >
> > Yes, but the goal of this patch is not to completely fix the dmlogwrites
> > infrastructure and set of tests. The goal is to disable a subset of
> > tests that are known to produce spurious corruptions on XFS until that
> > issue can be addressed, so it doesn't result in continued bug reports in
> > the meantime. I don't run these tests routinely on other fs', so it's
> > not really my place to decide that the tradeoff between this problem and
> > the ability of the test to reproduce legitimate bugs justifies disabling
> > the test on those configs.
> >
> 
> Brian,
> 
> Let's not take this course please.
> Please post patches v1 2/4-4/4 without patch v1 1/4
> The only objection was to patch 1/4 and it is not strictly needed
> to solve the problem you care about.
> 

Sure, I'm fine with that approach. I posted this because it was
essentially where I started and there doesn't seem to be much agreement
on a proper fix.  We had precedent to use dm-thinp in generic/482, so it
seemed ideal to at least try to keep the tests active in the near term.
IOW, my approach was to either try for an obvious/simple fix to keep the
tests active or otherwise disable the tests (at least on XFS) until a
more involved fix is agreed on, tested and implemented. That more
involved fix could be anything from genericizing the dm-thin approach to
replacing it such that discard zeroing is not a critical component, but
I don't want to gate addressing the spurious corruption problem on a
nontrivial rework of the test mechanism.

> I had a *concern* about pacthes 2-4, but I can live with that
> concern and it is certainly preferred to disabling the tests.
> 

Agree.

> I can follow up with fixing the dmlogwrites common helpers
> later when I get the time, so they do not rely on discard for
> correctness of replay.
> 
> As I wrote, all it takes is to issue an explicit zero/punch command
> in the beginning of replay halpers. Just need to find the command
> that works correctly and most efficiently with thinp.
> 
> If you have the time to do that (since I believe you already tested
> some commands) that would be great. Otherwise, I'll do that later.
> 

Well, I was testing some of the zeroing commands Christoph mentioned
moreso than punch, particularly with intent to remove the dependency on
dm-thin. The problem with that is I don't think it helps much for
anybody who is testing on devices without hardware offload. The more
efficient in-kernel zeroing is still pretty slow, so I suppose it
depends on how often it must be invoked in a particular test (once? per
recovery point? on-demand buried down in the recovery code?). An obvious
mitigation, at least for the generic tests, is to reduce the size of the
target devices such that the manual zeroing is less noticeable. I don't
see much impact of using a 100MB fs, for example, and the recovery tool
already does the manual zeroing. The tradeoff is that I think we'd want
some kind of _notrun check in situations where we know the zeroing
doesn't occur.

I don't see much difference with zero/punch on dm-thinp. An
fallocate(FL_PUNCH_HOLE|FL_KEEP_SIZE) doesn't work because it explicitly
requests hardware zeroing, which I don't have.
fallocate(FL_ZERO_RANGE|FL_KEEP_SIZE) works, but takes a minute or two
on my 10G device because it falls back to manual zeroing. There is a
NO_HIDE_STALE variant of PUNCH_HOLE, but I don't seem to have any
userspace tools that define NO_HIDE_STALE and it looks like it just
sends discards anyways. Of course, a 'blkdiscard -o 0 -l 10g <thindev>'
unmaps nearly the entire device in ~1s, but then we're back to the
argument of using discard for zeroing. :P

Brian

> Thanks,
> Amir.
>
Amir Goldstein Sept. 1, 2020, 2:04 p.m. UTC | #6
> I don't see much difference with zero/punch on dm-thinp. An
> fallocate(FL_PUNCH_HOLE|FL_KEEP_SIZE) doesn't work because it explicitly
> requests hardware zeroing, which I don't have.
> fallocate(FL_ZERO_RANGE|FL_KEEP_SIZE) works, but takes a minute or two
> on my 10G device because it falls back to manual zeroing. There is a
> NO_HIDE_STALE variant of PUNCH_HOLE, but I don't seem to have any
> userspace tools that define NO_HIDE_STALE and it looks like it just
> sends discards anyways. Of course, a 'blkdiscard -o 0 -l 10g <thindev>'
> unmaps nearly the entire device in ~1s, but then we're back to the
> argument of using discard for zeroing. :P
>

I don't think that is a problem if we build the test around thinp and its
well defined behavior on discard. This is what I was getting at:
1. Move dm-thinp setup inside the dm-logwrite helpers, so dm-logwrites
    tests cannot be written without dm-thinp by mistake.
2. Use explicit discard in start of replay helper to zap everything before
    replay

I'll add to my TODO list.

Thanks,
Amir.
diff mbox series

Patch

diff --git a/common/dmlogwrites b/common/dmlogwrites
index 573f4b8a..b0a28ce8 100644
--- a/common/dmlogwrites
+++ b/common/dmlogwrites
@@ -9,6 +9,14 @@  _require_log_writes()
 	[ -z "$LOGWRITES_DEV" -o ! -b "$LOGWRITES_DEV" ] && \
 		_notrun "This test requires a valid \$LOGWRITES_DEV"
 
+	# The logwrites mechanism relies on discard to provide zeroing behavior
+	# to clear out stale filesystem content. Discard doesn't reliably
+	# provide this behavior, and this leads to spurious corruptions on XFS
+	# filesystems by leaving out of order metadata in the fs. We must
+	# disable dmlogwrites on XFS until it implements a predictable mechanism
+	# to clear stale data.
+	[ $FSTYP == "xfs" ] && _notrun "dmlogwrites not supported on XFS"
+
 	_exclude_scratch_mount_option dax
 	_require_dm_target log-writes
 	_require_test_program "log-writes/replay-log"
@@ -39,6 +47,8 @@  _require_log_writes_dax_mountopt()
 	[ -z "$LOGWRITES_DEV" -o ! -b "$LOGWRITES_DEV" ] && \
 		_notrun "This test requires a valid \$LOGWRITES_DEV"
 
+	[ $FSTYP == "xfs" ] && _notrun "dmlogwrites not supported on XFS"
+
 	_require_dm_target log-writes
 	_require_test_program "log-writes/replay-log"