From 16d0fbd0d6d1ee84a7b333d85d0a70d353efca92 Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Tue, 8 May 2018 16:05:28 -0700 Subject: [PATCH] nvmf: Statically size the subsystems arrays The realloc breaks TAILQs inside the structures, which causes subtle bugs. Instead, statically allocate all of the subsystem arrays. This sets up the maximum number of subsystems to be configurable, but does not actually expose it through the config file yet. Change-Id: I7347b6002b6babc0678ce59cd218a454fe3a6f88 Signed-off-by: Ben Walker Reviewed-on: https://review.gerrithub.io/410521 Tested-by: SPDK Automated Test System Reviewed-by: Daniel Verkamp Reviewed-by: Jim Harris --- include/spdk/nvmf.h | 1 + lib/nvmf/ctrlr_discovery.c | 2 +- lib/nvmf/nvmf.c | 42 +++++++------------ lib/nvmf/nvmf_internal.h | 3 +- lib/nvmf/subsystem.c | 18 +++----- .../ctrlr_discovery.c/ctrlr_discovery_ut.c | 4 ++ test/unit/lib/nvmf/subsystem.c/subsystem_ut.c | 9 ++++ 7 files changed, 35 insertions(+), 44 deletions(-) diff --git a/include/spdk/nvmf.h b/include/spdk/nvmf.h index 9f8a7f9a6..66462b403 100644 --- a/include/spdk/nvmf.h +++ b/include/spdk/nvmf.h @@ -66,6 +66,7 @@ struct spdk_nvmf_tgt_opts { uint16_t max_qpairs_per_ctrlr; uint32_t in_capsule_data_size; uint32_t max_io_size; + uint32_t max_subsystems; }; /** * Initialize the default value of opts. diff --git a/lib/nvmf/ctrlr_discovery.c b/lib/nvmf/ctrlr_discovery.c index 840da2f7b..a2fe5ec16 100644 --- a/lib/nvmf/ctrlr_discovery.c +++ b/lib/nvmf/ctrlr_discovery.c @@ -69,7 +69,7 @@ nvmf_update_discovery_log(struct spdk_nvmf_tgt *tgt) return; } - for (sid = 0; sid < tgt->max_sid; sid++) { + for (sid = 0; sid < tgt->opts.max_subsystems; sid++) { subsystem = tgt->subsystems[sid]; if (subsystem == NULL) { continue; diff --git a/lib/nvmf/nvmf.c b/lib/nvmf/nvmf.c index 554c7e6e3..823bc967c 100644 --- a/lib/nvmf/nvmf.c +++ b/lib/nvmf/nvmf.c @@ -46,12 +46,11 @@ SPDK_LOG_REGISTER_COMPONENT("nvmf", SPDK_LOG_NVMF) -#define MAX_SUBSYSTEMS 4 - #define SPDK_NVMF_DEFAULT_MAX_QUEUE_DEPTH 128 #define SPDK_NVMF_DEFAULT_MAX_QPAIRS_PER_CTRLR 64 #define SPDK_NVMF_DEFAULT_IN_CAPSULE_DATA_SIZE 4096 #define SPDK_NVMF_DEFAULT_MAX_IO_SIZE 131072 +#define SPDK_NVMF_DEFAULT_MAX_SUBSYSTEMS 1024 void spdk_nvmf_tgt_opts_init(struct spdk_nvmf_tgt_opts *opts) @@ -60,6 +59,7 @@ spdk_nvmf_tgt_opts_init(struct spdk_nvmf_tgt_opts *opts) opts->max_qpairs_per_ctrlr = SPDK_NVMF_DEFAULT_MAX_QPAIRS_PER_CTRLR; opts->in_capsule_data_size = SPDK_NVMF_DEFAULT_IN_CAPSULE_DATA_SIZE; opts->max_io_size = SPDK_NVMF_DEFAULT_MAX_IO_SIZE; + opts->max_subsystems = SPDK_NVMF_DEFAULT_MAX_SUBSYSTEMS; } static int @@ -95,13 +95,13 @@ spdk_nvmf_tgt_create_poll_group(void *io_device, void *ctx_buf) spdk_nvmf_poll_group_add_transport(group, transport); } - group->num_sgroups = tgt->max_sid; - group->sgroups = calloc(group->num_sgroups, sizeof(struct spdk_nvmf_subsystem_poll_group)); + group->num_sgroups = tgt->opts.max_subsystems; + group->sgroups = calloc(tgt->opts.max_subsystems, sizeof(struct spdk_nvmf_subsystem_poll_group)); if (!group->sgroups) { return -1; } - for (sid = 0; sid < group->num_sgroups; sid++) { + for (sid = 0; sid < tgt->opts.max_subsystems; sid++) { struct spdk_nvmf_subsystem *subsystem; subsystem = tgt->subsystems[sid]; @@ -168,10 +168,14 @@ spdk_nvmf_tgt_create(struct spdk_nvmf_tgt_opts *opts) tgt->discovery_genctr = 0; tgt->discovery_log_page = NULL; tgt->discovery_log_page_size = 0; - tgt->subsystems = NULL; - tgt->max_sid = 0; TAILQ_INIT(&tgt->transports); + tgt->subsystems = calloc(tgt->opts.max_subsystems, sizeof(struct spdk_nvmf_subsystem *)); + if (!tgt->subsystems) { + free(tgt); + return NULL; + } + spdk_io_device_register(tgt, spdk_nvmf_tgt_create_poll_group, spdk_nvmf_tgt_destroy_poll_group, @@ -198,7 +202,7 @@ spdk_nvmf_tgt_destroy(struct spdk_nvmf_tgt *tgt) } if (tgt->subsystems) { - for (i = 0; i < tgt->max_sid; i++) { + for (i = 0; i < tgt->opts.max_subsystems; i++) { if (tgt->subsystems[i]) { spdk_nvmf_subsystem_destroy(tgt->subsystems[i]); } @@ -311,7 +315,7 @@ spdk_nvmf_tgt_find_subsystem(struct spdk_nvmf_tgt *tgt, const char *subnqn) return NULL; } - for (sid = 0; sid < tgt->max_sid; sid++) { + for (sid = 0; sid < tgt->opts.max_subsystems; sid++) { subsystem = tgt->subsystems[sid]; if (subsystem == NULL) { continue; @@ -445,21 +449,7 @@ poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, /* Make sure our poll group has memory for this subsystem allocated */ if (subsystem->id >= group->num_sgroups) { - void *buf; - - buf = realloc(group->sgroups, (subsystem->id + 1) * sizeof(*sgroup)); - if (!buf) { - return -ENOMEM; - } - - group->sgroups = buf; - - /* Zero out the newly allocated memory */ - memset(&group->sgroups[group->num_sgroups], - 0, - (subsystem->id + 1 - group->num_sgroups) * sizeof(group->sgroups[0])); - - group->num_sgroups = subsystem->id + 1; + return -ENOMEM; } sgroup = &group->sgroups[subsystem->id]; @@ -630,10 +620,6 @@ spdk_nvmf_poll_group_resume_subsystem(struct spdk_nvmf_poll_group *group, return rc; } - /* poll_group_update_subsystem may realloc the sgroups array. We need - * to do a new lookup to get the correct pointer. */ - sgroup = &group->sgroups[subsystem->id]; - sgroup->state = SPDK_NVMF_SUBSYSTEM_ACTIVE; /* Release all queued requests */ diff --git a/lib/nvmf/nvmf_internal.h b/lib/nvmf/nvmf_internal.h index 2945e205a..b8e9ef056 100644 --- a/lib/nvmf/nvmf_internal.h +++ b/lib/nvmf/nvmf_internal.h @@ -60,9 +60,8 @@ struct spdk_nvmf_tgt { uint64_t discovery_genctr; - /* Array of subsystem pointers of size max_sid indexed by sid */ + /* Array of subsystem pointers of size max_subsystems indexed by sid */ struct spdk_nvmf_subsystem **subsystems; - uint32_t max_sid; struct spdk_nvmf_discovery_log_page *discovery_log_page; size_t discovery_log_page_size; diff --git a/lib/nvmf/subsystem.c b/lib/nvmf/subsystem.c index 3b70720a6..466f5aa85 100644 --- a/lib/nvmf/subsystem.c +++ b/lib/nvmf/subsystem.c @@ -253,21 +253,13 @@ spdk_nvmf_subsystem_create(struct spdk_nvmf_tgt *tgt, } /* Find a free subsystem id (sid) */ - for (sid = 0; sid < tgt->max_sid; sid++) { + for (sid = 0; sid < tgt->opts.max_subsystems; sid++) { if (tgt->subsystems[sid] == NULL) { break; } } - if (sid == tgt->max_sid) { - struct spdk_nvmf_subsystem **subsys_array; - /* No free slots. Add more. */ - tgt->max_sid++; - subsys_array = realloc(tgt->subsystems, tgt->max_sid * sizeof(struct spdk_nvmf_subsystem *)); - if (!subsys_array) { - tgt->max_sid--; - return NULL; - } - tgt->subsystems = subsys_array; + if (sid >= tgt->opts.max_subsystems) { + return NULL; } subsystem = calloc(1, sizeof(struct spdk_nvmf_subsystem)); @@ -560,7 +552,7 @@ spdk_nvmf_subsystem_get_first(struct spdk_nvmf_tgt *tgt) struct spdk_nvmf_subsystem *subsystem; uint32_t sid; - for (sid = 0; sid < tgt->max_sid; sid++) { + for (sid = 0; sid < tgt->opts.max_subsystems; sid++) { subsystem = tgt->subsystems[sid]; if (subsystem) { return subsystem; @@ -582,7 +574,7 @@ spdk_nvmf_subsystem_get_next(struct spdk_nvmf_subsystem *subsystem) tgt = subsystem->tgt; - for (sid = subsystem->id + 1; sid < tgt->max_sid; sid++) { + for (sid = subsystem->id + 1; sid < tgt->opts.max_subsystems; sid++) { subsystem = tgt->subsystems[sid]; if (subsystem) { return subsystem; diff --git a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c index 22faad015..97633a80b 100644 --- a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c +++ b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c @@ -208,6 +208,10 @@ test_discovery_log(void) struct spdk_nvmf_discovery_log_page_entry *entry; struct spdk_nvme_transport_id trid = {}; + tgt.opts.max_subsystems = 1024; + tgt.subsystems = calloc(tgt.opts.max_subsystems, sizeof(struct spdk_nvmf_subsystem *)); + SPDK_CU_ASSERT_FATAL(tgt.subsystems != NULL); + /* Add one subsystem and verify that the discovery log contains it */ subsystem = spdk_nvmf_subsystem_create(&tgt, "nqn.2016-06.io.spdk:subsystem1", SPDK_NVMF_SUBTYPE_NVME, 0); diff --git a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c index b8395e08f..a08621974 100644 --- a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c +++ b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c @@ -243,6 +243,10 @@ test_spdk_nvmf_subsystem_add_ns(void) struct spdk_nvmf_ns_opts ns_opts; uint32_t nsid; + tgt.opts.max_subsystems = 1024; + tgt.subsystems = calloc(tgt.opts.max_subsystems, sizeof(struct spdk_nvmf_subsystem *)); + SPDK_CU_ASSERT_FATAL(tgt.subsystems != NULL); + /* Allow NSID to be assigned automatically */ spdk_nvmf_ns_opts_get_defaults(&ns_opts, sizeof(ns_opts)); nsid = spdk_nvmf_subsystem_add_ns(&subsystem, &bdev1, &ns_opts, sizeof(ns_opts)); @@ -280,6 +284,7 @@ test_spdk_nvmf_subsystem_add_ns(void) spdk_nvmf_subsystem_remove_ns(&subsystem, 5, subsystem_ns_remove_cb, NULL); free(subsystem.ns); + free(tgt.subsystems); } static void @@ -289,6 +294,10 @@ nvmf_test_create_subsystem(void) char nqn[256]; struct spdk_nvmf_subsystem *subsystem; + tgt.opts.max_subsystems = 1024; + tgt.subsystems = calloc(tgt.opts.max_subsystems, sizeof(struct spdk_nvmf_subsystem *)); + SPDK_CU_ASSERT_FATAL(tgt.subsystems != NULL); + snprintf(nqn, sizeof(nqn), "nqn.2016-06.io.spdk:subsystem1"); subsystem = spdk_nvmf_subsystem_create(&tgt, nqn, SPDK_NVMF_SUBTYPE_NVME, 0); SPDK_CU_ASSERT_FATAL(subsystem != NULL);