lib/nvmf: nvmf target stops to listen when subsystem is destroyed

There is a spdk_nvmf_tgt_listen() which opens a port for specified
transport (trid) which opens possibility to accept new connections
from initiators. However there is no counterpart of this function
(i.e. spdk_nvmf_tgt_stop_listen()), which would stop listening.
Instead the current code relies on spdk_nvmf_subsystem_destroy()
to stop the listener, which seems to be wrong.

Fixes #1129

Change-Id: I6e73d8c234dc451f0fee8394132eae34cd4f4756
Signed-off-by: Jan Kryl <jan.kryl@mayadata.io>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/479873
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Alexey Marchuk <alexeymar@mellanox.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>

CHANGELOG.md

@@ -79,6 +79,9 @@ modified to take a string.
 A function table, `spdk_nvmf_transport_ops`, and macro, `SPDK_NVMF_TRANSPORT_REGISTER`, have been added which
 enable registering out of tree transports.
 
+Add `spdk_nvmf_tgt_stop_listen()` that can be used to stop listening for
+incoming connections for specified target and trid. Listener is not stopped
+implicitly upon destruction of a subsystem any more.
 
 ### util
 

include/spdk/nvmf.h

@@ -359,6 +359,19 @@ void spdk_nvmf_tgt_listen(struct spdk_nvmf_tgt *tgt,
 			  spdk_nvmf_tgt_listen_done_fn cb_fn,
 			  void *cb_arg);
 
