From b10fbdf58f552152caa19cc05692de3ed13d94a0 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Tue, 8 Jun 2021 19:08:19 +0000 Subject: [PATCH] nvme: store fabrics connect data ptr in status structure Since the connect will be completed asynchronously, we need to keep the pointer around so we can access (and free it!) later when the command completes. Also change the code to poll on the status using the new nvme_wait_for_completion_poll(), as prep for upcoming patches. Signed-off-by: Jim Harris Signed-off-by: Konrad Sztyber Change-Id: I28add8f967fd000afed1e50e491a16ea9da16c22 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8603 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Changpeng Liu Reviewed-by: Aleksey Marchuk --- lib/nvme/nvme.c | 1 + lib/nvme/nvme_fabric.c | 21 ++++++--- lib/nvme/nvme_internal.h | 5 ++ .../lib/nvme/nvme_fabric.c/nvme_fabric_ut.c | 47 +++++++++++++++---- 4 files changed, 58 insertions(+), 16 deletions(-) diff --git a/lib/nvme/nvme.c b/lib/nvme/nvme.c index 819f0d6c9..5f34451ea 100644 --- a/lib/nvme/nvme.c +++ b/lib/nvme/nvme.c @@ -244,6 +244,7 @@ nvme_completion_poll_cb(void *arg, const struct spdk_nvme_cpl *cpl) if (status->timed_out) { /* There is no routine waiting for the completion of this request, free allocated memory */ + spdk_free(status->dma_data); free(status); return; } diff --git a/lib/nvme/nvme_fabric.c b/lib/nvme/nvme_fabric.c index 4a3c4c193..6f4896fc8 100644 --- a/lib/nvme/nvme_fabric.c +++ b/lib/nvme/nvme_fabric.c @@ -416,6 +416,8 @@ nvme_fabric_qpair_connect(struct spdk_nvme_qpair *qpair, uint32_t num_entries) return -ENOMEM; } + status->dma_data = nvmf_data; + memset(&cmd, 0, sizeof(cmd)); cmd.opcode = SPDK_NVME_OPC_FABRIC; cmd.fctype = SPDK_NVMF_FABRIC_COMMAND_CONNECT; @@ -441,16 +443,23 @@ nvme_fabric_qpair_connect(struct spdk_nvme_qpair *qpair, uint32_t num_entries) nvme_completion_poll_cb, status); if (rc < 0) { SPDK_ERRLOG("Failed to allocate/submit FABRIC_CONNECT command, rc %d\n", rc); - spdk_free(nvmf_data); + spdk_free(status->dma_data); free(status); return rc; } /* If we time out, the qpair will abort the request upon destruction. */ - rc = nvme_wait_for_completion_timeout(qpair, status, ctrlr->opts.fabrics_connect_timeout_us); - if (rc) { + if (ctrlr->opts.fabrics_connect_timeout_us > 0) { + status->timeout_tsc = spdk_get_ticks() + ctrlr->opts.fabrics_connect_timeout_us * + spdk_get_ticks_hz() / SPDK_SEC_TO_USEC; + } + + /* Wait until the command completes or times out */ + while (nvme_wait_for_completion_robust_lock_timeout_poll(qpair, status, NULL) == -EAGAIN) {} + + if (status->timed_out || spdk_nvme_cpl_is_error(&status->cpl)) { SPDK_ERRLOG("Connect command failed, rc %d, trtype:%s adrfam:%s traddr:%s trsvcid:%s subnqn:%s\n", - rc, + status->timed_out ? -ECANCELED : -EIO, spdk_nvme_transport_id_trtype_str(ctrlr->trid.trtype), spdk_nvme_transport_id_adrfam_str(ctrlr->trid.adrfam), ctrlr->trid.traddr, @@ -460,8 +469,8 @@ nvme_fabric_qpair_connect(struct spdk_nvme_qpair *qpair, uint32_t num_entries) SPDK_ERRLOG("Connect command completed with error: sct %d, sc %d\n", status->cpl.status.sct, status->cpl.status.sc); } - spdk_free(nvmf_data); if (!status->timed_out) { + spdk_free(status->dma_data); free(status); } return -EIO; @@ -473,7 +482,7 @@ nvme_fabric_qpair_connect(struct spdk_nvme_qpair *qpair, uint32_t num_entries) SPDK_DEBUGLOG(nvme, "CNTLID 0x%04" PRIx16 "\n", ctrlr->cntlid); } - spdk_free(nvmf_data); + spdk_free(status->dma_data); free(status); return 0; } diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index f954d1959..e7a84885a 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -383,6 +383,11 @@ struct nvme_request { struct nvme_completion_poll_status { struct spdk_nvme_cpl cpl; uint64_t timeout_tsc; + /** + * DMA buffer retained throughout the duration of the command. It'll be released + * automatically if the command times out, otherwise the user is responsible for freeing it. + */ + void *dma_data; bool done; /* This flag indicates that the request has been timed out and the memory must be freed in a completion callback */ diff --git a/test/unit/lib/nvme/nvme_fabric.c/nvme_fabric_ut.c b/test/unit/lib/nvme/nvme_fabric.c/nvme_fabric_ut.c index 0b5c8a6c1..67c8ecd85 100644 --- a/test/unit/lib/nvme/nvme_fabric.c/nvme_fabric_ut.c +++ b/test/unit/lib/nvme/nvme_fabric.c/nvme_fabric_ut.c @@ -42,8 +42,6 @@ pid_t g_spdk_nvme_pid; struct spdk_nvmf_fabric_prop_set_cmd g_ut_cmd = {}; struct spdk_nvmf_fabric_prop_get_rsp g_ut_response = {}; -DEFINE_STUB_V(nvme_completion_poll_cb, (void *arg, const struct spdk_nvme_cpl *cpl)); - DEFINE_STUB_V(spdk_nvme_ctrlr_get_default_ctrlr_opts, (struct spdk_nvme_ctrlr_opts *opts, size_t opts_size)); @@ -82,6 +80,7 @@ DEFINE_STUB(spdk_nvme_transport_id_adrfam_str, const char *, DEFINE_STUB(nvme_ctrlr_process_init, int, (struct spdk_nvme_ctrlr *ctrlr), 0); static struct spdk_nvmf_fabric_connect_data g_nvmf_data; +static struct nvme_request *g_request; int spdk_nvme_ctrlr_cmd_io_raw(struct spdk_nvme_ctrlr *ctrlr, @@ -100,15 +99,30 @@ spdk_nvme_ctrlr_cmd_io_raw(struct spdk_nvme_ctrlr *ctrlr, memcpy(&req->cmd, cmd, sizeof(req->cmd)); memcpy(&g_nvmf_data, buf, sizeof(g_nvmf_data)); + g_request = req; return 0; } -DEFINE_RETURN_MOCK(nvme_wait_for_completion_timeout, int); +void +nvme_completion_poll_cb(void *arg, const struct spdk_nvme_cpl *cpl) +{ + struct nvme_completion_poll_status *status = arg; + + if (status->timed_out) { + spdk_free(status->dma_data); + free(status); + } + + g_request = NULL; +} + +static bool g_nvme_wait_for_completion_timeout; + int -nvme_wait_for_completion_timeout(struct spdk_nvme_qpair *qpair, - struct nvme_completion_poll_status *status, - uint64_t timeout_in_usecs) +nvme_wait_for_completion_robust_lock_timeout_poll(struct spdk_nvme_qpair *qpair, + struct nvme_completion_poll_status *status, + pthread_mutex_t *robust_mutex) { struct spdk_nvmf_fabric_connect_rsp *rsp = (void *)&status->cpl; @@ -116,8 +130,7 @@ nvme_wait_for_completion_timeout(struct spdk_nvme_qpair *qpair, rsp->status_code_specific.success.cntlid = 1; } - status->timed_out = false; - HANDLE_RETURN_MOCK(nvme_wait_for_completion_timeout); + status->timed_out = g_nvme_wait_for_completion_timeout; return 0; } @@ -218,6 +231,19 @@ spdk_nvme_ctrlr_cmd_admin_raw(struct spdk_nvme_ctrlr *ctrlr, return 0; } +static void +abort_request(struct nvme_request *request) +{ + struct spdk_nvme_cpl cpl = { + .status = { + .sct = SPDK_NVME_SCT_GENERIC, + .sc = SPDK_NVME_SC_ABORTED_SQ_DELETION, + } + }; + + request->cb_fn(request->cb_arg, &cpl); +} + static void test_nvme_fabric_prop_set_cmd(void) { @@ -403,11 +429,12 @@ test_nvme_fabric_qpair_connect(void) /* Wait_for completion timeout */ STAILQ_INSERT_HEAD(&qpair.free_req, &req, stailq); - MOCK_SET(nvme_wait_for_completion_timeout, 1); + g_nvme_wait_for_completion_timeout = true; rc = nvme_fabric_qpair_connect(&qpair, 1); CU_ASSERT(rc == -EIO); - MOCK_CLEAR(nvme_wait_for_completion_timeout); + g_nvme_wait_for_completion_timeout = false; + abort_request(g_request); /* Input parameters invalid */ rc = nvme_fabric_qpair_connect(&qpair, 0);