On Jul 02, 2024 / 17:51, Shin'ichiro Kawasaki wrote: > CC+: Ofir Sorry, I forgot to add Ofir to the CC list. Let me resend this note. > > A heads up: this patch causes conflict with Ofir's patch to move tests/nvme/rc > helpers to common/nvme [*]. Depending on which comes first, Daniel or Ofir will > need patch rework. > > [*] https://lore.kernel.org/linux-block/20240624104620.2156041-2-ofir.gal@xxxxxxxxxxx/ > > > On Jun 27, 2024 / 11:10, Daniel Wagner wrote: > > Most of the NVMEeoF tests are exercising the host code of the nvme > > subsystem. There is no real reason not to run these against a real > > target. We just have to skip the soft target setup and make it possible > > to setup a remote target. > > > > Because all tests use now the common setup/cleanup helpers we just need > > to intercept this call and forward it to an external component. > > > > As we already have various nvme variables to setup the target which we > > should allow to overwrite. Also introduce a NVME_TARGET_CONTROL variable > > which points to a script which gets executed whenever a targets needs to > > be created/destroyed. > > > > Signed-off-by: Daniel Wagner <dwagner@xxxxxxx> > > Daniel, thanks for this v3 patch. Please find some comments in line. > I ran "make check" and observed some shellcheck warnings: > > $ make check > shellcheck -x -e SC2119 -f gcc check common/* \ > tests/*/rc tests/*/[0-9]*[0-9] src/*.sh > tests/nvme/rc:962:5: warning: eval negates the benefit of arrays. Drop eval to preserve whitespace/symbols (or eval as string). [SC2294] > tests/nvme/041:34:36: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/042:40:52: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/042:54:61: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/043:37:36: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/044:36:36: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/044:42:36: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/045:41:36: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/045:47:36: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/045:72:43: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/045:82:43: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/051:40:25: note: Double quote to prevent globbing and word splitting. [SC2086] > tests/nvme/051:41:25: note: Double quote to prevent globbing and word splitting. [SC2086] > > > --- > > Documentation/running-tests.md | 33 ++++++++++++++++++++ > > check | 4 +++ > > tests/nvme/rc | 57 ++++++++++++++++++++++++++++++++-- > > 3 files changed, 92 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md > > index 968702e76bb5..fe4f729bd037 100644 > > --- a/Documentation/running-tests.md > > +++ b/Documentation/running-tests.md > > @@ -120,6 +120,9 @@ The NVMe tests can be additionally parameterized via environment variables. > > - NVME_NUM_ITER: 1000 (default) > > The number of iterations a test should do. This parameter had an old name > > 'nvme_num_iter'. The old name is still usable, but not recommended. > > +- NVME_TARGET_CONTROL: When defined, the generic target setup/cleanup code will > > + be skipped and this script gets called. This makes it possible to run > > + the fabric nvme tests against a real target. > > > > ### Running nvme-rdma and SRP tests > > > > @@ -167,3 +170,33 @@ if ! findmnt -t configfs /sys/kernel/config > /dev/null; then > > mount -t configfs configfs /sys/kernel/config > > fi > > ``` > > +### NVME_TARGET_CONTROL > > + > > +When NVME_TARGET_CONTROL is set, blktests will call the script which the > > +environment variable points to, to fetch the configuration values to be used for > > +the runs, e.g subsysnqn or hostnqn. This allows the blktest to be run against > > +external configured/setup targets. > > + > > +The blktests expects that the script interface implements following > > +commands: > > + > > +config: > > + --show-blkdev-type > > + --show-trtype > > + --show-hostnqn > > + --show-hostid > > + --show-host-traddr > > + --show-traddr > > + --show-trsvid > > + --show-subsys-uuid > > + --show-subsysnqn > > + > > +setup: > > + --subsysnqn SUBSYSNQN > > + --subsys-uuid SUBSYS_UUID > > + --hostnqn HOSTNQN > > + --ctrlkey CTRLKEY > > + --hostkey HOSTKEY > > + > > +cleanup: > > + --subsysnqn SUBSYSNQN > > Thanks. I think the NVME_TARGET_CONTROL script command line interface is well > documented. > > > diff --git a/check b/check > > index 3ed4510f3f40..d0475629773d 100755 > > --- a/check > > +++ b/check > > @@ -603,6 +603,10 @@ _run_group() { > > # shellcheck disable=SC1090 > > . "tests/${group}/rc" > > > > + if declare -fF group_setup >/dev/null; then > > + group_setup > > + fi > > This new hook adds some complexity, but I can not think of better way. So, I > agree to add this hook. > > > + > > if declare -fF group_requires >/dev/null; then > > group_requires > > if [[ -v SKIP_REASONS ]]; then > > diff --git a/tests/nvme/rc b/tests/nvme/rc > > index c1ddf412033b..4465dea0370b 100644 > > --- a/tests/nvme/rc > > +++ b/tests/nvme/rc > > @@ -23,6 +23,7 @@ _check_conflict_and_set_default NVME_IMG_SIZE nvme_img_size 1G > > _check_conflict_and_set_default NVME_NUM_ITER nvme_num_iter 1000 > > nvmet_blkdev_type=${nvmet_blkdev_type:-"device"} > > NVMET_BLKDEV_TYPES=${NVMET_BLKDEV_TYPES:-"device file"} > > +nvme_target_control="${NVME_TARGET_CONTROL:-}" > > > > _NVMET_TRTYPES_is_valid() { > > local type > > @@ -135,6 +136,13 @@ _nvme_requires() { > > return 0 > > } > > > > +group_setup() { > > + if [[ -n "${nvme_target_control}" ]]; then > > + NVMET_TRTYPES="$(${nvme_target_control} config --show-trtype)" > > + NVMET_BLKDEV_TYPES="$(${nvme_target_control} config --show-blkdev-type)" > > + fi > > +} > > + > > group_requires() { > > _have_root > > _NVMET_TRTYPES_is_valid > > @@ -359,6 +367,10 @@ _cleanup_nvmet() { > > fi > > done > > > > + if [[ -n "${nvme_target_control}" ]]; then > > + return > > + fi > > + > > for port in "${NVMET_CFS}"/ports/*; do > > name=$(basename "${port}") > > echo "WARNING: Test did not clean up port: ${name}" > > @@ -403,11 +415,26 @@ _cleanup_nvmet() { > > > > _setup_nvmet() { > > _register_test_cleanup _cleanup_nvmet > > + > > + if [[ -n "${nvme_target_control}" ]]; then > > + def_hostnqn="$(${nvme_target_control} config --show-hostnqn)" > > + def_hostid="$(${nvme_target_control} config --show-hostid)" > > + def_host_traddr="$(${nvme_target_control} config --show-host-traddr)" > > + def_traddr="$(${nvme_target_control} config --show-traddr)" > > + def_trsvcid="$(${nvme_target_control} config --show-trsvid)" > > + def_subsys_uuid="$(${nvme_target_control} config --show-subsys-uuid)" > > + def_subsysnqn="$(${nvme_target_control} config --show-subsysnqn)" > > I guess that the lines above caused unpredictable values in $def_*, then > caused many of the shellcheck warnings in tests/nvme/*. I'm afraid that another > commit will be required to modify tests/nvme/* and address the warnings. > > > + return > > + fi > > + > > modprobe -q nvmet > > + > > if [[ "${nvme_trtype}" != "loop" ]]; then > > modprobe -q nvmet-"${nvme_trtype}" > > fi > > + > > modprobe -q nvme-"${nvme_trtype}" > > + > > if [[ "${nvme_trtype}" == "rdma" ]]; then > > start_soft_rdma > > for i in $(rdma_network_interfaces) > > @@ -425,6 +452,7 @@ _setup_nvmet() { > > fi > > done > > fi > > + > > if [[ "${nvme_trtype}" = "fc" ]]; then > > modprobe -q nvme-fcloop > > _setup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \ > > @@ -873,11 +901,13 @@ _find_nvme_passthru_loop_dev() { > > > > _nvmet_target_setup() { > > local blkdev_type="${nvmet_blkdev_type}" > > + local subsys_uuid="${def_subsys_uuid}" > > + local subsysnqn="${def_subsysnqn}" > > local blkdev > > + local ARGS=() > > local ctrlkey="" > > local hostkey="" > > - local subsysnqn="${def_subsysnqn}" > > - local subsys_uuid="${def_subsys_uuid}" > > + local blkdev > > local port > > > > while [[ $# -gt 0 ]]; do > > @@ -909,6 +939,22 @@ _nvmet_target_setup() { > > esac > > done > > > > + if [[ -n "${hostkey}" ]]; then > > + ARGS+=(--hostkey "${hostkey}") > > + fi > > + if [[ -n "${ctrlkey}" ]]; then > > + ARGS+=(--ctrkey "${ctrlkey}") > > + fi > > + > > + if [[ -n "${nvme_target_control}" ]]; then > > + eval "${nvme_target_control}" setup \ > > + --subsysnqn "${subsysnqn}" \ > > + --subsys-uuid "${subsys_uuid}" \ > > + --hostnqn "${def_hostnqn}" \ > > + "${ARGS[@]}" &> /dev/null > > I suggest to replaces ${ARGS[@]} with ${ARGS[*]}. It avoids one of the > shellcheck warnings, hopefully. > > > + return > > + fi > > + > > truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)" > > if [[ "${blkdev_type}" == "device" ]]; then > > blkdev="$(losetup -f --show "$(_nvme_def_file_path)")" > > @@ -948,6 +994,13 @@ _nvmet_target_cleanup() { > > esac > > done > > > > + if [[ -n "${nvme_target_control}" ]]; then > > + eval "${nvme_target_control}" cleanup \ > > + --subsysnqn "${subsysnqn}" \ > > + > /dev/null > > + return > > + fi > > + > > _get_nvmet_ports "${subsysnqn}" ports > > > > for port in "${ports[@]}"; do > > -- > > 2.45.2 > >
