[PATCH v4 1/3] common/rc: add scratch shutdown support for overlayfs

[PATCH v4 1/3] common/rc: add scratch shutdown support for overlayfs

[Chengguang Xu]

check -overlay overrides SCRATCH_MNT variable to it's own,
so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
when running shutdown on overlayfs.

Introduce a helper _scratch_shutdown to mainly handle scratch
shutdown and convert godown to _scratch_shutdown to support
overlayfs running on below cases.

generic/043
generic/044
generic/045
generic/046
generic/047
generic/048
generic/051
generic/052
generic/054
generic/055
generic/392
generic/417
generic/461
generic/468

In order to avoid overlayfs running on improper shutdown
cases, add _require_local_device $SCRATCH_DEV to below cases.

generic/042
generic/050
generic/388

Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
---
Changes since v3:
1. When _scratch_shutdown() detecting improper shutdown on overlayfs,
prompt error message and exit immediately.
2. Convert godown to _scratch_shutdown for the cases overlayfs shuold run.
3. Add _require_local_device to the cases overlayfs should not run.

Changes since v2:
1. Make option for _scratch_shutdown(), so caller can
specify option when calling.
2. Add comment for why overlay requires special handling.
3. Add commit log.

Changes since v1:
_scratch_shutdown() does not call notrun.

 common/rc         | 38 ++++++++++++++++++++++++++++++++++++--
 tests/generic/042 |  1 +
 tests/generic/043 |  2 +-
 tests/generic/044 |  2 +-
 tests/generic/045 |  2 +-
 tests/generic/046 |  2 +-
 tests/generic/047 |  2 +-
 tests/generic/048 |  2 +-
 tests/generic/049 |  2 +-
 tests/generic/050 |  1 +
 tests/generic/051 |  2 +-
 tests/generic/052 |  2 +-
 tests/generic/054 |  2 +-
 tests/generic/055 |  2 +-
 tests/generic/388 |  3 ++-
 tests/generic/392 |  2 +-
 tests/generic/417 |  2 +-
 tests/generic/461 |  2 +-
 tests/generic/468 |  2 +-
 19 files changed, 55 insertions(+), 18 deletions(-)

diff --git a/common/rc b/common/rc
index 4c053a5..fc9a4f5 100644
--- a/common/rc
+++ b/common/rc
@@ -382,6 +382,25 @@ _scratch_cycle_mount()
     _scratch_mount "$opts"
 }
 
+_scratch_shutdown()
+{
+	if [ $FSTYP = "overlay" ]; then
+		# In lagacy overlay usage, it may specify directory as
+		# SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV
+		# will be null, so check OVL_BASE_SCRATCH_DEV before
+		# running shutdown to avoid shutting down base fs accidently.
+		if [ -z $OVL_BASE_SCRATCH_DEV ]; then
+			_fail "Called _scratch_shutdown without validating " \
+				"\$OVL_BASE_SCRATCH_DEV, " \
+				"please call _require_scratch_shutdown first."
+		else
+			src/godown $* $OVL_BASE_SCRATCH_MNT
+		fi
+	else
+		src/godown $* $SCRATCH_MNT
+	fi
+}
+
 _test_mount()
 {
     if [ "$FSTYP" == "overlay" ]; then
@@ -2908,8 +2927,23 @@ _require_scratch_shutdown()
 
 	_scratch_mkfs > /dev/null 2>&1
 	_scratch_mount
-	src/godown -f $SCRATCH_MNT 2>&1 \
-		|| _notrun "$FSTYP does not support shutdown"
+
+	if [ $FSTYP = "overlay" ]; then
+		if [ -z $OVL_BASE_SCRATCH_DEV ]; then
+			# In lagacy overlay usage, it may specify directory as
+			# SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV
+			# will be null, so check OVL_BASE_SCRATCH_DEV before
+			# running shutdown to avoid shutting down base fs accidently.
+			_notrun "$SCRATCH_DEV is not a block device"
+		else
+			src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \
+			|| _notrun "Underlying filesystem does not support shutdown"
+		fi
+	else
+		src/godown -f $SCRATCH_MNT 2>&1 \
+			|| _notrun "$FSTYP does not support shutdown"
+	fi
+
 	_scratch_unmount
 }
 
