[PATCH] nvme: test nvmet-wq sysfs interface

[Shinichiro Kawasaki]

On Nov 04, 2024 / 11:29, Chaitanya Kulkarni wrote:
> Add a test that randomly sets the cpumask from available CPUs for
> the nvmet-wq while running the fio workload. This patch has been
> tested on nvme-loop and nvme-tcp transport.
> 
> Signed-off-by: Chaitanya Kulkarni <kch at nvidia.com>

Thanks. As I noted in another mail, this test case generates a kernel
INFO message, and it looks like catching a new bug. So I think this
test case worth adding. Please find my review comments in line.

> ---
>  tests/nvme/055     | 99 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/055.out |  3 ++
>  2 files changed, 102 insertions(+)
>  create mode 100755 tests/nvme/055
>  create mode 100644 tests/nvme/055.out
> 
> diff --git a/tests/nvme/055 b/tests/nvme/055
> new file mode 100755
> index 0000000..9fe27a3
> --- /dev/null
> +++ b/tests/nvme/055
> @@ -0,0 +1,99 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (C) 2024 Chaitanya Kulkarni
> +#
> +# Test nvmet-wq cpumask sysfs attribute with NVMe-oF and fio workload
> +#
> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="Test nvmet-wq cpumask sysfs attribute with fio on NVMe-oF device"
> +TIMED=1
> +
> +requires() {
> +	_nvme_requires
> +	 _have_fio && _have_loop

Nit, a unneccesaary space.

> +	_require_nvme_trtype_is_fabrics
> +}

I suggest to add set_conditions() here as below. Without it, the test case is
skipped when users do not se NVMET_TRTYPES variable. If we add set_conditions(),
_have_loop call in requires() can be removed.

set_conditions() {
       _set_nvme_trtype "$@"
}

> +
> +cleanup_setup() {
> +	_nvme_disconnect_subsys
> +	_nvmet_target_cleanup
> +}
> +
> +test() {
> +	local cpumask_path="/sys/devices/virtual/workqueue/nvmet-wq/cpumask"
> +	local original_cpumask
> +	local min_cpus
> +	local max_cpus
> +	local numbers
> +	local idx
> +	local ns
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	_setup_nvmet
> +	_nvmet_target_setup
> +	_nvme_connect_subsys
> +
> +	if [ ! -f "$cpumask_path" ]; then
> +		SKIP_REASONS+=("nvmet_wq cpumask sysfs attribute not found.")
> +		cleanup_setup
> +		return 1
> +	fi
> +
> +	ns=$(_find_nvme_ns "${def_subsys_uuid}")
> +
> +	original_cpumask=$(cat "$cpumask_path")
> +
> +	num_cpus=$(nproc)
> +	max_cpus=$(( num_cpus < 20 ? num_cpus : 20 ))
> +	min_cpus=0
> +	#shellcheck disable=SC2207
> +	numbers=($(seq $min_cpus $max_cpus))

Nit: the shellcheck error can be vaoided with this:

        read -a numbers -d '' < <(seq $min_cpus $max_cpus)

> +
> +	_run_fio_rand_io --filename="/dev/${ns}" --time_based --runtime=130s \

The --time_based and --runtime=130s options are not required, because fio helper
bash functions will add them.

Instead, let's add this line before the fio command.

         : ${TIMEOUT:=60}

The fio helper functions will refect this TIMEOUT value to the --runtime
option. This will allow users to control runtime cost as TIMED=1 indicates.

> +			 --iodepth=8 --size="${NVME_IMG_SIZE}" &> "$FULL" &
> +
> +	# Let the fio settle down else we will break in the loop for fio check
> +	sleep 1
> +	for ((i = 0; i < max_cpus; i++)); do
> +		if ! pgrep -x fio &> /dev/null ; then

pgrep command is not in the GNU coreutils. I suggest to keep the fio process id
in a variable and check with "kill -0" command. The test case block/005 does it.

> +			break
> +		fi
> +
> +		if [[ ${#numbers[@]} -eq 0 ]]; then
> +			break
> +		fi
> +
> +		idx=$((RANDOM % ${#numbers[@]}))
> +
> +		#shellcheck disable=SC2004
> +		cpu_mask=$(printf "%X" $((1 << ${numbers[idx]})))
> +		echo "$cpu_mask" > "$cpumask_path"

When I ran this test case, I observed an error at this line:

    echo: write error: Value too large for defined data type

I think the cpu_mask calculation is wrong, and it should be like this:

               cpu_mask=0
               for ((n = 0; n < numbers[idx]; n++)); do
                       cpu_mask=$((cpu_mask + (1 << n)))
               done
               cpu_mask=$(printf "%X" $((cpu_mask)))


> +		if [[ $(cat "$cpumask_path") =~ ^[0,]*${cpu_mask}\n$ ]]; then
> +			echo "Test Failed: cpumask was not set correctly "
> +			echo "Expected ${cpu_mask} found $(cat "$cpumask_path")"
> +			cleanup_setup
> +			return 1
> +		fi
> +		sleep 3
> +		# Remove the selected number
> +		numbers=("${numbers[@]:0:$idx}" "${numbers[@]:$((idx + 1))}")
> +	done
> +
> +	killall fio &> /dev/null

killall is not in GNU coreutils either (blktests already uses it at other
places though...)   Does "kill -9" work instead?

> +
> +	# Restore original cpumask
> +	echo "$original_cpumask" > "$cpumask_path"
> +	restored_cpumask=$(cat "$cpumask_path")
> +
> +	if [[ "$restored_cpumask" != "$original_cpumask" ]]; then
> +		echo "Failed to restore original cpumask."
> +		cleanup_setup
> +		return 1
> +	fi
> +
> +	cleanup_setup
> +	echo "Test complete"
> +}
> diff --git a/tests/nvme/055.out b/tests/nvme/055.out
> new file mode 100644
> index 0000000..427dfee
> --- /dev/null
> +++ b/tests/nvme/055.out
> @@ -0,0 +1,3 @@
> +Running nvme/055
> +disconnected 1 controller(s)
> +Test complete
> -- 
> 2.40.0
>