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

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

[Chengguang Xu]

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

Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
---

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 | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 4c053a5..a5c4ace 100644
--- a/common/rc
+++ b/common/rc
@@ -382,6 +382,24 @@ _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
+			echo -n "Ignore shutdown because "
+			echo "$SCRATCH_DEV is not a block device"
+		else
+			src/godown $* $OVL_BASE_SCRATCH_MNT
+		fi
+	else
+		src/godown $* $SCRATCH_MNT
+	fi
+}
+
 _test_mount()
 {
     if [ "$FSTYP" == "overlay" ]; then
@@ -2908,8 +2926,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
 }
 
-- 
1.8.3.1

[PATCH v3 2/3] common/rc: add a check case in _require_xfs_io_command() to support syncfs.

[Chengguang Xu]

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 a5c4ace..a31ea48 100644
--- a/common/rc
+++ b/common/rc
@@ -2105,6 +2105,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 v3 3/3] generic/470: 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 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/470     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/470.out |   2 +
 tests/generic/group   |   1 +
 3 files changed, 105 insertions(+)
 create mode 100755 tests/generic/470
 create mode 100644 tests/generic/470.out

diff --git a/tests/generic/470 b/tests/generic/470
new file mode 100755
index 0000000..9d6c37a
--- /dev/null
+++ b/tests/generic/470
@@ -0,0 +1,102 @@
+#! /bin/bash
+# FS QA Test 470
+#
+# 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=1
+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).
+# option explanation:
+#    -f          : write out a full manifest file
+#    -w <file>   : send output to file
+#    -r <file>   : read checksum or manifest from file
+#
+#    -[ugoamcde] : specify which fields to include in checksum calculation.
+#         u      : include uid
+#         g      : include gid
+#         o      : include mode
+#         m      : include mtime
+#         a      : include atime
+#         c      : include ctime
+#         d      : include file data
+#         e      : include open errors (aborts otherwise)
+#         s      : include block structure (holes)
+#    -[UGOAMCDES]: exclude respective field from calculation
+
+$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 >/dev/null 2>&1
+status=$?
+
+echo "Silence is golden"
+exit
diff --git a/tests/generic/470.out b/tests/generic/470.out
new file mode 100644
index 0000000..79fb532
--- /dev/null
+++ b/tests/generic/470.out
@@ -0,0 +1,2 @@
+QA output created by 470
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index 6c3bb03..493a44c 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -472,3 +472,4 @@
 467 auto quick exportfs
 468 shutdown auto quick metadata
 469 auto quick
+470 auto quick shutdown sync
-- 
1.8.3.1

Re: [PATCH v3 3/3] generic/470: add syncfs test

[Amir Goldstein]

On Tue, Dec 12, 2017 at 5:09 AM, 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 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/470     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/470.out |   2 +
>  tests/generic/group   |   1 +
>  3 files changed, 105 insertions(+)
>  create mode 100755 tests/generic/470
>  create mode 100644 tests/generic/470.out
>
> diff --git a/tests/generic/470 b/tests/generic/470
> new file mode 100755
> index 0000000..9d6c37a
> --- /dev/null
> +++ b/tests/generic/470
> @@ -0,0 +1,102 @@
> +#! /bin/bash
> +# FS QA Test 470
> +#
> +# 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=1
> +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).
> +# option explanation:
> +#    -f          : write out a full manifest file
> +#    -w <file>   : send output to file
> +#    -r <file>   : read checksum or manifest from file
> +#
> +#    -[ugoamcde] : specify which fields to include in checksum calculation.
> +#         u      : include uid
> +#         g      : include gid
> +#         o      : include mode
> +#         m      : include mtime
> +#         a      : include atime
> +#         c      : include ctime
> +#         d      : include file data
> +#         e      : include open errors (aborts otherwise)
> +#         s      : include block structure (holes)
> +#    -[UGOAMCDES]: exclude respective field from calculation
> +

That's too much. A comment about what attributes you are not comparing
would have been enough.

> +$FSSUM_PROG -ugomAcdES -f -w $tmp.fssum $SCRATCH_MNT

You can fssum $SCRATCH_MNT/testfile. you only have one file..


> +$XFS_IO_PROG -c "syncfs" $SCRATCH_MNT/testfile >/dev/null 2>&1
> +_scratch_shutdown
> +_scratch_cycle_mount
> +$FSSUM_PROG -r $tmp.fssum $SCRATCH_MNT >/dev/null 2>&1

$SCRATCH_MNT/testfile

Otherwise looks good.
Amir.

Re: [PATCH v3 3/3] generic/470: add syncfs test

[Chengguang Xu]

