From 7346be69e76dcc2636cce6d28d2220d8e66c2570 Mon Sep 17 00:00:00 2001 From: Ziye Yang Date: Tue, 27 Feb 2018 12:54:59 +0800 Subject: [PATCH] nvmf: Make the ctrlr create/remove in subsystem in an asynchronous way Ctrlrs list maintanined by the subsystem structure should be operated by the thread which creates the subsystem. And this will make the operations correct. Change-Id: I7f881a77b1846658b3acd4270b74f86816e87803 Signed-off-by: Ziye Yang Reviewed-on: https://review.gerrithub.io/401541 Tested-by: SPDK Automated Test System Reviewed-by: Daniel Verkamp Reviewed-by: Jim Harris --- lib/nvmf/ctrlr.c | 101 +++++++++++++++++++------- lib/nvmf/nvmf_internal.h | 1 + lib/nvmf/subsystem.c | 1 + test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c | 4 +- 4 files changed, 79 insertions(+), 28 deletions(-) diff --git a/lib/nvmf/ctrlr.c b/lib/nvmf/ctrlr.c index 13b11a732..15a5540a9 100644 --- a/lib/nvmf/ctrlr.c +++ b/lib/nvmf/ctrlr.c @@ -94,9 +94,42 @@ _spdk_nvmf_request_complete(void *ctx) spdk_nvmf_request_complete(req); } +static void +_spdk_nvmf_ctrlr_add_admin_qpair(void *ctx) +{ + struct spdk_nvmf_request *req = ctx; + struct spdk_nvmf_fabric_connect_rsp *rsp = &req->rsp->connect_rsp; + struct spdk_nvmf_qpair *qpair = req->qpair; + struct spdk_nvmf_ctrlr *ctrlr = qpair->ctrlr; + + ctrlr->admin_qpair = qpair; + ctrlr_add_qpair_and_update_rsp(qpair, ctrlr, rsp); + spdk_nvmf_request_complete(req); +} + +static void +_spdk_nvmf_subsystem_add_ctrlr(void *ctx) +{ + struct spdk_nvmf_request *req = ctx; + struct spdk_nvmf_qpair *qpair = req->qpair; + struct spdk_nvmf_fabric_connect_rsp *rsp = &req->rsp->connect_rsp; + struct spdk_nvmf_ctrlr *ctrlr = qpair->ctrlr; + + if (spdk_nvmf_subsystem_add_ctrlr(ctrlr->subsys, ctrlr)) { + SPDK_ERRLOG("Unable to add controller to subsystem\n"); + free(ctrlr); + qpair->ctrlr = NULL; + rsp->status.sc = SPDK_NVME_SC_INTERNAL_DEVICE_ERROR; + spdk_thread_send_msg(qpair->group->thread, _spdk_nvmf_request_complete, req); + return; + } + + spdk_thread_send_msg(qpair->group->thread, _spdk_nvmf_ctrlr_add_admin_qpair, req); +} + static struct spdk_nvmf_ctrlr * spdk_nvmf_ctrlr_create(struct spdk_nvmf_subsystem *subsystem, - struct spdk_nvmf_qpair *admin_qpair, + struct spdk_nvmf_request *req, struct spdk_nvmf_fabric_connect_cmd *connect_cmd, struct spdk_nvmf_fabric_connect_data *connect_data) { @@ -111,6 +144,7 @@ spdk_nvmf_ctrlr_create(struct spdk_nvmf_subsystem *subsystem, return NULL; } + req->qpair->ctrlr = ctrlr; TAILQ_INIT(&ctrlr->qpairs); ctrlr->kato = connect_cmd->kato; ctrlr->async_event_config.raw = 0; @@ -146,17 +180,15 @@ spdk_nvmf_ctrlr_create(struct spdk_nvmf_subsystem *subsystem, SPDK_DEBUGLOG(SPDK_LOG_NVMF, "cc 0x%x\n", ctrlr->vcprop.cc.raw); SPDK_DEBUGLOG(SPDK_LOG_NVMF, "csts 0x%x\n", ctrlr->vcprop.csts.raw); - if (spdk_nvmf_subsystem_add_ctrlr(subsystem, ctrlr)) { - SPDK_ERRLOG("Unable to add controller to subsystem\n"); - free(ctrlr); - return NULL; - } + spdk_thread_send_msg(subsystem->thread, _spdk_nvmf_subsystem_add_ctrlr, req); return ctrlr; } -static void ctrlr_destruct(struct spdk_nvmf_ctrlr *ctrlr) +static void ctrlr_destruct(void *ctx) { + struct spdk_nvmf_ctrlr *ctrlr = ctx; + spdk_nvmf_subsystem_remove_ctrlr(ctrlr->subsys, ctrlr); free(ctrlr); } @@ -249,10 +281,41 @@ ctrlr_delete_qpair(void *ctx) spdk_nvmf_transport_qpair_fini(qpair); if (ctrlr->num_qpairs == 0) { - ctrlr_destruct(ctrlr); + spdk_thread_send_msg(ctrlr->subsys->thread, ctrlr_destruct, ctrlr); } } +static void +_spdk_nvmf_ctrlr_add_io_qpair(void *ctx) +{ + struct spdk_nvmf_request *req = ctx; + struct spdk_nvmf_fabric_connect_rsp *rsp = &req->rsp->connect_rsp; + struct spdk_nvmf_fabric_connect_data *data = req->data; + struct spdk_nvmf_ctrlr *ctrlr; + struct spdk_nvmf_qpair *qpair = req->qpair; + struct spdk_nvmf_qpair *admin_qpair; + struct spdk_nvmf_tgt *tgt = qpair->transport->tgt; + struct spdk_nvmf_subsystem *subsystem; + + SPDK_DEBUGLOG(SPDK_LOG_NVMF, "Connect I/O Queue for controller id 0x%x\n", data->cntlid); + + subsystem = spdk_nvmf_tgt_find_subsystem(tgt, data->subnqn); + /* We already checked this in spdk_nvmf_ctrlr_connect */ + assert(subsystem != NULL); + + ctrlr = spdk_nvmf_subsystem_get_ctrlr(subsystem, data->cntlid); + if (ctrlr == NULL) { + SPDK_ERRLOG("Unknown controller ID 0x%x\n", data->cntlid); + SPDK_NVMF_INVALID_CONNECT_DATA(rsp, cntlid); + spdk_thread_send_msg(qpair->group->thread, _spdk_nvmf_request_complete, req); + return; + } + + admin_qpair = ctrlr->admin_qpair; + qpair->ctrlr = ctrlr; + spdk_thread_send_msg(admin_qpair->group->thread, spdk_nvmf_ctrlr_add_io_qpair, req); +} + static int spdk_nvmf_ctrlr_connect(struct spdk_nvmf_request *req) { @@ -262,7 +325,6 @@ 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; @@ -352,29 +414,16 @@ spdk_nvmf_ctrlr_connect(struct spdk_nvmf_request *req) } /* Establish a new ctrlr */ - ctrlr = spdk_nvmf_ctrlr_create(subsystem, qpair, cmd, data); + ctrlr = spdk_nvmf_ctrlr_create(subsystem, req, cmd, data); if (!ctrlr) { SPDK_ERRLOG("spdk_nvmf_ctrlr_create() failed\n"); rsp->status.sc = SPDK_NVME_SC_INTERNAL_DEVICE_ERROR; return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; + } else { + return SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS; } - - ctrlr->admin_qpair = qpair; - ctrlr_add_qpair_and_update_rsp(qpair, ctrlr, rsp); - return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; } else { - SPDK_DEBUGLOG(SPDK_LOG_NVMF, "Connect I/O Queue for controller id 0x%x\n", data->cntlid); - - ctrlr = spdk_nvmf_subsystem_get_ctrlr(subsystem, data->cntlid); - if (ctrlr == NULL) { - SPDK_ERRLOG("Unknown controller ID 0x%x\n", data->cntlid); - SPDK_NVMF_INVALID_CONNECT_DATA(rsp, cntlid); - return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; - } - - admin_qpair = ctrlr->admin_qpair; - qpair->ctrlr = ctrlr; - spdk_thread_send_msg(admin_qpair->group->thread, spdk_nvmf_ctrlr_add_io_qpair, req); + spdk_thread_send_msg(subsystem->thread, _spdk_nvmf_ctrlr_add_io_qpair, req); return SPDK_NVMF_REQUEST_EXEC_STATUS_ASYNCHRONOUS; } } diff --git a/lib/nvmf/nvmf_internal.h b/lib/nvmf/nvmf_internal.h index 74b04dab9..7eec61245 100644 --- a/lib/nvmf/nvmf_internal.h +++ b/lib/nvmf/nvmf_internal.h @@ -181,6 +181,7 @@ struct spdk_nvmf_ctrlr { }; struct spdk_nvmf_subsystem { + struct spdk_thread *thread; uint32_t id; enum spdk_nvmf_subsystem_state state; diff --git a/lib/nvmf/subsystem.c b/lib/nvmf/subsystem.c index b77ae22ec..4a1ebecb5 100644 --- a/lib/nvmf/subsystem.c +++ b/lib/nvmf/subsystem.c @@ -255,6 +255,7 @@ spdk_nvmf_subsystem_create(struct spdk_nvmf_tgt *tgt, return NULL; } + subsystem->thread = spdk_get_thread(); subsystem->state = SPDK_NVMF_SUBSYSTEM_INACTIVE; subsystem->tgt = tgt; subsystem->id = sid; diff --git a/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c b/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c index 2c385293f..7ef87deaf 100644 --- a/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c +++ b/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c @@ -314,7 +314,7 @@ test_connect(void) /* Valid admin connect command */ memset(&rsp, 0, sizeof(rsp)); 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 != NULL); free(qpair.ctrlr); @@ -441,7 +441,7 @@ test_connect(void) memset(&rsp, 0, sizeof(rsp)); MOCK_SET(spdk_nvmf_subsystem_get_ctrlr, struct spdk_nvmf_ctrlr *, NULL); 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 == 1);