[PATCHv2 0/4] generic: Add some tests around journal replay/recoveryloop

[PATCHv2 0/4] generic: Add some tests around journal replay/recoveryloop

[Ritesh Harjani]

Hello,

Sending v2 with tests/ext4/ converted to tests/generic/
(although I had not received any review comments on v1).
It seems all of the tests which I had sent in v1 are not ext4 specific anyways.
So in v2, I have made those into tests/generic/.

These are some of the tests which when tested with ext4 fast_commit feature
w/o kernel fixes, could cause tests failures and/or KASAN bug (generic/486).

I gave these tests a run with default xfs, btrfs and f2fs configs (along with
ext4). No surprises observed.

[v1]: https://lore.kernel.org/all/cover.1644070604.git.riteshh@linux.ibm.com/

Ritesh Harjani (4):
  generic/468: Add another falloc test entry
  common/punch: Add block_size argument to _filter_fiemap_**
  generic/676: Add a new shutdown recovery test
  generic/677: Add a test to check unwritten extents tracking

 common/punch          | 12 +++++---
 tests/generic/468     |  4 +++
 tests/generic/468.out |  2 ++
 tests/generic/676     | 72 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/676.out |  7 +++++
 tests/generic/677     | 64 ++++++++++++++++++++++++++++++++++++++
 tests/generic/677.out |  6 ++++
 7 files changed, 162 insertions(+), 5 deletions(-)
 create mode 100755 tests/generic/676
 create mode 100644 tests/generic/676.out
 create mode 100755 tests/generic/677
 create mode 100644 tests/generic/677.out

--
2.31.1

[PATCHv2 1/4] generic/468: Add another falloc test entry

[Ritesh Harjani]

Add another falloc test entry which could hit a kernel bug
with ext4 fast_commit feature w/o below kernel commit [1].

<log>
[  410.888496][ T2743] BUG: KASAN: use-after-free in ext4_mb_mark_bb+0x26a/0x6c0
[  410.890432][ T2743] Read of size 8 at addr ffff888171886000 by task mount/2743

This happens when falloc -k size is huge which spans across more than
1 flex block group in ext4. This causes a bug in fast_commit replay
code which is fixed by kernel commit at [1].

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 tests/generic/468     | 4 ++++
 tests/generic/468.out | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/tests/generic/468 b/tests/generic/468
index 95752d3b..cbef9746 100755
--- a/tests/generic/468
+++ b/tests/generic/468
@@ -34,6 +34,9 @@ _scratch_mkfs >/dev/null 2>&1
 _require_metadata_journaling $SCRATCH_DEV
 _scratch_mount

+blocksize=4096
+fact=18
+
 testfile=$SCRATCH_MNT/testfile

 # check inode metadata after shutdown
@@ -85,6 +88,7 @@ for i in fsync fdatasync; do
 	test_falloc $i "-k " 1024
 	test_falloc $i "-k " 4096
 	test_falloc $i "-k " 104857600
+	test_falloc $i "-k " $((32768*$blocksize*$fact))
 done

 status=0
diff --git a/tests/generic/468.out b/tests/generic/468.out
index b3a28d5e..a09cedb8 100644
--- a/tests/generic/468.out
+++ b/tests/generic/468.out
@@ -5,9 +5,11 @@ QA output created by 468
 ==== falloc -k 1024 test with fsync ====
 ==== falloc -k 4096 test with fsync ====
 ==== falloc -k 104857600 test with fsync ====
+==== falloc -k 2415919104 test with fsync ====
 ==== falloc 1024 test with fdatasync ====
 ==== falloc 4096 test with fdatasync ====
 ==== falloc 104857600 test with fdatasync ====
 ==== falloc -k 1024 test with fdatasync ====
 ==== falloc -k 4096 test with fdatasync ====
 ==== falloc -k 104857600 test with fdatasync ====
+==== falloc -k 2415919104 test with fdatasync ====
--
2.31.1

[PATCHv2 2/4] common/punch: Add block_size argument to _filter_fiemap_**

[Ritesh Harjani]

Add block_size paramter to _filter_fiemap_flags() and
_filter_hole_fiemap(). This is used in next patches

