From 282b8b70a7e61dd67150fdb49b10a53ded6080a9 Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Wed, 25 Aug 2021 15:03:09 -0700 Subject: [PATCH] bdev/nvme: Don't allocate inactive namespaces If the number of namespaces is very large, this can cause excessive memory allocation. This is especially true because when the number of namespaces is large, it is almost always very sparsely populated. Signed-off-by: Ben Walker Change-Id: I27d94956c222ae3c49c6a7422164ae3a8ec8d963 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9302 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Aleksey Marchuk --- module/bdev/nvme/bdev_nvme.c | 53 ++++++++++--------- module/bdev/nvme/common.h | 7 --- .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 30 ++++++----- 3 files changed, 46 insertions(+), 44 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index 72b8a90483..f4feac7fe3 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -347,10 +347,13 @@ bdev_nvme_destruct(void *ctx) nvme_ns->bdev = NULL; - if (!nvme_ns->populated) { + assert(nvme_ns->id > 0); + + if (nvme_ns->ctrlr->namespaces[nvme_ns->id - 1] == NULL) { pthread_mutex_unlock(&nvme_ns->ctrlr->mutex); nvme_ctrlr_release(nvme_ns->ctrlr); + free(nvme_ns); } else { pthread_mutex_unlock(&nvme_ns->ctrlr->mutex); } @@ -1699,7 +1702,6 @@ nvme_ctrlr_populate_namespace(struct nvme_ctrlr *nvme_ctrlr, struct nvme_ns *nvm } nvme_ns->ns = ns; - nvme_ns->populated = true; nvme_ns->ana_state = SPDK_NVME_ANA_OPTIMIZED_STATE; if (nvme_ctrlr->ana_log_page != NULL) { @@ -1714,7 +1716,8 @@ done: nvme_ctrlr->ref++; pthread_mutex_unlock(&nvme_ctrlr->mutex); } else { - memset(nvme_ns, 0, sizeof(*nvme_ns)); + nvme_ctrlr->namespaces[nvme_ns->id - 1] = NULL; + free(nvme_ns); } if (ctx) { @@ -1737,12 +1740,14 @@ nvme_ctrlr_depopulate_namespace(struct nvme_ctrlr *nvme_ctrlr, struct nvme_ns *n pthread_mutex_lock(&nvme_ctrlr->mutex); - nvme_ns->populated = false; + nvme_ctrlr->namespaces[nvme_ns->id - 1] = NULL; if (nvme_ns->bdev != NULL) { pthread_mutex_unlock(&nvme_ctrlr->mutex); return; } + + free(nvme_ns); pthread_mutex_unlock(&nvme_ctrlr->mutex); nvme_ctrlr_release(nvme_ctrlr); @@ -1774,7 +1779,7 @@ nvme_ctrlr_populate_namespaces(struct nvme_ctrlr *nvme_ctrlr, nvme_ns = nvme_ctrlr->namespaces[i]; ns_is_active = spdk_nvme_ctrlr_is_active_ns(ctrlr, nsid); - if (nvme_ns->populated && ns_is_active) { + if (nvme_ns != NULL && ns_is_active) { /* NS is still there but attributes may have changed */ ns = spdk_nvme_ctrlr_get_ns(ctrlr, nsid); num_sectors = spdk_nvme_ns_get_num_sectors(ns); @@ -1794,7 +1799,16 @@ nvme_ctrlr_populate_namespaces(struct nvme_ctrlr *nvme_ctrlr, } } - if (!nvme_ns->populated && ns_is_active) { + if (nvme_ns == NULL && ns_is_active) { + nvme_ns = calloc(1, sizeof(struct nvme_ns)); + if (nvme_ns == NULL) { + SPDK_ERRLOG("Failed to allocate namespace\n"); + /* This just fails to attach the namespace. It may work on a future attempt. */ + continue; + } + + nvme_ctrlr->namespaces[nsid - 1] = nvme_ns; + nvme_ns->id = nsid; nvme_ns->ctrlr = nvme_ctrlr; @@ -1806,7 +1820,7 @@ nvme_ctrlr_populate_namespaces(struct nvme_ctrlr *nvme_ctrlr, nvme_ctrlr_populate_namespace(nvme_ctrlr, nvme_ns, ctx); } - if (nvme_ns->populated && !ns_is_active) { + if (nvme_ns != NULL && !ns_is_active) { nvme_ctrlr_depopulate_namespace(nvme_ctrlr, nvme_ns); } } @@ -1835,7 +1849,7 @@ nvme_ctrlr_depopulate_namespaces(struct nvme_ctrlr *nvme_ctrlr) uint32_t nsid = i + 1; nvme_ns = nvme_ctrlr->namespaces[nsid - 1]; - if (nvme_ns->populated) { + if (nvme_ns != NULL) { assert(nvme_ns->id == nsid); nvme_ctrlr_depopulate_namespace(nvme_ctrlr, nvme_ns); } @@ -1870,9 +1884,10 @@ nvme_ctrlr_set_ana_states(const struct spdk_nvme_ana_group_descriptor *desc, } nvme_ns = nvme_ctrlr->namespaces[nsid - 1]; - assert(nvme_ns != NULL); - if (!nvme_ns->populated) { + assert(nvme_ns != NULL); + if (nvme_ns == NULL) { + /* Target told us that an inactive namespace had an ANA change */ continue; } @@ -2047,7 +2062,7 @@ nvme_ctrlr_create(struct spdk_nvme_ctrlr *ctrlr, { struct nvme_ctrlr *nvme_ctrlr; struct nvme_ctrlr_trid *trid_entry; - uint32_t i, num_ns; + uint32_t num_ns; const struct spdk_nvme_ctrlr_data *cdata; int rc; @@ -2074,17 +2089,7 @@ nvme_ctrlr_create(struct spdk_nvme_ctrlr *ctrlr, goto err; } - for (i = 0; i < num_ns; i++) { - nvme_ctrlr->namespaces[i] = calloc(1, sizeof(struct nvme_ns)); - if (nvme_ctrlr->namespaces[i] == NULL) { - SPDK_ERRLOG("Failed to allocate block namespace struct\n"); - rc = -ENOMEM; - goto err; - } - nvme_ctrlr->num_ns++; - } - - assert(num_ns == nvme_ctrlr->num_ns); + nvme_ctrlr->num_ns = num_ns; } trid_entry = calloc(1, sizeof(*trid_entry)); @@ -2383,7 +2388,7 @@ nvme_ctrlr_populate_namespaces_done(struct nvme_ctrlr *nvme_ctrlr, for (i = 0; i < nvme_ctrlr->num_ns; i++) { nsid = i + 1; nvme_ns = nvme_ctrlr->namespaces[nsid - 1]; - if (!nvme_ns->populated) { + if (nvme_ns == NULL) { continue; } assert(nvme_ns->id == nsid); @@ -2450,7 +2455,7 @@ bdev_nvme_compare_namespaces(struct nvme_ctrlr *nvme_ctrlr, nsid = i + 1; nvme_ns = nvme_ctrlr->namespaces[i]; - if (!nvme_ns->populated) { + if (nvme_ns == NULL) { continue; } diff --git a/module/bdev/nvme/common.h b/module/bdev/nvme/common.h index a9ffd7771e..f00de1afb7 100644 --- a/module/bdev/nvme/common.h +++ b/module/bdev/nvme/common.h @@ -67,13 +67,6 @@ struct nvme_async_probe_ctx { struct nvme_ns { uint32_t id; - /** Marks whether this data structure has its bdevs - * populated for the associated namespace. It is used - * to keep track if we need manage the populated - * resources when a newly active namespace is found, - * or when a namespace becomes inactive. - */ - bool populated; struct spdk_nvme_ns *ns; struct nvme_ctrlr *ctrlr; struct nvme_bdev *bdev; diff --git a/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c b/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c index 8eeb8b3f60..c88cf31981 100644 --- a/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c +++ b/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c @@ -727,6 +727,10 @@ ut_create_ana_log_page(struct spdk_nvme_ctrlr *ctrlr, char *buf, uint32_t length for (i = 0; i < ctrlr->num_ns; i++) { ns = &ctrlr->ns[i]; + if (!ns->is_active) { + continue; + } + memset(ana_desc, 0, UT_ANA_DESC_SIZE); ana_desc->ana_group_id = ns->id; @@ -1830,10 +1834,10 @@ test_aer_cb(void) SPDK_CU_ASSERT_FATAL(nvme_ctrlr != NULL); CU_ASSERT(nvme_ctrlr->num_ns == 4); - CU_ASSERT(nvme_ctrlr->namespaces[0]->populated == false); - CU_ASSERT(nvme_ctrlr->namespaces[1]->populated == true); - CU_ASSERT(nvme_ctrlr->namespaces[2]->populated == true); - CU_ASSERT(nvme_ctrlr->namespaces[3]->populated == true); + CU_ASSERT(nvme_ctrlr->namespaces[0] == NULL); + CU_ASSERT(nvme_ctrlr->namespaces[1] != NULL); + CU_ASSERT(nvme_ctrlr->namespaces[2] != NULL); + CU_ASSERT(nvme_ctrlr->namespaces[3] != NULL); bdev = nvme_ctrlr->namespaces[3]->bdev; SPDK_CU_ASSERT_FATAL(bdev != NULL); @@ -1852,10 +1856,10 @@ test_aer_cb(void) aer_cb(nvme_ctrlr, &cpl); - CU_ASSERT(nvme_ctrlr->namespaces[0]->populated == true); - CU_ASSERT(nvme_ctrlr->namespaces[1]->populated == true); - CU_ASSERT(nvme_ctrlr->namespaces[2]->populated == false); - CU_ASSERT(nvme_ctrlr->namespaces[3]->populated == true); + CU_ASSERT(nvme_ctrlr->namespaces[0] != NULL); + CU_ASSERT(nvme_ctrlr->namespaces[1] != NULL); + CU_ASSERT(nvme_ctrlr->namespaces[2] == NULL); + CU_ASSERT(nvme_ctrlr->namespaces[3] != NULL); CU_ASSERT(bdev->disk.blockcnt == 2048); /* Change ANA state of active namespaces. */ @@ -2607,11 +2611,11 @@ test_init_ana_log_page(void) SPDK_CU_ASSERT_FATAL(nvme_ctrlr != NULL); CU_ASSERT(nvme_ctrlr->num_ns == 5); - CU_ASSERT(nvme_ctrlr->namespaces[0]->populated == true); - CU_ASSERT(nvme_ctrlr->namespaces[1]->populated == true); - CU_ASSERT(nvme_ctrlr->namespaces[2]->populated == true); - CU_ASSERT(nvme_ctrlr->namespaces[3]->populated == true); - CU_ASSERT(nvme_ctrlr->namespaces[4]->populated == true); + CU_ASSERT(nvme_ctrlr->namespaces[0] != NULL); + CU_ASSERT(nvme_ctrlr->namespaces[1] != NULL); + CU_ASSERT(nvme_ctrlr->namespaces[2] != NULL); + CU_ASSERT(nvme_ctrlr->namespaces[3] != NULL); + CU_ASSERT(nvme_ctrlr->namespaces[4] != NULL); CU_ASSERT(nvme_ctrlr->namespaces[0]->ana_state == SPDK_NVME_ANA_OPTIMIZED_STATE); CU_ASSERT(nvme_ctrlr->namespaces[1]->ana_state == SPDK_NVME_ANA_NON_OPTIMIZED_STATE); CU_ASSERT(nvme_ctrlr->namespaces[2]->ana_state == SPDK_NVME_ANA_INACCESSIBLE_STATE);