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

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

[Chengguang Xu]

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

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

 common/rc | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 4c053a5..911dba3 100644
--- a/common/rc
+++ b/common/rc
@@ -382,6 +382,15 @@ _scratch_cycle_mount()
     _scratch_mount "$opts"
 }
 
+_scratch_shutdown()
+{
+	if [ $FSTYP = "overlay" ]; then
+		src/godown -f $OVL_BASE_SCRATCH_MNT
+	else
+		src/godown -f $SCRATCH_MNT
+	fi
+}
+
 _test_mount()
 {
     if [ "$FSTYP" == "overlay" ]; then
@@ -2908,8 +2917,19 @@ _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
+			_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 v2 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 911dba3..4827ba6 100644
--- a/common/rc
+++ b/common/rc
@@ -2096,6 +2096,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 v2 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 v1:
Use fs shutdown and fssum to check syncfs result instead of
checking delalloc state of extents.

 tests/generic/470     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/470.out |  2 ++
 tests/generic/group   |  1 +
 3 files changed, 91 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..b488747
--- /dev/null
+++ b/tests/generic/470
@@ -0,0 +1,88 @@
+#! /bin/bash
+# FS QA Test 470
+#
+# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
+# underlying filesystem. 
+#
+# 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_test
+_require_fssum
+_require_scratch
+_require_scratch_shutdown
+_require_xfs_io_command "syncfs"
+
+
+FCNT=1000
+
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+# In order to mitigate interference of write-back,
+# create many files for test.
+for i in `seq 1 $FCNT`; do
+	$XFS_IO_PROG -f -c "pwrite 1K 1M" \
+		$SCRATCH_MNT/$i >/dev/null 2>&1
+done
+
+$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/before_test $SCRATCH_MNT
+$XFS_IO_PROG -c "syncfs" $SCRATCH_MNT/${FCNT} >/dev/null 2>&1
+_scratch_shutdown
+_scratch_cycle_mount
+$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/after_test $SCRATCH_MNT
+diff $TEST_DIR/before_test $TEST_DIR/after_test
+if [ $? -eq 0 ]; then
+	status=0
+fi
+
+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 v2 3/3] generic/470: add syncfs test

[Amir Goldstein]

On Thu, Dec 7, 2017 at 4:22 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 v1:
> Use fs shutdown and fssum to check syncfs result instead of
> checking delalloc state of extents.
>
>  tests/generic/470     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/470.out |  2 ++
>  tests/generic/group   |  1 +
>  3 files changed, 91 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..b488747
> --- /dev/null
> +++ b/tests/generic/470
> @@ -0,0 +1,88 @@
> +#! /bin/bash
> +# FS QA Test 470
> +#
> +# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
> +# underlying filesystem.
> +#
> +# 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_test
> +_require_fssum
> +_require_scratch
> +_require_scratch_shutdown
> +_require_xfs_io_command "syncfs"
> +
> +
> +FCNT=1000
> +
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +# In order to mitigate interference of write-back,
> +# create many files for test.
> +for i in `seq 1 $FCNT`; do
> +       $XFS_IO_PROG -f -c "pwrite 1K 1M" \
> +               $SCRATCH_MNT/$i >/dev/null 2>&1
> +done
> +
> +$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/before_test $SCRATCH_MNT
> +$XFS_IO_PROG -c "syncfs" $SCRATCH_MNT/${FCNT} >/dev/null 2>&1
> +_scratch_shutdown
> +_scratch_cycle_mount
> +$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/after_test $SCRATCH_MNT
> +diff $TEST_DIR/before_test $TEST_DIR/after_test

Just FYI, md5sum can take multiple files so this can be written as:

md5sum $SCRATCH_MNT/$i > $TEST_DIR/before_test
...
md5sum -c $TEST_DIR/before_test > /dev/null 2>&1

Which reads a lot simpler to me.  At least I don't know what 'ugomAcdES'
stands for and for this simple use case I probably don't need to know.

Cheers,
Amir.

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

[Chengguang Xu]

> 
> 在 2017年12月7日，上午11:04，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Thu, Dec 7, 2017 at 4:22 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 v1:
>> Use fs shutdown and fssum to check syncfs result instead of
>> checking delalloc state of extents.
>> 
>> tests/generic/470     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> tests/generic/470.out |  2 ++
>> tests/generic/group   |  1 +
>> 3 files changed, 91 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..b488747
>> --- /dev/null
>> +++ b/tests/generic/470
>> @@ -0,0 +1,88 @@
>> +#! /bin/bash
>> +# FS QA Test 470
>> +#
>> +# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
>> +# underlying filesystem.
>> +#
>> +# 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_test
>> +_require_fssum
>> +_require_scratch
>> +_require_scratch_shutdown
>> +_require_xfs_io_command "syncfs"
>> +
>> +
>> +FCNT=1000
>> +
>> +_scratch_mkfs >/dev/null 2>&1
>> +_scratch_mount
>> +
>> +# In order to mitigate interference of write-back,
>> +# create many files for test.
>> +for i in `seq 1 $FCNT`; do
>> +       $XFS_IO_PROG -f -c "pwrite 1K 1M" \
>> +               $SCRATCH_MNT/$i >/dev/null 2>&1
>> +done
>> +
>> +$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/before_test $SCRATCH_MNT
>> +$XFS_IO_PROG -c "syncfs" $SCRATCH_MNT/${FCNT} >/dev/null 2>&1
>> +_scratch_shutdown
>> +_scratch_cycle_mount
>> +$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/after_test $SCRATCH_MNT
>> +diff $TEST_DIR/before_test $TEST_DIR/after_test
> 
> Just FYI, md5sum can take multiple files so this can be written as:
> 
> md5sum $SCRATCH_MNT/$i > $TEST_DIR/before_test
> ...
> md5sum -c $TEST_DIR/before_test > /dev/null 2>&1
> 
> Which reads a lot simpler to me.  At least I don't know what 'ugomAcdES'
> stands for and for this simple use case I probably don't need to know.
> 

Hmm, fssum seems better than md5sum for metadata check, as it’s usage shows below.
If short comment of fssum option is acceptable, I prefer to keep it. How do you think? 


$ ./fssum  -h
usage: fssum <options> <path>
  options:
    -f          : write out a full manifest file
    -w <file>   : send output to file
    -v          : verbose mode (debugging only)
    -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
    -n          : reset all flags
    -N          : set all flags
    -x path     : exclude path when building checksum (multiple ok)
    -h          : this help