> 在 2017年12月12日，下午2:00，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Tue, Dec 12, 2017 at 5:09 AM, 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 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/470     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> tests/generic/470.out |   2 +
>> tests/generic/group   |   1 +
>> 3 files changed, 105 insertions(+)
>> create mode 100755 tests/generic/470
>> create mode 100644 tests/generic/470.out
>> 
>> diff --git a/tests/generic/470 b/tests/generic/470
>> new file mode 100755
>> index 0000000..9d6c37a
>> --- /dev/null
>> +++ b/tests/generic/470
>> @@ -0,0 +1,102 @@
>> +#! /bin/bash
>> +# FS QA Test 470
>> +#
>> +# 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=1
>> +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).
>> +# option explanation:
>> +#    -f          : write out a full manifest file
>> +#    -w <file>   : send output to file
>> +#    -r <file>   : read checksum or manifest from file
>> +#
>> +#    -[ugoamcde] : specify which fields to include in checksum calculation.
>> +#         u      : include uid
>> +#         g      : include gid
>> +#         o      : include mode
>> +#         m      : include mtime
>> +#         a      : include atime
>> +#         c      : include ctime
>> +#         d      : include file data
>> +#         e      : include open errors (aborts otherwise)
>> +#         s      : include block structure (holes)
>> +#    -[UGOAMCDES]: exclude respective field from calculation
>> +
> 
> That's too much. A comment about what attributes you are not comparing
> would have been enough.
> 
>> +$FSSUM_PROG -ugomAcdES -f -w $tmp.fssum $SCRATCH_MNT
> 
> You can fssum $SCRATCH_MNT/testfile. you only have one file..

fssum requires a path not a file, even we have only one file.

Thanks,
Chengguang.

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

[Eryu Guan]

On Tue, Dec 12, 2017 at 11:09:39AM +0800, Chengguang Xu 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 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     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/471.out |   2 +
>  tests/generic/group   |   1 +
>  3 files changed, 105 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..9d6c37a
> --- /dev/null
> +++ b/tests/generic/471
> @@ -0,0 +1,102 @@
> +#! /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=1
> +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

Need "_require_metadata_journaling $SCRATCH_DEV" after _scratch_mkfs, so
filesystems like no-journal mode ext4 or ext2 (driven by ext4 module)
won't run this test.

> +_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).
> +# option explanation:
> +#    -f          : write out a full manifest file
> +#    -w <file>   : send output to file
> +#    -r <file>   : read checksum or manifest from file
> +#
> +#    -[ugoamcde] : specify which fields to include in checksum calculation.
> +#         u      : include uid
> +#         g      : include gid
> +#         o      : include mode
> +#         m      : include mtime
> +#         a      : include atime
> +#         c      : include ctime
> +#         d      : include file data
> +#         e      : include open errors (aborts otherwise)
> +#         s      : include block structure (holes)
> +#    -[UGOAMCDES]: exclude respective field from calculation
> +
> +$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 >/dev/null 2>&1

No need to throw away the fssum -r output, just dump its output and let
it match the golden image. On failure, fssum reports something like:

metadata and data mismatch in /testfile
FAIL

and on success, it prints a "OK". So the code can be:

...
$FSSUM_PROG -r $tmp.fssum $SCRATCH_MNT

status=0
exit

> +status=$?
> +
> +echo "Silence is golden"
> +exit
> diff --git a/tests/generic/471.out b/tests/generic/471.out
> new file mode 100644
> index 0000000..79fb532
> --- /dev/null
> +++ b/tests/generic/471.out
> @@ -0,0 +1,2 @@
> +QA output created by 471
> +Silence is golden

And update .out file as:

QA output created by 471
OK

This way, it's clearer in which way test fails on failure:

    --- tests/generic/471.out   2017-12-12 15:37:59.011005399 +0800
    +++ /root/xfstests/results//xfs_4k/generic/471.out.bad      2017-12-12 17:19:54.043896626 +0800
    @@ -1,2 +1,3 @@
     QA output created by 471
    -OK
    +metadata and data mismatch in /testfile
    +FAIL

Thanks,
Eryu

> diff --git a/tests/generic/group b/tests/generic/group
> index 6c3bb03..493a44c 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -472,3 +472,4 @@
>  468 shutdown auto quick metadata
>  469 auto quick
>  470 auto quick dax
> +471 auto quick shutdown sync
> -- 
> 1.8.3.1
> 

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

[Amir Goldstein]

On Tue, Dec 12, 2017 at 11:24 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Tue, Dec 12, 2017 at 11:09:39AM +0800, Chengguang Xu 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 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     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/generic/471.out |   2 +
>>  tests/generic/group   |   1 +
>>  3 files changed, 105 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..9d6c37a
>> --- /dev/null
>> +++ b/tests/generic/471
>> @@ -0,0 +1,102 @@
>> +#! /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=1
>> +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
>
> Need "_require_metadata_journaling $SCRATCH_DEV" after _scratch_mkfs, so
> filesystems like no-journal mode ext4 or ext2 (driven by ext4 module)
> won't run this test.
>

In that case, Chengguang would also have to teach _require_metadata_journaling
about OVERLAY_BASE_SCRATCH_DEV....

Chengguang,

Sorry for all the extra work.
That means you are making an important improvement to xfstests!

Thanks,
Amir.

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

[Chengguang Xu]

> 
> 在 2017年12月12日，下午5:56，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Tue, Dec 12, 2017 at 11:24 AM, Eryu Guan <eguan@redhat.com> wrote:
>> On Tue, Dec 12, 2017 at 11:09:39AM +0800, Chengguang Xu 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 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     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> tests/generic/471.out |   2 +
>>> tests/generic/group   |   1 +
>>> 3 files changed, 105 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..9d6c37a
>>> --- /dev/null
>>> +++ b/tests/generic/471
>>> @@ -0,0 +1,102 @@
>>> +#! /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=1
>>> +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
>> 
>> Need "_require_metadata_journaling $SCRATCH_DEV" after _scratch_mkfs, so
>> filesystems like no-journal mode ext4 or ext2 (driven by ext4 module)
>> won't run this test.
>> 
> 
> In that case, Chengguang would also have to teach _require_metadata_journaling
> about OVERLAY_BASE_SCRATCH_DEV....
> 
> Chengguang,
> 
> Sorry for all the extra work.
> That means you are making an important improvement to xfstests!