Also this fixes some of the end of line whitespace issues while we are
at it.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 common/punch | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/common/punch b/common/punch
index b6b8a0b9..f99e21ad 100644
--- a/common/punch
+++ b/common/punch
@@ -109,6 +109,7 @@ _filter_fiemap()

 _filter_fiemap_flags()
 {
+	block_size=$1
 	$AWK_PROG '
 		$3 ~ /hole/ {
 			print $1, $2, $3;
@@ -135,23 +136,24 @@ _filter_fiemap_flags()
 			}
 			print $1, $2, flag_str
 		}' |
-	_coalesce_extents
+	_coalesce_extents $block_size
 }

-# Filters fiemap output to only print the
+# Filters fiemap output to only print the
 # file offset column and whether or not
 # it is an extent or a hole
 _filter_hole_fiemap()
 {
+	block_size=$1
 	$AWK_PROG '
 		$3 ~ /hole/ {
-			print $1, $2, $3;
+			print $1, $2, $3;
 			next;
-		}
+		}
 		$5 ~ /0x[[:xdigit:]]+/ {
 			print $1, $2, "extent";
 		}' |
-	_coalesce_extents
+	_coalesce_extents $block_size
 }

 #     10000 Unwritten preallocated extent
--
2.31.1

[PATCHv2 3/4] generic/676: Add a new shutdown recovery test

[Ritesh Harjani]

In certain cases (it is noted with ext4 fast_commit feature) that, replay phase
may not delete the right range of blocks (after sudden FS shutdown)
due to some operations which depends on inode->i_size (which during replay of
an inode with fast_commit could be 0 for sometime).
This fstest is added to test for such scenarios for all generic fs.

This test case is based on the test case shared via Xin Yin.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 tests/generic/676     | 72 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/676.out |  7 +++++
 2 files changed, 79 insertions(+)
 create mode 100755 tests/generic/676
 create mode 100644 tests/generic/676.out

diff --git a/tests/generic/676 b/tests/generic/676
new file mode 100755
index 00000000..315edcdf
--- /dev/null
+++ b/tests/generic/676
@@ -0,0 +1,72 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
+#
+# FS QA Test 676
+#
+# This test with ext4 fast_commit feature w/o below patch missed to delete the right
+# range during replay phase, since it depends upon inode->i_size (which might not be
+# stable during replay phase, at least for ext4).
+# 0b5b5a62b945a141: ext4: use ext4_ext_remove_space() for fast commit replay delete range
+# (Based on test case shared by Xin Yin <yinxin.x@bytedance.com>)
+#
+
+. ./common/preamble
+_begin_fstest auto shutdown quick log recoveryloop
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.*
+   _scratch_unmount > /dev/null 2>&1
+}
+
+# Import common functions.
+. ./common/filter
+. ./common/punch
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_require_scratch
+_require_xfs_io_command "fpunch"
+_require_xfs_io_command "fzero"
+_require_xfs_io_command "fiemap"
+
+t1=$SCRATCH_MNT/foo
+t2=$SCRATCH_MNT/bar
+
+_scratch_mkfs > $seqres.full 2>&1
+
+_scratch_mount >> $seqres.full 2>&1
+
+bs=$(_get_block_size $SCRATCH_MNT)
+
+# create and write data to t1
+$XFS_IO_PROG -f -c "pwrite 0 $((100*$bs))" $t1 | _filter_xfs_io_numbers
+
+# fzero certain range in between with -k
+$XFS_IO_PROG -c "fzero -k  $((40*$bs)) $((20*$bs))" $t1
+
+# create and fsync a new file t2
+$XFS_IO_PROG -f -c "fsync" $t2
+
+# fpunch within the i_size of a file
+$XFS_IO_PROG -c "fpunch $((30*$bs)) $((20*$bs))" $t1
+
+# fsync t1 to trigger journal operation
+$XFS_IO_PROG -c "fsync" $t1
+
+# shutdown FS now for replay journal to kick in next mount
+_scratch_shutdown -v >> $seqres.full 2>&1
+
+_scratch_cycle_mount
+
+# check fiemap reported is valid or not
+$XFS_IO_PROG -c "fiemap -v" $t1 | _filter_fiemap_flags $bs
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/676.out b/tests/generic/676.out
new file mode 100644
index 00000000..78375940
--- /dev/null
+++ b/tests/generic/676.out
@@ -0,0 +1,7 @@
+QA output created by 676
+wrote XXXX/XXXX bytes at offset XXXX
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+0: [0..29]: none
+1: [30..49]: hole
+2: [50..59]: unwritten
+3: [60..99]: nonelast
--
2.31.1

