Skip to contentSkip to conversation

nvmet: Enable AENs support for Discovery controllers

PATCH Superseded Jay Sternberg 4 participants 27 messages Oct 16, 2018 – Nov 8, 2018

Subsystems (2)
NVMe NVMe target

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jay Sternberg

author
#0 lore raw

v2 cover nvmet: Enable AENs support for Discovery controllers

v2 - corrected two patch descriptions and a patch's author

Technical proposal 8002 defines enhancements to provide NVMe-oF with the same level of resource enumeration as is available with local PCIe-based NVMe devices. It defines an NVMe-oF level mechanism for notifying provisioned hosts about remote resource state changes (i.e., added, removed, modified), specifically Discovery Log changes.

These changes allow an optional explicit persistent connection to Discovery controllers, and defines a mechanism to indicate changes occurring on Subsystems and Namespace controllers via Asynchronous Event Notifications.

Jay Sternberg (10):
nvmet: Provide aen bit functions for multiple controller types
nvmet: Change aen mask functions to use bit numbers
nvmet: Allow Keep Alive for Discovery controller
nvmet: Make kato and AEN processing for use by other controllers
nvmet: Add defines for discovery change async events
nvmet: Add support to Discovery controllers for commands
nvmet: Add parent to nvmet_referral_disable
nvmet: Maintain a global list of ports
nvmet: Allow all hosts access to the Discovery subsystem
nvmet: Enable Discovery Controller AENs

