linux-nvme.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To: Sagi Grimberg <sagi@grimberg.me>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	Omar Sandoval <osandov@osandov.com>
Cc: Keith Busch <kbusch@kernel.org>,
	Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH v2 3/7] nvme: make tests transport type agnostic
Date: Fri, 7 Aug 2020 03:09:27 +0000	[thread overview]
Message-ID: <BYAPR04MB4965624DFCE5A870BDE20A4686490@BYAPR04MB4965.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20200806191518.593880-4-sagi@grimberg.me

On 8/6/20 12:15, Sagi Grimberg wrote:
> Pass in nvme_trtype to common routines that can
> support multiple transport types.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   tests/nvme/002 |  4 ++--
>   tests/nvme/003 |  4 ++--
>   tests/nvme/004 |  6 +++---
>   tests/nvme/005 |  6 +++---
>   tests/nvme/006 |  2 +-
>   tests/nvme/007 |  2 +-
>   tests/nvme/008 |  6 +++---
>   tests/nvme/009 |  6 +++---
>   tests/nvme/010 |  6 +++---
>   tests/nvme/011 |  6 +++---
>   tests/nvme/012 |  6 +++---
>   tests/nvme/013 |  6 +++---
>   tests/nvme/014 |  6 +++---
>   tests/nvme/015 |  6 +++---
>   tests/nvme/016 |  2 +-
>   tests/nvme/017 |  2 +-
>   tests/nvme/018 |  6 +++---
>   tests/nvme/019 |  6 +++---
>   tests/nvme/020 |  6 +++---
>   tests/nvme/021 |  6 +++---
>   tests/nvme/022 |  6 +++---
>   tests/nvme/023 |  6 +++---
>   tests/nvme/024 |  6 +++---
>   tests/nvme/025 |  6 +++---
>   tests/nvme/026 |  6 +++---
>   tests/nvme/027 |  6 +++---
>   tests/nvme/028 |  8 ++++----
>   tests/nvme/029 |  6 +++---
>   tests/nvme/030 |  2 +-
>   tests/nvme/031 |  4 ++--
>   tests/nvme/rc  | 39 ++++++++++++++++++++++++++++++++-------
>   31 files changed, 110 insertions(+), 85 deletions(-)
> 
> diff --git a/tests/nvme/002 b/tests/nvme/002
> index 999e222705bf..8540623497c7 100755
> --- a/tests/nvme/002
> +++ b/tests/nvme/002
> @@ -21,7 +21,7 @@ test() {
>   
>   	local iterations=1000
>   	local port
> -	port="$(_create_nvmet_port "loop")"
> +	port="$(_create_nvmet_port ${nvme_trtype})"
Is there a way to directly use nvme_trtype especially in rc ?
if not disregard this comment.
>   
>   	local loop_dev
>   	loop_dev="$(losetup -f)"
> @@ -31,7 +31,7 @@ test() {
>   		_add_nvmet_subsys_to_port "${port}" "blktests-subsystem-$i"
>   	done
>   
> -	_nvme_discover "loop" | _filter_discovery
> +	_nvme_discover "${nvme_trtype}" | _filter_discovery
Same here for nvme_trtype.
>   
>   	for ((i = iterations - 1; i >= 0; i--)); do
>   		_remove_nvmet_subsystem_from_port "${port}" "blktests-subsystem-$i"
> diff --git a/tests/nvme/003 b/tests/nvme/003
> index 6ea6a23b7942..68f823011d7d 100755
> --- a/tests/nvme/003
> +++ b/tests/nvme/003
> @@ -21,7 +21,7 @@ test() {
>   	_setup_nvmet
>   
>   	local port
> -	port="$(_create_nvmet_port "loop")"
> +	port="$(_create_nvmet_port "${nvme_trtype}")"
>   
>   	local loop_dev
>   	loop_dev="$(losetup -f)"
> @@ -29,7 +29,7 @@ test() {
>   	_create_nvmet_subsystem "blktests-subsystem-1" "${loop_dev}"
>   	_add_nvmet_subsys_to_port "${port}" "blktests-subsystem-1"
>   
> -	_nvme_connect_subsys "loop" "nqn.2014-08.org.nvmexpress.discovery"
> +	_nvme_connect_subsys "${nvme_trtype}" "nqn.2014-08.org.nvmexpress.discovery"
>   
Same here for nvme_trtype.
>   	# This is ugly but checking for the absence of error messages is ...
>   	sleep 10
> diff --git a/tests/nvme/004 b/tests/nvme/004

> diff --git a/tests/nvme/018 b/tests/nvme/018
> index 4863274cc525..43340d3c4d25 100755
> --- a/tests/nvme/018
> +++ b/tests/nvme/018
> @@ -29,12 +29,12 @@ test() {
>   
>   	_create_nvmet_subsystem "${subsys_name}" "${file_path}" \
>   		 "91fdba0d-f87b-4c25-b80f-db7be1418b9e"
> -	port="$(_create_nvmet_port "loop")"
> +	port="$(_create_nvmet_port ${nvme_trtype})"
>   	_add_nvmet_subsys_to_port "${port}" "${subsys_name}"
>   
> -	_nvme_connect_subsys "loop" "${subsys_name}"
> +	_nvme_connect_subsys ${nvme_trtype} "${subsys_name}"
>   
> -	nvmedev="$(_find_nvme_loop_dev)"
> +	nvmedev="$(_find_nvme_dev)"
>   	cat "/sys/block/${nvmedev}n1/uuid"
>   	cat "/sys/block/${nvmedev}n1/wwid"
>   
> diff --git a/tests/nvme/019 b/tests/nvme/019
> index 19c5b4755387..98d82ae21b42 100755
> --- a/tests/nvme/019
> +++ b/tests/nvme/019
> @@ -33,12 +33,12 @@ test() {
>   
>   	_create_nvmet_subsystem "${subsys_name}" "${loop_dev}" \
>   		"91fdba0d-f87b-4c25-b80f-db7be1418b9e"
> -	port="$(_create_nvmet_port "loop")"
> +	port="$(_create_nvmet_port ${nvme_trtype})"> diff --git a/tests/nvme/004 b/tests/nvme/004
> index 7ea539fda55e..af434674beaa 100755
> --- a/tests/nvme/004
> +++ b/tests/nvme/004
> @@ -22,7 +22,7 @@ test() {
>   	_setup_nvmet
>   
>   	local port
> -	port="$(_create_nvmet_port "loop")"
> +	port="$(_create_nvmet_port ${nvme_trtype})"
>   
>   	truncate -s 1G "$TMPDIR/img"
>   
> @@ -33,10 +33,10 @@ test() {
>   		"91fdba0d-f87b-4c25-b80f-db7be1418b9e"
>   	_add_nvmet_subsys_to_port "${port}" "blktests-subsystem-1"
>   
> -	_nvme_connect_subsys "loop" "blktests-subsystem-1"
> +	_nvme_connect_subsys ${nvme_trtype} "blktests-subsystem-1"
>   
>   	local nvmedev
> -	nvmedev="$(_find_nvme_loop_dev)"
> +	nvmedev="$(_find_nvme_dev)"
>   	cat "/sys/block/${nvmedev}n1/uuid"
>   	cat "/sys/block/${nvmedev}n1/wwid"

Since we are touching nvmedev can we move above uuid and wwid to
a wrapper something like _nvme_show_uuid_wwid ${nvmedev}n1 ?

>
> @@ -36,12 +36,12 @@ test() {
>   
>   	loop_dev="$(losetup -f --show "$TMPDIR/img")"
>   
> -	port="$(_create_nvmet_port "loop")"
> +	port="$(_create_nvmet_port ${nvme_trtype})"
>   
>   	for ((i = 0; i < iterations; i++)); do
>   		_create_nvmet_subsystem "${subsys}$i" "${loop_dev}"
>   		_add_nvmet_subsys_to_port "${port}" "${subsys}$i"
> -		_nvme_connect_subsys "loop" "${subsys}$i"
> +		_nvme_connect_subsys ${nvme_trtype} "${subsys}$i"
Same here for nvme_trtype as first comment.
>   		_nvme_disconnect_subsys "${subsys}$i" >> "${FULL}" 2>&1
>   		_remove_nvmet_subsystem_from_port "${port}" "${subsys}$i"
>   		_remove_nvmet_subsystem "${subsys}$i"
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index 6d57cf591300..191f0497416a 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -6,6 +6,9 @@
>   
>   . common/rc
>   
> +def_traddr="127.0.0.1"
> +def_adrfam="ipv4"
> +def_trsvcid="4420"
>   nvme_trtype=${nvme_trtype:-"loop"}
>   
>   _nvme_requires() {
> @@ -62,8 +65,8 @@ _cleanup_nvmet() {
>   	for dev in /sys/class/nvme/nvme*; do
>   		dev="$(basename "$dev")"
>   		transport="$(cat "/sys/class/nvme/${dev}/transport")"
> -		if [[ "$transport" == "loop" ]]; then
> -			echo "WARNING: Test did not clean up loop device: ${dev}"
> +		if [[ "$transport" == "${nvme_trtype}" ]]; then
> +			echo "WARNING: Test did not clean up ${nvme_trtype} device: ${dev}"
>   			_nvme_disconnect_ctrl "${dev}"
>   		fi
>   	done
> @@ -87,14 +90,20 @@ _cleanup_nvmet() {
>   	shopt -u nullglob
>   	trap SIGINT
>   
> -	modprobe -r nvme-loop 2>/dev/null
> +	modprobe -r nvme-${nvme_trtype} 2>/dev/null
> +	if [[ "${nvme_trtype}" != "loop" ]]; then
> +		modprobe -r nvmet-${nvme_trtype} 2>/dev/null
This is not from your patch but I'd keep the error message it has
turned out to be useful for me when debugging refcount problem 
especially unload and load scenario.

> +	fi
>   	modprobe -r nvmet 2>/dev/null
>   }
>   
>   _setup_nvmet() {
>   	_register_test_cleanup _cleanup_nvmet
>   	modprobe nvmet
> -	modprobe nvme-loop
> +	if [[ "${nvme_trtype}" != "loop" ]]; then
> +		modprobe nvmet-${nvme_trtype}
> +	fi
> +	modprobe nvme-${nvme_trtype}
>   }
>   
>   _nvme_disconnect_ctrl() {
> @@ -112,20 +121,33 @@ _nvme_disconnect_subsys() {
>   _nvme_connect_subsys() {
>   	local trtype="$1"
>   	local subsysnqn="$2"
> +	local traddr="${3:-$def_traddr}"
> +	local trsvcid="${4:-$def_trsvcid}"
>   
>   	cmd="nvme connect -t ${trtype} -n ${subsysnqn}"
> +	if [[ "${trtype}" != "loop" ]]; then
> +		cmd=$cmd" -a ${traddr} -s ${trsvcid}"
> +	fi
>   	eval $cmd
>   }
>   
>   _nvme_discover() {
>   	local trtype="$1"
> +	local traddr="${2:-$def_traddr}"
> +	local trsvcid="${3:-$def_trsvcid}"
>   
>   	cmd="nvme discover -t ${trtype}"
> +	if [[ "${trtype}" != "loop" ]]; then
> +		cmd=$cmd" -a ${traddr} -s ${trsvcid}"
> +	fi
>   	eval $cmd
>   }
>   
>   _create_nvmet_port() {
>   	local trtype="$1"
> +	local traddr="${2:-$def_traddr}"
> +	local adrfam="${3:-$def_adrfam}"
> +	local trsvcid="${4:-$def_trsvcid}"
>   
>   	local port
>   	for ((port = 0; ; port++)); do
> @@ -136,6 +158,9 @@ _create_nvmet_port() {
>   
>   	mkdir "${NVMET_CFS}/ports/${port}"
>   	echo "${trtype}" > "${NVMET_CFS}/ports/${port}/addr_trtype"
> +	echo "${traddr}" > "${NVMET_CFS}/ports/${port}/addr_traddr"
> +	echo "${adrfam}" > "${NVMET_CFS}/ports/${port}/addr_adrfam"
> +	echo "${trsvcid}" > "${NVMET_CFS}/ports/${port}/addr_trsvcid"

Do we need argument count check before we apply these to configfs ?
>   
>   	echo "${port}"
>   }
> @@ -207,13 +232,13 @@ _remove_nvmet_subsystem_from_port() {
>   	rm "${NVMET_CFS}/ports/${port}/subsystems/${nvmet_subsystem}"
>   }
>   
> -_find_nvme_loop_dev() {
> +_find_nvme_dev() {
>   	local dev
>   	local transport
>   	for dev in /sys/class/nvme/nvme*; do
>   		dev="$(basename "$dev")"
>   		transport="$(cat "/sys/class/nvme/${dev}/transport")"
> -		if [[ "$transport" == "loop" ]]; then
> +		if [[ "$transport" == "${nvme_trtype}" ]]; then
>   			echo "$dev"
>   			for ((i = 0; i < 10; i++)); do
>   				if [[ -e /sys/block/$dev/uuid &&
> @@ -233,6 +258,6 @@ _filter_discovery() {
>   }
>   
>   _discovery_genctr() {
> -	_nvme_discover "loop" |
> +	_nvme_discover "${nvme_trtype}" |
>   		sed -n -e 's/^.*Generation counter \([0-9]\+\).*$/\1/p'
>   }
> 


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

  reply	other threads:[~2020-08-07  3:09 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-06 19:15 [PATCH v2 0/7] blktests: Add support to run nvme tests with tcp/rdma transports Sagi Grimberg
2020-08-06 19:15 ` [PATCH v2 1/7] nvme: consolidate nvme requirements based on transport type Sagi Grimberg
2020-08-07  2:41   ` Chaitanya Kulkarni
2020-08-07 17:14     ` Sagi Grimberg
2020-08-06 19:15 ` [PATCH v2 2/7] nvme: consolidate some nvme-cli utility functions Sagi Grimberg
2020-08-07  2:53   ` Chaitanya Kulkarni
2020-08-07 17:15     ` Sagi Grimberg
2020-08-08  1:47       ` Chaitanya Kulkarni
2020-08-06 19:15 ` [PATCH v2 3/7] nvme: make tests transport type agnostic Sagi Grimberg
2020-08-07  3:09   ` Chaitanya Kulkarni [this message]
2020-08-07 17:19     ` Sagi Grimberg
2020-08-10 17:35       ` Chaitanya Kulkarni
2020-08-06 19:15 ` [PATCH v2 4/7] tests/nvme: restrict tests to specific transports Sagi Grimberg
2020-08-07  3:15   ` Chaitanya Kulkarni
2020-08-07 17:21     ` Sagi Grimberg
2020-08-06 19:15 ` [PATCH v2 5/7] nvme: support nvme-tcp when runinng tests Sagi Grimberg
2020-08-07  3:16   ` Chaitanya Kulkarni
2020-08-07 23:46     ` Sagi Grimberg
2020-08-08  1:49       ` Chaitanya Kulkarni
2020-08-06 19:15 ` [PATCH v2 6/7] common/multipath-over-rdma: don't retry module unload Sagi Grimberg
2020-08-07  3:18   ` Chaitanya Kulkarni
2020-08-07 14:03   ` Bart Van Assche
2020-08-07 17:23     ` Sagi Grimberg
2020-08-07 17:34       ` Bart Van Assche
2020-08-07 17:50         ` Sagi Grimberg
2020-08-06 19:15 ` [PATCH v2 7/7] nvme: support rdma transport type Sagi Grimberg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BYAPR04MB4965624DFCE5A870BDE20A4686490@BYAPR04MB4965.namprd04.prod.outlook.com \
    --to=chaitanya.kulkarni@wdc.com \
    --cc=Johannes.Thumshirn@wdc.com \
    --cc=hch@lst.de \
    --cc=kbusch@kernel.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=osandov@osandov.com \
    --cc=sagi@grimberg.me \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).