The default field mask is ugoamCdES. If the checksum/manifest is read from a
file, the mask is taken from there and the values given on the command line
are ignored.


Thanks,
-cgxu

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

[Amir Goldstein]

On Thu, Dec 7, 2017 at 5:31 AM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>
>> 在 2017年12月7日，上午11:04，Amir Goldstein <amir73il@gmail.com> 写道：
>>
>> On Thu, Dec 7, 2017 at 4:22 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 v1:
>>> Use fs shutdown and fssum to check syncfs result instead of
>>> checking delalloc state of extents.
>>>
>>> tests/generic/470     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>> tests/generic/470.out |  2 ++
>>> tests/generic/group   |  1 +
>>> 3 files changed, 91 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..b488747
>>> --- /dev/null
>>> +++ b/tests/generic/470
>>> @@ -0,0 +1,88 @@
>>> +#! /bin/bash
>>> +# FS QA Test 470
>>> +#
>>> +# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
>>> +# underlying filesystem.
>>> +#
>>> +# 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_test
>>> +_require_fssum
>>> +_require_scratch
>>> +_require_scratch_shutdown
>>> +_require_xfs_io_command "syncfs"
>>> +
>>> +
>>> +FCNT=1000
>>> +
>>> +_scratch_mkfs >/dev/null 2>&1
>>> +_scratch_mount
>>> +
>>> +# In order to mitigate interference of write-back,
>>> +# create many files for test.
>>> +for i in `seq 1 $FCNT`; do
>>> +       $XFS_IO_PROG -f -c "pwrite 1K 1M" \
>>> +               $SCRATCH_MNT/$i >/dev/null 2>&1
>>> +done
>>> +
>>> +$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/before_test $SCRATCH_MNT
>>> +$XFS_IO_PROG -c "syncfs" $SCRATCH_MNT/${FCNT} >/dev/null 2>&1
>>> +_scratch_shutdown
>>> +_scratch_cycle_mount
>>> +$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/after_test $SCRATCH_MNT
>>> +diff $TEST_DIR/before_test $TEST_DIR/after_test
>>
>> Just FYI, md5sum can take multiple files so this can be written as:
>>
>> md5sum $SCRATCH_MNT/$i > $TEST_DIR/before_test
>> ...
>> md5sum -c $TEST_DIR/before_test > /dev/null 2>&1
>>
>> Which reads a lot simpler to me.  At least I don't know what 'ugomAcdES'
>> stands for and for this simple use case I probably don't need to know.
>>
>
> Hmm, fssum seems better than md5sum for metadata check, as it’s usage shows below.
> If short comment of fssum option is acceptable, I prefer to keep it. How do you think?
>

Short comment would be great.
Thanks,
Amir.

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

[Eryu Guan]

On Thu, Dec 07, 2017 at 10:22:07AM +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 v1:
> Use fs shutdown and fssum to check syncfs result instead of
> checking delalloc state of extents.
> 
>  tests/generic/470     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/470.out |  2 ++
>  tests/generic/group   |  1 +
>  3 files changed, 91 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..b488747
> --- /dev/null
> +++ b/tests/generic/470
> @@ -0,0 +1,88 @@
> +#! /bin/bash
> +# FS QA Test 470
> +#
> +# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
> +# underlying filesystem. 

Trailing whitespace in above line.

> +#
> +# 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_test
> +_require_fssum
> +_require_scratch
> +_require_scratch_shutdown
> +_require_xfs_io_command "syncfs"
> +
> +
> +FCNT=1000
> +
> +_scratch_mkfs >/dev/null 2>&1
> +_scratch_mount
> +
> +# In order to mitigate interference of write-back,
> +# create many files for test.

Sorry, I still don't understand how writeback could interfere this test
from this comment, what happens if we don't create such files? Why
writing files starting from offset 1k?

> +for i in `seq 1 $FCNT`; do
> +	$XFS_IO_PROG -f -c "pwrite 1K 1M" \
> +		$SCRATCH_MNT/$i >/dev/null 2>&1
> +done
> +
> +$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/before_test $SCRATCH_MNT

$tmp.fssum looks a better choice of the fssum record file, then you
don't need _require_test anymore.

> +$XFS_IO_PROG -c "syncfs" $SCRATCH_MNT/${FCNT} >/dev/null 2>&1
> +_scratch_shutdown
> +_scratch_cycle_mount
> +$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/after_test $SCRATCH_MNT
> +diff $TEST_DIR/before_test $TEST_DIR/after_test
> +if [ $? -eq 0 ]; then
> +	status=0
> +fi

No need to check return value of diff here, just set status=0
unconditionally, test will break the golden image matching anyway if
diff finds something different.

But why not use "-r" option of fssum to read and compare the checksums?
e.g.

$FSSUM_PROG -r $tmp.fssum $SCRATCH_MNT

Thanks,
Eryu

> +
> +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 v2 3/3] generic/470: add syncfs test

[Chengguang Xu]

> 
> 在 2017年12月7日，下午1:44，Eryu Guan <eguan@redhat.com> 写道：
> 
> On Thu, Dec 07, 2017 at 10:22:07AM +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 v1:
>> Use fs shutdown and fssum to check syncfs result instead of
>> checking delalloc state of extents.
>> 
>> tests/generic/470     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> tests/generic/470.out |  2 ++
>> tests/generic/group   |  1 +
>> 3 files changed, 91 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..b488747
>> --- /dev/null
>> +++ b/tests/generic/470
>> @@ -0,0 +1,88 @@
>> +#! /bin/bash
>> +# FS QA Test 470
>> +#
>> +# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
>> +# underlying filesystem. 
> 
> Trailing whitespace in above line.
> 
>> +#
>> +# 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_test
>> +_require_fssum
>> +_require_scratch
>> +_require_scratch_shutdown
>> +_require_xfs_io_command "syncfs"
>> +
>> +
>> +FCNT=1000
>> +
>> +_scratch_mkfs >/dev/null 2>&1
>> +_scratch_mount
>> +
>> +# In order to mitigate interference of write-back,
>> +# create many files for test.
> 
> Sorry, I still don't understand how writeback could interfere this test
> from this comment, what happens if we don't create such files? Why
> writing files starting from offset 1k?

There is no explicit explanation how writeback interferes this case,
also there are many triggers make writeback starts syncing work.
I just want to increase hit ratio of failure by make many test files,
as many as possible, but it’s also limited by time and other resource.

The reason of offset 1k is same as above, compare to test a normal file, 
I think file with hole can increase failure ratio sometimes.

