xfs
[Top] [All Lists]

Re: [PATCH v2] generic/032: add xfs unwritten extent data corruption rep

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH v2] generic/032: add xfs unwritten extent data corruption reproducer
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 23 Sep 2014 11:26:56 +1000
Cc: fstests@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1411414836-64598-1-git-send-email-bfoster@xxxxxxxxxx>
References: <1411414836-64598-1-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Sep 22, 2014 at 03:40:36PM -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>

I still think the error handling for the unwritten extent case is
wrong. Failure debug is exactly what $seqres.full is for, so that's
where failure information should go. If we detect a failure case
and have to abort immediately, then _fail() should be used. And
_fail() leaves a message to look at $seqres.full for details.

So:

> +     # Check for unwritten extents. We should have none since we wrote over
> +     # the entire preallocated region and ran fsync.
> +     $XFS_IO_PROG \
> +             -c "fiemap -v" \
> +             $SCRATCH_MNT/file | _filter_fiemap | \
> +             grep unwritten >> $seqres.full 2>&1
> +     if [ $? == 0 ]; then
> +             # data corruption! dump the extent list and break out to dump
> +             # the file content
> +             $XFS_IO_PROG -c "fiemap -v" $SCRATCH_MNT/file
> +             break
> +     fi

Can simply be:

        $XFS_IO_PROG -c "fiemap -v" $SCRATCH_MNT/file | \
                tee -a $seqres.full | \
                filter_fiemap | grep unwritten
        [ $? == 0 ] && _fail "Unwritten extents found!"

and will result in the output:

generic/032  0s.... [failed, exit status 1] - output mismatch (see 
....results/generic/032.bad)

And results/generic/032.bad will contain:

....
Unwritten extents found!
(see ..../results/generic/032.full for details)

And the complete fiemap output will be in results/generic/032.full.

> +done
> +
> +echo $iters iterations
> +
> +kill $syncpid
> +wait
> +
> +# clear page cache and dump the file
> +_scratch_remount
> +hexdump $SCRATCH_MNT/file
> +
> +_scratch_unmount

No need to unmount. check does that when checking the filesystem,
and if not the next _require_scratch call will do it....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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