fstests: fix btrfs test failures after commit 27d077ec0bda
PATCH v2 1/4 fstests: fix btrfs test failures after commit 27d077ec0bda
From: Filipe Manana <[email protected]>
Commit 27d077ec0bda (common: use mount/umount helpers everywhere) made a few btrfs test fail for 2 different reasons:
1) Some tests (btrfs/029 and btrfs/031) use $SCRATCH_MNT as a mount point for some subvolume created in $TEST_DEV, therefore calling _scratch_unmount does not work as it passes $SCRATCH_DEV as the argument to the umount program. This is intentional to test reflinks accross different mountpoints of the same filesystem but for different subvolumes;
2) For multiple devices filesystems (btrfs/003 and btrfs/011) that test the device replace feature, we need to unmount using the mount path ($SCRATCH_MNT) because unmounting using one of the devices as an argument ($SCRATCH_DEV) does not always work - after replace operations we get in /proc/mounts a device other than $SCRATCH_DEV associated with the mount point $SCRATCH_MNT (this is mentioned in a comment at btrfs/011 for example), so we need to pass that other device to the umount program or pass it the mount point.
Using $SCRATCH_MNT as a mountpoint for a device other than $SCRATCH_DEV is misleading, but that's a different problem that existed long before and this change attempts only to fix the regression from 27d077ec0bda, leaving such cleanups to later.
Fix this by making _sctatch_unmmount() pass $SCRATCH_MNT to umount instead of $SCRATCH_DEV.
---
V2: Change _sctatch_unmount() to pass $SCRATCH_MNT as the argument to the
umount program instead of $SCRATCH_DEV. This makes the btrfs tests pass again.
common/rc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
common/rc +1 -1
@@ -319,7 +319,7 @@ _scratch_unmount() if [ "$FSTYP" == "overlay" ]; then _overlay_scratch_unmount else - $UMOUNT_PROG $SCRATCH_DEV + $UMOUNT_PROG $SCRATCH_MNT fi }
Signature · 1 line
2.1.3
PATCH v2 2/4 fstests: cleanup test btrfs/029
From: Filipe Manana <[email protected]>
The test was using $SCRATCH_MNT as a mountpoint for $SCRATCH_DEV, which is counter intuitive and not expected by the fstests framework - this made the test fail after commit 27d077ec0bda (common: use mount/umount helpers everywhere). So rewrite the test to use the scratch device for all data and use a test specific directory inside $TEST_DIR to use as a mount point for a cross mount of $SCRATCH_DEV.
This test was also overriding $seqres.full, through the redirect ">" operator, if a call to cp failed. Fix that by using instead the operator ">>". Also make the test use the function _mount() instead of calling the mount program directly.
---
V2: First version of the change, introduced in the v2 of the corresponding
patchset.
tests/btrfs/029 | 45 +++++++++++++++++++-------------------------- tests/btrfs/029.out | 12 ++++++------ 2 files changed, 25 insertions(+), 32 deletions(-)
tests/btrfs/029 +19 -26
@@ -39,7 +39,6 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _cleanup() { - _scratch_unmount &>/dev/null cd / rm -f $tmp.* }
@@ -56,55 +55,49 @@ _supported_os Linux _require_test _require_scratch _require_cp_reflink +_need_to_be_root -SOURCE_DIR=$TEST_DIR/test-$seq -CROSS_DEV_DIR=$SCRATCH_MNT/test-$seq -# mount point & target for twice-mounted device -TEST_DIR2=$TEST_DIR/mount2 -DUAL_MOUNT_DIR=$SCRATCH_MNT/test-bis-$seq - -rm -rf $SOURCE_DIR -mkdir $SOURCE_DIR +reflink_test_dir=$TEST_DIR/test-$seq +rm -rf $reflink_test_dir +mkdir $reflink_test_dir rm -f $seqres.full _scratch_mkfs > /dev/null 2>&1 -$XFS_IO_PROG -f -c 'pwrite -S 0x61 0 9000' $SOURCE_DIR/original \ - >> $seqres.full +_scratch_mount +$XFS_IO_PROG -f -c 'pwrite -S 0x61 0 9000' $SCRATCH_MNT/original >> $seqres.full _filter_testdirs() { _filter_test_dir | _filter_scratch } -_create_reflinks_to() +_create_reflinks() { # auto reflink, should fall back to non-reflink - rm -rf $1; mkdir $1 + rm -rf $2 echo "reflink=auto:" - cp --reflink=auto $SOURCE_DIR/original $1/copy - md5sum $SOURCE_DIR/original | _filter_testdirs - md5sum $1/copy | _filter_testdirs + cp --reflink=auto $1 $2 + md5sum $1 | _filter_testdirs + md5sum $2 | _filter_testdirs # always reflink, should fail outright - rm -rf $1; mkdir $1 + rm -rf $2 echo "reflink=always:" - cp --reflink=always $SOURCE_DIR/original $1/copyfail > $seqres.full 2>&1 \ - || echo "cp reflink failed" + cp --reflink=always $1 $2 >> $seqres.full 2>&1 || echo "cp reflink failed" # The failed target actually gets created by cp: - ls $1/copyfail | _filter_testdirs + ls $2 | _filter_testdirs } echo "test reflinks across different devices" -_scratch_mount -_create_reflinks_to $CROSS_DEV_DIR -_scratch_unmount +_create_reflinks $SCRATCH_MNT/original $reflink_test_dir/copy echo "test reflinks across different mountpoints of same device" -mount $TEST_DEV $SCRATCH_MNT || _fail "Couldn't double-mount $TEST_DEV" -_create_reflinks_to $DUAL_MOUNT_DIR -_scratch_unmount +rm -rf $reflink_test_dir/* +_mount $SCRATCH_DEV $reflink_test_dir +_create_reflinks $SCRATCH_MNT/original $reflink_test_dir/copy +$UMOUNT_PROG $reflink_test_dir # success, all done status=0
tests/btrfs/029.out +6 -6
@@ -1,15 +1,15 @@ QA output created by 029 test reflinks across different devices reflink=auto: -42d69d1a6d333a7ebdf64792a555e392 TEST_DIR/test-029/original -42d69d1a6d333a7ebdf64792a555e392 SCRATCH_MNT/test-029/copy +42d69d1a6d333a7ebdf64792a555e392 SCRATCH_MNT/original +42d69d1a6d333a7ebdf64792a555e392 TEST_DIR/test-029/copy reflink=always: cp reflink failed -SCRATCH_MNT/test-029/copyfail +TEST_DIR/test-029/copy test reflinks across different mountpoints of same device reflink=auto: -42d69d1a6d333a7ebdf64792a555e392 TEST_DIR/test-029/original -42d69d1a6d333a7ebdf64792a555e392 SCRATCH_MNT/test-bis-029/copy +42d69d1a6d333a7ebdf64792a555e392 SCRATCH_MNT/original +42d69d1a6d333a7ebdf64792a555e392 TEST_DIR/test-029/copy reflink=always: cp reflink failed -SCRATCH_MNT/test-bis-029/copyfail +TEST_DIR/test-029/copy
Signature · 1 line
2.1.3
PATCH v2 3/4 fstests: cleanup test btrfs/031
In reply to [email protected]From: Filipe Manana <[email protected]>
The test was using $SCRATCH_MNT as a mountpoint for $SCRATCH_DEV, which is counter intuitive and not expected by the fstests framework - this made the test fail after commit 27d077ec0bda (common: use mount/umount helpers everywhere). So rewrite the test to use the scratch device for all data and use a test specific directory inside $TEST_DIR to use as a mount point for a cross mount of $SCRATCH_DEV.
---
V2: First version of the change, introduced in the v2 of the corresponding
patchset.
tests/btrfs/031 | 102 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 51 insertions(+), 51 deletions(-)
tests/btrfs/031 +51 -51
@@ -36,11 +36,6 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _cleanup() { - _scratch_unmount - rm -rf $TESTDIR1 - rm -rf $TESTDIR2 - $BTRFS_UTIL_PROG subvolume delete $SUBVOL1 >> $seqres.full - $BTRFS_UTIL_PROG subvolume delete $SUBVOL2 >> $seqres.full cd / rm -f $tmp.* }
@@ -57,84 +52,89 @@ _supported_os Linux _require_test _require_scratch _require_cp_reflink +_need_to_be_root -_checksum_files() { - for F in file1 file2 file3 - do - echo "$F:" - for D in $TESTDIR1 $SCRATCH_MNT $SUBVOL2 - do - _md5_checksum $D/$F - done - done +_checksum_files() +{ + for f in file1 file2 file3; do + echo "$f:" + for d in $testdir1 $cross_mount_test_dir $subvol2; do + _md5_checksum $d/$f + done + done } -TESTDIR1=$TEST_DIR/test-$seq-1 -TESTDIR2=$TEST_DIR/test-$seq-2 -SUBVOL1=$TEST_DIR/subvol-$seq-1 -SUBVOL2=$TEST_DIR/subvol-$seq-2 +testdir1=$SCRATCH_MNT/test-$seq-1 +testdir2=$SCRATCH_MNT/test-$seq-2 +subvol1=$SCRATCH_MNT/subvol-$seq-1 +subvol2=$SCRATCH_MNT/subvol-$seq-2 +cross_mount_test_dir=$TEST_DIR/test-$seq + +rm -f $seqres.full -_scratch_unmount 2>/dev/null -rm -rf $seqres.full -rm -rf $TESTDIR1 $TESTDIR2 -$BTRFS_UTIL_PROG subvolume delete $SUBVOL1 >/dev/null 2>&1 -$BTRFS_UTIL_PROG subvolume delete $SUBVOL2 >/dev/null 2>&1 +_scratch_mkfs > /dev/null 2>&1 +_scratch_mount -mkdir $TESTDIR1 -mkdir $TESTDIR2 -$BTRFS_UTIL_PROG subvolume create $SUBVOL1 >> $seqres.full -$BTRFS_UTIL_PROG subvolume create $SUBVOL2 >> $seqres.full -_mount -t btrfs -o subvol=subvol-$seq-1 $TEST_DEV $SCRATCH_MNT +mkdir $testdir1 +mkdir $testdir2 +$BTRFS_UTIL_PROG subvolume create $subvol1 >> $seqres.full +$BTRFS_UTIL_PROG subvolume create $subvol2 >> $seqres.full +rm -rf $cross_mount_test_dir +mkdir $cross_mount_test_dir + +_mount -t btrfs -o subvol=subvol-$seq-1 $SCRATCH_DEV $cross_mount_test_dir echo "Create initial files" -# TESTDIR1/file1 is very small and will be inlined -$XFS_IO_PROG -f -c 'pwrite -S 0x61 0 10' $TESTDIR1/file1 \ +# #testdir1/file1 is very small and will be inlined +$XFS_IO_PROG -f -c 'pwrite -S 0x61 0 10' $testdir1/file1 \ >> $seqres.full -$XFS_IO_PROG -f -c 'pwrite -S 0x62 0 13000' $SCRATCH_MNT/file2 \ +$XFS_IO_PROG -f -c 'pwrite -S 0x62 0 13000' $cross_mount_test_dir/file2 \ >> $seqres.full -$XFS_IO_PROG -f -c 'pwrite -S 0x63 0 17000' $SUBVOL2/file3 \ +$XFS_IO_PROG -f -c 'pwrite -S 0x63 0 17000' $subvol2/file3 \ >> $seqres.full echo "Create reflinks to the initial files on other subvolumes" -cp --reflink=always $TESTDIR1/file1 $SUBVOL1 -cp --reflink=always $TESTDIR1/file1 $SUBVOL2 -cp --reflink=always $SUBVOL1/file2 $TESTDIR1/ -cp --reflink=always $SUBVOL1/file2 $SUBVOL2 -cp --reflink=always $SUBVOL2/file3 $TESTDIR1/ -cp --reflink=always $SUBVOL2/file3 $SUBVOL1 +cp --reflink=always $testdir1/file1 $subvol1 +cp --reflink=always $testdir1/file1 $subvol2 +cp --reflink=always $subvol1/file2 $testdir1/ +cp --reflink=always $subvol1/file2 $subvol2 +cp --reflink=always $subvol2/file3 $testdir1/ +cp --reflink=always $subvol2/file3 $subvol1 echo "Verify the reflinks" -_verify_reflink $SCRATCH_MNT/file2 $TESTDIR1/file2 -_verify_reflink $SCRATCH_MNT/file2 $SUBVOL2/file2 -_verify_reflink $SUBVOL2/file3 $TESTDIR1/file3 -_verify_reflink $SUBVOL2/file3 $SCRATCH_MNT/file3 +_verify_reflink $cross_mount_test_dir/file2 $testdir1/file2 +_verify_reflink $cross_mount_test_dir/file2 $subvol2/file2 +_verify_reflink $subvol2/file3 $testdir1/file3 +_verify_reflink $subvol2/file3 $cross_mount_test_dir/file3 echo "Verify the file contents:" _checksum_files echo -e "---\nOverwrite some files with new content" -$XFS_IO_PROG -c 'pwrite -S 0x64 0 20' $TESTDIR1/file1 >> $seqres.full -$XFS_IO_PROG -c 'pwrite -S 0x66 0 21000' $SUBVOL2/file2 >> $seqres.full -$XFS_IO_PROG -c 'pwrite -S 0x65 5000 5000' $SCRATCH_MNT/file3 \ +$XFS_IO_PROG -c 'pwrite -S 0x64 0 20' $testdir1/file1 >> $seqres.full +$XFS_IO_PROG -c 'pwrite -S 0x66 0 21000' $subvol2/file2 >> $seqres.full +$XFS_IO_PROG -c 'pwrite -S 0x65 5000 5000' $cross_mount_test_dir/file3 \ >> $seqres.full echo -n "Verify that non-overwritten reflinks " echo "still have the same data blocks" -_verify_reflink $TESTDIR1/file2 $SCRATCH_MNT/file2 -_verify_reflink $TESTDIR1/file3 $SUBVOL2/file3 +_verify_reflink $testdir1/file2 $cross_mount_test_dir/file2 +_verify_reflink $testdir1/file3 $subvol2/file3 echo "Verify the file contents:" _checksum_files echo -e "---\nShuffle files between directories" -mv $TESTDIR1/file* $TESTDIR2 -mv $SCRATCH_MNT/file* $TESTDIR1/ -mv $SUBVOL2/file* $SCRATCH_MNT/ -mv $TESTDIR2/file* $SUBVOL2/ +mv $testdir1/file* $testdir2 +mv $cross_mount_test_dir/file* $testdir1/ +mv $subvol2/file* $cross_mount_test_dir/ +mv $testdir2/file* $subvol2/ # No _verify_reflink here as data is copied when moving files # between subvols echo "Verify the file contents:" _checksum_files +$UMOUNT_PROG $cross_mount_test_dir + # success, all done status=0 exit
Signature · 1 line
2.1.3
PATCH v2 4/4 fstests: fix cleanup of test btrfs/003
In reply to [email protected]From: Filipe Manana <[email protected]>
If the test fails after removing a device and before adding it back, it attempts to add back the device in its _cleanup() function. However this is broken because the device identifier is stored in a variable local to the function _test_replace() and not in a global variable. So make the variable global instead of local.
---
V2: No changes from v1. Only added to V2 of the corresponding patchset.
tests/btrfs/003 | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
tests/btrfs/003 +5 -5
@@ -29,6 +29,7 @@ here=`pwd` tmp=/tmp/$$ status=1 # failure is the default! dev_removed=0 +removed_dev_htl="" trap "_cleanup; exit \$status" 0 1 2 3 15 _cleanup()
@@ -37,7 +38,7 @@ _cleanup() rm -f $tmp.* if [ $dev_removed == 1 ]; then _scratch_unmount - _devmgt_add "${DEVHTL}" + _devmgt_add "${removed_dev_htl}" fi }
@@ -125,7 +126,6 @@ _test_replace() local n=${#devs[@]} local ds local d - local DEVHTL="" # exclude the first and the last disk in the disk pool n=$(($n-1))
@@ -142,10 +142,10 @@ _test_replace() # retrive the HTL for this scsi disk d=`echo $ds|cut -d"/" -f3` - DEVHTL=`ls -l /sys/class/block/${d} | rev | cut -d "/" -f 3 | rev` + removed_dev_htl=`ls -l /sys/class/block/${d} | rev | cut -d "/" -f 3 | rev` #fail disk - _devmgt_remove ${DEVHTL} $ds + _devmgt_remove ${removed_dev_htl} $ds dev_removed=1 $BTRFS_UTIL_PROG filesystem show $SCRATCH_DEV | grep "Some devices missing" >> $seqres.full || _fail \
@@ -162,7 +162,7 @@ _test_replace() # cleaup. add the removed disk _scratch_unmount - _devmgt_add "${DEVHTL}" + _devmgt_add "${removed_dev_htl}" dev_removed=0 }
Signature · 1 line
2.1.3
Quoted 43 lines · On Wed, Dec 23, 2015 at 04:43:12AM +0000, [email protected] wrote:
From: Filipe Manana <[email protected]>
Commit
27d077ec0bda(common: use mount/umount helpers everywhere) made a few btrfs test fail for 2 different reasons:1) Some tests (btrfs/029 and btrfs/031) use $SCRATCH_MNT as a mount point for some subvolume created in $TEST_DEV, therefore calling
_scratch_unmountdoes not work as it passes $SCRATCH_DEV as the argument to the umount program. This is intentional to test reflinks accross different mountpoints of the same filesystem but for different subvolumes;2) For multiple devices filesystems (btrfs/003 and btrfs/011) that test the device replace feature, we need to unmount using the mount path ($SCRATCH_MNT) because unmounting using one of the devices as an argument ($SCRATCH_DEV) does not always work - after replace operations we get in
/proc/mountsa device other than $SCRATCH_DEV associated with the mount point $SCRATCH_MNT (this is mentioned in a comment at btrfs/011 for example), so we need to pass that other device to the umount program or pass it the mount point.Using $SCRATCH_MNT as a mountpoint for a device other than $SCRATCH_DEV is misleading, but that's a different problem that existed long before and this change attempts only to fix the regression from
27d077ec0bda, leaving such cleanups to later.
Fix this by making_sctatch_unmmount()pass $SCRATCH_MNT to umount instead of $SCRATCH_DEV.Signed-off-by Filipe Manana <[email protected]>---
V2: Change
_sctatch_unmount()to pass $SCRATCH_MNT as the argument to the
umount program instead of $SCRATCH_DEV. This makes the btrfs tests pass again.common/rc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)common/rc +0
-0
common/rc _scratch_unmount() · line 323
Quoted 6 lines
common/rc +1
-1@@ -319,7 +319,7 @@ _scratch_unmount() if [ "$FSTYP" == "overlay" ]; then _overlay_scratch_unmount else - $UMOUNT_PROG $SCRATCH_DEV + $UMOUNT_PROG $SCRATCH_MNT fi
This causes generic/050 to fail (and there's probably others, too):
@@ -15,7 +15,7 @@
mount: SCRATCH_DEV is write-protected, mounting read-only
mount: cannot mount SCRATCH_DEV read-only unmounting read-only filesystem
-umount: SCRATCH_DEV: not mounted
+umount: SCRATCH_MNT: not mounted
mounting filesystem with -o norecovery on a read-only device:
mount: SCRATCH_DEV is write-protected, mounting read-only
...
And, looking at the change for a second or two, the above code is obviously wrong. Have a look at _overlay_scratch_unmount() and have a think about what we normally do when multiple fileystem types need a non-standard behaviour for a generic operation?
Hint: it starts with 'case "$FSTYP" in'....
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]