[PATCH v2] generic: test i_mode recovery after power failure

[PATCH v2] generic: test i_mode recovery after power failure

[Chao Yu]

After fsync, filesystem should guarantee inode metadata including
permission info being persisted, so even after sudden power-cut,
during mount, we should recover i_mode fields correctly, in order
to not loss those meta info.

So adding this testcase to check whether generic filesystem can
guarantee that.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v2:
change as Filipe suggested:
- use dmflakey to simulate power-cut for generic filesystem which
doesn't support godown ioctl interface.
- minor code style change.
 tests/generic/532     | 105 ++++++++++++++++++++++++++++++++++++++++++
 tests/generic/532.out |   2 +
 tests/generic/group   |   1 +
 3 files changed, 108 insertions(+)
 create mode 100755 tests/generic/532
 create mode 100644 tests/generic/532.out

diff --git a/tests/generic/532 b/tests/generic/532
new file mode 100755
index 000000000000..1c5dffcdc4f6
--- /dev/null
+++ b/tests/generic/532
@@ -0,0 +1,105 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2019 Huawei.  All Rights Reserved.
+#
+# FS QA Test 532
+#
+# This testcase is trying to test recovery flow of generic filesystem,
+# w/ below steps, once i_mode changes, after we fsync that file, we can
+# expect that i_mode can be recovered after sudden power-cuts.
+# 1. touch testfile or mkdir testdir
+# 2. chmod 777 testfile/testdir
+# 3. sync
+# 4. chmod 755 testfile/testdir
+# 5. fsync testfile/testdir
+# 6. record last i_mode
+# 7. flakey drop
+# 8. remount
+# 9. check i_mode
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	_cleanup_flakey
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/dmflakey
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+
+_require_scratch
+_require_dm_target flakey
+
+_scratch_mkfs >/dev/null 2>&1
+_require_metadata_journaling $SCRATCH_DEV
+_init_flakey
+
+testfile=$SCRATCH_MNT/testfile
+testdir=$SCRATCH_MNT/testdir
+
+do_check()
+{
+	local target=$1
+	local is_dir=$2
+
+	_mount_flakey
+
+	if [ $is_dir = 1 ]; then
+		mkdir $target
+	else
+		touch $target
+	fi
+
+	echo "Test chmod $target" >> $seqres.full
+
+	chmod 777 $target
+	sync
+
+	chmod 755 $target
+	$XFS_IO_PROG $target -c "fsync"
+
+	local before=`stat -c %a $target`
+
+	_flakey_drop_and_remount
+
+	local after=`stat -c %a $target`
+
+	# check inode's i_mode
+	if [ "$before" != "$after" ]; then
+		echo "Before: $before"
+		echo "After : $after"
+	fi
+
+	if [ $is_dir = 1 ]; then
+		rmdir $target
+	else
+		rm -f $target
+	fi
+	_unmount_flakey
+}
+
+echo "Silence is golden"
+
+do_check $testfile 0
+do_check $testdir 1
+
+status=0
+exit
diff --git a/tests/generic/532.out b/tests/generic/532.out
new file mode 100644
index 000000000000..1f7d4677c3be
--- /dev/null
+++ b/tests/generic/532.out
@@ -0,0 +1,2 @@
+QA output created by 532
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index 15227b6713f7..c19361667886 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -534,3 +534,4 @@
 529 auto quick attr
 530 auto quick unlink
 531 auto quick unlink
+532 shutdown auto quick metadata
-- 
2.18.0.rc1

[PATCH v2] generic: test i_mode recovery after power failure

[Chao Yu]

After fsync, filesystem should guarantee inode metadata including
permission info being persisted, so even after sudden power-cut,
during mount, we should recover i_mode fields correctly, in order
to not loss those meta info.

So adding this testcase to check whether generic filesystem can
guarantee that.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v2:
change as Filipe suggested:
- use dmflakey to simulate power-cut for generic filesystem which
doesn't support godown ioctl interface.
- minor code style change.
 tests/generic/532     | 105 ++++++++++++++++++++++++++++++++++++++++++
 tests/generic/532.out |   2 +
 tests/generic/group   |   1 +
 3 files changed, 108 insertions(+)
 create mode 100755 tests/generic/532
 create mode 100644 tests/generic/532.out

diff --git a/tests/generic/532 b/tests/generic/532
new file mode 100755
index 000000000000..1c5dffcdc4f6
--- /dev/null
+++ b/tests/generic/532
@@ -0,0 +1,105 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2019 Huawei.  All Rights Reserved.
+#
+# FS QA Test 532
+#
+# This testcase is trying to test recovery flow of generic filesystem,
+# w/ below steps, once i_mode changes, after we fsync that file, we can
+# expect that i_mode can be recovered after sudden power-cuts.
+# 1. touch testfile or mkdir testdir
+# 2. chmod 777 testfile/testdir
+# 3. sync
+# 4. chmod 755 testfile/testdir
+# 5. fsync testfile/testdir
+# 6. record last i_mode
+# 7. flakey drop
+# 8. remount
+# 9. check i_mode
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	_cleanup_flakey
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/dmflakey
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+
+_require_scratch
+_require_dm_target flakey
+
+_scratch_mkfs >/dev/null 2>&1
+_require_metadata_journaling $SCRATCH_DEV
+_init_flakey
+
+testfile=$SCRATCH_MNT/testfile
+testdir=$SCRATCH_MNT/testdir
+
+do_check()
+{
+	local target=$1
+	local is_dir=$2
+
+	_mount_flakey
+
+	if [ $is_dir = 1 ]; then
+		mkdir $target
+	else
+		touch $target
+	fi
+
+	echo "Test chmod $target" >> $seqres.full
+
+	chmod 777 $target
+	sync
+
+	chmod 755 $target
+	$XFS_IO_PROG $target -c "fsync"
+
+	local before=`stat -c %a $target`
+
+	_flakey_drop_and_remount
+
+	local after=`stat -c %a $target`
+
+	# check inode's i_mode
+	if [ "$before" != "$after" ]; then
+		echo "Before: $before"
+		echo "After : $after"
+	fi
+
+	if [ $is_dir = 1 ]; then
+		rmdir $target
+	else
+		rm -f $target
+	fi
+	_unmount_flakey
+}
+
+echo "Silence is golden"
+
+do_check $testfile 0
+do_check $testdir 1
+
+status=0
+exit
diff --git a/tests/generic/532.out b/tests/generic/532.out
new file mode 100644
index 000000000000..1f7d4677c3be
--- /dev/null
+++ b/tests/generic/532.out
@@ -0,0 +1,2 @@
+QA output created by 532
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index 15227b6713f7..c19361667886 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -534,3 +534,4 @@
 529 auto quick attr
 530 auto quick unlink
 531 auto quick unlink
