[PATCH v2 09/11] nvmet: Allow all hosts access to the Discovery subsystem

[PATCH v2 09/11] nvmet: Allow all hosts access to the Discovery subsystem

[Jay Sternberg]

From: Sagi Grimberg <sagi@grimberg.me>

All hosts should have access to the Discovery subsystem.

This change removes the need to pass the nvmet_req structure into
the nvmet_host_discovery_allowed function and removes the need for
the function nvmet_host_discovery_allowed.

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

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

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index f5cd31e..67ea1d2 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -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);
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 9ae8208..8798a2f 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -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];
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index ea93a46..0a65415 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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);
-- 
1.8.3.1

[PATCH v2 11/11] nvmet: Enable Discovery Controller AENs

[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(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index c1ad175..8917c62 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -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;
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 8798a2f..f9f8c3e 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -20,6 +20,60 @@
 
 u64 nvmet_genctr;
 
+static void __nvmet_disc_change_event(struct nvmet_port *port,
+				      struct nvmet_ctrl *ctrl)
+{
+	if (ctrl->port != port)
+		return;
+
+	if (nvmet_aen_bit_disabled(ctrl, NVME_AEN_BIT_DISC_CHANGE))
+		return;
+
+	nvmet_add_async_event(ctrl, NVME_AER_TYPE_NOTICE,
+			      NVME_AER_NOTICE_DISC_CHANGED, NVME_LOG_DISC);
+}
+
+void nvmet_port_disc_change_event(struct nvmet_port *port,
+				  struct nvmet_subsys *subsys)
+{
+	struct nvmet_ctrl *ctrl;
+
+	list_for_each_entry(ctrl, &nvmet_disc_subsys->ctrls, subsys_entry) {
+		if (subsys && !nvmet_host_allowed(subsys, ctrl->hostnqn))
+			continue;
+
+		__nvmet_disc_change_event(port, ctrl);
+	}
+}
+
+static void __nvmet_subsys_disc_change_event(struct nvmet_port *port,
+					     struct nvmet_subsys *subsys,
+					     struct nvmet_host *host)
+{
+	struct nvmet_ctrl *ctrl;
+
+	list_for_each_entry(ctrl, &nvmet_disc_subsys->ctrls, subsys_entry) {
+		if (host && strcmp(nvmet_host_name(host), ctrl->hostnqn))
+			continue;
+
+		__nvmet_disc_change_event(port, ctrl);
+	}
+}
+
+void nvmet_subsys_disc_change_event(struct nvmet_subsys *subsys,
+				    struct nvmet_host *host)
+{
+	struct nvmet_port *port;
+	struct nvmet_subsys_link *s;
+
+	list_for_each_entry(port, nvmet_ports, global_entry)
+		list_for_each_entry(s, &port->subsystems, entry) {
+			if (s->subsys != subsys)
+				continue;
+			__nvmet_subsys_disc_change_event(port, subsys, host);
+		}
+}
+
 void nvmet_referral_enable(struct nvmet_port *parent, struct nvmet_port *port)
 {
 	down_write(&nvmet_config_sem);
@@ -27,6 +81,7 @@ void nvmet_referral_enable(struct nvmet_port *parent, struct nvmet_port *port)
 		list_add_tail(&port->entry, &parent->referrals);
 		port->enabled = true;
 		nvmet_genctr++;
+		nvmet_port_disc_change_event(parent, NULL);
 	}
 	up_write(&nvmet_config_sem);
 }
@@ -38,6 +93,7 @@ void nvmet_referral_disable(struct nvmet_port *parent, struct nvmet_port *port)
 		port->enabled = false;
 		list_del_init(&port->entry);
 		nvmet_genctr++;
+		nvmet_port_disc_change_event(parent, NULL);
 	}
 	up_write(&nvmet_config_sem);
 }
@@ -136,6 +192,8 @@ static void nvmet_execute_get_disc_log_page(struct nvmet_req *req)
 	hdr->numrec = cpu_to_le64(numrec);
 	hdr->recfmt = cpu_to_le16(0);
 
+	nvmet_clear_aen_bit(req, NVME_AEN_BIT_DISC_CHANGE);
+
 	up_read(&nvmet_config_sem);
 
 	status = nvmet_copy_to_sgl(req, 0, hdr, data_len);
@@ -174,6 +232,8 @@ static void nvmet_execute_identify_disc_ctrl(struct nvmet_req *req)
 	if (req->port->inline_data_size)
 		id->sgls |= cpu_to_le32(1 << 20);
 
+	id->oaes = cpu_to_le32(NVMET_DISC_AEN_CFG_OPTIONAL);
+
 	strcpy(id->subnqn, ctrl->subsys->subsysnqn);
 
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 9bb546d..3f5651f 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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);
 