[PATCHv2 4/4] generic/677: Add a test to check unwritten extents tracking

[Ritesh Harjani]

With these sequence of operation (in certain cases like with ext4 fast_commit)
could miss to track unwritten extents during replay phase
(after sudden FS shutdown).

This fstest adds a test case to test this.

5e4d0eba1ccaf19f
ext4: fix fast commit may miss tracking range for FALLOC_FL_ZERO_RANGE

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 tests/generic/677     | 64 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/677.out |  6 ++++
 2 files changed, 70 insertions(+)
 create mode 100755 tests/generic/677
 create mode 100644 tests/generic/677.out

diff --git a/tests/generic/677 b/tests/generic/677
new file mode 100755
index 00000000..e316763a
--- /dev/null
+++ b/tests/generic/677
@@ -0,0 +1,64 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
+#
+# FS QA Test 677
+#
+# Test below sequence of operation which (w/o below kernel patch) in case of
+# ext4 with fast_commit may misss to track unwritten extents.
+# commit 5e4d0eba1ccaf19f
+# ext4: fix fast commit may miss tracking range for FALLOC_FL_ZERO_RANGE
+#
+. ./common/preamble
+_begin_fstest auto quick log shutdown recoveryloop
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.*
+}
+
+# Import common functions.
+. ./common/filter
+. ./common/punch
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_require_scratch
+_require_xfs_io_command "fzero"
+_require_xfs_io_command "fiemap"
+
+t1=$SCRATCH_MNT/t1
+
+_scratch_mkfs > $seqres.full 2>&1
+
+_scratch_mount >> $seqres.full 2>&1
+
+bs=$(_get_block_size $SCRATCH_MNT)
+
+# create and write data to t1
+$XFS_IO_PROG -f -c "pwrite 0 $((100*$bs))" $t1 | _filter_xfs_io_numbers
+
+# fsync t1
+$XFS_IO_PROG -c "fsync" $t1
+
+# fzero certain range in between
+$XFS_IO_PROG -c "fzero -k  $((40*$bs)) $((20*$bs))" $t1
+
+# fsync t1
+$XFS_IO_PROG -c "fsync" $t1
+
+# shutdown FS now for replay of journal to kick during next mount
+_scratch_shutdown -v >> $seqres.full 2>&1
+
+_scratch_cycle_mount
+
+# check fiemap reported is valid or not
+$XFS_IO_PROG -c "fiemap -v" $t1 | _filter_fiemap_flags $bs
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/677.out b/tests/generic/677.out
new file mode 100644
index 00000000..b91ab77a
--- /dev/null
+++ b/tests/generic/677.out
@@ -0,0 +1,6 @@
+QA output created by 677
+wrote XXXX/XXXX bytes at offset XXXX
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+0: [0..39]: none
+1: [40..59]: unwritten
+2: [60..99]: nonelast
--
2.31.1

Re: [PATCHv2 1/4] generic/468: Add another falloc test entry

[Darrick J. Wong]

On Tue, Mar 15, 2022 at 07:58:56PM +0530, Ritesh Harjani wrote:
> Add another falloc test entry which could hit a kernel bug
> with ext4 fast_commit feature w/o below kernel commit [1].
> 
> <log>
> [  410.888496][ T2743] BUG: KASAN: use-after-free in ext4_mb_mark_bb+0x26a/0x6c0
> [  410.890432][ T2743] Read of size 8 at addr ffff888171886000 by task mount/2743
> 
> This happens when falloc -k size is huge which spans across more than
> 1 flex block group in ext4. This causes a bug in fast_commit replay
> code which is fixed by kernel commit at [1].
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  tests/generic/468     | 4 ++++
>  tests/generic/468.out | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/tests/generic/468 b/tests/generic/468
> index 95752d3b..cbef9746 100755
> --- a/tests/generic/468
> +++ b/tests/generic/468
> @@ -34,6 +34,9 @@ _scratch_mkfs >/dev/null 2>&1
>  _require_metadata_journaling $SCRATCH_DEV
>  _scratch_mount
> 
> +blocksize=4096