+532 shutdown auto quick metadata
-- 
2.18.0.rc1

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Filipe Manana]

On Tue, Mar 5, 2019 at 11:48 AM Chao Yu <yuchao0@huawei.com> wrote:
>
> After fsync, filesystem should guarantee inode metadata including
> permission info being persisted, so even after sudden power-cut,
> during mount, we should recover i_mode fields correctly, in order
> to not loss those meta info.
>
> So adding this testcase to check whether generic filesystem can
> guarantee that.
>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> v2:
> change as Filipe suggested:
> - use dmflakey to simulate power-cut for generic filesystem which
> doesn't support godown ioctl interface.
> - minor code style change.
>  tests/generic/532     | 105 ++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/532.out |   2 +
>  tests/generic/group   |   1 +
>  3 files changed, 108 insertions(+)
>  create mode 100755 tests/generic/532
>  create mode 100644 tests/generic/532.out
>
> diff --git a/tests/generic/532 b/tests/generic/532
> new file mode 100755
> index 000000000000..1c5dffcdc4f6
> --- /dev/null
> +++ b/tests/generic/532
> @@ -0,0 +1,105 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2019 Huawei.  All Rights Reserved.
> +#
> +# FS QA Test 532
> +#
> +# This testcase is trying to test recovery flow of generic filesystem,
> +# w/ below steps, once i_mode changes, after we fsync that file, we can
> +# expect that i_mode can be recovered after sudden power-cuts.
> +# 1. touch testfile or mkdir testdir
> +# 2. chmod 777 testfile/testdir
> +# 3. sync
> +# 4. chmod 755 testfile/testdir
> +# 5. fsync testfile/testdir
> +# 6. record last i_mode
> +# 7. flakey drop
> +# 8. remount
> +# 9. check i_mode
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1       # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       _cleanup_flakey
> +       cd /
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/dmflakey
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +_supported_fs generic
> +_supported_os Linux
> +
> +_require_scratch
> +_require_dm_target flakey
> +
> +_scratch_mkfs >/dev/null 2>&1
> +_require_metadata_journaling $SCRATCH_DEV
> +_init_flakey
> +
> +testfile=$SCRATCH_MNT/testfile
> +testdir=$SCRATCH_MNT/testdir
> +
> +do_check()
> +{
> +       local target=$1
> +       local is_dir=$2
> +
> +       _mount_flakey
> +
> +       if [ $is_dir = 1 ]; then
> +               mkdir $target
> +       else
> +               touch $target
> +       fi
> +
> +       echo "Test chmod $target" >> $seqres.full
> +
> +       chmod 777 $target
> +       sync
> +
> +       chmod 755 $target
> +       $XFS_IO_PROG $target -c "fsync"
> +
> +       local before=`stat -c %a $target`
> +
> +       _flakey_drop_and_remount
> +
> +       local after=`stat -c %a $target`
> +
> +       # check inode's i_mode
> +       if [ "$before" != "$after" ]; then
> +               echo "Before: $before"
> +               echo "After : $after"
> +       fi
> +
> +       if [ $is_dir = 1 ]; then
> +               rmdir $target
> +       else
> +               rm -f $target
> +       fi
> +       _unmount_flakey
> +}
> +
> +echo "Silence is golden"
> +
> +do_check $testfile 0
> +do_check $testdir 1
> +
> +status=0
> +exit
> diff --git a/tests/generic/532.out b/tests/generic/532.out
> new file mode 100644
> index 000000000000..1f7d4677c3be
> --- /dev/null
> +++ b/tests/generic/532.out
> @@ -0,0 +1,2 @@
> +QA output created by 532
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index 15227b6713f7..c19361667886 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -534,3 +534,4 @@
>  529 auto quick attr
>  530 auto quick unlink
>  531 auto quick unlink
> +532 shutdown auto quick metadata

Since the test now uses dmflakey, it should no longer belong to the
'shutdown' group and should
belong to the 'log' group like all other tests using dmflakey.
Probably Eryu can change that when
he picks the patch.

Other than that it looks good to me, thanks.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

> --
> 2.18.0.rc1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Filipe Manana]

