Commit f08cea71 authored by Ben Walker's avatar Ben Walker Committed by Jim Harris
Browse files

nvmf: Perform QID validation using a bit mask 



Instead of scanning a list of all qpairs, use a bit
mask to determine if the requested QID is unique.

This is not for performance reasons, but because
eventually the ctrlr's list of qpairs is going to
need to go away.

Change-Id: Ic25ee60e4f9cd9d596815719760d5be892f29d0c
Signed-off-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-on: https://review.gerrithub.io/413286


Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatarChangpeng Liu <changpeng.liu@intel.com>
Reviewed-by: default avatarDaniel Verkamp <daniel.verkamp@intel.com>
parent 5eb33f0a

lib/nvmf/ctrlr.c

+19 −8
Original line number Diff line number Diff line
@@ -36,6 +36,7 @@ 
#include "nvmf_internal.h"

#include "transport.h"



#include "spdk/bit_array.h"

#include "spdk/endian.h"

#include "spdk/io_channel.h"

#include "spdk/trace.h"
@@ -76,24 +77,24 @@ ctrlr_add_qpair_and_update_rsp(struct spdk_nvmf_qpair *qpair, 
			       struct spdk_nvmf_ctrlr *ctrlr,

			       struct spdk_nvmf_fabric_connect_rsp *rsp)

{

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

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

	if (qpair->qid >= spdk_bit_array_capacity(ctrlr->qpair_mask)) {

		SPDK_ERRLOG("qpair limit %d\n", ctrlr->num_qpairs);

		rsp->status.sct = SPDK_NVME_SCT_COMMAND_SPECIFIC;

		rsp->status.sc = SPDK_NVME_SC_INVALID_QUEUE_IDENTIFIER;

		return;

	}



	/* check if we would exceed ctrlr connection limit */