What happens if the file blocksize isn't 4k?  Does fastcommit only
support one block size?  I didn't think it has any such restriction?

> +fact=18

This needs a bit more explanation -- why 18?  I think the reason is that
you need the fallocate to cross into another flexbg, and flexbgs (by
default) are 16bg long, right?

If that's the case, then don't you need to detect the flexbg size so
that this is still an effective test if someone runs fstests with
MKFS_OPTIONS='-G 32' or something?

--D

> +
>  testfile=$SCRATCH_MNT/testfile
> 
>  # check inode metadata after shutdown
> @@ -85,6 +88,7 @@ for i in fsync fdatasync; do
>  	test_falloc $i "-k " 1024
>  	test_falloc $i "-k " 4096
>  	test_falloc $i "-k " 104857600
> +	test_falloc $i "-k " $((32768*$blocksize*$fact))
>  done
> 
>  status=0
> diff --git a/tests/generic/468.out b/tests/generic/468.out
> index b3a28d5e..a09cedb8 100644
> --- a/tests/generic/468.out
> +++ b/tests/generic/468.out
> @@ -5,9 +5,11 @@ QA output created by 468
>  ==== falloc -k 1024 test with fsync ====
>  ==== falloc -k 4096 test with fsync ====
>  ==== falloc -k 104857600 test with fsync ====
> +==== falloc -k 2415919104 test with fsync ====
>  ==== falloc 1024 test with fdatasync ====
>  ==== falloc 4096 test with fdatasync ====
>  ==== falloc 104857600 test with fdatasync ====
>  ==== falloc -k 1024 test with fdatasync ====
>  ==== falloc -k 4096 test with fdatasync ====
>  ==== falloc -k 104857600 test with fdatasync ====
> +==== falloc -k 2415919104 test with fdatasync ====
> --
> 2.31.1
> 

Re: [PATCHv2 3/4] generic/676: Add a new shutdown recovery test

[Darrick J. Wong]

On Tue, Mar 15, 2022 at 07:58:58PM +0530, Ritesh Harjani wrote:
> In certain cases (it is noted with ext4 fast_commit feature) that, replay phase
> may not delete the right range of blocks (after sudden FS shutdown)
> due to some operations which depends on inode->i_size (which during replay of
> an inode with fast_commit could be 0 for sometime).
> This fstest is added to test for such scenarios for all generic fs.
> 
> This test case is based on the test case shared via Xin Yin.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  tests/generic/676     | 72 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/676.out |  7 +++++
>  2 files changed, 79 insertions(+)
>  create mode 100755 tests/generic/676
>  create mode 100644 tests/generic/676.out
> 
> diff --git a/tests/generic/676 b/tests/generic/676
> new file mode 100755
> index 00000000..315edcdf
> --- /dev/null
> +++ b/tests/generic/676
> @@ -0,0 +1,72 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
> +#
> +# FS QA Test 676
> +#
> +# This test with ext4 fast_commit feature w/o below patch missed to delete the right
> +# range during replay phase, since it depends upon inode->i_size (which might not be
> +# stable during replay phase, at least for ext4).
> +# 0b5b5a62b945a141: ext4: use ext4_ext_remove_space() for fast commit replay delete range
> +# (Based on test case shared by Xin Yin <yinxin.x@bytedance.com>)
> +#
> +
> +. ./common/preamble
> +_begin_fstest auto shutdown quick log recoveryloop

This isn't a looping recovery test.  Maybe we should create a 'recovery'
group for tests that only run once?  I think we already have a few
fstests like that.

> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.*
> +   _scratch_unmount > /dev/null 2>&1

I think the test harness does this for you already, right?

> +}
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/punch
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_require_scratch
> +_require_xfs_io_command "fpunch"
> +_require_xfs_io_command "fzero"
> +_require_xfs_io_command "fiemap"

_require_scratch_shutdown

> +
> +t1=$SCRATCH_MNT/foo
> +t2=$SCRATCH_MNT/bar
> +
> +_scratch_mkfs > $seqres.full 2>&1
> +
> +_scratch_mount >> $seqres.full 2>&1
> +
> +bs=$(_get_block_size $SCRATCH_MNT)

