Commit 30ef8cac authored by Ziye Yang's avatar Ziye Yang Committed by Jim Harris
Browse files

nvmf: Solve the coredump issue when client conduct `nvme disconnect` 

This patch is used to solve
https://github.com/spdk/spdk/issues/235



With multiple core bining for NVMe-oF target,
the qpairs which belonging to the same ctrlr may
be scheduled to different cores. Thus there is
resource contention to access the struct spdk_nvmf_ctrlr.

And we put the thread info in polling group. Morever,
we introduce an admin_qpair in ctrlr. Since admin_qpair will
always be created at first and freed at last, to reference
this pointer is safe.

Change-Id: I12ac26f9e65b4ed8e48687750046455af0e3be1d
Signed-off-by: default avatarZiye Yang <optimistyzy@gmail.com>
Reviewed-on: https://review.gerrithub.io/398904


Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatarDaniel Verkamp <daniel.verkamp@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent 2982a74d

lib/nvmf/ctrlr.c

+26 −9
Original line number Diff line number Diff line
@@ -167,6 +167,14 @@ ctrlr_add_qpair_and_update_rsp(struct spdk_nvmf_qpair *qpair, 
		      rsp->status_code_specific.success.cntlid);

}



static void

_spdk_nvmf_request_complete(void *ctx)

{

	struct spdk_nvmf_request *req = ctx;



	spdk_nvmf_request_complete(req);

}



static void

spdk_nvmf_ctrlr_add_io_qpair(void *ctx)

{
@@ -184,34 +192,34 @@ spdk_nvmf_ctrlr_add_io_qpair(void *ctx) 
	if (ctrlr->subsys->subtype == SPDK_NVMF_SUBTYPE_DISCOVERY) {

		SPDK_ERRLOG("I/O connect not allowed on discovery controller\n");

		SPDK_NVMF_INVALID_CONNECT_CMD(rsp, qid);

		return;

		goto end;

	}



	if (!ctrlr->vcprop.cc.bits.en) {

		SPDK_ERRLOG("Got I/O connect before ctrlr was enabled\n");

		SPDK_NVMF_INVALID_CONNECT_CMD(rsp, qid);

		return;

		goto end;

	}



	if (1u << ctrlr->vcprop.cc.bits.iosqes != sizeof(struct spdk_nvme_cmd)) {

		SPDK_ERRLOG("Got I/O connect with invalid IOSQES %u\n",

			    ctrlr->vcprop.cc.bits.iosqes);

		SPDK_NVMF_INVALID_CONNECT_CMD(rsp, qid);

		return;

		goto end;

	}



	if (1u << ctrlr->vcprop.cc.bits.iocqes != sizeof(struct spdk_nvme_cpl)) {

		SPDK_ERRLOG("Got I/O connect with invalid IOCQES %u\n",

			    ctrlr->vcprop.cc.bits.iocqes);

		SPDK_NVMF_INVALID_CONNECT_CMD(rsp, qid);

		return;

		goto end;

	}



	if (spdk_nvmf_ctrlr_get_qpair(ctrlr, cmd->qid)) {

		SPDK_ERRLOG("Got I/O connect with duplicate QID %u\n", cmd->qid);

		rsp->status.sct = SPDK_NVME_SCT_GENERIC;

		rsp->status.sc = SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR;

		return;

		goto end;

	}



	/* check if we would exceed ctrlr connection limit */
@@ -219,10 +227,13 @@ spdk_nvmf_ctrlr_add_io_qpair(void *ctx) 
		SPDK_ERRLOG("qpair limit %d\n", ctrlr->num_qpairs);

		rsp->status.sct = SPDK_NVME_SCT_COMMAND_SPECIFIC;

		rsp->status.sc = SPDK_NVMF_FABRIC_SC_CONTROLLER_BUSY;

		return;

		goto end;

	}



	ctrlr_add_qpair_and_update_rsp(qpair, ctrlr, rsp);



