Commit 12c640c0 authored by Jim Harris's avatar Jim Harris Committed by Tomasz Zawadzki
Browse files

nvmf: use sgroup->queued TAILQ for CONNECTs that need to be retried 



The current method for retrying CONNECTs is not reliable, in fact we have been
seeing a lot of CI failures around the CONNECTs timing out.

We can actually make this much more reliable, and also way simpler. Each
sgroup already has a TAILQ of requests that will get retried when the
sgroup gets resumed. So just put the CONNECT request on that TAILQ, and
modify the resume logic to send these commands down the exec_fabrics()
path.

We can remove all of the timeout related code for this now too. We know that
these CONNECTs will get retried when the subsystem is in RESUMING state.

Tested this using a local patch that would inject a 10ms delay (using a
timed poller) before starting a RESUME, with debug prints showing when
a CONNECT was queued, and running the connect_stress.sh in a loop on my
test system.

Fixes issue #3095.

Signed-off-by: default avatarJim Harris <jim.harris@samsung.com>
Change-Id: I06ae83399b91e63b590f88bf420e3cba2149223a
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/21392


Community-CI: Mellanox Build Bot
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Reviewed-by: default avatarTomasz Zawadzki <tomasz.zawadzki@intel.com>
parent a1f570c2

lib/nvmf/ctrlr.c

+6 −36
Original line number Diff line number Diff line
@@ -920,28 +920,6 @@ out: 
	return status;

}



static int nvmf_ctrlr_cmd_connect(struct spdk_nvmf_request *req);



static int

retry_connect(void *arg)

{

	struct spdk_nvmf_request *req = arg;

	struct spdk_nvmf_subsystem_poll_group *sgroup;

	int rc;



	sgroup = nvmf_subsystem_pg_from_connect_cmd(req);

	/* subsystem may be deleted during the retry interval, so we need to check sgroup */

	if (sgroup != NULL) {

		sgroup->mgmt_io_outstanding++;

	}

	spdk_poller_unregister(&req->poller);

	rc = nvmf_ctrlr_cmd_connect(req);

	if (rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE) {

		_nvmf_request_complete(req);

	}

	return SPDK_POLLER_BUSY;

}



static int

nvmf_ctrlr_cmd_connect(struct spdk_nvmf_request *req)

{
@@ -974,25 +952,17 @@ nvmf_ctrlr_cmd_connect(struct spdk_nvmf_request *req) 
	    (subsystem->state == SPDK_NVMF_SUBSYSTEM_DEACTIVATING)) {

		struct spdk_nvmf_subsystem_poll_group *sgroup;



		if (req->timeout_tsc == 0) {

			/* We will only retry the request up to 1 second. */

			req->timeout_tsc = spdk_get_ticks() + spdk_get_ticks_hz();

		} else if (spdk_get_ticks() > req->timeout_tsc) {

			SPDK_ERRLOG("Subsystem '%s' was not ready for 1 second\n", subsystem->subnqn);

			rsp->status.sct = SPDK_NVME_SCT_COMMAND_SPECIFIC;

			rsp->status.sc = SPDK_NVMF_FABRIC_SC_CONTROLLER_BUSY;

			return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

		}



		/* Subsystem is not ready to handle a connect. Use a poller to retry it

		 * again later. Decrement the mgmt_io_outstanding to avoid the

		 * subsystem waiting for this command to complete before unpausing.

		/* Subsystem is not ready to handle a connect. Decrement

		 * the mgmt_io_outstanding to avoid the subsystem waiting

		 * for this command to complete before unpausing. Queued

		 * requests get retried when subsystem resumes.

		 */

		sgroup = nvmf_subsystem_pg_from_connect_cmd(req);

		assert(sgroup != NULL);

		sgroup->mgmt_io_outstanding--;

		TAILQ_REMOVE(&req->qpair->outstanding, req, link);

		TAILQ_INSERT_TAIL(&sgroup->queued, req, link);

		SPDK_DEBUGLOG(nvmf, "Subsystem '%s' is not ready for connect, retrying...\n", subsystem->subnqn);

		req->poller = SPDK_POLLER_REGISTER(retry_connect, req, 100);

		return SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS;

	}

lib/nvmf/nvmf.c

+2 −0
Original line number Diff line number Diff line
@@ -1898,6 +1898,8 @@ nvmf_poll_group_resume_subsystem(struct spdk_nvmf_poll_group *group, 
		TAILQ_REMOVE(&sgroup->queued, req, link);

		if (spdk_nvmf_request_using_zcopy(req)) {

			spdk_nvmf_request_zcopy_start(req);

		} else if (nvmf_request_is_fabric_connect(req)) {

			spdk_nvmf_request_exec_fabrics(req);

		} else {

			spdk_nvmf_request_exec(req);

		}

test/unit/lib/nvmf/nvmf.c/nvmf_ut.c

+1 −0
Original line number Diff line number Diff line
@@ -95,6 +95,7 @@ DEFINE_STUB(nvmf_transport_qpair_get_listen_trid, int, 
	    (struct spdk_nvmf_qpair *qpair,

	     struct spdk_nvme_transport_id *trid), 0);

DEFINE_STUB_V(spdk_nvmf_request_exec, (struct spdk_nvmf_request *req));

DEFINE_STUB_V(spdk_nvmf_request_exec_fabrics, (struct spdk_nvmf_request *req));

DEFINE_STUB_V(spdk_nvmf_request_zcopy_start, (struct spdk_nvmf_request *req));

DEFINE_STUB(spdk_nvmf_get_transport_name, const char *,

	    (struct spdk_nvmf_transport *transport), NULL);