xfs
[Top] [All Lists]

Re: [PATCH 1/3] 264: Functional test case for the btrfs snapshot

To: Anand Jain <Anand.Jain@xxxxxxxxxx>
Subject: Re: [PATCH 1/3] 264: Functional test case for the btrfs snapshot
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Wed, 19 Oct 2011 05:42:09 -0400
Cc: hch@xxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, linux-btrfs@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx, david@xxxxxxxxxxxxx
In-reply-to: <1318919336-32206-2-git-send-email-Anand.Jain@xxxxxxxxxx>
References: <20111013005652.GM3159@dastard> <1318919336-32206-1-git-send-email-Anand.Jain@xxxxxxxxxx> <1318919336-32206-2-git-send-email-Anand.Jain@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Oct 18, 2011 at 02:28:54PM +0800, Anand Jain wrote:
> Create snapshots in various ways, modify the data around the block and
> file boundaries and verify the data integrity.

The test itselt looks good enough, but I have some comments on the
pool infrastructure changes.  I also think they should probably be
a separate preparatory patch, or at least documented in the changelog
as well.

> index 5367be6..7c135c7 100644
> --- a/README
> +++ b/README
> @@ -36,12 +36,17 @@ Preparing system for tests (IRIX and Linux):
>                not be run.
>                
>          (these must be two DIFFERENT partitions)
> +
> +    - for btrfs only: some tests would need 3 or more independent SCRATCH 
> disks,
> +      which should be setenv SCRATCH_DEV_POOL instead of SCRATCH_DEV
> +              
>                
>      - setup your environment
>          - setenv TEST_DEV "device containing TEST PARTITION"
>          - setenv TEST_DIR "mount point of TEST PARTITION"   
>               - optionally:
>               - setenv SCRATCH_DEV "device containing SCRATCH PARTITION"
> +             - setenv SCRATCH_DEV_POOL "pool of SCRATCH disks for testing 
> btrfs"

How does one find out what the pool name is?  You'll also need to
document how to create the pool from disks.

> @@ -229,6 +229,20 @@ if [ ! -d "$TEST_DIR" ]; then
>      exit 1
>  fi
>  
> +# a btrfs tester will set only SCRATCH_DEV_POOL, we will put first of its dev
> +# to SCRATCH_DEV and rest to SCRATCH_DEV_POOL to maintain the backward 
> compatibility
> +if [ "$HOSTOS" == "Linux" ]; then
> +    FSTYP_tmp=`blkid -c /dev/null -s TYPE -o value $TEST_DEV`
> +else
> +    FSTYP_tmp=xfs
> +fi

Why do we need a second FSTYP detection?  If the existing one isn't
early enough make sure it's done early enough instead of duplicating
it.

> --- a/common.rc
> +++ b/common.rc
> @@ -1498,7 +1498,11 @@ _nfiles()
>                  file=f$f
>                  echo > $file
>                  if [ $size -gt 0 ]; then
> -                    dd if=/dev/zero of=$file bs=1024 count=$size
> +                 if [ $randomdata == false ]; then
> +                     dd if=/dev/zero of=$file bs=1024 count=$size 2>&1 | 
> _filter_dd
> +                 else
> +                     dd if=/dev/urandom of=$file bs=1024 count=$size 2>&1 | 
> _filter_dd
> +                 fi

I'd rather see the randomdata flag passed down explicitly to _descend and
_nfiles rather than setting a magic environment variable.

> @@ -1508,7 +1512,11 @@ _nfiles()
>  _descend()
>  {
>          dirname=$1; depth=$2
> -        mkdir $dirname  || die "mkdir $dirname failed"
> +     if [ -d $dirname ]; then
> +             dirname=`mktemp -dq $dirname/dir.XXXXXX`
> +     else
> +             mkdir $dirname  || die "mkdir $dirname failed"
> +     fi

Why would the directory here already exist?  This at least needs
very good documentation.  Also the indentation seems off compared
to the surrounding code.

> @@ -1550,6 +1559,7 @@ _populate_fs()
>          s)      size=$OPTARG;;
>          v)      verbose=true;;
>          r)      root=$OPTARG;;
> +     x)      randomdata=true;;

indendation is off again.

> +# scratch_dev_pool should contain the disks pool for the btrfs raid
> +_require_scratch_dev_pool()
> +{
> +     local i
> +     case "$FSTYP" in
> +     btrfs)
> +             if [ -z "$SCRATCH_DEV_POOL" ]
> +             then

For new code I'd generally prefer the more readable

                if [ ... ]; then

although the above form unfortunately still is fairly common in
xfsprogs.

> +# We will check if the device is virtual (eg: loop device) since it does not
> +# have the delete entry-point. Otherwise SCSI and USB devices are fine. 
> +_require_deletable_scratch_dev_pool()
> +{
> +     local i
> +     local x
> +     for i in $SCRATCH_DEV_POOL; do
> +             x=`echo $i | cut -d"/" -f 3`
> +             ls -l /sys/class/block/${x} | grep -q "virtual" 
> +             if [ $? == "0" ]; then
> +                     _notrun "$i is a virtual device which is not deletable"
> +             fi
> +     done
> +}
> +
> +# arg 1 is dev to remove and is output of the below eg.
> +# ls -l /sys/class/block/sdd | rev | cut -d "/" -f 3 | rev
> +_devmgt_remove()
> +{
> +     echo 1 > /sys/class/scsi_device/${1}/device/delete || _fail "Remove 
> disk failed"
> +}
> +
> +# arg 1 is dev to add and is output of the below eg.
> +# ls -l /sys/class/block/sdd | rev | cut -d "/" -f 3 | rev
> +_devmgt_add()
> +{
> +     local h
> +     local tdl
> +     # arg 1 will be in h:t:d:l format now in the h and "t d l" format
> +     h=`echo ${1} | cut -d":" -f 1`
> +     tdl=`echo ${1} | cut -d":" -f 2-|sed 's/:/ /g'`
> +
> +     echo ${tdl} >  /sys/class/scsi_host/host${h}/scan || _fail "Add disk 
> failed"
> +}

This code looks a bit fragile to me, but I think we can fix it on the go
if we encouter issues.

> diff --git a/group b/group
> index 2a8970c..cfbae8c 100644
> --- a/group
> +++ b/group
> @@ -377,3 +377,4 @@ deprecated
>  261 auto quick quota
>  262 auto quick quota
>  263 rw auto quick
> +264 auto quick

It might be worth to add pool or snaphot groups if you add more
tests like this.

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