nvmf: Don't require a full subsystem pause to change the allowed hosts

The list of allowed hosts is only checked during handling of CONNECT
commands - not in the main I/O path. Protect that list with a mutex
instead of requiring a full pause of the subsystem to allow
dynamic management of the allowed hosts without impacting any
active I/O.

Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Change-Id: I3f7e87cc1fa6de200c422928c07153fc60fab28c
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/4555
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Mellanox Build Bot
Reviewed-by: Aleksey Marchuk <alexeymar@mellanox.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>

include/spdk/nvmf.h

@@ -462,8 +462,6 @@ struct spdk_nvmf_subsystem *spdk_nvmf_subsystem_get_next(struct spdk_nvmf_subsys
 /**
  * Allow the given host NQN to connect to the given subsystem.
  *
- * May only be performed on subsystems in the PAUSED or INACTIVE states.
- *
  * \param subsystem Subsystem to add host to.
  * \param hostnqn The NQN for the host.
  *
@@ -473,9 +471,11 @@ int spdk_nvmf_subsystem_add_host(struct spdk_nvmf_subsystem *subsystem,
 				 const char *hostnqn);
 
 /**
- * Remove the given host NQN from the allowed hosts whitelist.
+ * Remove the given host NQN from the list of allowed hosts.
  *
- * May only be performed on subsystems in the PAUSED or INACTIVE states.
+ * This call only removes the host from the allowed list of hosts.
+ * If a host with the given NQN is already connected it will not be disconnected,
+ * but it will not be able to create new connections.
  *
  * \param subsystem Subsystem to remove host from.
  * \param hostnqn The NQN for the host.

lib/nvmf/nvmf_internal.h

@@ -274,6 +274,13 @@ struct spdk_nvmf_subsystem {
 	uint32_t					max_allowed_nsid;
 
 	TAILQ_HEAD(, spdk_nvmf_ctrlr)			ctrlrs;
+
+	/* A mutex used to protect the hosts list. Unlike the namespace
+	 * array, this list is not used on the I/O path (it's needed for handling things like
+	 * the CONNECT command), so use a mutex to protect it instead of requiring the subsystem
+	 * state to be paused. This removes the requirement to pause the subsystem when hosts
+	 * are added or removed dynamically. */
+	pthread_mutex_t					mutex;
 	TAILQ_HEAD(, spdk_nvmf_host)			hosts;
 	TAILQ_HEAD(, spdk_nvmf_subsystem_listener)	listeners;
 

lib/nvmf/subsystem.c

@@ -281,6 +281,7 @@ spdk_nvmf_subsystem_create(struct spdk_nvmf_tgt *tgt,
 	subsystem->max_allowed_nsid = num_ns;
 	subsystem->next_cntlid = 0;
 	snprintf(subsystem->subnqn, sizeof(subsystem->subnqn), "%s", nqn);
+	pthread_mutex_init(&subsystem->mutex, NULL);
 	TAILQ_INIT(&subsystem->listeners);
 	TAILQ_INIT(&subsystem->hosts);
 	TAILQ_INIT(&subsystem->ctrlrs);
@@ -306,6 +307,7 @@ spdk_nvmf_subsystem_create(struct spdk_nvmf_tgt *tgt,
 	return subsystem;
 }
 
+/* Must hold subsystem->mutex while calling this function */
 static void
 nvmf_subsystem_remove_host(struct spdk_nvmf_subsystem *subsystem, struct spdk_nvmf_host *host)
 {
@@ -348,10 +350,14 @@ spdk_nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem)
 
 	nvmf_subsystem_remove_all_listeners(subsystem, false);
 
+	pthread_mutex_lock(&subsystem->mutex);
+
 	TAILQ_FOREACH_SAFE(host, &subsystem->hosts, link, host_tmp) {
 		nvmf_subsystem_remove_host(subsystem, host);
 	}
 
+	pthread_mutex_unlock(&subsystem->mutex);
+
 	TAILQ_FOREACH_SAFE(ctrlr, &subsystem->ctrlrs, link, ctrlr_tmp) {
 		nvmf_ctrlr_destruct(ctrlr);
 	}
@@ -369,6 +375,8 @@ spdk_nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem)
 	subsystem->tgt->subsystems[subsystem->id] = NULL;
 	subsystem->tgt->discovery_genctr++;
 
+	pthread_mutex_destroy(&subsystem->mutex);
+
 	free(subsystem);
 }
 
