Re: [PATCH blktest] nvme/033: add test case for nvme update hardware queue count

[Omar Sandoval <osandov@osandov.com>]

On Tue, Apr 07, 2020 at 10:16:33PM +0800, Weiping Zhang wrote:
> Modify nvme module parameter write_queues to change hardware
> queue count, then reset nvme controller to reinitialize nvme
> with different queue count.
> 
> Attention, this test case may trigger a kernel panic.
> 
> Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> ---
>  tests/nvme/033     | 87 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/033.out |  2 ++
>  2 files changed, 89 insertions(+)
>  create mode 100755 tests/nvme/033
>  create mode 100644 tests/nvme/033.out
> 
> diff --git a/tests/nvme/033 b/tests/nvme/033
> new file mode 100755
> index 0000000..e3b9211
> --- /dev/null
> +++ b/tests/nvme/033
> @@ -0,0 +1,87 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2020 Weiping Zhang <zwp10758@gmail.com>
> +#
> +# Test nvme update hardware queue count larger than system cpu count
> +#
> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="test nvme update hardware queue count larger than system cpu count"
> +QUICK=1
> +
> +requires() {
> +	_have_program dd
> +}
> +
> +device_requires() {
> +	_test_dev_is_nvme
> +}
> +
> +test_device() {
> +	echo "Running ${TEST_NAME}"
> +
> +	local old_write_queues
> +	local cur_hw_io_queues
> +	local file
> +	local sys_dev=$TEST_DEV_SYSFS/device
> +
> +	# backup old module parameter: write_queues
> +	file=/sys/module/nvme/parameters/write_queues
> +	if [[ ! -e "$file" ]]; then
> +		echo "$file does not exist"
> +		return 1
> +	fi

Tests shouldn't fail if kernel support is missing. You can check this
with `_have_module_param nvme write_queues` in requires().

> +	old_write_queues="$(cat $file)"
> +
> +	# get current hardware queue count
> +	file="$sys_dev/queue_count"
> +	if [[ ! -e "$file" ]]; then
> +		echo "$file does not exist"
> +		return 1
> +	fi

Again, this should be checked before the test is run (device_requires(),
in this case). Something like

if [[ ! -e "$TEST_DEV_SYSFS/device/queue_count" ]]; then
	SKIP_REASON="device does have queue_count sysfs attribute"
	return
fi

> +	cur_hw_io_queues="$(cat "$file")"
> +	# minus admin queue
> +	cur_hw_io_queues=$((cur_hw_io_queues - 1))
> +
> +	# set write queues count to increase more hardware queues
> +	file=/sys/module/nvme/parameters/write_queues
> +	echo "$cur_hw_io_queues" > "$file"
> +
> +	# reset controller, make it effective
> +	file="$sys_dev/reset_controller"
> +	if [[ ! -e "$file" ]]; then
> +		echo "$file does not exist"
> +		return 1
> +	fi

Same thing here.

> +	echo 1 > "$file"
> +
> +	# wait nvme reinitialized
> +	for ((m = 0; m < 10; m++)); do
> +		if [[ -b "${TEST_DEV}" ]]; then
> +			break
> +		fi
> +		sleep 0.5
> +	done
> +	if (( m > 9 )); then
> +		echo "nvme still not reinitialized after 5 seconds!"
> +		return 1

You're not resetting the write_queues parameter here.

> +	fi
> +
> +	# read data from device (may kernel panic)
> +	dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 status=none
> +
> +	# If all work well restore hardware queue to default
> +	file=/sys/module/nvme/parameters/write_queues
> +	echo "$old_write_queues" > "$file"
> +
> +	# reset controller
> +	file="$sys_dev/reset_controller"
> +	echo 1 > "$file"

Does this need the same logic to wait for the device to reappear?

> +	# read data from device (may kernel panic)
> +	dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 iflag=direct status=none
> +	dd if=/dev/zero of="${TEST_DEV}" bs=4096 count=1 oflag=direct status=none
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/nvme/033.out b/tests/nvme/033.out
> new file mode 100644
> index 0000000..9648c73
> --- /dev/null
> +++ b/tests/nvme/033.out
> @@ -0,0 +1,2 @@
> +Running nvme/033
> +Test complete
> -- 
> 2.18.1
>