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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
