xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v2] xfstests: add test for btrfs-progs restore feature
From: Filipe David Manana <fdmanana@xxxxxxxxx>
Date: Tue, 25 Feb 2014 21:02:43 +0000
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>, "linux-btrfs@xxxxxxxxxxxxxxx" <linux-btrfs@xxxxxxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:reply-to:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=0BHiObYCLnr3J4K0SdEQdgKODrYwEmWT1HVomWcKTrU=; b=YchH5W2pnB5hsizb5KQABb/78fLofBEjIptPxjRQqS/JCHGB+6uMmQztb3WrrBpQhO OYIY/UNTnSHqrjtV8sJo8FyqScqkpB26thKlaX5LznNUwLO/rTaZctmhL3nV6wwg9BeH P7XqLPA9fHETw6xGsfg/FMVI+1RR4D56R71AHi2lTyCASgvFcsER2mVRCTVME886C6Bt ptDwG1M+iF6VWemYueEdbrbhSFDXHurIf+Forcr0mc4+uxJzQgOuIcrYARWhDtaB/FXX GlNSmaRx1hWyntp90ONc/Om+jKSZxG/fofNyaFdCfcMFsk+5vOnMJihUnF6N0auoyFP1 zeCA==
In-reply-to: <20140225195420.GX13647@dastard>
References: <1393352816-26065-1-git-send-email-fdmanana@xxxxxxxxx> <1393353848-26790-1-git-send-email-fdmanana@xxxxxxxxx> <20140225195420.GX13647@dastard>
Reply-to: fdmanana@xxxxxxxxx
On Tue, Feb 25, 2014 at 7:54 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> 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`

Didn't we agree before, for a previous path, to export "here" from the
main control skip and then cleanup tests to not redefine it?
I am confused now :)

>
>> +
>> +_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.

I want to force a btrfs transaction commit. Plain old 'sync' would do
it as well for sure, but that applies against all mounted FSs, while
btrfs filesystem sync is applied against a single fs.
Plus, since this is a btrfs specific test, is it non sense to exercise
commands from btrfs-progs?

>
> 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...

I need to make sure there's fragmentation (i.e. several file extent
items in the fs btree with data offset fields > 0).

>
>> +     _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. ;)

No it doesn't unmount.

>
>> +
>> +     _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.

The restore command allows you to grab files from a (potentially
damaged) btrfs filesystem and save them to a destination path, no
matter what its filesystem is (btrfs, extN, xfs, etc)

>
> 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.

Alright... Many other existing tests do things like this.

Thanks Dave

>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

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