It’s worth to check all _require functions for overlayfs, but just in this case,
do we really need this check before test? IMO，if fs supports shutdown then should
run this test, am I wrong?


Thanks,
Chengguang.

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

[Amir Goldstein]

On Tue, Dec 12, 2017 at 12:05 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>
>> 在 2017年12月12日，下午5:56，Amir Goldstein <amir73il@gmail.com> 写道：
>>
>> On Tue, Dec 12, 2017 at 11:24 AM, Eryu Guan <eguan@redhat.com> wrote:
>>> On Tue, Dec 12, 2017 at 11:09:39AM +0800, Chengguang Xu 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 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     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> tests/generic/471.out |   2 +
>>>> tests/generic/group   |   1 +
>>>> 3 files changed, 105 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..9d6c37a
>>>> --- /dev/null
>>>> +++ b/tests/generic/471
>>>> @@ -0,0 +1,102 @@
>>>> +#! /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=1
>>>> +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
>>>
>>> Need "_require_metadata_journaling $SCRATCH_DEV" after _scratch_mkfs, so
>>> filesystems like no-journal mode ext4 or ext2 (driven by ext4 module)
>>> won't run this test.
>>>
>>
>> In that case, Chengguang would also have to teach _require_metadata_journaling
>> about OVERLAY_BASE_SCRATCH_DEV....
>>
>> Chengguang,
>>
>> Sorry for all the extra work.
>> That means you are making an important improvement to xfstests!
>
> It’s worth to check all _require functions for overlayfs, but just in this case,
> do we really need this check before test? IMO，if fs supports shutdown then should
> run this test, am I wrong?
>
>
Seems correct to me. unless I am missing something.

Amir.

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

[Eryu Guan]

On Tue, Dec 12, 2017 at 06:05:50PM +0800, Chengguang Xu wrote:
> > 
> > 在 2017年12月12日，下午5:56，Amir Goldstein <amir73il@gmail.com> 写道：
> > 
> > On Tue, Dec 12, 2017 at 11:24 AM, Eryu Guan <eguan@redhat.com> wrote:
> >> On Tue, Dec 12, 2017 at 11:09:39AM +0800, Chengguang Xu 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 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     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>> tests/generic/471.out |   2 +
> >>> tests/generic/group   |   1 +
> >>> 3 files changed, 105 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..9d6c37a
> >>> --- /dev/null
> >>> +++ b/tests/generic/471
> >>> @@ -0,0 +1,102 @@
> >>> +#! /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=1
> >>> +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
> >> 
> >> Need "_require_metadata_journaling $SCRATCH_DEV" after _scratch_mkfs, so
> >> filesystems like no-journal mode ext4 or ext2 (driven by ext4 module)
> >> won't run this test.
> >> 
> > 
> > In that case, Chengguang would also have to teach _require_metadata_journaling
> > about OVERLAY_BASE_SCRATCH_DEV....
> > 
> > Chengguang,
> > 
> > Sorry for all the extra work.
> > That means you are making an important improvement to xfstests!
> 
> It’s worth to check all _require functions for overlayfs, but just in this case,
> do we really need this check before test? IMO，if fs supports shutdown then should
> run this test, am I wrong?

I think you're right, I was looking at other shutdown tests for too long
and thought this one needed the jounal check too, sorry about that!

But as Amir suggested, other shutdown tests need overlay support in
_require_metadata_journaling, but I think that can be fixed in a
follow-up patch, as using ext2 or no-journal mode ext4 as the backing
filesystems of overlay is not a common setup, the possiblity of someone
hitting false positive is relative low.

Thanks,
Eryu

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

[Amir Goldstein]

On Tue, Dec 12, 2017 at 5:09 AM, Chengguang Xu <cgxu519@icloud.com> wrote:
> OverlayFS overrides SCRATCH_MNT variable to it's own,
> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
> when running shutdown on overlayfs.
>
> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
> ---

Maybe "check -overlay overrides SCRATCH_MNT...

Otherwise, looks good.

Amir.

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

[Eryu Guan]

