From f08cea7169474a4c019de977e5ff379039bd35ac Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Fri, 1 Jun 2018 15:00:20 -0700 Subject: [PATCH] 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: Ben Walker Reviewed-on: https://review.gerrithub.io/413286 Tested-by: SPDK Automated Test System Reviewed-by: Changpeng Liu Reviewed-by: Daniel Verkamp --- lib/nvmf/ctrlr.c | 35 ++++++++++++++++++--------- lib/nvmf/nvmf_internal.h | 4 ++- test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c | 8 +++++- 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/lib/nvmf/ctrlr.c b/lib/nvmf/ctrlr.c index d5b33f559..bbc0591a9 100644 --- a/lib/nvmf/ctrlr.c +++ b/lib/nvmf/ctrlr.c @@ -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); - 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) { + 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 (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; diff --git a/lib/nvmf/nvmf_internal.h b/lib/nvmf/nvmf_internal.h index e33003486..26398d1cb 100644 --- a/lib/nvmf/nvmf_internal.h +++ b/lib/nvmf/nvmf_internal.h @@ -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]; diff --git a/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c b/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c index e27a05e7d..0b6ee6ba6 100644 --- a/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c +++ b/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c @@ -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