[PATCH] nvme: don't send keep-alives to the discovery controller

[PATCH] nvme: don't send keep-alives to the discovery controller

[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

[PATCH] test/nvme/003: add test case for patch "nvme: don't send keep-alives to the discovery controller"

[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

Re: [PATCH] nvme: don't send keep-alives to the discovery controller

[Christoph Hellwig]

Looks good,

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

Re: [PATCH] nvme: don't send keep-alives to the discovery controller

[Keith Busch]

Thanks, applied.

Re: [PATCH] test/nvme/003: add test case for patch "nvme: don't send keep-alives to the discovery controller"

[Omar Sandoval]

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!