> 
>> +for i in `seq 1 $FCNT`; do
>> +	$XFS_IO_PROG -f -c "pwrite 1K 1M" \
>> +		$SCRATCH_MNT/$i >/dev/null 2>&1
>> +done
>> +
>> +$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/before_test $SCRATCH_MNT
> 
> $tmp.fssum looks a better choice of the fssum record file, then you
> don't need _require_test anymore.
> 
>> +$XFS_IO_PROG -c "syncfs" $SCRATCH_MNT/${FCNT} >/dev/null 2>&1
>> +_scratch_shutdown
>> +_scratch_cycle_mount
>> +$FSSUM_PROG -ugomAcdES -f -w $TEST_DIR/after_test $SCRATCH_MNT
>> +diff $TEST_DIR/before_test $TEST_DIR/after_test
>> +if [ $? -eq 0 ]; then
>> +	status=0
>> +fi
> 
> No need to check return value of diff here, just set status=0
> unconditionally, test will break the golden image matching anyway if
> diff finds something different.
> 
> But why not use "-r" option of fssum to read and compare the checksums?
> e.g.
> 
> $FSSUM_PROG -r $tmp.fssum $SCRATCH_MNT
> 
> Thanks,
> Eryu
> 
>> +
>> +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 v2 3/3] generic/470: add syncfs test

[Eryu Guan]

On Thu, Dec 07, 2017 at 02:20:26PM +0800, Chengguang Xu wrote:
> > 
> > 在 2017年12月7日，下午1:44，Eryu Guan <eguan@redhat.com> 写道：
> > 
> > On Thu, Dec 07, 2017 at 10:22:07AM +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 v1:
> >> Use fs shutdown and fssum to check syncfs result instead of
> >> checking delalloc state of extents.
> >> 
> >> tests/generic/470     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> tests/generic/470.out |  2 ++
> >> tests/generic/group   |  1 +
> >> 3 files changed, 91 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..b488747
> >> --- /dev/null
> >> +++ b/tests/generic/470
> >> @@ -0,0 +1,88 @@
> >> +#! /bin/bash
> >> +# FS QA Test 470
> >> +#
> >> +# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
> >> +# underlying filesystem. 
> > 
> > Trailing whitespace in above line.
> > 
> >> +#
> >> +# 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_test
> >> +_require_fssum
> >> +_require_scratch
> >> +_require_scratch_shutdown
> >> +_require_xfs_io_command "syncfs"
> >> +
> >> +
> >> +FCNT=1000
> >> +
> >> +_scratch_mkfs >/dev/null 2>&1
> >> +_scratch_mount
> >> +
> >> +# In order to mitigate interference of write-back,
> >> +# create many files for test.
> > 
> > Sorry, I still don't understand how writeback could interfere this test
> > from this comment, what happens if we don't create such files? Why
> > writing files starting from offset 1k?
> 
> There is no explicit explanation how writeback interferes this case,
> also there are many triggers make writeback starts syncing work.
> I just want to increase hit ratio of failure by make many test files,
> as many as possible, but it’s also limited by time and other resource.
> 
> The reason of offset 1k is same as above, compare to test a normal file, 
> I think file with hole can increase failure ratio sometimes.

Yeah, increasing the reproducibility would be a good reason too. Do you
happen to tune the number of files to see if 1000 is a good fit? e.g.
with 100 files test reproduced the overlay bug 20% of times, with 1000
files the reproducibility increased to 80%, etc. And the hole in the
beginning too, what's the actual impact on the reproducibility?

And you're right about the test time, usually we want to balance between
test time and reproducibility too, so we need to tune and measure the
numbers like test files, loop counts etc.

I think these are all good comments for test :)

Thanks,
Eryu

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

[Chengguang Xu]

在 2017年12月7日，下午3:13，Eryu Guan <eguan@redhat.com> 写道：
> 
> On Thu, Dec 07, 2017 at 02:20:26PM +0800, Chengguang Xu wrote:
>>> 
>>> 在 2017年12月7日，下午1:44，Eryu Guan <eguan@redhat.com> 写道：
>>> 
>>> On Thu, Dec 07, 2017 at 10:22:07AM +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 v1:
>>>> Use fs shutdown and fssum to check syncfs result instead of
>>>> checking delalloc state of extents.
>>>> 
>>>> tests/generic/470     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> tests/generic/470.out |  2 ++
>>>> tests/generic/group   |  1 +
>>>> 3 files changed, 91 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..b488747
>>>> --- /dev/null
>>>> +++ b/tests/generic/470
>>>> @@ -0,0 +1,88 @@
>>>> +#! /bin/bash
>>>> +# FS QA Test 470
>>>> +#
>>>> +# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
>>>> +# underlying filesystem. 
>>> 
>>> Trailing whitespace in above line.
>>> 
>>>> +#
>>>> +# 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_test
>>>> +_require_fssum
>>>> +_require_scratch
>>>> +_require_scratch_shutdown
>>>> +_require_xfs_io_command "syncfs"
>>>> +
>>>> +
>>>> +FCNT=1000
>>>> +
>>>> +_scratch_mkfs >/dev/null 2>&1
>>>> +_scratch_mount
>>>> +
>>>> +# In order to mitigate interference of write-back,
>>>> +# create many files for test.
>>> 
>>> Sorry, I still don't understand how writeback could interfere this test
>>> from this comment, what happens if we don't create such files? Why
>>> writing files starting from offset 1k?
>> 
>> There is no explicit explanation how writeback interferes this case,
>> also there are many triggers make writeback starts syncing work.
>> I just want to increase hit ratio of failure by make many test files,
>> as many as possible, but it’s also limited by time and other resource.
>> 
>> The reason of offset 1k is same as above, compare to test a normal file, 
>> I think file with hole can increase failure ratio sometimes.
> 
> Yeah, increasing the reproducibility would be a good reason too. Do you
> happen to tune the number of files to see if 1000 is a good fit? e.g.
> with 100 files test reproduced the overlay bug 20% of times, with 1000
> files the reproducibility increased to 80%, etc. And the hole in the
> beginning too, what's the actual impact on the reproducibility?
> 
> And you're right about the test time, usually we want to balance between
> test time and reproducibility too, so we need to tune and measure the
> numbers like test files, loop counts etc.
> 
> I think these are all good comments for test :)