@@ -684,6 +692,7 @@ spdk_nvmf_subsystem_get_next(struct spdk_nvmf_subsystem *subsystem)
 	return NULL;
 }
 
+/* Must hold subsystem->mutex while calling this function */
 static struct spdk_nvmf_host *
 nvmf_subsystem_find_host(struct spdk_nvmf_subsystem *subsystem, const char *hostnqn)
 {
@@ -707,26 +716,28 @@ spdk_nvmf_subsystem_add_host(struct spdk_nvmf_subsystem *subsystem, const char *
 		return -EINVAL;
 	}
 
-	if (!(subsystem->state == SPDK_NVMF_SUBSYSTEM_INACTIVE ||
+	pthread_mutex_lock(&subsystem->mutex);
-	      subsystem->state == SPDK_NVMF_SUBSYSTEM_PAUSED)) {
-		return -EAGAIN;
-	}
 
 	if (nvmf_subsystem_find_host(subsystem, hostnqn)) {
 		/* This subsystem already allows the specified host. */
+		pthread_mutex_unlock(&subsystem->mutex);
 		return 0;
 	}
 
 	host = calloc(1, sizeof(*host));
 	if (!host) {
+		pthread_mutex_unlock(&subsystem->mutex);
 		return -ENOMEM;
 	}
 
 	snprintf(host->nqn, sizeof(host->nqn), "%s", hostnqn);
 
 	TAILQ_INSERT_HEAD(&subsystem->hosts, host, link);
+
 	subsystem->tgt->discovery_genctr++;
 
+	pthread_mutex_unlock(&subsystem->mutex);
+
 	return 0;
 }
 
@@ -735,17 +746,17 @@ spdk_nvmf_subsystem_remove_host(struct spdk_nvmf_subsystem *subsystem, const cha
 {
 	struct spdk_nvmf_host *host;
 
-	if (!(subsystem->state == SPDK_NVMF_SUBSYSTEM_INACTIVE ||
+	pthread_mutex_lock(&subsystem->mutex);
-	      subsystem->state == SPDK_NVMF_SUBSYSTEM_PAUSED)) {
-		return -EAGAIN;
-	}
 
 	host = nvmf_subsystem_find_host(subsystem, hostnqn);
 	if (host == NULL) {
+		pthread_mutex_unlock(&subsystem->mutex);
 		return -ENOENT;
 	}
 
 	nvmf_subsystem_remove_host(subsystem, host);
+	pthread_mutex_unlock(&subsystem->mutex);
+
 	return 0;
 }
 
@@ -771,6 +782,8 @@ spdk_nvmf_subsystem_get_allow_any_host(const struct spdk_nvmf_subsystem *subsyst
 bool
 spdk_nvmf_subsystem_host_allowed(struct spdk_nvmf_subsystem *subsystem, const char *hostnqn)
 {
+	bool allowed;
+
 	if (!hostnqn) {
 		return false;
 	}
@@ -779,7 +792,11 @@ spdk_nvmf_subsystem_host_allowed(struct spdk_nvmf_subsystem *subsystem, const ch
 		return true;
 	}
 
-	return nvmf_subsystem_find_host(subsystem, hostnqn) != NULL;
+	pthread_mutex_lock(&subsystem->mutex);
+	allowed =  nvmf_subsystem_find_host(subsystem, hostnqn) != NULL;
+	pthread_mutex_unlock(&subsystem->mutex);
+
+	return allowed;
 }
 
 struct spdk_nvmf_host *