On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote:
> OverlayFS overrides SCRATCH_MNT variable to it's own,
> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
> when running shutdown on overlayfs.
> 
> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
> ---
> 
> 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 | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 4c053a5..a5c4ace 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -382,6 +382,24 @@ _scratch_cycle_mount()
>      _scratch_mount "$opts"
>  }
>  
> +_scratch_shutdown()
> +{
> +	if [ $FSTYP = "overlay" ]; then
> +		# In lagacy overlay usage, it may specify directory as 

Trailing whitespace in above line.

> +		# 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
> +			echo -n "Ignore shutdown because "
> +			echo "$SCRATCH_DEV is not a block device"

Hmm, I think a '_fail "<message>"' would be more appropriate here, so
that test fails and exits immediately. Test should call
_require_scratch_shutdown first before calling _scratch_shutdown, if
not, it indicates a useage error of the helper, we need to prompt the
test writer to call _require_scratch_shutdown first in error message.

> +		else
> +			src/godown $* $OVL_BASE_SCRATCH_MNT
> +		fi
> +	else
> +		src/godown $* $SCRATCH_MNT
> +	fi
> +}
> +
>  _test_mount()
>  {
>      if [ "$FSTYP" == "overlay" ]; then
> @@ -2908,8 +2926,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 

Trailing whitespace in above line.

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

Sorry for not bringing this up earlier, but I think we need to convert
all existing direct callers of 'godown' to use this new helper,
otherwise tests will call godown on an overlay merged dir and result in
test failure as:

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

You can go through all tests in 'shutdown' group in generic dir. But I
noticed that not all tests in 'shutdown' group can be converted to
_scratch_shutdown straightforwardly, e.g. generic/042 and generic/050,
they require $SCRATCH_DEV to be a local device file (either block device
or char device (ubifs uses char device)) rather than a dir or an NFS
export, we need an additional _require rule in both of the tests, so
that they won't run on overlay:

_require_local_device $SCRATCH_DEV

Other tests in 'shutdown' group can be converted easily.

Thanks,
Eryu

>  
> -- 
> 1.8.3.1
> 

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

[Chengguang Xu]

> 
> 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道：
> 
> On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote:
>> OverlayFS overrides SCRATCH_MNT variable to it's own,
>> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
>> when running shutdown on overlayfs.
>> 
>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
>> ---
>> 
>> 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 | 37 +++++++++++++++++++++++++++++++++++--
>> 1 file changed, 35 insertions(+), 2 deletions(-)
>> 
>> diff --git a/common/rc b/common/rc
>> index 4c053a5..a5c4ace 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -382,6 +382,24 @@ _scratch_cycle_mount()
>>     _scratch_mount "$opts"
>> }
>> 
>> +_scratch_shutdown()
>> +{
>> +	if [ $FSTYP = "overlay" ]; then
>> +		# In lagacy overlay usage, it may specify directory as 
> 
> Trailing whitespace in above line.
> 
>> +		# 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
>> +			echo -n "Ignore shutdown because "
>> +			echo "$SCRATCH_DEV is not a block device"
> 
> Hmm, I think a '_fail "<message>"' would be more appropriate here, so
> that test fails and exits immediately. Test should call
> _require_scratch_shutdown first before calling _scratch_shutdown, if
> not, it indicates a useage error of the helper, we need to prompt the
> test writer to call _require_scratch_shutdown first in error message.
> 
>> +		else
>> +			src/godown $* $OVL_BASE_SCRATCH_MNT
>> +		fi
>> +	else
>> +		src/godown $* $SCRATCH_MNT
>> +	fi
>> +}
>> +
>> _test_mount()
>> {
>>     if [ "$FSTYP" == "overlay" ]; then
>> @@ -2908,8 +2926,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 
> 
> Trailing whitespace in above line.
> 
>> +			# 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
>> }
> 
> Sorry for not bringing this up earlier, but I think we need to convert
> all existing direct callers of 'godown' to use this new helper,
> otherwise tests will call godown on an overlay merged dir and result in
> test failure as:
> 
> +/root/xfstests/src/godown: error on xfsctl(GOINGDOWN) of "/mnt/scratch/ovl-mnt": Inappropriate ioctl for device
> 
> You can go through all tests in 'shutdown' group in generic dir. But I
> noticed that not all tests in 'shutdown' group can be converted to
> _scratch_shutdown straightforwardly, e.g. generic/042 and generic/050,
> they require $SCRATCH_DEV to be a local device file (either block device
> or char device (ubifs uses char device)) rather than a dir or an NFS
> export, we need an additional _require rule in both of the tests, so
> that they won't run on overlay:
> 
> _require_local_device $SCRATCH_DEV
> 
> Other tests in 'shutdown' group can be converted easily.

We should fix the improper godown calling for overlayfs in generic test cases,
but it’s not directly caused by this patch, so I suggest merging this patch
first, then we can make another patch to fix godown problem for overlayfs.
What do you think?

Thanks,
Chengguang.

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

[Eryu Guan]

