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

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

[Ritesh Harjani]

Hello,

The ext4 fast_commit kernel fix has landed into mainline tree [1].
In this v3, I have addressed review comments from Darrick.
Does this looks good to be picked up?

I have tested ext4 1k, 4k (w & w/o fast_commit). Also tested other FS with
default configs (like xfs, btrfs, f2fs). No surprises were seen.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d

-ritesh

Changelogs:
===========

v2 => v3
=========
1. Addressed review comments from Darrick.
2. Rebased to latest master.

v1 => v2
=========
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/.

Summary
=========
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.

[v2]: https://lore.kernel.org/all/cover.1647342932.git.riteshh@linux.ibm.com/
[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/678: Add a new shutdown recovery test
  generic/679: Add a test to check unwritten extents tracking

 common/punch          |  9 +++---
 tests/generic/468     |  8 +++++
 tests/generic/468.out |  2 ++
 tests/generic/678     | 72 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/678.out |  7 +++++
 tests/generic/679     | 65 ++++++++++++++++++++++++++++++++++++++
 tests/generic/679.out |  6 ++++
 7 files changed, 165 insertions(+), 4 deletions(-)
 create mode 100755 tests/generic/678
 create mode 100644 tests/generic/678.out
 create mode 100755 tests/generic/679
 create mode 100644 tests/generic/679.out

--
2.31.1

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

[Ritesh Harjani]

From: Ritesh Harjani <riteshh@linux.ibm.com>

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     | 8 ++++++++
 tests/generic/468.out | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/tests/generic/468 b/tests/generic/468
index 95752d3b..5e73cff9 100755
--- a/tests/generic/468
+++ b/tests/generic/468
@@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1
 _require_metadata_journaling $SCRATCH_DEV
 _scratch_mount
 
+# blocksize and fact are used in the last case of the fsync/fdatasync test.
+# This is mainly trying to test recovery operation in case where the data
+# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.
+blocks=32768
+blocksize=4096
+fact=18
+
 testfile=$SCRATCH_MNT/testfile
 
 # check inode metadata after shutdown
@@ -85,6 +92,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 " $(($blocks*$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

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

[Ritesh Harjani]

From: Ritesh Harjani <riteshh@linux.ibm.com>

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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/common/punch b/common/punch
index b6b8a0b9..706e7c11 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,19 +136,19 @@ _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()
 {
 	$AWK_PROG '
 		$3 ~ /hole/ {
-			print $1, $2, $3; 
+			print $1, $2, $3;
 			next;
-		}   
+		}
 		$5 ~ /0x[[:xdigit:]]+/ {
 			print $1, $2, "extent";
 		}' |
-- 
2.31.1

[PATCHv3 3/4] generic/678: Add a new shutdown recovery test

[Ritesh Harjani]

From: Ritesh Harjani <riteshh@linux.ibm.com>

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/678     | 72 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/678.out |  7 +++++
 2 files changed, 79 insertions(+)
 create mode 100755 tests/generic/678
 create mode 100644 tests/generic/678.out

diff --git a/tests/generic/678 b/tests/generic/678
new file mode 100755
index 00000000..46a7be6c
--- /dev/null
+++ b/tests/generic/678
@@ -0,0 +1,72 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
+#
+# FS QA Test 678
+#
+# 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.*
+}
+
+# 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_file_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/678.out b/tests/generic/678.out
new file mode 100644
index 00000000..e0992edd
--- /dev/null
+++ b/tests/generic/678.out
@@ -0,0 +1,7 @@
+QA output created by 678
+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

[PATCHv3 4/4] generic/679: Add a test to check unwritten extents tracking

[Ritesh Harjani]

From: Ritesh Harjani <riteshh@linux.ibm.com>

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/679     | 65 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/679.out |  6 ++++
 2 files changed, 71 insertions(+)
 create mode 100755 tests/generic/679
 create mode 100644 tests/generic/679.out

