From 050346e05e9a7a36dd6f7ab15ec3e768baa1b717 Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Wed, 25 Aug 2021 16:18:18 -0700 Subject: [PATCH] bdev/nvme: Add accessors for getting namespaces Try to use these accessors instead of directly using the namespaces array. This will make changing the data structure easier later on. Change-Id: I3367d0e0065894f3aa199ed1698d27976b4cbbb5 Signed-off-by: Ben Walker Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9315 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Aleksey Marchuk Reviewed-by: Jim Harris Reviewed-by: Shuhei Matsumoto --- module/bdev/nvme/bdev_nvme.c | 35 ++++------ module/bdev/nvme/common.c | 47 +++++++++++++ module/bdev/nvme/common.h | 4 ++ module/bdev/nvme/vbdev_opal.c | 2 +- .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 66 +++++++++---------- 5 files changed, 98 insertions(+), 56 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index f4feac7fe3..2fea31b461 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -349,7 +349,7 @@ bdev_nvme_destruct(void *ctx) assert(nvme_ns->id > 0); - if (nvme_ns->ctrlr->namespaces[nvme_ns->id - 1] == NULL) { + if (nvme_ctrlr_get_ns(nvme_ns->ctrlr, nvme_ns->id) == NULL) { pthread_mutex_unlock(&nvme_ns->ctrlr->mutex); nvme_ctrlr_release(nvme_ns->ctrlr); @@ -1776,7 +1776,7 @@ nvme_ctrlr_populate_namespaces(struct nvme_ctrlr *nvme_ctrlr, for (i = 0; i < nvme_ctrlr->num_ns; i++) { uint32_t nsid = i + 1; - nvme_ns = nvme_ctrlr->namespaces[i]; + nvme_ns = nvme_ctrlr_get_ns(nvme_ctrlr, nsid); ns_is_active = spdk_nvme_ctrlr_is_active_ns(ctrlr, nsid); if (nvme_ns != NULL && ns_is_active) { @@ -1848,7 +1848,7 @@ nvme_ctrlr_depopulate_namespaces(struct nvme_ctrlr *nvme_ctrlr) for (i = 0; i < nvme_ctrlr->num_ns; i++) { uint32_t nsid = i + 1; - nvme_ns = nvme_ctrlr->namespaces[nsid - 1]; + nvme_ns = nvme_ctrlr_get_ns(nvme_ctrlr, nsid); if (nvme_ns != NULL) { assert(nvme_ns->id == nsid); nvme_ctrlr_depopulate_namespace(nvme_ctrlr, nvme_ns); @@ -1883,7 +1883,7 @@ nvme_ctrlr_set_ana_states(const struct spdk_nvme_ana_group_descriptor *desc, continue; } - nvme_ns = nvme_ctrlr->namespaces[nsid - 1]; + nvme_ns = nvme_ctrlr_get_ns(nvme_ctrlr, nsid); assert(nvme_ns != NULL); if (nvme_ns == NULL) { @@ -2375,7 +2375,6 @@ nvme_ctrlr_populate_namespaces_done(struct nvme_ctrlr *nvme_ctrlr, { struct nvme_ns *nvme_ns; struct nvme_bdev *nvme_bdev; - uint32_t i, nsid; size_t j; assert(nvme_ctrlr != NULL); @@ -2385,13 +2384,8 @@ nvme_ctrlr_populate_namespaces_done(struct nvme_ctrlr *nvme_ctrlr, * There can be more than one bdev per NVMe controller. */ j = 0; - for (i = 0; i < nvme_ctrlr->num_ns; i++) { - nsid = i + 1; - nvme_ns = nvme_ctrlr->namespaces[nsid - 1]; - if (nvme_ns == NULL) { - continue; - } - assert(nvme_ns->id == nsid); + nvme_ns = nvme_ctrlr_get_first_active_ns(nvme_ctrlr); + while (nvme_ns != NULL) { nvme_bdev = nvme_ns->bdev; if (j < ctx->count) { ctx->names[j] = nvme_bdev->disk.name; @@ -2402,6 +2396,8 @@ nvme_ctrlr_populate_namespaces_done(struct nvme_ctrlr *nvme_ctrlr, populate_namespaces_cb(ctx, 0, -ERANGE); return; } + + nvme_ns = nvme_ctrlr_get_next_active_ns(nvme_ctrlr, nvme_ns); } populate_namespaces_cb(ctx, j, 0); @@ -2443,7 +2439,6 @@ static int bdev_nvme_compare_namespaces(struct nvme_ctrlr *nvme_ctrlr, struct spdk_nvme_ctrlr *new_ctrlr) { - uint32_t i, nsid; struct nvme_ns *nvme_ns; struct spdk_nvme_ns *new_ns; @@ -2451,20 +2446,16 @@ bdev_nvme_compare_namespaces(struct nvme_ctrlr *nvme_ctrlr, return -EINVAL; } - for (i = 0; i < nvme_ctrlr->num_ns; i++) { - nsid = i + 1; - - nvme_ns = nvme_ctrlr->namespaces[i]; - if (nvme_ns == NULL) { - continue; - } - - new_ns = spdk_nvme_ctrlr_get_ns(new_ctrlr, nsid); + nvme_ns = nvme_ctrlr_get_first_active_ns(nvme_ctrlr); + while (nvme_ns != NULL) { + new_ns = spdk_nvme_ctrlr_get_ns(new_ctrlr, nvme_ns->id); assert(new_ns != NULL); if (!bdev_nvme_compare_ns(nvme_ns->ns, new_ns)) { return -EINVAL; } + + nvme_ns = nvme_ctrlr_get_next_active_ns(nvme_ctrlr, nvme_ns); } return 0; diff --git a/module/bdev/nvme/common.c b/module/bdev/nvme/common.c index 29f60fc481..5efd4ae2c8 100644 --- a/module/bdev/nvme/common.c +++ b/module/bdev/nvme/common.c @@ -38,6 +38,53 @@ struct nvme_ctrlrs g_nvme_ctrlrs = TAILQ_HEAD_INITIALIZER(g_nvme_ctrlrs); pthread_mutex_t g_bdev_nvme_mutex = PTHREAD_MUTEX_INITIALIZER; bool g_bdev_nvme_module_finish; +struct nvme_ns * +nvme_ctrlr_get_ns(struct nvme_ctrlr *nvme_ctrlr, uint32_t nsid) +{ + assert(nsid > 0); + assert(nsid <= nvme_ctrlr->num_ns); + if (nsid == 0 || nsid > nvme_ctrlr->num_ns) { + return NULL; + } + + return nvme_ctrlr->namespaces[nsid - 1]; +} + +struct nvme_ns * +nvme_ctrlr_get_first_active_ns(struct nvme_ctrlr *nvme_ctrlr) +{ + uint32_t i; + + for (i = 0; i < nvme_ctrlr->num_ns; i++) { + if (nvme_ctrlr->namespaces[i] != NULL) { + return nvme_ctrlr->namespaces[i]; + } + } + + return NULL; +} + +struct nvme_ns * +nvme_ctrlr_get_next_active_ns(struct nvme_ctrlr *nvme_ctrlr, struct nvme_ns *ns) +{ + uint32_t i; + + if (ns == NULL) { + return NULL; + } + + /* ns->id is a 1's based value and we want to start at the next + * entry in this array, so we start at ns->id and don't subtract to + * convert to 0's based. */ + for (i = ns->id; i < nvme_ctrlr->num_ns; i++) { + if (nvme_ctrlr->namespaces[i] != NULL) { + return nvme_ctrlr->namespaces[i]; + } + } + + return NULL; +} + struct nvme_ctrlr * nvme_ctrlr_get(const struct spdk_nvme_transport_id *trid) { diff --git a/module/bdev/nvme/common.h b/module/bdev/nvme/common.h index f00de1afb7..fb80a452a4 100644 --- a/module/bdev/nvme/common.h +++ b/module/bdev/nvme/common.h @@ -177,4 +177,8 @@ void nvme_ctrlr_delete(struct nvme_ctrlr *nvme_ctrlr); int bdev_nvme_create_bdev_channel_cb(void *io_device, void *ctx_buf); void bdev_nvme_destroy_bdev_channel_cb(void *io_device, void *ctx_buf); +struct nvme_ns *nvme_ctrlr_get_ns(struct nvme_ctrlr *nvme_ctrlr, uint32_t nsid); +struct nvme_ns *nvme_ctrlr_get_first_active_ns(struct nvme_ctrlr *nvme_ctrlr); +struct nvme_ns *nvme_ctrlr_get_next_active_ns(struct nvme_ctrlr *nvme_ctrlr, struct nvme_ns *ns); + #endif /* SPDK_COMMON_BDEV_NVME_H */ diff --git a/module/bdev/nvme/vbdev_opal.c b/module/bdev/nvme/vbdev_opal.c index 5449f891d8..8015c293c7 100644 --- a/module/bdev/nvme/vbdev_opal.c +++ b/module/bdev/nvme/vbdev_opal.c @@ -357,7 +357,7 @@ vbdev_opal_create(const char *nvme_ctrlr_name, uint32_t nsid, uint8_t locking_ra opal_bdev->opal_dev = nvme_ctrlr->opal_dev; assert(nsid <= nvme_ctrlr->num_ns); - nvme_bdev = nvme_ctrlr->namespaces[nsid - 1]->bdev; + nvme_bdev = nvme_ctrlr_get_ns(nvme_ctrlr, nsid)->bdev; assert(nvme_bdev != NULL); base_bdev_name = nvme_bdev->disk.name; 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 c88cf31981..6fbe0d74a6 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 @@ -1516,7 +1516,7 @@ test_pending_reset(void) nvme_ctrlr = nvme_ctrlr_get_by_name("nvme0"); SPDK_CU_ASSERT_FATAL(nvme_ctrlr != NULL); - bdev = nvme_ctrlr->namespaces[0]->bdev; + bdev = nvme_ctrlr_get_ns(nvme_ctrlr, 1)->bdev; SPDK_CU_ASSERT_FATAL(bdev != NULL); ch1 = spdk_get_io_channel(bdev); @@ -1689,7 +1689,7 @@ test_attach_ctrlr(void) CU_ASSERT(attached_names[0] != NULL && strcmp(attached_names[0], "nvme0n1") == 0); attached_names[0] = NULL; - nbdev = nvme_ctrlr->namespaces[0]->bdev; + nbdev = nvme_ctrlr_get_ns(nvme_ctrlr, 1)->bdev; SPDK_CU_ASSERT_FATAL(nbdev != NULL); CU_ASSERT(bdev_nvme_get_ctrlr(&nbdev->disk) == ctrlr); @@ -1834,12 +1834,12 @@ test_aer_cb(void) SPDK_CU_ASSERT_FATAL(nvme_ctrlr != NULL); CU_ASSERT(nvme_ctrlr->num_ns == 4); - 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_get_ns(nvme_ctrlr, 1) == NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 2) != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 3) != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 4) != NULL); - bdev = nvme_ctrlr->namespaces[3]->bdev; + bdev = nvme_ctrlr_get_ns(nvme_ctrlr, 4)->bdev; SPDK_CU_ASSERT_FATAL(bdev != NULL); CU_ASSERT(bdev->disk.blockcnt == 1024); @@ -1856,10 +1856,10 @@ test_aer_cb(void) aer_cb(nvme_ctrlr, &cpl); - 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_get_ns(nvme_ctrlr, 1) != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 2) != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 3) == NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 4) != NULL); CU_ASSERT(bdev->disk.blockcnt == 2048); /* Change ANA state of active namespaces. */ @@ -1876,9 +1876,9 @@ test_aer_cb(void) spdk_delay_us(10000); poll_threads(); - CU_ASSERT(nvme_ctrlr->namespaces[0]->ana_state == SPDK_NVME_ANA_NON_OPTIMIZED_STATE); - CU_ASSERT(nvme_ctrlr->namespaces[1]->ana_state == SPDK_NVME_ANA_INACCESSIBLE_STATE); - CU_ASSERT(nvme_ctrlr->namespaces[3]->ana_state == SPDK_NVME_ANA_CHANGE_STATE); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 1)->ana_state == SPDK_NVME_ANA_NON_OPTIMIZED_STATE); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 2)->ana_state == SPDK_NVME_ANA_INACCESSIBLE_STATE); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 4)->ana_state == SPDK_NVME_ANA_CHANGE_STATE); rc = bdev_nvme_delete("nvme0", NULL); CU_ASSERT(rc == 0); @@ -2027,7 +2027,7 @@ test_submit_nvme_cmd(void) nvme_ctrlr = nvme_ctrlr_get_by_name("nvme0"); SPDK_CU_ASSERT_FATAL(nvme_ctrlr != NULL); - bdev = nvme_ctrlr->namespaces[0]->bdev; + bdev = nvme_ctrlr_get_ns(nvme_ctrlr, 1)->bdev; SPDK_CU_ASSERT_FATAL(bdev != NULL); set_thread(0); @@ -2279,7 +2279,7 @@ test_abort(void) nvme_ctrlr = nvme_ctrlr_get_by_name("nvme0"); SPDK_CU_ASSERT_FATAL(nvme_ctrlr != NULL); - bdev = nvme_ctrlr->namespaces[0]->bdev; + bdev = nvme_ctrlr_get_ns(nvme_ctrlr, 1)->bdev; SPDK_CU_ASSERT_FATAL(bdev != NULL); write_io = ut_alloc_bdev_io(SPDK_BDEV_IO_TYPE_WRITE, bdev, NULL); @@ -2492,13 +2492,13 @@ test_bdev_unregister(void) nvme_ctrlr = nvme_ctrlr_get_by_name("nvme0"); SPDK_CU_ASSERT_FATAL(nvme_ctrlr != NULL); - nvme_ns1 = nvme_ctrlr->namespaces[0]; + nvme_ns1 = nvme_ctrlr_get_ns(nvme_ctrlr, 1); SPDK_CU_ASSERT_FATAL(nvme_ns1 != NULL); bdev1 = nvme_ns1->bdev; SPDK_CU_ASSERT_FATAL(bdev1 != NULL); - nvme_ns2 = nvme_ctrlr->namespaces[1]; + nvme_ns2 = nvme_ctrlr_get_ns(nvme_ctrlr, 2); SPDK_CU_ASSERT_FATAL(nvme_ns2 != NULL); bdev2 = nvme_ns2->bdev; @@ -2611,21 +2611,21 @@ 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] != 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); - CU_ASSERT(nvme_ctrlr->namespaces[3]->ana_state == SPDK_NVME_ANA_PERSISTENT_LOSS_STATE); - CU_ASSERT(nvme_ctrlr->namespaces[4]->ana_state == SPDK_NVME_ANA_CHANGE_STATE); - CU_ASSERT(nvme_ctrlr->namespaces[0]->bdev != NULL); - CU_ASSERT(nvme_ctrlr->namespaces[1]->bdev != NULL); - CU_ASSERT(nvme_ctrlr->namespaces[2]->bdev != NULL); - CU_ASSERT(nvme_ctrlr->namespaces[3]->bdev != NULL); - CU_ASSERT(nvme_ctrlr->namespaces[4]->bdev != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 1) != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 2) != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 3) != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 4) != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 5) != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 1)->ana_state == SPDK_NVME_ANA_OPTIMIZED_STATE); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 2)->ana_state == SPDK_NVME_ANA_NON_OPTIMIZED_STATE); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 3)->ana_state == SPDK_NVME_ANA_INACCESSIBLE_STATE); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 4)->ana_state == SPDK_NVME_ANA_PERSISTENT_LOSS_STATE); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 5)->ana_state == SPDK_NVME_ANA_CHANGE_STATE); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 1)->bdev != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 2)->bdev != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 3)->bdev != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 4)->bdev != NULL); + CU_ASSERT(nvme_ctrlr_get_ns(nvme_ctrlr, 5)->bdev != NULL); rc = bdev_nvme_delete("nvme0", NULL); CU_ASSERT(rc == 0);