From 940765a9036a7eddc1dab2c08c57f5fd08fd562d Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Thu, 19 Aug 2021 14:01:09 -0700 Subject: [PATCH] nvme: No longer allocate arrays of size NN in cuse handling If NN is very, very large, this allocates too much memory. For now, just use a list. Change-Id: I904977673d8fb6c86f03c94ba798c6cc07f4a4d8 Signed-off-by: Ben Walker Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9301 Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot Community-CI: Broadcom CI Reviewed-by: Jim Harris Reviewed-by: Shuhei Matsumoto --- lib/nvme/nvme_cuse.c | 81 ++++++++++--------- test/nvme/cuse/cuse.c | 18 +++++ test/unit/lib/nvme/nvme_cuse.c/nvme_cuse_ut.c | 38 ++++++--- 3 files changed, 87 insertions(+), 50 deletions(-) diff --git a/lib/nvme/nvme_cuse.c b/lib/nvme/nvme_cuse.c index d2effe8bd..0cd845e1d 100644 --- a/lib/nvme/nvme_cuse.c +++ b/lib/nvme/nvme_cuse.c @@ -43,8 +43,6 @@ #include "nvme_cuse.h" struct cuse_device { - bool is_started; - char dev_name[128]; uint32_t index; int claim_fd; @@ -57,7 +55,7 @@ struct cuse_device { struct fuse_session *session; struct cuse_device *ctrlr_device; - struct cuse_device *ns_devices; /**< Array of cuse ns devices */ + TAILQ_HEAD(, cuse_device) ns_devices; TAILQ_ENTRY(cuse_device) tailq; }; @@ -796,6 +794,9 @@ cuse_thread(void *arg) pthread_exit(NULL); } +static struct cuse_device *nvme_cuse_get_cuse_ns_device(struct spdk_nvme_ctrlr *ctrlr, + uint32_t nsid); + /***************************************************************************** * CUSE devices management */ @@ -806,11 +807,16 @@ cuse_nvme_ns_start(struct cuse_device *ctrlr_device, uint32_t nsid) struct cuse_device *ns_device; int rv; - ns_device = &ctrlr_device->ns_devices[nsid - 1]; - if (ns_device->is_started) { + ns_device = nvme_cuse_get_cuse_ns_device(ctrlr_device->ctrlr, nsid); + if (ns_device != NULL) { return 0; } + ns_device = calloc(1, sizeof(struct cuse_device)); + if (ns_device == NULL) { + return -ENOMEM; + } + ns_device->ctrlr = ctrlr_device->ctrlr; ns_device->ctrlr_device = ctrlr_device; ns_device->nsid = nsid; @@ -823,31 +829,27 @@ cuse_nvme_ns_start(struct cuse_device *ctrlr_device, uint32_t nsid) } rv = cuse_session_create(ns_device); if (rv != 0) { + free(ns_device); return rv; } rv = pthread_create(&ns_device->tid, NULL, cuse_thread, ns_device); if (rv != 0) { SPDK_ERRLOG("pthread_create failed\n"); + free(ns_device); return -rv; } - ns_device->is_started = true; + TAILQ_INSERT_TAIL(&ctrlr_device->ns_devices, ns_device, tailq); return 0; } static void -cuse_nvme_ns_stop(struct cuse_device *ctrlr_device, uint32_t nsid) +cuse_nvme_ns_stop(struct cuse_device *ctrlr_device, struct cuse_device *ns_device) { - struct cuse_device *ns_device; - - ns_device = &ctrlr_device->ns_devices[nsid - 1]; - if (!ns_device->is_started) { - return; - } - fuse_session_exit(ns_device->session); pthread_join(ns_device->tid, NULL); - ns_device->is_started = false; + TAILQ_REMOVE(&ctrlr_device->ns_devices, ns_device, tailq); + free(ns_device); } static int @@ -915,45 +917,47 @@ nvme_cuse_unclaim(struct cuse_device *ctrlr_device) static void cuse_nvme_ctrlr_stop(struct cuse_device *ctrlr_device) { - uint32_t i; - uint32_t num_ns = spdk_nvme_ctrlr_get_num_ns(ctrlr_device->ctrlr); + struct cuse_device *ns_device, *tmp; - for (i = 1; i <= num_ns; i++) { - cuse_nvme_ns_stop(ctrlr_device, i); + TAILQ_FOREACH_SAFE(ns_device, &ctrlr_device->ns_devices, tailq, tmp) { + cuse_nvme_ns_stop(ctrlr_device, ns_device); } - if (!ctrlr_device->is_started) { - return; - } + assert(TAILQ_EMPTY(&ctrlr_device->ns_devices)); + fuse_session_exit(ctrlr_device->session); pthread_join(ctrlr_device->tid, NULL); - ctrlr_device->is_started = false; TAILQ_REMOVE(&g_ctrlr_ctx_head, ctrlr_device, tailq); spdk_bit_array_clear(g_ctrlr_started, ctrlr_device->index); if (spdk_bit_array_count_set(g_ctrlr_started) == 0) { spdk_bit_array_free(&g_ctrlr_started); } nvme_cuse_unclaim(ctrlr_device); - free(ctrlr_device->ns_devices); free(ctrlr_device); } static int cuse_nvme_ctrlr_update_namespaces(struct cuse_device *ctrlr_device) { + struct cuse_device *ns_device, *tmp; uint32_t nsid; - uint32_t num_ns = spdk_nvme_ctrlr_get_num_ns(ctrlr_device->ctrlr); - for (nsid = 1; nsid <= num_ns; nsid++) { - if (!spdk_nvme_ctrlr_is_active_ns(ctrlr_device->ctrlr, nsid)) { - cuse_nvme_ns_stop(ctrlr_device, nsid); - continue; + /* Remove namespaces that have disappeared */ + TAILQ_FOREACH_SAFE(ns_device, &ctrlr_device->ns_devices, tailq, tmp) { + if (!spdk_nvme_ctrlr_is_active_ns(ctrlr_device->ctrlr, ns_device->nsid)) { + cuse_nvme_ns_stop(ctrlr_device, ns_device); } + } + /* Add new namespaces */ + nsid = spdk_nvme_ctrlr_get_first_active_ns(ctrlr_device->ctrlr); + while (nsid != 0) { if (cuse_nvme_ns_start(ctrlr_device, nsid) < 0) { SPDK_ERRLOG("Cannot start CUSE namespace device."); return -1; } + + nsid = spdk_nvme_ctrlr_get_next_active_ns(ctrlr_device->ctrlr, nsid); } return 0; @@ -964,7 +968,6 @@ nvme_cuse_start(struct spdk_nvme_ctrlr *ctrlr) { int rv = 0; struct cuse_device *ctrlr_device; - uint32_t num_ns = spdk_nvme_ctrlr_get_num_ns(ctrlr); SPDK_NOTICELOG("Creating cuse device for controller\n"); @@ -1014,11 +1017,11 @@ nvme_cuse_start(struct spdk_nvme_ctrlr *ctrlr) rv = -rv; goto clear_and_free; } - ctrlr_device->is_started = true; TAILQ_INSERT_TAIL(&g_ctrlr_ctx_head, ctrlr_device, tailq); - ctrlr_device->ns_devices = (struct cuse_device *)calloc(num_ns, sizeof(struct cuse_device)); + TAILQ_INIT(&ctrlr_device->ns_devices); + /* Start all active namespaces */ if (cuse_nvme_ctrlr_update_namespaces(ctrlr_device) < 0) { SPDK_ERRLOG("Cannot start CUSE namespace devices."); @@ -1057,22 +1060,20 @@ static struct cuse_device * nvme_cuse_get_cuse_ns_device(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid) { struct cuse_device *ctrlr_device = NULL; - uint32_t num_ns = spdk_nvme_ctrlr_get_num_ns(ctrlr); - - if (nsid < 1 || nsid > num_ns) { - return NULL; - } + struct cuse_device *ns_device; ctrlr_device = nvme_cuse_get_cuse_ctrlr_device(ctrlr); if (!ctrlr_device) { return NULL; } - if (!ctrlr_device->ns_devices[nsid - 1].is_started) { - return NULL; + TAILQ_FOREACH(ns_device, &ctrlr_device->ns_devices, tailq) { + if (ns_device->nsid == nsid) { + return ns_device; + } } - return &ctrlr_device->ns_devices[nsid - 1]; + return NULL; } static void diff --git a/test/nvme/cuse/cuse.c b/test/nvme/cuse/cuse.c index 9999c51c2..d677b21df 100644 --- a/test/nvme/cuse/cuse.c +++ b/test/nvme/cuse/cuse.c @@ -62,6 +62,24 @@ spdk_nvme_ctrlr_is_active_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid) return nsid >= g_active_nsid_min && nsid < g_active_num_ns + g_active_nsid_min; } +uint32_t +spdk_nvme_ctrlr_get_first_active_ns(struct spdk_nvme_ctrlr *ctrlr) +{ + return g_active_nsid_min; +} + +uint32_t +spdk_nvme_ctrlr_get_next_active_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid) +{ + nsid = nsid + 1; + + if (spdk_nvme_ctrlr_is_active_ns(ctrlr, nsid)) { + return nsid; + } + + return 0; +} + DEFINE_STUB(spdk_nvme_ctrlr_reset, int, (struct spdk_nvme_ctrlr *ctrlr), 0); DEFINE_STUB(spdk_nvme_ctrlr_reset_subsystem, int, (struct spdk_nvme_ctrlr *ctrlr), 0); diff --git a/test/unit/lib/nvme/nvme_cuse.c/nvme_cuse_ut.c b/test/unit/lib/nvme/nvme_cuse.c/nvme_cuse_ut.c index f11e44e82..2468ed2c2 100644 --- a/test/unit/lib/nvme/nvme_cuse.c/nvme_cuse_ut.c +++ b/test/unit/lib/nvme/nvme_cuse.c/nvme_cuse_ut.c @@ -95,6 +95,22 @@ spdk_nvme_ctrlr_get_num_ns(struct spdk_nvme_ctrlr *ctrlr) return ctrlr->num_ns; } +uint32_t +spdk_nvme_ctrlr_get_first_active_ns(struct spdk_nvme_ctrlr *ctrlr) +{ + return 1; +} + +uint32_t +spdk_nvme_ctrlr_get_next_active_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid) +{ + if (nsid > ctrlr->num_ns) { + return 0; + } + + return nsid + 1; +} + DEFINE_RETURN_MOCK(nvme_io_msg_send, int); int nvme_io_msg_send(struct spdk_nvme_ctrlr *ctrlr, @@ -233,20 +249,19 @@ test_nvme_cuse_get_cuse_ns_device(void) { struct spdk_nvme_ctrlr ctrlr = {}; struct cuse_device ctrlr_device = {}; - struct cuse_device ns_devices[3] = {}; + struct cuse_device ns_device = { .nsid = 1 }; struct cuse_device *cuse_dev = NULL; ctrlr.num_ns = 3; ctrlr_device.ctrlr = &ctrlr; - ctrlr_device.ns_devices = ns_devices; - ns_devices[0].is_started = true; - ns_devices[1].is_started = false; + TAILQ_INIT(&ctrlr_device.ns_devices); + TAILQ_INSERT_TAIL(&ctrlr_device.ns_devices, &ns_device, tailq); SPDK_CU_ASSERT_FATAL(TAILQ_EMPTY(&g_ctrlr_ctx_head)); TAILQ_INSERT_TAIL(&g_ctrlr_ctx_head, &ctrlr_device, tailq); cuse_dev = nvme_cuse_get_cuse_ns_device(&ctrlr, 1); - CU_ASSERT(cuse_dev == &ns_devices[0]); + CU_ASSERT(cuse_dev == &ns_device); /* nsid 2 was not started */ cuse_dev = nvme_cuse_get_cuse_ns_device(&ctrlr, 2); @@ -353,20 +368,23 @@ test_nvme_cuse_stop(void) { struct spdk_nvme_ctrlr ctrlr = {}; struct cuse_device *ctrlr_device = NULL; + struct cuse_device *ns_dev1, *ns_dev2; /* Allocate memory for nvme_cuse_stop() to free. */ ctrlr_device = calloc(1, sizeof(struct cuse_device)); SPDK_CU_ASSERT_FATAL(ctrlr_device != NULL); - ctrlr_device->ns_devices = calloc(2, sizeof(struct cuse_device)); - SPDK_CU_ASSERT_FATAL(ctrlr_device->ns_devices != NULL); + TAILQ_INIT(&ctrlr_device->ns_devices); + ns_dev1 = calloc(1, sizeof(struct cuse_device)); + SPDK_CU_ASSERT_FATAL(ns_dev1 != NULL); + ns_dev2 = calloc(1, sizeof(struct cuse_device)); + SPDK_CU_ASSERT_FATAL(ns_dev2 != NULL); g_ctrlr_started = spdk_bit_array_create(128); SPDK_CU_ASSERT_FATAL(g_ctrlr_started != NULL); - ctrlr_device->is_started = true; - ctrlr_device->ns_devices[0].is_started = true; - ctrlr_device->ns_devices[1].is_started = true; + TAILQ_INSERT_TAIL(&ctrlr_device->ns_devices, ns_dev1, tailq); + TAILQ_INSERT_TAIL(&ctrlr_device->ns_devices, ns_dev2, tailq); ctrlr.num_ns = 2; ctrlr_device->ctrlr = &ctrlr; pthread_mutex_init(&g_cuse_mtx, NULL);