On Tue, Mar 5, 2019 at 11:48 AM Chao Yu <yuchao0@huawei.com> wrote:
>
> After fsync, filesystem should guarantee inode metadata including
> permission info being persisted, so even after sudden power-cut,
> during mount, we should recover i_mode fields correctly, in order
> to not loss those meta info.
>
> So adding this testcase to check whether generic filesystem can
> guarantee that.
>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> v2:
> change as Filipe suggested:
> - use dmflakey to simulate power-cut for generic filesystem which
> doesn't support godown ioctl interface.
> - minor code style change.
>  tests/generic/532     | 105 ++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/532.out |   2 +
>  tests/generic/group   |   1 +
>  3 files changed, 108 insertions(+)
>  create mode 100755 tests/generic/532
>  create mode 100644 tests/generic/532.out
>
> diff --git a/tests/generic/532 b/tests/generic/532
> new file mode 100755
> index 000000000000..1c5dffcdc4f6
> --- /dev/null
> +++ b/tests/generic/532
> @@ -0,0 +1,105 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2019 Huawei.  All Rights Reserved.
> +#
> +# FS QA Test 532
> +#
> +# This testcase is trying to test recovery flow of generic filesystem,
> +# w/ below steps, once i_mode changes, after we fsync that file, we can
> +# expect that i_mode can be recovered after sudden power-cuts.
> +# 1. touch testfile or mkdir testdir
> +# 2. chmod 777 testfile/testdir
> +# 3. sync
> +# 4. chmod 755 testfile/testdir
> +# 5. fsync testfile/testdir
> +# 6. record last i_mode
> +# 7. flakey drop
> +# 8. remount
> +# 9. check i_mode
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1       # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       _cleanup_flakey
> +       cd /
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/dmflakey
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +_supported_fs generic
> +_supported_os Linux
> +
> +_require_scratch
> +_require_dm_target flakey
> +
> +_scratch_mkfs >/dev/null 2>&1
> +_require_metadata_journaling $SCRATCH_DEV
> +_init_flakey
> +
> +testfile=$SCRATCH_MNT/testfile
> +testdir=$SCRATCH_MNT/testdir
> +
> +do_check()
> +{
> +       local target=$1
> +       local is_dir=$2
> +
> +       _mount_flakey
> +
> +       if [ $is_dir = 1 ]; then
> +               mkdir $target
> +       else
> +               touch $target
> +       fi
> +
> +       echo "Test chmod $target" >> $seqres.full
> +
> +       chmod 777 $target
> +       sync
> +
> +       chmod 755 $target
> +       $XFS_IO_PROG $target -c "fsync"
> +
> +       local before=`stat -c %a $target`
> +
> +       _flakey_drop_and_remount
> +
> +       local after=`stat -c %a $target`
> +
> +       # check inode's i_mode
> +       if [ "$before" != "$after" ]; then
> +               echo "Before: $before"
> +               echo "After : $after"
> +       fi
> +
> +       if [ $is_dir = 1 ]; then
> +               rmdir $target
> +       else
> +               rm -f $target
> +       fi
> +       _unmount_flakey
> +}
> +
> +echo "Silence is golden"
> +
> +do_check $testfile 0
> +do_check $testdir 1
> +
> +status=0
> +exit
> diff --git a/tests/generic/532.out b/tests/generic/532.out
> new file mode 100644
> index 000000000000..1f7d4677c3be
> --- /dev/null
> +++ b/tests/generic/532.out
> @@ -0,0 +1,2 @@
> +QA output created by 532
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index 15227b6713f7..c19361667886 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -534,3 +534,4 @@
>  529 auto quick attr
>  530 auto quick unlink
>  531 auto quick unlink
> +532 shutdown auto quick metadata

Since the test now uses dmflakey, it should no longer belong to the
'shutdown' group and should
belong to the 'log' group like all other tests using dmflakey.
Probably Eryu can change that when
he picks the patch.

Other than that it looks good to me, thanks.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

> --
> 2.18.0.rc1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Dave Chinner]

On Tue, Mar 05, 2019 at 07:47:44PM +0800, Chao Yu wrote:
> After fsync, filesystem should guarantee inode metadata including
> permission info being persisted, so even after sudden power-cut,
> during mount, we should recover i_mode fields correctly, in order
> to not loss those meta info.
> 
> So adding this testcase to check whether generic filesystem can
> guarantee that.

Can I make a suggestion here?

I've noticed that we are getting a lot of these one-off, random
"fsync persists one specific piece of metadata in one specific case"
tests, mainly in response to some fsync bug that was found in btrfs.
This is reactive, and does not find new bugs in this area.

We are also beyond the point where the number of tests is
maintainable (e.g. to be able to make infrastructure changes), so we
really should be looking to consolidate largely similar tests into
single tests where possible.

I'd strongly suggest that a robust fsync tester should be written
for all the cases that are currently being addressed in an ad hoc
fashion. Then they can be consolidated into a single test run, and
we can get rid of all the one-off "fsync failed on this <specific
thing>" tests from the harness.

Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
and generic/311.

Can we please consider rolling all of these "do something, fsync,
drop-writes, remount check" into fsync-tester.c and do the same for
all future one-off "did fsync persist X" tests?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Dave Chinner]

On Tue, Mar 05, 2019 at 07:47:44PM +0800, Chao Yu wrote:
> After fsync, filesystem should guarantee inode metadata including
> permission info being persisted, so even after sudden power-cut,
> during mount, we should recover i_mode fields correctly, in order
> to not loss those meta info.
> 
> So adding this testcase to check whether generic filesystem can
> guarantee that.

Can I make a suggestion here?

I've noticed that we are getting a lot of these one-off, random
"fsync persists one specific piece of metadata in one specific case"
tests, mainly in response to some fsync bug that was found in btrfs.
This is reactive, and does not find new bugs in this area.

We are also beyond the point where the number of tests is
maintainable (e.g. to be able to make infrastructure changes), so we
really should be looking to consolidate largely similar tests into
single tests where possible.

I'd strongly suggest that a robust fsync tester should be written
for all the cases that are currently being addressed in an ad hoc
fashion. Then they can be consolidated into a single test run, and
we can get rid of all the one-off "fsync failed on this <specific
thing>" tests from the harness.

Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
and generic/311.

Can we please consider rolling all of these "do something, fsync,
drop-writes, remount check" into fsync-tester.c and do the same for
all future one-off "did fsync persist X" tests?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Chao Yu]

On 2019/3/6 4:53, Dave Chinner wrote:
> On Tue, Mar 05, 2019 at 07:47:44PM +0800, Chao Yu wrote:
>> After fsync, filesystem should guarantee inode metadata including
>> permission info being persisted, so even after sudden power-cut,
>> during mount, we should recover i_mode fields correctly, in order
>> to not loss those meta info.
>>
>> So adding this testcase to check whether generic filesystem can
>> guarantee that.
> 
> Can I make a suggestion here?
> 
> I've noticed that we are getting a lot of these one-off, random
> "fsync persists one specific piece of metadata in one specific case"
> tests, mainly in response to some fsync bug that was found in btrfs.
> This is reactive, and does not find new bugs in this area.
> 
> We are also beyond the point where the number of tests is
> maintainable (e.g. to be able to make infrastructure changes), so we
> really should be looking to consolidate largely similar tests into
> single tests where possible.
> 
> I'd strongly suggest that a robust fsync tester should be written
> for all the cases that are currently being addressed in an ad hoc
> fashion. Then they can be consolidated into a single test run, and
> we can get rid of all the one-off "fsync failed on this <specific
> thing>" tests from the harness.

Thanks for the suggestion, and that makes sense.

> 
> Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> and generic/311.
> 
> Can we please consider rolling all of these "do something, fsync,
> drop-writes, remount check" into fsync-tester.c and do the same for
> all future one-off "did fsync persist X" tests?

Let me add this testcase into fsync-tester.c, still we need add new
testcase in generic/ directory instead of changing generic/311 directly,
right? since as I remember that Eryu mentioned that:

"Usually we don't add new sub-tests to existing tests, so new failures
introduced by the new sub-tests won't be treated as false regressions.
Please add a new test instead."

