Commit c2471e45 authored by Alexey Marchuk's avatar Alexey Marchuk Committed by Jim Harris
Browse files

nvmf: Clean unassociated_qpairs on connect error 



When spdk_nvmf_poll_group_add returns an error,
qpair is in uninitialized state and spdk_nvmf_qpair_disconnect
handles this state in a special way, i.e. we don't decrement
the `group->current_unassociated_qpairs` counter. That is not
a critical error but may lead to uneven qpairs distribution.

Signed-off-by: default avatarAlexey Marchuk <alexeymar@nvidia.com>
Change-Id: If68c4c4c8f3a99a690ba15694b5568940a7e0c21
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/25012


Community-CI: Community CI Samsung <spdk.community.ci.samsung@gmail.com>
Reviewed-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarJim Harris <jim.harris@nvidia.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent 5469bd2d

lib/nvmf/ctrlr.c

+1 −0
Original line number Diff line number Diff line
@@ -910,6 +910,7 @@ _nvmf_ctrlr_connect(struct spdk_nvmf_request *req) 
	qpair->connect_received = true;



	pthread_mutex_lock(&qpair->group->mutex);

	assert(qpair->group->current_unassociated_qpairs > 0);

	qpair->group->current_unassociated_qpairs--;

	pthread_mutex_unlock(&qpair->group->mutex);

lib/nvmf/nvmf.c

+8 −0
Original line number Diff line number Diff line
@@ -1127,6 +1127,13 @@ _nvmf_poll_group_add(void *_ctx) 


	if (spdk_nvmf_poll_group_add(group, qpair) != 0) {

		SPDK_ERRLOG("Unable to add the qpair to a poll group.\n");



		assert(qpair->state == SPDK_NVMF_QPAIR_UNINITIALIZED);

		pthread_mutex_lock(&group->mutex);

		assert(group->current_unassociated_qpairs > 0);

		group->current_unassociated_qpairs--;

		pthread_mutex_unlock(&group->mutex);



		spdk_nvmf_qpair_disconnect(qpair);

	}

}
@@ -1340,6 +1347,7 @@ _nvmf_qpair_destroy(void *ctx, int status) 
		}

	} else {

		pthread_mutex_lock(&qpair->group->mutex);

		assert(qpair->group->current_unassociated_qpairs > 0);

		qpair->group->current_unassociated_qpairs--;

		pthread_mutex_unlock(&qpair->group->mutex);

	}

test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c

+24 −0
Original line number Diff line number Diff line
@@ -544,6 +544,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -563,6 +564,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -581,6 +583,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	req.length = sizeof(connect_data) - 1;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);
@@ -593,6 +596,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	cmd.connect_cmd.recfmt = 1234;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);
@@ -605,6 +609,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	MOCK_SET(spdk_nvmf_tgt_find_subsystem, NULL);

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);
@@ -619,6 +624,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	memset(connect_data.hostnqn, 'b', sizeof(connect_data.hostnqn));

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);
@@ -633,6 +639,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	MOCK_SET(spdk_nvmf_subsystem_host_allowed, false);

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);
@@ -645,6 +652,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	cmd.connect_cmd.sqsize = 0;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);
@@ -659,6 +667,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	cmd.connect_cmd.sqsize = 32;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);
@@ -674,6 +683,7 @@ test_connect(void) 
	cmd.connect_cmd.qid = 1;

	cmd.connect_cmd.sqsize = 64;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);
@@ -689,6 +699,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	connect_data.cntlid = 0x1234;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);
@@ -709,6 +720,7 @@ test_connect(void) 
	cmd.connect_cmd.sqsize = 63;

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -725,6 +737,7 @@ test_connect(void) 
	MOCK_SET(nvmf_subsystem_get_ctrlr, NULL);

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -743,6 +756,7 @@ test_connect(void) 
	subsystem.state = SPDK_NVMF_SUBSYSTEM_ACTIVE;

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -762,6 +776,7 @@ test_connect(void) 
	subsystem.state = SPDK_NVMF_SUBSYSTEM_ACTIVE;

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -787,6 +802,7 @@ test_connect(void) 
	subsystem.state = SPDK_NVMF_SUBSYSTEM_ACTIVE;

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -811,6 +827,7 @@ test_connect(void) 
	ctrlr.vcprop.cc.bits.en = 0;

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -827,6 +844,7 @@ test_connect(void) 
	ctrlr.vcprop.cc.bits.iosqes = 3;

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -843,6 +861,7 @@ test_connect(void) 
	ctrlr.vcprop.cc.bits.iocqes = 3;

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -859,6 +878,7 @@ test_connect(void) 
	cmd.connect_cmd.qid = 3;

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -873,6 +893,7 @@ test_connect(void) 
	cmd.connect_cmd.qid = 1;

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);

	poll_threads();
@@ -896,6 +917,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);

	poll_threads();
@@ -922,6 +944,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rsp.nvme_cpl.status.sct == SPDK_NVME_SCT_COMMAND_SPECIFIC);
@@ -936,6 +959,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	sgroups[subsystem.id].mgmt_io_outstanding++;

	TAILQ_INSERT_TAIL(&qpair.outstanding, &req, link);

	group.current_unassociated_qpairs = 1;

	rc = nvmf_ctrlr_cmd_connect(&req);

	poll_threads();

	CU_ASSERT(rsp.nvme_cpl.status.sct == SPDK_NVME_SCT_COMMAND_SPECIFIC);