diff --git a/tests/generic/042 b/tests/generic/042
index 68ff03c..00b3a34 100755
--- a/tests/generic/042
+++ b/tests/generic/042
@@ -92,6 +92,7 @@ _require_xfs_io_command "fpunch"
 _require_xfs_io_command "fzero"
 
 _scratch_mkfs >/dev/null 2>&1
+_require_local_device $SCRATCH_DEV
 _require_metadata_journaling $SCRATCH_DEV
 _scratch_mount
 
diff --git a/tests/generic/043 b/tests/generic/043
index 5dadab3..f61222c 100755
--- a/tests/generic/043
+++ b/tests/generic/043
@@ -63,7 +63,7 @@ done
 # give the system a chance to write something out
 sleep 10
 
-src/godown $SCRATCH_MNT
+_scratch_shutdown
 
 _scratch_unmount
 _scratch_mount
diff --git a/tests/generic/044 b/tests/generic/044
index 804b1b1..f327ee0 100755
--- a/tests/generic/044
+++ b/tests/generic/044
@@ -69,7 +69,7 @@ done
 # give the system a chance to write something out
 sleep 10
 
-src/godown $SCRATCH_MNT
+_scratch_shutdown
 
 _scratch_unmount
 _scratch_mount
diff --git a/tests/generic/045 b/tests/generic/045
index 5fa7b09..5348910 100755
--- a/tests/generic/045
+++ b/tests/generic/045
@@ -69,7 +69,7 @@ done
 # give the system a chance to write something out
 sleep 10
 
-src/godown $SCRATCH_MNT
+_scratch_shutdown
 
 _scratch_unmount
 _scratch_mount
diff --git a/tests/generic/046 b/tests/generic/046
index bf38d53..1155ee9 100755
--- a/tests/generic/046
+++ b/tests/generic/046
@@ -69,7 +69,7 @@ done
 # give the system a chance to write something out
 sleep 10
 
-src/godown $SCRATCH_MNT
+_scratch_shutdown
 
 _scratch_unmount
 _scratch_mount
diff --git a/tests/generic/047 b/tests/generic/047
index 7d09b04..66965f2 100755
--- a/tests/generic/047
+++ b/tests/generic/047
@@ -92,7 +92,7 @@ do
 done
 
 # shutdown immediately after, then remount and test
-src/godown $SCRATCH_MNT
+_scratch_shutdown
 _scratch_unmount
 _scratch_mount
 _scratch_unmount
diff --git a/tests/generic/048 b/tests/generic/048
index ae561fc..fd9fcd9 100755
--- a/tests/generic/048
+++ b/tests/generic/048
@@ -96,7 +96,7 @@ done
 
 # sync, then shutdown immediately after, then remount and test
 sync
-src/godown $SCRATCH_MNT
+_scratch_shutdown
 _scratch_unmount
 _scratch_mount
 _scratch_unmount
diff --git a/tests/generic/049 b/tests/generic/049
index ef2b44c..0003046 100755
--- a/tests/generic/049
+++ b/tests/generic/049
@@ -93,7 +93,7 @@ done
 
 # sync, then shutdown immediately after, then remount and test
 sync
-src/godown $SCRATCH_MNT
+_scratch_shutdown
 _scratch_unmount
 _scratch_mount
 _scratch_unmount
diff --git a/tests/generic/050 b/tests/generic/050
index efa45f0..dbf0ac5 100755
--- a/tests/generic/050
+++ b/tests/generic/050
@@ -44,6 +44,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fs generic
 _supported_os Linux
 
+_require_local_device $SCRATCH_DEV
 _require_scratch_nocheck
 _require_scratch_shutdown
 _require_norecovery
diff --git a/tests/generic/051 b/tests/generic/051
index 29ac61b..129a466 100755
--- a/tests/generic/051
+++ b/tests/generic/051
@@ -80,7 +80,7 @@ sync
 
 # now shutdown and unmount
 sleep 5
-$here/src/godown $load_dir
+_scratch_shutdown
 $KILLALL_PROG -q $FSSTRESS_PROG
 wait
 