_get_file_block_size, in case the file allocation unit isn't the same as
the fs blocksize?  (e.g. bigalloc, xfs realtime, etc.)

--D

> +
> +# create and write data to t1
> +$XFS_IO_PROG -f -c "pwrite 0 $((100*$bs))" $t1 | _filter_xfs_io_numbers
> +
> +# fzero certain range in between with -k
> +$XFS_IO_PROG -c "fzero -k  $((40*$bs)) $((20*$bs))" $t1
> +
> +# create and fsync a new file t2
> +$XFS_IO_PROG -f -c "fsync" $t2
> +
> +# fpunch within the i_size of a file
> +$XFS_IO_PROG -c "fpunch $((30*$bs)) $((20*$bs))" $t1
> +
> +# fsync t1 to trigger journal operation
> +$XFS_IO_PROG -c "fsync" $t1
> +
> +# shutdown FS now for replay journal to kick in next mount
> +_scratch_shutdown -v >> $seqres.full 2>&1
> +
> +_scratch_cycle_mount
> +
> +# check fiemap reported is valid or not
> +$XFS_IO_PROG -c "fiemap -v" $t1 | _filter_fiemap_flags $bs
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/676.out b/tests/generic/676.out
> new file mode 100644
> index 00000000..78375940
> --- /dev/null
> +++ b/tests/generic/676.out
> @@ -0,0 +1,7 @@
> +QA output created by 676
> +wrote XXXX/XXXX bytes at offset XXXX
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +0: [0..29]: none
> +1: [30..49]: hole
> +2: [50..59]: unwritten
> +3: [60..99]: nonelast
> --
> 2.31.1
> 

Re: [PATCHv2 4/4] generic/677: Add a test to check unwritten extents tracking

[Darrick J. Wong]

On Tue, Mar 15, 2022 at 07:58:59PM +0530, Ritesh Harjani wrote:
> With these sequence of operation (in certain cases like with ext4 fast_commit)
> could miss to track unwritten extents during replay phase
> (after sudden FS shutdown).
> 
> This fstest adds a test case to test this.
> 
> 5e4d0eba1ccaf19f
> ext4: fix fast commit may miss tracking range for FALLOC_FL_ZERO_RANGE
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  tests/generic/677     | 64 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/677.out |  6 ++++
>  2 files changed, 70 insertions(+)
>  create mode 100755 tests/generic/677
>  create mode 100644 tests/generic/677.out
> 
> diff --git a/tests/generic/677 b/tests/generic/677
> new file mode 100755
> index 00000000..e316763a
> --- /dev/null
> +++ b/tests/generic/677
> @@ -0,0 +1,64 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
> +#
> +# FS QA Test 677
> +#
> +# Test below sequence of operation which (w/o below kernel patch) in case of
> +# ext4 with fast_commit may misss to track unwritten extents.
> +# commit 5e4d0eba1ccaf19f
> +# ext4: fix fast commit may miss tracking range for FALLOC_FL_ZERO_RANGE
> +#
> +. ./common/preamble
> +_begin_fstest auto quick log shutdown recoveryloop
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.*
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/punch
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_require_scratch
> +_require_xfs_io_command "fzero"
> +_require_xfs_io_command "fiemap"
> +
> +t1=$SCRATCH_MNT/t1
> +
> +_scratch_mkfs > $seqres.full 2>&1
> +
> +_scratch_mount >> $seqres.full 2>&1
> +
> +bs=$(_get_block_size $SCRATCH_MNT)

Same comments about blocksize, group names, and
_require_scratch_shutdown as the last patch.

--D

> +
> +# create and write data to t1
> +$XFS_IO_PROG -f -c "pwrite 0 $((100*$bs))" $t1 | _filter_xfs_io_numbers
> +
> +# fsync t1
> +$XFS_IO_PROG -c "fsync" $t1
> +
> +# fzero certain range in between
> +$XFS_IO_PROG -c "fzero -k  $((40*$bs)) $((20*$bs))" $t1
> +
> +# fsync t1
> +$XFS_IO_PROG -c "fsync" $t1
> +
> +# shutdown FS now for replay of journal to kick during next mount
> +_scratch_shutdown -v >> $seqres.full 2>&1
> +
> +_scratch_cycle_mount
> +
> +# check fiemap reported is valid or not
> +$XFS_IO_PROG -c "fiemap -v" $t1 | _filter_fiemap_flags $bs
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/677.out b/tests/generic/677.out
> new file mode 100644
> index 00000000..b91ab77a
> --- /dev/null
> +++ b/tests/generic/677.out
> @@ -0,0 +1,6 @@
> +QA output created by 677
> +wrote XXXX/XXXX bytes at offset XXXX
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +0: [0..39]: none
> +1: [40..59]: unwritten
> +2: [60..99]: nonelast
> --
> 2.31.1
> 

