From d67119d8bddd63ee6ce2eabdc0087bf34d8c5615 Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Tue, 6 Oct 2020 13:50:37 -0700 Subject: [PATCH] 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 Change-Id: I3f7e87cc1fa6de200c422928c07153fc60fab28c Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/4555 Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot Reviewed-by: Aleksey Marchuk Reviewed-by: Shuhei Matsumoto Reviewed-by: Changpeng Liu --- include/spdk/nvmf.h | 8 ++++---- lib/nvmf/nvmf_internal.h | 7 +++++++ lib/nvmf/subsystem.c | 35 ++++++++++++++++++++++++++--------- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/include/spdk/nvmf.h b/include/spdk/nvmf.h index 980dd08fe..e8176191e 100644 --- a/include/spdk/nvmf.h +++ b/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. diff --git a/lib/nvmf/nvmf_internal.h b/lib/nvmf/nvmf_internal.h index 7e373976c..9b74edbf6 100644 --- a/lib/nvmf/nvmf_internal.h +++ b/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; diff --git a/lib/nvmf/subsystem.c b/lib/nvmf/subsystem.c index a8df0411f..438824fea 100644 --- a/lib/nvmf/subsystem.c +++ b/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 || - subsystem->state == SPDK_NVMF_SUBSYSTEM_PAUSED)) { - return -EAGAIN; - } + pthread_mutex_lock(&subsystem->mutex); 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 || - subsystem->state == SPDK_NVMF_SUBSYSTEM_PAUSED)) { - return -EAGAIN; - } + pthread_mutex_lock(&subsystem->mutex); 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 *