diff --git a/tests/generic/052 b/tests/generic/052
index cf0f456..126f08e 100755
--- a/tests/generic/052
+++ b/tests/generic/052
@@ -63,7 +63,7 @@ echo "touch files"
 touch $SCRATCH_MNT/{0,1,2,3,4,5,6,7,8,9}{0,1,2,3,4,5,6,7,8,9}
 
 echo "godown"
-src/godown -v -f $SCRATCH_MNT >> $seqres.full
+_scratch_shutdown -v -f >> $seqres.full
 
 echo "unmount"
 _scratch_unmount
diff --git a/tests/generic/054 b/tests/generic/054
index db41500..12f471a 100755
--- a/tests/generic/054
+++ b/tests/generic/054
@@ -108,7 +108,7 @@ for s in sync nosync ; do
 	ls $SCRATCH_MNT | _filter_lostfound
 
 	_echofull "godown"
-	src/godown -v -f $SCRATCH_MNT >> $seqres.full
+	_scratch_shutdown -v -f >> $seqres.full
 
 	_echofull "unmount"
 	_scratch_unmount >>$seqres.full 2>&1 \
diff --git a/tests/generic/055 b/tests/generic/055
index 1bbe310..c543e75 100755
--- a/tests/generic/055
+++ b/tests/generic/055
@@ -118,7 +118,7 @@ do
     ls -RF $SCRATCH_MNT >$tmp.ls1
 
     _echofull "godown"
-    src/godown -v -f $SCRATCH_MNT >> $seqres.full
+    _scratch_shutdown -v -f >> $seqres.full
 
     _echofull "unmount"
     _scratch_unmount >>$seqres.full 2>&1 \
diff --git a/tests/generic/388 b/tests/generic/388
index 4f5a63b..e0f4821 100755
--- a/tests/generic/388
+++ b/tests/generic/388
@@ -54,6 +54,7 @@ _supported_fs generic
 _supported_os Linux
 
 _require_scratch
+_require_local_device $SCRATCH_DEV
 _require_scratch_shutdown
 _require_command "$KILLALL_PROG" "killall"
 
@@ -72,7 +73,7 @@ for i in $(seq 1 $((50 * TIME_FACTOR)) ); do
 	# purposely include 0 second sleeps to test shutdown immediately after
 	# recovery
 	sleep $((RANDOM % 3))
-	./src/godown $SCRATCH_MNT
+	_scratch_shutdown
 
 	ps -e | grep fsstress > /dev/null 2>&1
 	while [ $? -eq 0 ]; do
diff --git a/tests/generic/392 b/tests/generic/392
index 6922f7d..9d53413 100755
--- a/tests/generic/392
+++ b/tests/generic/392
@@ -73,7 +73,7 @@ check_inode_metadata()
 	before=`stat "$stat_opt" $testfile`
 
 	$XFS_IO_PROG -c "$sync_mode" $testfile | _filter_xfs_io
-	src/godown $SCRATCH_MNT | tee -a $seqres.full
+	_scratch_shutdown | tee -a $seqres.full
 	_scratch_cycle_mount
 
 	after=`stat "$stat_opt" $testfile`
