Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 24 Apr 2023 00:20:37 +0800
From:   Zorro Lang <zlang@redhat.com>
To:     "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
Cc:     fstests@vger.kernel.org, linux-ext4@vger.kernel.org,
        Baokun Li <libaokun1@huawei.com>
Subject: Re: [RFC 2/2] ext4/061: Regression test of jbd2 journal_task race
 against unmount
Message-ID: <20230423162037.6kpvxqsnomvtxsmn@zlang-mailbox>
References: <aa8a633b016114b94d10dd0f9f0b7a355aeb1d62.1682179372.git.ritesh.list@gmail.com>
 <75819a20d2337c154e360c60ec53b7e519ebb97a.1682179372.git.ritesh.list@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <75819a20d2337c154e360c60ec53b7e519ebb97a.1682179372.git.ritesh.list@gmail.com>
Precedence: bulk

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
>