From fce942877211653815bbc9c918b464127d0ed5ce Mon Sep 17 00:00:00 2001 From: Madhu Adav MJ Date: Mon, 26 Oct 2020 15:38:32 +0530 Subject: [PATCH] nvmf: Async event support for discovery log change Added asynchronous event notices for discovery log change as per nvme fabrics spec 1.1. This allows a host with persistent connection to discovery controller to automatically connect to any new subsystem available to the host automatically. According to nvme fabrics spec 1.1, if the connect command specifies a non-zero keep alive timer value and the discovery controller does not support asynchronous events then we need to return Connect Invalid. Since SPDK does not implement this check instead added support for asynchronous events in discovery controller. Change-Id: I4cade5f7d24826ce97a2fa2b4ca688a1d728c1db Signed-off-by: Madhu Adav MJ Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/4870 Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Michael Haeuptle Reviewed-by: Aleksey Marchuk Reviewed-by: Anil Veerabhadrappa Reviewed-by: Jim Harris Community-CI: Broadcom CI --- CHANGELOG.md | 6 + include/spdk/nvme_spec.h | 17 ++- lib/nvmf/ctrlr.c | 107 ++++++++++++++++-- lib/nvmf/ctrlr_discovery.c | 19 ++++ lib/nvmf/nvmf_internal.h | 2 + lib/nvmf/subsystem.c | 9 +- .../ctrlr_discovery.c/ctrlr_discovery_ut.c | 4 + test/unit/lib/nvmf/fc.c/fc_ut.c | 3 + test/unit/lib/nvmf/subsystem.c/subsystem_ut.c | 3 + 9 files changed, 155 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90730f963..37e67e258 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## v21.01: (Upcoming Release) +### nvmf + +The SPDK nvmf target now supports async event notification for discovery log changes. +This allows the initiator to create persistent connection to discovery controller and +be notified of any discovery log changes. + ## v20.10: ### accel diff --git a/include/spdk/nvme_spec.h b/include/spdk/nvme_spec.h index c4d7d1948..c48b30170 100644 --- a/include/spdk/nvme_spec.h +++ b/include/spdk/nvme_spec.h @@ -716,7 +716,9 @@ union spdk_nvme_feat_async_event_configuration { uint32_t fw_activation_notice : 1; uint32_t telemetry_log_notice : 1; uint32_t ana_change_notice : 1; - uint32_t reserved : 20; + uint32_t reserved : 19; + /** Discovery log change (refer to the NVMe over Fabrics specification) */ + uint32_t discovery_log_change_notice : 1; } bits; }; SPDK_STATIC_ASSERT(sizeof(union spdk_nvme_feat_async_event_configuration) == 4, "Incorrect size"); @@ -1680,7 +1682,11 @@ struct __attribute__((packed)) __attribute__((aligned)) spdk_nvme_ctrlr_data { /** Supports Asymmetric Namespace Access Change Notices. */ uint32_t ana_change_notices : 1; - uint32_t reserved3 : 20; + uint32_t reserved3 : 19; + + /** Supports Discovery log change notices (refer to the NVMe over Fabrics specification) */ + uint32_t discovery_log_change_notices : 1; + } oaes; /** controller attributes */ @@ -2799,7 +2805,12 @@ enum spdk_nvme_async_event_info_notice { /* Asymmetric Namespace Access Change */ SPDK_NVME_ASYNC_EVENT_ANA_CHANGE = 0x3, - /* 0x4 - 0xFF Reserved */ + /* 0x4 - 0xEF Reserved */ + + /** Discovery log change event(refer to the NVMe over Fabrics specification) */ + SPDK_NVME_ASYNC_EVENT_DISCOVERY_LOG_CHANGE = 0xF0, + + /* 0xF1 - 0xFF Reserved */ }; /** diff --git a/lib/nvmf/ctrlr.c b/lib/nvmf/ctrlr.c index 9f462b0ad..3a49b595f 100644 --- a/lib/nvmf/ctrlr.c +++ b/lib/nvmf/ctrlr.c @@ -363,14 +363,26 @@ nvmf_ctrlr_create(struct spdk_nvmf_subsystem *subsystem, * "The Keep Alive command is reserved for * Discovery controllers. A transport may specify a * fixed Discovery controller activity timeout value - * (e.g., 2 minutes). If no commands are received + * (e.g., 2 minutes). If no commands are received * by a Discovery controller within that time * period, the controller may perform the * actions for Keep Alive Timer expiration". - * kato is in millisecond. + * + * From the 1.1 nvme-of spec: + * "A host requests an explicit persistent connection + * to a Discovery controller and Asynchronous Event Notifications from + * the Discovery controller on that persistent connection by specifying + * a non-zero Keep Alive Timer value in the Connect command." + * + * In case non-zero KATO is used, we enable discovery_log_change_notice + * otherwise we disable it and use default discovery controller KATO. + * KATO is in millisecond. */ if (ctrlr->feat.keep_alive_timer.bits.kato == 0) { ctrlr->feat.keep_alive_timer.bits.kato = NVMF_DISC_KATO_IN_MS; + ctrlr->feat.async_event_configuration.bits.discovery_log_change_notice = 0; + } else { + ctrlr->feat.async_event_configuration.bits.discovery_log_change_notice = 1; } } @@ -2073,10 +2085,16 @@ spdk_nvmf_ctrlr_identify_ctrlr(struct spdk_nvmf_ctrlr *ctrlr, struct spdk_nvme_c SPDK_DEBUGLOG(nvmf, "ctrlr data: maxcmd 0x%x\n", cdata->maxcmd); SPDK_DEBUGLOG(nvmf, "sgls data: 0x%x\n", from_le32(&cdata->sgls)); - /* - * NVM subsystem fields (reserved for discovery subsystems) - */ - if (subsystem->subtype == SPDK_NVMF_SUBTYPE_NVME) { + + if (subsystem->subtype == SPDK_NVMF_SUBTYPE_DISCOVERY) { + /* + * NVM Discovery subsystem fields + */ + cdata->oaes.discovery_log_change_notices = 1; + } else { + /* + * NVM subsystem fields (reserved for discovery subsystems) + */ spdk_strcpy_pad(cdata->mn, spdk_nvmf_subsystem_get_mn(subsystem), sizeof(cdata->mn), ' '); spdk_strcpy_pad(cdata->sn, spdk_nvmf_subsystem_get_sn(subsystem), sizeof(cdata->sn), ' '); cdata->kas = ctrlr->cdata.kas; @@ -2458,6 +2476,24 @@ nvmf_ctrlr_get_features(struct spdk_nvmf_request *req) feature = cmd->cdw10_bits.get_features.fid; + if (ctrlr->subsys->subtype == SPDK_NVMF_SUBTYPE_DISCOVERY) { + /* + * Features supported by Discovery controller + */ + switch (feature) { + case SPDK_NVME_FEAT_KEEP_ALIVE_TIMER: + return get_features_generic(req, ctrlr->feat.keep_alive_timer.raw); + case SPDK_NVME_FEAT_ASYNC_EVENT_CONFIGURATION: + return get_features_generic(req, ctrlr->feat.async_event_configuration.raw); + default: + SPDK_ERRLOG("Get Features command with unsupported feature ID 0x%02x\n", feature); + response->status.sc = SPDK_NVME_SC_INVALID_FIELD; + return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; + } + } + /* + * Process Get Features command for non-discovery controller + */ ana_state = ctrlr->listener->ana_state; switch (ana_state) { case SPDK_NVME_ANA_INACCESSIBLE_STATE: @@ -2532,6 +2568,24 @@ nvmf_ctrlr_set_features(struct spdk_nvmf_request *req) feature = cmd->cdw10_bits.set_features.fid; + if (ctrlr->subsys->subtype == SPDK_NVMF_SUBTYPE_DISCOVERY) { + /* + * Features supported by Discovery controller + */ + switch (feature) { + case SPDK_NVME_FEAT_KEEP_ALIVE_TIMER: + return nvmf_ctrlr_set_features_keep_alive_timer(req); + case SPDK_NVME_FEAT_ASYNC_EVENT_CONFIGURATION: + return nvmf_ctrlr_set_features_async_event_configuration(req); + default: + SPDK_ERRLOG("Set Features command with unsupported feature ID 0x%02x\n", feature); + response->status.sc = SPDK_NVME_SC_INVALID_FIELD; + return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; + } + } + /* + * Process Set Features command for non-discovery controller + */ ana_state = ctrlr->listener->ana_state; switch (ana_state) { case SPDK_NVME_ANA_INACCESSIBLE_STATE: @@ -2640,11 +2694,14 @@ nvmf_ctrlr_process_admin_cmd(struct spdk_nvmf_request *req) } if (ctrlr->subsys->subtype == SPDK_NVMF_SUBTYPE_DISCOVERY) { - /* Discovery controllers only support Get Log Page, Identify and Keep Alive. */ + /* Discovery controllers only support these admin OPS. */ switch (cmd->opc) { case SPDK_NVME_OPC_IDENTIFY: case SPDK_NVME_OPC_GET_LOG_PAGE: case SPDK_NVME_OPC_KEEP_ALIVE: + case SPDK_NVME_OPC_SET_FEATURES: + case SPDK_NVME_OPC_GET_FEATURES: + case SPDK_NVME_OPC_ASYNC_EVENT_REQUEST: break; default: goto invalid_opcode; @@ -2806,7 +2863,7 @@ nvmf_ctrlr_async_event_ana_change_notice(struct spdk_nvmf_ctrlr *ctrlr) event.bits.log_page_identifier = SPDK_NVME_LOG_ASYMMETRIC_NAMESPACE_ACCESS; /* If there is no outstanding AER request, queue the event. Then - * if an AER is later submited, this event can be sent as a + * if an AER is later submitted, this event can be sent as a * response. */ if (ctrlr->nr_aer_reqs == 0) { @@ -2851,6 +2908,40 @@ nvmf_ctrlr_async_event_reservation_notification(struct spdk_nvmf_ctrlr *ctrlr) nvmf_ctrlr_async_event_notification(ctrlr, &event); } +int +nvmf_ctrlr_async_event_discovery_log_change_notice(struct spdk_nvmf_ctrlr *ctrlr) +{ + union spdk_nvme_async_event_completion event = {0}; + + /* Users may disable the event notification manually or + * it may not be enabled due to keep alive timeout + * not being set in connect command to discovery controller. + */ + if (!ctrlr->feat.async_event_configuration.bits.discovery_log_change_notice) { + return 0; + } + + event.bits.async_event_type = SPDK_NVME_ASYNC_EVENT_TYPE_NOTICE; + event.bits.async_event_info = SPDK_NVME_ASYNC_EVENT_DISCOVERY_LOG_CHANGE; + event.bits.log_page_identifier = SPDK_NVME_LOG_DISCOVERY; + + /* If there is no outstanding AER request, queue the event. Then + * if an AER is later submitted, this event can be sent as a + * response. + */ + if (ctrlr->nr_aer_reqs == 0) { + if (ctrlr->notice_event.bits.async_event_type == + SPDK_NVME_ASYNC_EVENT_TYPE_NOTICE) { + return 0; + } + + ctrlr->notice_event.raw = event.raw; + return 0; + } + + return nvmf_ctrlr_async_event_notification(ctrlr, &event); +} + void nvmf_qpair_free_aer(struct spdk_nvmf_qpair *qpair) { diff --git a/lib/nvmf/ctrlr_discovery.c b/lib/nvmf/ctrlr_discovery.c index bb978941d..7e7be6b63 100644 --- a/lib/nvmf/ctrlr_discovery.c +++ b/lib/nvmf/ctrlr_discovery.c @@ -47,6 +47,25 @@ #include "spdk/bdev_module.h" #include "spdk/log.h" +void +nvmf_update_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn) +{ + struct spdk_nvmf_subsystem *discovery_subsystem; + struct spdk_nvmf_ctrlr *ctrlr; + + tgt->discovery_genctr++; + discovery_subsystem = spdk_nvmf_tgt_find_subsystem(tgt, SPDK_NVMF_DISCOVERY_NQN); + + if (discovery_subsystem) { + /** There is a change in discovery log for hosts with given hostnqn */ + TAILQ_FOREACH(ctrlr, &discovery_subsystem->ctrlrs, link) { + if (hostnqn == NULL || strcmp(hostnqn, ctrlr->hostnqn) == 0) { + nvmf_ctrlr_async_event_discovery_log_change_notice(ctrlr); + } + } + } +} + static struct spdk_nvmf_discovery_log_page * nvmf_generate_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn, size_t *log_page_size) { diff --git a/lib/nvmf/nvmf_internal.h b/lib/nvmf/nvmf_internal.h index f20ae924e..3b1b02f8d 100644 --- a/lib/nvmf/nvmf_internal.h +++ b/lib/nvmf/nvmf_internal.h @@ -305,6 +305,7 @@ void nvmf_poll_group_pause_subsystem(struct spdk_nvmf_poll_group *group, void 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 nvmf_update_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn); void nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt, const char *hostnqn, struct iovec *iov, uint32_t iovcnt, uint64_t offset, uint32_t length); @@ -358,6 +359,7 @@ void nvmf_subsystem_set_ana_state(struct spdk_nvmf_subsystem *subsystem, int nvmf_ctrlr_async_event_ns_notice(struct spdk_nvmf_ctrlr *ctrlr); int nvmf_ctrlr_async_event_ana_change_notice(struct spdk_nvmf_ctrlr *ctrlr); +int nvmf_ctrlr_async_event_discovery_log_change_notice(struct spdk_nvmf_ctrlr *ctrlr); void nvmf_ctrlr_async_event_reservation_notification(struct spdk_nvmf_ctrlr *ctrlr); void nvmf_ns_reservation_request(void *ctx); void nvmf_ctrlr_reservation_notice_log(struct spdk_nvmf_ctrlr *ctrlr, diff --git a/lib/nvmf/subsystem.c b/lib/nvmf/subsystem.c index 04141fb42..5e6426815 100644 --- a/lib/nvmf/subsystem.c +++ b/lib/nvmf/subsystem.c @@ -303,7 +303,7 @@ spdk_nvmf_subsystem_create(struct spdk_nvmf_tgt *tgt, MODEL_NUMBER_DEFAULT); tgt->subsystems[sid] = subsystem; - tgt->discovery_genctr++; + nvmf_update_discovery_log(tgt, NULL); return subsystem; } @@ -374,7 +374,7 @@ spdk_nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem) free(subsystem->ns); subsystem->tgt->subsystems[subsystem->id] = NULL; - subsystem->tgt->discovery_genctr++; + nvmf_update_discovery_log(subsystem->tgt, NULL); pthread_mutex_destroy(&subsystem->mutex); @@ -735,7 +735,7 @@ spdk_nvmf_subsystem_add_host(struct spdk_nvmf_subsystem *subsystem, const char * TAILQ_INSERT_HEAD(&subsystem->hosts, host, link); - subsystem->tgt->discovery_genctr++; + nvmf_update_discovery_log(subsystem->tgt, hostnqn); pthread_mutex_unlock(&subsystem->mutex); @@ -839,6 +839,7 @@ spdk_nvmf_subsystem_set_allow_any_host(struct spdk_nvmf_subsystem *subsystem, bo { pthread_mutex_lock(&subsystem->mutex); subsystem->flags.allow_any_host = allow_any_host; + nvmf_update_discovery_log(subsystem->tgt, NULL); pthread_mutex_unlock(&subsystem->mutex); return 0; @@ -937,7 +938,7 @@ _nvmf_subsystem_add_listener_done(void *ctx, int status) } TAILQ_INSERT_HEAD(&listener->subsystem->listeners, listener, link); - listener->subsystem->tgt->discovery_genctr++; + nvmf_update_discovery_log(listener->subsystem->tgt, NULL); listener->cb_fn(listener->cb_arg, status); } 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 652e6baec..bd43ee78a 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 @@ -55,6 +55,10 @@ DEFINE_STUB(spdk_nvmf_transport_stop_listen, DEFINE_STUB_V(spdk_bdev_close, (struct spdk_bdev_desc *desc)); +DEFINE_STUB(nvmf_ctrlr_async_event_discovery_log_change_notice, + int, + (struct spdk_nvmf_ctrlr *ctrlr), 0); + const char * spdk_bdev_get_name(const struct spdk_bdev *bdev) { diff --git a/test/unit/lib/nvmf/fc.c/fc_ut.c b/test/unit/lib/nvmf/fc.c/fc_ut.c index 63b719d1f..f968fe691 100644 --- a/test/unit/lib/nvmf/fc.c/fc_ut.c +++ b/test/unit/lib/nvmf/fc.c/fc_ut.c @@ -132,6 +132,9 @@ DEFINE_STUB_V(spdk_nvmf_ctrlr_data_init, (struct spdk_nvmf_transport_opts *opts, DEFINE_STUB(spdk_nvmf_request_complete, int, (struct spdk_nvmf_request *req), -ENOSPC); +DEFINE_STUB_V(nvmf_update_discovery_log, + (struct spdk_nvmf_tgt *tgt, const char *hostnqn)); + const char * spdk_nvme_transport_id_trtype_str(enum spdk_nvme_transport_type trtype) { diff --git a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c index d61385ade..fbc556915 100644 --- a/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c +++ b/test/unit/lib/nvmf/subsystem.c/subsystem_ut.c @@ -64,6 +64,9 @@ DEFINE_STUB(spdk_nvmf_transport_stop_listen, (struct spdk_nvmf_transport *transport, const struct spdk_nvme_transport_id *trid), 0); +DEFINE_STUB_V(nvmf_update_discovery_log, + (struct spdk_nvmf_tgt *tgt, const char *hostnqn)); + int spdk_nvmf_transport_listen(struct spdk_nvmf_transport *transport, const struct spdk_nvme_transport_id *trid)