https://patchwork.kernel.org/patch/10042149/

Or to avoid redundant copied code from generic/311 in each fsync related
patch, do I need just add my case into fsync_tester.c? and before
announcement of fstests master branch, we can add one testcase into
generic/ directory, in that testcase we gather/test all new added cases in
fsync_tester.c recently.

Which one is easier for you to maintain?

Thanks,

> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Chao Yu]

On 2019/3/6 4:53, Dave Chinner wrote:
> On Tue, Mar 05, 2019 at 07:47:44PM +0800, Chao Yu wrote:
>> After fsync, filesystem should guarantee inode metadata including
>> permission info being persisted, so even after sudden power-cut,
>> during mount, we should recover i_mode fields correctly, in order
>> to not loss those meta info.
>>
>> So adding this testcase to check whether generic filesystem can
>> guarantee that.
> 
> Can I make a suggestion here?
> 
> I've noticed that we are getting a lot of these one-off, random
> "fsync persists one specific piece of metadata in one specific case"
> tests, mainly in response to some fsync bug that was found in btrfs.
> This is reactive, and does not find new bugs in this area.
> 
> We are also beyond the point where the number of tests is
> maintainable (e.g. to be able to make infrastructure changes), so we
> really should be looking to consolidate largely similar tests into
> single tests where possible.
> 
> I'd strongly suggest that a robust fsync tester should be written
> for all the cases that are currently being addressed in an ad hoc
> fashion. Then they can be consolidated into a single test run, and
> we can get rid of all the one-off "fsync failed on this <specific
> thing>" tests from the harness.

Thanks for the suggestion, and that makes sense.

> 
> Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> and generic/311.
> 
> Can we please consider rolling all of these "do something, fsync,
> drop-writes, remount check" into fsync-tester.c and do the same for
> all future one-off "did fsync persist X" tests?

Let me add this testcase into fsync-tester.c, still we need add new
testcase in generic/ directory instead of changing generic/311 directly,
right? since as I remember that Eryu mentioned that:

"Usually we don't add new sub-tests to existing tests, so new failures
introduced by the new sub-tests won't be treated as false regressions.
Please add a new test instead."

https://patchwork.kernel.org/patch/10042149/

Or to avoid redundant copied code from generic/311 in each fsync related
patch, do I need just add my case into fsync_tester.c? and before
announcement of fstests master branch, we can add one testcase into
generic/ directory, in that testcase we gather/test all new added cases in
fsync_tester.c recently.

Which one is easier for you to maintain?

Thanks,

> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Dave Chinner]

On Wed, Mar 06, 2019 at 10:29:10AM +0800, Chao Yu wrote:
> On 2019/3/6 4:53, Dave Chinner wrote:
> > On Tue, Mar 05, 2019 at 07:47:44PM +0800, Chao Yu wrote:
> >> After fsync, filesystem should guarantee inode metadata including
> >> permission info being persisted, so even after sudden power-cut,
> >> during mount, we should recover i_mode fields correctly, in order
> >> to not loss those meta info.
> >>
> >> So adding this testcase to check whether generic filesystem can
> >> guarantee that.
> > 
> > Can I make a suggestion here?
> > 
> > I've noticed that we are getting a lot of these one-off, random
> > "fsync persists one specific piece of metadata in one specific case"
> > tests, mainly in response to some fsync bug that was found in btrfs.
> > This is reactive, and does not find new bugs in this area.
> > 
> > We are also beyond the point where the number of tests is
> > maintainable (e.g. to be able to make infrastructure changes), so we
> > really should be looking to consolidate largely similar tests into
> > single tests where possible.
> > 
> > I'd strongly suggest that a robust fsync tester should be written
> > for all the cases that are currently being addressed in an ad hoc
> > fashion. Then they can be consolidated into a single test run, and
> > we can get rid of all the one-off "fsync failed on this <specific
> > thing>" tests from the harness.
> 
> Thanks for the suggestion, and that makes sense.
> 
> > 
> > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > and generic/311.
> > 
> > Can we please consider rolling all of these "do something, fsync,
> > drop-writes, remount check" into fsync-tester.c and do the same for
> > all future one-off "did fsync persist X" tests?
> 
> Let me add this testcase into fsync-tester.c, still we need add new
> testcase in generic/ directory instead of changing generic/311 directly,
> right? since as I remember that Eryu mentioned that:
> 
> "Usually we don't add new sub-tests to existing tests, so new failures
> introduced by the new sub-tests won't be treated as false regressions.
> Please add a new test instead."

Yeah, sure, but there's no point in adding a new fstest for every
case we move/add to fsync-tester.c - that's no better than what we have
now. i.e the whole point of using fsync-tester.c is to aggregate
lots of individual tests full of copy-n-paste dm-flakey
infrastructure into a single test. Copy-n-paste of fsync tester
boiler plate is not an improvement.

There are other 'aggregation' style tests in fstests, and we do
extend them from time to time. Some that come to mind are the
falloc/fpunch/fzero corner case tests, and we add new functionality
to fsstress and fsx all the time which affects every test that
runs them, etc.


> https://patchwork.kernel.org/patch/10042149/
> 
> Or to avoid redundant copied code from generic/311 in each fsync related
> patch, do I need just add my case into fsync_tester.c? and before
> announcement of fstests master branch, we can add one testcase into
> generic/ directory, in that testcase we gather/test all new added cases in
> fsync_tester.c recently.

Gathering them all into fsync-tester is what I'm suggesting should
be done....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Dave Chinner]

