[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfstests: Btrfs: add test for large metadata blocks
From: Koen De Wit <koen.de.wit@xxxxxxxxxx>
Date: Sat, 08 Feb 2014 09:30:51 +0100
Cc: xfs@xxxxxxxxxxx, linux-btrfs@xxxxxxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140207224934.GH13647@dastard>
Organization: Oracle Corporation
References: <1391793285-935-1-git-send-email-koen.de.wit@xxxxxxxxxx> <20140207224934.GH13647@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0
Thanks for the review, Dave!
Comments inline.

On 02/07/2014 11:49 PM, Dave Chinner wrote:
On Fri, Feb 07, 2014 at 06:14:45PM +0100, Koen De Wit wrote:
Tests Btrfs filesystems with all possible metadata block sizes, by
setting large extended attributes on files.

Signed-off-by: Koen De Wit <koen.de.wit@xxxxxxxxxx>
There's a few things here that need fixing.

+pagesize=`$here/src/feature -s`
+pagesize_kb=`expr $pagesize / 1024`
+# Test all valid leafsizes
+for leafsize in `seq $pagesize_kb $pagesize_kb 64`; do
+    _scratch_unmount >/dev/null 2>&1
Indentation are tabs, and tabs are 8 spaces in size, please.

OK, I fixed this in v2.

+    _scratch_mkfs -l ${leafsize}K >/dev/null
+    _scratch_mount
No need to use _scratch_unmount here - you should be doing a
_check_scratch_fs at the end of the loop.

Fixed in v2 too.

+    # Calculate the xattr size, but leave 512 bytes for other metadata.
+    xattr_size=`expr $leafsize \* 1024 - 512`
+    touch $SCRATCH_MNT/emptyfile
+    # smallfile will be inlined, bigfile not.
+    $XFS_IO_PROG -f -c "pwrite 0 100" $SCRATCH_MNT/smallfile >/dev/null
+    $XFS_IO_PROG -f -c "pwrite 0 9000" $SCRATCH_MNT/bigfile >/dev/null
+    ln -s $SCRATCH_MNT/bigfile $SCRATCH_MNT/bigfile_softlink
+    files=(emptyfile smallfile bigfile bigfile_softlink)
+    chars=(a b c d)
+    for i in `seq 0 1 3`; do
+        char=${chars[$i]}
+        file=$SCRATCH_MNT/${files[$i]}
+        lnkfile=${file}_hardlink
+        ln $file $lnkfile
+        xattr_value=`head -c $xattr_size < /dev/zero | tr '\0' $char`
+        set_md5=`echo -n "$xattr_value" | md5sum`
Just dump the md5sum to the output file.

+        ${ATTR_PROG} -Lq -s attr_$char -V $xattr_value $file
+        get_md5=`${ATTR_PROG} -Lq -g attr_$char $file | md5sum`
+        get_ln_md5=`${ATTR_PROG} -Lq -g attr_$char $lnkfile | md5sum`
And dump these to the output file, too. Then the golden image
matching when the test is finish will tell you if it passed or not.

        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

+    # Test attributes with a size larger than the leafsize.
+    # Should result in an error.
+    if [ "$leafsize" -lt "64" ]; then
+        # Bash command lines cannot be larger than 64K characters, so we
+        # do not test attribute values with a size >64KB.
+        xattr_size=`expr $leafsize \* 1024 + 512`
+        xattr_value=`head -c $xattr_size < /dev/zero | tr '\0' x`
+        ${ATTR_PROG} -q -s attr_toobig -V $xattr_value \
+            $SCRATCH_MNT/emptyfile >> $seqres.full 2>&1
+        if [ "$?" -eq "0" ]; then
+            echo "Expected error, xattr_size is bigger than ${leafsize}K"
+        fi
What you are doing is redirecting the error to $seqres.full
so that it doesn't end up in the output file, then detecting the
absence of an error and dumping a message to the output file to make
the test fail.

IOWs, the ATTR_PROG failure message should be in the golden output
file and you don't have to do anything else to detect a pass/fail

Same here: the bigger the page size, the less this code will be executed. If 
the page size is 64KB, this code isn't executed at all.
To make sure the golden output does not depend on the page size, I chose to 
suppress all output as long as the test is successful. Is there a better way to 
accomplish this?

+# 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 
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.


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