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@xxxxxxx>
> > ---
> > 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.
