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) <[email protected]>
---
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
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/[email protected]/
Commit: ext4: Unregister sysfs path before destroying jbd2 journal
Signed-off-by: Ritesh Harjani (IBM) <[email protected]>
---
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/[email protected]/
+# 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
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) <[email protected]>
> ---
> 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
>
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/[email protected]/
> Commit: ext4: Unregister sysfs path before destroying jbd2 journal
>
> Signed-off-by: Ritesh Harjani (IBM) <[email protected]>
> ---
> 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/[email protected]/
> +# 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
>
Zorro Lang <[email protected]> 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) <[email protected]>
>> ---
>> 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