diff --git a/tests/generic/679 b/tests/generic/679
new file mode 100755
index 00000000..4f35a9cd
--- /dev/null
+++ b/tests/generic/679
@@ -0,0 +1,65 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
+#
+# FS QA Test 679
+#
+# 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"
+_require_scratch_shutdown
+
+t1=$SCRATCH_MNT/t1
+
+_scratch_mkfs > $seqres.full 2>&1
+
+_scratch_mount >> $seqres.full 2>&1
+
+bs=$(_get_file_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/679.out b/tests/generic/679.out
new file mode 100644
index 00000000..4d3c3377
--- /dev/null
+++ b/tests/generic/679.out
@@ -0,0 +1,6 @@
+QA output created by 679
+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: [PATCHv3 0/4] generic: Add some tests around journal replay/recoveryloop

[Zorro Lang]

On Thu, Mar 31, 2022 at 06:24:19PM +0530, Ritesh Harjani wrote:
> Hello,

Hi,

Your below patches looks like not pure text format, they might contain
binary character or some special characers, looks like the "^M" [1].

So please check how you generate/edit/send these patches, make sure
they're pure text for Linux then send again.

Thanks,
Zorro


[1]
# cat -A /path/to/your_patch
index 95752d3b..5e73cff9 100755^M$
--- a/tests/generic/468^M$
+++ b/tests/generic/468^M$
@@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1^M$
 _require_metadata_journaling $SCRATCH_DEV^M$
 _scratch_mount^M$
 ^M$
+# blocksize and fact are used in the last case of the fsync/fdatasync test.^M$
+# This is mainly trying to test recovery operation in case where the data^M$
+# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.^M$
+blocks=32768^M$
+blocksize=4096^M$
+fact=18^M$
+^M$
...
...

> 
> The ext4 fast_commit kernel fix has landed into mainline tree [1].
> In this v3, I have addressed review comments from Darrick.
> Does this looks good to be picked up?
> 
> I have tested ext4 1k, 4k (w & w/o fast_commit). Also tested other FS with
> default configs (like xfs, btrfs, f2fs). No surprises were seen.
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d
> 
> -ritesh
> 
> Changelogs:
> ===========
> 
> v2 => v3
> =========
> 1. Addressed review comments from Darrick.
> 2. Rebased to latest master.
> 
> v1 => v2
> =========
> 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/.
> 
> Summary
> =========
> 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.
> 
> [v2]: https://lore.kernel.org/all/cover.1647342932.git.riteshh@linux.ibm.com/
> [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/678: Add a new shutdown recovery test
>   generic/679: Add a test to check unwritten extents tracking
> 
>  common/punch          |  9 +++---
>  tests/generic/468     |  8 +++++
>  tests/generic/468.out |  2 ++
>  tests/generic/678     | 72 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/678.out |  7 +++++
>  tests/generic/679     | 65 ++++++++++++++++++++++++++++++++++++++
>  tests/generic/679.out |  6 ++++
>  7 files changed, 165 insertions(+), 4 deletions(-)
>  create mode 100755 tests/generic/678
>  create mode 100644 tests/generic/678.out
>  create mode 100755 tests/generic/679
>  create mode 100644 tests/generic/679.out
> 
> --
> 2.31.1
> 

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

[Ritesh Harjani]

On 22/03/31 10:59PM, Zorro Lang wrote:
> On Thu, Mar 31, 2022 at 06:24:19PM +0530, Ritesh Harjani wrote:
> > Hello,
>
> Hi,
>
> Your below patches looks like not pure text format, they might contain
> binary character or some special characers, looks like the "^M" [1].

Ohh, is it. Might be some settings needed to be tweaked for using
gmail for community patches.

>
> So please check how you generate/edit/send these patches, make sure
> they're pure text for Linux then send again.

Sure Zorro, I will check it again at my end. However, when I tried using
b4 am -o - cover.1648730443.git.ritesh.list@gmail.com | git am

It worked fine for me and applied all 4 patches to my local tree.

I also didn't notice any of such characters in my other inbox
("riteshh@in.ibm.com"), which was cc'd too. hmm, strange.

Sure, I will check it once again at my end and resend this.

-ritesh


>
> Thanks,
> Zorro
>
>
> [1]
> # cat -A /path/to/your_patch
> index 95752d3b..5e73cff9 100755^M$
> --- a/tests/generic/468^M$
> +++ b/tests/generic/468^M$
> @@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1^M$
>  _require_metadata_journaling $SCRATCH_DEV^M$
>  _scratch_mount^M$
>  ^M$
> +# blocksize and fact are used in the last case of the fsync/fdatasync test.^M$
> +# This is mainly trying to test recovery operation in case where the data^M$
> +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.^M$
> +blocks=32768^M$
> +blocksize=4096^M$
> +fact=18^M$
> +^M$
> ...
> ...
>
> >
> > The ext4 fast_commit kernel fix has landed into mainline tree [1].
> > In this v3, I have addressed review comments from Darrick.
> > Does this looks good to be picked up?
> >
> > I have tested ext4 1k, 4k (w & w/o fast_commit). Also tested other FS with
> > default configs (like xfs, btrfs, f2fs). No surprises were seen.
> >
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d
> >
> > -ritesh
> >
> > Changelogs:
> > ===========
> >
> > v2 => v3
> > =========
> > 1. Addressed review comments from Darrick.
> > 2. Rebased to latest master.
> >
> > v1 => v2
> > =========
> > 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/.
> >
> > Summary
> > =========
> > 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.
> >
> > [v2]: https://lore.kernel.org/all/cover.1647342932.git.riteshh@linux.ibm.com/
> > [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/678: Add a new shutdown recovery test
> >   generic/679: Add a test to check unwritten extents tracking
> >
> >  common/punch          |  9 +++---
> >  tests/generic/468     |  8 +++++
> >  tests/generic/468.out |  2 ++
> >  tests/generic/678     | 72 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/678.out |  7 +++++
> >  tests/generic/679     | 65 ++++++++++++++++++++++++++++++++++++++
> >  tests/generic/679.out |  6 ++++
> >  7 files changed, 165 insertions(+), 4 deletions(-)
> >  create mode 100755 tests/generic/678
> >  create mode 100644 tests/generic/678.out
> >  create mode 100755 tests/generic/679
> >  create mode 100644 tests/generic/679.out
> >
> > --
> > 2.31.1
> >
>

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

[Ritesh Harjani]

On 22/03/31 09:49PM, Ritesh Harjani wrote:
> On 22/03/31 10:59PM, Zorro Lang wrote:
> > On Thu, Mar 31, 2022 at 06:24:19PM +0530, Ritesh Harjani wrote:
> > > Hello,
> >
> > Hi,
> >
> > Your below patches looks like not pure text format, they might contain
> > binary character or some special characers, looks like the "^M" [1].

Sorry to bother you. But here is what I tried.
1. Download the mbx file using b4 am. I didn't see any such character ("^M") in
   the patches.
2. Saved the patch using mutt. Again didn't see such character while doing
	cat -A /patch/to/patch
3. Downloaded the mail using eml format from webmail. Here I do see this
   character appended. But that happens not just for my patch, but for all
   other patches too.

So could this be related to the way you are downloading these patches.
Please let me know, if I need to resend these patches again? Because, I don't
see this behavior at my end. But I would happy to correct it, if that's not the
case.

-ritesh

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

[Zorro Lang]

On Thu, Mar 31, 2022 at 10:23:35PM +0530, Ritesh Harjani wrote:
> On 22/03/31 09:49PM, Ritesh Harjani wrote:
> > On 22/03/31 10:59PM, Zorro Lang wrote:
> > > On Thu, Mar 31, 2022 at 06:24:19PM +0530, Ritesh Harjani wrote:
> > > > Hello,
> > >
> > > Hi,
> > >
> > > Your below patches looks like not pure text format, they might contain
> > > binary character or some special characers, looks like the "^M" [1].
> 
> Sorry to bother you. But here is what I tried.
> 1. Download the mbx file using b4 am. I didn't see any such character ("^M") in
>    the patches.
> 2. Saved the patch using mutt. Again didn't see such character while doing
> 	cat -A /patch/to/patch
> 3. Downloaded the mail using eml format from webmail. Here I do see this
>    character appended. But that happens not just for my patch, but for all
>    other patches too.
> 
> So could this be related to the way you are downloading these patches.
> Please let me know, if I need to resend these patches again? Because, I don't
> see this behavior at my end. But I would happy to correct it, if that's not the
> case.

Hmm... weird, When I tried to open your patch emails, my mutt show me:

  [-- application/octet-stream is unsupported (use 'v' to view this part) --]

Then I have to input 'v' to see the patch content. I'm not sure what's wrong,
this's the 2nd time I hit this "octet-stream is unsupported" issue yesterday.

Hi Darrick, or any other forks, can you open above 4 patches normally? If that's
only my personal issue, I'll check my side.

Thanks,
Zorro

> 
> -ritesh
> 

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

[Ojaswin Mujoo]

On Fri, Apr 01, 2022 at 01:30:47PM +0800, Zorro Lang wrote:
> On Thu, Mar 31, 2022 at 10:23:35PM +0530, Ritesh Harjani wrote:
> > On 22/03/31 09:49PM, Ritesh Harjani wrote:
> > > On 22/03/31 10:59PM, Zorro Lang wrote:
> > > > On Thu, Mar 31, 2022 at 06:24:19PM +0530, Ritesh Harjani wrote:
> > > > > Hello,
> > > >
> > > > Hi,
> > > >
> > > > Your below patches looks like not pure text format, they might contain
> > > > binary character or some special characers, looks like the "^M" [1].
> > 
> > Sorry to bother you. But here is what I tried.
> > 1. Download the mbx file using b4 am. I didn't see any such character ("^M") in
> >    the patches.
> > 2. Saved the patch using mutt. Again didn't see such character while doing
> > 	cat -A /patch/to/patch
> > 3. Downloaded the mail using eml format from webmail. Here I do see this
> >    character appended. But that happens not just for my patch, but for all
> >    other patches too.
> > 
> > So could this be related to the way you are downloading these patches.
> > Please let me know, if I need to resend these patches again? Because, I don't
> > see this behavior at my end. But I would happy to correct it, if that's not the
> > case.
> 
> Hmm... weird, When I tried to open your patch emails, my mutt show me:
> 
>   [-- application/octet-stream is unsupported (use 'v' to view this part) --]
> 
> Then I have to input 'v' to see the patch content. I'm not sure what's wrong,
> this's the 2nd time I hit this "octet-stream is unsupported" issue yesterday.
> 
> Hi Darrick, or any other forks, can you open above 4 patches normally? If that's
> only my personal issue, I'll check my side.
Hi Zorro,

The patchset seems to open normally at my end.

Thanks,
Ojaswin
> 
> Thanks,
> Zorro
> 
> > 
> > -ritesh
> > 
> 

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

[Darrick J. Wong]

On Fri, Apr 01, 2022 at 01:30:47PM +0800, Zorro Lang wrote:
> On Thu, Mar 31, 2022 at 10:23:35PM +0530, Ritesh Harjani wrote:
> > On 22/03/31 09:49PM, Ritesh Harjani wrote:
> > > On 22/03/31 10:59PM, Zorro Lang wrote:
> > > > On Thu, Mar 31, 2022 at 06:24:19PM +0530, Ritesh Harjani wrote:
> > > > > Hello,
> > > >
> > > > Hi,
> > > >
> > > > Your below patches looks like not pure text format, they might contain
> > > > binary character or some special characers, looks like the "^M" [1].
> > 
> > Sorry to bother you. But here is what I tried.
> > 1. Download the mbx file using b4 am. I didn't see any such character ("^M") in
> >    the patches.
> > 2. Saved the patch using mutt. Again didn't see such character while doing
> > 	cat -A /patch/to/patch
> > 3. Downloaded the mail using eml format from webmail. Here I do see this
> >    character appended. But that happens not just for my patch, but for all
> >    other patches too.
> > 
> > So could this be related to the way you are downloading these patches.
> > Please let me know, if I need to resend these patches again? Because, I don't
> > see this behavior at my end. But I would happy to correct it, if that's not the
> > case.
> 
> Hmm... weird, When I tried to open your patch emails, my mutt show me:
> 
>   [-- application/octet-stream is unsupported (use 'v' to view this part) --]
> 
> Then I have to input 'v' to see the patch content. I'm not sure what's wrong,
> this's the 2nd time I hit this "octet-stream is unsupported" issue yesterday.
> 
> Hi Darrick, or any other forks, can you open above 4 patches normally? If that's
> only my personal issue, I'll check my side.

There's no application/octet anywhere in the email that I received.
Has your IT department gone rogue^W^Wincreased value-add again?

--D

> Thanks,
> Zorro
> 
> > 
> > -ritesh
> > 
> 

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

[Zorro Lang]

On Fri, Apr 01, 2022 at 10:04:51AM -0700, Darrick J. Wong wrote:
> On Fri, Apr 01, 2022 at 01:30:47PM +0800, Zorro Lang wrote:
> > On Thu, Mar 31, 2022 at 10:23:35PM +0530, Ritesh Harjani wrote:
> > > On 22/03/31 09:49PM, Ritesh Harjani wrote:
> > > > On 22/03/31 10:59PM, Zorro Lang wrote:
> > > > > On Thu, Mar 31, 2022 at 06:24:19PM +0530, Ritesh Harjani wrote:
> > > > > > Hello,
> > > > >
> > > > > Hi,
> > > > >
> > > > > Your below patches looks like not pure text format, they might contain
> > > > > binary character or some special characers, looks like the "^M" [1].
> > > 
> > > Sorry to bother you. But here is what I tried.
> > > 1. Download the mbx file using b4 am. I didn't see any such character ("^M") in
> > >    the patches.
> > > 2. Saved the patch using mutt. Again didn't see such character while doing
> > > 	cat -A /patch/to/patch
> > > 3. Downloaded the mail using eml format from webmail. Here I do see this
> > >    character appended. But that happens not just for my patch, but for all
> > >    other patches too.
> > > 
> > > So could this be related to the way you are downloading these patches.
> > > Please let me know, if I need to resend these patches again? Because, I don't
> > > see this behavior at my end. But I would happy to correct it, if that's not the
> > > case.
> > 
> > Hmm... weird, When I tried to open your patch emails, my mutt show me:
> > 
> >   [-- application/octet-stream is unsupported (use 'v' to view this part) --]
> > 
> > Then I have to input 'v' to see the patch content. I'm not sure what's wrong,
> > this's the 2nd time I hit this "octet-stream is unsupported" issue yesterday.
> > 
> > Hi Darrick, or any other forks, can you open above 4 patches normally? If that's
> > only my personal issue, I'll check my side.
> 
> There's no application/octet anywhere in the email that I received.
> Has your IT department gone rogue^W^Wincreased value-add again?

Thanks Darrick and Ojaswin! I'll check with our IT department. But it's weird, only
this patchset and 2 of another patch[1](it's sent 3 times) has this problem, I see
all other patches normally. And Ritesh, sorry for taking your time to check it :-D

Thanks,
Zorro

[1]
[PATCH v2] generic/674: replace _require_scratch_reflink with _require_scratch_dedupe

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > 
> > > -ritesh
> > > 
> > 
> 

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

[Dave Chinner]

On Thu, Mar 31, 2022 at 06:24:20PM +0530, Ritesh Harjani wrote:
> From: Ritesh Harjani <riteshh@linux.ibm.com>
> 
> 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     | 8 ++++++++
>  tests/generic/468.out | 2 ++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/tests/generic/468 b/tests/generic/468
> index 95752d3b..5e73cff9 100755
> --- a/tests/generic/468
> +++ b/tests/generic/468
> @@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1
>  _require_metadata_journaling $SCRATCH_DEV
>  _scratch_mount
>  
> +# blocksize and fact are used in the last case of the fsync/fdatasync test.
> +# This is mainly trying to test recovery operation in case where the data
> +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.
> +blocks=32768
> +blocksize=4096

Block size can change based on mkfs parameters. You should extract
this dynamically from the filesystem the test is being run on.

> +fact=18

What is "fact" supposed to mean?

Indeed, wouldn't this simply be better as something like:

larger_than_ext4_fg_size=$((32768 * $blksize * 18))

And then

>  testfile=$SCRATCH_MNT/testfile
>  
>  # check inode metadata after shutdown
> @@ -85,6 +92,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 " $(($blocks*$blocksize*$fact))

	test_falloc $i "-k " $larger_than_ext4_fg_size

And just scrub all the sizes from the golden output?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCHv3 3/4] generic/678: Add a new shutdown recovery test

[Dave Chinner]

On Thu, Mar 31, 2022 at 06:24:22PM +0530, Ritesh Harjani wrote:
> From: Ritesh Harjani <riteshh@linux.ibm.com>
> 
> 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/678     | 72 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/678.out |  7 +++++
>  2 files changed, 79 insertions(+)
>  create mode 100755 tests/generic/678
>  create mode 100644 tests/generic/678.out
> 
> diff --git a/tests/generic/678 b/tests/generic/678
> new file mode 100755
> index 00000000..46a7be6c
> --- /dev/null
> +++ b/tests/generic/678
> @@ -0,0 +1,72 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
> +#
> +# FS QA Test 678
> +#
> +# 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.*
> +}

It's the same as the default cleanup function.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCHv3 4/4] generic/679: Add a test to check unwritten extents tracking

[Dave Chinner]

On Thu, Mar 31, 2022 at 06:24:23PM +0530, Ritesh Harjani wrote:
> From: Ritesh Harjani <riteshh@linux.ibm.com>
> 
> 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/679     | 65 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/679.out |  6 ++++
>  2 files changed, 71 insertions(+)
>  create mode 100755 tests/generic/679
>  create mode 100644 tests/generic/679.out
> 
> diff --git a/tests/generic/679 b/tests/generic/679
> new file mode 100755
> index 00000000..4f35a9cd
> --- /dev/null
> +++ b/tests/generic/679
> @@ -0,0 +1,65 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
> +#
> +# FS QA Test 679
> +#
> +# 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.*
> +}

Same as default.

> +
> +# 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"
> +_require_scratch_shutdown
> +
> +t1=$SCRATCH_MNT/t1
> +
> +_scratch_mkfs > $seqres.full 2>&1
> +
> +_scratch_mount >> $seqres.full 2>&1
> +
> +bs=$(_get_file_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/679.out b/tests/generic/679.out
> new file mode 100644
> index 00000000..4d3c3377
> --- /dev/null
> +++ b/tests/generic/679.out
> @@ -0,0 +1,6 @@
> +QA output created by 679
> +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

This is a subset of the the previous test, and looks like it should
be tested first before adding the second file and punch operation
the previous test adds to this write/zero operations. IOWs, they
look like they could easily be combined into a single test without
losing anything except having an extra test that has to be run...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCHv3 4/4] generic/679: Add a test to check unwritten extents tracking

[Ritesh Harjani]

On 22/04/04 09:43AM, Dave Chinner wrote:
> On Thu, Mar 31, 2022 at 06:24:23PM +0530, Ritesh Harjani wrote:
> > From: Ritesh Harjani <riteshh@linux.ibm.com>
> >
> > 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/679     | 65 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/679.out |  6 ++++
> >  2 files changed, 71 insertions(+)
> >  create mode 100755 tests/generic/679
> >  create mode 100644 tests/generic/679.out
> >
> > diff --git a/tests/generic/679 b/tests/generic/679
> > new file mode 100755
> > index 00000000..4f35a9cd
> > --- /dev/null
> > +++ b/tests/generic/679
> > @@ -0,0 +1,65 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
> > +#
> > +# FS QA Test 679
> > +#
> > +# 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.*
> > +}
>
> Same as default.

