From 65ff07719d3092fc4a89da972a565ebc5bd10d97 Mon Sep 17 00:00:00 2001 From: Evgeniy Kochetov Date: Mon, 1 Feb 2021 16:13:49 +0200 Subject: [PATCH] nvme/ctrlr: Retrieve active NS list before NS construct This patch changes the order of IDENTIFY_ACTIVE_NS and CONSTRUCT_NS controller states. It is required to further improve memory management for namespaces by allocating memory only for active ones. Signed-off-by: Evgeniy Kochetov Change-Id: Ie540442b1bd9e897afcbaa4319c139109dd0c515 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/6503 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Shuhei Matsumoto Reviewed-by: Jim Harris --- lib/nvme/nvme_ctrlr.c | 25 ++++++++-------- lib/nvme/nvme_internal.h | 10 +++---- .../lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c | 30 ++++++++++++------- 3 files changed, 37 insertions(+), 28 deletions(-) diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index 44c8bebae..044c409f4 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -1161,12 +1161,12 @@ nvme_ctrlr_state_string(enum nvme_ctrlr_state state) return "set number of queues"; case NVME_CTRLR_STATE_WAIT_FOR_SET_NUM_QUEUES: return "wait for set number of queues"; - case NVME_CTRLR_STATE_CONSTRUCT_NS: - return "construct namespaces"; case NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS: return "identify active ns"; case NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY_ACTIVE_NS: return "wait for identify active ns"; + case NVME_CTRLR_STATE_CONSTRUCT_NS: + return "construct namespaces"; case NVME_CTRLR_STATE_IDENTIFY_NS: return "identify ns"; case NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY_NS: @@ -1953,7 +1953,7 @@ _nvme_active_ns_ctx_deleter(struct nvme_active_ns_ctx *ctx) assert(ctx->state == NVME_ACTIVE_NS_STATE_DONE); nvme_ctrlr_identify_active_ns_swap(ctrlr, &ctx->new_ns_list); nvme_active_ns_ctx_destroy(ctx); - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY_NS, ctrlr->opts.admin_timeout_ms); + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_CONSTRUCT_NS, ctrlr->opts.admin_timeout_ms); } static void @@ -2332,7 +2332,7 @@ nvme_ctrlr_set_num_queues_done(void *arg, const struct spdk_nvme_cpl *cpl) spdk_nvme_ctrlr_free_qid(ctrlr, i); } - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_CONSTRUCT_NS, + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS, ctrlr->opts.admin_timeout_ms); } @@ -2524,10 +2524,6 @@ nvme_ctrlr_destruct_namespaces(struct spdk_nvme_ctrlr *ctrlr) ctrlr->ns = NULL; ctrlr->num_ns = 0; } - - spdk_free(ctrlr->active_ns_list); - ctrlr->active_ns_list = NULL; - ctrlr->max_active_ns_idx = 0; } static void @@ -3240,18 +3236,18 @@ nvme_ctrlr_process_init(struct spdk_nvme_ctrlr *ctrlr) rc = nvme_ctrlr_set_num_queues(ctrlr); break; + case NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS: + _nvme_ctrlr_identify_active_ns(ctrlr); + break; + case NVME_CTRLR_STATE_CONSTRUCT_NS: rc = nvme_ctrlr_construct_namespaces(ctrlr); if (!rc) { - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS, + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY_NS, ctrlr->opts.admin_timeout_ms); } break; - case NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS: - _nvme_ctrlr_identify_active_ns(ctrlr); - break; - case NVME_CTRLR_STATE_IDENTIFY_NS: rc = nvme_ctrlr_identify_namespaces(ctrlr); break; @@ -3485,6 +3481,9 @@ nvme_ctrlr_destruct_poll_async(struct spdk_nvme_ctrlr *ctrlr, } nvme_ctrlr_destruct_namespaces(ctrlr); + spdk_free(ctrlr->active_ns_list); + ctrlr->active_ns_list = NULL; + ctrlr->max_active_ns_idx = 0; spdk_bit_array_free(&ctrlr->free_io_qids); diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index 21397623a..495ff9424 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -587,11 +587,6 @@ enum nvme_ctrlr_state { */ NVME_CTRLR_STATE_WAIT_FOR_SET_NUM_QUEUES, - /** - * Construct Namespace data structures of the controller. - */ - NVME_CTRLR_STATE_CONSTRUCT_NS, - /** * Get active Namespace list of the controller. */ @@ -602,6 +597,11 @@ enum nvme_ctrlr_state { */ NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY_ACTIVE_NS, + /** + * Construct Namespace data structures of the controller. + */ + NVME_CTRLR_STATE_CONSTRUCT_NS, + /** * Get Identify Namespace Data structure for each NS. */ diff --git a/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c b/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c index 8378b2f2c..9883e0855 100644 --- a/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c +++ b/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c @@ -2083,6 +2083,8 @@ test_nvme_ctrlr_init_set_nvmf_ioccsz(void) CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_NUM_QUEUES); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_CONSTRUCT_NS); CU_ASSERT(ctrlr.ioccsz_bytes == 0); @@ -2100,6 +2102,8 @@ test_nvme_ctrlr_init_set_nvmf_ioccsz(void) CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_NUM_QUEUES); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_CONSTRUCT_NS); CU_ASSERT(ctrlr.ioccsz_bytes == 4096); @@ -2119,6 +2123,8 @@ test_nvme_ctrlr_init_set_nvmf_ioccsz(void) CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_NUM_QUEUES); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_CONSTRUCT_NS); CU_ASSERT(ctrlr.ioccsz_bytes == 4096); @@ -2138,6 +2144,8 @@ test_nvme_ctrlr_init_set_nvmf_ioccsz(void) CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_NUM_QUEUES); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_CONSTRUCT_NS); CU_ASSERT(ctrlr.ioccsz_bytes == 4096); @@ -2157,6 +2165,8 @@ test_nvme_ctrlr_init_set_nvmf_ioccsz(void) CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_NUM_QUEUES); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_CONSTRUCT_NS); CU_ASSERT(ctrlr.ioccsz_bytes == 0); @@ -2181,8 +2191,8 @@ test_nvme_ctrlr_init_set_num_queues(void) ctrlr.opts.num_io_queues = 64; /* Num queues is zero-based. So, use 31 to get 32 queues */ fake_cpl.cdw0 = 31 + (31 << 16); - CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> CONSTRUCT_NS */ - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_CONSTRUCT_NS); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_ACTIVE_NS */ + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS); CU_ASSERT(ctrlr.opts.num_io_queues == 32); fake_cpl.cdw0 = 0; @@ -2503,8 +2513,8 @@ test_nvme_ctrlr_active_ns_list_v0(void) ctrlr.cdata.nn = 1024; ctrlr.state = NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS; - SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_NS */ - SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_NS); + SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> CONSTRUCT_NS */ + SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_CONSTRUCT_NS); CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1)); CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1024)); CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1025)); @@ -2534,8 +2544,8 @@ test_nvme_ctrlr_active_ns_list_v2(void) /* No active namespaces */ memset(active_ns_list, 0, sizeof(active_ns_list)); ctrlr.state = NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS; - SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_NS */ - SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_NS); + SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> CONSTRUCT_NS */ + SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_CONSTRUCT_NS); CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1)); CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1024)); CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1025)); @@ -2553,8 +2563,8 @@ test_nvme_ctrlr_active_ns_list_v2(void) ctrlr.state = NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS; g_active_ns_list = active_ns_list; g_active_ns_list_length = SPDK_COUNTOF(active_ns_list); - SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_NS */ - SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_NS); + SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> CONSTRUCT_NS */ + SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_CONSTRUCT_NS); CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1)); CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1024)); CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1025)); @@ -2572,8 +2582,8 @@ test_nvme_ctrlr_active_ns_list_v2(void) } ctrlr.state = NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS; - SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_NS */ - SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_NS); + SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> CONSTRUCT_NS */ + SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_CONSTRUCT_NS); CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1)); CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1023)); CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1024));