On Tue, Dec 12, 2017 at 06:20:51PM +0800, Chengguang Xu wrote:
> > 
> > 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道：
> > 
> > On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote:
> >> OverlayFS overrides SCRATCH_MNT variable to it's own,
> >> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
> >> when running shutdown on overlayfs.
> >> 
> >> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
> >> ---
> >> 
> >> 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 | 37 +++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 35 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/common/rc b/common/rc
> >> index 4c053a5..a5c4ace 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -382,6 +382,24 @@ _scratch_cycle_mount()
> >>     _scratch_mount "$opts"
> >> }
> >> 
> >> +_scratch_shutdown()
> >> +{
> >> +	if [ $FSTYP = "overlay" ]; then
> >> +		# In lagacy overlay usage, it may specify directory as 
> > 
> > Trailing whitespace in above line.
> > 
> >> +		# 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
> >> +			echo -n "Ignore shutdown because "
> >> +			echo "$SCRATCH_DEV is not a block device"
> > 
> > Hmm, I think a '_fail "<message>"' would be more appropriate here, so
> > that test fails and exits immediately. Test should call
> > _require_scratch_shutdown first before calling _scratch_shutdown, if
> > not, it indicates a useage error of the helper, we need to prompt the
> > test writer to call _require_scratch_shutdown first in error message.
> > 
> >> +		else
> >> +			src/godown $* $OVL_BASE_SCRATCH_MNT
> >> +		fi
> >> +	else
> >> +		src/godown $* $SCRATCH_MNT
> >> +	fi
> >> +}
> >> +
> >> _test_mount()
> >> {
> >>     if [ "$FSTYP" == "overlay" ]; then
> >> @@ -2908,8 +2926,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 
> > 
> > Trailing whitespace in above line.
> > 
> >> +			# 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
> >> }
> > 
> > Sorry for not bringing this up earlier, but I think we need to convert
> > all existing direct callers of 'godown' to use this new helper,
> > otherwise tests will call godown on an overlay merged dir and result in
> > test failure as:
> > 
> > +/root/xfstests/src/godown: error on xfsctl(GOINGDOWN) of "/mnt/scratch/ovl-mnt": Inappropriate ioctl for device
> > 
> > You can go through all tests in 'shutdown' group in generic dir. But I
> > noticed that not all tests in 'shutdown' group can be converted to
> > _scratch_shutdown straightforwardly, e.g. generic/042 and generic/050,
> > they require $SCRATCH_DEV to be a local device file (either block device
> > or char device (ubifs uses char device)) rather than a dir or an NFS
> > export, we need an additional _require rule in both of the tests, so
> > that they won't run on overlay:
> > 
> > _require_local_device $SCRATCH_DEV
> > 
> > Other tests in 'shutdown' group can be converted easily.
> 
> We should fix the improper godown calling for overlayfs in generic test cases,
> but it’s not directly caused by this patch, so I suggest merging this patch

IMO, it's just this patch that causes these kind of false failures in
other generic shutdown tests. It's this patch that makes generic
shutdown tests running on overlay possible, thus causes false failures.

We should fix all these problems in one patch, otherwise anyone tests
overlay using xfs/ext4 (or other filesystems that support shutdown) as
backing filesystems could see these failures.  OTOH, the overlay support
in _require_metadata_journaling issue is a relative corner case, I think
it's safe to fix that in a separate patch.

Thanks,
Eryu

> first, then we can make another patch to fix godown problem for overlayfs.
> What do you think?
> 
> Thanks,
> Chengguang.
> 
> 
> 

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

[Chengguang Xu]

> 在 2017年12月12日，下午9:39，Eryu Guan <eguan@redhat.com> 写道：
> 
> On Tue, Dec 12, 2017 at 06:20:51PM +0800, Chengguang Xu wrote:
>>> 
>>> 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道：
>>> 
>>> On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote:
>>>> OverlayFS overrides SCRATCH_MNT variable to it's own,
>>>> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
>>>> when running shutdown on overlayfs.
>>>> 
>>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
>>>> ---
>>>> 
>>>> 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 | 37 +++++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 35 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/common/rc b/common/rc
>>>> index 4c053a5..a5c4ace 100644
>>>> --- a/common/rc
>>>> +++ b/common/rc
>>>> @@ -382,6 +382,24 @@ _scratch_cycle_mount()
>>>>    _scratch_mount "$opts"
>>>> }
>>>> 
>>>> +_scratch_shutdown()
>>>> +{
>>>> +	if [ $FSTYP = "overlay" ]; then
>>>> +		# In lagacy overlay usage, it may specify directory as 
>>> 
>>> Trailing whitespace in above line.
>>> 
>>>> +		# 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
>>>> +			echo -n "Ignore shutdown because "
>>>> +			echo "$SCRATCH_DEV is not a block device"
>>> 
>>> Hmm, I think a '_fail "<message>"' would be more appropriate here, so
>>> that test fails and exits immediately. Test should call
>>> _require_scratch_shutdown first before calling _scratch_shutdown, if
>>> not, it indicates a useage error of the helper, we need to prompt the
>>> test writer to call _require_scratch_shutdown first in error message.
>>> 
>>>> +		else
>>>> +			src/godown $* $OVL_BASE_SCRATCH_MNT
>>>> +		fi
>>>> +	else
>>>> +		src/godown $* $SCRATCH_MNT
>>>> +	fi
>>>> +}
>>>> +
>>>> _test_mount()
>>>> {
>>>>    if [ "$FSTYP" == "overlay" ]; then
>>>> @@ -2908,8 +2926,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 
>>> 
>>> Trailing whitespace in above line.
>>> 
>>>> +			# 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
>>>> }
>>> 
>>> Sorry for not bringing this up earlier, but I think we need to convert
>>> all existing direct callers of 'godown' to use this new helper,
>>> otherwise tests will call godown on an overlay merged dir and result in
>>> test failure as:
>>> 
>>> +/root/xfstests/src/godown: error on xfsctl(GOINGDOWN) of "/mnt/scratch/ovl-mnt": Inappropriate ioctl for device
>>> 
>>> You can go through all tests in 'shutdown' group in generic dir. But I
>>> noticed that not all tests in 'shutdown' group can be converted to
>>> _scratch_shutdown straightforwardly, e.g. generic/042 and generic/050,
>>> they require $SCRATCH_DEV to be a local device file (either block device
>>> or char device (ubifs uses char device)) rather than a dir or an NFS
>>> export, we need an additional _require rule in both of the tests, so
>>> that they won't run on overlay:
>>> 
>>> _require_local_device $SCRATCH_DEV
>>> 
>>> Other tests in 'shutdown' group can be converted easily.
>> 
>> We should fix the improper godown calling for overlayfs in generic test cases,
>> but it’s not directly caused by this patch, so I suggest merging this patch
> 
> IMO, it's just this patch that causes these kind of false failures in
> other generic shutdown tests. It's this patch that makes generic
> shutdown tests running on overlay possible, thus causes false failures.
> 
> We should fix all these problems in one patch, otherwise anyone tests
> overlay using xfs/ext4 (or other filesystems that support shutdown) as
> backing filesystems could see these failures.  OTOH, the overlay support
> in _require_metadata_journaling issue is a relative corner case, I think
> it's safe to fix that in a separate patch.