diff --git a/tests/generic/417 b/tests/generic/417
index 690ceb5..e66e0ae 100755
--- a/tests/generic/417
+++ b/tests/generic/417
@@ -75,7 +75,7 @@ function create_dirty_orphans() {
 	sleep 3
 
 	echo "godown"
-	src/godown -v -f $SCRATCH_MNT >> $seqres.full
+	_scratch_shutdown -v -f >> $seqres.full
 
 	# kill the multi_open_unlink
 	kill $pid 2>/dev/null
diff --git a/tests/generic/461 b/tests/generic/461
index 2f85114..50b9c6d 100755
--- a/tests/generic/461
+++ b/tests/generic/461
@@ -68,7 +68,7 @@ sync
 
 # now shutdown and unmount
 sleep 5
-$here/src/godown $load_dir
+_scratch_shutdown
 $KILLALL_PROG -q $FSSTRESS_PROG
 wait
 
diff --git a/tests/generic/468 b/tests/generic/468
index b97a8d6..30c4174 100755
--- a/tests/generic/468
+++ b/tests/generic/468
@@ -84,7 +84,7 @@ check_inode_metadata()
 	before=`stat "$stat_opt" $testfile`
 
 	$XFS_IO_PROG -c "$sync_mode" $testfile | _filter_xfs_io
-	$here/src/godown $SCRATCH_MNT | tee -a $seqres.full
+	_scratch_shutdown | tee -a $seqres.full
 	_scratch_cycle_mount
 
 	after=`stat "$stat_opt" $testfile`
-- 
1.8.3.1

[PATCH v4 2/3] common/rc: check syncfs support

[Chengguang Xu]

common/rc: add a check case in _require_xfs_io_command() to support syncfs

Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
---
 common/rc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common/rc b/common/rc
index fc9a4f5..da7c827 100644
--- a/common/rc
+++ b/common/rc
@@ -2106,6 +2106,10 @@ _require_xfs_io_command()
 	"utimes" )
 		testio=`$XFS_IO_PROG -f -c "utimes" 0 0 0 0 $testfile 2>&1`
 		;;
+	"syncfs")
+		touch $testfile
+		testio=`$XFS_IO_PROG -c "syncfs" $testfile 2>&1`
+		;;
 	*)
 		testio=`$XFS_IO_PROG -c "help $command" 2>&1`
 	esac
-- 
1.8.3.1

[PATCH v4 3/3] generic/471: add syncfs test

[Chengguang Xu]

Inspired by syncfs bug of overlayfs which does not sync dirtyinodes in
underlying filesystem.
Run syncfs and shutdown filesystem(or underlying filesystem of overlayfs)
to check syncfs result.

Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
---
Changes since v3:
1. Change case No to 471.
2. Change failure checking method.
3. Cut comment about fssum.

Changes since v2:
1. Modify multiple test files to single small test file.
2. Use fssum to check result instead of diff.
3. Add comment for why running sync before test.
4. Add option explanation of fssum.

Changes since v1:
Use fs shutdown and fssum to check syncfs result instead of
checking delalloc state of extents.

 tests/generic/471     | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/471.out |  2 ++
 tests/generic/group   |  1 +
 3 files changed, 87 insertions(+)
 create mode 100755 tests/generic/471
 create mode 100644 tests/generic/471.out

diff --git a/tests/generic/471 b/tests/generic/471
new file mode 100755
index 0000000..bd1a72f
--- /dev/null
+++ b/tests/generic/471
@@ -0,0 +1,84 @@
+#! /bin/bash
+# FS QA Test 471
+#
+# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
+# underlying filesystem.
+#
+# Create a small file then run syncfs and shutdown filesystem(or underlying
+# filesystem of overlayfs) to check syncfs result.
+#
+# Test will be skipped if filesystem(or underlying filesystem of overlayfs)
+# does not support shutdown.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2017 Chengguang Xu <cgxu519@icloud.com>
+# All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=0
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+_supported_fs generic
+_supported_os Linux
+_require_fssum
+_require_scratch
+_require_scratch_shutdown
+_require_xfs_io_command "syncfs"
+
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+# Background writeback will flush dirty inode by dirty ratio and dirty time
+# period(default 30 seconds), in order to avoid interference from it,
+# run sync before test to make all dirty inodes clean, and it also
+# accelerates syncfs on testing filesystem so that test case can finish
+# in 30 seconds.
+
+sync
+
+$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/testfile >/dev/null 2>&1
+
+# fssum used for comparing checksum of test file(data & metedata),
+# exclude checking about atime, block structure, open error.
+$FSSUM_PROG -ugomAcdES -f -w $tmp.fssum $SCRATCH_MNT
+$XFS_IO_PROG -c "syncfs" $SCRATCH_MNT/testfile >/dev/null 2>&1
+_scratch_shutdown
+_scratch_cycle_mount
+$FSSUM_PROG -r $tmp.fssum $SCRATCH_MNT
+
+exit
diff --git a/tests/generic/471.out b/tests/generic/471.out
new file mode 100644
index 0000000..6944d77
--- /dev/null
+++ b/tests/generic/471.out
@@ -0,0 +1,2 @@
+QA output created by 471
+OK
diff --git a/tests/generic/group b/tests/generic/group
index 1e60722..3a61f42 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -473,3 +473,4 @@
 468 shutdown auto quick metadata
 469 auto quick
 470 auto quick dax
