Commit 2575aaec authored Oct 09, 2019 by Seth Howell Committed by Jim Harris Oct 11, 2019

nvme: make sure we queue requests in order.

My recent changes that introduced batching to queued request
resubmission also introduced a regression that can lead to reordering
requests before submitting them to the drive. This change prevents that.

We wait until inside the internal _nvme_qpair_submit_request function to
check for queued entries to avoid queueing a request that has children.

If a request that has children gets queued, when we process completions
and resubmit the parent, it will result in the children being submitted.
Since we only account for the number of requests we completed in the
last iteration, some of the child requests may be requeued out of order,
or worse, none of the child requests will end up being submitted to the
transport and they will all be queued behind previously queued requests.

Change-Id: I58e1c458c25fbf3f9f75364f05b1076b166a6212
Signed-off-by: Seth Howell <seth.howell@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/470890

Reviewed-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>

parent 225c74e4

lib/nvme/nvme_qpair.c

+16 −0

Original line number	Diff line number	Diff line
		@@ -671,6 +671,16 @@ nvme_qpair_submit_request(struct spdk_nvme_qpair qpair, struct nvme_request re
		{
		int rc;

		if (spdk_unlikely(!STAILQ_EMPTY(&qpair->queued_req) && req->num_children == 0)) {
		/*
		* requests that have no children should be sent to the transport after all
		* currently queued requests. Requests with chilren will be split and go back
		* through this path.
		*/
		STAILQ_INSERT_TAIL(&qpair->queued_req, req, stailq);
		return 0;
		}

		rc = _nvme_qpair_submit_request(qpair, req);
		if (rc == -EAGAIN) {
		STAILQ_INSERT_TAIL(&qpair->queued_req, req, stailq);
		@@ -685,6 +695,12 @@ nvme_qpair_resubmit_request(struct spdk_nvme_qpair qpair, struct nvme_request
		{
		int rc;

		/*
		* We should never have a request with children on the queue.
		* This is necessary to preserve the 1:1 relationship between
		* completions and resubmissions.
		*/
		assert(req->num_children == 0);
		rc = _nvme_qpair_submit_request(qpair, req);
		if (spdk_unlikely(rc == -EAGAIN)) {
		STAILQ_INSERT_HEAD(&qpair->queued_req, req, stailq);