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 <madhu.adav@flipkart.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/4870
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Michael Haeuptle <michaelhaeuptle@gmail.com>
Reviewed-by: Aleksey Marchuk <alexeymar@mellanox.com>
Reviewed-by: Anil Veerabhadrappa <anil.veerabhadrappa@broadcom.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Community-CI: Broadcom CI

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

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 */
 };
 
 /**

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)
 {

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)
 {

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,

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);
 }
 

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)
 {

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)
 {

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)