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

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

[Chengguang Xu]

check -overlay overrides variable SCRATCH_MNT to it's own,
so overlayfs can not be accepted by _require_scratch_shutdown.
In order to test some cases which require shutdown on overlayfs,
adjust shutdown target from SCRATCH_MNT to
OVL_BASE_SCRATCH_MNT(underlaying filesystem) when detecting
overlayfs to pass require check successfully. And in actual case,
convert bare godown calling to a helper _scratch_shutdown which
can recognize overlayfs and handle shutdown correctly for avoiding
failure of original cases in the generic shutdown group except cases
require local device.

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

Changes since v4:
1. Add return code check for _scratch_mkfs and _scratch_mount in
_require_scratch_shutdown.
2. Change commit log.

Changes since v3:
1. When _scratch_shutdown() detecting improper shutdown on overlayfs,
prompt error message and exit immediately.
2. Convert godown to _scratch_shutdown for the cases overlayfs shuold run.
3. Add _require_local_device to the cases overlayfs should not run.

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

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

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

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

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

[Chengguang Xu]

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

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

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

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

[Chengguang Xu]

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

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

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

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

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

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

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

[Amir Goldstein]

On Fri, Dec 15, 2017 at 9:48 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>
> ---

Hi Chengguang,

I just noticed that this test (now generic/474) is skipped for overlay over ext4
for a strange looking reason - it seems that in the overlay_mount() call from
_scratch_cycle_mount() the function hits
_notrun "upper fs needs to support d_type".

This doesn't happen on my test system with overlay over xfs.
Did you happen to test this with overlay over ext4 at the time?
Will you have time to look into this?

Thanks,
Amir.

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

[cgxu519]

Hi Amir,

I think I tested both xfs/ext4. Let me have a look at the code for detail.


Thanks,

Chengguang


On 08/26/2018 10:53 PM, Amir Goldstein wrote:
> On Fri, Dec 15, 2017 at 9:48 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>
>> ---
> Hi Chengguang,
>
> I just noticed that this test (now generic/474) is skipped for overlay over ext4
> for a strange looking reason - it seems that in the overlay_mount() call from
> _scratch_cycle_mount() the function hits
> _notrun "upper fs needs to support d_type".
>
> This doesn't happen on my test system with overlay over xfs.
> Did you happen to test this with overlay over ext4 at the time?
> Will you have time to look into this?
>
> Thanks,
> Amir.

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

[cgxu519]

Hi Amir,

It seems godown will cause side effect on ext4/tune2fs, mount an ext4 
filesystem after running godown,
then tune2fs -l <dev> complains like below. so definitely it will not 
pass d_type check.


[root@x201 xfstests-dev]# tune2fs -l /dev/mapper/test-test1
tune2fs 1.44.2 (14-May-2018)
tune2fs: Superblock checksum does not match superblock while trying to 
open /dev/mapper/test-test1
Couldn't find valid filesystem superblock.


Thanks,
Chengguang


On 08/30/2018 11:12 AM, cgxu519 wrote:
> Hi Amir,
>
> I think I tested both xfs/ext4. Let me have a look at the code for 
> detail.
>
>
> Thanks,
>
> Chengguang
>
>
> On 08/26/2018 10:53 PM, Amir Goldstein wrote:
>> On Fri, Dec 15, 2017 at 9:48 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>
>>> ---
>> Hi Chengguang,
>>
>> I just noticed that this test (now generic/474) is skipped for 
>> overlay over ext4
>> for a strange looking reason - it seems that in the overlay_mount() 
>> call from
>> _scratch_cycle_mount() the function hits
>> _notrun "upper fs needs to support d_type".
>>
>> This doesn't happen on my test system with overlay over xfs.
>> Did you happen to test this with overlay over ext4 at the time?
>> Will you have time to look into this?
>>
>> Thanks,
>> Amir.
>

Re: problem with tune2fs after ext4 godown

[Amir Goldstein]

[switching over to ext4 list]

On Fri, Aug 31, 2018 at 5:19 PM cgxu519 <cgxu519@gmx.com> wrote:
>
>
> Hi Amir,
>
> It seems godown will cause side effect on ext4/tune2fs, mount an ext4
> filesystem after running godown,
> then tune2fs -l <dev> complains like below. so definitely it will not
> pass d_type check.
>
>
> [root@x201 xfstests-dev]# tune2fs -l /dev/mapper/test-test1
> tune2fs 1.44.2 (14-May-2018)
> tune2fs: Superblock checksum does not match superblock while trying to
> open /dev/mapper/test-test1
> Couldn't find valid filesystem superblock.
>
>

Chengguang,

Bringing this to the attention of ext4 developers,
can you please re-iterate the reproducer (without the mention of
overlayfs and genereic/474) to demonstrate the problem.

And please try to avoid "top posting".

Thanks,
Amir.

Re: problem with tune2fs after ext4 godown