	if (ctrlr->num_qpairs >= ctrlr->max_qpairs_allowed) {

		SPDK_ERRLOG("qpair limit %d\n", ctrlr->num_qpairs);

	if (spdk_bit_array_get(ctrlr->qpair_mask, qpair->qid)) {

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

		rsp->status.sct = SPDK_NVME_SCT_COMMAND_SPECIFIC;

		rsp->status.sc = SPDK_NVME_SC_INVALID_QUEUE_IDENTIFIER;

		return;

	}





	qpair->ctrlr = ctrlr;

	ctrlr->num_qpairs++;

	spdk_bit_array_set(ctrlr->qpair_mask, qpair->qid);

	TAILQ_INSERT_HEAD(&ctrlr->qpairs, qpair, link);



	rsp->status.sc = SPDK_NVME_SC_SUCCESS;
@@ -164,15 +165,21 @@ spdk_nvmf_ctrlr_create(struct spdk_nvmf_subsystem *subsystem, 
	TAILQ_INIT(&ctrlr->qpairs);

	ctrlr->num_qpairs = 0;

	ctrlr->subsys = subsystem;

	ctrlr->max_qpairs_allowed = tgt->opts.max_qpairs_per_ctrlr;



	ctrlr->qpair_mask = spdk_bit_array_create(tgt->opts.max_qpairs_per_ctrlr);

	if (!ctrlr->qpair_mask) {

		SPDK_ERRLOG("Failed to allocate controller qpair mask\n");

		free(ctrlr);

		return NULL;

	}



	ctrlr->feat.keep_alive_timer.bits.kato = connect_cmd->kato;

	ctrlr->feat.async_event_configuration.bits.ns_attr_notice = 1;

	ctrlr->feat.volatile_write_cache.bits.wce = 1;



	/* Subtract 1 for admin queue, 1 for 0's based */

	ctrlr->feat.number_of_queues.bits.ncqr = ctrlr->max_qpairs_allowed - 1 - 1;

	ctrlr->feat.number_of_queues.bits.nsqr = ctrlr->max_qpairs_allowed - 1 - 1;

	ctrlr->feat.number_of_queues.bits.ncqr = tgt->opts.max_qpairs_per_ctrlr - 1 - 1;

	ctrlr->feat.number_of_queues.bits.nsqr = tgt->opts.max_qpairs_per_ctrlr - 1 - 1;



	memcpy(ctrlr->hostid, connect_data->hostid, sizeof(ctrlr->hostid));
@@ -215,9 +222,12 @@ spdk_nvmf_ctrlr_destruct(struct spdk_nvmf_ctrlr *ctrlr) 


		TAILQ_REMOVE(&ctrlr->qpairs, qpair, link);

		ctrlr->num_qpairs--;

		spdk_bit_array_clear(ctrlr->qpair_mask, qpair->qid);

		spdk_nvmf_transport_qpair_fini(qpair);

	}



	spdk_bit_array_free(&ctrlr->qpair_mask);



	spdk_nvmf_subsystem_remove_ctrlr(ctrlr->subsys, ctrlr);



	free(ctrlr);
@@ -443,6 +453,7 @@ _spdk_nvmf_ctrlr_remove_qpair(void *ctx) 


	TAILQ_REMOVE(&ctrlr->qpairs, qpair, link);

	ctrlr->num_qpairs--;

	spdk_bit_array_clear(ctrlr->qpair_mask, qpair->qid);



	/* Send a message to the thread that owns the qpair and destroy it. */

	qpair->ctrlr = NULL;

lib/nvmf/nvmf_internal.h

+3 −1
Original line number Diff line number Diff line
@@ -201,9 +201,11 @@ struct spdk_nvmf_ctrlr { 
	struct spdk_nvmf_ctrlr_feat feat;



	struct spdk_nvmf_qpair *admin_qpair;



	TAILQ_HEAD(, spdk_nvmf_qpair) qpairs;

	int num_qpairs;

	int max_qpairs_allowed;

	struct spdk_bit_array *qpair_mask;



	struct spdk_nvmf_request *aer_req;

	union spdk_nvme_async_event_completion notice_event;

	uint8_t hostid[16];

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

+7 −1
Original line number Diff line number Diff line
@@ -36,6 +36,7 @@ 
#include "spdk_cunit.h"

#include "spdk_internal/mock.h"



#include "common/lib/test_env.c"

#include "nvmf/ctrlr.c"



SPDK_LOG_REGISTER_COMPONENT("nvmf", SPDK_LOG_NVMF)
@@ -279,10 +280,11 @@ test_connect(void) 
	memset(&ctrlr, 0, sizeof(ctrlr));

	TAILQ_INIT(&ctrlr.qpairs);

	ctrlr.subsys = &subsystem;

	ctrlr.qpair_mask = spdk_bit_array_create(3);

	SPDK_CU_ASSERT_FATAL(ctrlr.qpair_mask != NULL);

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

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

	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);
@@ -339,6 +341,7 @@ test_connect(void) 
	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);

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

	CU_ASSERT(qpair.ctrlr != NULL);

	spdk_bit_array_free(&qpair.ctrlr->qpair_mask);

	free(qpair.ctrlr);

	qpair.ctrlr = NULL;
@@ -535,6 +538,7 @@ test_connect(void) 
	qpair2.group = &group;

	qpair2.qid = 1;

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

	spdk_bit_array_set(ctrlr.qpair_mask, 1);

	cmd.connect_cmd.qid = 1;

	rc = spdk_nvmf_ctrlr_connect(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS);
@@ -546,6 +550,8 @@ test_connect(void) 
	/* Clean up globals */

	MOCK_SET(spdk_nvmf_tgt_find_subsystem, struct spdk_nvmf_subsystem *, NULL);

	MOCK_SET(spdk_nvmf_poll_group_create, struct spdk_nvmf_poll_group *, NULL);



	spdk_bit_array_free(&ctrlr.qpair_mask);

}



static void