xfs
[Top] [All Lists]

Re: [PATCH v2] xfstests: add test for btrfs-progs restore feature

To: Filipe David Borba Manana <fdmanana@xxxxxxxxx>
Subject: Re: [PATCH v2] xfstests: add test for btrfs-progs restore feature
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 26 Feb 2014 06:54:20 +1100
Cc: xfs@xxxxxxxxxxx, linux-btrfs@xxxxxxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1393353848-26790-1-git-send-email-fdmanana@xxxxxxxxx>
References: <1393352816-26065-1-git-send-email-fdmanana@xxxxxxxxx> <1393353848-26790-1-git-send-email-fdmanana@xxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Feb 25, 2014 at 06:44:08PM +0000, Filipe David Borba Manana wrote:
> This is a regression test to verify that the restore feature of btrfs-progs
> is able to correctly recover files that have compressed extents, specially 
> when
> the respective file extent items have a non-zero data offset field.
> 
> This issue is fixed by the following btrfs-progs patch:
> 
>     Btrfs-progs: fix restore of files with compressed extents
> 
> Signed-off-by: Filipe David Borba Manana <fdmanana@xxxxxxxxx>
....
> +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

here=`pwd`

> +
> +_cleanup()
> +{
> +    rm -fr $tmp
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs btrfs
> +_supported_os Linux
> +_require_scratch
> +_need_to_be_root
> +
> +rm -f $seqres.full
> +
> +test_btrfs_restore()
> +{
> +     if [ -z $1 ]
> +     then
> +             OPTIONS=""
> +     else
> +             OPTIONS="-o compress-force=$1"
> +     fi
> +     _scratch_mkfs >/dev/null 2>&1
> +     _scratch_mount $OPTIONS
> +
> +     $XFS_IO_PROG -f -c "pwrite -S 0xff -b 100000 0 100000" \
> +             $SCRATCH_MNT/foo | _filter_xfs_io
> +
> +     # Ensure a single file extent item is persisted.
> +     _run_btrfs_util_prog filesystem sync $SCRATCH_MNT

What's the difference here between "sync" and the command run above?
Unless there's some specific reason for using the above command (and
that needs to be commented), I think that sync(1) should be used
instead in all tests.

Indeed, why a separate command - just adding a "-c fsync" to the
xfs_io command, or even -s to make it open the file O_SYNC should do
what you need without needing a specific sync command....


> +
> +     $XFS_IO_PROG -c "pwrite -S 0xaa -b 100000 100000 100000" \
> +             $SCRATCH_MNT/foo | _filter_xfs_io
> +
> +     # Now ensure a second one is created (and not merged with previous one).
> +     _run_btrfs_util_prog filesystem sync $SCRATCH_MNT
> +
> +     # Make the extent item be split into several ones, each with a data
> +     # offset field != 0
> +     $XFS_IO_PROG -c "pwrite -S 0x1e -b 2 10000 2" $SCRATCH_MNT/foo \
> +             | _filter_xfs_io
> +     $XFS_IO_PROG -c "pwrite -S 0xd0 -b 11 33000 11" $SCRATCH_MNT/foo \
> +             | _filter_xfs_io
> +     $XFS_IO_PROG -c "pwrite -S 0xbc -b 100 99000 100" $SCRATCH_MNT/foo \
> +             | _filter_xfs_io
> +
> +     md5sum $SCRATCH_MNT/foo | _filter_scratch

So you are doing this with first having "persisted" the new extents.
Seems kind of strange that you need to persist some and not
others...

> +     _scratch_unmount
> +     _check_scratch_fs

_check_scratch_fs should be unmounting the SCRATCH_DEV itself
internally. If it's not doing that for btrfs, then the btrfs check
code needs fixing. ;)

> +
> +     _run_btrfs_util_prog restore $SCRATCH_DEV $tmp
> +     md5sum $tmp/foo | cut -d ' ' -f 1

What, exactly, are you restoring to /tmp/$$? Does this assume that
/tmp is a btrfs filesystem? If so, that is an invalid assumption -
/tmp can be any type of filesystem at all.

It's also wrong to use $tmp like this....

> +}
> +
> +mkdir $tmp
> +echo "Testing restore of file compressed with lzo"
> +test_btrfs_restore "lzo"
> +echo "Testing restore of file compressed with zlib"
> +test_btrfs_restore "zlib"
> +echo "Testing restore of file without any compression"
> +test_btrfs_restore

Yup, using $tmp like this is definitely wrong. $tmp is really for test
harness files and test logs, not for *test data*. TEST_DIR is what you
should be using here, not $tmp.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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