Commit ff9516bd authored by Changpeng Liu's avatar Changpeng Liu Committed by Tomasz Zawadzki
Browse files

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: default avatarChangpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/477754


Community-CI: SPDK CI Jenkins <sys_sgci@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: default avatarAlexey Marchuk <alexeymar@mellanox.com>
parent 10cd9e86

lib/nvme/nvme_internal.h

+7 −1
Original line number Diff line number Diff line
@@ -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

lib/nvme/nvme_qpair.c

+12 −0
Original line number Diff line number Diff line
@@ -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);

test/unit/lib/nvme/nvme_qpair.c/nvme_qpair_ut.c

+34 −0
Original line number Diff line number Diff line
@@ -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();