| Message ID | 1513324055-77618-3-git-send-email-cgxu519@icloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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. >
[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.
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
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); }
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
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