Bart, thanks for the patch. Please find my comments in line.
On Nov 25, 2024 / 13:10, Bart Van Assche wrote:
> Test concurrent requeuing of zoned writes and request queue freezing. While
> test test passes with kernel 6.9, it triggers a hang with kernels 6.10 and
I guess you mean, s/test test passes/the test passes/
> later. This shows that this hang is a regression introduced by the zone
> write plugging code.
>
> sysrq: Show Blocked State
> task:(udev-worker) state:D stack:0 pid:75392 tgid:75392 ppid:2178 flags:0x00000006
> Call Trace:
> <TASK>
> __schedule+0x3e8/0x1410
> schedule+0x27/0xf0
> blk_mq_freeze_queue_wait+0x6f/0xa0
> queue_attr_store+0x60/0xc0
> kernfs_fop_write_iter+0x13e/0x1f0
> vfs_write+0x25b/0x420
> ksys_write+0x65/0xe0
> do_syscall_64+0x82/0x160
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> tests/zbd/012 | 70 +++++++++++++++++++++++++++++++++++++++++++++++
> tests/zbd/012.out | 2 ++
> 2 files changed, 72 insertions(+)
> create mode 100644 tests/zbd/012
> create mode 100644 tests/zbd/012.out
>
> diff --git a/tests/zbd/012 b/tests/zbd/012
> new file mode 100644
For the consistency, the test script should be executable. I expect the file
mode 100755 here.
> index 000000000000..0551d01011af
> --- /dev/null
> +++ b/tests/zbd/012
> @@ -0,0 +1,70 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
The other blktests files have GPL-2.0+ or GPL-3.0+. Unless you have specific
reason, I suggest one of the two instead of GPL-2.0.
> +# Copyright (C) 2024 Google LLC
I would like have a short description here in same manner as the commit message.
How about this?
# Test concurrent requeuing of zoned writes and request queue freezing. It
# triggers a hang caused by the regression introduced by the zone write
# plugging.
> +
> +. tests/scsi/rc
s/scsi/zbd/
> +. common/scsi_debug
> +
> +DESCRIPTION="test requeuing of zoned writes and queue freezing"
> +QUICK=1
The test case uses the TIMEOUT value to control its runtime. So the QUICK=1
should not be set, and TIMED=1 should be set.
> +
> +requires() {
> + _have_fio_zbd_zonemode
> +}
> +
> +toggle_iosched() {
Nit: I suggest to add "local iosched" here.
> + while true; do
> + for iosched in none mq-deadline; do
> + echo "${iosched}" > "/sys/class/block/$(basename "$zdev")/queue/scheduler"
> + sleep .1
> + done
> + done
> +}
> +
> +test() {
> + echo "Running ${TEST_NAME}"
> +
> + local qd=1
> + local scsi_debug_params=(
> + delay=0
> + dev_size_mb=1024
> + every_nth=$((2 * qd))
> + max_queue="${qd}"
> + opts=0x8000 # SDEBUG_OPT_HOST_BUSY
> + sector_size=4096
> + statistics=1
> + zbc=host-managed
> + zone_nr_conv=0
> + zone_size_mb=4
> + )
> + _init_scsi_debug "${scsi_debug_params[@]}" &&
> + local zdev="/dev/${SCSI_DEBUG_DEVICES[0]}" fail &&
> + ls -ld "${zdev}" >>"${FULL}" &&
> + { toggle_iosched & } &&
> + toggle_iosched_pid=$! &&
> + local fio_args=(
> + --direct=1
> + --filename="${zdev}"
> + --iodepth="${qd}"
> + --ioengine=io_uring
> + --ioscheduler=none
> + --name=pipeline-zoned-writes
> + --output="${RESULTS_DIR}/fio-output-zbd-092.txt"
I think the separated fio output file is rather difficult to find. Also,
_run_fio sets --output-format=terse, which is rather difficult to understand.
I suggest to not set the --output option here and to use "fio" instead of
"_run_fio". This way, the default fio output will be recorded in $FULL.
> + --runtime="${TIMEOUT:-30}"
> + --rw=randwrite
> + --time_based
> + --zonemode=zbd
> + ) &&
> + _run_fio "${fio_args[@]}" >>"${FULL}" 2>&1 ||
> + fail=true
> +
> + kill "${toggle_iosched_pid}" 2>&1
> + _exit_scsi_debug
> +
> + if [ -z "$fail" ]; then
> + echo "Test complete"
> + else
> + echo "Test failed"
> + return 1
> + fi
> +}
> diff --git a/tests/zbd/012.out b/tests/zbd/012.out
> new file mode 100644
> index 000000000000..8ff654950c5f
> --- /dev/null
> +++ b/tests/zbd/012.out
> @@ -0,0 +1,2 @@
> +Running zbd/012
> +Test complete