I didn’t do much accurate testing about reproducibility, and also don’t have
a plan to do that. Actually in my testing ENV, the BUG is always reproducible
even for only a few of test files. 1000 is just my definition for
quite many files, and it’s really hard and meaningless to guess what number is 
best suit for variety of testing ENVs. So if you think the number 1000 is 
improper number, then I can modify it to right number that you think. Hole is
the same.

> 
> Thanks,
> Eryu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Amir Goldstein]

On Thu, Dec 7, 2017 at 9:42 AM, Chengguang Xu <cgxu519@icloud.com> wrote:
> 在 2017年12月7日，下午3:13，Eryu Guan <eguan@redhat.com> 写道：
>>
>> On Thu, Dec 07, 2017 at 02:20:26PM +0800, Chengguang Xu wrote:
>>>>
>>>> 在 2017年12月7日，下午1:44，Eryu Guan <eguan@redhat.com> 写道：
>>>>
>>>> On Thu, Dec 07, 2017 at 10:22:07AM +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 v1:
>>>>> Use fs shutdown and fssum to check syncfs result instead of
>>>>> checking delalloc state of extents.
>>>>>
>>>>> tests/generic/470     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> tests/generic/470.out |  2 ++
>>>>> tests/generic/group   |  1 +
>>>>> 3 files changed, 91 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..b488747
>>>>> --- /dev/null
>>>>> +++ b/tests/generic/470
>>>>> @@ -0,0 +1,88 @@
>>>>> +#! /bin/bash
>>>>> +# FS QA Test 470
>>>>> +#
>>>>> +# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
>>>>> +# underlying filesystem.
>>>>
>>>> Trailing whitespace in above line.
>>>>
>>>>> +#
>>>>> +# 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_test
>>>>> +_require_fssum
>>>>> +_require_scratch
>>>>> +_require_scratch_shutdown
>>>>> +_require_xfs_io_command "syncfs"
>>>>> +
>>>>> +
>>>>> +FCNT=1000
>>>>> +
>>>>> +_scratch_mkfs >/dev/null 2>&1
>>>>> +_scratch_mount
>>>>> +
>>>>> +# In order to mitigate interference of write-back,
>>>>> +# create many files for test.
>>>>
>>>> Sorry, I still don't understand how writeback could interfere this test
>>>> from this comment, what happens if we don't create such files? Why
>>>> writing files starting from offset 1k?
>>>
>>> There is no explicit explanation how writeback interferes this case,
>>> also there are many triggers make writeback starts syncing work.
>>> I just want to increase hit ratio of failure by make many test files,
>>> as many as possible, but it’s also limited by time and other resource.
>>>
>>> The reason of offset 1k is same as above, compare to test a normal file,
>>> I think file with hole can increase failure ratio sometimes.
>>
>> Yeah, increasing the reproducibility would be a good reason too. Do you
>> happen to tune the number of files to see if 1000 is a good fit? e.g.
>> with 100 files test reproduced the overlay bug 20% of times, with 1000
>> files the reproducibility increased to 80%, etc. And the hole in the
>> beginning too, what's the actual impact on the reproducibility?
>>
>> And you're right about the test time, usually we want to balance between
>> test time and reproducibility too, so we need to tune and measure the
>> numbers like test files, loop counts etc.
>>
>> I think these are all good comments for test :)
>
> I didn’t do much accurate testing about reproducibility, and also don’t have
> a plan to do that. Actually in my testing ENV, the BUG is always reproducible
> even for only a few of test files.

What is the underlying fs you are testing with?

> 1000 is just my definition for
> quite many files, and it’s really hard and meaningless to guess what number is
> best suit for variety of testing ENVs. So if you think the number 1000 is
> improper number, then I can modify it to right number that you think. Hole is
> the same.
>

I agree with Eryu that you should not invent numbers, unless test completes
in a few seconds and reproduces reliably - then you can invent numbers...

But this got me thinking about the details of overlayfs syncfs bug.
Overlayfs syncfs *will* actually call underlying fs syncfs (I fixed that)
but *will not* flush dirty inode data. What that means depends on
the underlying fs. For ext4 with default jounal=ordered, syncfs will
commit uncommitted journal transactions to disk, that will force data writeback
for all inodes, whose *metadata* is modified in uncommitted transactions.

Ted, please correct me if I am wrong.

For xfs, situation is a bit different, create operations are also delayed,
so your test could fail on overlayfs over xfs more easily.

I think that means that if you create the files and write them in the same
transaction, syncfs *will* actually sync on inodes data, so it is anyway
only the very last files that you write that won't be flushed no matter how
many files you will write.
What you should try to do to increase the changes of the bug on more fs:
- create new files and truncate them to final size but leaving them sparse
- sync
- buffered write to all files
- syncfs
- shutdown
- cycle_mount

The rules for choosing the right amount of files/data should be:
- After first sync, all inodes in the system is not dirty
- During the time it takes to write all files, flusher thread may kick in
  (default 30 seconds), flush data of your tests files and interfere with
  the test. Lets rule out another user doing sync, because most test
  machines are VMs that just run the tests
- If one loop iteration of the test above takes less than X second for a
  valid fs on a slow disk, then if flusher thread does interfere, its
  interference will be over in less then X second
  (because we are the only ones dirtying data on the system).
- If X < 10 seconds, then running 2 or 3 loops of the iteration should
  be enough to guaranty that we run at least one iteration without
  interference.

So I think if you actually write as little files and data as possible,
but loop several times.
IMO, tune the parameters, so an iteration takes ~1 second on slow disk
and run 3 iterations.
I have a test setup with spinning disk I can run your test on xfs/ext4 if
you like more samples then your own test environment.

And please write comments about those heuristics...

Thanks,
Amir.

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

[Chengguang Xu]