In order to add overlay support in some requirement checks like  _require_metadata_journaling, 
I think it’s better save underlying filesystem type to $OVL_BASE_FSTYP and doing proper check based
on it.

Currently $OVL_BASE_FSTYP and $FSTYP all set to “overlay”, Is there any specific reason for it? 



 


Thanks,
Chengguang.

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

[Amir Goldstein]

On Wed, Jan 3, 2018 at 2:44 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>
[...]
> In order to add overlay support in some requirement checks like  _require_metadata_journaling,
> I think it’s better save underlying filesystem type to $OVL_BASE_FSTYP and doing proper check based
> on it.
>
> Currently $OVL_BASE_FSTYP and $FSTYP all set to “overlay”, Is there any specific reason for it?
>

In _overlay_config_override():
        # Config file may specify base fs type, but we obay -overlay flag
        export OVL_BASE_FSTYP="$FSTYP"
        export FSTYP=overlay

So either your setup is wrong or there is a bug.
$OVL_BASE_FSTYP *should* contain the base fs type,
but only if you defined FSTYPE in your config file when running ./check -overlay

Cheers,
Amir.

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

[Chengguang Xu]

> 
> 在 2018年1月3日，下午8:58，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Wed, Jan 3, 2018 at 2:44 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>> 
> [...]
>> In order to add overlay support in some requirement checks like  _require_metadata_journaling,
>> I think it’s better save underlying filesystem type to $OVL_BASE_FSTYP and doing proper check based
>> on it.
>> 
>> Currently $OVL_BASE_FSTYP and $FSTYP all set to “overlay”, Is there any specific reason for it?
>> 
> 
> In _overlay_config_override():
>        # Config file may specify base fs type, but we obay -overlay flag
>        export OVL_BASE_FSTYP="$FSTYP"
>        export FSTYP=overlay
> 
> So either your setup is wrong or there is a bug.
> $OVL_BASE_FSTYP *should* contain the base fs type,
> but only if you defined FSTYPE in your config file when running ./check -overlay

Yeah, I didn’t specify by hand in config file and just supposed to detect automatically.
I didn’t check very carefully but seems just slight modification like below could let it 
support auto detection.


$ git diff
diff --git a/check b/check
index f8db3cd..e460424 100755
--- a/check
+++ b/check
@@ -265,7 +265,7 @@ while [ $# -gt 0 ]; do
        -nfs)           FSTYP=nfs ;;
        -glusterfs)     FSTYP=glusterfs ;;
        -cifs)          FSTYP=cifs ;;
-       -overlay)       FSTYP=overlay; export OVERLAY=true ;;
+       -overlay)       export OVERLAY=true ;;
        -pvfs2)         FSTYP=pvfs2 ;;
        -tmpfs)         FSTYP=tmpfs ;;
        -ubifs)         FSTYP=ubifs ;;
diff --git a/common/config b/common/config
index d0fbfe5..e21d8ee 100644
--- a/common/config
+++ b/common/config
@@ -530,7 +530,9 @@ _overlay_config_override()
        [ -b "$TEST_DEV" ] || return 0

        # Config file may specify base fs type, but we obay -overlay flag
-       export OVL_BASE_FSTYP="$FSTYP"
+       if [ -z $OVL_BASE_FSTYP ]; then
+               export OVL_BASE_FSTYP="$FSTYP"
+       fi
        export FSTYP=overlay

        # Store original base fs vars
@@ -691,13 +693,6 @@ get_next_config() {
                _check_device SCRATCH_LOGDEV optional $SCRATCH_LOGDEV
        fi

-       # Override FSTYP from config when running ./check -overlay
-       # and maybe override base fs TEST/SCRATCH_DEV with overlay base dirs.
-       # We need to do this *after* default mount options are set by base FSTYP
-       # and *after* SCRATCH_DEV is deduced from SCRATCH_DEV_POOL
-       if [ "$OVERLAY" == "true" -o "$FSTYP" == "overlay" ]; then
-               _overlay_config_override
-       fi
 }

 if [ -z "$CONFIG_INCLUDED" ]; then
@@ -711,6 +706,15 @@ if [ -z "$CONFIG_INCLUDED" ]; then
           [ ! -z "$TEST_DEV" ]; then
                FSTYP=`blkid -c /dev/null -s TYPE -o value $TEST_DEV`
        fi
