Commit f634e8b0 authored by Jacek Kalwas's avatar Jacek Kalwas Committed by Tomasz Zawadzki
Browse files

nvme/tcp: fix icreq timeout tracking 



This issue was introduced with the sock async connect feature, as
the icreq is sent to the socket but queued and flushed only after
a successful TCP connection. Therefore, setting the icreq timeout
beforehand is incorrect.

Moved icreq send to sock connect callback so behavior is restored.

Fabric connect poll is deferred now because conceptually there is no
reason to invoke it before sock connection is established.

Change-Id: I24fe850f558f8813d643562d86fd0ec96101149c
Signed-off-by: default avatarJacek Kalwas <jacek.kalwas@nutanix.com>
Reviewed-on: https://review.spdk.io/c/spdk/spdk/+/26113


Reviewed-by: default avatarKonrad Sztyber <ksztyber@nvidia.com>
Tested-by: default avatarSPDK Automated Test System <spdkbot@gmail.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarTomasz Zawadzki <tomasz@tzawadzki.com>
parent 6b4317e4

lib/nvme/nvme_tcp.c

+22 −16
Original line number Diff line number Diff line
@@ -43,13 +43,14 @@ 


enum nvme_tcp_qpair_state {

	NVME_TCP_QPAIR_STATE_INVALID = 0,

	NVME_TCP_QPAIR_STATE_INITIALIZING = 1,

	NVME_TCP_QPAIR_STATE_FABRIC_CONNECT_SEND = 2,

	NVME_TCP_QPAIR_STATE_FABRIC_CONNECT_POLL = 3,

	NVME_TCP_QPAIR_STATE_AUTHENTICATING = 4,

	NVME_TCP_QPAIR_STATE_RUNNING = 5,

	NVME_TCP_QPAIR_STATE_EXITING = 6,

	NVME_TCP_QPAIR_STATE_EXITED = 7,

	NVME_TCP_QPAIR_STATE_SOCK_CONNECTING = 1,

	NVME_TCP_QPAIR_STATE_INITIALIZING = 2,

	NVME_TCP_QPAIR_STATE_FABRIC_CONNECT_SEND = 3,

	NVME_TCP_QPAIR_STATE_FABRIC_CONNECT_POLL = 4,

	NVME_TCP_QPAIR_STATE_AUTHENTICATING = 5,

	NVME_TCP_QPAIR_STATE_RUNNING = 6,

	NVME_TCP_QPAIR_STATE_EXITING = 7,

	NVME_TCP_QPAIR_STATE_EXITED = 8,

};



/* NVMe TCP transport extensions for spdk_nvme_ctrlr */
@@ -95,8 +96,9 @@ struct nvme_tcp_qpair { 
		uint16_t host_hdgst_enable: 1;

		uint16_t host_ddgst_enable: 1;

		uint16_t icreq_send_ack: 1;

		uint16_t icresp_received: 1;

		uint16_t in_connect_poll: 1;

		uint16_t reserved: 12;

		uint16_t reserved: 11;

	} flags;



	/** Specifies the maximum number of PDU-Data bytes per H2C Data Transfer PDU */
