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 <xiehuiming@huawei.com>
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/461734
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

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

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();