+471 shutdown auto quick sync
-- 
1.8.3.1

Re: [PATCH v4 3/3] generic/471: add syncfs test

[Amir Goldstein]

On Thu, Dec 14, 2017 at 4:19 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
> Inspired by syncfs bug of overlayfs which does not sync dirtyinodes in
> underlying filesystem.
> Run syncfs and shutdown filesystem(or underlying filesystem of overlayfs)
> to check syncfs result.
>
> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
> ---
> Changes since v3:
> 1. Change case No to 471.
> 2. Change failure checking method.
> 3. Cut comment about fssum.
>

Looks good.

> Changes since v2:
> 1. Modify multiple test files to single small test file.
> 2. Use fssum to check result instead of diff.
> 3. Add comment for why running sync before test.
> 4. Add option explanation of fssum.
>
> Changes since v1:
> Use fs shutdown and fssum to check syncfs result instead of
> checking delalloc state of extents.
>
>  tests/generic/471     | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/471.out |  2 ++
>  tests/generic/group   |  1 +
>  3 files changed, 87 insertions(+)
>  create mode 100755 tests/generic/471
>  create mode 100644 tests/generic/471.out
>
> diff --git a/tests/generic/471 b/tests/generic/471
> new file mode 100755
> index 0000000..bd1a72f
> --- /dev/null
> +++ b/tests/generic/471
> @@ -0,0 +1,84 @@
> +#! /bin/bash
> +# FS QA Test 471
> +#
> +# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
> +# underlying filesystem.
> +#
> +# Create a small file then run syncfs and shutdown filesystem(or underlying
> +# filesystem of overlayfs) to check syncfs result.
> +#
> +# Test will be skipped if filesystem(or underlying filesystem of overlayfs)
> +# does not support shutdown.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 Chengguang Xu <cgxu519@icloud.com>
> +# All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=0
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       cd /
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +_supported_fs generic
> +_supported_os Linux
> +_require_fssum
> +_require_scratch
> +_require_scratch_shutdown
> +_require_xfs_io_command "syncfs"
> +
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +# Background writeback will flush dirty inode by dirty ratio and dirty time
> +# period(default 30 seconds), in order to avoid interference from it,
> +# run sync before test to make all dirty inodes clean, and it also
> +# accelerates syncfs on testing filesystem so that test case can finish
> +# in 30 seconds.
> +
> +sync
> +
> +$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/testfile >/dev/null 2>&1
> +
> +# fssum used for comparing checksum of test file(data & metedata),
> +# exclude checking about atime, block structure, open error.
> +$FSSUM_PROG -ugomAcdES -f -w $tmp.fssum $SCRATCH_MNT
> +$XFS_IO_PROG -c "syncfs" $SCRATCH_MNT/testfile >/dev/null 2>&1
> +_scratch_shutdown
> +_scratch_cycle_mount
> +$FSSUM_PROG -r $tmp.fssum $SCRATCH_MNT
> +
> +exit
> diff --git a/tests/generic/471.out b/tests/generic/471.out
> new file mode 100644
> index 0000000..6944d77
> --- /dev/null
> +++ b/tests/generic/471.out
> @@ -0,0 +1,2 @@
> +QA output created by 471
> +OK
> diff --git a/tests/generic/group b/tests/generic/group
> index 1e60722..3a61f42 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -473,3 +473,4 @@
>  468 shutdown auto quick metadata
>  469 auto quick
>  470 auto quick dax
> +471 shutdown auto quick sync
> --
> 1.8.3.1
>

Re: [PATCH v4 2/3] common/rc: check syncfs support

[Amir Goldstein]

On Thu, Dec 14, 2017 at 4:19 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
> common/rc: add a check case in _require_xfs_io_command() to support syncfs
>
> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>

looks good.

