Commit 33e23361 authored by Konrad Sztyber's avatar Konrad Sztyber Committed by Tomasz Zawadzki
Browse files

nvmf: allow commands depending on qpair state 



The state of a qpair is now used to determine whether a command is
allowed to execute.  This replaces checks testing qpair->ctrlr == NULL
to see if the CONNECT command has been received.  Relying on the state
will make it easy to add checks for more states (e.g. authentication) in
the future.

Signed-off-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Change-Id: I49274f3525d4f2fc267cef32a9c68824ea5d4705
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/22589


Community-CI: Mellanox Build Bot
Reviewed-by: default avatarBen Walker <ben@nvidia.com>
Reviewed-by: default avatarJim Harris <jim.harris@samsung.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent db221b40

lib/nvmf/ctrlr.c

+42 −30
Original line number Diff line number Diff line
@@ -3662,13 +3662,7 @@ nvmf_ctrlr_process_admin_cmd(struct spdk_nvmf_request *req) 
	struct spdk_nvmf_subsystem_poll_group *sgroup;

	int rc;



	if (ctrlr == NULL) {

		SPDK_ERRLOG("Admin command sent before CONNECT\n");

		response->status.sct = SPDK_NVME_SCT_GENERIC;

		response->status.sc = SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR;

		return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

	}



	assert(ctrlr != NULL);

	if (cmd->opc == SPDK_NVME_OPC_ASYNC_EVENT_REQUEST) {

		/* We do not want to treat AERs as outstanding commands,

		 * so decrement mgmt_io_outstanding here to offset
@@ -3767,15 +3761,8 @@ nvmf_ctrlr_process_fabrics_cmd(struct spdk_nvmf_request *req) 


	if (qpair->ctrlr == NULL) {

		/* No ctrlr established yet; the only valid command is Connect */

		if (cap_hdr->fctype == SPDK_NVMF_FABRIC_COMMAND_CONNECT) {

		assert(cap_hdr->fctype == SPDK_NVMF_FABRIC_COMMAND_CONNECT);

		return nvmf_ctrlr_cmd_connect(req);

		} else {

			SPDK_DEBUGLOG(nvmf, "Got fctype 0x%x, expected Connect\n",

				      cap_hdr->fctype);

			req->rsp->nvme_cpl.status.sct = SPDK_NVME_SCT_GENERIC;

			req->rsp->nvme_cpl.status.sc = SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR;

			return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

		}

	} else if (nvmf_qpair_is_admin_queue(qpair)) {

		/*

		 * Controller session is established, and this is an admin queue.
@@ -4323,13 +4310,7 @@ nvmf_ctrlr_process_io_cmd(struct spdk_nvmf_request *req) 
	response->status.sc = SPDK_NVME_SC_SUCCESS;

	nsid = cmd->nsid;



	if (spdk_unlikely(ctrlr == NULL)) {

		SPDK_ERRLOG("I/O command sent before CONNECT\n");

		response->status.sct = SPDK_NVME_SCT_GENERIC;

		response->status.sc = SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR;

		return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

	}



	assert(ctrlr != NULL);

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

		SPDK_ERRLOG("I/O command sent to disabled controller\n");

		response->status.sct = SPDK_NVME_SCT_GENERIC;
@@ -4645,17 +4626,45 @@ nvmf_check_subsystem_active(struct spdk_nvmf_request *req) 


			ns_info->io_outstanding++;

		}

	}



	return true;

}



static bool

nvmf_check_qpair_active(struct spdk_nvmf_request *req)

{

	struct spdk_nvmf_qpair *qpair = req->qpair;



	if (spdk_likely(qpair->state == SPDK_NVMF_QPAIR_ENABLED)) {

		return true;

	}



	switch (qpair->state) {

	case SPDK_NVMF_QPAIR_CONNECTING:

		if (req->cmd->nvmf_cmd.opcode != SPDK_NVME_OPC_FABRIC) {

			SPDK_ERRLOG("Received command 0x%x on qid %u before CONNECT\n",

				    req->cmd->nvmf_cmd.opcode, qpair->qid);

			break;

		}

		if (req->cmd->nvmf_cmd.fctype != SPDK_NVMF_FABRIC_COMMAND_CONNECT) {

			SPDK_ERRLOG("Received fctype 0x%x on qid %u before CONNECT\n",

				    req->cmd->nvmf_cmd.fctype, qpair->qid);

			break;

		}

		return true;

	default:

		SPDK_ERRLOG("Received command 0x%x on qid %u in state %d\n",

			    req->cmd->nvmf_cmd.opcode, qpair->qid, qpair->state);

		break;

	}



		if (spdk_unlikely(!spdk_nvmf_qpair_is_active(qpair))) {

	req->rsp->nvme_cpl.status.sct = SPDK_NVME_SCT_GENERIC;

	req->rsp->nvme_cpl.status.sc = SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR;

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

	_nvmf_request_complete(req);

			return false;

		}

	}



	return true;

	return false;

}



void
@@ -4667,6 +4676,9 @@ spdk_nvmf_request_exec(struct spdk_nvmf_request *req) 
	if (spdk_unlikely(!nvmf_check_subsystem_active(req))) {

		return;

	}

	if (spdk_unlikely(!nvmf_check_qpair_active(req))) {

		return;

	}



	if (SPDK_DEBUGLOG_FLAG_ENABLED("nvmf")) {

		spdk_nvme_print_command(qpair->qid, &req->cmd->nvme_cmd);

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

+52 −3
Original line number Diff line number Diff line
@@ -393,11 +393,13 @@ static void 
test_process_fabrics_cmd(void)

{

	struct	spdk_nvmf_request req = {};

	int	ret;

	bool	ret;

	struct	spdk_nvmf_qpair req_qpair = {};

	union	nvmf_h2c_msg  req_cmd = {};

	union	nvmf_c2h_msg   req_rsp = {};



	TAILQ_INIT(&req_qpair.outstanding);

	req_qpair.state = SPDK_NVMF_QPAIR_CONNECTING;

	req.qpair = &req_qpair;

	req.cmd  = &req_cmd;

	req.rsp  = &req_rsp;
@@ -405,9 +407,10 @@ test_process_fabrics_cmd(void) 


	/* No ctrlr and invalid command check */

	req.cmd->nvmf_cmd.fctype = SPDK_NVMF_FABRIC_COMMAND_PROPERTY_GET;

	ret = nvmf_ctrlr_process_fabrics_cmd(&req);

	ret = nvmf_check_qpair_active(&req);

	CU_ASSERT_EQUAL(req.rsp->nvme_cpl.status.sct, SPDK_NVME_SCT_GENERIC);

	CU_ASSERT_EQUAL(req.rsp->nvme_cpl.status.sc, SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR);

	CU_ASSERT_EQUAL(ret, SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(ret == false);

}



static bool
@@ -2747,11 +2750,13 @@ test_spdk_nvmf_request_zcopy_start(void) 
	/* Fail because no controller */

	CU_ASSERT(nvmf_ctrlr_use_zcopy(&req));

	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_INIT);

	qpair.state = SPDK_NVMF_QPAIR_CONNECTING;

	qpair.ctrlr = NULL;

	spdk_nvmf_request_zcopy_start(&req);

	CU_ASSERT_EQUAL(req.zcopy_phase, NVMF_ZCOPY_PHASE_INIT_FAILED);

	CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sct, SPDK_NVME_SCT_GENERIC);

	CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sc, SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR);

	qpair.state = SPDK_NVMF_QPAIR_ENABLED;

	qpair.ctrlr = &ctrlr;

	req.zcopy_phase = NVMF_ZCOPY_PHASE_NONE;
