From 4f4f505c77ace180f7be6ebd9fc2e6cc19f206f3 Mon Sep 17 00:00:00 2001 From: tyler_sun Date: Sat, 24 Apr 2021 15:58:44 +0800 Subject: [PATCH] nvme: get changed ns log once AER notice of ns changed received. Each time the following file "/sys/kernel/config/nvmet/subsystems/nqn_name/namespaces/ns_id/enable" on the target side was changed, the SPDK initiator should receive an async event (type: SPDK_NVME_ASYNC_EVENT_TYPE_NOTICE, info: SPDK_NVME_ASYNC_EVENT_NS_ATTR_CHANGED). But actually not. Since for SPDK, when target sent the non-first event, the condition "nvmet_aen_bit_disabled(ctrl, NVME_AEN_BIT_NS_ATTR)" that prevents target from sending event was matched. This commit fix this issue by issuing a get_log_page cmd for each async event received, just as the kernel initiator does. Fixes #1825. Signed-off-by: tyler.sun Change-Id: I2973470a81893456ca12e86ac390ea1de0eed62c Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/7107 Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Changpeng Liu Reviewed-by: Aleksey Marchuk Reviewed-by: Ziye Yang --- include/spdk/nvme_spec.h | 5 +++ lib/nvme/nvme_ctrlr.c | 86 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/include/spdk/nvme_spec.h b/include/spdk/nvme_spec.h index db122a66a..d44755819 100644 --- a/include/spdk/nvme_spec.h +++ b/include/spdk/nvme_spec.h @@ -72,6 +72,11 @@ extern "C" { */ #define SPDK_NVME_DATASET_MANAGEMENT_RANGE_MAX_BLOCKS 0xFFFFFFFFu +/** + * Maximum number of entries in the log page of Changed Namespace List. + */ +#define SPDK_NVME_MAX_CHANGED_NAMESPACES 1024 + union spdk_nvme_cap_register { uint64_t raw; struct { diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index 28cafdff8..5588ca41f 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -38,6 +38,7 @@ #include "spdk/env.h" #include "spdk/string.h" +#include "spdk/endian.h" struct nvme_active_ns_ctx; @@ -2614,24 +2615,81 @@ fail: return rc; } +static int +nvme_ctrlr_clear_changed_ns_log(struct spdk_nvme_ctrlr *ctrlr) +{ + struct nvme_completion_poll_status *status; + int rc = -ENOMEM; + char *buffer = NULL; + uint32_t nsid; + size_t buf_size = (SPDK_NVME_MAX_CHANGED_NAMESPACES * sizeof(uint32_t)); + + buffer = spdk_dma_zmalloc(buf_size, 4096, NULL); + if (!buffer) { + NVME_CTRLR_ERRLOG(ctrlr, "Failed to allocate buffer for getting " + "changed ns log.\n"); + return rc; + } + + status = calloc(1, sizeof(*status)); + if (!status) { + NVME_CTRLR_ERRLOG(ctrlr, "Failed to allocate status tracker\n"); + goto free_buffer; + } + + rc = spdk_nvme_ctrlr_cmd_get_log_page(ctrlr, + SPDK_NVME_LOG_CHANGED_NS_LIST, + SPDK_NVME_GLOBAL_NS_TAG, + buffer, buf_size, 0, + nvme_completion_poll_cb, status); + + if (rc) { + NVME_CTRLR_ERRLOG(ctrlr, "spdk_nvme_ctrlr_cmd_get_log_page() failed: rc=%d\n", rc); + free(status); + goto free_buffer; + } + + rc = nvme_wait_for_completion_timeout(ctrlr->adminq, status, + ctrlr->opts.admin_timeout_ms * 1000); + if (!status->timed_out) { + free(status); + } + + if (rc) { + NVME_CTRLR_ERRLOG(ctrlr, "wait for spdk_nvme_ctrlr_cmd_get_log_page failed: rc=%d\n", rc); + goto free_buffer; + } + + /* only check the case of overflow. */ + nsid = from_le32(buffer); + if (nsid == 0xffffffffu) { + NVME_CTRLR_WARNLOG(ctrlr, "changed ns log overflowed.\n"); + } + +free_buffer: + spdk_dma_free(buffer); + return rc; +} + void nvme_ctrlr_process_async_event(struct spdk_nvme_ctrlr *ctrlr, const struct spdk_nvme_cpl *cpl) { union spdk_nvme_async_event_completion event; struct spdk_nvme_ctrlr_process *active_proc; + bool ns_changed = false; int rc; event.raw = cpl->cdw0; if ((event.bits.async_event_type == SPDK_NVME_ASYNC_EVENT_TYPE_NOTICE) && (event.bits.async_event_info == SPDK_NVME_ASYNC_EVENT_NS_ATTR_CHANGED)) { - rc = nvme_ctrlr_identify_active_ns(ctrlr); - if (rc) { - return; - } - nvme_ctrlr_update_namespaces(ctrlr); - nvme_io_msg_ctrlr_update(ctrlr); + /* + * apps (e.g., test/nvme/aer/aer.c) may also get changed ns log (through + * active_proc->aer_cb_fn). To avoid impaction, move our operations + * behind call of active_proc->aer_cb_fn. + */ + ns_changed = true; } if ((event.bits.async_event_type == SPDK_NVME_ASYNC_EVENT_TYPE_NOTICE) && @@ -2647,6 +2705,22 @@ nvme_ctrlr_process_async_event(struct spdk_nvme_ctrlr *ctrlr, if (active_proc && active_proc->aer_cb_fn) { active_proc->aer_cb_fn(active_proc->aer_cb_arg, cpl); } + + if (ns_changed) { + /* + * Must have the changed ns log cleared by getting it. + * Otherwise, the target won't send + * the subsequent ns enabling/disabling events to us. + */ + nvme_ctrlr_clear_changed_ns_log(ctrlr); + + rc = nvme_ctrlr_identify_active_ns(ctrlr); + if (rc) { + return; + } + nvme_ctrlr_update_namespaces(ctrlr); + nvme_io_msg_ctrlr_update(ctrlr); + } } static void