end:

	spdk_thread_send_msg(qpair->group->thread, _spdk_nvmf_request_complete, req);

}



static void
@@ -251,6 +262,7 @@ spdk_nvmf_ctrlr_connect(struct spdk_nvmf_request *req) 
	struct spdk_nvmf_qpair *qpair = req->qpair;

	struct spdk_nvmf_tgt *tgt = qpair->transport->tgt;

	struct spdk_nvmf_ctrlr *ctrlr;

	struct spdk_nvmf_qpair *admin_qpair = NULL;

	struct spdk_nvmf_subsystem *subsystem;

	const char *subnqn, *hostnqn;

	void *end;
@@ -347,6 +359,7 @@ spdk_nvmf_ctrlr_connect(struct spdk_nvmf_request *req) 
			return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

		}



		ctrlr->admin_qpair = qpair;

		ctrlr_add_qpair_and_update_rsp(qpair, ctrlr, rsp);

		return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

	} else {
@@ -359,16 +372,20 @@ spdk_nvmf_ctrlr_connect(struct spdk_nvmf_request *req) 
			return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

		}



		admin_qpair = ctrlr->admin_qpair;

		qpair->ctrlr = ctrlr;

		spdk_nvmf_ctrlr_add_io_qpair(req);

		return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

		spdk_thread_send_msg(admin_qpair->group->thread, spdk_nvmf_ctrlr_add_io_qpair, req);

		return SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS;

	}

}



void

spdk_nvmf_ctrlr_disconnect(struct spdk_nvmf_qpair *qpair)

{

	ctrlr_delete_qpair(qpair);

	struct spdk_nvmf_ctrlr *ctrlr = qpair->ctrlr;

	struct spdk_nvmf_qpair *admin_qpair = ctrlr->admin_qpair;



	spdk_thread_send_msg(admin_qpair->group->thread, ctrlr_delete_qpair, qpair);

}



struct spdk_nvmf_qpair *

lib/nvmf/nvmf.c

+1 −0
Original line number Diff line number Diff line
@@ -109,6 +109,7 @@ spdk_nvmf_tgt_create_poll_group(void *io_device, void *ctx_buf) 
	}



	group->poller = spdk_poller_register(spdk_nvmf_poll_group_poll, group, 0);

	group->thread = spdk_get_thread();



	return 0;

}

lib/nvmf/nvmf_internal.h

+2 −0
Original line number Diff line number Diff line
@@ -94,6 +94,7 @@ struct spdk_nvmf_subsystem_poll_group { 
};



struct spdk_nvmf_poll_group {

	struct spdk_thread				*thread;

	struct spdk_poller				*poller;



	TAILQ_HEAD(, spdk_nvmf_transport_poll_group)	tgroups;
@@ -168,6 +169,7 @@ struct spdk_nvmf_ctrlr { 
		union spdk_nvme_csts_register	csts;

	} vcprop; /* virtual controller properties */



	struct spdk_nvmf_qpair *admin_qpair;

	TAILQ_HEAD(, spdk_nvmf_qpair) qpairs;

	int num_qpairs;

	int max_qpairs_allowed;

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

+23 −7
Original line number Diff line number Diff line
@@ -223,6 +223,12 @@ nvme_status_success(const struct spdk_nvme_status *status) 
	return status->sct == SPDK_NVME_SCT_GENERIC && status->sc == SPDK_NVME_SC_SUCCESS;

}



void

spdk_thread_send_msg(const struct spdk_thread *thread, spdk_thread_fn fn, void *ctx)

{

	fn(ctx);

}



static void

test_connect(void)

