From 1c083e6200760905d052469a8e92c053a6096698 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Fri, 19 Nov 2021 09:35:56 +0000 Subject: [PATCH] nvme: set keep alive for discovery controllers Discovery services using the SPDK nvme driver may use long-lasting connections that detect AER completions to determine when there are changes in the discovery log. This means that we still need to send keep alives on discovery controller admin queues. So move the SET_KEEP_ALIVE_TIMEOUT state immediately after IDENTIFY, and run the SET_KEEP_ALIVE_TIMEOUT state even for discovery controllers. Note, we need the IDENTIFY's KAS value to properly set the keep alive timeout, so we have to keep the IDENTIFY state before SET_KEEP_ALIVE_TIMEOUT. Signed-off-by: Jim Harris Change-Id: I5c6403c28fb72d42629c5f9009a89c4bfd44d162 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/10329 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Aleksey Marchuk --- lib/nvme/nvme_ctrlr.c | 53 ++++++++++--------- lib/nvme/nvme_internal.h | 20 +++---- .../lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c | 20 +++++-- 3 files changed, 55 insertions(+), 38 deletions(-) diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index efbe09b2d..8dc3a339b 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -1355,6 +1355,10 @@ nvme_ctrlr_state_string(enum nvme_ctrlr_state state) return "identify controller"; case NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY: return "wait for identify controller"; + case NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT: + return "set keep alive timeout"; + case NVME_CTRLR_STATE_WAIT_FOR_KEEP_ALIVE_TIMEOUT: + return "wait for set keep alive timeout"; case NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC: return "identify controller iocs specific"; case NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY_IOCS_SPECIFIC: @@ -1395,10 +1399,6 @@ nvme_ctrlr_state_string(enum nvme_ctrlr_state state) return "set doorbell buffer config"; case NVME_CTRLR_STATE_WAIT_FOR_DB_BUF_CFG: return "wait for doorbell buffer config"; - case NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT: - return "set keep alive timeout"; - case NVME_CTRLR_STATE_WAIT_FOR_KEEP_ALIVE_TIMEOUT: - return "wait for set keep alive timeout"; case NVME_CTRLR_STATE_SET_HOST_ID: return "set host ID"; case NVME_CTRLR_STATE_WAIT_FOR_HOST_ID: @@ -1490,7 +1490,7 @@ nvme_ctrlr_set_doorbell_buffer_config_done(void *arg, const struct spdk_nvme_cpl } else { NVME_CTRLR_INFOLOG(ctrlr, "Doorbell buffer config enabled\n"); } - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT, + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_HOST_ID, ctrlr->opts.admin_timeout_ms); } @@ -1501,13 +1501,13 @@ nvme_ctrlr_set_doorbell_buffer_config(struct spdk_nvme_ctrlr *ctrlr) uint64_t prp1, prp2, len; if (!ctrlr->cdata.oacs.doorbell_buffer_config) { - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT, + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_HOST_ID, ctrlr->opts.admin_timeout_ms); return 0; } if (ctrlr->trid.trtype != SPDK_NVME_TRANSPORT_PCIE) { - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT, + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_HOST_ID, ctrlr->opts.admin_timeout_ms); return 0; } @@ -1925,7 +1925,7 @@ nvme_ctrlr_identify_done(void *arg, const struct spdk_nvme_cpl *cpl) ctrlr->flags |= SPDK_NVME_CTRLR_COMPARE_AND_WRITE_SUPPORTED; } - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC, + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT, ctrlr->opts.admin_timeout_ms); } @@ -2768,8 +2768,12 @@ nvme_ctrlr_set_keep_alive_timeout_done(void *arg, const struct spdk_nvme_cpl *cp ctrlr->next_keep_alive_tick = spdk_get_ticks(); } - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_HOST_ID, - ctrlr->opts.admin_timeout_ms); + if (spdk_nvme_ctrlr_is_discovery(ctrlr)) { + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_READY, NVME_TIMEOUT_INFINITE); + } else { + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC, + ctrlr->opts.admin_timeout_ms); + } } static int @@ -2778,15 +2782,20 @@ nvme_ctrlr_set_keep_alive_timeout(struct spdk_nvme_ctrlr *ctrlr) int rc; if (ctrlr->opts.keep_alive_timeout_ms == 0) { - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_HOST_ID, - ctrlr->opts.admin_timeout_ms); + if (spdk_nvme_ctrlr_is_discovery(ctrlr)) { + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_READY, NVME_TIMEOUT_INFINITE); + } else { + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC, + ctrlr->opts.admin_timeout_ms); + } return 0; } - if (ctrlr->cdata.kas == 0) { + /* Note: Discovery controller identify data does not populate KAS according to spec. */ + if (!spdk_nvme_ctrlr_is_discovery(ctrlr) && ctrlr->cdata.kas == 0) { NVME_CTRLR_DEBUGLOG(ctrlr, "Controller KAS is 0 - not enabling Keep Alive\n"); ctrlr->opts.keep_alive_timeout_ms = 0; - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_HOST_ID, + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC, ctrlr->opts.admin_timeout_ms); return 0; } @@ -3864,17 +3873,17 @@ nvme_ctrlr_process_init(struct spdk_nvme_ctrlr *ctrlr) case NVME_CTRLR_STATE_RESET_ADMIN_QUEUE: nvme_transport_qpair_reset(ctrlr->adminq); - if (spdk_nvme_ctrlr_is_discovery(ctrlr)) { - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_READY, NVME_TIMEOUT_INFINITE); - } else { - nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY, ctrlr->opts.admin_timeout_ms); - } + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_IDENTIFY, NVME_TIMEOUT_INFINITE); break; case NVME_CTRLR_STATE_IDENTIFY: rc = nvme_ctrlr_identify(ctrlr); break; + case NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT: + rc = nvme_ctrlr_set_keep_alive_timeout(ctrlr); + break; + case NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC: rc = nvme_ctrlr_identify_iocs_specific(ctrlr); break; @@ -3924,10 +3933,6 @@ nvme_ctrlr_process_init(struct spdk_nvme_ctrlr *ctrlr) rc = nvme_ctrlr_set_doorbell_buffer_config(ctrlr); break; - case NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT: - rc = nvme_ctrlr_set_keep_alive_timeout(ctrlr); - break; - case NVME_CTRLR_STATE_SET_HOST_ID: rc = nvme_ctrlr_set_host_id(ctrlr); break; @@ -3949,6 +3954,7 @@ nvme_ctrlr_process_init(struct spdk_nvme_ctrlr *ctrlr) case NVME_CTRLR_STATE_ENABLE_WAIT_FOR_CC: case NVME_CTRLR_STATE_ENABLE_WAIT_FOR_READY_1_WAIT_FOR_CSTS: case NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY: + case NVME_CTRLR_STATE_WAIT_FOR_KEEP_ALIVE_TIMEOUT: case NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY_IOCS_SPECIFIC: case NVME_CTRLR_STATE_WAIT_FOR_GET_ZNS_CMD_EFFECTS_LOG: case NVME_CTRLR_STATE_WAIT_FOR_SET_NUM_QUEUES: @@ -3958,7 +3964,6 @@ nvme_ctrlr_process_init(struct spdk_nvme_ctrlr *ctrlr) case NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY_NS_IOCS_SPECIFIC: case NVME_CTRLR_STATE_WAIT_FOR_CONFIGURE_AER: case NVME_CTRLR_STATE_WAIT_FOR_DB_BUF_CFG: - case NVME_CTRLR_STATE_WAIT_FOR_KEEP_ALIVE_TIMEOUT: case NVME_CTRLR_STATE_WAIT_FOR_HOST_ID: spdk_nvme_qpair_process_completions(ctrlr->adminq, 0); break; diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index 6053b75d9..442ab4b32 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -653,6 +653,16 @@ enum nvme_ctrlr_state { */ NVME_CTRLR_STATE_WAIT_FOR_IDENTIFY, + /** + * Set Keep Alive Timeout of the controller. + */ + NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT, + + /** + * Waiting for Set Keep Alive Timeout to be completed. + */ + NVME_CTRLR_STATE_WAIT_FOR_KEEP_ALIVE_TIMEOUT, + /** * Get Identify I/O Command Set Specific Controller data structure. */ @@ -754,16 +764,6 @@ enum nvme_ctrlr_state { */ NVME_CTRLR_STATE_WAIT_FOR_DB_BUF_CFG, - /** - * Set Keep Alive Timeout of the controller. - */ - NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT, - - /** - * Waiting for Set Keep Alive Timeout to be completed. - */ - NVME_CTRLR_STATE_WAIT_FOR_KEEP_ALIVE_TIMEOUT, - /** * Set Host ID of the controller. */ 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 c183091ec..7846303bc 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 @@ -2284,6 +2284,8 @@ test_nvme_ctrlr_init_set_nvmf_ioccsz(void) ctrlr.state = NVME_CTRLR_STATE_IDENTIFY; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_NUM_QUEUES); @@ -2303,6 +2305,8 @@ test_nvme_ctrlr_init_set_nvmf_ioccsz(void) ctrlr.state = NVME_CTRLR_STATE_IDENTIFY; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_NUM_QUEUES); @@ -2324,6 +2328,8 @@ test_nvme_ctrlr_init_set_nvmf_ioccsz(void) ctrlr.state = NVME_CTRLR_STATE_IDENTIFY; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_NUM_QUEUES); @@ -2345,6 +2351,8 @@ test_nvme_ctrlr_init_set_nvmf_ioccsz(void) ctrlr.state = NVME_CTRLR_STATE_IDENTIFY; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_NUM_QUEUES); @@ -2366,6 +2374,8 @@ test_nvme_ctrlr_init_set_nvmf_ioccsz(void) ctrlr.state = NVME_CTRLR_STATE_IDENTIFY; CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_NUM_QUEUES); @@ -2390,6 +2400,8 @@ test_nvme_ctrlr_init_set_num_queues(void) SPDK_CU_ASSERT_FATAL(nvme_ctrlr_construct(&ctrlr) == 0); ctrlr.state = NVME_CTRLR_STATE_IDENTIFY; + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> SET_KEEP_ALIVE_TIMEOUT */ + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> SET_IDENTIFY_IOCS_SPECIFIC */ CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC); CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> SET_NUM_QUEUES */ @@ -2417,8 +2429,8 @@ test_nvme_ctrlr_init_set_keep_alive_timeout(void) ctrlr.cdata.kas = 1; ctrlr.state = NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT; fake_cpl.cdw0 = 120000; - CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> SET_HOST_ID */ - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_HOST_ID); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_IOCS_SPECIFIC */ + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC); CU_ASSERT(ctrlr.opts.keep_alive_timeout_ms == 120000); fake_cpl.cdw0 = 0; @@ -2427,8 +2439,8 @@ test_nvme_ctrlr_init_set_keep_alive_timeout(void) ctrlr.cdata.kas = 1; ctrlr.state = NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT; set_status_code = SPDK_NVME_SC_INVALID_FIELD; - CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> SET_HOST_ID */ - CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_SET_HOST_ID); + CU_ASSERT(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_IOCS_SPECIFIC */ + CU_ASSERT(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_IOCS_SPECIFIC); CU_ASSERT(ctrlr.opts.keep_alive_timeout_ms == 60000); set_status_code = SPDK_NVME_SC_SUCCESS;