LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] nvme: don't send keep-alives to the discovery controller
@ 2018-03-27  9:28 Johannes Thumshirn
  2018-03-27  9:31 ` [PATCH] test/nvme/003: add test case for patch "nvme: don't send keep-alives to the discovery controller" Johannes Thumshirn
  2018-03-28  8:04 ` [PATCH] nvme: don't send keep-alives to the discovery controller Christoph Hellwig
  0 siblings, 2 replies; 5+ messages in thread
From: Johannes Thumshirn @ 2018-03-27  9:28 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch, Sagi Grimberg
  Cc: Linux Kernel Mailinglist, Linux NVMe Mailinglist, marting,
	Johannes Thumshirn

NVMe over Fabrics 1.0 Section 5.2 "Discovery Controller Properties and
Command Support" Figure 31 "Discovery Controller – Admin Commands"
explicitly listst all commands but "Get Log Page" and "Identify" as
reserved, but NetApp report the Linux host is sending Keep Alive
commands to the discovery controller, which is a violation of the
Spec.

We're already checking for discovery controllers when configuring the
keep alive timeout but when creating a discovery controller we're not
hard wiring the keep alive timeout to 0 and thus remain on
NVME_DEFAULT_KATO for the discovery controller.

This can be easily remproduced when issuing a direct connect to the
discovery susbsystem using:
'nvme connect [...] --nqn=nqn.2014-08.org.nvmexpress.discovery'

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Fixes: 07bfcd09a288 ("nvme-fabrics: add a generic NVMe over Fabrics library")
Reported-by: Martin George <marting@netapp.com>
---
 drivers/nvme/host/fabrics.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 8f0f34d06d46..3583f9492a45 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -608,8 +608,10 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			opts->discovery_nqn =
 				!(strcmp(opts->subsysnqn,
 					 NVME_DISC_SUBSYS_NAME));
-			if (opts->discovery_nqn)
+			if (opts->discovery_nqn) {
+				opts->kato = 0;
 				opts->nr_io_queues = 0;
+			}
 			break;
 		case NVMF_OPT_TRADDR:
 			p = match_strdup(args);
-- 
2.12.3

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] test/nvme/003: add test case for patch "nvme: don't send keep-alives to the discovery controller"
  2018-03-27  9:28 [PATCH] nvme: don't send keep-alives to the discovery controller Johannes Thumshirn
@ 2018-03-27  9:31 ` Johannes Thumshirn
  2018-04-07  4:01   ` Omar Sandoval
  2018-03-28  8:04 ` [PATCH] nvme: don't send keep-alives to the discovery controller Christoph Hellwig
  1 sibling, 1 reply; 5+ messages in thread
From: Johannes Thumshirn @ 2018-03-27  9:31 UTC (permalink / raw)
  To: Omar Sandoval
  Cc: Linux Kernel Mailinglist, Linux NVMe Mailinglist, Johannes Thumshirn

Add a regression test for the patch titled "nvme: don't send
keep-alives to the discovery controller".

This patch creates a local loopback nvme target and then connects to
it. If the patch is not applied we see two error messages in dmesg:
"failed nvme_keep_alive_end_io error=" from the nvme host and "nvmet:
unsupported cmd 24" from the nvme target. If the patch is applied
dmesg is quiet.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 tests/nvme/003     | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/003.out |  2 ++
 2 files changed, 63 insertions(+)
 create mode 100755 tests/nvme/003
 create mode 100644 tests/nvme/003.out

diff --git a/tests/nvme/003 b/tests/nvme/003
new file mode 100755
index 000000000000..5353de5ae9c3
--- /dev/null
+++ b/tests/nvme/003
@@ -0,0 +1,61 @@
+#!/bin/bash
+#
+# Regression test for patch "nvme: don't send keep-alives to the discovery
+# controller"
+#
+# Copyright (C) 2018 Johannes Thumshirn
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+DESCRIPTION="Test if we're sending keep-alives to a discovery controller"
+
+QUICK=1
+
+requires() {
+	_have_program nvme && _have_module nvme-loop && _have_module loop \
+		&& _have_configfs
+}
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	modprobe nvmet
+	modprobe nvme-loop
+
+	port=$(_create_nvmet_port "loop")
+
+	loop_dev="$(losetup -f)"
+
+	_create_nvmet_subsystem "blktests-subsystem-1" ${loop_dev}
+	_add_nvmet_subsys_to_port ${port} "blktests-subsystem-1"
+
+	nvme connect -t loop -n nqn.2014-08.org.nvmexpress.discovery
+
+	# This is ugly but checking for the absence of error messages is ...
+	sleep 10
+
+	if dmesg | grep -q "failed nvme_keep_alive_end_io error="; then
+		echo "Fail"
+	fi
+
+	if dmesg | grep -q "nvmet: unsupported cmd 24"; then
+		echo "Fail"
+	fi
+
+	_remove_nvmet_subsystem_from_port ${port} "blktests-subsystem-1"
+	_remove_nvmet_subsystem "blktests-subsystem-1"
+	_remove_nvmet_port ${port}
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/003.out b/tests/nvme/003.out
new file mode 100644
index 000000000000..01b275612159
--- /dev/null
+++ b/tests/nvme/003.out
@@ -0,0 +1,2 @@
+Running nvme/003
+Test complete
-- 
2.12.3

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] nvme: don't send keep-alives to the discovery controller
  2018-03-27  9:28 [PATCH] nvme: don't send keep-alives to the discovery controller Johannes Thumshirn
  2018-03-27  9:31 ` [PATCH] test/nvme/003: add test case for patch "nvme: don't send keep-alives to the discovery controller" Johannes Thumshirn
