xfs
[Top] [All Lists]

Re: [PATCH] xfstests: Btrfs: add test for large metadata blocks

To: Dave Chinner <david@xxxxxxxxxxxxx>, jbacik@xxxxxx
Subject: Re: [PATCH] xfstests: Btrfs: add test for large metadata blocks
From: Koen De Wit <koen.de.wit@xxxxxxxxxx>
Date: Mon, 10 Feb 2014 22:40:14 +0100
Cc: xfs@xxxxxxxxxxx, linux-btrfs@xxxxxxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140209230259.GK13647@dastard>
Organization: Oracle Corporation
References: <1391793285-935-1-git-send-email-koen.de.wit@xxxxxxxxxx> <20140207224934.GH13647@dastard> <52F5EB3B.5030902@xxxxxxxxxx> <20140209230259.GK13647@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

On 02/10/2014 12:02 AM, Dave Chinner wrote:
On Sat, Feb 08, 2014 at 09:30:51AM +0100, Koen De Wit wrote:
On 02/07/2014 11:49 PM, Dave Chinner wrote:
On Fri, Feb 07, 2014 at 06:14:45PM +0100, Koen De Wit wrote:
        echo -n "$xattr_value" | md5sum
        ${ATTR_PROG} -Lq -s attr_$char -V $xattr_value $file
        ${ATTR_PROG} -Lq -g attr_$char $file | md5sum
        ${ATTR_PROG} -Lq -g attr_$char $lnkfile | md5sum

is all that neds to be done here.
The problem with this is that the length of the output will depend on the page 
size. The code above runs for every valid leafsize, which can be any multiple 
of the page size up to 64KB, as defined in the loop initialization:
     for leafsize in `seq $pagesize_kb $pagesize_kb 64`; do
That's only a limit on the mkfs leafsize parameter, yes? An the
limiation is that the leaf size can't be smaller than page size?

So really, the attribute sizes that are being tested are independent
of the mkfs parameters being tested. i.e:

        for attrsize in `seq 4 4 64`; do
                if [ $attrsize -lt $pagesize ]; then
                        leafsize=$pagesize
                else
                        leafsize=$attrsize
                fi
                $BTRFS_MKFS_PROG -l $leafsize $SCRATCH_DEV

And now the test executes a fixed loop, testing the same attribute
sizes on all the filesystems under test. i.e. the attribute sizes
being tested are *independent* of the mkfs parameters being tested.
Always test the same attribute sizes, the mkfs parameters simply
vary by page size.

OK, thanks for the suggestion!  I implemented it like this in v3, I just 
changed the calculation of the leafsize because it must be a multiple of the 
pagesize. (A leafsize of 12KB is not valid for systems with 8KB pages.)

+_scratch_unmount + +# Some illegal leafsizes + +_scratch_mkfs
-l 0 2>> $seqres.full +echo $?
Same again - you are dumping the error output into a different
file, then detecting the error manually. pass the output of
_scratch_mkfs through a filter, and let errors cause golden
output mismatches.
I did this to make the golden output not depend on the output of
mkfs.btrfs, inspired by
http://oss.sgi.com/cgi-bin/gitweb.cgi?p=xfs/cmds/xfstests.git;a=commit;h=fd7a8e885732475c17488e28b569ac1530c8eb59
and
http://oss.sgi.com/cgi-bin/gitweb.cgi?p=xfs/cmds/xfstests.git;a=commit;h=78d86b996c9c431542fdbac11fa08764b16ceb7d
However, in my opinion the test should simply be updated if the
output of mkfs.btrfs changes, so I agree with you and I fixed this
in v2.
While I agree with the sentiment, I'm questioning the
implementation. i.e. you've done this differently to every other
test that needs to check for failures. run_check woul dbe just
fine, as would be simply filtering the output of mkfs.

run_check will make the test fail if the return code differs from 0, and Josef brought up 
an example scenario (MKFS_OPTIONS="-O skinny-metadata") where mkfs.btrfs 
produces additional output.

In v3, I implemented the failure check similar to btrfs/022:

        _scratch_mkfs -l $1 >>$seqres.full 2>&1
        [ $? -ne 0 ] || _fail "'$1' is an illegal value for the" \
                "leafsize option, mkfs should have failed."

Is this the right way?


Thanks,
Koen.

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