Re: [PATCH v2] xfstests: add regression test for btrfs incremental send

[Filipe David Manana <fdmanana@gmail.com>]

On Mon, Feb 17, 2014 at 1:17 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Sun, Feb 16, 2014 at 11:43:17PM +0000, Filipe David Manana wrote:
>> On Sun, Feb 16, 2014 at 11:08 PM, Dave Chinner <david@fromorbit.com> wrote:
>> > On Sat, Feb 15, 2014 at 03:36:13PM +0000, Filipe David Borba Manana wrote:
>> >> Test for a btrfs incremental send issue where we end up sending a
>> >> wrong section of data from a file extent if the corresponding file
>> >> extent is compressed and the respective file extent item has a non
>> >> zero data offset.
>> >>
>> >> Fixed by the following linux kernel btrfs patch:
>> >>
>> >>    Btrfs: use right clone root offset for compressed extents
>> >>
>> >> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
>> >> ---
>> >>
>> >> V2: Made the test more reliable. Now it doesn't depend anymore of btrfs'
>> >>     hole punch implementation leaving hole file extent items when we punch
>> >>     beyond the file's current size.
>> >>
>> >>  tests/btrfs/040     |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  tests/btrfs/040.out |    1 +
>> >>  tests/btrfs/group   |    1 +
>> >>  3 files changed, 117 insertions(+)
>> >>  create mode 100755 tests/btrfs/040
>> >>  create mode 100644 tests/btrfs/040.out
>> >>
>> >> diff --git a/tests/btrfs/040 b/tests/btrfs/040
>> >> new file mode 100755
>> >> index 0000000..d6b37bf
>> >> --- /dev/null
>> >> +++ b/tests/btrfs/040
>> >> @@ -0,0 +1,115 @@
>> >> +#! /bin/bash
>> >> +# FS QA Test No. btrfs/040
>> >> +#
>> >> +# Test for a btrfs incremental send issue where we end up sending a
>> >> +# wrong section of data from a file extent if the corresponding file
>> >> +# extent is compressed and the respective file extent item has a non
>> >> +# zero data offset.
>> >> +#
>> >> +# Fixed by the following linux kernel btrfs patch:
>> >> +#
>> >> +#   Btrfs: use right clone root offset for compressed extents
>> >> +#
>> >> +#-----------------------------------------------------------------------
>> >> +# Copyright (c) 2014 Filipe Manana.  All Rights Reserved.
>> >> +#
>> >> +# This program is free software; you can redistribute it and/or
>> >> +# modify it under the terms of the GNU General Public License as
>> >> +# published by the Free Software Foundation.
>> >> +#
>> >> +# This program is distributed in the hope that it would be useful,
>> >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> >> +# GNU General Public License for more details.
>> >> +#
>> >> +# You should have received a copy of the GNU General Public License
>> >> +# along with this program; if not, write the Free Software Foundation,
>> >> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> >> +#-----------------------------------------------------------------------
>> >> +#
>> >> +
>> >> +seq=`basename $0`
>> >> +seqres=$RESULT_DIR/$seq
>> >> +echo "QA output created by $seq"
>> >> +
>> >> +here=`pwd`
>> >> +tmp=`mktemp -d`
>> >> +status=1     # failure is the default!
>> >> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> >> +
>> >> +_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
>> >> +
>> >> +FSSUM_PROG=$here/src/fssum
>> >> +[ -x $FSSUM_PROG ] || _notrun "fssum not built"
>> >> +
>> >> +rm -f $seqres.full
>> >> +
>> >> +_scratch_mkfs >/dev/null 2>&1
>> >> +_scratch_mount "-o compress-force=lzo"
>> >> +
>> >> +run_check $XFS_IO_PROG -f -c "truncate 118811" $SCRATCH_MNT/foo
>> >> +run_check $XFS_IO_PROG -c "pwrite -S 0x0d -b 39987 92267 39987" \
>> >> +     $SCRATCH_MNT/foo
>> >
>> > Ugh. filter the output, don't use run_check.
>> >
>> > $XFS_IO_PROG -f -c "truncate 118811" $SCRATCH_MNT/foo
>> > $XFS_IO_PROG -c "pwrite -S 0x0d -b 39987 92267 39987" \
>> >         $SCRATCH_MNT/foo | _filter_xfs_io
>> >
>> > If something fails, we still want the test to continue running, even
>> > if all it does is exercise error handling paths. run_check simply
>> > terminates the test at the first failure.
>>
>> What's the point of continuing? The test will fail anyway, all of the
>> xfs_io calls are necessary to trigger the bug.
>
> Users don't stop doing doing stuff on a filesystem when a single
> failure occurs, so why should the tests? If you stop the moment a
> single failure occurs then you aren't ever going to stress the error
> handling paths, are you?
>
>> > _run_btrfs_util_prog()
>> > {
>> >         run_check $BTRFS_UTIL_PROG $*
>> > }
>> >
>> > would be a good start because it gets that run_check pattern out of
>> > the main test scripts and hence out of the heads of test writers.
>>
>> Well, will get rid of those run_check calls, but that will imply
>> adding some | _filter_scratch in many places. So shortening lines is
>> not a great argument :)
>
> I'm not talking about shortening lines here. I'm talking about the
> correct principles and conceptsi being in the forefront of a test
> writer's mind. Having the concept of "need to filter the output" in
> the head of test writers is *exactly* the right mindset to have.
>
> Indeed, if you have a block of code that needs common filtering,
> that's easy to do:
>
> do_test()
> {
>         # put test in function
> }
>
> do_test | _filter_scratch
>
> Will apply that filter to the entire output of the test, and so
> you don't need it on every command.
>
> Remember - an xfstest is not a "pass/fail" test. It's a "run this
> set of commands, and then check the entire output matches the known
> good output" test. i.e. we are testing the entire set of commands as
> a whole - we are not testing each individual command that is run.
> It's a very different principle to the "test every command that can
> fail" method of writing tests.
>
> _fail should only be used if the test cannot possibly be continued
> (e.g. scratch filesystem corrupted and cannot be mounted).  If one
> of the early commands fails, then that's fine - the test will fail -
> but we still want to run the other commands if we can so as to get
> the best test coverage we can get even on failed tests.

Ok, thanks for the very detailed explanation, very helpful :)


>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com



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