> 
> 在 2017年12月7日，下午4:17，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Thu, Dec 7, 2017 at 9:42 AM, Chengguang Xu <cgxu519@icloud.com> wrote:
>> 在 2017年12月7日，下午3:13，Eryu Guan <eguan@redhat.com> 写道：
>>> 
>>> On Thu, Dec 07, 2017 at 02:20:26PM +0800, Chengguang Xu wrote:
>>>>> 
>>>>> 在 2017年12月7日，下午1:44，Eryu Guan <eguan@redhat.com> 写道：
>>>>> 
>>>>> On Thu, Dec 07, 2017 at 10:22:07AM +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 v1:
>>>>>> Use fs shutdown and fssum to check syncfs result instead of
>>>>>> checking delalloc state of extents.
>>>>>> 
>>>>>> tests/generic/470     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> tests/generic/470.out |  2 ++
>>>>>> tests/generic/group   |  1 +
>>>>>> 3 files changed, 91 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..b488747
>>>>>> --- /dev/null
>>>>>> +++ b/tests/generic/470
>>>>>> @@ -0,0 +1,88 @@
>>>>>> +#! /bin/bash
>>>>>> +# FS QA Test 470
>>>>>> +#
>>>>>> +# Inspired by syncfs bug of overlayfs which does not sync dirty inodes in
>>>>>> +# underlying filesystem.
>>>>> 
>>>>> Trailing whitespace in above line.
>>>>> 
>>>>>> +#
>>>>>> +# 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_test
>>>>>> +_require_fssum
>>>>>> +_require_scratch
>>>>>> +_require_scratch_shutdown
>>>>>> +_require_xfs_io_command "syncfs"
>>>>>> +
>>>>>> +
>>>>>> +FCNT=1000
>>>>>> +
>>>>>> +_scratch_mkfs >/dev/null 2>&1
>>>>>> +_scratch_mount
>>>>>> +
>>>>>> +# In order to mitigate interference of write-back,
>>>>>> +# create many files for test.
>>>>> 
>>>>> Sorry, I still don't understand how writeback could interfere this test
>>>>> from this comment, what happens if we don't create such files? Why
>>>>> writing files starting from offset 1k?
>>>> 
>>>> There is no explicit explanation how writeback interferes this case,
>>>> also there are many triggers make writeback starts syncing work.
>>>> I just want to increase hit ratio of failure by make many test files,
>>>> as many as possible, but it’s also limited by time and other resource.
>>>> 
>>>> The reason of offset 1k is same as above, compare to test a normal file,
>>>> I think file with hole can increase failure ratio sometimes.
>>> 
>>> Yeah, increasing the reproducibility would be a good reason too. Do you
>>> happen to tune the number of files to see if 1000 is a good fit? e.g.
>>> with 100 files test reproduced the overlay bug 20% of times, with 1000
>>> files the reproducibility increased to 80%, etc. And the hole in the
>>> beginning too, what's the actual impact on the reproducibility?
>>> 
>>> And you're right about the test time, usually we want to balance between
>>> test time and reproducibility too, so we need to tune and measure the
>>> numbers like test files, loop counts etc.
>>> 
>>> I think these are all good comments for test :)
>> 
>> I didn’t do much accurate testing about reproducibility, and also don’t have
>> a plan to do that. Actually in my testing ENV, the BUG is always reproducible
>> even for only a few of test files.
> 
> What is the underlying fs you are testing with?

I usually use xfs as underlying.

> 
>> 1000 is just my definition for
>> quite many files, and it’s really hard and meaningless to guess what number is
>> best suit for variety of testing ENVs. So if you think the number 1000 is
>> improper number, then I can modify it to right number that you think. Hole is
>> the same.
>> 
> 
> I agree with Eryu that you should not invent numbers, unless test completes
> in a few seconds and reproduces reliably - then you can invent numbers...
> 
> But this got me thinking about the details of overlayfs syncfs bug.
> Overlayfs syncfs *will* actually call underlying fs syncfs (I fixed that)
> but *will not* flush dirty inode data. What that means depends on
> the underlying fs. For ext4 with default jounal=ordered, syncfs will
> commit uncommitted journal transactions to disk, that will force data writeback
> for all inodes, whose *metadata* is modified in uncommitted transactions.
> 
> Ted, please correct me if I am wrong.
> 
> For xfs, situation is a bit different, create operations are also delayed,
> so your test could fail on overlayfs over xfs more easily.
> 
> I think that means that if you create the files and write them in the same
> transaction, syncfs *will* actually sync on inodes data, so it is anyway
> only the very last files that you write that won't be flushed no matter how
> many files you will write.
> What you should try to do to increase the changes of the bug on more fs:
> - create new files and truncate them to final size but leaving them sparse
> - sync
> - buffered write to all files
> - syncfs
> - shutdown
> - cycle_mount
> 
> The rules for choosing the right amount of files/data should be:
> - After first sync, all inodes in the system is not dirty
> - During the time it takes to write all files, flusher thread may kick in
>  (default 30 seconds), flush data of your tests files and interfere with
>  the test. Lets rule out another user doing sync, because most test
>  machines are VMs that just run the tests
> - If one loop iteration of the test above takes less than X second for a
>  valid fs on a slow disk, then if flusher thread does interfere, its
>  interference will be over in less then X second
>  (because we are the only ones dirtying data on the system).
> - If X < 10 seconds, then running 2 or 3 loops of the iteration should
>  be enough to guaranty that we run at least one iteration without
>  interference.

I did more detail tests for three different data modes of ext4 and found
the overlayfs syncfs bug is reproducible on data=ordered and data=writeback,
but on data=journal mode, data is flushed and correct. I only wrote only a few words
to a single file and the bug is always reproducible on my test environment. 

For writeback interferences, AFAIK, from dirty ratio and period.
If we drop all dirty caches & sync before the test, I think we can
avoid interference from it.

So if we don’t have anything else to interference test result, 
I just want to modify to write a small single file as test target.

Am I missing anything?

> 
> So I think if you actually write as little files and data as possible,
> but loop several times.
> IMO, tune the parameters, so an iteration takes ~1 second on slow disk
> and run 3 iterations.
> I have a test setup with spinning disk I can run your test on xfs/ext4 if
> you like more samples then your own test environment.
> 
> And please write comments about those heuristics...
> 
> Thanks,
> Amir.

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

[Amir Goldstein]

On Mon, Dec 11, 2017 at 12:03 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>
>> 在 2017年12月7日，下午4:17，Amir Goldstein <amir73il@gmail.com> 写道：
[...]
>
> I did more detail tests for three different data modes of ext4 and found
> the overlayfs syncfs bug is reproducible on data=ordered and data=writeback,
> but on data=journal mode, data is flushed and correct.

That is expected, because overlayfs does call upper's sync_fs() method and
for journal=data that will flush all dirty pages as well.

> I only wrote only a few words
> to a single file and the bug is always reproducible on my test environment.
>
> For writeback interferences, AFAIK, from dirty ratio and period.
> If we drop all dirty caches & sync before the test, I think we can
> avoid interference from it.
>

Why? does either drop_caches or sync() reset the flusher thread
periodic flush dirty pages?

> So if we don’t have anything else to interference test result,
> I just want to modify to write a small single file as test target.
>
> Am I missing anything?

I think the chance of flusher thread interfering the test and
flushing the dirty page you wrote before _scratch_shutdown exists,
but is small enough so we can neglect it and keep the test as simple
as possible.

