From f2b22d68d65f86ff9d7d2d2a6d700dbd3b2b017d Mon Sep 17 00:00:00 2001 From: Seth Howell Date: Fri, 13 Jul 2018 16:23:23 -0700 Subject: [PATCH] subsystem: defer channel iter until pg functions return The poll group pause, resume, remove, and add functions are only called from the subsystem_state_change_on_pg function. Previously, they would return immediately and the state change would move on to the next channel. However, some of these functions (specifically remove) kick off asynchronous APIs and we should not iterate past them until those asynchronous operations complete. Change-Id: I78804273b39f2d171ba26ac4478ad515356833f3 Signed-off-by: Seth Howell Reviewed-on: https://review.gerrithub.io/419289 Reviewed-by: Jim Harris Reviewed-by: Changpeng Liu Reviewed-by: Shuhei Matsumoto Reviewed-by: Ben Walker Tested-by: SPDK CI Jenkins Chandler-Test-Pool: SPDK Automated Test System --- lib/nvmf/nvmf.c | 109 +++++++++++------- lib/nvmf/nvmf_internal.h | 20 ++-- lib/nvmf/subsystem.c | 18 +-- .../ctrlr_discovery.c/ctrlr_discovery_ut.c | 24 ++-- test/unit/lib/nvmf/subsystem.c/subsystem_ut.c | 24 ++-- 5 files changed, 116 insertions(+), 79 deletions(-) diff --git a/lib/nvmf/nvmf.c b/lib/nvmf/nvmf.c index 912b486fe..7a2d3ba11 100644 --- a/lib/nvmf/nvmf.c +++ b/lib/nvmf/nvmf.c @@ -75,7 +75,8 @@ struct nvmf_qpair_disconnect_ctx { struct nvmf_qpair_disconnect_many_ctx { struct spdk_nvmf_subsystem *subsystem; struct spdk_nvmf_poll_group *group; - nvmf_qpair_disconnect_cpl cb_fn; + spdk_nvmf_poll_group_mod_done cpl_fn; + void *cpl_ctx; }; void @@ -137,7 +138,7 @@ spdk_nvmf_tgt_create_poll_group(void *io_device, void *ctx_buf) continue; } - spdk_nvmf_poll_group_add_subsystem(group, subsystem); + spdk_nvmf_poll_group_add_subsystem(group, subsystem, NULL, NULL); } group->poller = spdk_poller_register(spdk_nvmf_poll_group_poll, group, 0); @@ -854,36 +855,48 @@ spdk_nvmf_poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, return poll_group_update_subsystem(group, subsystem); } -int +void spdk_nvmf_poll_group_add_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { struct spdk_nvmf_subsystem_poll_group *sgroup; - int rc; + int rc = 0; rc = poll_group_update_subsystem(group, subsystem); if (rc) { - return rc; + goto fini; } sgroup = &group->sgroups[subsystem->id]; sgroup->state = SPDK_NVMF_SUBSYSTEM_ACTIVE; TAILQ_INIT(&sgroup->queued); - - return 0; +fini: + if (cb_fn) { + cb_fn(cb_arg, rc); + } } static void _nvmf_poll_group_remove_subsystem_cb(void *ctx, int status) { + struct nvmf_qpair_disconnect_many_ctx *qpair_ctx = ctx; + struct spdk_nvmf_subsystem *subsystem; + struct spdk_nvmf_poll_group *group; struct spdk_nvmf_subsystem_poll_group *sgroup; + spdk_nvmf_poll_group_mod_done cpl_fn = NULL; + void *cpl_ctx = NULL; uint32_t nsid; - if (status) { - return; - } + group = qpair_ctx->group; + subsystem = qpair_ctx->subsystem; + cpl_fn = qpair_ctx->cpl_fn; + cpl_ctx = qpair_ctx->cpl_ctx; + sgroup = &group->sgroups[subsystem->id]; - sgroup = ctx; + if (status) { + goto fini; + } for (nsid = 0; nsid < sgroup->num_channels; nsid++) { if (sgroup->channels[nsid]) { @@ -895,6 +908,11 @@ _nvmf_poll_group_remove_subsystem_cb(void *ctx, int status) sgroup->num_channels = 0; free(sgroup->channels); sgroup->channels = NULL; +fini: + free(qpair_ctx); + if (cpl_fn) { + cpl_fn(cpl_ctx, status); + } } static void @@ -904,7 +922,6 @@ _nvmf_subsystem_disconnect_next_qpair(void *ctx) struct nvmf_qpair_disconnect_many_ctx *qpair_ctx = ctx; struct spdk_nvmf_subsystem *subsystem; struct spdk_nvmf_poll_group *group; - struct spdk_nvmf_subsystem_poll_group *sgroup; int rc = 0; group = qpair_ctx->group; @@ -921,19 +938,15 @@ _nvmf_subsystem_disconnect_next_qpair(void *ctx) } if (!qpair || rc != 0) { - if (qpair_ctx->cb_fn) { - sgroup = &group->sgroups[subsystem->id]; - qpair_ctx->cb_fn(sgroup, rc); - } - free(qpair_ctx); - return; + _nvmf_poll_group_remove_subsystem_cb(ctx, rc); } return; } -int +void spdk_nvmf_poll_group_remove_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { struct spdk_nvmf_qpair *qpair; struct spdk_nvmf_subsystem_poll_group *sgroup; @@ -943,12 +956,14 @@ spdk_nvmf_poll_group_remove_subsystem(struct spdk_nvmf_poll_group *group, ctx = calloc(1, sizeof(struct nvmf_qpair_disconnect_many_ctx)); if (!ctx) { - return -ENOMEM; + SPDK_ERRLOG("Unable to allocate memory for context to remove poll subsystem\n"); + goto fini; } - ctx->cb_fn = _nvmf_poll_group_remove_subsystem_cb; ctx->group = group; ctx->subsystem = subsystem; + ctx->cpl_fn = cb_fn; + ctx->cpl_ctx = cb_arg; sgroup = &group->sgroups[subsystem->id]; sgroup->state = SPDK_NVMF_SUBSYSTEM_INACTIVE; @@ -962,50 +977,62 @@ spdk_nvmf_poll_group_remove_subsystem(struct spdk_nvmf_poll_group *group, if (qpair) { rc = spdk_nvmf_qpair_disconnect(qpair, _nvmf_subsystem_disconnect_next_qpair, ctx); } else { - free(ctx); - _nvmf_poll_group_remove_subsystem_cb(sgroup, 0); + /* call the callback immediately. It will handle any channel iteration */ + _nvmf_poll_group_remove_subsystem_cb(ctx, 0); } if (rc != 0) { free(ctx); - return rc; + goto fini; } - return 0; + return; +fini: + if (cb_fn) { + cb_fn(cb_arg, rc); + } } -int +void spdk_nvmf_poll_group_pause_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { struct spdk_nvmf_subsystem_poll_group *sgroup; + int rc = 0; if (subsystem->id >= group->num_sgroups) { - return -1; + rc = -1; + goto fini; } sgroup = &group->sgroups[subsystem->id]; if (sgroup == NULL) { - return -1; + rc = -1; + goto fini; } assert(sgroup->state == SPDK_NVMF_SUBSYSTEM_ACTIVE); /* TODO: This currently does not quiesce I/O */ sgroup->state = SPDK_NVMF_SUBSYSTEM_PAUSED; - - return 0; +fini: + if (cb_fn) { + cb_fn(cb_arg, rc); + } } -int +void spdk_nvmf_poll_group_resume_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { struct spdk_nvmf_request *req, *tmp; struct spdk_nvmf_subsystem_poll_group *sgroup; - int rc; + int rc = 0; if (subsystem->id >= group->num_sgroups) { - return -1; + rc = -1; + goto fini; } sgroup = &group->sgroups[subsystem->id]; @@ -1014,7 +1041,7 @@ spdk_nvmf_poll_group_resume_subsystem(struct spdk_nvmf_poll_group *group, rc = poll_group_update_subsystem(group, subsystem); if (rc) { - return rc; + goto fini; } sgroup->state = SPDK_NVMF_SUBSYSTEM_ACTIVE; @@ -1024,6 +1051,8 @@ spdk_nvmf_poll_group_resume_subsystem(struct spdk_nvmf_poll_group *group, TAILQ_REMOVE(&sgroup->queued, req, link); spdk_nvmf_request_exec(req); } - - return 0; +fini: + if (cb_fn) { + cb_fn(cb_arg, rc); + } } diff --git a/lib/nvmf/nvmf_internal.h b/lib/nvmf/nvmf_internal.h index 1618cec4e..b7980e26b 100644 --- a/lib/nvmf/nvmf_internal.h +++ b/lib/nvmf/nvmf_internal.h @@ -42,6 +42,7 @@ #include "spdk/assert.h" #include "spdk/queue.h" #include "spdk/util.h" +#include "spdk/thread.h" #define SPDK_NVMF_MAX_SGL_ENTRIES 16 @@ -252,6 +253,8 @@ struct spdk_nvmf_subsystem { TAILQ_ENTRY(spdk_nvmf_subsystem) entries; }; +typedef void(*spdk_nvmf_poll_group_mod_done)(void *cb_arg, int status); + struct spdk_nvmf_transport *spdk_nvmf_tgt_get_transport(struct spdk_nvmf_tgt *tgt, enum spdk_nvme_transport_type); @@ -259,14 +262,15 @@ int spdk_nvmf_poll_group_add_transport(struct spdk_nvmf_poll_group *group, struct spdk_nvmf_transport *transport); int spdk_nvmf_poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, struct spdk_nvmf_subsystem *subsystem); -int spdk_nvmf_poll_group_add_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem); -int spdk_nvmf_poll_group_remove_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem); -int spdk_nvmf_poll_group_pause_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem); -int spdk_nvmf_poll_group_resume_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem); +void spdk_nvmf_poll_group_add_subsystem(struct spdk_nvmf_poll_group *group, + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg); +void spdk_nvmf_poll_group_remove_subsystem(struct spdk_nvmf_poll_group *group, + struct spdk_nvmf_subsystem *subsystem, spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg); +void spdk_nvmf_poll_group_pause_subsystem(struct spdk_nvmf_poll_group *group, + struct spdk_nvmf_subsystem *subsystem, spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg); +void spdk_nvmf_poll_group_resume_subsystem(struct spdk_nvmf_poll_group *group, + struct spdk_nvmf_subsystem *subsystem, spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg); void spdk_nvmf_request_exec(struct spdk_nvmf_request *req); int spdk_nvmf_request_free(struct spdk_nvmf_request *req); int spdk_nvmf_request_complete(struct spdk_nvmf_request *req); diff --git a/lib/nvmf/subsystem.c b/lib/nvmf/subsystem.c index 408bad96b..4ab8d7621 100644 --- a/lib/nvmf/subsystem.c +++ b/lib/nvmf/subsystem.c @@ -422,13 +422,19 @@ subsystem_state_change_done(struct spdk_io_channel_iter *i, int status) free(ctx); } +static void +subsystem_state_change_continue(void *ctx, int status) +{ + struct spdk_io_channel_iter *i = ctx; + spdk_for_each_channel_continue(i, status); +} + static void subsystem_state_change_on_pg(struct spdk_io_channel_iter *i) { struct subsystem_state_change_ctx *ctx; struct spdk_io_channel *ch; struct spdk_nvmf_poll_group *group; - int rc = -1; ctx = spdk_io_channel_iter_get_ctx(i); ch = spdk_io_channel_iter_get_channel(i); @@ -436,24 +442,22 @@ subsystem_state_change_on_pg(struct spdk_io_channel_iter *i) switch (ctx->requested_state) { case SPDK_NVMF_SUBSYSTEM_INACTIVE: - rc = spdk_nvmf_poll_group_remove_subsystem(group, ctx->subsystem); + spdk_nvmf_poll_group_remove_subsystem(group, ctx->subsystem, subsystem_state_change_continue, i); break; case SPDK_NVMF_SUBSYSTEM_ACTIVE: if (ctx->subsystem->state == SPDK_NVMF_SUBSYSTEM_ACTIVATING) { - rc = spdk_nvmf_poll_group_add_subsystem(group, ctx->subsystem); + spdk_nvmf_poll_group_add_subsystem(group, ctx->subsystem, subsystem_state_change_continue, i); } else if (ctx->subsystem->state == SPDK_NVMF_SUBSYSTEM_RESUMING) { - rc = spdk_nvmf_poll_group_resume_subsystem(group, ctx->subsystem); + spdk_nvmf_poll_group_resume_subsystem(group, ctx->subsystem, subsystem_state_change_continue, i); } break; case SPDK_NVMF_SUBSYSTEM_PAUSED: - rc = spdk_nvmf_poll_group_pause_subsystem(group, ctx->subsystem); + spdk_nvmf_poll_group_pause_subsystem(group, ctx->subsystem, subsystem_state_change_continue, i); break; default: assert(false); break; } - - spdk_for_each_channel_continue(i, rc); } static int diff --git a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c index 97633a80b..3eb829795 100644 --- a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c +++ b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c @@ -170,32 +170,32 @@ spdk_nvmf_poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, return 0; } -int +void spdk_nvmf_poll_group_add_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { - return 0; } -int +void spdk_nvmf_poll_group_remove_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { - return 0; } -int +void spdk_nvmf_poll_group_pause_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { - return 0; } -int +void spdk_nvmf_poll_group_resume_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { - return 0; } static void diff --git a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c index a08621974..c0a9778ec 100644 --- a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c +++ b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c @@ -126,32 +126,32 @@ spdk_nvmf_poll_group_update_subsystem(struct spdk_nvmf_poll_group *group, return 0; } -int +void spdk_nvmf_poll_group_add_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { - return 0; } -int +void spdk_nvmf_poll_group_remove_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { - return 0; } -int +void spdk_nvmf_poll_group_pause_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { - return 0; } -int +void spdk_nvmf_poll_group_resume_subsystem(struct spdk_nvmf_poll_group *group, - struct spdk_nvmf_subsystem *subsystem) + struct spdk_nvmf_subsystem *subsystem, + spdk_nvmf_poll_group_mod_done cb_fn, void *cb_arg) { - return 0; } int