Commit 7c063325 authored Jun 12, 2025 by Jacek Kalwas Committed by Konrad Sztyber Jun 27, 2025

nvme/tcp: fix nvme_tcp_qpair_process_completions error handling

When an I/O qpair fails, poll_status needs to be released — just like
for the admin qpair.

Initial fix was introduced with 1c733603 and Related to #3207 but was not
covering IO qpairs as there was no test catching the issue. However the
same leak is possible for IOs as there is no other code to handle such
negative case.

Running UTs available here https://review.spdk.io/c/spdk/spdk/+/22350


with changes

+DEFINE_STUB_V(nvme_poll_group_write_disconnect_qpair_fd, (struct spdk_nvme_poll_group *group));
-       SPDK_CU_ASSERT_FATAL(nvme_qpair_init(qpair, 0, &ctrlr, 0, 1, true) == 0);
+       SPDK_CU_ASSERT_FATAL(nvme_qpair_init(qpair, 1, &ctrlr, 0, 1, true) == 0);

allows to reproduce the issue for IO qpairs as well.

While addressing this, also simplify the handling: regardless of
the qpair type, nvme_ctrlr_disconnect_qpair can be used, since the mutex
is recursive because of that removed the comment explaining the
difference.

Change-Id: Ic53704f6d3839b2b85d2b88d73955eb97a8a60ea
Signed-off-by: Jacek Kalwas <jacek.kalwas@nutanix.com>
Reviewed-on: https://review.spdk.io/c/spdk/spdk/+/26116


Tested-by: SPDK Automated Test System <spdkbot@gmail.com>
Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@solidigm.com>
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Reviewed-by: Konrad Sztyber <ksztyber@nvidia.com>
Reviewed-by: Jim Harris <jim.harris@nvidia.com>
Reviewed-by: Tomasz Zawadzki <tomasz@tzawadzki.com>

parent 339e1517

lib/nvme/nvme_tcp.c

+7 −17

Original line number	Diff line number	Diff line
		@@ -2096,6 +2096,7 @@ static int
		nvme_tcp_qpair_process_completions(struct spdk_nvme_qpair *qpair, uint32_t max_completions)
		{
		struct nvme_tcp_qpair *tqpair = nvme_tcp_qpair(qpair);
		enum nvme_qpair_state state_prev;
		uint32_t reaped;
		int rc;

		@@ -2148,26 +2149,15 @@ nvme_tcp_qpair_process_completions(struct spdk_nvme_qpair *qpair, uint32_t max_c

		return reaped;
		fail:

		/*
		* Since admin queues take the ctrlr_lock before entering this function,
		* we can call nvme_transport_ctrlr_disconnect_qpair. For other qpairs we need
		* to call the generic function which will take the lock for us.
		*/
		state_prev = nvme_qpair_get_state(qpair);
		qpair->transport_failure_reason = SPDK_NVME_QPAIR_FAILURE_UNKNOWN;
		nvme_ctrlr_disconnect_qpair(qpair);

		if (nvme_qpair_is_admin_queue(qpair)) {
		enum nvme_qpair_state state_prev = nvme_qpair_get_state(qpair);

		nvme_transport_ctrlr_disconnect_qpair(qpair->ctrlr, qpair);

		if (state_prev == NVME_QPAIR_CONNECTING && qpair->poll_status != NULL) {
		/* Needed to free the poll_status */
		if (state_prev == NVME_QPAIR_CONNECTING && qpair->poll_status != NULL) {
		nvme_tcp_ctrlr_connect_qpair_poll(qpair->ctrlr, qpair);
		}
		} else {
		nvme_ctrlr_disconnect_qpair(qpair);
		}

		return -ENXIO;
		}