@@ -3291,6 +3296,49 @@ test_nvmf_ctrlr_ns_attachment(void) 
	free(subsystem.ns);

}



static void

test_nvmf_check_qpair_active(void)

{

	union nvmf_c2h_msg rsp = {};

	union nvmf_h2c_msg cmd = {};

	struct spdk_nvmf_qpair qpair = { .outstanding = TAILQ_HEAD_INITIALIZER(qpair.outstanding) };

	struct spdk_nvmf_request req = { .qpair = &qpair, .cmd = &cmd, .rsp = &rsp };

	size_t i;



	/* qpair is active */

	cmd.nvme_cmd.opc = SPDK_NVME_OPC_READ;

	qpair.state = SPDK_NVMF_QPAIR_ENABLED;

	CU_ASSERT_EQUAL(nvmf_check_qpair_active(&req), true);



	/* qpair is connecting - CONNECT is allowed */

	cmd.nvmf_cmd.opcode = SPDK_NVME_OPC_FABRIC;

	cmd.nvmf_cmd.fctype = SPDK_NVMF_FABRIC_COMMAND_CONNECT;

	qpair.state = SPDK_NVMF_QPAIR_CONNECTING;

	CU_ASSERT_EQUAL(nvmf_check_qpair_active(&req), true);



	/* qpair is connecting - other commands are disallowed */

	cmd.nvme_cmd.opc = SPDK_NVME_OPC_READ;

	qpair.state = SPDK_NVMF_QPAIR_CONNECTING;

	CU_ASSERT_EQUAL(nvmf_check_qpair_active(&req), false);

	CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sct, SPDK_NVME_SCT_GENERIC);

	CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sc, SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR);



	/* qpair is in one of the other states - all commands are disallowed */

	int disallowed_states[] = {

		SPDK_NVMF_QPAIR_UNINITIALIZED,

		SPDK_NVMF_QPAIR_DEACTIVATING,

		SPDK_NVMF_QPAIR_ERROR,

	};

	qpair.state_cb = qpair_state_change_done;

	cmd.nvme_cmd.opc = SPDK_NVME_OPC_READ;

	for (i = 0; i < SPDK_COUNTOF(disallowed_states); ++i) {

		qpair.state = disallowed_states[i];

		CU_ASSERT_EQUAL(nvmf_check_qpair_active(&req), false);

		CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sct, SPDK_NVME_SCT_GENERIC);

		CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sc, SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR);

	}

}



int

main(int argc, char **argv)

{
@@ -3331,6 +3379,7 @@ main(int argc, char **argv) 
	CU_ADD_TEST(suite, test_nvmf_ctrlr_get_features_host_behavior_support);

	CU_ADD_TEST(suite, test_nvmf_ctrlr_set_features_host_behavior_support);

	CU_ADD_TEST(suite, test_nvmf_ctrlr_ns_attachment);

	CU_ADD_TEST(suite, test_nvmf_check_qpair_active);



	allocate_threads(1);

	set_thread(0);