xfs
[Top] [All Lists]

Re: [PATCH] xfs/062: add xfs unwritten extent data corruption reproducer

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs/062: add xfs unwritten extent data corruption reproducer
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Mon, 22 Sep 2014 15:40:18 -0400
Cc: fstests@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140920234500.GK4267@dastard>
References: <1411150385-32121-1-git-send-email-bfoster@xxxxxxxxxx> <20140920234500.GK4267@dastard>
User-agent: Mutt/1.5.23 (2014-03-12)
On Sun, Sep 21, 2014 at 09:45:00AM +1000, Dave Chinner wrote:
> On Fri, Sep 19, 2014 at 02:13:05PM -0400, Brian Foster wrote:
> > XFS had a data corruption problem where writeback of pages to unwritten
> > extents would fail to run unwritten extent conversion at I/O completion.
> > This causes subsequent reads of written, but unconverted regions to
> > return zeroes. This occurs on sub-page block size filesystems when
> > writeback contends for the inode lock (e.g., with a file writer).
> > 
> > Add a test that creates the conditions to reproduce the data corruption
> > and detect it by looking for unwritten extents after all said extents
> > have been overwritten.
> > 
> > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > ---
> > 
> > Hi all,
> > 
> > Here's a test for the data corruption issue I sent a fix for yesterday:
> > 
> >     http://oss.sgi.com/archives/xfs/2014-09/msg00259.html
> > 
> > It took a little time to improve the effectiveness of the test, but I've
> > been able to run it in current form for 90+ iterations without a false
> > negative (e.g., test passes when it shouldn't) on my setup. It runs in
> > ~30s when there is no failure.
> > 
> > Brian
> > 
> >  tests/xfs/062     | 114 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/062.out |   5 +++
> >  tests/xfs/group   |   1 +
> >  3 files changed, 120 insertions(+)
> >  create mode 100755 tests/xfs/062
> >  create mode 100644 tests/xfs/062.out
> 
> There's nothing XFS specific about this test. Yes, it exposes a bug
> on XFS, but every filesystem should be able to run and pass this
> test.
> 

Indeed, converted to a generic test.

> > 
> > diff --git a/tests/xfs/062 b/tests/xfs/062
> > new file mode 100755
> > index 0000000..61f8cf4
> > --- /dev/null
> > +++ b/tests/xfs/062
...
> > +   if [ $? == 0 ]
> > +   then
> > +           xfs_bmap -v $SCRATCH_MNT/file
> > +           break
> > +   fi
> 
>       if [...]; then.
> 
> That's also a failure, right? So shouldn't it set status=1 and exit,
> leaving the cleanup function to kill and wait for everything to
> finish?
> 

It's the main/expected failure. The intent was to use the hexdump as the
determination for success/failure, but I had to determine when to break
out of the loop since the reproducer requires several iterations. I
figured the raw extent map output would be useful (the output of which
also calls out something is wrong when compared to the golden output) as
well as the content diff, so both are printed in the failure case.

I've added an additional comment here for the v2 I'm about to post. I
can just dump the extent map here and exit with status=1 if that is
preferred, but it seems appropriate to include the hexdump in the
failure scenario as well.

Brian

> > +done
> > +
> > +echo $iters iterations
> > +
> > +kill $syncpid
> > +wait
> > +
> > +# clear page cache and dump the file
> > +_scratch_unmount
> > +_scratch_mount
> 
> _scratch_remount
> 
> > +hexdump $SCRATCH_MNT/file
> > +
> > +_scratch_unmount
> > +_check_scratch_fs
> 
> Don't need to do that anymore.
> 
> > +
> > +status=0
> > +exit
> > diff --git a/tests/xfs/062.out b/tests/xfs/062.out
> > new file mode 100644
> > index 0000000..420f2e4
> > --- /dev/null
> > +++ b/tests/xfs/062.out
> > @@ -0,0 +1,5 @@
> > +QA output created by 062
> > +100 iterations
> > +0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
> > +*
> > +0100000
> > diff --git a/tests/xfs/group b/tests/xfs/group
> > index 09bce15..685cbe7 100644
> > --- a/tests/xfs/group
> > +++ b/tests/xfs/group
> > @@ -57,6 +57,7 @@
> >  059 dump ioctl auto quick
> >  060 dump ioctl auto quick
> >  061 dump ioctl auto quick
> > +062 auto quick
> 
> and the rw group, too.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>