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

[Christoph Hellwig <hch@infradead.org>]

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.