Re: [PATCHv2 1/4] generic/468: Add another falloc test entry

[Ritesh Harjani]

First of all thanks Darrick for the review.
And sorry about such a long delay in getting back to this.

On 22/03/15 09:51AM, Darrick J. Wong wrote:
> On Tue, Mar 15, 2022 at 07:58:56PM +0530, Ritesh Harjani wrote:
> > Add another falloc test entry which could hit a kernel bug
> > with ext4 fast_commit feature w/o below kernel commit [1].
> >
> > <log>
> > [  410.888496][ T2743] BUG: KASAN: use-after-free in ext4_mb_mark_bb+0x26a/0x6c0
> > [  410.890432][ T2743] Read of size 8 at addr ffff888171886000 by task mount/2743
> >
> > This happens when falloc -k size is huge which spans across more than
> > 1 flex block group in ext4. This causes a bug in fast_commit replay
> > code which is fixed by kernel commit at [1].
> >
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d
> >
> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > ---
> >  tests/generic/468     | 4 ++++
> >  tests/generic/468.out | 2 ++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/tests/generic/468 b/tests/generic/468
> > index 95752d3b..cbef9746 100755
> > --- a/tests/generic/468
> > +++ b/tests/generic/468
> > @@ -34,6 +34,9 @@ _scratch_mkfs >/dev/null 2>&1
> >  _require_metadata_journaling $SCRATCH_DEV
> >  _scratch_mount
> >
> > +blocksize=4096
>
> What happens if the file blocksize isn't 4k?  Does fastcommit only
> support one block size?  I didn't think it has any such restriction?

It does support other block size too.
Now, for bs < 4096 it is still fine. Yes, it won't trigger for 64K bs.
But that is ok since anyway to trigger this issue with 64K, we will need a much
larger disk size that anyway folks doesn't run fstests with
$((32768*65536*18))


>
> > +fact=18
>
> This needs a bit more explanation -- why 18?  I think the reason is that
> you need the fallocate to cross into another flexbg, and flexbgs (by
> default) are 16bg long, right?

That is right. Sure, I will add a comment explaining this, in the next revision.

>
> If that's the case, then don't you need to detect the flexbg size so
> that this is still an effective test if someone runs fstests with
> MKFS_OPTIONS='-G 32' or something?

We can do that. But it is rare for anyone to test only with '-G 32' and not test
with a default option. 32 might still be ok, but anything bigger then that
might still require the test to not run due to requirement of disk size.

And plus I wanted to keep the test generic enough such that it covers the
regression test with default fast_commit mkfs option.

-ritesh

>
> --D
>
> > +
> >  testfile=$SCRATCH_MNT/testfile
> >
> >  # check inode metadata after shutdown
> > @@ -85,6 +88,7 @@ for i in fsync fdatasync; do
> >  	test_falloc $i "-k " 1024
> >  	test_falloc $i "-k " 4096
> >  	test_falloc $i "-k " 104857600
> > +	test_falloc $i "-k " $((32768*$blocksize*$fact))
> >  done
> >
> >  status=0
> > diff --git a/tests/generic/468.out b/tests/generic/468.out
> > index b3a28d5e..a09cedb8 100644
> > --- a/tests/generic/468.out
> > +++ b/tests/generic/468.out
> > @@ -5,9 +5,11 @@ QA output created by 468
> >  ==== falloc -k 1024 test with fsync ====
> >  ==== falloc -k 4096 test with fsync ====
> >  ==== falloc -k 104857600 test with fsync ====
> > +==== falloc -k 2415919104 test with fsync ====
> >  ==== falloc 1024 test with fdatasync ====
> >  ==== falloc 4096 test with fdatasync ====
> >  ==== falloc 104857600 test with fdatasync ====
> >  ==== falloc -k 1024 test with fdatasync ====
> >  ==== falloc -k 4096 test with fdatasync ====
> >  ==== falloc -k 104857600 test with fdatasync ====
> > +==== falloc -k 2415919104 test with fdatasync ====
> > --
> > 2.31.1
> >