-- 
1.8.3.1

[PATCH v2 11/11] nvmet: Enable Discovery Controller AENs

[Sagi Grimberg]

Jay,

Can you please send another patch set as v2. its easier
to follow and apply later on...

[PATCH v2 00/11] nvmet: Enable AENs support for Discovery controllers

[Jay Sternberg]

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

-- 
1.8.3.1

[PATCH v2 11/11] nvmet: Enable Discovery Controller AENs

[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(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index c1ad175..8917c62 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -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;
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 8798a2f..f9f8c3e 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -20,6 +20,60 @@
 
 u64 nvmet_genctr;
 
+static void __nvmet_disc_change_event(struct nvmet_port *port,
+				      struct nvmet_ctrl *ctrl)
+{
+	if (ctrl->port != port)
+		return;
+
+	if (nvmet_aen_bit_disabled(ctrl, NVME_AEN_BIT_DISC_CHANGE))
+		return;
+
+	nvmet_add_async_event(ctrl, NVME_AER_TYPE_NOTICE,
+			      NVME_AER_NOTICE_DISC_CHANGED, NVME_LOG_DISC);
+}
+
+void nvmet_port_disc_change_event(struct nvmet_port *port,
+				  struct nvmet_subsys *subsys)
+{
+	struct nvmet_ctrl *ctrl;
+
+	list_for_each_entry(ctrl, &nvmet_disc_subsys->ctrls, subsys_entry) {
+		if (subsys && !nvmet_host_allowed(subsys, ctrl->hostnqn))
+			continue;
+
+		__nvmet_disc_change_event(port, ctrl);
+	}
+}
+
+static void __nvmet_subsys_disc_change_event(struct nvmet_port *port,
+					     struct nvmet_subsys *subsys,
+					     struct nvmet_host *host)
+{
+	struct nvmet_ctrl *ctrl;
+
+	list_for_each_entry(ctrl, &nvmet_disc_subsys->ctrls, subsys_entry) {
+		if (host && strcmp(nvmet_host_name(host), ctrl->hostnqn))
+			continue;
+
+		__nvmet_disc_change_event(port, ctrl);
+	}
+}
+
+void nvmet_subsys_disc_change_event(struct nvmet_subsys *subsys,
+				    struct nvmet_host *host)
+{
+	struct nvmet_port *port;
+	struct nvmet_subsys_link *s;
+
+	list_for_each_entry(port, nvmet_ports, global_entry)
+		list_for_each_entry(s, &port->subsystems, entry) {
+			if (s->subsys != subsys)
+				continue;
+			__nvmet_subsys_disc_change_event(port, subsys, host);
+		}
+}
+
 void nvmet_referral_enable(struct nvmet_port *parent, struct nvmet_port *port)
 {
 	down_write(&nvmet_config_sem);
@@ -27,6 +81,7 @@ void nvmet_referral_enable(struct nvmet_port *parent, struct nvmet_port *port)
 		list_add_tail(&port->entry, &parent->referrals);
 		port->enabled = true;
 		nvmet_genctr++;
+		nvmet_port_disc_change_event(parent, NULL);
 	}
 	up_write(&nvmet_config_sem);
 }
@@ -38,6 +93,7 @@ void nvmet_referral_disable(struct nvmet_port *parent, struct nvmet_port *port)
 		port->enabled = false;
 		list_del_init(&port->entry);
 		nvmet_genctr++;
+		nvmet_port_disc_change_event(parent, NULL);
 	}
 	up_write(&nvmet_config_sem);
 }
@@ -136,6 +192,8 @@ static void nvmet_execute_get_disc_log_page(struct nvmet_req *req)
 	hdr->numrec = cpu_to_le64(numrec);
 	hdr->recfmt = cpu_to_le16(0);
 
+	nvmet_clear_aen_bit(req, NVME_AEN_BIT_DISC_CHANGE);
+
 	up_read(&nvmet_config_sem);
 
 	status = nvmet_copy_to_sgl(req, 0, hdr, data_len);
@@ -174,6 +232,8 @@ static void nvmet_execute_identify_disc_ctrl(struct nvmet_req *req)
 	if (req->port->inline_data_size)
 		id->sgls |= cpu_to_le32(1 << 20);
 
+	id->oaes = cpu_to_le32(NVMET_DISC_AEN_CFG_OPTIONAL);
+
 	strcpy(id->subnqn, ctrl->subsys->subsysnqn);
 
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 9bb546d..3f5651f 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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);
 
-- 
1.8.3.1

[PATCH v2 11/11] nvmet: Enable Discovery Controller AENs

[Christoph Hellwig]

>  	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.