diff --git a/include/spdk/nvmf_transport.h b/include/spdk/nvmf_transport.h index 828c5828e..810bbb02f 100644 --- a/include/spdk/nvmf_transport.h +++ b/include/spdk/nvmf_transport.h @@ -126,7 +126,10 @@ struct spdk_nvmf_qpair { bool connect_received; bool disconnect_started; - struct spdk_nvmf_request *first_fused_req; + union { + struct spdk_nvmf_request *first_fused_req; + struct spdk_nvmf_request *connect_req; + }; TAILQ_HEAD(, spdk_nvmf_request) outstanding; TAILQ_ENTRY(spdk_nvmf_qpair) link; diff --git a/lib/nvmf/ctrlr.c b/lib/nvmf/ctrlr.c index 3e57f3530..01ed56f69 100644 --- a/lib/nvmf/ctrlr.c +++ b/lib/nvmf/ctrlr.c @@ -31,6 +31,8 @@ #define NVMF_CTRLR_RESET_SHN_TIMEOUT_IN_MS (NVMF_CC_RESET_SHN_TIMEOUT_IN_MS + 5000) +#define DUPLICATE_QID_RETRY_US 100 + /* * Report the SPDK version as the firmware revision. * SPDK_VERSION_STRING won't fit into FR (only 8 bytes), so try to fit the most important parts. @@ -209,6 +211,8 @@ nvmf_ctrlr_start_keep_alive_timer(struct spdk_nvmf_ctrlr *ctrlr) } } +static int _retry_qid_check(void *ctx); + static void ctrlr_add_qpair_and_send_rsp(struct spdk_nvmf_qpair *qpair, struct spdk_nvmf_ctrlr *ctrlr, @@ -219,10 +223,22 @@ ctrlr_add_qpair_and_send_rsp(struct spdk_nvmf_qpair *qpair, assert(ctrlr->admin_qpair->group->thread == spdk_get_thread()); if (spdk_bit_array_get(ctrlr->qpair_mask, qpair->qid)) { - SPDK_ERRLOG("Got I/O connect with duplicate QID %u\n", qpair->qid); - rsp->status.sct = SPDK_NVME_SCT_COMMAND_SPECIFIC; - rsp->status.sc = SPDK_NVME_SC_INVALID_QUEUE_IDENTIFIER; - spdk_nvmf_request_complete(req); + if (qpair->connect_req != NULL) { + SPDK_ERRLOG("Got I/O connect with duplicate QID %u\n", qpair->qid); + rsp->status.sct = SPDK_NVME_SCT_COMMAND_SPECIFIC; + rsp->status.sc = SPDK_NVME_SC_INVALID_QUEUE_IDENTIFIER; + qpair->connect_req = NULL; + qpair->ctrlr = NULL; + spdk_nvmf_request_complete(req); + } else { + SPDK_WARNLOG("Duplicate QID detected, re-check in %dus\n", + DUPLICATE_QID_RETRY_US); + qpair->connect_req = req; + /* Set qpair->ctrlr here so that we'll have it when the poller expires. */ + qpair->ctrlr = ctrlr; + req->poller = SPDK_POLLER_REGISTER(_retry_qid_check, qpair, + DUPLICATE_QID_RETRY_US); + } return; } @@ -239,6 +255,18 @@ ctrlr_add_qpair_and_send_rsp(struct spdk_nvmf_qpair *qpair, ctrlr->hostnqn); } +static int +_retry_qid_check(void *ctx) +{ + struct spdk_nvmf_qpair *qpair = ctx; + struct spdk_nvmf_request *req = qpair->connect_req; + struct spdk_nvmf_ctrlr *ctrlr = req->qpair->ctrlr; + + spdk_poller_unregister(&req->poller); + ctrlr_add_qpair_and_send_rsp(qpair, ctrlr, req); + return SPDK_POLLER_BUSY; +} + static void _nvmf_ctrlr_add_admin_qpair(void *ctx) { diff --git a/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c b/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c index 17e1abbf3..b699b2249 100644 --- a/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c +++ b/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c @@ -811,13 +811,46 @@ test_connect(void) sgroups[subsystem.id].mgmt_io_outstanding++; TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link); rc = nvmf_ctrlr_cmd_connect(&req); - poll_threads(); CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS); + poll_threads(); + /* First time, it will detect duplicate QID and schedule a retry. So for + * now we should expect the response to still be all zeroes. + */ + CU_ASSERT(spdk_mem_all_zero(&rsp, sizeof(rsp))); + CU_ASSERT(sgroups[subsystem.id].mgmt_io_outstanding == 1); + + /* Now advance the clock, so that the retry poller executes. */ + spdk_delay_us(DUPLICATE_QID_RETRY_US * 2); + poll_threads(); CU_ASSERT(rsp.nvme_cpl.status.sct == SPDK_NVME_SCT_COMMAND_SPECIFIC); CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVME_SC_INVALID_QUEUE_IDENTIFIER); CU_ASSERT(qpair.ctrlr == NULL); CU_ASSERT(sgroups[subsystem.id].mgmt_io_outstanding == 0); + /* I/O connect with temporarily duplicate queue ID. This covers race + * where qpair_mask bit may not yet be cleared, even though initiator + * has closed the connection. See issue #2955. */ + memset(&rsp, 0, sizeof(rsp)); + sgroups[subsystem.id].mgmt_io_outstanding++; + TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link); + rc = nvmf_ctrlr_cmd_connect(&req); + CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS); + poll_threads(); + /* First time, it will detect duplicate QID and schedule a retry. So for + * now we should expect the response to still be all zeroes. + */ + CU_ASSERT(spdk_mem_all_zero(&rsp, sizeof(rsp))); + CU_ASSERT(sgroups[subsystem.id].mgmt_io_outstanding == 1); + + /* Now advance the clock, so that the retry poller executes. */ + spdk_bit_array_clear(ctrlr.qpair_mask, 1); + spdk_delay_us(DUPLICATE_QID_RETRY_US * 2); + poll_threads(); + CU_ASSERT(nvme_status_success(&rsp.nvme_cpl.status)); + CU_ASSERT(qpair.ctrlr == &ctrlr); + CU_ASSERT(sgroups[subsystem.id].mgmt_io_outstanding == 0); + qpair.ctrlr = NULL; + /* I/O connect when admin qpair is being destroyed */ admin_qpair.group = NULL; admin_qpair.state = SPDK_NVMF_QPAIR_DEACTIVATING;