ohk. So the same _cleanup() function definition is available in common/preamble
now. So I guess, we can just remove this definition from here completely.

Got it. Thanks for pointing out.

>
> > +
> > +# 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"
> > +_require_scratch_shutdown
> > +
> > +t1=$SCRATCH_MNT/t1
> > +
> > +_scratch_mkfs > $seqres.full 2>&1
> > +
> > +_scratch_mount >> $seqres.full 2>&1
> > +
> > +bs=$(_get_file_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/679.out b/tests/generic/679.out
> > new file mode 100644
> > index 00000000..4d3c3377
> > --- /dev/null
> > +++ b/tests/generic/679.out
> > @@ -0,0 +1,6 @@
> > +QA output created by 679
> > +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
>
> This is a subset of the the previous test, and looks like it should
> be tested first before adding the second file and punch operation
> the previous test adds to this write/zero operations. IOWs, they
> look like they could easily be combined into a single test without
> losing anything except having an extra test that has to be run...

Sure, will look into combining the two in one common generic fstest.

Thanks for the review
-ritesh

Re: [PATCHv3 3/4] generic/678: Add a new shutdown recovery test

[Ritesh Harjani]

On 22/04/04 09:38AM, Dave Chinner wrote:
> On Thu, Mar 31, 2022 at 06:24:22PM +0530, Ritesh Harjani wrote:
> > From: Ritesh Harjani <riteshh@linux.ibm.com>
> >
> > 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/678     | 72 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/678.out |  7 +++++
> >  2 files changed, 79 insertions(+)
> >  create mode 100755 tests/generic/678
> >  create mode 100644 tests/generic/678.out
> >
> > diff --git a/tests/generic/678 b/tests/generic/678
> > new file mode 100755
> > index 00000000..46a7be6c
> > --- /dev/null
> > +++ b/tests/generic/678
> > @@ -0,0 +1,72 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 IBM Corporation.  All Rights Reserved.
> > +#
> > +# FS QA Test 678
> > +#
> > +# 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.*
> > +}
>
> It's the same as the default cleanup function.
>

