From e27421b344ac37963af2b3bd9128755e82710d50 Mon Sep 17 00:00:00 2001 From: Changpeng Liu Date: Thu, 11 Jul 2019 04:14:28 -0400 Subject: [PATCH] nvme: fix req leaks There are many req leaks when a controller failure occurs during submitting IO. It must free all of the children before freeing the parent req. If a part of the child req has been sent to the back end and a part of the child req fails, removes the failed req from the parent req and the parent req must be retained, freeing the parent req after all of the submitted reqs return. Change-Id: Ieb5423fd19c9bb0420f154b3cfc17918c2b80748 Signed-off-by: Huiming Xie Signed-off-by: Changpeng Liu Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/461734 Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Darek Stojaczyk --- lib/nvme/nvme_qpair.c | 41 ++++++++--- .../lib/nvme/nvme_qpair.c/nvme_qpair_ut.c | 72 +++++++++++++++++++ 2 files changed, 104 insertions(+), 9 deletions(-) diff --git a/lib/nvme/nvme_qpair.c b/lib/nvme/nvme_qpair.c index 2e662fcfd..97eb23de9 100644 --- a/lib/nvme/nvme_qpair.c +++ b/lib/nvme/nvme_qpair.c @@ -546,11 +546,6 @@ nvme_qpair_submit_request(struct spdk_nvme_qpair *qpair, struct nvme_request *re struct spdk_nvme_ctrlr *ctrlr = qpair->ctrlr; bool child_req_failed = false; - if (ctrlr->is_failed) { - nvme_free_request(req); - return -ENXIO; - } - nvme_qpair_check_enabled(qpair); if (req->num_children) { @@ -559,17 +554,28 @@ nvme_qpair_submit_request(struct spdk_nvme_qpair *qpair, struct nvme_request *re * request itself, since the parent is the original unsplit request. */ TAILQ_FOREACH_SAFE(child_req, &req->children, child_tailq, tmp) { - if (!child_req_failed) { + if (spdk_likely(!child_req_failed)) { rc = nvme_qpair_submit_request(qpair, child_req); - if (rc != 0) { + if (spdk_unlikely(rc != 0)) { child_req_failed = true; } } else { /* free remaining child_reqs since one child_req fails */ nvme_request_remove_child(req, child_req); + nvme_request_free_children(child_req); nvme_free_request(child_req); } } + if (spdk_unlikely(child_req_failed)) { + /* part of children requests have been submitted, + * return success for this case. + */ + if (req->num_children) { + return 0; + } + goto error; + } + return rc; } @@ -593,6 +599,11 @@ nvme_qpair_submit_request(struct spdk_nvme_qpair *qpair, struct nvme_request *re } } + if (spdk_unlikely(ctrlr->is_failed)) { + rc = -ENXIO; + goto error; + } + /* assign submit_tick before submitting req to specific transport */ if (spdk_unlikely(ctrlr->timeout_enabled)) { if (req->submit_tick == 0) { /* req submitted for the first time */ @@ -604,12 +615,12 @@ nvme_qpair_submit_request(struct spdk_nvme_qpair *qpair, struct nvme_request *re } if (spdk_likely(qpair->is_enabled)) { - return nvme_transport_qpair_submit_request(qpair, req); + rc = nvme_transport_qpair_submit_request(qpair, req); } else if (nvme_qpair_is_admin_queue(qpair) && req->cmd.opc == SPDK_NVME_OPC_FABRIC) { /* Always allow fabrics commands through on the admin qpair - these get * the controller out of reset state. */ - return nvme_transport_qpair_submit_request(qpair, req); + rc = nvme_transport_qpair_submit_request(qpair, req); } else { /* The controller is being reset - queue this request and * submit it later when the reset is completed. @@ -617,6 +628,18 @@ nvme_qpair_submit_request(struct spdk_nvme_qpair *qpair, struct nvme_request *re STAILQ_INSERT_TAIL(&qpair->queued_req, req, stailq); return 0; } + + if (spdk_likely(rc == 0)) { + return 0; + } + +error: + if (req->parent != NULL) { + nvme_request_remove_child(req->parent, req); + } + nvme_free_request(req); + + return rc; } void 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 16e0d974c..b2995f33e 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 @@ -368,6 +368,76 @@ test_nvme_qpair_add_cmd_error_injection(void) cleanup_submit_request_test(&qpair); } +static void +test_nvme_qpair_submit_request(void) +{ + int rc; + struct spdk_nvme_qpair qpair = {}; + struct spdk_nvme_ctrlr ctrlr = {}; + struct nvme_request *req, *req1, *req2, *req3, *req2_1, *req2_2, *req2_3; + + prepare_submit_request_test(&qpair, &ctrlr); + + /* + * Build a request chain like the following: + * req + * | + * --------------- + * | | | + * req1 req2 req3 + * | + * --------------- + * | | | + * req2_1 req2_2 req2_3 + */ + req = nvme_allocate_request_null(&qpair, NULL, NULL); + CU_ASSERT(req != NULL); + TAILQ_INIT(&req->children); + + req1 = nvme_allocate_request_null(&qpair, NULL, NULL); + CU_ASSERT(req1 != NULL); + req->num_children++; + TAILQ_INSERT_TAIL(&req->children, req1, child_tailq); + req1->parent = req; + + req2 = nvme_allocate_request_null(&qpair, NULL, NULL); + CU_ASSERT(req2 != NULL); + TAILQ_INIT(&req2->children); + req->num_children++; + TAILQ_INSERT_TAIL(&req->children, req2, child_tailq); + req2->parent = req; + + req3 = nvme_allocate_request_null(&qpair, NULL, NULL); + CU_ASSERT(req3 != NULL); + req->num_children++; + TAILQ_INSERT_TAIL(&req->children, req3, child_tailq); + req3->parent = req; + + req2_1 = nvme_allocate_request_null(&qpair, NULL, NULL); + CU_ASSERT(req2_1 != NULL); + req2->num_children++; + TAILQ_INSERT_TAIL(&req2->children, req2_1, child_tailq); + req2_1->parent = req2; + + req2_2 = nvme_allocate_request_null(&qpair, NULL, NULL); + CU_ASSERT(req2_2 != NULL); + req2->num_children++; + TAILQ_INSERT_TAIL(&req2->children, req2_2, child_tailq); + req2_2->parent = req2; + + req2_3 = nvme_allocate_request_null(&qpair, NULL, NULL); + CU_ASSERT(req2_3 != NULL); + req2->num_children++; + TAILQ_INSERT_TAIL(&req2->children, req2_3, child_tailq); + req2_3->parent = req2; + + ctrlr.is_failed = true; + rc = nvme_qpair_submit_request(&qpair, req); + SPDK_CU_ASSERT_FATAL(rc == -ENXIO); + + cleanup_submit_request_test(&qpair); +} + int main(int argc, char **argv) { CU_pSuite suite = NULL; @@ -394,6 +464,8 @@ int main(int argc, char **argv) #endif || CU_add_test(suite, "spdk_nvme_qpair_add_cmd_error_injection", test_nvme_qpair_add_cmd_error_injection) == NULL + || CU_add_test(suite, "spdk_nvme_qpair_submit_request", + test_nvme_qpair_submit_request) == NULL ) { CU_cleanup_registry(); return CU_get_error();