[RFC 1/2] ext4/060: Regression test against dioread_nolock mount option inconsistency

[RFC 1/2] ext4/060: Regression test against dioread_nolock mount option inconsistency

[Ritesh Harjani (IBM)]

During ext4_writepages, ext4 queries dioread_nolock mount option twice
and if someone remount the filesystem in between with ^dioread_nolock,
then this can cause an inconsistency causing WARN_ON() to be triggered.

This fix describes the problem in more detail -

https://lore.kernel.org/linux-ext4/20230328090534.662l7yxj2e425j7w@quack3/T/#md19c34646e8b4a816498532c298a66ecf2ae77d4

This test reproduces below warning for me w/o the fix.

------------[ cut here ]------------
WARNING: CPU: 2 PID: 26 at fs/ext4/page-io.c:231 ext4_put_io_end_defer+0xfb/0x140
Modules linked in:
CPU: 2 PID: 26 Comm: ksoftirqd/2 Not tainted 6.3.0-rc6-xfstests-00044-ga5c68786f1b1 #23
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014
RIP: 0010:ext4_put_io_end_defer+0xfb/0x140
Code: 5d 41 5e 41 5f e9 a5 73 d0 00 5b 48 89 ef 5d 41 5c 41 5d 41 5e 41 5f e9 d3 fa ff ff 49 83 be a8 03 00 00 00 0f 84 7b ff fd
<...>
Call Trace:
 <TASK>
 blk_update_request+0x116/0x4c0
 ? finish_task_switch.isra.0+0xfb/0x320
 blk_mq_end_request+0x1e/0x40
 blk_complete_reqs+0x40/0x50
 __do_softirq+0xd8/0x3e1
 ? smpboot_thread_fn+0x30/0x280
 run_ksoftirqd+0x3a/0x60
 smpboot_thread_fn+0x1d8/0x280
 ? __pfx_smpboot_thread_fn+0x10/0x10
 kthread+0xf6/0x120
 ? __pfx_kthread+0x10/0x10
 ret_from_fork+0x2c/0x50
 </TASK>