[cgxu519]

On 08/31/2018 10:28 PM, Amir Goldstein wrote:
> [switching over to ext4 list]
>
> On Fri, Aug 31, 2018 at 5:19 PM cgxu519 <cgxu519@gmx.com> wrote:
>>
>> Hi Amir,
>>
>> It seems godown will cause side effect on ext4/tune2fs, mount an ext4
>> filesystem after running godown,
>> then tune2fs -l <dev> complains like below. so definitely it will not
>> pass d_type check.
>>
>>
>> [root@x201 xfstests-dev]# tune2fs -l /dev/mapper/test-test1
>> tune2fs 1.44.2 (14-May-2018)
>> tune2fs: Superblock checksum does not match superblock while trying to
>> open /dev/mapper/test-test1
>> Couldn't find valid filesystem superblock.
>>
>>
> Chengguang,
>
> Bringing this to the attention of ext4 developers,
> can you please re-iterate the reproducer (without the mention of
> overlayfs and genereic/474) to demonstrate the problem.

steps to reproduce:

1) mount an ext4 fs
2) write a file in the fs
3) sync / or do nothing
4) running godown tool in xfstest to the fs
5) umount the fs
6) mount the fs
7) tune2fs -l <dev>

tune2fs -l will fail with below error.

[root@x201 xfstests-dev]# tune2fs -l /dev/mapper/test-test1
tune2fs 1.44.2 (14-May-2018)
tune2fs: Superblock checksum does not match superblock while trying to
open /dev/mapper/test-test1
Couldn't find valid filesystem superblock.


If I do not write any file in (2), then tune2fs -l in (7) will finish 
successfully.
If I run tune2fs -l during (4) and (5) or during (5) and (6) also finish 
successfully.

NOTE, I don't know this is a real issue or just by design.


Thanks,
Chengguang

Re: problem with tune2fs after ext4 godown

[Theodore Y. Ts'o]

On Fri, Aug 31, 2018 at 10:53:54PM +0800, cgxu519 wrote:
> steps to reproduce:
> 
> 1) mount an ext4 fs
> 2) write a file in the fs
> 3) sync / or do nothing
> 4) running godown tool in xfstest to the fs
> 5) umount the fs
> 6) mount the fs
> 7) tune2fs -l <dev>
> 
> tune2fs -l will fail with below error.
> 
> [root@x201 xfstests-dev]# tune2fs -l /dev/mapper/test-test1
> tune2fs 1.44.2 (14-May-2018)
> tune2fs: Superblock checksum does not match superblock while trying to
> open /dev/mapper/test-test1
> Couldn't find valid filesystem superblock.
> 
> NOTE, I don't know this is a real issue or just by design.

It's a real issue, thanks.  The fix is below:

					- Ted

>From 4274f516d4bc50648a4d97e4f67ecbd7b65cde4a Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Sat, 1 Sep 2018 14:42:14 -0400
Subject: [PATCH] ext4: recalucate superblock checksum after updating free
 blocks/inodes

When mounting the superblock, ext4_fill_super() calculates the free
blocks and free inodes and stores them in the superblock.  It's not
strictly necessary, since we don't use them any more, but it's nice to
keep them roughly aligned to reality.

Since it's not critical for file system correctness, the code doesn't
call ext4_commit_super().  The problem is that it's in
ext4_commit_super() that we recalculate the superblock checksum.  So
if we're not going to call ext4_commit_super(), we need to call
ext4_superblock_csum_set() to make sure the superblock checksum is
consistent.

Most of the time, this doesn't matter, since we end up calling
ext4_commit_super() very soon thereafter, and definitely by the time
the file system is unmounted.  However, it doesn't work in this
sequence:

mke2fs -Fq -t ext4 /dev/vdc 128M
mount /dev/vdc /vdc
cp xfstests/git-versions /vdc
godown /vdc
umount /vdc
mount /dev/vdc
tune2fs -l /dev/vdc

With this commit, the "tune2fs -l" no longer fails.

Reported-by: Chengguang Xu <cgxu519@gmx.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
---
 fs/ext4/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index f7750bc5b85a..e41da553b430 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4378,11 +4378,13 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	block = ext4_count_free_clusters(sb);
 	ext4_free_blocks_count_set(sbi->s_es, 
 				   EXT4_C2B(sbi, block));
+	ext4_superblock_csum_set(sb);
 	err = percpu_counter_init(&sbi->s_freeclusters_counter, block,
 				  GFP_KERNEL);
 	if (!err) {
 		unsigned long freei = ext4_count_free_inodes(sb);
 		sbi->s_es->s_free_inodes_count = cpu_to_le32(freei);
+		ext4_superblock_csum_set(sb);
 		err = percpu_counter_init(&sbi->s_freeinodes_counter, freei,
 					  GFP_KERNEL);
 	}
-- 
2.18.0.rc0