On Wed, Mar 06, 2019 at 10:29:10AM +0800, Chao Yu wrote:
> On 2019/3/6 4:53, Dave Chinner wrote:
> > On Tue, Mar 05, 2019 at 07:47:44PM +0800, Chao Yu wrote:
> >> After fsync, filesystem should guarantee inode metadata including
> >> permission info being persisted, so even after sudden power-cut,
> >> during mount, we should recover i_mode fields correctly, in order
> >> to not loss those meta info.
> >>
> >> So adding this testcase to check whether generic filesystem can
> >> guarantee that.
> > 
> > Can I make a suggestion here?
> > 
> > I've noticed that we are getting a lot of these one-off, random
> > "fsync persists one specific piece of metadata in one specific case"
> > tests, mainly in response to some fsync bug that was found in btrfs.
> > This is reactive, and does not find new bugs in this area.
> > 
> > We are also beyond the point where the number of tests is
> > maintainable (e.g. to be able to make infrastructure changes), so we
> > really should be looking to consolidate largely similar tests into
> > single tests where possible.
> > 
> > I'd strongly suggest that a robust fsync tester should be written
> > for all the cases that are currently being addressed in an ad hoc
> > fashion. Then they can be consolidated into a single test run, and
> > we can get rid of all the one-off "fsync failed on this <specific
> > thing>" tests from the harness.
> 
> Thanks for the suggestion, and that makes sense.
> 
> > 
> > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > and generic/311.
> > 
> > Can we please consider rolling all of these "do something, fsync,
> > drop-writes, remount check" into fsync-tester.c and do the same for
> > all future one-off "did fsync persist X" tests?
> 
> Let me add this testcase into fsync-tester.c, still we need add new
> testcase in generic/ directory instead of changing generic/311 directly,
> right? since as I remember that Eryu mentioned that:
> 
> "Usually we don't add new sub-tests to existing tests, so new failures
> introduced by the new sub-tests won't be treated as false regressions.
> Please add a new test instead."

Yeah, sure, but there's no point in adding a new fstest for every
case we move/add to fsync-tester.c - that's no better than what we have
now. i.e the whole point of using fsync-tester.c is to aggregate
lots of individual tests full of copy-n-paste dm-flakey
infrastructure into a single test. Copy-n-paste of fsync tester
boiler plate is not an improvement.

There are other 'aggregation' style tests in fstests, and we do
extend them from time to time. Some that come to mind are the
falloc/fpunch/fzero corner case tests, and we add new functionality
to fsstress and fsx all the time which affects every test that
runs them, etc.


> https://patchwork.kernel.org/patch/10042149/
> 
> Or to avoid redundant copied code from generic/311 in each fsync related
> patch, do I need just add my case into fsync_tester.c? and before
> announcement of fstests master branch, we can add one testcase into
> generic/ directory, in that testcase we gather/test all new added cases in
> fsync_tester.c recently.

Gathering them all into fsync-tester is what I'm suggesting should
be done....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Amir Goldstein]

> > >
> > > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > > and generic/311.
> > >

Right now 311 is not "quick".
That means adding quick tests to it without breaking it up or declaring it quick
is not a good idea.

> > > Can we please consider rolling all of these "do something, fsync,
> > > drop-writes, remount check" into fsync-tester.c and do the same for
> > > all future one-off "did fsync persist X" tests?
> >
> > Let me add this testcase into fsync-tester.c, still we need add new
> > testcase in generic/ directory instead of changing generic/311 directly,
> > right? since as I remember that Eryu mentioned that:
> >
> > "Usually we don't add new sub-tests to existing tests, so new failures
> > introduced by the new sub-tests won't be treated as false regressions.
> > Please add a new test instead."
>
> Yeah, sure, but there's no point in adding a new fstest for every
> case we move/add to fsync-tester.c - that's no better than what we have
> now. i.e the whole point of using fsync-tester.c is to aggregate
> lots of individual tests full of copy-n-paste dm-flakey
> infrastructure into a single test. Copy-n-paste of fsync tester
> boiler plate is not an improvement.
>
> There are other 'aggregation' style tests in fstests, and we do
> extend them from time to time. Some that come to mind are the
> falloc/fpunch/fzero corner case tests, and we add new functionality
> to fsstress and fsx all the time which affects every test that
> runs them, etc.
>
>
> > https://patchwork.kernel.org/patch/10042149/
> >
> > Or to avoid redundant copied code from generic/311 in each fsync related
> > patch, do I need just add my case into fsync_tester.c? and before
> > announcement of fstests master branch, we can add one testcase into
> > generic/ directory, in that testcase we gather/test all new added cases in
> > fsync_tester.c recently.
>
> Gathering them all into fsync-tester is what I'm suggesting should
> be done....
>

We could introduce the concept of test cases into the test infrastructure.
For example:

--- a/tests/generic/311
+++ b/tests/generic/311
@@ -90,7 +90,7 @@ _mount_flakey
 buffered=0
 direct=1

-for i in $(seq 1 20); do
+for i in $(seq ${TEST_CASE_START:-1} ${TEST_CASE_END:-20}); do
        lockfs=1
        SEED=$i
        echo "Running test $i buffered, normal suspend"
---

Adding a new test cases (beyond changing fsync_tester.c)
requires:
- Creating symlink tests/generic/311:21..24 -> 311
- Writing golden output tests/generic/311:21..24.out

Test infrastructure will parse TEST_CASE_START/END
from test name.

"No regressing tests" rule not broken.
No boiler plate copy of tests.

Thanks,
Amir.

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Amir Goldstein]

> > >
> > > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > > and generic/311.
> > >

Right now 311 is not "quick".
That means adding quick tests to it without breaking it up or declaring it quick
is not a good idea.

> > > Can we please consider rolling all of these "do something, fsync,
> > > drop-writes, remount check" into fsync-tester.c and do the same for
> > > all future one-off "did fsync persist X" tests?
> >
> > Let me add this testcase into fsync-tester.c, still we need add new
> > testcase in generic/ directory instead of changing generic/311 directly,
> > right? since as I remember that Eryu mentioned that:
> >
> > "Usually we don't add new sub-tests to existing tests, so new failures
> > introduced by the new sub-tests won't be treated as false regressions.
> > Please add a new test instead."
>
> Yeah, sure, but there's no point in adding a new fstest for every
> case we move/add to fsync-tester.c - that's no better than what we have
> now. i.e the whole point of using fsync-tester.c is to aggregate
> lots of individual tests full of copy-n-paste dm-flakey
> infrastructure into a single test. Copy-n-paste of fsync tester
> boiler plate is not an improvement.
>
> There are other 'aggregation' style tests in fstests, and we do
> extend them from time to time. Some that come to mind are the
> falloc/fpunch/fzero corner case tests, and we add new functionality
> to fsstress and fsx all the time which affects every test that
> runs them, etc.
>
>
> > https://patchwork.kernel.org/patch/10042149/
> >
> > Or to avoid redundant copied code from generic/311 in each fsync related
> > patch, do I need just add my case into fsync_tester.c? and before
> > announcement of fstests master branch, we can add one testcase into
> > generic/ directory, in that testcase we gather/test all new added cases in
> > fsync_tester.c recently.
>
> Gathering them all into fsync-tester is what I'm suggesting should
> be done....
>