> ---
>  common/rc | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/common/rc b/common/rc
> index fc9a4f5..da7c827 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2106,6 +2106,10 @@ _require_xfs_io_command()
>         "utimes" )
>                 testio=`$XFS_IO_PROG -f -c "utimes" 0 0 0 0 $testfile 2>&1`
>                 ;;
> +       "syncfs")
> +               touch $testfile
> +               testio=`$XFS_IO_PROG -c "syncfs" $testfile 2>&1`
> +               ;;
>         *)
>                 testio=`$XFS_IO_PROG -c "help $command" 2>&1`
>         esac
> --
> 1.8.3.1
>

Re: [PATCH v4 1/3] common/rc: add scratch shutdown support for overlayfs

[Amir Goldstein]

On Thu, Dec 14, 2017 at 4:19 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
> check -overlay overrides SCRATCH_MNT variable to it's own,
> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
> when running shutdown on overlayfs.
>
> Introduce a helper _scratch_shutdown to mainly handle scratch
> shutdown and convert godown to _scratch_shutdown to support
> overlayfs running on below cases.

You should do this in 2 patches:
1. replace all open coded godown calls with _scratch_shutdown helper
2. Add overlay support to _scratch_shutdown helper.

>
> generic/043
> generic/044
> generic/045
> generic/046
> generic/047
> generic/048
> generic/051
> generic/052
> generic/054
> generic/055
> generic/392
> generic/417
> generic/461
> generic/468

Really no need to specify these in change log. They already appear in diffstat.

>
> In order to avoid overlayfs running on improper shutdown
> cases, add _require_local_device $SCRATCH_DEV to below cases.

I don't understand.
In all these tests, there is already _require_scratch_shutdown.
Shouldn't that prevent overlay running improper shutdown?

Amir.

Re: [PATCH v4 1/3] common/rc: add scratch shutdown support for overlayfs

[Eryu Guan]

On Thu, Dec 14, 2017 at 04:46:21PM +0200, Amir Goldstein wrote:
> On Thu, Dec 14, 2017 at 4:19 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
> > check -overlay overrides SCRATCH_MNT variable to it's own,
> > so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
> > when running shutdown on overlayfs.
> >
> > Introduce a helper _scratch_shutdown to mainly handle scratch
> > shutdown and convert godown to _scratch_shutdown to support
> > overlayfs running on below cases.
> 
> You should do this in 2 patches:
> 1. replace all open coded godown calls with _scratch_shutdown helper
> 2. Add overlay support to _scratch_shutdown helper.

The description is not clear enough, the updates are not for running
existing shutdown tests to run on overlay, but to avoid test failures
using the bare 'src/godown $SCRATCH_MNT' when _require_scratch_shutdown
accepts overlay to run. e.g.

+/root/xfstests/src/godown: error on xfsctl(GOINGDOWN) of "/mnt/scratch/ovl-mnt": Inappropriate ioctl for device

So I think it's proper to do all the required updates in one patch,
instead of introducing (intermediate) false failures, and fixing them in
a follow-up patch.

> 
> >
> > generic/043
> > generic/044
> > generic/045
> > generic/046
> > generic/047
> > generic/048
> > generic/051
> > generic/052
> > generic/054
> > generic/055
> > generic/392
> > generic/417
> > generic/461
> > generic/468
> 
> Really no need to specify these in change log. They already appear in diffstat.

Agreed.

> 
> >
> > In order to avoid overlayfs running on improper shutdown
> > cases, add _require_local_device $SCRATCH_DEV to below cases.
> 
> I don't understand.
> In all these tests, there is already _require_scratch_shutdown.
> Shouldn't that prevent overlay running improper shutdown?

generic/042 and generic/050 assume we're testing against a local device
(do mkfs or operate on $SCRATCH_DEV directly), so we need additional
check on $SCRATCH_DEV. Perhaps we need some comments in each test. But I
don't see why generic/388 requires a local device too, maybe I missed
something, that's why we need comments :)

Thanks,
Eryu

Re: [PATCH v4 1/3] common/rc: add scratch shutdown support for overlayfs

[Amir Goldstein]

