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

Anand Jain
2011-10-20T15:32:31Z

comments in line.

On 19/10/2011 17:42, Christoph Hellwig wrote:
> 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.
>

agreed.

>> @@ -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.

looks like its ok not to have FSTYP checked here, it will follow the
following logic..

btrfs FS OR any FS
SCRATCH_DEV_POOL is unset and SCRATCH_DEV is set
. test-case with _require_scratch_dev_pool will not run
. test-case without _require_scratch_dev_pool will run

SCRATCH_DEV_POOL is set and SCRATCH_DEV is unset
. test-case with _require_scratch_dev_pool
- runs only if FSTYP=btrfs
. test-case without _require_scratch_dev_pool will run using first
dev in the SCRATCH_DEV_POOL as a SCRATCH_DEV
- if FSTYP=btrfs it includes SCRATCH_DEV_POOL disks to the FS
- if FSTYP=non-btrfs SCRATCH_DEV_POOL is ignored

SCRATCH_DEV_POOL is set and SCRATCH_DEV is set
. reports error in the config

SCRATCH_DEV_POOL is unset and SCRATCH_DEV is unset
. no change

>> --- 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.

makes sense. added.

>> @@ -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.

Hmm. Changing this back to the original design - where calling
function has to ensure a new directory name which does not exists.

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

oh! thks for pointing.

>> +# 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.

got it.

>> +# 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.

ok.

>> 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.

this can be considered when appropriate I think, as of now there is
one test case in each of them.
Thanks

Anand