+
+       # Override FSTYP from config when running ./check -overlay
+       # and maybe override base fs TEST/SCRATCH_DEV with overlay base dirs.
+       # We need to do this *after* default mount options are set by base FSTYP
+       # and *after* SCRATCH_DEV is deduced from SCRATCH_DEV_POOL
+       if [ "$OVERLAY" == "true" -o "$FSTYP" == "overlay" ]; then
+               _overlay_config_override
+       fi
+
        FSTYP=${FSTYP:=xfs}
        export FSTYP
        [ -z "$MOUNT_OPTIONS" ] && _mount_opts



Thanks,
Chengguang.

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

[Amir Goldstein]

On Wed, Jan 3, 2018 at 4:25 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>
>> 在 2018年1月3日，下午8:58，Amir Goldstein <amir73il@gmail.com> 写道：
>>
>> On Wed, Jan 3, 2018 at 2:44 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>>
>> [...]
>>> In order to add overlay support in some requirement checks like  _require_metadata_journaling,
>>> I think it’s better save underlying filesystem type to $OVL_BASE_FSTYP and doing proper check based
>>> on it.
>>>
>>> Currently $OVL_BASE_FSTYP and $FSTYP all set to “overlay”, Is there any specific reason for it?
>>>
>>
>> In _overlay_config_override():
>>        # Config file may specify base fs type, but we obay -overlay flag
>>        export OVL_BASE_FSTYP="$FSTYP"
>>        export FSTYP=overlay
>>
>> So either your setup is wrong or there is a bug.
>> $OVL_BASE_FSTYP *should* contain the base fs type,
>> but only if you defined FSTYPE in your config file when running ./check -overlay
>
> Yeah, I didn’t specify by hand in config file and just supposed to detect automatically.
> I didn’t check very carefully but seems just slight modification like below could let it
> support auto detection.
>

Sorry, I do not wish invest time to review this change because:
1. I don't see the need to support auto detect of base fs type (feel
free to explain)
2. I ran a lot of test to sanitize the new overlay config option with many
    configurations and this adds a lot more variants. I you wish to push this
    forward and have a good claim for the need, please specify which tests
    you ran to sanitize your change before requesting review.

If you wish to contribute to the xfstests overlay infrastructure there
are other features
that would bring more gain IMO, for example:
- Support mkfs and MKFS_OPTIONS for base fs
- Try to increase the number of generic tests that can be run with
basefs+overlay
  similar to your effort with the shutdown group tests

Thanks!
Amir.

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

[Chengguang Xu]

在 2018年1月3日，下午10:54，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Wed, Jan 3, 2018 at 4:25 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>> 
>>> 在 2018年1月3日，下午8:58，Amir Goldstein <amir73il@gmail.com> 写道：
>>> 
>>> On Wed, Jan 3, 2018 at 2:44 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>>> 
>>> [...]
>>>> In order to add overlay support in some requirement checks like  _require_metadata_journaling,
>>>> I think it’s better save underlying filesystem type to $OVL_BASE_FSTYP and doing proper check based
>>>> on it.
>>>> 
>>>> Currently $OVL_BASE_FSTYP and $FSTYP all set to “overlay”, Is there any specific reason for it?
>>>> 
>>> 
>>> In _overlay_config_override():
>>>       # Config file may specify base fs type, but we obay -overlay flag
>>>       export OVL_BASE_FSTYP="$FSTYP"
>>>       export FSTYP=overlay
>>> 
>>> So either your setup is wrong or there is a bug.
>>> $OVL_BASE_FSTYP *should* contain the base fs type,
>>> but only if you defined FSTYPE in your config file when running ./check -overlay
>> 
>> Yeah, I didn’t specify by hand in config file and just supposed to detect automatically.
>> I didn’t check very carefully but seems just slight modification like below could let it
>> support auto detection.
>> 
> 
> Sorry, I do not wish invest time to review this change because:
> 1. I don't see the need to support auto detect of base fs type (feel
> free to explain)

The reason of supporting auto detection maybe is the same as xfstests for local filesystem.
For some people who need to frequently change testing filesystem, they would prefer to
automatically recognize testing filesystem than modifying config file again and again.

> 2. I ran a lot of test to sanitize the new overlay config option with many
>    configurations and this adds a lot more variants. I you wish to push this
>    forward and have a good claim for the need, please specify which tests
>    you ran to sanitize your change before requesting review.
> 
> If you wish to contribute to the xfstests overlay infrastructure there
> are other features
> that would bring more gain IMO, for example:
> - Support mkfs and MKFS_OPTIONS for base fs
> - Try to increase the number of generic tests that can be run with
> basefs+overlay
>  similar to your effort with the shutdown group tests

OK，let’s do these first.

Thanks,
Chengguang.

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

[Chengguang Xu]