Re: [PATCHv2 3/4] generic/676: Add a new shutdown recovery test

[Ritesh Harjani]

On 22/03/15 09:55AM, Darrick J. Wong wrote:
> On Tue, Mar 15, 2022 at 07:58:58PM +0530, Ritesh Harjani wrote:
> > In certain cases (it is noted with ext4 fast_commit feature) that, replay phase
> > may not delete the right range of blocks (after sudden FS shutdown)
> > due to some operations which depends on inode->i_size (which during replay of
> > an inode with fast_commit could be 0 for sometime).
> > This fstest is added to test for such scenarios for all generic fs.
> >
> > This test case is based on the test case shared via Xin Yin.
> >
> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > ---
> >  tests/generic/676     | 72 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/676.out |  7 +++++
> >  2 files changed, 79 insertions(+)
> >  create mode 100755 tests/generic/676
> >  create mode 100644 tests/generic/676.out
> >
> > diff --git a/tests/generic/676 b/tests/generic/676
> > new file mode 100755
> > index 00000000..315edcdf
> > --- /dev/null
> > +++ b/tests/generic/676
> > @@ -0,0 +1,72 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
> > +#
> > +# FS QA Test 676
> > +#
> > +# This test with ext4 fast_commit feature w/o below patch missed to delete the right
> > +# range during replay phase, since it depends upon inode->i_size (which might not be
> > +# stable during replay phase, at least for ext4).
> > +# 0b5b5a62b945a141: ext4: use ext4_ext_remove_space() for fast commit replay delete range
> > +# (Based on test case shared by Xin Yin <yinxin.x@bytedance.com>)
> > +#
> > +
> > +. ./common/preamble
> > +_begin_fstest auto shutdown quick log recoveryloop
>
> This isn't a looping recovery test.  Maybe we should create a 'recovery'
> group for tests that only run once?  I think we already have a few
> fstests like that.

I gave it a thought, but I feel it might be unncessary.
From a developer/tester perspective who wanted to test anything related to
recovery would then have to use both recovery and recoveryloop.
Thoughts?

>
> > +
> > +# Override the default cleanup function.
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -r -f $tmp.*
> > +   _scratch_unmount > /dev/null 2>&1
>
> I think the test harness does this for you already, right?

Although, it looks like after running the test by default the run_section() in
check script, will do _test_unmount and _scratch_unmount.
But I still feel it's better if the individual test cleans up whatever it did
while running the test in it's cleanup routine, before exiting.

>
> > +}
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +. ./common/punch
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_require_scratch
> > +_require_xfs_io_command "fpunch"
> > +_require_xfs_io_command "fzero"
> > +_require_xfs_io_command "fiemap"
>
> _require_scratch_shutdown
>
> > +
> > +t1=$SCRATCH_MNT/foo
> > +t2=$SCRATCH_MNT/bar
> > +
> > +_scratch_mkfs > $seqres.full 2>&1
> > +
> > +_scratch_mount >> $seqres.full 2>&1
> > +
> > +bs=$(_get_block_size $SCRATCH_MNT)
>
> _get_file_block_size, in case the file allocation unit isn't the same as
> the fs blocksize?  (e.g. bigalloc, xfs realtime, etc.)

Sure. Agreed. Will make the change.

Thanks
-ritesh

Re: [PATCHv2 4/4] generic/677: Add a test to check unwritten extents tracking

[Ritesh Harjani]

