From 55e0ec894f6d5a7c70a2df3f15d8438d5101ea6d Mon Sep 17 00:00:00 2001 From: Jacek Kalwas Date: Thu, 26 Mar 2020 21:50:30 +0100 Subject: [PATCH] nvme: fix identify active ns NVMe ctrlr init state machine shall be async whenever possible so it is not blocking other code from processing. It can result in deadlock when cmd producer and consumer are sharing the same thread. This patch is making identify active ns async by introducing new state to wait for completions. Signed-off-by: Jacek Kalwas Change-Id: I346d35bab4733d3941e023602854fdd5b1ef23b5 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/1463 Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Shuhei Matsumoto Reviewed-by: Aleksey Marchuk Community-CI: Broadcom CI --- lib/nvme/nvme_ctrlr.c | 78 ++++++++++++++++--- lib/nvme/nvme_internal.h | 5 ++ .../lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c | 4 +- 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index b4465ad00..9dd453037 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -41,6 +41,7 @@ struct nvme_active_ns_ctx; +static void nvme_ctrlr_destruct_namespaces(struct spdk_nvme_ctrlr *ctrlr); static int nvme_ctrlr_construct_and_submit_aer(struct spdk_nvme_ctrlr *ctrlr, struct nvme_async_event_request *aer); static void nvme_ctrlr_identify_active_ns_async(struct nvme_active_ns_ctx *ctx); @@ -958,6 +959,8 @@ nvme_ctrlr_state_string(enum nvme_ctrlr_state state) 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_IDENTIFY_NS: return "identify ns"; case NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY_NS: @@ -1344,18 +1347,21 @@ enum nvme_active_ns_state { NVME_ACTIVE_NS_STATE_ERROR }; +typedef void (*nvme_active_ns_ctx_deleter)(struct nvme_active_ns_ctx *); + struct nvme_active_ns_ctx { struct spdk_nvme_ctrlr *ctrlr; uint32_t page; uint32_t num_pages; uint32_t next_nsid; uint32_t *new_ns_list; + nvme_active_ns_ctx_deleter deleter; enum nvme_active_ns_state state; }; static struct nvme_active_ns_ctx * -nvme_active_ns_ctx_create(struct spdk_nvme_ctrlr *ctrlr) +nvme_active_ns_ctx_create(struct spdk_nvme_ctrlr *ctrlr, nvme_active_ns_ctx_deleter deleter) { struct nvme_active_ns_ctx *ctx; uint32_t num_pages = 0; @@ -1382,6 +1388,7 @@ nvme_active_ns_ctx_create(struct spdk_nvme_ctrlr *ctrlr) ctx->num_pages = num_pages; ctx->new_ns_list = new_ns_list; ctx->ctrlr = ctrlr; + ctx->deleter = deleter; return ctx; } @@ -1408,16 +1415,22 @@ nvme_ctrlr_identify_active_ns_async_done(void *arg, const struct spdk_nvme_cpl * if (spdk_nvme_cpl_is_error(cpl)) { ctx->state = NVME_ACTIVE_NS_STATE_ERROR; - return; + goto out; } ctx->next_nsid = ctx->new_ns_list[1024 * ctx->page + 1023]; if (ctx->next_nsid == 0 || ++ctx->page == ctx->num_pages) { ctx->state = NVME_ACTIVE_NS_STATE_DONE; - return; + goto out; } nvme_ctrlr_identify_active_ns_async(ctx); + return; + +out: + if (ctx->deleter) { + ctx->deleter(ctx); + } } static void @@ -1429,7 +1442,7 @@ nvme_ctrlr_identify_active_ns_async(struct nvme_active_ns_ctx *ctx) if (ctrlr->num_ns == 0) { ctx->state = NVME_ACTIVE_NS_STATE_DONE; - return; + goto out; } /* @@ -1442,7 +1455,7 @@ nvme_ctrlr_identify_active_ns_async(struct nvme_active_ns_ctx *ctx) } ctx->state = NVME_ACTIVE_NS_STATE_DONE; - return; + goto out; } ctx->state = NVME_ACTIVE_NS_STATE_PROCESSING; @@ -1451,7 +1464,49 @@ nvme_ctrlr_identify_active_ns_async(struct nvme_active_ns_ctx *ctx) nvme_ctrlr_identify_active_ns_async_done, ctx); if (rc != 0) { ctx->state = NVME_ACTIVE_NS_STATE_ERROR; + goto out; } + + return; + +out: + if (ctx->deleter) { + ctx->deleter(ctx); + } +} + +static void +_nvme_active_ns_ctx_deleter(struct nvme_active_ns_ctx *ctx) +{ + struct spdk_nvme_ctrlr *ctrlr = ctx->ctrlr; + + if (ctx->state == NVME_ACTIVE_NS_STATE_ERROR) { + nvme_ctrlr_destruct_namespaces(ctrlr); + nvme_active_ns_ctx_destroy(ctx); + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_ERROR, NVME_TIMEOUT_INFINITE); + return; + } + + 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); +} + +static void +_nvme_ctrlr_identify_active_ns(struct spdk_nvme_ctrlr *ctrlr) +{ + struct nvme_active_ns_ctx *ctx; + + ctx = nvme_active_ns_ctx_create(ctrlr, _nvme_active_ns_ctx_deleter); + if (!ctx) { + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_ERROR, NVME_TIMEOUT_INFINITE); + return; + } + + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY_ACTIVE_NS, + ctrlr->opts.admin_timeout_ms); + nvme_ctrlr_identify_active_ns_async(ctx); } int @@ -1460,7 +1515,7 @@ nvme_ctrlr_identify_active_ns(struct spdk_nvme_ctrlr *ctrlr) struct nvme_active_ns_ctx *ctx; int rc; - ctx = nvme_active_ns_ctx_create(ctrlr); + ctx = nvme_active_ns_ctx_create(ctrlr, NULL); if (!ctx) { return -ENOMEM; } @@ -2542,12 +2597,11 @@ nvme_ctrlr_process_init(struct spdk_nvme_ctrlr *ctrlr) break; case NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS: - rc = nvme_ctrlr_identify_active_ns(ctrlr); - if (rc < 0) { - nvme_ctrlr_destruct_namespaces(ctrlr); - } - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY_NS, - ctrlr->opts.admin_timeout_ms); + _nvme_ctrlr_identify_active_ns(ctrlr); + break; + + case NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY_ACTIVE_NS: + spdk_nvme_qpair_process_completions(ctrlr->adminq, 0); break; case NVME_CTRLR_STATE_IDENTIFY_NS: diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index ef5e93ee1..e31ff4c0a 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -524,6 +524,11 @@ enum nvme_ctrlr_state { */ NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS, + /** + * Waiting for the Identify Active Namespace commands to be completed. + */ + NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY_ACTIVE_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 f687e5d5b..b11cf31d8 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 @@ -1790,7 +1790,7 @@ test_nvme_ctrlr_test_active_ns(void) { uint32_t nsid, minor; size_t ns_id_count; - struct spdk_nvme_ctrlr ctrlr = {}; + struct spdk_nvme_ctrlr ctrlr = {.state = NVME_CTRLR_STATE_READY}; ctrlr.page_size = 0x1000; @@ -1850,7 +1850,7 @@ static void test_nvme_ctrlr_test_active_ns_error_case(void) { int rc; - struct spdk_nvme_ctrlr ctrlr = {}; + struct spdk_nvme_ctrlr ctrlr = {.state = NVME_CTRLR_STATE_READY}; ctrlr.page_size = 0x1000; ctrlr.vs.bits.mjr = 1;