> 
> 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道：
> 
> On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote:
>> OverlayFS overrides SCRATCH_MNT variable to it's own,
>> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
>> when running shutdown on overlayfs.
>> 
>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
>> ---
>> 
>> 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 | 37 +++++++++++++++++++++++++++++++++++--
>> 1 file changed, 35 insertions(+), 2 deletions(-)
>> 
>> diff --git a/common/rc b/common/rc
>> index 4c053a5..a5c4ace 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -382,6 +382,24 @@ _scratch_cycle_mount()
>>     _scratch_mount "$opts"
>> }
>> 
>> +_scratch_shutdown()
>> +{
>> +	if [ $FSTYP = "overlay" ]; then
>> +		# In lagacy overlay usage, it may specify directory as 
> 
> Trailing whitespace in above line.
> 
>> +		# 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
>> +			echo -n "Ignore shutdown because "
>> +			echo "$SCRATCH_DEV is not a block device"
> 
> Hmm, I think a '_fail "<message>"' would be more appropriate here, so
> that test fails and exits immediately. Test should call
> _require_scratch_shutdown first before calling _scratch_shutdown, if
> not, it indicates a useage error of the helper, we need to prompt the
> test writer to call _require_scratch_shutdown first in error message.


The additional check I added here, is mainly for avoiding accident improper shutdown.
If you think we need to prompt message to indicate should call _require_scratch_shutdown first,
I think we had better modify godown to implement this function. What do you think?

Thanks,
Chengguang.

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

[Eryu Guan]

On Thu, Dec 14, 2017 at 11:35:04AM +0800, Chengguang Xu wrote:
> 
> > 
> > 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道：
> > 
> > On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote:
> >> OverlayFS overrides SCRATCH_MNT variable to it's own,
> >> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
> >> when running shutdown on overlayfs.
> >> 
> >> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
> >> ---
> >> 
> >> 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 | 37 +++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 35 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/common/rc b/common/rc
> >> index 4c053a5..a5c4ace 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -382,6 +382,24 @@ _scratch_cycle_mount()
> >>     _scratch_mount "$opts"
> >> }
> >> 
> >> +_scratch_shutdown()
> >> +{
> >> +	if [ $FSTYP = "overlay" ]; then
> >> +		# In lagacy overlay usage, it may specify directory as 
> > 
> > Trailing whitespace in above line.
> > 
> >> +		# 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
> >> +			echo -n "Ignore shutdown because "
> >> +			echo "$SCRATCH_DEV is not a block device"
> > 
> > Hmm, I think a '_fail "<message>"' would be more appropriate here, so
> > that test fails and exits immediately. Test should call
> > _require_scratch_shutdown first before calling _scratch_shutdown, if
> > not, it indicates a useage error of the helper, we need to prompt the
> > test writer to call _require_scratch_shutdown first in error message.
> 
> 
> The additional check I added here, is mainly for avoiding accident improper shutdown.

Yeah, I understand, and the _fail call I suggested is to exit the test
immediately with a proper error message when such an accident usage is
detected, while current code just prints a message and continues the
rest of the test, which is unnecessary.

> If you think we need to prompt message to indicate should call _require_scratch_shutdown first,
> I think we had better modify godown to implement this function. What do you think?

I just want to see a more specific & clearer error message on such a
usage failure, so people know what to do to correct this error. Similar
to what we do in common/rc::_require_command (_fail "usage:
_require_command <command> [<name>]").

Thanks,
Eryu

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

[Chengguang Xu]

> 
> 在 2017年12月14日，下午12:32，Eryu Guan <eguan@redhat.com> 写道：
> 
> On Thu, Dec 14, 2017 at 11:35:04AM +0800, Chengguang Xu wrote:
>> 
>>> 
>>> 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道：
>>> 
>>> On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote:
>>>> OverlayFS overrides SCRATCH_MNT variable to it's own,
>>>> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT)
>>>> when running shutdown on overlayfs.
>>>> 
>>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
>>>> ---
>>>> 
>>>> 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 | 37 +++++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 35 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/common/rc b/common/rc
>>>> index 4c053a5..a5c4ace 100644
>>>> --- a/common/rc
>>>> +++ b/common/rc
>>>> @@ -382,6 +382,24 @@ _scratch_cycle_mount()
>>>>    _scratch_mount "$opts"
>>>> }
>>>> 
>>>> +_scratch_shutdown()
>>>> +{
>>>> +	if [ $FSTYP = "overlay" ]; then
>>>> +		# In lagacy overlay usage, it may specify directory as 
>>> 
>>> Trailing whitespace in above line.
>>> 
>>>> +		# 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
>>>> +			echo -n "Ignore shutdown because "
>>>> +			echo "$SCRATCH_DEV is not a block device"
>>> 
>>> Hmm, I think a '_fail "<message>"' would be more appropriate here, so
>>> that test fails and exits immediately. Test should call
>>> _require_scratch_shutdown first before calling _scratch_shutdown, if
>>> not, it indicates a useage error of the helper, we need to prompt the
>>> test writer to call _require_scratch_shutdown first in error message.
>> 
>> 
>> The additional check I added here, is mainly for avoiding accident improper shutdown.
> 
> Yeah, I understand, and the _fail call I suggested is to exit the test
> immediately with a proper error message when such an accident usage is
> detected, while current code just prints a message and continues the
> rest of the test, which is unnecessary.
> 
>> If you think we need to prompt message to indicate should call _require_scratch_shutdown first,
>> I think we had better modify godown to implement this function. What do you think?
> 
> I just want to see a more specific & clearer error message on such a
> usage failure, so people know what to do to correct this error. Similar
> to what we do in common/rc::_require_command (_fail "usage:
> _require_command <command> [<name>]”).

I agree exit the test immediately if you get into this condition. 
But for error message let me keep it as what it is, because the 
reason of the failure is actually config problem not improper 
command usage.

Thanks,
Chengguang.