Sure, will remove this definition from here in next revision.
Since it is available in common/preamble now.

-ritesh

> Cheers,
>
> Dave.
>
> --
> Dave Chinner
> david@fromorbit.com

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

[Ritesh Harjani]

On 22/04/04 09:28AM, Dave Chinner wrote:
> On Thu, Mar 31, 2022 at 06:24:20PM +0530, Ritesh Harjani wrote:
> > From: Ritesh Harjani <riteshh@linux.ibm.com>
> >
> > 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     | 8 ++++++++
> >  tests/generic/468.out | 2 ++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/tests/generic/468 b/tests/generic/468
> > index 95752d3b..5e73cff9 100755
> > --- a/tests/generic/468
> > +++ b/tests/generic/468
> > @@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1
> >  _require_metadata_journaling $SCRATCH_DEV
> >  _scratch_mount
> >
> > +# blocksize and fact are used in the last case of the fsync/fdatasync test.
> > +# This is mainly trying to test recovery operation in case where the data
> > +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.
> > +blocks=32768
> > +blocksize=4096
>
> Block size can change based on mkfs parameters. You should extract
> this dynamically from the filesystem the test is being run on.
>

Yes, but we still have kept just 4096 because, anything bigger than that like
65536 might require a bigger disk size itself to test. The overall size
requirement of the disk will then become ~36G (32768 * 65536 * 18)
Hence I went ahead with 4096 which is good enough for testing.