We could introduce the concept of test cases into the test infrastructure.
For example:

--- a/tests/generic/311
+++ b/tests/generic/311
@@ -90,7 +90,7 @@ _mount_flakey
 buffered=0
 direct=1

-for i in $(seq 1 20); do
+for i in $(seq ${TEST_CASE_START:-1} ${TEST_CASE_END:-20}); do
        lockfs=1
        SEED=$i
        echo "Running test $i buffered, normal suspend"
---

Adding a new test cases (beyond changing fsync_tester.c)
requires:
- Creating symlink tests/generic/311:21..24 -> 311
- Writing golden output tests/generic/311:21..24.out

Test infrastructure will parse TEST_CASE_START/END
from test name.

"No regressing tests" rule not broken.
No boiler plate copy of tests.

Thanks,
Amir.

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Dave Chinner]

On Wed, Mar 06, 2019 at 09:44:54AM +0200, Amir Goldstein wrote:
> > > >
> > > > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > > > and generic/311.
> > > >
> 
> Right now 311 is not "quick".
> That means adding quick tests to it without breaking it up or declaring it quick
> is not a good idea.

Why would we need to change the group? Indeed, I almost never use
the "quick" group anymore because it doesn't mean "quickly run a
smoke test" anymore. It now just means "test doesn't take a long
time" but that still adds up to 30-60 minutes of runtime (depending
on storage) because of the hundreds of tests in the quick group.

If you are testing crash recovery changes, then you are likely
running the "log" group to execute all the crash recovery tests,
maybe the "metadata" group, and maybe the "shutdown" group.

So I don't think the this test not being in the "quick" group is
relevant at all.

> > > https://patchwork.kernel.org/patch/10042149/
> > >
> > > Or to avoid redundant copied code from generic/311 in each fsync related
> > > patch, do I need just add my case into fsync_tester.c? and before
> > > announcement of fstests master branch, we can add one testcase into
> > > generic/ directory, in that testcase we gather/test all new added cases in
> > > fsync_tester.c recently.
> >
> > Gathering them all into fsync-tester is what I'm suggesting should
> > be done....
> >
> 
> We could introduce the concept of test cases into the test infrastructure.
> For example:
> 
> --- a/tests/generic/311
> +++ b/tests/generic/311
> @@ -90,7 +90,7 @@ _mount_flakey
>  buffered=0
>  direct=1
> 
> -for i in $(seq 1 20); do
> +for i in $(seq ${TEST_CASE_START:-1} ${TEST_CASE_END:-20}); do
>         lockfs=1
>         SEED=$i
>         echo "Running test $i buffered, normal suspend"
> ---
> 
> Adding a new test cases (beyond changing fsync_tester.c)
> requires:
> - Creating symlink tests/generic/311:21..24 -> 311
> - Writing golden output tests/generic/311:21..24.out

Why create complex new infrastructure for something we already have
mechanisms to do?

i.e. move the common code into common/<file>, include it in the new
test, and call:

_<common_test_func> 21 24

To constrain it to just the cases that this test runs.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Dave Chinner]

On Wed, Mar 06, 2019 at 09:44:54AM +0200, Amir Goldstein wrote:
> > > >
> > > > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > > > and generic/311.
> > > >
> 
> Right now 311 is not "quick".
> That means adding quick tests to it without breaking it up or declaring it quick
> is not a good idea.

Why would we need to change the group? Indeed, I almost never use
the "quick" group anymore because it doesn't mean "quickly run a
smoke test" anymore. It now just means "test doesn't take a long
time" but that still adds up to 30-60 minutes of runtime (depending
on storage) because of the hundreds of tests in the quick group.

If you are testing crash recovery changes, then you are likely
running the "log" group to execute all the crash recovery tests,
maybe the "metadata" group, and maybe the "shutdown" group.

So I don't think the this test not being in the "quick" group is
relevant at all.

> > > https://patchwork.kernel.org/patch/10042149/
> > >
> > > Or to avoid redundant copied code from generic/311 in each fsync related
> > > patch, do I need just add my case into fsync_tester.c? and before
> > > announcement of fstests master branch, we can add one testcase into
> > > generic/ directory, in that testcase we gather/test all new added cases in
> > > fsync_tester.c recently.
> >
> > Gathering them all into fsync-tester is what I'm suggesting should
> > be done....
> >
> 
> We could introduce the concept of test cases into the test infrastructure.
> For example:
> 
> --- a/tests/generic/311
> +++ b/tests/generic/311
> @@ -90,7 +90,7 @@ _mount_flakey
>  buffered=0
>  direct=1
> 
> -for i in $(seq 1 20); do
> +for i in $(seq ${TEST_CASE_START:-1} ${TEST_CASE_END:-20}); do
>         lockfs=1
>         SEED=$i
>         echo "Running test $i buffered, normal suspend"
> ---
> 
> Adding a new test cases (beyond changing fsync_tester.c)
> requires:
> - Creating symlink tests/generic/311:21..24 -> 311
> - Writing golden output tests/generic/311:21..24.out

Why create complex new infrastructure for something we already have
mechanisms to do?

i.e. move the common code into common/<file>, include it in the new
test, and call:

_<common_test_func> 21 24

To constrain it to just the cases that this test runs.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Amir Goldstein]

On Thu, Mar 7, 2019 at 12:12 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Wed, Mar 06, 2019 at 09:44:54AM +0200, Amir Goldstein wrote:
> > > > >
> > > > > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > > > > and generic/311.
> > > > >
> >
> > Right now 311 is not "quick".
> > That means adding quick tests to it without breaking it up or declaring it quick
> > is not a good idea.
>
> Why would we need to change the group? Indeed, I almost never use
> the "quick" group anymore because it doesn't mean "quickly run a
> smoke test" anymore. It now just means "test doesn't take a long
> time" but that still adds up to 30-60 minutes of runtime (depending
> on storage) because of the hundreds of tests in the quick group.
>
> If you are testing crash recovery changes, then you are likely
> running the "log" group to execute all the crash recovery tests,
> maybe the "metadata" group, and maybe the "shutdown" group.
>
> So I don't think the this test not being in the "quick" group is
> relevant at all.
>