+/**
+ * Stop accepting new connections at the provided address.
+ *
+ * This is a counterpart to spdk_nvmf_tgt_listen().
+ *
+ * \param tgt The target associated with the listen address.
+ * \param trid The address to stop listening at.
+ *
+ * \return int. 0 on success or a negated errno on failure.
+ */
+int spdk_nvmf_tgt_stop_listen(struct spdk_nvmf_tgt *tgt,
+			      struct spdk_nvme_transport_id *trid);
+
 /**
  * Poll the target for incoming connections.
  *
@@ -687,6 +700,8 @@ const char *spdk_nvmf_host_get_nqn(struct spdk_nvmf_host *host);
 /**
  * Accept new connections on the address provided.
  *
+ * This does not start the listener. Use spdk_nvmf_tgt_listen() for that.
+ *
  * May only be performed on subsystems in the PAUSED or INACTIVE states.
  *
  * \param subsystem Subsystem to add listener to.
@@ -698,7 +713,11 @@ int spdk_nvmf_subsystem_add_listener(struct spdk_nvmf_subsystem *subsystem,
 				     struct spdk_nvme_transport_id *trid);
 
 /**
- * Stop accepting new connections on the address provided
+ * Remove the listener from subsystem.
+ *
+ * New connections to the address won't be propagated to the subsystem.
+ * However to stop listening at target level one must use the
+ * spdk_nvmf_tgt_stop_listen().
  *
  * May only be performed on subsystems in the PAUSED or INACTIVE states.
  *

lib/nvmf/nvmf.c

@@ -280,6 +280,7 @@ spdk_nvmf_tgt_destroy_cb(void *io_device)
 	if (tgt->subsystems) {
 		for (i = 0; i < tgt->max_subsystems; i++) {
 			if (tgt->subsystems[i]) {
+				spdk_nvmf_subsystem_remove_all_listeners(tgt->subsystems[i], true);
 				spdk_nvmf_subsystem_destroy(tgt->subsystems[i]);
 			}
 		}
@@ -571,6 +572,35 @@ spdk_nvmf_tgt_listen(struct spdk_nvmf_tgt *tgt,
 	}
 }
 
+int
+spdk_nvmf_tgt_stop_listen(struct spdk_nvmf_tgt *tgt,
+			  struct spdk_nvme_transport_id *trid)
+{
+	struct spdk_nvmf_transport *transport;
+	const char *trtype;
+	int rc;
+
+	transport = spdk_nvmf_tgt_get_transport(tgt, trid->trstring);
+	if (!transport) {
+		trtype = spdk_nvme_transport_id_trtype_str(trid->trtype);
+		if (trtype != NULL) {
+			SPDK_ERRLOG("Unable to stop listen on transport %s. The transport must be created first.\n",
+				    trtype);
+		} else {
+			SPDK_ERRLOG("The specified trtype %d is unknown. Please make sure that it is properly registered.\n",
+				    trid->trtype);
+		}
+		return -EINVAL;
+	}
+
+	rc = spdk_nvmf_transport_stop_listen(transport, trid);
+	if (rc < 0) {
+		SPDK_ERRLOG("Failed to stop listening on address '%s'\n", trid->traddr);
+		return rc;
+	}
+	return 0;
+}
+
 struct spdk_nvmf_tgt_add_transport_ctx {
 	struct spdk_nvmf_tgt *tgt;
 	struct spdk_nvmf_transport *transport;

lib/nvmf/nvmf_internal.h

@@ -434,6 +434,8 @@ int spdk_nvmf_subsystem_add_ctrlr(struct spdk_nvmf_subsystem *subsystem,
 				  struct spdk_nvmf_ctrlr *ctrlr);
 void spdk_nvmf_subsystem_remove_ctrlr(struct spdk_nvmf_subsystem *subsystem,
 				      struct spdk_nvmf_ctrlr *ctrlr);
+void spdk_nvmf_subsystem_remove_all_listeners(struct spdk_nvmf_subsystem *subsystem,
+		bool stop);
 struct spdk_nvmf_ctrlr *spdk_nvmf_subsystem_get_ctrlr(struct spdk_nvmf_subsystem *subsystem,
 		uint16_t cntlid);
 int spdk_nvmf_ctrlr_async_event_ns_notice(struct spdk_nvmf_ctrlr *ctrlr);

lib/nvmf/nvmf_rpc.c

@@ -43,6 +43,8 @@
 #include "spdk_internal/event.h"
 #include "spdk_internal/log.h"
 
+#include "nvmf_internal.h"
+
 static int
 json_write_hex_str(struct spdk_json_write_ctx *w, const void *data, size_t size)
 {
@@ -458,6 +460,7 @@ spdk_rpc_nvmf_subsystem_stopped(struct spdk_nvmf_subsystem *subsystem,
 	struct spdk_jsonrpc_request *request = cb_arg;
 	struct spdk_json_write_ctx *w;
 
+	spdk_nvmf_subsystem_remove_all_listeners(subsystem, true);
 	spdk_nvmf_subsystem_destroy(subsystem);
 
 	w = spdk_jsonrpc_begin_result(request);
@@ -661,6 +664,7 @@ nvmf_rpc_listen_paused(struct spdk_nvmf_subsystem *subsystem,
 							 "Invalid parameters");
 			ctx->response_sent = true;
 		}
+		spdk_nvmf_tgt_stop_listen(ctx->tgt, &ctx->trid);
 	} else {
 		spdk_jsonrpc_send_error_response(ctx->request, SPDK_JSONRPC_ERROR_INVALID_PARAMS,
 						 "Invalid parameters");

lib/nvmf/subsystem.c

@@ -314,14 +314,17 @@ _spdk_nvmf_subsystem_remove_host(struct spdk_nvmf_subsystem *subsystem, struct s
 
 static void
 _nvmf_subsystem_remove_listener(struct spdk_nvmf_subsystem *subsystem,
-				struct spdk_nvmf_listener *listener)
+				struct spdk_nvmf_listener *listener,
+				bool stop)
 {
 	struct spdk_nvmf_transport *transport;
 
+	if (stop) {
 		transport = spdk_nvmf_tgt_get_transport(subsystem->tgt, listener->trid.trstring);
 		if (transport != NULL) {
 			spdk_nvmf_transport_stop_listen(transport, &listener->trid);
 		}
+	}
 
 	TAILQ_REMOVE(&subsystem->listeners, listener, link);
 	free(listener);
@@ -330,7 +333,6 @@ _nvmf_subsystem_remove_listener(struct spdk_nvmf_subsystem *subsystem,
 void
 spdk_nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem)
 {
-	struct spdk_nvmf_listener	*listener, *listener_tmp;
 	struct spdk_nvmf_host		*host, *host_tmp;
 	struct spdk_nvmf_ctrlr		*ctrlr, *ctrlr_tmp;
 	struct spdk_nvmf_ns		*ns;
@@ -343,9 +345,7 @@ spdk_nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem)
 
 	SPDK_DEBUGLOG(SPDK_LOG_NVMF, "subsystem is %p\n", subsystem);
 
-	TAILQ_FOREACH_SAFE(listener, &subsystem->listeners, link, listener_tmp) {
+	spdk_nvmf_subsystem_remove_all_listeners(subsystem, false);
-		_nvmf_subsystem_remove_listener(subsystem, listener);
-	}
 
 	TAILQ_FOREACH_SAFE(host, &subsystem->hosts, link, host_tmp) {
 		_spdk_nvmf_subsystem_remove_host(subsystem, host);
@@ -800,11 +800,22 @@ spdk_nvmf_subsystem_remove_listener(struct spdk_nvmf_subsystem *subsystem,
 		return -ENOENT;
 	}
 
-	_nvmf_subsystem_remove_listener(subsystem, listener);
+	_nvmf_subsystem_remove_listener(subsystem, listener, false);
 
 	return 0;
 }
 
+void
+spdk_nvmf_subsystem_remove_all_listeners(struct spdk_nvmf_subsystem *subsystem,
+		bool stop)
+{
+	struct spdk_nvmf_listener       *listener, *listener_tmp;
+
+	TAILQ_FOREACH_SAFE(listener, &subsystem->listeners, link, listener_tmp) {
+		_nvmf_subsystem_remove_listener(subsystem, listener, stop);
+	}
+}
+
 bool
 spdk_nvmf_subsystem_listener_allowed(struct spdk_nvmf_subsystem *subsystem,
 				     struct spdk_nvme_transport_id *trid)