xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfstests: add regression test for btrfs incremental send
From: Filipe David Manana <fdmanana@xxxxxxxxx>
Date: Mon, 17 Feb 2014 01:32:41 +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=93oUadUfiHECy8jRh+zQELzsYHDAVLn5PET+r3OB2Ts=; b=ZRzgwOnRaZ8leEtcm10JxoZQwq9Xo7HtRfW0u5X8kaX9ArrAhZS7VfBCfljbmyRdNB NKPs2Z2hhZ+EHwDAnzDO9Xne4//WNLtUEsHr0jZOKJMFINyTddJ96Vb2cU0+OxGmllL4 1h/tvb0v6YAD+KF2MPntL0swUYj53DJoKRgxwxa+3RiBbh3L9q8rf5+nsuhS0YymRKpo eT4AWZKF/LlixQmIHcl257+rZertlM6Ng0DLhVARNLgmmDORyv0+pT1EN/4wGhBVTEe7 M9Fj+NU5/YXjowj5IugmfjPE/7r6kFFoJKSHaOwLaPfCXJkEmofYMYhJXAK5Ga4huIoc PwcA==
In-reply-to: <20140217011714.GZ13997@dastard>
References: <1392408522-764-1-git-send-email-fdmanana@xxxxxxxxx> <1392478573-4513-1-git-send-email-fdmanana@xxxxxxxxx> <20140216230851.GX13647@dastard> <CAL3q7H46_AHDtACuZunRJgSymejgr73NVZ-TWNuBuD6z3j9KXg@xxxxxxxxxxxxxx> <20140217011714.GZ13997@dastard>
Reply-to: fdmanana@xxxxxxxxx


On Monday, February 17, 2014, Dave Chinner <david@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >>
> >> 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
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.

Thanks for the detailed explanation Dave, very useful :)

Â

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>