OK. Just pointing your attention to the fact that the test generic/520
is a result of public discussion of how crash consistency tests should
be aggregated into xfstests tests.

I must say that I like the result in generic/520 much better than I like
generic/311, because of its readability - it is easier to see which test
cases are covered.

Thanks,
Amir.

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Amir Goldstein]

On Thu, Mar 7, 2019 at 12:12 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Wed, Mar 06, 2019 at 09:44:54AM +0200, Amir Goldstein wrote:
> > > > >
> > > > > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > > > > and generic/311.
> > > > >
> >
> > Right now 311 is not "quick".
> > That means adding quick tests to it without breaking it up or declaring it quick
> > is not a good idea.
>
> Why would we need to change the group? Indeed, I almost never use
> the "quick" group anymore because it doesn't mean "quickly run a
> smoke test" anymore. It now just means "test doesn't take a long
> time" but that still adds up to 30-60 minutes of runtime (depending
> on storage) because of the hundreds of tests in the quick group.
>
> If you are testing crash recovery changes, then you are likely
> running the "log" group to execute all the crash recovery tests,
> maybe the "metadata" group, and maybe the "shutdown" group.
>
> So I don't think the this test not being in the "quick" group is
> relevant at all.
>

OK. Just pointing your attention to the fact that the test generic/520
is a result of public discussion of how crash consistency tests should
be aggregated into xfstests tests.

I must say that I like the result in generic/520 much better than I like
generic/311, because of its readability - it is easier to see which test
cases are covered.

Thanks,
Amir.

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Dave Chinner]

On Thu, Mar 07, 2019 at 09:12:06AM +0200, Amir Goldstein wrote:
> On Thu, Mar 7, 2019 at 12:12 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Wed, Mar 06, 2019 at 09:44:54AM +0200, Amir Goldstein wrote:
> > > > > >
> > > > > > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > > > > > and generic/311.
> > > > > >
> > >
> > > Right now 311 is not "quick".
> > > That means adding quick tests to it without breaking it up or declaring it quick
> > > is not a good idea.
> >
> > Why would we need to change the group? Indeed, I almost never use
> > the "quick" group anymore because it doesn't mean "quickly run a
> > smoke test" anymore. It now just means "test doesn't take a long
> > time" but that still adds up to 30-60 minutes of runtime (depending
> > on storage) because of the hundreds of tests in the quick group.
> >
> > If you are testing crash recovery changes, then you are likely
> > running the "log" group to execute all the crash recovery tests,
> > maybe the "metadata" group, and maybe the "shutdown" group.
> >
> > So I don't think the this test not being in the "quick" group is
> > relevant at all.
> >
> 
> OK. Just pointing your attention to the fact that the test generic/520
> is a result of public discussion of how crash consistency tests should
> be aggregated into xfstests tests.

That was about how the crashmonkey tests would be integrated, not
generic fsync tests should be integrated.  There lots of
auto-generated crashmonkey tests them and they were proposing a
single fs test per single fsync test. We ended up settling  on
"aggregating into related groups" and generic/520 only covers one
specific group - only about 5 test cases of the many, many
crashmonkey test cases that were proposed. Which leaves me to
ponder: what happened to the rest of the Crashmonkey test cases that
were supposed to follow on from generic/520?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Dave Chinner]

On Thu, Mar 07, 2019 at 09:12:06AM +0200, Amir Goldstein wrote:
> On Thu, Mar 7, 2019 at 12:12 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Wed, Mar 06, 2019 at 09:44:54AM +0200, Amir Goldstein wrote:
> > > > > >
> > > > > > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > > > > > and generic/311.
> > > > > >
> > >
> > > Right now 311 is not "quick".
> > > That means adding quick tests to it without breaking it up or declaring it quick
> > > is not a good idea.
> >
> > Why would we need to change the group? Indeed, I almost never use
> > the "quick" group anymore because it doesn't mean "quickly run a
> > smoke test" anymore. It now just means "test doesn't take a long
> > time" but that still adds up to 30-60 minutes of runtime (depending
> > on storage) because of the hundreds of tests in the quick group.
> >
> > If you are testing crash recovery changes, then you are likely
> > running the "log" group to execute all the crash recovery tests,
> > maybe the "metadata" group, and maybe the "shutdown" group.
> >
> > So I don't think the this test not being in the "quick" group is
> > relevant at all.
> >
> 
> OK. Just pointing your attention to the fact that the test generic/520
> is a result of public discussion of how crash consistency tests should
> be aggregated into xfstests tests.

That was about how the crashmonkey tests would be integrated, not
generic fsync tests should be integrated.  There lots of
auto-generated crashmonkey tests them and they were proposing a
single fs test per single fsync test. We ended up settling  on
"aggregating into related groups" and generic/520 only covers one
specific group - only about 5 test cases of the many, many
crashmonkey test cases that were proposed. Which leaves me to
ponder: what happened to the rest of the Crashmonkey test cases that
were supposed to follow on from generic/520?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [f2fs-dev] [PATCH v2] generic: test i_mode recovery after power failure

[Jayashree Mohan]

Hi Dave,

We will submit patches for the rest of the tests soon. We are working on it.

Thanks,
Jayashree Mohan


Thanks,
Jayashree Mohan



On Thu, Mar 7, 2019 at 2:22 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Thu, Mar 07, 2019 at 09:12:06AM +0200, Amir Goldstein wrote:
> > On Thu, Mar 7, 2019 at 12:12 AM Dave Chinner <david@fromorbit.com> wrote:
> > >
> > > On Wed, Mar 06, 2019 at 09:44:54AM +0200, Amir Goldstein wrote:
> > > > > > >
> > > > > > > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > > > > > > and generic/311.
> > > > > > >
> > > >
> > > > Right now 311 is not "quick".
> > > > That means adding quick tests to it without breaking it up or declaring it quick
> > > > is not a good idea.
> > >
> > > Why would we need to change the group? Indeed, I almost never use
> > > the "quick" group anymore because it doesn't mean "quickly run a
> > > smoke test" anymore. It now just means "test doesn't take a long
> > > time" but that still adds up to 30-60 minutes of runtime (depending
> > > on storage) because of the hundreds of tests in the quick group.
> > >
> > > If you are testing crash recovery changes, then you are likely
> > > running the "log" group to execute all the crash recovery tests,
> > > maybe the "metadata" group, and maybe the "shutdown" group.
> > >
> > > So I don't think the this test not being in the "quick" group is
> > > relevant at all.
> > >
> >
> > OK. Just pointing your attention to the fact that the test generic/520
> > is a result of public discussion of how crash consistency tests should
> > be aggregated into xfstests tests.
>
> That was about how the crashmonkey tests would be integrated, not
> generic fsync tests should be integrated.  There lots of
> auto-generated crashmonkey tests them and they were proposing a
> single fs test per single fsync test. We ended up settling  on
> "aggregating into related groups" and generic/520 only covers one
> specific group - only about 5 test cases of the many, many
> crashmonkey test cases that were proposed. Which leaves me to
> ponder: what happened to the rest of the Crashmonkey test cases that
> were supposed to follow on from generic/520?
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Jayashree Mohan]