On Thu, Dec 14, 2017 at 5:02 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Thu, Dec 14, 2017 at 04:46:21PM +0200, Amir Goldstein wrote:
>> On Thu, Dec 14, 2017 at 4:19 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>> > check -overlay overrides SCRATCH_MNT variable to it's own,
>> > so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
>> > when running shutdown on overlayfs.
>> >
>> > Introduce a helper _scratch_shutdown to mainly handle scratch
>> > shutdown and convert godown to _scratch_shutdown to support
>> > overlayfs running on below cases.
>>
>> You should do this in 2 patches:
>> 1. replace all open coded godown calls with _scratch_shutdown helper
>> 2. Add overlay support to _scratch_shutdown helper.
>
> The description is not clear enough, the updates are not for running
> existing shutdown tests to run on overlay, but to avoid test failures
> using the bare 'src/godown $SCRATCH_MNT' when _require_scratch_shutdown
> accepts overlay to run. e.g.
>
> +/root/xfstests/src/godown: error on xfsctl(GOINGDOWN) of "/mnt/scratch/ovl-mnt": Inappropriate ioctl for device
>
> So I think it's proper to do all the required updates in one patch,
> instead of introducing (intermediate) false failures, and fixing them in
> a follow-up patch.
>

I take your word for it. In that case, I'll wait for the patch will
clearer description.

Amir.

Re: [PATCH v4 1/3] common/rc: add scratch shutdown support for overlayfs

[Chengguang Xu]

> 
> 在 2017年12月14日，下午11:02，Eryu Guan <eguan@redhat.com> 写道：
> 
> On Thu, Dec 14, 2017 at 04:46:21PM +0200, Amir Goldstein wrote:
>> On Thu, Dec 14, 2017 at 4:19 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>> check -overlay overrides SCRATCH_MNT variable to it's own,
>>> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
>>> when running shutdown on overlayfs.
>>> 
>>> Introduce a helper _scratch_shutdown to mainly handle scratch
>>> shutdown and convert godown to _scratch_shutdown to support
>>> overlayfs running on below cases.
>> 
>> You should do this in 2 patches:
>> 1. replace all open coded godown calls with _scratch_shutdown helper
>> 2. Add overlay support to _scratch_shutdown helper.
> 
> The description is not clear enough, the updates are not for running
> existing shutdown tests to run on overlay, but to avoid test failures
> using the bare 'src/godown $SCRATCH_MNT' when _require_scratch_shutdown
> accepts overlay to run. e.g.
> 
> +/root/xfstests/src/godown: error on xfsctl(GOINGDOWN) of "/mnt/scratch/ovl-mnt": Inappropriate ioctl for device
> 
> So I think it's proper to do all the required updates in one patch,
> instead of introducing (intermediate) false failures, and fixing them in
> a follow-up patch.
> 
>> 
>>> 
>>> generic/043
>>> generic/044
>>> generic/045
>>> generic/046
>>> generic/047
>>> generic/048
>>> generic/051
>>> generic/052
>>> generic/054
>>> generic/055
>>> generic/392
>>> generic/417
>>> generic/461
>>> generic/468
>> 
>> Really no need to specify these in change log. They already appear in diffstat.
> 
> Agreed.
> 
>> 
>>> 
>>> In order to avoid overlayfs running on improper shutdown
>>> cases, add _require_local_device $SCRATCH_DEV to below cases.
>> 
>> I don't understand.
>> In all these tests, there is already _require_scratch_shutdown.
>> Shouldn't that prevent overlay running improper shutdown?
> 
> generic/042 and generic/050 assume we're testing against a local device
> (do mkfs or operate on $SCRATCH_DEV directly), so we need additional
> check on $SCRATCH_DEV. Perhaps we need some comments in each test. But I
> don't see why generic/388 requires a local device too, maybe I missed
> something, that's why we need comments :)
> 

generic/388 causes underlying filesystem corruption and can’t be mounted
unless running new mkfs. For local device based filesystems, it’s not a problem
but overlayfs does not do real mkfs for device so the cases after this will be
all failed. Even worse，there is no check about _scratch_mount result before 
doing shutdown，so it can cause base fs shutting down unexpectedly. 
For safety, I’ll add a check in _require_scratch_shutdown, but _scratch_shutdown
caller still needs to check mount status carefully in their test.


Thanks,
Chengguang.