@ 2018-03-28  8:04 ` Christoph Hellwig
  2018-03-28 15:07   ` Keith Busch
  1 sibling, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2018-03-28  8:04 UTC (permalink / raw)
  To: Johannes Thumshirn
  Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg,
	Linux Kernel Mailinglist, Linux NVMe Mailinglist, marting

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] nvme: don't send keep-alives to the discovery controller
  2018-03-28  8:04 ` [PATCH] nvme: don't send keep-alives to the discovery controller Christoph Hellwig
@ 2018-03-28 15:07   ` Keith Busch
  0 siblings, 0 replies; 5+ messages in thread
From: Keith Busch @ 2018-03-28 15:07 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Johannes Thumshirn, Sagi Grimberg, Linux Kernel Mailinglist,
	Linux NVMe Mailinglist, marting

Thanks, applied.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] test/nvme/003: add test case for patch "nvme: don't send keep-alives to the discovery controller"
  2018-03-27  9:31 ` [PATCH] test/nvme/003: add test case for patch "nvme: don't send keep-alives to the discovery controller" Johannes Thumshirn
@ 2018-04-07  4:01   ` Omar Sandoval
  0 siblings, 0 replies; 5+ messages in thread
From: Omar Sandoval @ 2018-04-07  4:01 UTC (permalink / raw)
  To: Johannes Thumshirn; +Cc: Linux Kernel Mailinglist, Linux NVMe Mailinglist

On Tue, Mar 27, 2018 at 11:31:47AM +0200, Johannes Thumshirn wrote:
> Add a regression test for the patch titled "nvme: don't send
> keep-alives to the discovery controller".
> 
> This patch creates a local loopback nvme target and then connects to
> it. If the patch is not applied we see two error messages in dmesg:
> "failed nvme_keep_alive_end_io error=" from the nvme host and "nvmet:
> unsupported cmd 24" from the nvme target. If the patch is applied
> dmesg is quiet.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

Applied, thanks, Johannes!

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-04-07  4:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-27  9:28 [PATCH] nvme: don't send keep-alives to the discovery controller Johannes Thumshirn
2018-03-27  9:31 ` [PATCH] test/nvme/003: add test case for patch "nvme: don't send keep-alives to the discovery controller" Johannes Thumshirn
2018-04-07  4:01   ` Omar Sandoval
2018-03-28  8:04 ` [PATCH] nvme: don't send keep-alives to the discovery controller Christoph Hellwig
2018-03-28 15:07   ` Keith Busch

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).