[

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 tests/ext4/060     | 88 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/ext4/060.out |  2 ++
 2 files changed, 90 insertions(+)
 create mode 100755 tests/ext4/060
 create mode 100644 tests/ext4/060.out

diff --git a/tests/ext4/060 b/tests/ext4/060
new file mode 100755
index 00000000..d9fe1a99
--- /dev/null
+++ b/tests/ext4/060
@@ -0,0 +1,88 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2023 IBM Corporation.  All Rights Reserved.
+#
+# FS QA Test 060
+#
+# This is to test a ext4 regression against inconsistent values of
+# dioread_nolock mount option while in ext4_writepages path.
+# See - https://lore.kernel.org/linux-ext4/20230328090534.662l7yxj2e425j7w@quack3/T/#md19c34646e8b4a816498532c298a66ecf2ae77d4
+#
+. ./common/preamble
+_begin_fstest auto quick
+
+PID1=""
+PIDS=""
+trap "_cleanup; exit \$status" 0 1 2 3 15
+# Override the default cleanup function.
+ _cleanup()
+{
+	{
+		kill -SIGKILL $PID1 $PIDS
+		wait $PID1 $PIDS
+	} > /dev/null 2>&1
+
+	cd /
+	rm -r -f $tmp.*
+}
+
+# Import common functions.
+ . ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs ext4
+_require_scratch
+
+_scratch_mkfs_ext4 >> $seqres.full 2>&1
+_scratch_mount
+_scratch_remount "dioread_nolock" >> $seqres.full 2>&1
+ret=$?
+if [ $ret -ne 0 ]; then
+	_notrun "dioread_nolock mount option not supported"
+fi
+
+testfile=$SCRATCH_MNT/testfile
+
+function run_buff_io_loop()
+{
+	# add buffered io case here
+	while [ 1 ]; do
+		xfs_io -fc "truncate 0" -c "pwrite 0 200M" -c "fsync" "$testfile.$1" > /dev/null 2>&1
+		sleep 2;
+	done
+}
+
+function run_remount_loop()
+{
+	# add remount loop case here
+	while [ 1 ]; do
+		_scratch_remount "dioread_nolock" >> $seqres.full 2>&1
+		sleep 1
+		_scratch_remount "dioread_lock" >> $seqres.full 2>&1
+		sleep 1
+	done
+}
+
+run_remount_loop &
+PID1=$!
+
+for i in $(seq 1 20); do
+	run_buff_io_loop $i &
+	PID=$!
+	PIDS="${PIDS} ${PID}"
+done
+
+sleep 10
+
+{
+	kill -SIGKILL $PID1 $PIDS
+	wait $PID1 $PIDS
+} > /dev/null 2>&1
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/ext4/060.out b/tests/ext4/060.out
new file mode 100644
index 00000000..8ffce4de
--- /dev/null
+++ b/tests/ext4/060.out
@@ -0,0 +1,2 @@
+QA output created by 060
+Silence is golden
--
2.39.2

[RFC 2/2] ext4/061: Regression test of jbd2 journal_task race against unmount

[Ritesh Harjani (IBM)]

This test adds a testcase to catch race condition against
reading of journal_task and parallel unmount.
This patch in kernel fixes this [1]

    ext4_put_super()                cat /sys/fs/ext4/loop2/journal_task
            |                               ext4_attr_show();
    ext4_jbd2_journal_destroy();                    |
            |                                journal_task_show()
            |                                       |
            |                               task_pid_vnr(NULL);
    sbi->s_journal = NULL;

RIP: 0010:__task_pid_nr_ns+0x4d/0xe0
<...>
Call Trace:
 <TASK>
 ext4_attr_show+0x1bd/0x3e0
 sysfs_kf_seq_show+0x8e/0x110
 seq_read_iter+0x11b/0x4d0
 vfs_read+0x216/0x2e0
 ksys_read+0x69/0xf0
 do_syscall_64+0x3f/0x90
 entry_SYSCALL_64_after_hwframe+0x72/0xdc
[

[1]: https://lore.kernel.org/all/20200318061301.4320-1-riteshh@linux.ibm.com/
Commit: ext4: Unregister sysfs path before destroying jbd2 journal

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 tests/ext4/061     | 82 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/ext4/061.out |  2 ++
 2 files changed, 84 insertions(+)
 create mode 100755 tests/ext4/061
 create mode 100644 tests/ext4/061.out

diff --git a/tests/ext4/061 b/tests/ext4/061
new file mode 100755
index 00000000..88bf138a
--- /dev/null
+++ b/tests/ext4/061
@@ -0,0 +1,82 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2023 IBM Corporation.  All Rights Reserved.
+#
+# FS QA Test 061
+#
+# Regression test for https://lore.kernel.org/all/20200318061301.4320-1-riteshh@linux.ibm.com/
+# ext4: Unregister sysfs path before destroying jbd2 journal
+#
+
+. ./common/preamble
+_begin_fstest auto quick
+
+pid_mloop=""
+pids_jloop=""
+trap "_cleanup; exit \$status" 0 1 2 3 15
+# Override the default cleanup function.
+_cleanup()
+{
+	{
+		kill -SIGKILL $pid_mloop $pids_jloop
+		wait $pid_mloop $pids_jloop
+	} > /dev/null 2>&1
+	cd /
+	rm -r -f $tmp.*
+}
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs ext4
+_fixed_by_kernel_commit 5e47868fb94b63c \
+	"ext4: unregister sysfs path before destroying jbd2 journal"
+_require_scratch
+_require_fs_sysfs journal_task
+
+_scratch_mkfs_ext4 >> $seqres.full 2>&1
+# mount filesystem
+_scratch_mount >> $seqres.full 2>&1
+scratch_dev=$(_short_dev $SCRATCH_DEV)
+
+function mount_loop()
+{
+	while [ 1 ]; do
+		_scratch_unmount >> $seqres.full 2>&1
+		sleep 1;
+		_scratch_mount >> $seqres.full 2>&1
+		sleep 1;
+	done
+}
+
+function read_journal_task_loop()
+{
+	while [ 1 ]; do
+		cat /sys/fs/ext4/$scratch_dev/journal_task > /dev/null 2>&1
+		sleep 1;
+	done
+}
+
+mount_loop &
+pid_mloop=$!
+
+for i in $(seq 1 100); do
+	read_journal_task_loop &
+	pid=$!
+	pids_jloop="${pids_jloop} ${pid}"
+done
+
+sleep 20
+{
+	kill -SIGKILL $pid_mloop $pids_jloop
+	wait $pid_mloop $pids_jloop
+} > /dev/null 2>&1
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/ext4/061.out b/tests/ext4/061.out
new file mode 100644
index 00000000..273be9e0
--- /dev/null
+++ b/tests/ext4/061.out
@@ -0,0 +1,2 @@
+QA output created by 061
+Silence is golden
--
2.39.2

Re: [RFC 1/2] ext4/060: Regression test against dioread_nolock mount option inconsistency

[Zorro Lang]

On Sat, Apr 22, 2023 at 09:47:33PM +0530, Ritesh Harjani (IBM) wrote:
> During ext4_writepages, ext4 queries dioread_nolock mount option twice
> and if someone remount the filesystem in between with ^dioread_nolock,
> then this can cause an inconsistency causing WARN_ON() to be triggered.
> 
> This fix describes the problem in more detail -
> 
> https://lore.kernel.org/linux-ext4/20230328090534.662l7yxj2e425j7w@quack3/T/#md19c34646e8b4a816498532c298a66ecf2ae77d4
> 
> This test reproduces below warning for me w/o the fix.
> 
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 26 at fs/ext4/page-io.c:231 ext4_put_io_end_defer+0xfb/0x140
> Modules linked in:
> CPU: 2 PID: 26 Comm: ksoftirqd/2 Not tainted 6.3.0-rc6-xfstests-00044-ga5c68786f1b1 #23
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014
> RIP: 0010:ext4_put_io_end_defer+0xfb/0x140
> Code: 5d 41 5e 41 5f e9 a5 73 d0 00 5b 48 89 ef 5d 41 5c 41 5d 41 5e 41 5f e9 d3 fa ff ff 49 83 be a8 03 00 00 00 0f 84 7b ff fd
> <...>
> Call Trace:
>  <TASK>
>  blk_update_request+0x116/0x4c0
>  ? finish_task_switch.isra.0+0xfb/0x320
>  blk_mq_end_request+0x1e/0x40
>  blk_complete_reqs+0x40/0x50
>  __do_softirq+0xd8/0x3e1
>  ? smpboot_thread_fn+0x30/0x280
>  run_ksoftirqd+0x3a/0x60
>  smpboot_thread_fn+0x1d8/0x280
>  ? __pfx_smpboot_thread_fn+0x10/0x10
>  kthread+0xf6/0x120
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork+0x2c/0x50
>  </TASK>
> [
> 
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  tests/ext4/060     | 88 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/ext4/060.out |  2 ++
>  2 files changed, 90 insertions(+)
>  create mode 100755 tests/ext4/060
>  create mode 100644 tests/ext4/060.out
> 
> diff --git a/tests/ext4/060 b/tests/ext4/060
> new file mode 100755
> index 00000000..d9fe1a99
> --- /dev/null
> +++ b/tests/ext4/060
> @@ -0,0 +1,88 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2023 IBM Corporation.  All Rights Reserved.
> +#
> +# FS QA Test 060
> +#
> +# This is to test a ext4 regression against inconsistent values of

Great, a new regression test case!

> +# dioread_nolock mount option while in ext4_writepages path.
> +# See - https://lore.kernel.org/linux-ext4/20230328090534.662l7yxj2e425j7w@quack3/T/#md19c34646e8b4a816498532c298a66ecf2ae77d4

You can use the commit id and subject to replace the link.

> +#
> +. ./common/preamble
> +_begin_fstest auto quick

also add mount/remount tag?

> +
> +PID1=""
> +PIDS=""
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +# Override the default cleanup function.
> + _cleanup()
> +{
> +	{
> +		kill -SIGKILL $PID1 $PIDS
> +		wait $PID1 $PIDS
> +	} > /dev/null 2>&1

I think the curly braces "{ }" is not necessary. Refer to generic/390 to deal
with the background processes.

[ -n "$PIDS" ] && kill -9 $PIDS
wait $PIDS

> +
> +	cd /
> +	rm -r -f $tmp.*
> +}
> +
> +# Import common functions.
> + . ./common/filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.

Remove this comment.

> +_supported_fs ext4

_fixed_by_kernel_commit ?

> +_require_scratch
> +
> +_scratch_mkfs_ext4 >> $seqres.full 2>&1
> +_scratch_mount
> +_scratch_remount "dioread_nolock" >> $seqres.full 2>&1
> +ret=$?

If the "$ret" is only used once as below...

> +if [ $ret -ne 0 ]; then

... then we can use "$?" directly.

> +	_notrun "dioread_nolock mount option not supported"

When ext4 start to support dioread_nolock/dioread_lock ?

If it's old enough, we don't need to check this option. Or we can have a new
helper (e.g. require_scratch_ext4_mount_option()). You can refer to
_require_scratch_ext4_feature(), or maybe we can change it to support mount
option test.

> +fi
> +
> +testfile=$SCRATCH_MNT/testfile
> +
> +function run_buff_io_loop()
> +{
> +	# add buffered io case here
> +	while [ 1 ]; do
> +		xfs_io -fc "truncate 0" -c "pwrite 0 200M" -c "fsync" "$testfile.$1" > /dev/null 2>&1

I only find the $testfile is used at here once, if so you can make it as
a local variable of this function.

> +		sleep 2;
> +	done
> +}
> +
> +function run_remount_loop()
> +{
> +	# add remount loop case here
> +	while [ 1 ]; do
> +		_scratch_remount "dioread_nolock" >> $seqres.full 2>&1
> +		sleep 1
> +		_scratch_remount "dioread_lock" >> $seqres.full 2>&1
> +		sleep 1
> +	done
> +}
> +
> +run_remount_loop &
> +PID1=$!

If you don't need to kill these processes in a specific order, I think
you can:

PIDS=$!

> +for i in $(seq 1 20); do
> +	run_buff_io_loop $i &
> +	PID=$!
> +	PIDS="${PIDS} ${PID}"

PIDS="$PIDS $!"

> +done
> +
> +sleep 10

$((10 * TIME_FACTOR)) ?

> +
> +{
> +	kill -SIGKILL $PID1 $PIDS
> +	wait $PID1 $PIDS
> +} > /dev/null 2>&1

kill -9 $$PIDS
wait $PIDS
unset PIDS

> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/ext4/060.out b/tests/ext4/060.out
> new file mode 100644
> index 00000000..8ffce4de
> --- /dev/null
> +++ b/tests/ext4/060.out
> @@ -0,0 +1,2 @@
> +QA output created by 060
> +Silence is golden
> --
> 2.39.2
> 

Re: [RFC 2/2] ext4/061: Regression test of jbd2 journal_task race against unmount

[Zorro Lang]

On Sat, Apr 22, 2023 at 09:47:34PM +0530, Ritesh Harjani (IBM) wrote:
> This test adds a testcase to catch race condition against
> reading of journal_task and parallel unmount.
> This patch in kernel fixes this [1]
> 
>     ext4_put_super()                cat /sys/fs/ext4/loop2/journal_task
>             |                               ext4_attr_show();
>     ext4_jbd2_journal_destroy();                    |
>             |                                journal_task_show()
>             |                                       |
>             |                               task_pid_vnr(NULL);
>     sbi->s_journal = NULL;
> 
> RIP: 0010:__task_pid_nr_ns+0x4d/0xe0
> <...>
> Call Trace:
>  <TASK>
>  ext4_attr_show+0x1bd/0x3e0
>  sysfs_kf_seq_show+0x8e/0x110
>  seq_read_iter+0x11b/0x4d0
>  vfs_read+0x216/0x2e0
>  ksys_read+0x69/0xf0
>  do_syscall_64+0x3f/0x90
>  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [
> 
> [1]: https://lore.kernel.org/all/20200318061301.4320-1-riteshh@linux.ibm.com/
> Commit: ext4: Unregister sysfs path before destroying jbd2 journal
> 
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  tests/ext4/061     | 82 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/ext4/061.out |  2 ++
>  2 files changed, 84 insertions(+)
>  create mode 100755 tests/ext4/061
>  create mode 100644 tests/ext4/061.out
> 
> diff --git a/tests/ext4/061 b/tests/ext4/061
> new file mode 100755
> index 00000000..88bf138a
> --- /dev/null
> +++ b/tests/ext4/061
> @@ -0,0 +1,82 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2023 IBM Corporation.  All Rights Reserved.
> +#
> +# FS QA Test 061
> +#
> +# Regression test for https://lore.kernel.org/all/20200318061301.4320-1-riteshh@linux.ibm.com/
> +# ext4: Unregister sysfs path before destroying jbd2 journal

The link isn't needed, if you points out the commit id and subject.

> +#
> +
> +. ./common/preamble
> +_begin_fstest auto quick
> +
> +pid_mloop=""
> +pids_jloop=""
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	{
> +		kill -SIGKILL $pid_mloop $pids_jloop
> +		wait $pid_mloop $pids_jloop
> +	} > /dev/null 2>&1

[ -n "$pids_jloop" ] && kill -9 $pids_jloop
[ -n "$pid_mloop" ] && kill -9 $pid_mloop
wait

> +	cd /
> +	rm -r -f $tmp.*
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs ext4
> +_fixed_by_kernel_commit 5e47868fb94b63c \
> +	"ext4: unregister sysfs path before destroying jbd2 journal"
> +_require_scratch
> +_require_fs_sysfs journal_task
> +
> +_scratch_mkfs_ext4 >> $seqres.full 2>&1
> +# mount filesystem
> +_scratch_mount >> $seqres.full 2>&1
> +scratch_dev=$(_short_dev $SCRATCH_DEV)

The $scratch_dev is only used in read_journal_task_loop() ...

> +
> +function mount_loop()
> +{
> +	while [ 1 ]; do
> +		_scratch_unmount >> $seqres.full 2>&1
> +		sleep 1;
> +		_scratch_mount >> $seqres.full 2>&1

Do you hope to fail the test directly if the mount fails? Or you hope
to use _try_scratch_mount ?

> +		sleep 1;
> +	done
> +}
> +
> +function read_journal_task_loop()
> +{
> +	while [ 1 ]; do
> +		cat /sys/fs/ext4/$scratch_dev/journal_task > /dev/null 2>&1

... so I think you can write this line as:

cat /sys/fs/ext4/$(_short_dev $SCRATCH_DEV)/journal_task ...


BTW, I'm wondering if you need to check the journal_task is supported?

> +		sleep 1;
> +	done
> +}
> +
> +mount_loop &
> +pid_mloop=$!
> +
> +for i in $(seq 1 100); do
> +	read_journal_task_loop &
> +	pid=$!
> +	pids_jloop="${pids_jloop} ${pid}"

pids_jloop="${pids_jloop} $!"

> +done
> +
> +sleep 20

20 * TIME_FACTOR ?

> +{
> +	kill -SIGKILL $pid_mloop $pids_jloop
> +	wait $pid_mloop $pids_jloop
> +} > /dev/null 2>&1

kill -9 $pid_mloop $pids_jloop
wait $pid_mloop $pids_jloop
unset pid_mloop pids_jloop

> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/ext4/061.out b/tests/ext4/061.out
> new file mode 100644
> index 00000000..273be9e0
> --- /dev/null
> +++ b/tests/ext4/061.out
> @@ -0,0 +1,2 @@
> +QA output created by 061
> +Silence is golden
> --
> 2.39.2
> 

Re: [RFC 1/2] ext4/060: Regression test against dioread_nolock mount option inconsistency

[Ritesh Harjani]

Zorro Lang <zlang@redhat.com> writes:

> On Sat, Apr 22, 2023 at 09:47:33PM +0530, Ritesh Harjani (IBM) wrote:
>> During ext4_writepages, ext4 queries dioread_nolock mount option twice
>> and if someone remount the filesystem in between with ^dioread_nolock,
>> then this can cause an inconsistency causing WARN_ON() to be triggered.
>>
>> This fix describes the problem in more detail -
>>
>> https://lore.kernel.org/linux-ext4/20230328090534.662l7yxj2e425j7w@quack3/T/#md19c34646e8b4a816498532c298a66ecf2ae77d4
>>
>> This test reproduces below warning for me w/o the fix.
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 2 PID: 26 at fs/ext4/page-io.c:231 ext4_put_io_end_defer+0xfb/0x140
>> Modules linked in:
>> CPU: 2 PID: 26 Comm: ksoftirqd/2 Not tainted 6.3.0-rc6-xfstests-00044-ga5c68786f1b1 #23
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014
>> RIP: 0010:ext4_put_io_end_defer+0xfb/0x140
>> Code: 5d 41 5e 41 5f e9 a5 73 d0 00 5b 48 89 ef 5d 41 5c 41 5d 41 5e 41 5f e9 d3 fa ff ff 49 83 be a8 03 00 00 00 0f 84 7b ff fd
>> <...>
>> Call Trace:
>>  <TASK>
>>  blk_update_request+0x116/0x4c0
>>  ? finish_task_switch.isra.0+0xfb/0x320
>>  blk_mq_end_request+0x1e/0x40
>>  blk_complete_reqs+0x40/0x50
>>  __do_softirq+0xd8/0x3e1
>>  ? smpboot_thread_fn+0x30/0x280
>>  run_ksoftirqd+0x3a/0x60
>>  smpboot_thread_fn+0x1d8/0x280
>>  ? __pfx_smpboot_thread_fn+0x10/0x10
>>  kthread+0xf6/0x120
>>  ? __pfx_kthread+0x10/0x10
>>  ret_from_fork+0x2c/0x50
>>  </TASK>
>> [
>>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>  tests/ext4/060     | 88 ++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/ext4/060.out |  2 ++
>>  2 files changed, 90 insertions(+)
>>  create mode 100755 tests/ext4/060
>>  create mode 100644 tests/ext4/060.out
>>
>> diff --git a/tests/ext4/060 b/tests/ext4/060
>> new file mode 100755
>> index 00000000..d9fe1a99
>> --- /dev/null
>> +++ b/tests/ext4/060
>> @@ -0,0 +1,88 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2023 IBM Corporation.  All Rights Reserved.
>> +#
>> +# FS QA Test 060
>> +#
>> +# This is to test a ext4 regression against inconsistent values of
>
> Great, a new regression test case!
>
>> +# dioread_nolock mount option while in ext4_writepages path.
>> +# See - https://lore.kernel.org/linux-ext4/20230328090534.662l7yxj2e425j7w@quack3/T/#md19c34646e8b4a816498532c298a66ecf2ae77d4
>
> You can use the commit id and subject to replace the link.
>
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick
>
> also add mount/remount tag?
>

Yes.

>> +
>> +PID1=""
>> +PIDS=""
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +# Override the default cleanup function.
>> + _cleanup()
>> +{
>> +	{
>> +		kill -SIGKILL $PID1 $PIDS
>> +		wait $PID1 $PIDS
>> +	} > /dev/null 2>&1
>
> I think the curly braces "{ }" is not necessary. Refer to generic/390 to deal
> with the background processes.

Ok, will check that.

>
> [ -n "$PIDS" ] && kill -9 $PIDS
> wait $PIDS
>

Sure.

>> +
>> +	cd /
>> +	rm -r -f $tmp.*
>> +}
>> +
>> +# Import common functions.
>> + . ./common/filter
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>
> Remove this comment.
>
>> +_supported_fs ext4
>
> _fixed_by_kernel_commit ?
>

Yes, I will check the commit-id and will update it.

>> +_require_scratch
>> +
>> +_scratch_mkfs_ext4 >> $seqres.full 2>&1
>> +_scratch_mount
>> +_scratch_remount "dioread_nolock" >> $seqres.full 2>&1
>> +ret=$?
>
> If the "$ret" is only used once as below...
>
>> +if [ $ret -ne 0 ]; then
>
> ... then we can use "$?" directly.
>
>> +	_notrun "dioread_nolock mount option not supported"
>
> When ext4 start to support dioread_nolock/dioread_lock ?

Ok. yes looks like dioread_nolock is quiet old. Will drop the check.

>
> If it's old enough, we don't need to check this option. Or we can have a new
> helper (e.g. require_scratch_ext4_mount_option()). You can refer to
> _require_scratch_ext4_feature(), or maybe we can change it to support mount
> option test.
>
>> +fi
>> +
>> +testfile=$SCRATCH_MNT/testfile
>> +
>> +function run_buff_io_loop()
>> +{
>> +	# add buffered io case here
>> +	while [ 1 ]; do
>> +		xfs_io -fc "truncate 0" -c "pwrite 0 200M" -c "fsync" "$testfile.$1" > /dev/null 2>&1
>
> I only find the $testfile is used at here once, if so you can make it as
> a local variable of this function.
>
>> +		sleep 2;
>> +	done
>> +}
>> +
>> +function run_remount_loop()
>> +{
>> +	# add remount loop case here
>> +	while [ 1 ]; do
>> +		_scratch_remount "dioread_nolock" >> $seqres.full 2>&1
>> +		sleep 1
>> +		_scratch_remount "dioread_lock" >> $seqres.full 2>&1
>> +		sleep 1
>> +	done
>> +}
>> +
>> +run_remount_loop &
>> +PID1=$!
>
> If you don't need to kill these processes in a specific order, I think
> you can:
>
> PIDS=$!
>

ok.

>> +for i in $(seq 1 20); do
>> +	run_buff_io_loop $i &
>> +	PID=$!
>> +	PIDS="${PIDS} ${PID}"
>
> PIDS="$PIDS $!"
>
>> +done
>> +
>> +sleep 10
>
> $((10 * TIME_FACTOR)) ?
>

Sure. will check more on TIME_FACTOR.

>> +
>> +{
>> +	kill -SIGKILL $PID1 $PIDS
>> +	wait $PID1 $PIDS
>> +} > /dev/null 2>&1
>
> kill -9 $$PIDS
> wait $PIDS
> unset PIDS
>

Thanks Zorro for the quick review. Agree with all of your comments.
I will work on these and will send out v2 addressing your review
comments.

-ritesh