xfs
[Top] [All Lists]

Re: [PATCH v2] xfstests: Add test case to test xfs projid32bit functiona

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v2] xfstests: Add test case to test xfs projid32bit functionality a bit more extensively.
From: Boris Ranto <ranto.boris@xxxxxxxxx>
Date: Thu, 6 Sep 2012 14:58:50 +0200
Cc: xfs-oss <xfs@xxxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxxx>
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=GqRsIn3R5xdAKofHGZYAeCFupU1BnV9STZBudBXGIYA=; b=SyaNFHgp5ITcYbN1dht50itUtOVQR8sLiNG4BY7yxqnq0mm7Be0WpjuAjEQbEnAkh4 BpXT3fRmMv9Oyn+3whNNfDbmmSa2ivMAZ7zaBfbtu6paKwHfHmMMvXfbl108Dc9XYHS4 AkYsNOQ+/oy8Fsu6HsCgQltem0iYXHGMjagMe/98dnl6J8XRydFm2BUQSAn8A1waSE/u ldLjO4GCn1aOTkvgWRKCumWNU+Gsh5iiIaJoT5rmzl0xFkYOUZyFLv6IM5zY857TF57d J5qD9wqtjY4rPpsdNoyFaE71m2Nq+x4UgaBFxo9c5Qz6sfjS65QPHam2fphGT9UeP+tk 5msQ==
In-reply-to: <20120906003150.GM15292@dastard>
References: <CAFZPdfg+5kRfUuVOtRk1pqKrRai6CZBxisvXnuPUr-_SNAmGTg@xxxxxxxxxxxxxx> <20120906003150.GM15292@dastard>
On Thu, Sep 6, 2012 at 2:31 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> On Tue, Aug 28, 2012 at 02:59:42PM +0200, Boris Ranto wrote:
>>  The test covers several areas including enabling projid32bit
>> functionality dynamically by xfs_admin, dumping, restoring, quota
>> reporting and xfs_db projid values reporting.
>>  This test case hits two bugs: one for broken xfsdump/xfsrestore
>> functionality and one for enabling projid32bit functionality with
>> xfs_admin on a LVM device (SCRATCH_DEV must be an LVM device to hit
>> this).
>
> The LVM problem is incidental - if there's a problem with LVM
> devices then xfs_db should show the same problem, as should all
> other xfs_admin commands that use xfs_db. Hence I don't think that
> there is any point in mentioning it here.
>
>>  This version does not create/handle any loop devices.
>>
>> Signed-off-by: Boris Ranto <ranto.boris@xxxxxxxxx>
>> ---
>>  285     |  138 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  285.out |   27 ++++++++++++
>>  group   |    1 +
>>  3 files changed, 166 insertions(+), 0 deletions(-)
>>  create mode 100644 285
>>  create mode 100644 285.out
>>
>> diff --git a/285 b/285
>> new file mode 100644
>> index 0000000..ad96aa6
>> --- /dev/null
>> +++ b/285
>> @@ -0,0 +1,138 @@
>> +#! /bin/bash
>> +# FS QA Test No. 285
>> +#
>> +# Test to verify project quota xfs_admin, xfsdump/xfsrestore and
>> +# xfs_db functionality
>> +#
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2012 Boris Ranto.  All Rights Reserved.
>
> Shouldn't this be Red Hat?
>

Probably yes, I'll change that in next version.

>> +#
>> +# 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
>> +#
>> +#-----------------------------------------------------------------------
>> +#
>> +# creator
>> +owner=ranto.boris@xxxxxxxxx
>> +
>> +seq=`basename $0`
>> +echo "QA output created by $seq"
>> +tmp=/tmp/$$
>> +here=`pwd`
>> +status=1    # failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +rm -f $seq.full
>> +
>> +# get standard environment, filters and checks
>> +. ./common.rc
>> +. ./common.quota
>> +
>> +_cleanup()
>> +{
>> +    cd /
>> +    umount $SCRATCH_MNT 2>/dev/null
>> +    rm -rf $tmp.*
>> +}
>> +
>> +# real QA test starts here
>> +_supported_fs xfs
>> +_require_xfs_quota
>> +_require_scratch
>> +_require_projid32bit
>> +
>> +export MOUNT_OPTIONS="-opquota"
>
> Kill that - '_qmount_option "pquota"' is the preferred way to do
> specific project quota configuration mounts.
>

