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

[Shinichiro Kawasaki]

Hi, Sagi, thanks for the comments.

On Apr 18, 2024 / 12:39, Sagi Grimberg wrote:
> 
> 
> 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.

Okay, actually this patch makes nvme_trtype to work same as NVMET_TRTYPES.
I will update the commit message and the description in running-tests.md
to clarify that nvme_trtype accepts multiple transports.

Though the existing nvme_trtype and the new NVMET_TRTYPES work same, I think
this is a good chance to take a step to replace the lowercase parameters to
uppercase ones. Will keep those two and describe that NVMET_TRTYPES is
recommended.

> 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki at 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.

Agreed, will add a check helper function for it.