From ff9516bdcca2d149a802b40b6a8b4d5e466cc943 Mon Sep 17 00:00:00 2001 From: Changpeng Liu Date: Thu, 12 Dec 2019 01:47:01 -0500 Subject: [PATCH] nvme: call the callback for the queued requests when there is submission failure For the requests which don't have children requests, SPDK may queue them to the queued_req list due to limited resources, in the completion path, we may resubmit them to the controller. When the controller was removed the submission path will return -ENXIO and we will free the requests directly, so the callback will not be trigerred for these requests. Here we added a flag to indicate the request is from queued_req list or not, so for the failure submission, we can triger user's callback. Fix issue #1097 Change-Id: I901ac81733c2319e540d24baf5b8faa1c649eb35 Signed-off-by: Changpeng Liu Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/477754 Community-CI: SPDK CI Jenkins Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Shuhei Matsumoto Reviewed-by: Alexey Marchuk --- lib/nvme/nvme_internal.h | 8 ++++- lib/nvme/nvme_qpair.c | 12 +++++++ .../lib/nvme/nvme_qpair.c/nvme_qpair_ut.c | 34 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index e0d82af90..109e2e5a4 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -236,7 +236,13 @@ struct nvme_request { uint8_t retries; - bool timed_out; + uint8_t timed_out : 1; + + /** + * True if the request is in the queued_req list. + */ + uint8_t queued : 1; + uint8_t reserved : 6; /** * Number of children requests still outstanding for this diff --git a/lib/nvme/nvme_qpair.c b/lib/nvme/nvme_qpair.c index 82d13dcf6..88a90e5e7 100644 --- a/lib/nvme/nvme_qpair.c +++ b/lib/nvme/nvme_qpair.c @@ -679,6 +679,7 @@ _nvme_qpair_submit_request(struct spdk_nvme_qpair *qpair, struct nvme_request *r } if (spdk_likely(rc == 0)) { + req->queued = false; return 0; } @@ -690,6 +691,14 @@ error: if (req->parent != NULL) { nvme_request_remove_child(req->parent, req); } + + /* The request is from queued_req list we should trigger the callback from caller */ + if (spdk_unlikely(req->queued)) { + nvme_qpair_manual_complete_request(qpair, req, SPDK_NVME_SCT_GENERIC, + SPDK_NVME_SC_INTERNAL_DEVICE_ERROR, true, true); + return rc; + } + nvme_free_request(req); return rc; @@ -707,12 +716,14 @@ nvme_qpair_submit_request(struct spdk_nvme_qpair *qpair, struct nvme_request *re * through this path. */ STAILQ_INSERT_TAIL(&qpair->queued_req, req, stailq); + req->queued = true; return 0; } rc = _nvme_qpair_submit_request(qpair, req); if (rc == -EAGAIN) { STAILQ_INSERT_TAIL(&qpair->queued_req, req, stailq); + req->queued = true; rc = 0; } @@ -730,6 +741,7 @@ nvme_qpair_resubmit_request(struct spdk_nvme_qpair *qpair, struct nvme_request * * completions and resubmissions. */ assert(req->num_children == 0); + assert(req->queued); rc = _nvme_qpair_submit_request(qpair, req); if (spdk_unlikely(rc == -EAGAIN)) { STAILQ_INSERT_HEAD(&qpair->queued_req, req, stailq); diff --git a/test/unit/lib/nvme/nvme_qpair.c/nvme_qpair_ut.c b/test/unit/lib/nvme/nvme_qpair.c/nvme_qpair_ut.c index 857efbd35..530cf8e0d 100644 --- a/test/unit/lib/nvme/nvme_qpair.c/nvme_qpair_ut.c +++ b/test/unit/lib/nvme/nvme_qpair.c/nvme_qpair_ut.c @@ -223,7 +223,9 @@ static void test_nvme_qpair_process_completions(void) /* If we are resetting, make sure that we don't call into the transport. */ STAILQ_INSERT_TAIL(&qpair.queued_req, &dummy_1, stailq); + dummy_1.queued = true; STAILQ_INSERT_TAIL(&qpair.queued_req, &dummy_2, stailq); + dummy_2.queued = true; g_num_cb_failed = 0; ctrlr.is_failed = false; ctrlr.is_removed = false; @@ -560,6 +562,36 @@ test_nvme_qpair_submit_request(void) cleanup_submit_request_test(&qpair); } +static void +test_nvme_qpair_resubmit_request_with_transport_failed(void) +{ + int rc; + struct spdk_nvme_qpair qpair = {}; + struct spdk_nvme_ctrlr ctrlr = {}; + struct nvme_request *req; + + prepare_submit_request_test(&qpair, &ctrlr); + + req = nvme_allocate_request_null(&qpair, dummy_cb_fn, NULL); + CU_ASSERT(req != NULL); + TAILQ_INIT(&req->children); + + STAILQ_INSERT_TAIL(&qpair.queued_req, req, stailq); + req->queued = true; + + g_transport_process_completions_rc = 1; + qpair.state = NVME_QPAIR_ENABLED; + g_num_cb_failed = 0; + MOCK_SET(nvme_transport_qpair_submit_request, -EINVAL); + rc = spdk_nvme_qpair_process_completions(&qpair, g_transport_process_completions_rc); + MOCK_CLEAR(nvme_transport_qpair_submit_request); + CU_ASSERT(rc == g_transport_process_completions_rc); + CU_ASSERT(STAILQ_EMPTY(&qpair.queued_req)); + CU_ASSERT(g_num_cb_failed == 1); + + cleanup_submit_request_test(&qpair); +} + int main(int argc, char **argv) { CU_pSuite suite = NULL; @@ -588,6 +620,8 @@ int main(int argc, char **argv) test_nvme_qpair_add_cmd_error_injection) == NULL || CU_add_test(suite, "spdk_nvme_qpair_submit_request", test_nvme_qpair_submit_request) == NULL + || CU_add_test(suite, "nvme_qpair_resubmit_request_with_transport_failed", + test_nvme_qpair_resubmit_request_with_transport_failed) == NULL ) { CU_cleanup_registry(); return CU_get_error();