I didn't know that, 244 that I used as a starting point used the MOUNT_OPTIONS.

>> +
>> +# create xfs fs without projid32bit ability, will be gained by xfs_admin
>> +_scratch_mkfs_xfs -i projid32bit=0 -d size=200m >> $seq.full || _fail
>> "mkfs failed"
>
> You need to turn off line wrapping when you post patches.
>
>> +_qmount
>
> _qmount_option "pquota"
>
>> +# require project quotas
>> +_require_prjquota $SCRATCH_DEV
>> +
>> +dir=$SCRATCH_MNT/pquota
>> +
>> +status=1
>> +
>> +mkdir -p $dir
>> +touch $dir/{16,32}less
>
> "less" is a weird filename suffix. What's it mean? Why not just
> something standard like "test"?
>

As Eric suggested it just meant that the number is less than 16/32 bits.

>> +inode16a=$(ls -i $dir/16less|cut -d ' ' -f 1)
>> +inode32a=$(ls -i $dir/32less|cut -d ' ' -f 1)
>> +$XFS_QUOTA_PROG -x -c "project -s -p $dir/16less 1234" $SCRATCH_DEV \
>> +        >> $seq.full
>> +$XFS_QUOTA_PROG -x -c "project -s -p $dir/32less 2123456789" $SCRATCH_DEV \
>> +        >> $seq.full 2>&1
>> +# These will be checked by $seq.out
>
> No need for comments like this
>
>> +echo "No 32bit project quotas:"
>> +$XFS_IO_PROG -r -c "lsproj" $dir/16less
>> +$XFS_IO_PROG -r -c "lsproj" $dir/32less
>> +
>> +umount $SCRATCH_MNT
>> +
>> +# Now, enable projid32bit support by xfs_admin
>> +xfs_admin -p $SCRATCH_DEV > /dev/null 2>&1
>
> Output to $seq.full, and use the _fail script if xfs-admin returned
> an error.
>
>> +echo "xfs_admin returned $?"
>> +
>> +# Now mount the fs, 32bit project quotas shall be supported, now
>> +_qmount
>> +$XFS_QUOTA_PROG -x -c "project -s -p $dir/32less 2123456789" $SCRATCH_DEV \
>> +        >> $seq.full
>> +
>> +# These will be checked by $seq.out
>> +echo "With 32bit project quota support:"
>> +$XFS_IO_PROG -r -c "lsproj" $dir/16less
>> +$XFS_IO_PROG -r -c "lsproj" $dir/32less
>> +
>> +# Dump the fs to a temporary file
>> +rm -f $tmp.dump.img
>> +$XFSDUMP_PROG -f $tmp.dump -L label -M media -l 0 $SCRATCH_MNT >>
>> $seq.full  || _fail "dump failed"
>> +echo "xfsdump returned $?"
>
> If xfsdump failed, this won't be run. If it succeeded, the output
> will always be zero. So this sort of output is not needed.
>
>> +# Prepare the device to restore the dumped file system
>> +dir=$SCRATCH_MNT/restore/pquota
>
> This is just confusing - the same variable points to different
> directories in different parts of the test. A good reason for
> separating the tests.
>
>> +# Just make the restore dir, the pquota dir will be created by xfsrestore
>> +mkdir -p $SCRATCH_MNT/restore
>> +
>> +# Restore
>> +$XFSRESTORE_PROG -f $tmp.dump $SCRATCH_MNT/restore > /dev/null 2>&1
>> +echo "xfsrestore returned $?"
>> +
>> +# Check that they are the same
>> +diff -urpN $SCRATCH_MNT/{,restore}/pquota
>> +echo "diff returned $?"
>
> _fail if diff returned somethign non-zero.
>

I used these '<something> returned $?' as a progress points in the output file.

