Re: [PATCH blktests v2 05/11] nvme/rc: introduce NVMET_TRTYPES

Sagi Grimberg
2024-04-18T09:39:57Z

On 16/04/2024 13:32, Shin'ichiro Kawasaki wrote:
> Some of the test cases in nvme test group can be run under various nvme
> target transport types. The configuration parameter nvme_trtype
> specifies the transport to use. But this configuration method has two
> drawbacks. Firstly, the blktests check script needs to be invoked
> multiple times to cover multiple transport types. Secondly, the test
> cases irrelevant to the transport types are executed exactly same
> conditions in the multiple blktests runs.
>
> To avoid the drawbacks, introduce the new configuration parameter
> NVMET_TRTYPES. This parameter can take multiple transport types like:
>
> NVMET_TRTYPES="loop tcp"
>
> Also introduce _nvmet_set_nvme_trtype() which can be called from the
> set_conditions() hook of the transport type dependent test cases.
> Blktests will repeat the test case as many as the number of elements in
> NVMET_TRTYPES, and set nvme_trtype for each test case run.

It would be nicer to keep the same name and just have it accept an array.
Not a must though.

> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
> Documentation/running-tests.md | 11 ++++++++---
> tests/nvme/rc | 33 ++++++++++++++++++++++++++++++++-
> 2 files changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md
> index ae80860..144acd1 100644
> --- a/Documentation/running-tests.md
> +++ b/Documentation/running-tests.md
> @@ -102,8 +102,13 @@ RUN_ZONED_TESTS=1
>
> The NVMe tests can be additionally parameterized via environment variables.
>
> +- NVMET_TRTYPES: 'loop' (default), 'tcp', 'rdma' and 'fc'
> + Set up NVME target backends with the specified transport. Multiple transports
> + can be listed with separating spaces, e.g., "loop tcp rdma". In this case, the
> + tests are repeated to cover all of the transports specified.
> - nvme_trtype: 'loop' (default), 'tcp', 'rdma' and 'fc'
> - Run the tests with the given transport.
> + Run the tests with the given transport. This parameter is still usable but
> + replaced with NVMET_TRTYPES. Use NVMET_TRTYPES instead.
> - nvme_img_size: '1G' (default)
> Run the tests with given image size in bytes. 'm', 'M', 'g'
> and 'G' postfix are supported.
> @@ -117,11 +122,11 @@ These tests will use the siw (soft-iWARP) driver by default. The rdma_rxe
>
> ```sh
> To use the siw driver:
> -nvme_trtype=rdma ./check nvme/
> +NVMET_TRTYPES=rdma ./check nvme/
> ./check srp/
>
> To use the rdma_rxe driver:
> -use_rxe=1 nvme_trtype=rdma ./check nvme/
> +use_rxe=1 NVMET_TRTYPES=rdma ./check nvme/
> use_rxe=1 ./check srp/
> ```
>
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index 1f5ff44..34ecdde 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -18,10 +18,41 @@ def_hostid="0f01fb42-9f7f-4856-b0b3-51e60b8de349"
> def_hostnqn="nqn.2014-08.org.nvmexpress:uuid:${def_hostid}"
> export def_subsysnqn="blktests-subsystem-1"
> export def_subsys_uuid="91fdba0d-f87b-4c25-b80f-db7be1418b9e"
> -nvme_trtype=${nvme_trtype:-"loop"}
> nvme_img_size=${nvme_img_size:-"1G"}
> nvme_num_iter=${nvme_num_iter:-"1000"}
>
> +# Check consistency of NVMET_TRTYPES and nvme_trtype configurations.
> +# If neither is configured, set the default value.
> +first_call=${first_call:-1}
> +if ((first_call)); then
> + if [[ -n $nvme_trtype ]]; then
> + if [[ -n $NVMET_TRTYPES ]]; then
> + echo "Both nvme_trtype and NVMET_TRTYPES are specified"
> + exit 1
> + fi
> + NVMET_TRTYPES="$nvme_trtype"
> + elif [[ -z $NVMET_TRTYPES ]]; then
> + nvme_trtype="loop"
> + NVMET_TRTYPES="$nvme_trtype"
> + fi
> + first_call=0
> +fi

It would be nice to have the string check be done at first so you don't
get any typo-related error later during the execution.