diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index 72b8a9048..f4feac7fe 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 a9ffd7771..f00de1afb 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 8eeb8b3f6..c88cf3198 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);