| Message ID | 1391793285-935-1-git-send-email-koen.de.wit@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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@oracle.com> 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. > + _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. > + # 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. i.e: 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. > + # 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 condition. > +_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. Cheers, Dave.
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@oracle.com> > 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. > i.e: > > 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 > condition. 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? >> +_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. Thanks, Koen. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > >>+_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. FWIW, the method for detecting the cp error in the second commit is for a very specific case. It could have also been done with a filter, as we have done in the past with such error messages. So what's good for one case is not necessarily the right way to handle the output for another. Cheers, Dave.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tests/btrfs/036 b/tests/btrfs/036 new file mode 100644 index 0000000..0f8287a --- /dev/null +++ b/tests/btrfs/036 @@ -0,0 +1,128 @@ +#! /bin/bash +# FS QA Test No. 036 +# +# Tests large metadata blocks in btrfs, which allows large extended +# attributes. +# +#----------------------------------------------------------------------- +# Copyright (c) 2014, Oracle and/or its affiliates. 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` +status=1 # failure is the default! + +# 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 + +rm -f $seqres.full + +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 + _scratch_mkfs -l ${leafsize}K >/dev/null + _scratch_mount + # 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` + ${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` + + # Using md5sums for comparison instead of the values themselves + # because bash command lines cannot be larger than 64K chars. + if [ "$set_md5" != "$get_md5" ]; then + echo "Got unexpected xattr value for attr_$char" + echo "from file $file . (leafsize is ${leafsize}K)" + fi + if [ "$set_md5" != "$get_ln_md5" ]; then + echo "Value for attr_$char differs for $file and" + echo "$lnkfile . (leafsize is ${leafsize}K)" + fi + done + + # 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 + fi + +done + +# Illegal attribute name (more than 256 characters) +attr_name=`head -c 260 < /dev/zero | tr '\0' n` +${ATTR_PROG} -s $attr_name -V attribute_name_too_big \ + $SCRATCH_MNT/emptyfile 2>&1 | head -n 1 + +_scratch_unmount + +# Some illegal leafsizes + +_scratch_mkfs -l 0 2>> $seqres.full +echo $? + +_scratch_mkfs -l 5678 2>> $seqres.full +echo $? + +_scratch_mkfs -l `expr $pagesize / 2 + $pagesize` 2>> $seqres.full +echo $? + +_scratch_mkfs -l 128K 2>> $seqres.full +echo $? + +_scratch_mkfs -l K + +# success, all done +status=0 +exit diff --git a/tests/btrfs/036.out b/tests/btrfs/036.out new file mode 100644 index 0000000..1d9bdfb --- /dev/null +++ b/tests/btrfs/036.out @@ -0,0 +1,7 @@ +QA output created by 036 +attr_set: Invalid argument +1 +1 +1 +1 +ERROR: size value is empty diff --git a/tests/btrfs/group b/tests/btrfs/group index f9f062f..2ca2225 100644 --- a/tests/btrfs/group +++ b/tests/btrfs/group @@ -37,3 +37,4 @@ 032 auto quick 033 auto quick 034 auto quick +036 auto quick
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@oracle.com> --- tests/btrfs/036 | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/btrfs/036.out | 7 +++ tests/btrfs/group | 1 + 3 files changed, 136 insertions(+), 0 deletions(-) create mode 100644 tests/btrfs/036 create mode 100644 tests/btrfs/036.out