Phil Cayton (1):
nvmet: Provide adding AENs for multiple controller types

 drivers/nvme/target/admin-cmd.c |  63 +++++++++++----------
 drivers/nvme/target/configfs.c  |  26 ++++++++-
 drivers/nvme/target/core.c      |  69 ++++++-----------------
 drivers/nvme/target/discovery.c | 121 +++++++++++++++++++++++++++++++++++++++-
 drivers/nvme/target/nvmet.h     |  49 ++++++++++++++--
 include/linux/nvme.h            |  15 ++++-
 6 files changed, 247 insertions(+), 96 deletions(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH v2 1/11 nvmet: Provide aen bit functions for multiple controller types

Move nvmet_aen_disabled and nvmet_clear_aen in preparation for other types of controllers to use, initially the discovery controller.

Signed-off-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by Sagi Grimberg <sagi at grimberg.me>

---
v2 - unchanged

 drivers/nvme/target/admin-cmd.c | 13 -------------
 drivers/nvme/target/core.c      |  7 -------
 drivers/nvme/target/nvmet.h     | 15 +++++++++++++++
 3 files changed, 15 insertions(+), 20 deletions(-)
drivers/nvme/target/admin-cmd.c +0 -13
index a21caea..4f32de0 100644
@@ -19,19 +19,6 @@
 #include <asm/unaligned.h>
 #include "nvmet.h"
 
-/*
- * This helper allows us to clear the AEN based on the RAE bit,
- * Please use this helper when processing the log pages which are
- * associated with the AEN.
- */
-static inline void nvmet_clear_aen(struct nvmet_req *req, u32 aen_bit)
-{
-	int rae = le32_to_cpu(req->cmd->common.cdw10[0]) & 1 << 15;
-
-	if (!rae)
-		clear_bit(aen_bit, &req->sq->ctrl->aen_masked);
-}
-
 u32 nvmet_get_log_page_len(struct nvme_command *cmd)
 {
 	u32 len = le16_to_cpu(cmd->get_log_page.numdu);
drivers/nvme/target/core.c +0 -7
index ebf3e7a..967e624 100644
@@ -149,13 +149,6 @@ static void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
 	schedule_work(&ctrl->async_event_work);
 }
 
-static bool nvmet_aen_disabled(struct nvmet_ctrl *ctrl, u32 aen)
-{
-	if (!(READ_ONCE(ctrl->aen_enabled) & aen))
-		return true;
-	return test_and_set_bit(aen, &ctrl->aen_masked);
-}
-
 static void nvmet_add_to_changed_ns_log(struct nvmet_ctrl *ctrl, __le32 nsid)
 {
 	u32 i;
drivers/nvme/target/nvmet.h +15 -0
index ec9af4e..01703b1 100644
@@ -324,6 +324,21 @@ struct nvmet_async_event {
 	u8			log_page;
 };
 
+static inline void nvmet_clear_aen(struct nvmet_req *req, u32 aen_bit)
+{
+	int rae = le32_to_cpu(req->cmd->common.cdw10[0]) & 1 << 15;
+
+	if (!rae)
+		clear_bit(aen_bit, &req->sq->ctrl->aen_masked);
+}
+
+static inline bool nvmet_aen_disabled(struct nvmet_ctrl *ctrl, u32 aen)
+{
+	if (!(READ_ONCE(ctrl->aen_enabled) & aen))
+		return true;
+	return test_and_set_bit(aen, &ctrl->aen_masked);
+}
+
 u16 nvmet_parse_connect_cmd(struct nvmet_req *req);
 u16 nvmet_bdev_parse_io_cmd(struct nvmet_req *req);
 u16 nvmet_file_parse_io_cmd(struct nvmet_req *req);
Signature · 1 line
1.8.3.1
#2 lore raw

PATCH v2 1/11 nvmet: Provide aen bit functions for multiple controller types

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
author
#3 lore raw

PATCH v2 2/11 nvmet: Change aen mask functions to use bit numbers

In reply to Jay Sternberg

Functions nvmet_aen_disabled and nvmet_clear_aen were using values not bit numbers ie 1 << 9 not 9 for bit function clear_bit and test_and_set_bit.

Signed-off-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Phil Cayton <phil.cayton at intel.com>

---
v2 - unchanged

 drivers/nvme/target/admin-cmd.c |  4 ++--
 drivers/nvme/target/core.c      |  4 ++--
 drivers/nvme/target/nvmet.h     | 10 +++++-----
 include/linux/nvme.h            | 12 +++++++++---
 4 files changed, 18 insertions(+), 12 deletions(-)
drivers/nvme/target/admin-cmd.c +2 -2
index 4f32de0..e50f88c 100644
@@ -176,7 +176,7 @@ static void nvmet_execute_get_log_changed_ns(struct nvmet_req *req)
 	if (!status)
 		status = nvmet_zero_sgl(req, len, req->data_len - len);
 	ctrl->nr_changed_ns = 0;
-	nvmet_clear_aen(req, NVME_AEN_CFG_NS_ATTR);
+	nvmet_clear_aen_bit(req, NVME_AEN_BIT_NS_ATTR);
 	mutex_unlock(&ctrl->lock);
 out:
 	nvmet_req_complete(req, status);

@@ -235,7 +235,7 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 
 	hdr.chgcnt = cpu_to_le64(nvmet_ana_chgcnt);
 	hdr.ngrps = cpu_to_le16(ngrps);
-	nvmet_clear_aen(req, NVME_AEN_CFG_ANA_CHANGE);
+	nvmet_clear_aen_bit(req, NVME_AEN_BIT_ANA_CHANGE);
 	up_read(&nvmet_ana_sem);
 
 	kfree(desc);
drivers/nvme/target/core.c +2 -2
index 967e624..5773b74 100644
@@ -179,7 +179,7 @@ void nvmet_ns_changed(struct nvmet_subsys *subsys, u32 nsid)
 
 	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
 		nvmet_add_to_changed_ns_log(ctrl, cpu_to_le32(nsid));
-		if (nvmet_aen_disabled(ctrl, NVME_AEN_CFG_NS_ATTR))
+		if (nvmet_aen_bit_disabled(ctrl, NVME_AEN_BIT_NS_ATTR))
 			continue;
 		nvmet_add_async_event(ctrl, NVME_AER_TYPE_NOTICE,
 				NVME_AER_NOTICE_NS_CHANGED,

@@ -196,7 +196,7 @@ void nvmet_send_ana_event(struct nvmet_subsys *subsys,
 	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
 		if (port && ctrl->port != port)
 			continue;
-		if (nvmet_aen_disabled(ctrl, NVME_AEN_CFG_ANA_CHANGE))
+		if (nvmet_aen_bit_disabled(ctrl, NVME_AEN_BIT_ANA_CHANGE))
 			continue;
 		nvmet_add_async_event(ctrl, NVME_AER_TYPE_NOTICE,
 				NVME_AER_NOTICE_ANA, NVME_LOG_ANA);
drivers/nvme/target/nvmet.h +5 -5
index 01703b1..5a56849 100644
@@ -324,19 +324,19 @@ struct nvmet_async_event {
 	u8			log_page;
 };
 
-static inline void nvmet_clear_aen(struct nvmet_req *req, u32 aen_bit)
+static inline void nvmet_clear_aen_bit(struct nvmet_req *req, u32 bn)
 {
 	int rae = le32_to_cpu(req->cmd->common.cdw10[0]) & 1 << 15;
 
 	if (!rae)
-		clear_bit(aen_bit, &req->sq->ctrl->aen_masked);
+		clear_bit(bn, &req->sq->ctrl->aen_masked);
 }
 
-static inline bool nvmet_aen_disabled(struct nvmet_ctrl *ctrl, u32 aen)
+static inline bool nvmet_aen_bit_disabled(struct nvmet_ctrl *ctrl, u32 bn)
 {
-	if (!(READ_ONCE(ctrl->aen_enabled) & aen))
+	if (!(READ_ONCE(ctrl->aen_enabled) & BIT(bn)))
 		return true;
-	return test_and_set_bit(aen, &ctrl->aen_masked);
+	return test_and_set_bit(bn, &ctrl->aen_masked);
 }
 
 u16 nvmet_parse_connect_cmd(struct nvmet_req *req);
include/linux/nvme.h +9 -3
index 68e91ef..47b69d5 100644
@@ -484,9 +484,15 @@ enum {
 };
 
 enum {
-	NVME_AEN_CFG_NS_ATTR		= 1 << 8,
-	NVME_AEN_CFG_FW_ACT		= 1 << 9,
-	NVME_AEN_CFG_ANA_CHANGE		= 1 << 11,
+	NVME_AEN_BIT_NS_ATTR		= 8,
+	NVME_AEN_BIT_FW_ACT		= 9,
+	NVME_AEN_BIT_ANA_CHANGE		= 11,
+};
+
+enum {
+	NVME_AEN_CFG_NS_ATTR		= BIT(NVME_AEN_BIT_NS_ATTR),
+	NVME_AEN_CFG_FW_ACT		= BIT(NVME_AEN_BIT_FW_ACT),
+	NVME_AEN_CFG_ANA_CHANGE		= BIT(NVME_AEN_BIT_ANA_CHANGE),
 };
 
 struct nvme_lba_range_type {
Signature · 1 line
1.8.3.1
#4 lore raw

PATCH v2 2/11 nvmet: Change aen mask functions to use bit numbers

Quoted 6 lines · Jay Sternberg

Functions nvmet_aen_disabled and nvmet_clear_aen were using values not bit numbers ie 1 << 9 not 9 for bit function clear_bit and test_and_set_bit.

Signed-off-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Phil Cayton <phil.cayton at intel.com>

Can you please explace the usage of the weird BIT() macro with the much more obvious 1 << bit as in the rest of the nvme code?

Otherwise looks fine:

Reviewed-by Christoph Hellwig <hch at lst.de>
author
#5 lore raw

PATCH v2 3/11 nvmet: Allow Keep Alive for Discovery controller

In reply to Jay Sternberg

Per change to specification allowing Discovery controllers to have explicit persistent connections, remove restriction on Discovery controllers allowing kato on connect.

Signed-off-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Hannes Reinecke <hare at suse.com>
Reviewed-by Sagi Grimberg <sagi at grimberg.me>

---
v2 - unchanged

 drivers/nvme/target/admin-cmd.c |  2 +-
 drivers/nvme/target/core.c      | 36 ++++++++++--------------------------
 drivers/nvme/target/discovery.c |  4 ++++
 drivers/nvme/target/nvmet.h     |  4 +++-
 4 files changed, 18 insertions(+), 28 deletions(-)
drivers/nvme/target/admin-cmd.c +1 -1
index e50f88c..e69edd5 100644
@@ -721,7 +721,7 @@ static void nvmet_execute_async_event(struct nvmet_req *req)
 	schedule_work(&ctrl->async_event_work);
 }
 
-static void nvmet_execute_keep_alive(struct nvmet_req *req)
+void nvmet_execute_keep_alive(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
drivers/nvme/target/core.c +10 -26
index 5773b74..f5cd31e 100644
@@ -990,31 +990,17 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	ctrl->cntlid = ret;
 
 	ctrl->ops = req->ops;
-	if (ctrl->subsys->type == NVME_NQN_DISC) {
-		/* Don't accept keep-alive timeout for discovery controllers */
-		if (kato) {
-			status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-			goto out_remove_ida;
-		}
 
-		/*
-		 * Discovery controllers use some arbitrary high value in order
-		 * to cleanup stale discovery sessions
-		 *
-		 * From the latest base diff RC:
-		 * "The Keep Alive command is not supported by
-		 * Discovery controllers. A transport may specify a
-		 * fixed Discovery controller activity timeout value
-		 * (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".
-		 */
-		ctrl->kato = NVMET_DISC_KATO;
-	} else {
-		/* keep-alive timeout in seconds */
-		ctrl->kato = DIV_ROUND_UP(kato, 1000);
-	}
+	/*
+	 * Discovery controllers may use some arbitrary high value
+	 * in order to cleanup stale discovery sessions
+	 */
+	if ((ctrl->subsys->type == NVME_NQN_DISC) && !kato)
+		kato = NVMET_DISC_KATO;
+
+	/* keep-alive timeout in seconds */
+	ctrl->kato = DIV_ROUND_UP(kato, 1000);
+
 	nvmet_start_keep_alive_timer(ctrl);
 
 	mutex_lock(&subsys->lock);

@@ -1024,8 +1010,6 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	*ctrlp = ctrl;
 	return 0;
 
-out_remove_ida:
-	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
 out_free_sqs:
 	kfree(ctrl->sqs);
 out_free_cqs:
drivers/nvme/target/discovery.c +4 -0
index eae29f4..6689c56 100644
@@ -194,6 +194,10 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 	}
 
 	switch (cmd->common.opcode) {
+	case nvme_admin_keep_alive:
+		req->execute = nvmet_execute_keep_alive;
+		req->data_len = 0;
+		return 0;
 	case nvme_admin_get_log_page:
 		req->data_len = nvmet_get_log_page_len(cmd);
drivers/nvme/target/nvmet.h +3 -1
index 5a56849..4910e83c 100644
@@ -352,6 +352,8 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
 void nvmet_req_execute(struct nvmet_req *req);
 void nvmet_req_complete(struct nvmet_req *req, u16 status);
 
+void nvmet_execute_keep_alive(struct nvmet_req *req);
+
 void nvmet_cq_setup(struct nvmet_ctrl *ctrl, struct nvmet_cq *cq, u16 qid,
 		u16 size);
 void nvmet_sq_setup(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq, u16 qid,

@@ -422,7 +424,7 @@ u16 nvmet_copy_from_sgl(struct nvmet_req *req, off_t off, void *buf,
 #define NVMET_DEFAULT_ANA_GRPID	1
 
 #define NVMET_KAS		10
-#define NVMET_DISC_KATO		120
+#define NVMET_DISC_KATO		120000 /* ms */
 
 int __init nvmet_init_configfs(void);
 void __exit nvmet_exit_configfs(void);
Signature · 1 line
1.8.3.1
#6 lore raw

PATCH v2 3/11 nvmet: Allow Keep Alive for Discovery controller

drivers/nvme/target/core.c u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn, · line 999

Quoted 2 lines
drivers/nvme/target/core.c +2 -0
+	if ((ctrl->subsys->type == NVME_NQN_DISC) && !kato)
+		kato = NVMET_DISC_KATO;

No need for the inner braces here.

Quoted 1 line

+#define NVMET_DISC_KATO 120000 /* ms */

I'd rename it to NVMET_DISC_KATO_MS to make the unit obvious and remove the need for the comment.

Otherwise looks fine:

Reviewed-by Christoph Hellwig <hch at lst.de>
author
#7 lore raw

PATCH v2 4/11 nvmet: Make kato and AEN processing for use by other controllers

In reply to Jay Sternberg

Make common process of get/set features available to other controllers by making simple functions static inline and others not static and prototypes in nvmet.h file

Also remove static from nvmet_execute_async_event and add prototype to nvmet.h to allow used by other controllers

Signed-off-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Hannes Reinecke <hare at suse.com>
Reviewed-by Sagi Grimberg <sagi at grimberg.me>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

---
v2 - unchanged

 drivers/nvme/target/admin-cmd.c | 44 +++++++++++++++++++++++++++--------------
 drivers/nvme/target/nvmet.h     | 14 +++++++++++++
 2 files changed, 43 insertions(+), 15 deletions(-)
drivers/nvme/target/admin-cmd.c +29 -15
index e69edd5..289ee18 100644
@@ -582,11 +582,34 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 	return status;
 }
 
+u16 nvmet_set_feat_kato(struct nvmet_req *req)
+{
+	u32 val32 = le32_to_cpu(req->cmd->common.cdw10[1]);
+
+	req->sq->ctrl->kato = DIV_ROUND_UP(val32, 1000);
+
+	nvmet_set_result(req, req->sq->ctrl->kato);
+
+	return 0;
+}
+
+u16 nvmet_set_feat_async_event(struct nvmet_req *req, u32 mask)
+{
+	u32 val32 = le32_to_cpu(req->cmd->common.cdw10[1]);
+
+	if (val32 & ~mask)
+		return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+
+	WRITE_ONCE(req->sq->ctrl->aen_enabled, val32);
+	nvmet_set_result(req, val32);
+
+	return 0;
+}
+
 static void nvmet_execute_set_features(struct nvmet_req *req)
 {
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10[0]);
-	u32 val32;
 	u16 status = 0;
 
 	switch (cdw10 & 0xff) {

@@ -595,19 +618,10 @@ static void nvmet_execute_set_features(struct nvmet_req *req)
 			(subsys->max_qid - 1) | ((subsys->max_qid - 1) << 16));
 		break;
 	case NVME_FEAT_KATO:
-		val32 = le32_to_cpu(req->cmd->common.cdw10[1]);
-		req->sq->ctrl->kato = DIV_ROUND_UP(val32, 1000);
-		nvmet_set_result(req, req->sq->ctrl->kato);
+		status = nvmet_set_feat_kato(req);
 		break;
 	case NVME_FEAT_ASYNC_EVENT:
-		val32 = le32_to_cpu(req->cmd->common.cdw10[1]);
-		if (val32 & ~NVMET_AEN_CFG_ALL) {
-			status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-			break;
-		}
-
-		WRITE_ONCE(req->sq->ctrl->aen_enabled, val32);
-		nvmet_set_result(req, val32);
+		status = nvmet_set_feat_async_event(req, NVMET_AEN_CFG_ALL);
 		break;
 	case NVME_FEAT_HOST_ID:
 		status = NVME_SC_CMD_SEQ_ERROR | NVME_SC_DNR;

@@ -672,7 +686,7 @@ static void nvmet_execute_get_features(struct nvmet_req *req)
 		break;
 #endif
 	case NVME_FEAT_ASYNC_EVENT:
-		nvmet_set_result(req, READ_ONCE(req->sq->ctrl->aen_enabled));
+		nvmet_get_feat_async_event(req);
 		break;
 	case NVME_FEAT_VOLATILE_WC:
 		nvmet_set_result(req, 1);

@@ -682,7 +696,7 @@ static void nvmet_execute_get_features(struct nvmet_req *req)
 			(subsys->max_qid-1) | ((subsys->max_qid-1) << 16));
 		break;
 	case NVME_FEAT_KATO:
-		nvmet_set_result(req, req->sq->ctrl->kato * 1000);
+		nvmet_get_feat_kato(req);
 		break;
 	case NVME_FEAT_HOST_ID:
 		/* need 128-bit host identifier flag */

@@ -705,7 +719,7 @@ static void nvmet_execute_get_features(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
-static void nvmet_execute_async_event(struct nvmet_req *req)
+void nvmet_execute_async_event(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
drivers/nvme/target/nvmet.h +14 -0
index 4910e83c..e263330 100644
@@ -339,6 +339,20 @@ static inline bool nvmet_aen_bit_disabled(struct nvmet_ctrl *ctrl, u32 bn)
 	return test_and_set_bit(bn, &ctrl->aen_masked);
 }
 
+static inline void nvmet_get_feat_kato(struct nvmet_req *req)
+{
+	nvmet_set_result(req, req->sq->ctrl->kato * 1000);
+}
+
+static inline void nvmet_get_feat_async_event(struct nvmet_req *req)
+{
+	nvmet_set_result(req, READ_ONCE(req->sq->ctrl->aen_enabled));
+}
+
+u16 nvmet_set_feat_kato(struct nvmet_req *req);
+u16 nvmet_set_feat_async_event(struct nvmet_req *req, u32 mask);
+void nvmet_execute_async_event(struct nvmet_req *req);
+
 u16 nvmet_parse_connect_cmd(struct nvmet_req *req);
 u16 nvmet_bdev_parse_io_cmd(struct nvmet_req *req);
 u16 nvmet_file_parse_io_cmd(struct nvmet_req *req);
Signature · 1 line
1.8.3.1
#8 lore raw

PATCH v2 4/11 nvmet: Make kato and AEN processing for use by other controllers

Quoted 6 lines · Jay Sternberg

Make common process of get/set features available to other controllers by making simple functions static inline and others not static and prototypes in nvmet.h file

Also remove static from nvmet_execute_async_event and add prototype to nvmet.h to allow used by other controllers

I'd move nvmet_get_feat_kato and nvmet_get_feat_async_event out of line, just to not bloat the header with too much random sprinkled implementation bits, given none of this is timing critical.

Otherwise looks good:

Reviewed-by Christoph Hellwig <hch at lst.de>
author
#9 lore raw

PATCH v2 5/11 nvmet: Add defines for discovery change async events

In reply to Jay Sternberg

Add AEN/AER values as defined by the specification

Signed-off-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Sagi Grimberg <sagi at grimberg.me>

---
v2 - unchanged

 drivers/nvme/target/nvmet.h | 2 ++
 include/linux/nvme.h        | 3 +++
 2 files changed, 5 insertions(+)
drivers/nvme/target/nvmet.h +2 -0
index e263330..dd7e5223 100644
@@ -35,6 +35,8 @@
  */
 #define NVMET_AEN_CFG_OPTIONAL \
 	(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_ANA_CHANGE)
+#define NVMET_DISC_AEN_CFG_OPTIONAL \
+	(NVME_AEN_CFG_DISC_CHANGE)
 
 /*
  * Plus mandatory SMART AENs (we'll never send them, but allow enabling them):
include/linux/nvme.h +3 -0
index 47b69d5..11ab828 100644
@@ -481,18 +481,21 @@ enum {
 	NVME_AER_NOTICE_NS_CHANGED	= 0x00,
 	NVME_AER_NOTICE_FW_ACT_STARTING = 0x01,
 	NVME_AER_NOTICE_ANA		= 0x03,
+	NVME_AER_NOTICE_DISC_CHANGED	= 0xf0,
 };
 
 enum {
 	NVME_AEN_BIT_NS_ATTR		= 8,
 	NVME_AEN_BIT_FW_ACT		= 9,
 	NVME_AEN_BIT_ANA_CHANGE		= 11,
+	NVME_AEN_BIT_DISC_CHANGE	= 31,
 };
 
 enum {
 	NVME_AEN_CFG_NS_ATTR		= BIT(NVME_AEN_BIT_NS_ATTR),
 	NVME_AEN_CFG_FW_ACT		= BIT(NVME_AEN_BIT_FW_ACT),
 	NVME_AEN_CFG_ANA_CHANGE		= BIT(NVME_AEN_BIT_ANA_CHANGE),
+	NVME_AEN_CFG_DISC_CHANGE	= BIT(NVME_AEN_BIT_DISC_CHANGE),
 };
 
 struct nvme_lba_range_type {
Signature · 1 line
1.8.3.1
#10 lore raw

PATCH v2 5/11 nvmet: Add defines for discovery change async events

Looks good,

Reviewed-by Christoph Hellwig <hch at lst.de>
#11 lore raw

PATCH v2 5/11 nvmet: Add defines for discovery change async events

In reply to Jay Sternberg
Quoted 1 line · Jay Sternberg

Add AEN/AER values as defined by the specification

Looks good,

Reviewed-by Christoph Hellwig <hch at lst.de>
author
#12 lore raw

PATCH v2 6/11 nvmet: Add support to Discovery controllers for commands

In reply to Jay Sternberg

Add custom get/set features to restrict commands allowed by Discovery controllers.

Signed-off-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Hannes Reinecke <hare at suse.com>
Reviewed-by Sagi Grimberg <sagi at grimberg.me>

---
v2 - unchanged

 drivers/nvme/target/discovery.c | 53 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
drivers/nvme/target/discovery.c +53 -0
index 6689c56..df609ef 100644
@@ -183,6 +183,47 @@ static void nvmet_execute_identify_disc_ctrl(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_disc_set_features(struct nvmet_req *req)
+{
+	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10[0]);
+	u16 stat;
+
+	switch (cdw10 & 0xff) {
+	case NVME_FEAT_KATO:
+		stat = nvmet_set_feat_kato(req);
+		break;
+	case NVME_FEAT_ASYNC_EVENT:
+		stat = nvmet_set_feat_async_event(req,
+						  NVMET_DISC_AEN_CFG_OPTIONAL);
+		break;
+	default:
+		stat = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+		break;
+	}
+
+	nvmet_req_complete(req, stat);
+}
+
+static void nvmet_execute_disc_get_features(struct nvmet_req *req)
+{
+	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10[0]);
+	u16 stat = 0;
+
+	switch (cdw10 & 0xff) {
+	case NVME_FEAT_KATO:
+		nvmet_get_feat_kato(req);
+		break;
+	case NVME_FEAT_ASYNC_EVENT:
+		nvmet_get_feat_async_event(req);
+		break;
+	default:
+		stat = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+		break;
+	}
+
+	nvmet_req_complete(req, stat);
+}
+
 u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 {
 	struct nvme_command *cmd = req->cmd;

@@ -194,6 +235,18 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 	}
 
 	switch (cmd->common.opcode) {
+	case nvme_admin_set_features:
+		req->execute = nvmet_execute_disc_set_features;
+		req->data_len = 0;
+		return 0;
+	case nvme_admin_get_features:
+		req->execute = nvmet_execute_disc_get_features;
+		req->data_len = 0;
+		return 0;
+	case nvme_admin_async_event:
+		req->execute = nvmet_execute_async_event;
+		req->data_len = 0;
+		return 0;
 	case nvme_admin_keep_alive:
 		req->execute = nvmet_execute_keep_alive;
 		req->data_len = 0;
Signature · 1 line
1.8.3.1
#13 lore raw

PATCH v2 6/11 nvmet: Add support to Discovery controllers for commands

Quoted 2 lines · Jay Sternberg

Add custom get/set features to restrict commands allowed by Discovery controllers.

This does't restrict anything, but actually adds the previously unimplemented commands, doesn't it?

Otherwise looks good:

Reviewed-by Christoph Hellwig <hch at lst.de>
author
#14 lore raw

PATCH v2 7/11 nvmet: Add parent to nvmet_referral_disable

In reply to Jay Sternberg

To be used when Discovery Change notifications occur

Signed-off-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Phil Cayton <phil.cayton at intel.com>

---
v2 - unchanged

 drivers/nvme/target/configfs.c  | 5 +++--
 drivers/nvme/target/discovery.c | 2 +-
 drivers/nvme/target/nvmet.h     | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)
drivers/nvme/target/configfs.c +3 -2
index b37a8e3..df841e0 100644
@@ -889,7 +889,7 @@ static ssize_t nvmet_referral_enable_store(struct config_item *item,
 	if (enable)
 		nvmet_referral_enable(parent, port);
 	else
-		nvmet_referral_disable(port);
+		nvmet_referral_disable(parent, port);
 
 	return count;
 inval:

@@ -915,9 +915,10 @@ static ssize_t nvmet_referral_enable_store(struct config_item *item,
 
 static void nvmet_referral_release(struct config_item *item)
 {
+	struct nvmet_port *parent = to_nvmet_port(item->ci_parent->ci_parent);
 	struct nvmet_port *port = to_nvmet_port(item);
 
-	nvmet_referral_disable(port);
+	nvmet_referral_disable(parent, port);
 	kfree(port);
 }
drivers/nvme/target/discovery.c +1 -1
index df609ef..9ae8208 100644
@@ -31,7 +31,7 @@ void nvmet_referral_enable(struct nvmet_port *parent, struct nvmet_port *port)
 	up_write(&nvmet_config_sem);
 }
 
-void nvmet_referral_disable(struct nvmet_port *port)
+void nvmet_referral_disable(struct nvmet_port *parent, struct nvmet_port *port)
 {
 	down_write(&nvmet_config_sem);
 	if (!list_empty(&port->entry)) {
drivers/nvme/target/nvmet.h +1 -1
index dd7e5223..dbde3ad 100644
@@ -410,7 +410,7 @@ void nvmet_send_ana_event(struct nvmet_subsys *subsys,
 void nvmet_disable_port(struct nvmet_port *port);
 
 void nvmet_referral_enable(struct nvmet_port *parent, struct nvmet_port *port);
-void nvmet_referral_disable(struct nvmet_port *port);
+void nvmet_referral_disable(struct nvmet_port *parent, struct nvmet_port *port);
 
 u16 nvmet_copy_to_sgl(struct nvmet_req *req, off_t off, const void *buf,
 		size_t len);
Signature · 1 line
1.8.3.1
#15 lore raw

PATCH v2 7/11 nvmet: Add parent to nvmet_referral_disable

Quoted 1 line · Jay Sternberg

To be used when Discovery Change notifications occur

Looks fine, but I'd be tempted to just merge this into the patch actually using it.

author
#16 lore raw

PATCH v2 8/11 nvmet: Maintain a global list of ports

In reply to Jay Sternberg

To be used when Discovery Change notifications occur

Signed-off-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Phil Cayton <phil.cayton at intel.com>

---
v2 - unchanged

 drivers/nvme/target/configfs.c | 7 +++++++
 drivers/nvme/target/nvmet.h    | 3 +++
 2 files changed, 10 insertions(+)
drivers/nvme/target/configfs.c +7 -0
index df841e0..c1ad175 100644
@@ -23,6 +23,9 @@
 static const struct config_item_type nvmet_host_type;
 static const struct config_item_type nvmet_subsys_type;
 
+static LIST_HEAD(nvmet_ports_list);
+struct list_head *nvmet_ports = &nvmet_ports_list;
+
 static const struct nvmet_transport_name {
 	u8		type;
 	const char	*name;

@@ -1091,6 +1094,8 @@ static void nvmet_port_release(struct config_item *item)
 {
 	struct nvmet_port *port = to_nvmet_port(item);
 
+	list_del(&port->global_entry);
+
 	kfree(port->ana_state);
 	kfree(port);
 }

@@ -1143,6 +1148,8 @@ static struct config_group *nvmet_ports_make(struct config_group *group,
 			port->ana_state[i] = NVME_ANA_INACCESSIBLE;
 	}
 
+	list_add(&port->global_entry, &nvmet_ports_list);
+
 	INIT_LIST_HEAD(&port->entry);
 	INIT_LIST_HEAD(&port->subsystems);
 	INIT_LIST_HEAD(&port->referrals);
drivers/nvme/target/nvmet.h +3 -0
index dbde3ad..ea93a46 100644
@@ -130,6 +130,7 @@ struct nvmet_port {
 	struct list_head		subsystems;
 	struct config_group		referrals_group;
 	struct list_head		referrals;
+	struct list_head		global_entry;
 	struct config_group		ana_groups_group;
 	struct nvmet_ana_group		ana_default_group;
 	enum nvme_ana_state		*ana_state;

@@ -420,6 +421,8 @@ u16 nvmet_copy_from_sgl(struct nvmet_req *req, off_t off, void *buf,
 
 u32 nvmet_get_log_page_len(struct nvme_command *cmd);
 
+extern struct list_head *nvmet_ports;
+
 #define NVMET_QUEUE_SIZE	1024
 #define NVMET_NR_QUEUES		128
 #define NVMET_MAX_CMD		NVMET_QUEUE_SIZE
Signature · 1 line
1.8.3.1
#17 lore raw

PATCH v2 8/11 nvmet: Maintain a global list of ports

Quoted 1 line · Jay Sternberg

To be used when Discovery Change notifications occur

Looks fine, but again I'm tempted to move this into the patch actually using the information.

author
#18 lore raw

PATCH v2 9/11 nvmet: allow host connect even if no allowed

In reply to Jay Sternberg

From: Sagi Grimberg <[email protected]>

It is perfectly valid that a host connects to a discovery subsystem and gets an empty discovery log page since no subsystems are provisioned to it. No reason to disallow connecting to the discovery subsystem all together.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
Reviewed-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Phil Cayton <phil.cayton at intel.com>

---
v2 - corrected author and updated description per authors request

 drivers/nvme/target/core.c      | 20 +++-----------------
 drivers/nvme/target/discovery.c |  2 +-
 drivers/nvme/target/nvmet.h     |  3 +--
 3 files changed, 5 insertions(+), 20 deletions(-)
drivers/nvme/target/core.c +3 -17
index f5cd31e..67ea1d2 100644
@@ -890,26 +890,12 @@ static bool __nvmet_host_allowed(struct nvmet_subsys *subsys,
 	return false;
 }
 
-static bool nvmet_host_discovery_allowed(struct nvmet_req *req,
-		const char *hostnqn)
-{
-	struct nvmet_subsys_link *s;
-
-	list_for_each_entry(s, &req->port->subsystems, entry) {
-		if (__nvmet_host_allowed(s->subsys, hostnqn))
-			return true;
-	}
-
-	return false;
-}
-
-bool nvmet_host_allowed(struct nvmet_req *req, struct nvmet_subsys *subsys,
-		const char *hostnqn)
+bool nvmet_host_allowed(struct nvmet_subsys *subsys, const char *hostnqn)
 {
 	lockdep_assert_held(&nvmet_config_sem);
 
 	if (subsys->type == NVME_NQN_DISC)
-		return nvmet_host_discovery_allowed(req, hostnqn);
+		return true;
 	else
 		return __nvmet_host_allowed(subsys, hostnqn);
 }

@@ -933,7 +919,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 
 	status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
 	down_read(&nvmet_config_sem);
-	if (!nvmet_host_allowed(req, subsys, hostnqn)) {
+	if (!nvmet_host_allowed(subsys, hostnqn)) {
 		pr_info("connect by host %s for subsystem %s not allowed\n",
 			hostnqn, subsysnqn);
 		req->rsp->result.u32 = IPO_IATTR_CONNECT_DATA(hostnqn);
drivers/nvme/target/discovery.c +1 -1
index 9ae8208..8798a2f 100644
@@ -107,7 +107,7 @@ static void nvmet_execute_get_disc_log_page(struct nvmet_req *req)
 
 	down_read(&nvmet_config_sem);
 	list_for_each_entry(p, &req->port->subsystems, entry) {
-		if (!nvmet_host_allowed(req, p->subsys, ctrl->hostnqn))
+		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
 			continue;
 		if (residual_len >= entry_size) {
 			char traddr[NVMF_TRADDR_SIZE];
drivers/nvme/target/nvmet.h +1 -2
index ea93a46..0a65415 100644
@@ -459,8 +459,7 @@ u16 nvmet_copy_from_sgl(struct nvmet_req *req, off_t off, void *buf,
 extern u64 nvmet_ana_chgcnt;
 extern struct rw_semaphore nvmet_ana_sem;
 
-bool nvmet_host_allowed(struct nvmet_req *req, struct nvmet_subsys *subsys,
-		const char *hostnqn);
+bool nvmet_host_allowed(struct nvmet_subsys *subsys, const char *hostnqn);
 
 int nvmet_bdev_ns_enable(struct nvmet_ns *ns);
 int nvmet_file_ns_enable(struct nvmet_ns *ns);
Signature · 1 line
1.8.3.1
#19 lore raw

PATCH v2 9/11 nvmet: allow host connect even if no allowed

drivers/nvme/target/core.c static bool __nvmet_host_allowed(struct nvmet_subsys *subsys, · line 900

Quoted 9 lines
drivers/nvme/target/core.c +2 -1
+bool nvmet_host_allowed(struct nvmet_subsys *subsys, const char *hostnqn)
 {
 	lockdep_assert_held(&nvmet_config_sem);
 
 	if (subsys->type == NVME_NQN_DISC)
-		return nvmet_host_discovery_allowed(req, hostnqn);
+		return true;
 	else
 		return __nvmet_host_allowed(subsys, hostnqn);

Please merge __nvmet_host_allowed into nvmet_host_allowed, with an early return for the NVME_NQN_DISC case. Please also write a comment explaining that special case.

author
#20 lore raw

PATCH v2 10/11 nvmet: Provide adding AENs for multiple controller types

In reply to Jay Sternberg

From: Phil Cayton <[email protected]>

Make nvmet_add_async_event a non-static function in preparation for use by other controllers, initally the discovery controller.

Signed-off-by Phil Cayton <phil.cayton at intel.com>
Reviewed-by Jay Sternberg <jay.e.sternberg at intel.com>

---
v2 - unchanged

 drivers/nvme/target/core.c  | 2 +-
 drivers/nvme/target/nvmet.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)
drivers/nvme/target/core.c +1 -1
index 67ea1d2..e9f9996 100644
@@ -129,7 +129,7 @@ static void nvmet_async_event_work(struct work_struct *work)
 	}
 }
 
-static void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
+void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
 		u8 event_info, u8 log_page)
 {
 	struct nvmet_async_event *aen;
drivers/nvme/target/nvmet.h +2 -0
index 0a65415..9bb546d 100644
@@ -422,6 +422,8 @@ u16 nvmet_copy_from_sgl(struct nvmet_req *req, off_t off, void *buf,
 u32 nvmet_get_log_page_len(struct nvme_command *cmd);
 
 extern struct list_head *nvmet_ports;
+void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
+		u8 event_info, u8 log_page);
 
 #define NVMET_QUEUE_SIZE	1024
 #define NVMET_NR_QUEUES		128
Signature · 1 line
1.8.3.1
#21 lore raw

PATCH v2 10/11 nvmet: Provide adding AENs for multiple controller types

Quoted 4 lines · Jay Sternberg

From: Phil Cayton <phil.cayton at intel.com>

Make nvmet_add_async_event a non-static function in preparation for use by other controllers, initally the discovery controller.

Looks fine, but please merge it into the patch actually using the feature.

author
#22 lore raw

PATCH v2 11/11 nvmet: Enable Discovery Controller AENs

In reply to Jay Sternberg

Add functions to find connections requesting Discovery Change events and send a notification to hosts that maintain an explicit persistent connection and have and active Asynchronous Event Request pending. Only Hosts that have access to the Subsystem effected by the change will receive notifications of Discovery Change event.

Call these functions each time there is a configfs change that effects the Discover Log Pages.

Set the OAES field in the Identify Controller response to advertise the support for Asynchronous Event Notifications.

Signed-off-by Jay Sternberg <jay.e.sternberg at intel.com>
Reviewed-by Phil Cayton <phil.cayton at intel.com>

---
v2 - updated commit description

 drivers/nvme/target/configfs.c  | 14 +++++++++-
 drivers/nvme/target/discovery.c | 60 +++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h     |  4 +++
 3 files changed, 77 insertions(+), 1 deletion(-)
drivers/nvme/target/configfs.c +13 -1
index c1ad175..8917c62 100644
@@ -603,6 +603,8 @@ static int nvmet_port_subsys_allow_link(struct config_item *parent,
 
 	list_add_tail(&link->entry, &port->subsystems);
 	nvmet_genctr++;
+	nvmet_port_disc_change_event(port, subsys);
+
 	up_write(&nvmet_config_sem);
 	return 0;
 

@@ -630,6 +632,8 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent,
 found:
 	list_del(&p->entry);
 	nvmet_genctr++;
+	nvmet_port_disc_change_event(port, subsys);
+
 	if (list_empty(&port->subsystems))
 		nvmet_disable_port(port);
 	up_write(&nvmet_config_sem);

@@ -679,6 +683,8 @@ static int nvmet_allowed_hosts_allow_link(struct config_item *parent,
 	}
 	list_add_tail(&link->entry, &subsys->hosts);
 	nvmet_genctr++;
+	nvmet_subsys_disc_change_event(subsys, host);
+
 	up_write(&nvmet_config_sem);
 	return 0;
 out_free_link:

@@ -705,6 +711,8 @@ static void nvmet_allowed_hosts_drop_link(struct config_item *parent,
 found:
 	list_del(&p->entry);
 	nvmet_genctr++;
+	nvmet_subsys_disc_change_event(subsys, host);
+
 	up_write(&nvmet_config_sem);
 	kfree(p);
 }

@@ -743,7 +751,11 @@ static ssize_t nvmet_subsys_attr_allow_any_host_store(struct config_item *item,
 		goto out_unlock;
 	}
 
-	subsys->allow_any_host = allow_any_host;
+	if (subsys->allow_any_host != allow_any_host) {
+		subsys->allow_any_host = allow_any_host;
+		nvmet_subsys_disc_change_event(subsys, NULL);
+	}
+
 out_unlock:
 	up_write(&nvmet_config_sem);
 	return ret ? ret : count;
drivers/nvme/target/discovery.c +60 -0

90 lines not on this page · raw message

drivers/nvme/target/nvmet.h +4 -0
index 9bb546d..3f5651f 100644
@@ -422,6 +422,10 @@ u16 nvmet_copy_from_sgl(struct nvmet_req *req, off_t off, void *buf,
 u32 nvmet_get_log_page_len(struct nvme_command *cmd);
 
 extern struct list_head *nvmet_ports;
+void nvmet_port_disc_change_event(struct nvmet_port *port,
+		struct nvmet_subsys *subsys);
+void nvmet_subsys_disc_change_event(struct nvmet_subsys *subsys,
+		struct nvmet_host *host);
 void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
 		u8 event_info, u8 log_page);
Signature · 1 line
1.8.3.1
#23 lore raw

PATCH v2 11/11 nvmet: Enable Discovery Controller AENs

drivers/nvme/target/configfs.c static int nvmet_port_subsys_allow_link(struct config_item *parent, · line 606

Quoted 3 lines
drivers/nvme/target/configfs.c +1 -0
 	list_add_tail(&link->entry, &port->subsystems);
 	nvmet_genctr++;
+	nvmet_port_disc_change_event(port, subsys);

It seems like the increment of nvmet_genctr should move into nvmet_port_disc_change_event / nvmet_subsys_disc_change_event.

I'd also name these functions similar to the namespace event handler, e.g. nvmet_port_disc_changed / nvmet_subsys_disc_changed.

maintainer
#24 lore raw

v2 cover nvmet: Enable AENs support for Discovery controllers

In reply to Jay Sternberg

For any patch in this series that doesn't already have it,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Christoph, Keith, any feedback from you guys?

maintainer
#25 lore raw

v2 cover nvmet: Enable AENs support for Discovery controllers

Quoted 5 lines · Sagi Grimberg

For any patch in this series that doesn't already have it,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Christoph, Keith, any feedback from you guys?

This mail comes out of nowhere without a previous thread here.

maintainer
#26 lore raw

v2 cover nvmet: Enable AENs support for Discovery controllers

Quoted 8 lines
Quoted 6 lines

For any patch in this series that doesn't already have it,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Christoph, Keith, any feedback from you guys?

This mail comes out of nowhere without a previous thread here.

Umm, Jay sent two revisions of this patch set.

http://lists.infradead.org/pipermail/linux-nvme/2018-October/020232.html

http://lists.infradead.org/pipermail/linux-nvme/2018-October/020426.html