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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
