From 613d441364275d1fedb9a024ffacc853e9792767 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Thu, 7 Jan 2021 01:21:06 +0900 Subject: [PATCH] bdev/nvme: Process only the head of linked list, nvme_ns->bdevs By the last changes, not only standard namespace but also ocssd namespace has only one nvme_bdev, and standard namespace processes only the head of nvme_ns->bdevs. This patch changes the common and standard namespace specific part to process only the head of nvme_ns->bdevs. The following patch will replace the linked list nvme_ns->bdevs by the pointer nvme_ns->bdev. Add a particular error case that nvme_bdev is failed to create even if ctrlr has one namespace. If ctrlr has one namespace but the corresponding bdev is failed to create, nvme_ns->populated should be false and hence nvme_ns->bdevs should not be accessed. However the code had not assumed such case. Signed-off-by: Shuhei Matsumoto Change-Id: I5495882fad8c8a012305177179a46d4373ba75f5 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/5800 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Aleksey Marchuk --- module/bdev/nvme/bdev_nvme.c | 31 ++++++++------- .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 38 ++++++++++++++++++- 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index 7cc910165..96412cdd0 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -1345,9 +1345,10 @@ nvme_ctrlr_depopulate_namespace_done(struct nvme_bdev_ns *nvme_ns) static void nvme_ctrlr_depopulate_standard_namespace(struct nvme_bdev_ns *nvme_ns) { - struct nvme_bdev *bdev, *tmp; + struct nvme_bdev *bdev; - TAILQ_FOREACH_SAFE(bdev, &nvme_ns->bdevs, tailq, tmp) { + bdev = TAILQ_FIRST(&nvme_ns->bdevs); + if (bdev != NULL) { spdk_bdev_unregister(&bdev->disk, NULL, NULL); } @@ -1421,6 +1422,7 @@ nvme_ctrlr_populate_namespaces(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr, ns = spdk_nvme_ctrlr_get_ns(ctrlr, nsid); num_sectors = spdk_nvme_ns_get_num_sectors(ns); bdev = TAILQ_FIRST(&nvme_ns->bdevs); + assert(bdev != NULL); if (bdev->disk.blockcnt != num_sectors) { SPDK_NOTICELOG("NSID %u is resized: bdev name %s, old size %" PRIu64 ", new size %" PRIu64 "\n", nsid, @@ -1812,7 +1814,7 @@ nvme_ctrlr_populate_namespaces_done(struct nvme_async_probe_ctx *ctx) { struct nvme_bdev_ctrlr *nvme_bdev_ctrlr; struct nvme_bdev_ns *nvme_ns; - struct nvme_bdev *nvme_bdev, *tmp; + struct nvme_bdev *nvme_bdev; uint32_t i, nsid; size_t j; @@ -1831,16 +1833,19 @@ nvme_ctrlr_populate_namespaces_done(struct nvme_async_probe_ctx *ctx) continue; } assert(nvme_ns->id == nsid); - TAILQ_FOREACH_SAFE(nvme_bdev, &nvme_ns->bdevs, tailq, tmp) { - if (j < ctx->count) { - ctx->names[j] = nvme_bdev->disk.name; - j++; - } else { - SPDK_ERRLOG("Maximum number of namespaces supported per NVMe controller is %du. Unable to return all names of created bdevs\n", - ctx->count); - populate_namespaces_cb(ctx, 0, -ERANGE); - return; - } + nvme_bdev = TAILQ_FIRST(&nvme_ns->bdevs); + if (nvme_bdev == NULL) { + assert(nvme_ns->type == NVME_BDEV_NS_OCSSD); + continue; + } + if (j < ctx->count) { + ctx->names[j] = nvme_bdev->disk.name; + j++; + } else { + SPDK_ERRLOG("Maximum number of namespaces supported per NVMe controller is %du. Unable to return all names of created bdevs\n", + ctx->count); + populate_namespaces_cb(ctx, 0, -ERANGE); + return; } } 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 fb3de4191..d553a8fd3 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 @@ -209,6 +209,7 @@ static TAILQ_HEAD(, spdk_nvme_ctrlr) g_ut_attached_ctrlrs = TAILQ_HEAD_INITIALIZ g_ut_attached_ctrlrs); static int g_ut_attach_ctrlr_status; static size_t g_ut_attach_bdev_count; +static int g_ut_register_bdev_status; static void ut_init_trid(struct spdk_nvme_transport_id *trid) @@ -744,7 +745,7 @@ spdk_nvme_poll_group_remove(struct spdk_nvme_poll_group *group, int spdk_bdev_register(struct spdk_bdev *bdev) { - return 0; + return g_ut_register_bdev_status; } void @@ -1332,6 +1333,41 @@ test_attach_ctrlr(void) CU_ASSERT(nvme_bdev_ctrlr_get_by_name("nvme0") == NULL); ut_detach_ctrlr(ctrlr); + + /* Ctrlr has one namespace but one nvme_bdev_ctrlr with no namespace is + * created because creating one nvme_bdev failed. + */ + ctrlr = ut_attach_ctrlr(&trid, 1); + SPDK_CU_ASSERT_FATAL(ctrlr != NULL); + + ctrlr->ns[0].is_active = true; + g_ut_register_bdev_status = -EINVAL; + g_ut_attach_bdev_count = 0; + + rc = bdev_nvme_create(&trid, &hostid, "nvme0", attached_names, 32, NULL, 0, + attach_ctrlr_done, NULL, NULL); + CU_ASSERT(rc == 0); + + spdk_delay_us(1000); + poll_threads(); + + nvme_bdev_ctrlr = nvme_bdev_ctrlr_get_by_name("nvme0"); + SPDK_CU_ASSERT_FATAL(nvme_bdev_ctrlr != NULL); + CU_ASSERT(nvme_bdev_ctrlr->ctrlr == ctrlr); + CU_ASSERT(nvme_bdev_ctrlr->num_ns == 1); + + CU_ASSERT(attached_names[0] == NULL); + + rc = bdev_nvme_delete("nvme0"); + CU_ASSERT(rc == 0); + + poll_threads(); + + CU_ASSERT(nvme_bdev_ctrlr_get_by_name("nvme0") == NULL); + + ut_detach_ctrlr(ctrlr); + + g_ut_register_bdev_status = 0; } static void