@@ -1149,7 +1151,7 @@ nvme_tcp_pdu_ch_handle(struct nvme_tcp_qpair *tqpair) 


	SPDK_DEBUGLOG(nvme, "pdu type = %d\n", pdu->hdr.common.pdu_type);

	if (pdu->hdr.common.pdu_type == SPDK_NVME_TCP_PDU_TYPE_IC_RESP) {

		if (tqpair->state != NVME_TCP_QPAIR_STATE_INVALID) {

		if (tqpair->flags.icresp_received) {

			SPDK_ERRLOG("Already received IC_RESP PDU, and we should reject this pdu=%p\n", pdu);

			fes = SPDK_NVME_TCP_TERM_REQ_FES_PDU_SEQUENCE_ERROR;

			goto err;
@@ -1475,8 +1477,7 @@ nvme_tcp_send_icreq_complete(void *cb_arg) 
	SPDK_DEBUGLOG(nvme, "Complete the icreq send for tqpair=%p %u\n", tqpair, tqpair->qpair.id);



	tqpair->flags.icreq_send_ack = true;



	if (tqpair->state == NVME_TCP_QPAIR_STATE_INITIALIZING) {

	if (tqpair->flags.icresp_received) {

		SPDK_DEBUGLOG(nvme, "tqpair %p %u, finalize icresp\n", tqpair, tqpair->qpair.id);

		tqpair->state = NVME_TCP_QPAIR_STATE_FABRIC_CONNECT_SEND;

	}
@@ -1543,8 +1544,8 @@ nvme_tcp_icresp_handle(struct nvme_tcp_qpair *tqpair, 


	nvme_tcp_qpair_set_recv_state(tqpair, NVME_TCP_PDU_RECV_STATE_AWAIT_PDU_READY);



	tqpair->flags.icresp_received = true;

	if (!tqpair->flags.icreq_send_ack) {

		tqpair->state = NVME_TCP_QPAIR_STATE_INITIALIZING;

		SPDK_DEBUGLOG(nvme, "tqpair %p %u, waiting icreq ack\n", tqpair, tqpair->qpair.id);

		return;

	}
@@ -2217,7 +2218,11 @@ nvme_tcp_sock_connect_cb_fn(void *cb_arg, int status) 
	if (status < 0) {

		SPDK_ERRLOG("sock connection error of tqpair=%p with %d (%s)\n", tqpair, status,

			    spdk_strerror(abs(status)));

		return;

	}



	tqpair->state = NVME_TCP_QPAIR_STATE_INITIALIZING;

	nvme_tcp_qpair_icreq_send(tqpair);

}



static int
@@ -2302,6 +2307,7 @@ nvme_tcp_qpair_connect_sock(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_qpai 
		opts.impl_opts_size = sizeof(impl_opts);

	}



	tqpair->state = NVME_TCP_QPAIR_STATE_SOCK_CONNECTING;

	tqpair->sock = spdk_sock_connect_async(ctrlr->trid.traddr, port, sock_impl_name, &opts,

					       nvme_tcp_sock_connect_cb_fn, tqpair);

	if (!tqpair->sock) {
@@ -2333,7 +2339,9 @@ nvme_tcp_ctrlr_connect_qpair_poll(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvm 
	tqpair->flags.in_connect_poll = 1;



	switch (tqpair->state) {

	case NVME_TCP_QPAIR_STATE_INVALID:

	case NVME_TCP_QPAIR_STATE_SOCK_CONNECTING:

		rc = -EAGAIN;

		break;

	case NVME_TCP_QPAIR_STATE_INITIALIZING:

		if (spdk_get_ticks() > tqpair->icreq_timeout_tsc) {

			SPDK_ERRLOG("Failed to construct the tqpair=%p via correct icresp\n", tqpair);
@@ -2429,13 +2437,11 @@ nvme_tcp_ctrlr_connect_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_qpa 
	}



	tqpair->maxr2t = NVME_TCP_MAX_R2T_DEFAULT;

	/* Explicitly set the state and recv_state of tqpair */

	tqpair->state = NVME_TCP_QPAIR_STATE_INVALID;

	/* Explicitly set recv_state of tqpair */

	if (tqpair->recv_state != NVME_TCP_PDU_RECV_STATE_AWAIT_PDU_READY) {

		nvme_tcp_qpair_set_recv_state(tqpair, NVME_TCP_PDU_RECV_STATE_AWAIT_PDU_READY);

	}



	nvme_tcp_qpair_icreq_send(tqpair);

	return rc;

}

test/unit/lib/nvme/nvme_tcp.c/nvme_tcp_ut.c

+9 −5
Original line number Diff line number Diff line
@@ -923,7 +923,7 @@ test_nvme_tcp_pdu_ch_handle(void) 


	/* case 2: Expected PDU header length and received are different. Expect: fail */

	tqpair.recv_pdu->hdr.common.pdu_type = SPDK_NVME_TCP_PDU_TYPE_IC_RESP;

	tqpair.state = NVME_TCP_QPAIR_STATE_INVALID;

	tqpair.state = NVME_TCP_QPAIR_STATE_SOCK_CONNECTING;

	tqpair.recv_pdu->hdr.common.plen = sizeof(struct spdk_nvme_tcp_ic_resp);

	tqpair.recv_pdu->hdr.common.hlen = 0;

	nvme_tcp_pdu_ch_handle(&tqpair);
@@ -935,7 +935,7 @@ test_nvme_tcp_pdu_ch_handle(void) 


	/* case 3: The TCP/IP tqpair connection is not negotiated. Expect: fail */

	tqpair.recv_pdu->hdr.common.pdu_type = SPDK_NVME_TCP_PDU_TYPE_CAPSULE_RESP;

	tqpair.state = NVME_TCP_QPAIR_STATE_INVALID;

	tqpair.state = NVME_TCP_QPAIR_STATE_SOCK_CONNECTING;

	tqpair.recv_pdu->hdr.common.plen = sizeof(struct spdk_nvme_tcp_ic_resp);

	tqpair.recv_pdu->hdr.common.hlen = 0;

	nvme_tcp_pdu_ch_handle(&tqpair);
@@ -958,7 +958,7 @@ test_nvme_tcp_pdu_ch_handle(void) 


	/* case 5: plen error. Expect: fail */

	tqpair.recv_pdu->hdr.common.pdu_type = SPDK_NVME_TCP_PDU_TYPE_IC_RESP;

	tqpair.state = NVME_TCP_QPAIR_STATE_INVALID;

	tqpair.state = NVME_TCP_QPAIR_STATE_SOCK_CONNECTING;

	tqpair.recv_pdu->hdr.common.plen = 0;

	tqpair.recv_pdu->hdr.common.hlen = sizeof(struct spdk_nvme_tcp_ic_resp);

	nvme_tcp_pdu_ch_handle(&tqpair);
@@ -1022,7 +1022,7 @@ test_nvme_tcp_pdu_ch_handle(void) 


	/* case 6: Expect:  PASS */

	tqpair.recv_pdu->hdr.common.pdu_type = SPDK_NVME_TCP_PDU_TYPE_IC_RESP;

	tqpair.state = NVME_TCP_QPAIR_STATE_INVALID;

	tqpair.state = NVME_TCP_QPAIR_STATE_SOCK_CONNECTING;

	tqpair.recv_pdu->hdr.common.plen = sizeof(struct spdk_nvme_tcp_ic_resp);

	tqpair.recv_pdu->hdr.common.hlen = sizeof(struct spdk_nvme_tcp_ic_resp);

	nvme_tcp_pdu_ch_handle(&tqpair);
@@ -1103,7 +1103,7 @@ test_nvme_tcp_qpair_icreq_send(void) 
	tqpair.stats = &stats;

	ic_req = &pdu.hdr.ic_req;



	tqpair.state = NVME_TCP_QPAIR_STATE_RUNNING;

	tqpair.state = NVME_TCP_QPAIR_STATE_SOCK_CONNECTING;

	tqpair.qpair.ctrlr->opts.header_digest = true;

	tqpair.qpair.ctrlr->opts.data_digest = true;

	TAILQ_INIT(&tqpair.send_queue);
@@ -1224,6 +1224,7 @@ test_nvme_tcp_icresp_handle(void) 
	tqpair.send_pdu = &send_pdu;

	tqpair.recv_pdu = &recv_pdu;

	tqpair.stats = &stats;

	tqpair.state = NVME_TCP_QPAIR_STATE_INITIALIZING;

	TAILQ_INIT(&tqpair.send_queue);



	/* case 1: Expected ICResp PFV and got are different. */
@@ -1406,6 +1407,7 @@ test_nvme_tcp_ctrlr_connect_qpair(void) 
	tqpair->send_pdu = &pdu;

	tqpair->qpair.ctrlr = &ctrlr;

	tqpair->qpair.state = NVME_QPAIR_CONNECTING;

	tqpair->state = NVME_TCP_QPAIR_STATE_SOCK_CONNECTING;

	tqpair->num_entries = 128;

	ic_req = &pdu.hdr.ic_req;
@@ -1424,6 +1426,8 @@ test_nvme_tcp_ctrlr_connect_qpair(void) 
	rc = nvme_tcp_ctrlr_connect_qpair(&ctrlr, qpair);

	CU_ASSERT(rc == 0);



	/* assume sock connection established */

	nvme_tcp_sock_connect_cb_fn(tqpair, 0);

	tqpair->flags.icreq_send_ack = 1;



	/* skip NVME_TCP_PDU_RECV_STATE_AWAIT_PDU_READY state */