xfs
[Top] [All Lists]

Re: [PATCH] xfstests: add function _require_fssum()

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfstests: add function _require_fssum()
From: Filipe David Manana <fdmanana@xxxxxxxxx>
Date: Mon, 24 Feb 2014 23:08:27 +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=d1fYBAcFpBvPPMVIJRdymxdJHGCpr67RXz7wxxgkzEk=; b=fp5tJyn+R4b9+dZn2rX6OkTRjRu3fcHnWnnC+UhqD4dLzQncB6yh3/nLxHnEDUFfd/ p6BFils6txTA1OIwWXA4//ODov0UbgHscGTGGBJnYKZwDiSkzSRn5p7Jd48yPSC4+hTQ ZBBoHQStgBfwRG7InYVGfbFkyNldA2/qz1qEzUOTMEFON75EH1daobn6vjE2PORhMmYM 4dTWsMgh9TyqJkiO0j5e1onSuAWk9KLbB7pPO211jBXCoQl0BZi/W3Tg6SfRkhdg5JsI 8yVxFjMU4puinV1x1mrZzglmv2ugb04qjywowcfPgksen2venaQSv6vn4Z6g/59Si5sP KbAg==
In-reply-to: <20140224222216.GP4317@dastard>
References: <1393242983-16149-1-git-send-email-fdmanana@xxxxxxxxx> <20140224122310.GO4317@dastard> <CAL3q7H5tfnb+uqa-ru5YNcok1n8PQxxai7UfkH-YgCdts84Tew@xxxxxxxxxxxxxx> <20140224222216.GP4317@dastard>
Reply-to: fdmanana@xxxxxxxxx
On Mon, Feb 24, 2014 at 10:22 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> On Mon, Feb 24, 2014 at 01:22:36PM +0000, Filipe David Manana wrote:
>> On Mon, Feb 24, 2014 at 12:23 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>> > On Mon, Feb 24, 2014 at 11:56:23AM +0000, Filipe David Borba Manana wrote:
>> >> To avoid repeating detection of fssum presence in many btrfs tests, as
>> >> suggested by Dave Chinner.
>> >>
>> >> Signed-off-by: Filipe David Borba Manana <fdmanana@xxxxxxxxx>
>> >> ---
>> >>  common/rc       |    7 +++++++
>> >>  tests/btrfs/007 |    5 +----
>> >>  tests/btrfs/016 |    5 +----
>> >>  tests/btrfs/030 |    5 +----
>> >>  tests/btrfs/038 |    5 +----
>> >>  tests/btrfs/039 |    5 +----
>> >>  tests/btrfs/040 |    5 +----
>> >>  tests/btrfs/041 |    5 +----
>> >>  tests/btrfs/042 |    5 +----
>> >>  9 files changed, 15 insertions(+), 32 deletions(-)
>> >>  mode change 100644 => 100755 tests/btrfs/016
>> >>
>> >> diff --git a/common/rc b/common/rc
>> >> index 5df504c..cce05cc 100644
>> >> --- a/common/rc
>> >> +++ b/common/rc
>> >> @@ -2144,6 +2144,13 @@ _require_cp_reflink()
>> >>                 _notrun "This test requires a cp with --reflink support."
>> >>  }
>> >>
>> >> +_require_fssum()
>> >> +{
>> >> +     HERE=`pwd`
>> >> +     FSSUM_PROG=$HERE/src/fssum
>> >> +     [ -x $FSSUM_PROG ] || _notrun "fssum not built"
>> >> +}
>> >
>> > $here is defined by check to be the root of the xfstests instance
>> > that is running. There's 60+ tests that already us it. Hence:
>> >
>> > _require_fssum()
>> > {
>> >         FSSUM_PROG=$here/src/fssum
>> >         [ -x $FSSUM_PROG ] || _notrun "fssum not built"
>> > }
>> >
>> > Is all you need here.
>>
>> Hum, doesn't work unless the test file defines $here. At least when
>> running a single only (.e.g. ./check btrfs/041).
>
> Right, I realised that it isn't exported from check, and the template
> for new tests defines it as:
>
> here=\`pwd\`
>
> which is essentially the same thing. Almost every test does it.
>
> $ git grep here=\`pwd\` |wc -l
> 364
>
> And it works just fine when running a test as you describe above.
> Many of the _requires* functions just use $here directly, and does a
> lot of the common/* infrastructure. It is assumed that $here is set
> and valid and so if it wasn't set, lots of different things would
> break.
>
> Oh, I note that btrfs/034 and btrfs/037 don't set it, so they need
> fixing, too.
>
> Looking at it, though, we shoul djust export the value from check
> and remove the assignment that occurs in every test as they end up
> being the same value:
>
> $ sudo ./check generic/001
> check_here=/home/dave/src/xfstests-dev
> FSTYP         -- xfs (debug)
> PLATFORM      -- Linux/x86_64 test2 3.14.0-rc3-dgc+
> MKFS_OPTIONS  -- -f -bsize=4096 /dev/vdb
> MOUNT_OPTIONS -- /dev/vdb /mnt/scratch
>
> generic/001 5s ... [failed, exit status 1] - output mismatch (see 
> /home/dave/src/xfstests-dev/results//generic/001.out.bad)
> .....
> $ cat /home/dave/src/xfstests-dev/results//generic/001.out.bad
> QA output created by 001
> here=/home/dave/src/xfstests-dev
> $
>
>> >> diff --git a/tests/btrfs/007 b/tests/btrfs/007
>> >> index 5df9ccb..5430613 100755
>> >> --- a/tests/btrfs/007
>> >> +++ b/tests/btrfs/007
>> >> @@ -31,7 +31,6 @@ seq=`basename $0`
>> >>  seqres=$RESULT_DIR/$seq
>> >>  echo "QA output created by $seq"
>> >>
>> >> -here=`pwd`
>> >>  tmp=`mktemp -d`
>> >>  status=1
>> >
>> > Yeah, redefining $here is a bad thing to do :/
>
> Right, my mistake. It needs to be defined for the entire test, not
> in a requires function. Hence I think we should just export it from
> check....

Hum, ok. So the decision is to let the tests explicitly define the
variable "here", and not export "here" from the main check script.

>
>> > It also points out that the btrfs tests are using a non-standard
>> > $tmp directory - one that is in the xfstests source directory.
>> > That's a bad thing, too - tests should be using:
>> >
>> > tmp=/tmp/$$
>> >
>> > to store small temporary files.
>> >
>> > If /tmp is too small for what a test needs, then the test should be
>> > using $TEST_DIR as the store for the temporary files to exercise
>> > the filesystem under test as much as possible.  e.g. send image
>> > files build form snapshots of SCRATCH_DEV should be stored on TEST_DIR,
>> > not in $tmp; filesystem image files that are mounted by loopback
>> > should be stored on TEST_DIR or SCRATCH_MNT, not $tmp. And so on.
>> >
>> > i.e. the idea is that you direct as much of the IO to the test_DIR
>> > and SCRATCH_MNT as possible, not to the filesystem that is hosting
>> > $tmp or the xfstests source directory....
>>
>> Right. Sounds like a separate patch (to use TEST_DIR/$$ for e.g. as a
>> place to store temporary test data).
>
> tmp should be set to /tmp/$$. That's where the *test harness* will
> direct temporary log files, mkfs output, etc. If you have temporary
> data for the *test*, and it's large, use TEST_DIR, not $tmp.
> $tmp is generally only for things like log files, time stamps,
> output files that need later processing because they can't be
> filtered directly, etc.
>
> Why? Because if the test corrupts $TEST_DIR and you've got all the
> test harness tmp files on TEST_DIR, how is the test harness going
> to work out what went wrong during the test?

Makes sense. All these btrfs tests are adding small files to /tmp,
less than ~100Kb or so (certainly not megabytes), so that seems fine.

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>