[PATCH 1/4 v2] fstests: fix btrfs test failures after commit 27d077ec0bda

[PATCH 1/4 v2] fstests: fix btrfs test failures after commit 27d077ec0bda

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

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.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---

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(-)

diff --git a/common/rc b/common/rc
index d33e3fb..16a47fe 100644
--- a/common/rc
+++ b/common/rc
@@ -319,7 +319,7 @@ _scratch_unmount()
 	if [ "$FSTYP" == "overlay" ]; then
 		_overlay_scratch_unmount
 	else
-		$UMOUNT_PROG $SCRATCH_DEV
+		$UMOUNT_PROG $SCRATCH_MNT
 	fi
 }
 
-- 
2.1.3

[PATCH 2/4 v2] fstests: cleanup test btrfs/029

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

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.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---

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(-)

diff --git a/tests/btrfs/029 b/tests/btrfs/029
index cdce6e1..87c289b 100755
--- a/tests/btrfs/029
+++ b/tests/btrfs/029
@@ -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
diff --git a/tests/btrfs/029.out b/tests/btrfs/029.out
index 9390d95..0547d28 100644
--- a/tests/btrfs/029.out
+++ b/tests/btrfs/029.out
@@ -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
-- 
2.1.3

[PATCH 3/4 v2] fstests: cleanup test btrfs/031

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

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.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---

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(-)

diff --git a/tests/btrfs/031 b/tests/btrfs/031
index 0159c95..fe8cd4e 100755
--- a/tests/btrfs/031
+++ b/tests/btrfs/031
@@ -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
-- 
2.1.3

[PATCH 4/4 v2] fstests: fix cleanup of test btrfs/003

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

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.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---

V2: No changes from v1. Only added to V2 of the corresponding patchset.

 tests/btrfs/003 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/btrfs/003 b/tests/btrfs/003
index 353cb48..ae7e377 100755
--- a/tests/btrfs/003
+++ b/tests/btrfs/003
@@ -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
 }
 
-- 
2.1.3

Re: [PATCH 1/4 v2] fstests: fix btrfs test failures after commit 27d077ec0bda

[Dave Chinner]

On Wed, Dec 23, 2015 at 04:43:12AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> 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.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
> 
> 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(-)
> 
> diff --git a/common/rc b/common/rc
> index d33e3fb..16a47fe 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -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.
-- 
Dave Chinner
david@fromorbit.com