Amir.

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

[Chengguang Xu]

> 
> 在 2017年12月11日，下午6:46，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Mon, Dec 11, 2017 at 12:03 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>> 
>>> 在 2017年12月7日，下午4:17，Amir Goldstein <amir73il@gmail.com> 写道：
> [...]
>> 
>> I did more detail tests for three different data modes of ext4 and found
>> the overlayfs syncfs bug is reproducible on data=ordered and data=writeback,
>> but on data=journal mode, data is flushed and correct.
> 
> That is expected, because overlayfs does call upper's sync_fs() method and
> for journal=data that will flush all dirty pages as well.
> 
>> I only wrote only a few words
>> to a single file and the bug is always reproducible on my test environment.
>> 
>> For writeback interferences, AFAIK, from dirty ratio and period.
>> If we drop all dirty caches & sync before the test, I think we can
>> avoid interference from it.
>> 
> 
> Why? does either drop_caches or sync() reset the flusher thread
> periodic flush dirty pages?

Sorry，We have to finish test in 30 seconds after we write test file, 
otherwise may be affected by background flusher. 


> 
>> So if we don’t have anything else to interference test result,
>> I just want to modify to write a small single file as test target.
>> 
>> Am I missing anything?
> 
> I think the chance of flusher thread interfering the test and
> flushing the dirty page you wrote before _scratch_shutdown exists,
> but is small enough so we can neglect it and keep the test as simple
> as possible.

As I know, in normal case flusher thread check dirty inode expiring every 5 seconds(default) and flush dirty 
inode when expires 30 seconds(default). If we can finish test in 30s after running test, it would be OK. 
What do you think? 


> 
> Amir.

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

[Amir Goldstein]

On Mon, Dec 11, 2017 at 2:33 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>
>> 在 2017年12月11日，下午6:46，Amir Goldstein <amir73il@gmail.com> 写道：
>>
>> On Mon, Dec 11, 2017 at 12:03 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>>>
>>>> 在 2017年12月7日，下午4:17，Amir Goldstein <amir73il@gmail.com> 写道：
>> [...]
>>>
>>> I did more detail tests for three different data modes of ext4 and found
>>> the overlayfs syncfs bug is reproducible on data=ordered and data=writeback,
>>> but on data=journal mode, data is flushed and correct.
>>
>> That is expected, because overlayfs does call upper's sync_fs() method and
>> for journal=data that will flush all dirty pages as well.
>>
>>> I only wrote only a few words
>>> to a single file and the bug is always reproducible on my test environment.
>>>
>>> For writeback interferences, AFAIK, from dirty ratio and period.
>>> If we drop all dirty caches & sync before the test, I think we can
>>> avoid interference from it.
>>>
>>
>> Why? does either drop_caches or sync() reset the flusher thread
>> periodic flush dirty pages?
>
> Sorry，We have to finish test in 30 seconds after we write test file,
> otherwise may be affected by background flusher.
>
>
>>
>>> So if we don’t have anything else to interference test result,
>>> I just want to modify to write a small single file as test target.
>>>
>>> Am I missing anything?
>>
>> I think the chance of flusher thread interfering the test and
>> flushing the dirty page you wrote before _scratch_shutdown exists,
>> but is small enough so we can neglect it and keep the test as simple
>> as possible.
>
> As I know, in normal case flusher thread check dirty inode expiring every 5 seconds(default) and flush dirty
> inode when expires 30 seconds(default). If we can finish test in 30s after running test, it would be OK.
> What do you think?
>

5 seconds is ext4 default journal commit interval. this is when dirty
metadata will be flushed.
30 is generic flusher thread interval.
What if test started 29 seconds after last flush?

I am saying that the chances of interference from flusher thread are quite
if the test is very quick.

If you create files and sync at the start of the test, overlayfs
syncfs will call
ext4 sync_fs and that will have nothing to do, because no metadata is dirty,
so test will be quick and we can neglect that change of interference.

*If* you wish to reduce that chance for interference loop the test twice, but
I don't think that's a must.

Amir.

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

[Chengguang Xu]

> 
> 在 2017年12月11日，下午8:44，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Mon, Dec 11, 2017 at 2:33 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>> 
>>> 在 2017年12月11日，下午6:46，Amir Goldstein <amir73il@gmail.com> 写道：
>>> 
>>> On Mon, Dec 11, 2017 at 12:03 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>>>> 
>>>>> 在 2017年12月7日，下午4:17，Amir Goldstein <amir73il@gmail.com> 写道：
>>> [...]
>>>> 
>>>> I did more detail tests for three different data modes of ext4 and found
>>>> the overlayfs syncfs bug is reproducible on data=ordered and data=writeback,
>>>> but on data=journal mode, data is flushed and correct.
>>> 
>>> That is expected, because overlayfs does call upper's sync_fs() method and
>>> for journal=data that will flush all dirty pages as well.
>>> 
>>>> I only wrote only a few words
>>>> to a single file and the bug is always reproducible on my test environment.
>>>> 
>>>> For writeback interferences, AFAIK, from dirty ratio and period.
>>>> If we drop all dirty caches & sync before the test, I think we can
>>>> avoid interference from it.
>>>> 
>>> 
>>> Why? does either drop_caches or sync() reset the flusher thread
>>> periodic flush dirty pages?
>> 
>> Sorry，We have to finish test in 30 seconds after we write test file,
>> otherwise may be affected by background flusher.
>> 
>> 
>>> 
>>>> So if we don’t have anything else to interference test result,
>>>> I just want to modify to write a small single file as test target.
>>>> 
>>>> Am I missing anything?
>>> 
>>> I think the chance of flusher thread interfering the test and
>>> flushing the dirty page you wrote before _scratch_shutdown exists,
>>> but is small enough so we can neglect it and keep the test as simple
>>> as possible.
>> 
>> As I know, in normal case flusher thread check dirty inode expiring every 5 seconds(default) and flush dirty
>> inode when expires 30 seconds(default). If we can finish test in 30s after running test, it would be OK.
>> What do you think?
>> 
> 
> 5 seconds is ext4 default journal commit interval. this is when dirty
> metadata will be flushed.
> 30 is generic flusher thread interval.
> What if test started 29 seconds after last flush?

Maybe we have misunderstanding about the flushing intervals.
I’m saying the intervals which are under /proc/sys/vm to control 
writeback behaviors, it seems not related to any specific filesystem 
like ext4 or others.

dirty_expire_interval
default:3000(millisecond)