Hi Dave,

We will submit patches for the rest of the tests soon. We are working on it.

Thanks,
Jayashree Mohan


Thanks,
Jayashree Mohan



On Thu, Mar 7, 2019 at 2:22 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Thu, Mar 07, 2019 at 09:12:06AM +0200, Amir Goldstein wrote:
> > On Thu, Mar 7, 2019 at 12:12 AM Dave Chinner <david@fromorbit.com> wrote:
> > >
> > > On Wed, Mar 06, 2019 at 09:44:54AM +0200, Amir Goldstein wrote:
> > > > > > >
> > > > > > > Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> > > > > > > and generic/311.
> > > > > > >
> > > >
> > > > Right now 311 is not "quick".
> > > > That means adding quick tests to it without breaking it up or declaring it quick
> > > > is not a good idea.
> > >
> > > Why would we need to change the group? Indeed, I almost never use
> > > the "quick" group anymore because it doesn't mean "quickly run a
> > > smoke test" anymore. It now just means "test doesn't take a long
> > > time" but that still adds up to 30-60 minutes of runtime (depending
> > > on storage) because of the hundreds of tests in the quick group.
> > >
> > > If you are testing crash recovery changes, then you are likely
> > > running the "log" group to execute all the crash recovery tests,
> > > maybe the "metadata" group, and maybe the "shutdown" group.
> > >
> > > So I don't think the this test not being in the "quick" group is
> > > relevant at all.
> > >
> >
> > OK. Just pointing your attention to the fact that the test generic/520
> > is a result of public discussion of how crash consistency tests should
> > be aggregated into xfstests tests.
>
> That was about how the crashmonkey tests would be integrated, not
> generic fsync tests should be integrated.  There lots of
> auto-generated crashmonkey tests them and they were proposing a
> single fs test per single fsync test. We ended up settling  on
> "aggregating into related groups" and generic/520 only covers one
> specific group - only about 5 test cases of the many, many
> crashmonkey test cases that were proposed. Which leaves me to
> ponder: what happened to the rest of the Crashmonkey test cases that
> were supposed to follow on from generic/520?
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Eryu Guan]

On Wed, Mar 06, 2019 at 07:53:44AM +1100, Dave Chinner wrote:
> On Tue, Mar 05, 2019 at 07:47:44PM +0800, Chao Yu wrote:
> > After fsync, filesystem should guarantee inode metadata including
> > permission info being persisted, so even after sudden power-cut,
> > during mount, we should recover i_mode fields correctly, in order
> > to not loss those meta info.
> > 
> > So adding this testcase to check whether generic filesystem can
> > guarantee that.
> 
> Can I make a suggestion here?
> 
> I've noticed that we are getting a lot of these one-off, random
> "fsync persists one specific piece of metadata in one specific case"
> tests, mainly in response to some fsync bug that was found in btrfs.
> This is reactive, and does not find new bugs in this area.
> 
> We are also beyond the point where the number of tests is
> maintainable (e.g. to be able to make infrastructure changes), so we
> really should be looking to consolidate largely similar tests into
> single tests where possible.

This sounds reasonable, and could reduce the test time a bit as well.

> 
> I'd strongly suggest that a robust fsync tester should be written
> for all the cases that are currently being addressed in an ad hoc
> fashion. Then they can be consolidated into a single test run, and
> we can get rid of all the one-off "fsync failed on this <specific
> thing>" tests from the harness.
> 
> Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> and generic/311.
> 
> Can we please consider rolling all of these "do something, fsync,
> drop-writes, remount check" into fsync-tester.c and do the same for
> all future one-off "did fsync persist X" tests?

I'd like to take this patch and the one from Filipe for now, and look
into folding them into fsync-tester later, in a separate patch.

Thanks,
Eryu

Re: [PATCH v2] generic: test i_mode recovery after power failure

[Eryu Guan]

On Wed, Mar 06, 2019 at 07:53:44AM +1100, Dave Chinner wrote:
> On Tue, Mar 05, 2019 at 07:47:44PM +0800, Chao Yu wrote:
> > After fsync, filesystem should guarantee inode metadata including
> > permission info being persisted, so even after sudden power-cut,
> > during mount, we should recover i_mode fields correctly, in order
> > to not loss those meta info.
> > 
> > So adding this testcase to check whether generic filesystem can
> > guarantee that.
> 
> Can I make a suggestion here?
> 
> I've noticed that we are getting a lot of these one-off, random
> "fsync persists one specific piece of metadata in one specific case"
> tests, mainly in response to some fsync bug that was found in btrfs.
> This is reactive, and does not find new bugs in this area.
> 
> We are also beyond the point where the number of tests is
> maintainable (e.g. to be able to make infrastructure changes), so we
> really should be looking to consolidate largely similar tests into
> single tests where possible.

This sounds reasonable, and could reduce the test time a bit as well.

> 
> I'd strongly suggest that a robust fsync tester should be written
> for all the cases that are currently being addressed in an ad hoc
> fashion. Then they can be consolidated into a single test run, and
> we can get rid of all the one-off "fsync failed on this <specific
> thing>" tests from the harness.
> 
> Oh, wait, we *already have that infrastructure*: src/fsync-tester.c
> and generic/311.
> 
> Can we please consider rolling all of these "do something, fsync,
> drop-writes, remount check" into fsync-tester.c and do the same for
> all future one-off "did fsync persist X" tests?

I'd like to take this patch and the one from Filipe for now, and look
into folding them into fsync-tester later, in a separate patch.

Thanks,
Eryu