>> +touch $dir/32lessv2
>> +inode16b=$(ls -i $dir/16less|cut -d ' ' -f 1)
>> +inode32b=$(ls -i $dir/32less|cut -d ' ' -f 1)
>> +inode32v2=$(ls -i $dir/32lessv2|cut -d ' ' -f 1)
>> +$XFS_QUOTA_PROG -x -c "project -s -p $dir/32lessv2 2123456789" $SCRATCH_MNT 
>> \
>> +        >> $seq.full
>
> Why do you shorten some long lines, and not others...
>
>> +echo "The restored file system + one additional file:"
>> +$XFS_IO_PROG -r -c "lsproj" $dir/16less
>> +$XFS_IO_PROG -r -c "lsproj" $dir/32less
>> +$XFS_IO_PROG -r -c "lsproj" $dir/32lessv2
>> +
>> +umount $SCRATCH_MNT
>> +
>> +# Now, we can examine the file systems with xfs_db
>> +# These two should report the same values
>> +echo "These two values of 16bit project quota ids shall be the same"
>
> No need to output this into the out file. The comment says it all,
> and we don't need the text in the output file to test for test
> failure.
>

Again, as Eric suggested I used that to improve output file
readability and diagnostics.

>> +$XFS_DB_PROG -c "inode $inode16a" -c "print core.projid_lo" -c "print
>> core.projid_hi" $SCRATCH_DEV
>
> $XFS_DB_PROG -c "inode $inode16a" \
>         -c "print core.projid_lo" \
>         -c "print core.projid_hi" \
>         $SCRATCH_DEV
>
>         or, because it's called 5 times:
>
> _print_projid()
> {
>
>         $XFS_DB_PROG -c "inode $1" \
>                 -c "print core.projid_lo" \
>                 -c "print core.projid_hi" \
>                 $SCRATCH_DEV
> }
>
> _print_projid $inode16a
> _print_projid $inode16b
> _print_projid $inode32a
> _print_projid $inode32b
> _print_projid $inode32v2
>

OK, I'll switch to the _print_projid in next version.

>> +$XFS_DB_PROG -c "inode $inode16b" -c "print core.projid_lo" -c "print
>> core.projid_hi" $SCRATCH_DEV
>> +
>> +# These three should report the same values
>> +echo "These three values of 32bit project quota ids shall be the same"
>> +$XFS_DB_PROG -c "inode $inode32a" -c "print core.projid_lo" -c "print
>> core.projid_hi" $SCRATCH_DEV
>> +$XFS_DB_PROG -c "inode $inode32b" -c "print core.projid_lo" -c "print
>> core.projid_hi" $SCRATCH_DEV
>> +$XFS_DB_PROG -c "inode $inode32v2" -c "print core.projid_lo" -c
>> "print core.projid_hi" $SCRATCH_DEV
>> +
>> +status=0
>> +exit
>> diff --git a/285.out b/285.out
>> new file mode 100644
>> index 0000000..a601452
>> --- /dev/null
>> +++ b/285.out
>> @@ -0,0 +1,27 @@
>> +QA output created by 285
>> +No 32bit project quotas:
>> +projid = 1234
>> +projid = 0
>> +xfs_admin returned 0
>> +With 32bit project quota support:
>> +projid = 1234
>> +projid = 2123456789
>> +xfsdump returned 0
>> +xfsrestore returned 0
>> +diff returned 0
>> +The restored file system + one additional file:
>> +projid = 1234
>> +projid = 2123456789
>> +projid = 2123456789
>> +These two values of 16bit project quota ids shall be the same
>> +core.projid_lo = 1234
>> +core.projid_hi = 0
>> +core.projid_lo = 1234
>> +core.projid_hi = 0
>> +These three values of 32bit project quota ids shall be the same
>> +core.projid_lo = 24853
>> +core.projid_hi = 32401
>> +core.projid_lo = 24853
>> +core.projid_hi = 32401
>> +core.projid_lo = 24853
>> +core.projid_hi = 32401
>> diff --git a/group b/group
>> index 104ed35..bbc74fe 100644
>> --- a/group
>> +++ b/group
>> @@ -403,3 +403,4 @@ deprecated
>>  282 dump ioctl auto quick
>>  283 dump ioctl auto quick
>>  284 auto
>> +285 auto dump quota
>
> And quick.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx

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