dirty_writeback_interval
default:500(millisecond)

Anyway, let me modify test case base on our discussion.


> 
> I am saying that the chances of interference from flusher thread are quite
> if the test is very quick.
> 
> If you create files and sync at the start of the test, overlayfs
> syncfs will call
> ext4 sync_fs and that will have nothing to do, because no metadata is dirty,
> so test will be quick and we can neglect that change of interference.
> 
> *If* you wish to reduce that chance for interference loop the test twice, but
> I don't think that's a must.
> 
> Amir.

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

[Chengguang Xu]

> 在 2017年12月11日，下午9:20，Chengguang Xu <cgxu519@icloud.com> 写道：
> 
>> 
>> 在 2017年12月11日，下午8:44，Amir Goldstein <amir73il@gmail.com> 写道：
>> 
>> On Mon, Dec 11, 2017 at 2:33 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>>> 
>>>> 在 2017年12月11日，下午6:46，Amir Goldstein <amir73il@gmail.com> 写道：
>>>> 
>>>> On Mon, Dec 11, 2017 at 12:03 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>>>>> 
>>>>>> 在 2017年12月7日，下午4:17，Amir Goldstein <amir73il@gmail.com> 写道：
>>>> [...]
>>>>> 
>>>>> I did more detail tests for three different data modes of ext4 and found
>>>>> the overlayfs syncfs bug is reproducible on data=ordered and data=writeback,
>>>>> but on data=journal mode, data is flushed and correct.
>>>> 
>>>> That is expected, because overlayfs does call upper's sync_fs() method and
>>>> for journal=data that will flush all dirty pages as well.
>>>> 
>>>>> I only wrote only a few words
>>>>> to a single file and the bug is always reproducible on my test environment.
>>>>> 
>>>>> For writeback interferences, AFAIK, from dirty ratio and period.
>>>>> If we drop all dirty caches & sync before the test, I think we can
>>>>> avoid interference from it.
>>>>> 
>>>> 
>>>> Why? does either drop_caches or sync() reset the flusher thread
>>>> periodic flush dirty pages?
>>> 
>>> Sorry，We have to finish test in 30 seconds after we write test file,
>>> otherwise may be affected by background flusher.
>>> 
>>> 
>>>> 
>>>>> So if we don’t have anything else to interference test result,
>>>>> I just want to modify to write a small single file as test target.
>>>>> 
>>>>> Am I missing anything?
>>>> 
>>>> I think the chance of flusher thread interfering the test and
>>>> flushing the dirty page you wrote before _scratch_shutdown exists,
>>>> but is small enough so we can neglect it and keep the test as simple
>>>> as possible.
>>> 
>>> As I know, in normal case flusher thread check dirty inode expiring every 5 seconds(default) and flush dirty
>>> inode when expires 30 seconds(default). If we can finish test in 30s after running test, it would be OK.
>>> What do you think?
>>> 
>> 
>> 5 seconds is ext4 default journal commit interval. this is when dirty
>> metadata will be flushed.
>> 30 is generic flusher thread interval.
>> What if test started 29 seconds after last flush?
> 
> Maybe we have misunderstanding about the flushing intervals.
> I’m saying the intervals which are under /proc/sys/vm to control 
> writeback behaviors, it seems not related to any specific filesystem 
> like ext4 or others.
> 
> dirty_expire_interval
> default:3000(millisecond)
> 
> dirty_writeback_interval
> default:500(millisecond)

Correction:

dirty_expire_interval
default:3000(centiseconds)

dirty_writeback_interval
default:500(centiseconds)


> 
> Anyway, let me modify test case base on our discussion.
> 
> 
>> 
>> I am saying that the chances of interference from flusher thread are quite
>> if the test is very quick.
>> 
>> If you create files and sync at the start of the test, overlayfs
>> syncfs will call
>> ext4 sync_fs and that will have nothing to do, because no metadata is dirty,
>> so test will be quick and we can neglect that change of interference.
>> 
>> *If* you wish to reduce that chance for interference loop the test twice, but
>> I don't think that's a must.
>> 
>> Amir.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Amir Goldstein]

On Mon, Dec 11, 2017 at 4:31 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>
>> 在 2017年12月11日，下午9:20，Chengguang Xu <cgxu519@icloud.com> 写道：
>>
>>>
>>> 在 2017年12月11日，下午8:44，Amir Goldstein <amir73il@gmail.com> 写道：
>>>
>>> On Mon, Dec 11, 2017 at 2:33 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>>>>
>>>>> 在 2017年12月11日，下午6:46，Amir Goldstein <amir73il@gmail.com> 写道：
>>>>>
>>>>> On Mon, Dec 11, 2017 at 12:03 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>>>>>>
>>>>>>> 在 2017年12月7日，下午4:17，Amir Goldstein <amir73il@gmail.com> 写道：
>>>>> [...]
>>>>>>
>>>>>> I did more detail tests for three different data modes of ext4 and found
>>>>>> the overlayfs syncfs bug is reproducible on data=ordered and data=writeback,
>>>>>> but on data=journal mode, data is flushed and correct.
>>>>>
>>>>> That is expected, because overlayfs does call upper's sync_fs() method and
>>>>> for journal=data that will flush all dirty pages as well.
>>>>>
>>>>>> I only wrote only a few words
>>>>>> to a single file and the bug is always reproducible on my test environment.
>>>>>>
>>>>>> For writeback interferences, AFAIK, from dirty ratio and period.
>>>>>> If we drop all dirty caches & sync before the test, I think we can
>>>>>> avoid interference from it.
>>>>>>
>>>>>
>>>>> Why? does either drop_caches or sync() reset the flusher thread
>>>>> periodic flush dirty pages?
>>>>
>>>> Sorry，We have to finish test in 30 seconds after we write test file,
>>>> otherwise may be affected by background flusher.
>>>>
>>>>
>>>>>
>>>>>> So if we don’t have anything else to interference test result,
>>>>>> I just want to modify to write a small single file as test target.
>>>>>>
>>>>>> Am I missing anything?
>>>>>
>>>>> I think the chance of flusher thread interfering the test and
>>>>> flushing the dirty page you wrote before _scratch_shutdown exists,
>>>>> but is small enough so we can neglect it and keep the test as simple
>>>>> as possible.
>>>>
>>>> As I know, in normal case flusher thread check dirty inode expiring every 5 seconds(default) and flush dirty
>>>> inode when expires 30 seconds(default). If we can finish test in 30s after running test, it would be OK.
>>>> What do you think?
>>>>
>>>
>>> 5 seconds is ext4 default journal commit interval. this is when dirty
>>> metadata will be flushed.
>>> 30 is generic flusher thread interval.
>>> What if test started 29 seconds after last flush?
>>
>> Maybe we have misunderstanding about the flushing intervals.
>> I’m saying the intervals which are under /proc/sys/vm to control
>> writeback behaviors, it seems not related to any specific filesystem
>> like ext4 or others.
>>
>> dirty_expire_interval
>> default:3000(millisecond)
>>
>> dirty_writeback_interval
>> default:500(millisecond)
>
> Correction:
>
> dirty_expire_interval
> default:3000(centiseconds)
>
> dirty_writeback_interval
> default:500(centiseconds)
>
>