But sure, I will add a comment explaining why we have hardcoded it to 4096
so that others don't get confused. Larger than this size disk anyway doesn't get
tested much right?


> > +fact=18
>
> What is "fact" supposed to mean?
>
> Indeed, wouldn't this simply be better as something like:
>
> larger_than_ext4_fg_size=$((32768 * $blksize * 18))
>
> And then
>
> >  testfile=$SCRATCH_MNT/testfile
> >
> >  # check inode metadata after shutdown
> > @@ -85,6 +92,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 " $(($blocks*$blocksize*$fact))
>
> 	test_falloc $i "-k " $larger_than_ext4_fg_size
>

Yes, looks good to me. Thanks for suggestion.


> And just scrub all the sizes from the golden output?
>

This won't be needed since I still would like to go with 4096 blocksize,
to avoid a large disk size requirement which anyway won't be tested much.

If this sounds good to you, I will fix rest of the changes as discussed in
the next revision.

-ritesh

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

[Theodore Ts'o]

On Tue, Apr 05, 2022 at 04:36:03PM +0530, Ritesh Harjani wrote:
> > > +# blocksize and fact are used in the last case of the fsync/fdatasync test.
> > > +# This is mainly trying to test recovery operation in case where the data
> > > +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.
> > > +blocks=32768
> > > +blocksize=4096
> >
> > Block size can change based on mkfs parameters. You should extract
> > this dynamically from the filesystem the test is being run on.
> >
> 
> Yes, but we still have kept just 4096 because, anything bigger than that like
> 65536 might require a bigger disk size itself to test. The overall size
> requirement of the disk will then become ~36G (32768 * 65536 * 18)
> Hence I went ahead with 4096 which is good enough for testing.

What if the block size is *smaller*?  For example, I run an ext4/1k
configuration (which is how I test block size > page size on x86 VM's :-).

> But sure, I will add a comment explaining why we have hardcoded it to 4096
> so that others don't get confused. Larger than this size disk anyway doesn't get
> tested much right?

At $WORK we use a 100GB disk by default when running xfstests, and I
wouldn't be surprised if theree are other folks who might use larger
disk sizes.

Maybe test to see whether the scratch disk is too small for the given
parameters and if so skip the test using _notrun?

							- Ted

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

[Dave Chinner]

On Tue, Apr 05, 2022 at 04:36:03PM +0530, Ritesh Harjani wrote:
> On 22/04/04 09:28AM, Dave Chinner wrote:
> > On Thu, Mar 31, 2022 at 06:24:20PM +0530, Ritesh Harjani wrote:
> > > From: Ritesh Harjani <riteshh@linux.ibm.com>
> > >
> > > 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     | 8 ++++++++
> > >  tests/generic/468.out | 2 ++
> > >  2 files changed, 10 insertions(+)
> > >
> > > diff --git a/tests/generic/468 b/tests/generic/468
> > > index 95752d3b..5e73cff9 100755
> > > --- a/tests/generic/468
> > > +++ b/tests/generic/468
> > > @@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1
> > >  _require_metadata_journaling $SCRATCH_DEV
> > >  _scratch_mount
> > >
> > > +# blocksize and fact are used in the last case of the fsync/fdatasync test.
> > > +# This is mainly trying to test recovery operation in case where the data
> > > +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.
> > > +blocks=32768
> > > +blocksize=4096
> >
> > Block size can change based on mkfs parameters. You should extract
> > this dynamically from the filesystem the test is being run on.
> >
> 
> Yes, but we still have kept just 4096 because, anything bigger than that like
> 65536 might require a bigger disk size itself to test. The overall size
> requirement of the disk will then become ~36G (32768 * 65536 * 18)
> Hence I went ahead with 4096 which is good enough for testing.

If the test setup doesn't have a disk large enough, then the test
should be skipped. That's what '_require_scratch_size' is for.

i.e. _require_scratch_size $larger_than_ext4_fg_size

Will do that check once we've calculated the size needed.

> But sure, I will add a comment explaining why we have hardcoded it to 4096
> so that others don't get confused. Larger than this size disk anyway doesn't get
> tested much right?

You shouldn't be constricting the test based on assumptions about
test configurations. If someone decides to test 64k block size, then
they can size their devices appropriately for the configuration they
want to test.  If a 64kB block size filesystem can overrun the
on-disk structure and fail, then the test should exercise that and
fail appropriately.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Ritesh Harjani]

On 22/04/05 06:00PM, Theodore Ts'o wrote:
> On Tue, Apr 05, 2022 at 04:36:03PM +0530, Ritesh Harjani wrote:
> > > > +# blocksize and fact are used in the last case of the fsync/fdatasync test.
> > > > +# This is mainly trying to test recovery operation in case where the data
> > > > +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.
> > > > +blocks=32768
> > > > +blocksize=4096
> > >
> > > Block size can change based on mkfs parameters. You should extract
> > > this dynamically from the filesystem the test is being run on.
> > >
> >
> > Yes, but we still have kept just 4096 because, anything bigger than that like
> > 65536 might require a bigger disk size itself to test. The overall size
> > requirement of the disk will then become ~36G (32768 * 65536 * 18)
> > Hence I went ahead with 4096 which is good enough for testing.
>
> What if the block size is *smaller*?  For example, I run an ext4/1k
> configuration (which is how I test block size > page size on x86 VM's :-).

For 1k bs, this test can still reproduce the problem. Because the given size
will easily overflow the required number of blocks in 1K case.

>
> > But sure, I will add a comment explaining why we have hardcoded it to 4096
> > so that others don't get confused. Larger than this size disk anyway doesn't get
> > tested much right?
>
> At $WORK we use a 100GB disk by default when running xfstests, and I
> wouldn't be surprised if theree are other folks who might use larger
> disk sizes.

Ohk, sure. Thanks for the info.

>
> Maybe test to see whether the scratch disk is too small for the given
> parameters and if so skip the test using _notrun?
>

Yes, I think I got the point. I will make the changes accordingly.

-ritesh

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

[Ritesh Harjani]

On 22/04/06 02:05PM, Dave Chinner wrote:
> On Tue, Apr 05, 2022 at 04:36:03PM +0530, Ritesh Harjani wrote:
> > On 22/04/04 09:28AM, Dave Chinner wrote:
> > > On Thu, Mar 31, 2022 at 06:24:20PM +0530, Ritesh Harjani wrote:
> > > > From: Ritesh Harjani <riteshh@linux.ibm.com>
> > > >
> > > > 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     | 8 ++++++++
> > > >  tests/generic/468.out | 2 ++
> > > >  2 files changed, 10 insertions(+)
> > > >
> > > > diff --git a/tests/generic/468 b/tests/generic/468
> > > > index 95752d3b..5e73cff9 100755
> > > > --- a/tests/generic/468
> > > > +++ b/tests/generic/468
> > > > @@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1
> > > >  _require_metadata_journaling $SCRATCH_DEV
> > > >  _scratch_mount
> > > >
> > > > +# blocksize and fact are used in the last case of the fsync/fdatasync test.
> > > > +# This is mainly trying to test recovery operation in case where the data
> > > > +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.
> > > > +blocks=32768
> > > > +blocksize=4096
> > >
> > > Block size can change based on mkfs parameters. You should extract
> > > this dynamically from the filesystem the test is being run on.
> > >
> >
> > Yes, but we still have kept just 4096 because, anything bigger than that like
> > 65536 might require a bigger disk size itself to test. The overall size
> > requirement of the disk will then become ~36G (32768 * 65536 * 18)
> > Hence I went ahead with 4096 which is good enough for testing.
>
> If the test setup doesn't have a disk large enough, then the test
> should be skipped. That's what '_require_scratch_size' is for.
>
> i.e. _require_scratch_size $larger_than_ext4_fg_size
>
> Will do that check once we've calculated the size needed.

Sure.

>
> > But sure, I will add a comment explaining why we have hardcoded it to 4096
> > so that others don't get confused. Larger than this size disk anyway doesn't get
> > tested much right?
>
> You shouldn't be constricting the test based on assumptions about
> test configurations. If someone decides to test 64k block size, then
> they can size their devices appropriately for the configuration they
> want to test.  If a 64kB block size filesystem can overrun the
> on-disk structure and fail, then the test should exercise that and
> fail appropriately.

Sure Dave. Got the point. I will try and make the changes, such that
test doesn't assume any particular user test configuration. And be generic as
much as possible so that we could hit the issue we are aiming via this test.

-ritesh