On 22/03/15 09:56AM, Darrick J. Wong wrote:
> On Tue, Mar 15, 2022 at 07:58:59PM +0530, Ritesh Harjani wrote:
> > With these sequence of operation (in certain cases like with ext4 fast_commit)
> > could miss to track unwritten extents during replay phase
> > (after sudden FS shutdown).
> >
> > This fstest adds a test case to test this.
> >
> > 5e4d0eba1ccaf19f
> > ext4: fix fast commit may miss tracking range for FALLOC_FL_ZERO_RANGE
> >
> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > ---
> >  tests/generic/677     | 64 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/677.out |  6 ++++
> >  2 files changed, 70 insertions(+)
> >  create mode 100755 tests/generic/677
> >  create mode 100644 tests/generic/677.out
> >
> > diff --git a/tests/generic/677 b/tests/generic/677
> > new file mode 100755
> > index 00000000..e316763a
> > --- /dev/null
> > +++ b/tests/generic/677
> > @@ -0,0 +1,64 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
> > +#
> > +# FS QA Test 677
> > +#
> > +# Test below sequence of operation which (w/o below kernel patch) in case of
> > +# ext4 with fast_commit may misss to track unwritten extents.
> > +# commit 5e4d0eba1ccaf19f
> > +# ext4: fix fast commit may miss tracking range for FALLOC_FL_ZERO_RANGE
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick log shutdown recoveryloop
> > +
> > +# Override the default cleanup function.
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -r -f $tmp.*
> > +}
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +. ./common/punch
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_require_scratch
> > +_require_xfs_io_command "fzero"
> > +_require_xfs_io_command "fiemap"
> > +
> > +t1=$SCRATCH_MNT/t1
> > +
> > +_scratch_mkfs > $seqres.full 2>&1
> > +
> > +_scratch_mount >> $seqres.full 2>&1
> > +
> > +bs=$(_get_block_size $SCRATCH_MNT)
>
> Same comments about blocksize, group names, and
> _require_scratch_shutdown as the last patch.

Sure, I will fix blocksize and _require_scratch_shutdown part.
I have added my thoughts above group names in previous patch.

Thanks for reviewing this.

-ritesh

Re: [PATCHv2 3/4] generic/676: Add a new shutdown recovery test

[Ritesh Harjani]

On 22/03/29 05:02PM, Ritesh Harjani wrote:
> On 22/03/15 09:55AM, Darrick J. Wong wrote:
> > On Tue, Mar 15, 2022 at 07:58:58PM +0530, Ritesh Harjani wrote:
> > > In certain cases (it is noted with ext4 fast_commit feature) that, replay phase
> > > may not delete the right range of blocks (after sudden FS shutdown)
> > > due to some operations which depends on inode->i_size (which during replay of
> > > an inode with fast_commit could be 0 for sometime).
> > > This fstest is added to test for such scenarios for all generic fs.
> > >
> > > This test case is based on the test case shared via Xin Yin.
> > >
> > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > > ---
> > >  tests/generic/676     | 72 +++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/676.out |  7 +++++
> > >  2 files changed, 79 insertions(+)
> > >  create mode 100755 tests/generic/676
> > >  create mode 100644 tests/generic/676.out
> > >
> > > diff --git a/tests/generic/676 b/tests/generic/676
> > > new file mode 100755
> > > index 00000000..315edcdf
> > > --- /dev/null
> > > +++ b/tests/generic/676
> > > @@ -0,0 +1,72 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
> > > +#
> > > +# FS QA Test 676
> > > +#
> > > +# This test with ext4 fast_commit feature w/o below patch missed to delete the right
> > > +# range during replay phase, since it depends upon inode->i_size (which might not be
> > > +# stable during replay phase, at least for ext4).
> > > +# 0b5b5a62b945a141: ext4: use ext4_ext_remove_space() for fast commit replay delete range
> > > +# (Based on test case shared by Xin Yin <yinxin.x@bytedance.com>)
> > > +#
> > > +
> > > +. ./common/preamble
> > > +_begin_fstest auto shutdown quick log recoveryloop
> >
> > This isn't a looping recovery test.  Maybe we should create a 'recovery'
> > group for tests that only run once?  I think we already have a few
> > fstests like that.
>
> I gave it a thought, but I feel it might be unncessary.
> From a developer/tester perspective who wanted to test anything related to
> recovery would then have to use both recovery and recoveryloop.
> Thoughts?
>
> >
> > > +
> > > +# Override the default cleanup function.
> > > +_cleanup()
> > > +{
> > > +	cd /
> > > +	rm -r -f $tmp.*
> > > +   _scratch_unmount > /dev/null 2>&1
> >
> > I think the test harness does this for you already, right?

Ok, I agree with this. I will remove _scratch_unmount operation
from these two new tests in v3.

-ritesh