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