Right. I was confusing the 2 different tunables.
I agree your test should be fine with simple small write after sync
in an isolated test environment.

Amir.

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

[Dave Chinner]

On Mon, Dec 11, 2017 at 02:44:50PM +0200, Amir Goldstein wrote:
> On Mon, Dec 11, 2017 at 2:33 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
> >>
> >> 在 2017年12月11日，下午6:46，Amir Goldstein <amir73il@gmail.com> 写道：
> >>
> >> On Mon, Dec 11, 2017 at 12:03 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
> >>>>
> >>>> 在 2017年12月7日，下午4:17，Amir Goldstein <amir73il@gmail.com> 写道：
> >> [...]
> >>>
> >>> I did more detail tests for three different data modes of ext4 and found
> >>> the overlayfs syncfs bug is reproducible on data=ordered and data=writeback,
> >>> but on data=journal mode, data is flushed and correct.
> >>
> >> That is expected, because overlayfs does call upper's sync_fs() method and
> >> for journal=data that will flush all dirty pages as well.
> >>
> >>> I only wrote only a few words
> >>> to a single file and the bug is always reproducible on my test environment.
> >>>
> >>> For writeback interferences, AFAIK, from dirty ratio and period.
> >>> If we drop all dirty caches & sync before the test, I think we can
> >>> avoid interference from it.
> >>>
> >>
> >> Why? does either drop_caches or sync() reset the flusher thread
> >> periodic flush dirty pages?
> >
> > Sorry，We have to finish test in 30 seconds after we write test file,
> > otherwise may be affected by background flusher.
> >
> >
> >>
> >>> So if we don’t have anything else to interference test result,
> >>> I just want to modify to write a small single file as test target.
> >>>
> >>> Am I missing anything?
> >>
> >> I think the chance of flusher thread interfering the test and
> >> flushing the dirty page you wrote before _scratch_shutdown exists,
> >> but is small enough so we can neglect it and keep the test as simple
> >> as possible.
> >
> > As I know, in normal case flusher thread check dirty inode expiring every 5 seconds(default) and flush dirty
> > inode when expires 30 seconds(default). If we can finish test in 30s after running test, it would be OK.
> > What do you think?
> >
> 
> 5 seconds is ext4 default journal commit interval. this is when dirty
> metadata will be flushed.
> 30 is generic flusher thread interval.
> What if test started 29 seconds after last flush?

Then the inode is only 1 second "old" when the flush occurs and so
it gets ignored. it'll get flushed only once it's been dirty for
longer than a flusher thread interval.

IIRC there's quite a few tests where we make the assumption we've
got at least 30s before the dirty cached data will be written by the
kernel. e.g. for shutting down the filesystem before data hits the
disk to test metadata vs data recovery state.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Amir Goldstein]

On Thu, Dec 7, 2017 at 4:22 AM, Chengguang Xu <cgxu519@icloud.com> wrote:
> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
> ---
>
> Changes since v1:
> _scratch_shutdown() does not call notrun.
>
>  common/rc | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index 4c053a5..911dba3 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -382,6 +382,15 @@ _scratch_cycle_mount()
>      _scratch_mount "$opts"
>  }
>
> +_scratch_shutdown()
> +{
> +       if [ $FSTYP = "overlay" ]; then

You need the OVL_BASE_SCRATCH_DEV protection here as well.
Yes, test SHOULD have _require_scratch_shutdown() before calling
_scratch_shutdown(), but if test is not compliant worst case is that
test "fails" instead of "not run", but NOT that host fs is shutdown.

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

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

[Eryu Guan]

On Thu, Dec 07, 2017 at 10:22:05AM +0800, Chengguang Xu wrote:
> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>

Thanks a lot for all the revisions!

Better to have some commit log to describe this change.

> ---
> 
> Changes since v1:
> _scratch_shutdown() does not call notrun.
> 
>  common/rc | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 4c053a5..911dba3 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -382,6 +382,15 @@ _scratch_cycle_mount()
>      _scratch_mount "$opts"
>  }
>  
> +_scratch_shutdown()
> +{
> +	if [ $FSTYP = "overlay" ]; then
> +		src/godown -f $OVL_BASE_SCRATCH_MNT
> +	else
> +		src/godown -f $SCRATCH_MNT

Make "-f" optional? e.g.

	src/godown $* $SCRATCH_MNT

then call could decide if the test want to flush the log on shutdown, by
calling "_scratch_shutdown" or "_scratch_shutdown -f".

> +	fi
> +}
> +
>  _test_mount()
>  {
>      if [ "$FSTYP" == "overlay" ]; then
> @@ -2908,8 +2917,19 @@ _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
> +			_notrun "$SCRATCH_DEV is not a block device"

Better to leave some comments here to describe that we don't want to
shutdown other filesystems accidently.

Thanks,
Eryu

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

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

[Dave Chinner]

On Thu, Dec 07, 2017 at 10:22:05AM +0800, Chengguang Xu wrote:
> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>

Commit message explaining why overlay requires special handling?

> ---
> 
> Changes since v1:
> _scratch_shutdown() does not call notrun.
> 
>  common/rc | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 4c053a5..911dba3 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -382,6 +382,15 @@ _scratch_cycle_mount()
>      _scratch_mount "$opts"
>  }
>  
> +_scratch_shutdown()
> +{
> +	if [ $FSTYP = "overlay" ]; then
> +		src/godown -f $OVL_BASE_SCRATCH_MNT
> +	else
> +		src/godown -f $SCRATCH_MNT
> +	fi
> +}
> +
>  _test_mount()
>  {
>      if [ "$FSTYP" == "overlay" ]; then
> @@ -2908,8 +2917,19 @@ _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
> +			_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

Comment explaining why overlay requires special handling.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com