Re: [PATCH] generic: test for deduplication between different files

[Eryu Guan <guaneryu@gmail.com>]

On Sun, Aug 19, 2018 at 04:41:31PM +0100, Filipe Manana wrote:
> On Sun, Aug 19, 2018 at 3:07 PM, Eryu Guan <guaneryu@gmail.com> wrote:
> > On Fri, Aug 17, 2018 at 09:39:24AM +0100, fdmanana@kernel.org wrote:
> >> From: Filipe Manana <fdmanana@suse.com>
> >>
> >> Test that deduplication of an entire file that has a size that is not
> >> aligned to the filesystem's block size into a different file does not
> >> corrupt the destination's file data.
> >>
> >> This test is motivated by a bug found in Btrfs which is fixed by the
> >> following patch for the linux kernel:
> >>
> >>   "Btrfs: fix data corruption when deduplicating between different files"
> >>
> >> XFS also fails this test, at least as of linux kernel 4.18-rc7, exactly
> >> with the same corruption as in Btrfs - some bytes of a block get replaced
> >> with zeroes after the deduplication.
> >>
> >> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> >> ---
> >>  tests/generic/505     | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/generic/505.out | 33 ++++++++++++++++++++
> >>  tests/generic/group   |  1 +
> >>  3 files changed, 118 insertions(+)
> >>  create mode 100755 tests/generic/505
> >>  create mode 100644 tests/generic/505.out
> >>
> >> diff --git a/tests/generic/505 b/tests/generic/505
> >> new file mode 100755
> >> index 00000000..5ee232a2
> >> --- /dev/null
> >> +++ b/tests/generic/505
> >> @@ -0,0 +1,84 @@
> >> +#! /bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (C) 2018 SUSE Linux Products GmbH. All Rights Reserved.
> >> +#
> >> +# FS QA Test No. 505
> >> +#
> >> +# Test that deduplication of an entire file that has a size that is not aligned
> >> +# to the filesystem's block size into a different file does not corrupt the
> >> +# destination's file data.
> >> +#
> >> +seq=`basename $0`
> >> +seqres=$RESULT_DIR/$seq
> >> +echo "QA output created by $seq"
> >> +tmp=/tmp/$$
> >> +status=1     # failure is the default!
> >> +trap "_cleanup; exit \$status" 0 1 2 3 15
> >> +
> >> +_cleanup()
> >> +{
> >> +     cd /
> >> +     rm -f $tmp.*
> >> +}
> >> +
> >> +# get standard environment, filters and checks
> >> +. ./common/rc
> >> +. ./common/filter
> >> +. ./common/reflink
> >> +
> >> +# real QA test starts here
> >> +_supported_fs generic
> >> +_supported_os Linux
> >> +_require_scratch_dedupe
> >> +
> >> +rm -f $seqres.full
> >> +
> >> +_scratch_mkfs >>$seqres.full 2>&1
> >> +_scratch_mount
> >> +
> >> +# The first byte with a value of 0xae starts at an offset (2518890) which is not
> >> +# a multiple of the block size.
> >> +$XFS_IO_PROG -f \
> >> +     -c "pwrite -S 0x6b 0 2518890" \
> >> +     -c "pwrite -S 0xae 2518890 102398" \
> >> +     $SCRATCH_MNT/foo | _filter_xfs_io
> >> +
> >> +# Create a second file with a length not aligned to the block size, whose bytes
> >> +# all have the value 0x6b, so that its extent(s) can be deduplicated with the
> >> +# first file.
> >> +$XFS_IO_PROG -f -c "pwrite -S 0x6b 0 557771" $SCRATCH_MNT/bar | _filter_xfs_io
> >> +
> >> +# The file is filled with bytes having the value 0x6b from offset 0 to offset
> >> +# 2518889 and with the value 0xae from offset 2518890 to offset 2621287.
> >> +echo "File content before deduplication:"
> >> +od -t x1 $SCRATCH_MNT/foo
> >> +
> >> +# Now deduplicate the entire second file into a range of the first file that
> >> +# also has all bytes with the value 0x6b. The destination range's end offset
> >> +# must not be aligned to the block size and must be less then the offset of
> >> +# the first byte with the value 0xae (byte at offset 2518890).
> >> +$XFS_IO_PROG -c "dedupe $SCRATCH_MNT/bar 0 1957888 557771" $SCRATCH_MNT/foo \
> >> +     | _filter_xfs_io
> >> +
> >> +# The bytes in the range starting at offset 2515659 (end of the deduplication
> >> +# range) and ending at offset 2519040 (start offset rounded up to the block
> >> +# size) must all have the value 0xae (and not replaced with 0x00 values).
> >
> > This doesn't seem right to me, range [2515659, 2518890) should be 0x6b
> > not 0xae, while range [2518890, 2519040) indeed should contain 0xae.
> 
> Yes, indeed. My mistake (got it right in the comment before the first
> call to "od").
> Can you fix it up (if there's nothing else to fix), or do you need me
> to send a new version?

Sure, I can fix it on commit. But I've already pushed this week's update
to upstream, so you won't see it until next week :)

Thanks,
Eryu