{
@@ -231,6 +237,7 @@ test_connect(void) 
	struct spdk_nvmf_transport transport;

	struct spdk_nvmf_subsystem subsystem;

	struct spdk_nvmf_request req;

	struct spdk_nvmf_qpair admin_qpair;

	struct spdk_nvmf_qpair qpair;

	struct spdk_nvmf_qpair qpair2;

	struct spdk_nvmf_ctrlr ctrlr;
@@ -255,6 +262,10 @@ test_connect(void) 
	ctrlr.vcprop.cc.bits.iocqes = 4;

	ctrlr.max_qpairs_allowed = 3;



	memset(&admin_qpair, 0, sizeof(admin_qpair));

	TAILQ_INSERT_TAIL(&ctrlr.qpairs, &admin_qpair, link);

	admin_qpair.group = &group;



	memset(&tgt, 0, sizeof(tgt));

	tgt.opts.max_queue_depth = 64;

	tgt.opts.max_qpairs_per_ctrlr = 3;
@@ -264,6 +275,7 @@ test_connect(void) 


	memset(&qpair, 0, sizeof(qpair));

	qpair.transport = &transport;

	qpair.group = &group;



	memset(&connect_data, 0, sizeof(connect_data));

	memcpy(connect_data.hostid, hostid, sizeof(hostid));
@@ -410,12 +422,15 @@ test_connect(void) 
	CU_ASSERT(qpair.ctrlr == NULL);

	connect_data.cntlid = 0xFFFF;



	ctrlr.admin_qpair = &admin_qpair;

	ctrlr.subsys = &subsystem;



	/* Valid I/O queue connect command */

	memset(&rsp, 0, sizeof(rsp));

	MOCK_SET(spdk_nvmf_subsystem_get_ctrlr, struct spdk_nvmf_ctrlr *, &ctrlr);

	cmd.connect_cmd.qid = 1;

	rc = spdk_nvmf_ctrlr_connect(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);

	CU_ASSERT(nvme_status_success(&rsp.nvme_cpl.status));

	CU_ASSERT(qpair.ctrlr == &ctrlr);

	qpair.ctrlr = NULL;
@@ -438,7 +453,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	subsystem.subtype = SPDK_NVMF_SUBTYPE_DISCOVERY;

	rc = spdk_nvmf_ctrlr_connect(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);

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

	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVMF_FABRIC_SC_INVALID_PARAM);

	CU_ASSERT(rsp.connect_rsp.status_code_specific.invalid.iattr == 0);
@@ -450,7 +465,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	ctrlr.vcprop.cc.bits.en = 0;

	rc = spdk_nvmf_ctrlr_connect(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);

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

	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVMF_FABRIC_SC_INVALID_PARAM);

	CU_ASSERT(rsp.connect_rsp.status_code_specific.invalid.iattr == 0);
@@ -462,7 +477,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	ctrlr.vcprop.cc.bits.iosqes = 3;

	rc = spdk_nvmf_ctrlr_connect(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);

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

	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVMF_FABRIC_SC_INVALID_PARAM);

	CU_ASSERT(rsp.connect_rsp.status_code_specific.invalid.iattr == 0);
@@ -474,7 +489,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	ctrlr.vcprop.cc.bits.iocqes = 3;

	rc = spdk_nvmf_ctrlr_connect(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);

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

	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVMF_FABRIC_SC_INVALID_PARAM);

	CU_ASSERT(rsp.connect_rsp.status_code_specific.invalid.iattr == 0);
@@ -486,7 +501,7 @@ test_connect(void) 
	memset(&rsp, 0, sizeof(rsp));

	ctrlr.num_qpairs = 3;

	rc = spdk_nvmf_ctrlr_connect(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);

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

	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVMF_FABRIC_SC_CONTROLLER_BUSY);

	CU_ASSERT(qpair.ctrlr == NULL);
@@ -495,11 +510,12 @@ test_connect(void) 
	/* I/O connect with duplicate queue ID */

	memset(&rsp, 0, sizeof(rsp));

	memset(&qpair2, 0, sizeof(qpair2));

	qpair2.group = &group;

	qpair2.qid = 1;

	TAILQ_INSERT_TAIL(&ctrlr.qpairs, &qpair, link);

	cmd.connect_cmd.qid = 1;

	rc = spdk_nvmf_ctrlr_connect(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);

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

	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR);

	CU_ASSERT(qpair.ctrlr == NULL);