[PATCH v2 0/2] nvme_fc: add uevent to allow dynamic connects

[PATCH v2 0/2] nvme_fc: add uevent to allow dynamic connects

[James Smart]

These patches add support for generating a uevent which can be
caugth and converted to a nvme-cli connect-all request for the
FC initiator and target ports.

The patches were cut on nvme-4.12 after apply the patches contained in
http://lists.infradead.org/pipermail/linux-nvme/2017-May/010156.html

James Smart (2):
  nvme_fc: create fc class and transport device
  nvme_fc: add uevent for auto-connect

 drivers/nvme/host/fc.c         | 102 +++++++++++++++++++++++++++++++++++++++--
 include/linux/nvme-fc-driver.h |   2 +
 2 files changed, 100 insertions(+), 4 deletions(-)

-- 
2.11.0

[PATCH v2 1/2] nvme_fc: create fc class and transport device

[James Smart]

Added a new fc class, and a device node for the nvme_fc transport
under it. I expect the fc class will eventually be the location the
SCSI and FC transports merge in the future.

Signed-off-by: James Smart <james.smart at broadcom.com>
---
v2: cut on nvme-4.12

 drivers/nvme/host/fc.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 8dd06e97736d..4ae99d546378 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -217,6 +217,9 @@ static DEFINE_IDA(nvme_fc_ctrl_cnt);
 
 static struct workqueue_struct *nvme_fc_wq;
 
+static struct class *fc_class;
+static struct device *nvmefc_device;
+
 
 
 /* *********************** FC-NVME Port Management ************************ */
@@ -2959,17 +2962,38 @@ static int __init nvme_fc_init_module(void)
 {
 	int ret;
 
+	fc_class = class_create(THIS_MODULE, "fc");
+	if (IS_ERR(fc_class)) {
+		pr_err("couldn't register class fc\n");
+		return PTR_ERR(fc_class);
+	}
+
+	nvmefc_device = device_create(fc_class, NULL, MKDEV(0, 0), NULL,
+				"nvme_fc_transport");
+	if (IS_ERR(nvmefc_device)) {
+		pr_err("couldn't create nvme_fc device!\n");
+		ret = PTR_ERR(nvmefc_device);
+		goto out_destroy_class;
+	}
+
 	nvme_fc_wq = create_workqueue("nvme_fc_wq");
-	if (!nvme_fc_wq)
-		return -ENOMEM;
+	if (!nvme_fc_wq) {
+		ret = -ENOMEM;
+		goto out_destroy_device;
+	}
 
 	ret = nvmf_register_transport(&nvme_fc_transport);
 	if (ret)
-		goto err;
+		goto out_destroy_workqueue;
 
 	return 0;
-err:
+
+out_destroy_workqueue:
 	destroy_workqueue(nvme_fc_wq);
+out_destroy_device:
+	device_destroy(fc_class, MKDEV(0, 0));
+out_destroy_class:
+	class_destroy(fc_class);
 	return ret;
 }
 
@@ -2985,6 +3009,9 @@ static void __exit nvme_fc_exit_module(void)
 
 	ida_destroy(&nvme_fc_local_port_cnt);
 	ida_destroy(&nvme_fc_ctrl_cnt);
+
+	device_destroy(fc_class, MKDEV(0, 0));
+	class_destroy(fc_class);
 }
 
 module_init(nvme_fc_init_module);
-- 
2.11.0

[PATCH v2 1/2] nvme_fc: create fc class and transport device

[Christoph Hellwig]

On Sat, May 13, 2017@12:05:58PM -0700, James Smart wrote:
> Added a new fc class, and a device node for the nvme_fc transport
> under it. I expect the fc class will eventually be the location the
> SCSI and FC transports merge in the future.

If we chose a nvme name for it that pretty much keeps us from doing
that.  It would seem a bit silly to create a new module for just these
few lines, but I think we should keep the name neutral and document the
purpose with extensive comments.  And maybe (just maybe) move it into a
separate source file.

[PATCH v2 2/2] nvme_fc: add uevent for auto-connect

[James Smart]

To support auto-connecting to FC-NVME devices upon their dynamic
appearance, add a uevent that can kick off connection scripts.
uevent is posted against the nvme_fc transport device.

Added checking to stop the "feature" of connecting to the same
subsystem multiple times on FC devices.

Added routine nvme_fc_rescan_remoteport() to allow lldd to request
nvme scan. For example, lldd may invoke this after receiving an
RSCN for a logged in device containing a discovery controller.

Tested with the following rule to kick an nvme-cli connect-all for the
FC initiator and FC target ports. This is just an example for testing
and not intended for real life use.

ACTION=="change", SUBSYSTEM=="fc", ENV{FC_EVENT}=="nvmediscovery", \
        ENV{NVMEFC_HOST_TRADDR}=="*", ENV{NVMEFC_TRADDR}=="*", \
	RUN+="/bin/sh -c '/usr/local/sbin/nvme connect-all --transport=fc --host-traddr=$env{NVMEFC_HOST_TRADDR} --traddr=$env{NVMEFC_TRADDR} >> /tmp/nvme_fc.log'"

Signed-off-by: James Smart <james.smart at broadcom.com>
---
v2: changed syntax of event to specify a fc event type with FC_EVENT
  variable, and independent variables for host_traddr and traddr.
  Added nvme_fc_rescan_remoteport().

 drivers/nvme/host/fc.c         | 67 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/nvme-fc-driver.h |  2 ++
 2 files changed, 69 insertions(+)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 4ae99d546378..9a594ea323e8 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -392,6 +392,26 @@ nvme_fc_unregister_localport(struct nvme_fc_local_port *portptr)
 }
 EXPORT_SYMBOL_GPL(nvme_fc_unregister_localport);
 
+static void
+nvme_fc_signal_discovery_scan(struct nvme_fc_lport *lport,
+		struct nvme_fc_rport *rport)
+{
+	char hostaddr[80];	/* NVMEFC_HOST_TRADDR=...*/
+	char tgtaddr[80];	/* NVMEFC_TRADDR=...*/
+	char *envp[4] = { "FC_EVENT=nvmediscovery", hostaddr, tgtaddr, NULL };
+
+	if (!(rport->remoteport.port_role & FC_PORT_ROLE_NVME_DISCOVERY))
+		return;
+
+	snprintf(hostaddr, sizeof(hostaddr),
+		"NVMEFC_HOST_TRADDR=nn-0x%016llx:pn-0x%016llx",
+		lport->localport.node_name, lport->localport.port_name);
+	snprintf(tgtaddr, sizeof(tgtaddr),
+		"NVMEFC_TRADDR=nn-0x%016llx:pn-0x%016llx",
+		rport->remoteport.node_name, rport->remoteport.port_name);
+	kobject_uevent_env(&nvmefc_device->kobj, KOBJ_CHANGE, envp);
+}
+
 /**
  * nvme_fc_register_remoteport - transport entry point called by an
  *                              LLDD to register the existence of a NVME
@@ -456,6 +476,8 @@ nvme_fc_register_remoteport(struct nvme_fc_local_port *localport,
 	list_add_tail(&newrec->endp_list, &lport->endp_list);
 	spin_unlock_irqrestore(&nvme_fc_lock, flags);
 
+	nvme_fc_signal_discovery_scan(lport, newrec);
+
 	*portptr = &newrec->remoteport;
 	return 0;
 
@@ -574,6 +596,23 @@ nvme_fc_unregister_remoteport(struct nvme_fc_remote_port *portptr)
 }
 EXPORT_SYMBOL_GPL(nvme_fc_unregister_remoteport);
 
+/**
+ * nvme_fc_rescan_remoteport - transport entry point called by an
+ *                              LLDD to request a nvme device rescan.
+ * @remoteport: pointer to the (registered) remote port that is to be
+ *              rescanned.
+ *
+ * Returns: N/A
+ */
+void
+nvme_fc_rescan_remoteport(struct nvme_fc_remote_port *remoteport)
+{
+	struct nvme_fc_rport *rport = remoteport_to_rport(remoteport);
+
+	nvme_fc_signal_discovery_scan(rport->lport, rport);
+}
+EXPORT_SYMBOL_GPL(nvme_fc_rescan_remoteport);
+
 
 /* *********************** FC-NVME DMA Handling **************************** */
 
@@ -2699,6 +2738,19 @@ static const struct blk_mq_ops nvme_fc_admin_mq_ops = {
 };
 
 
+static inline bool
+__nvme_fc_options_match(struct nvmf_ctrl_options *opts,
+			struct nvme_fc_ctrl *ctrl)
+{
+	if (strcmp(opts->subsysnqn, ctrl->ctrl.opts->subsysnqn) ||
+	    strcmp(opts->host->nqn, ctrl->ctrl.opts->host->nqn) ||
+	    memcmp(&opts->host->id, &ctrl->ctrl.opts->host->id,
+				sizeof(uuid_be)))
+		return false;
+
+	return true;
+}
+
 static struct nvme_ctrl *
 nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	struct nvme_fc_lport *lport, struct nvme_fc_rport *rport)
@@ -2706,6 +2758,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	struct nvme_fc_ctrl *ctrl;
 	unsigned long flags;
 	int ret, idx;
+	bool found = false;
 
 	if (!(rport->remoteport.port_role &
 	    (FC_PORT_ROLE_NVME_DISCOVERY | FC_PORT_ROLE_NVME_TARGET))) {
@@ -2713,6 +2766,20 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 		goto out_fail;
 	}
 
+	spin_lock_irqsave(&rport->lock, flags);
+	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
+		if (__nvme_fc_options_match(opts, ctrl)) {
+			found = true;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&rport->lock, flags);
+
+	if (found) {
+		ret = -EALREADY;
+		goto out_fail;
+	}
+
 	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
 	if (!ctrl) {
 		ret = -ENOMEM;
diff --git a/include/linux/nvme-fc-driver.h b/include/linux/nvme-fc-driver.h
index 492e9f402223..c42313f9a451 100644
--- a/include/linux/nvme-fc-driver.h
+++ b/include/linux/nvme-fc-driver.h
@@ -446,6 +446,8 @@ int nvme_fc_register_remoteport(struct nvme_fc_local_port *localport,
 
 int nvme_fc_unregister_remoteport(struct nvme_fc_remote_port *remoteport);
 
+void nvme_fc_rescan_remoteport(struct nvme_fc_remote_port *remoteport);
+
 
 
 /*
-- 
2.11.0

[PATCH v2 2/2] nvme_fc: add uevent for auto-connect

[Christoph Hellwig]

> +nvme_fc_signal_discovery_scan(struct nvme_fc_lport *lport,
> +		struct nvme_fc_rport *rport)
> +{
> +	char hostaddr[80];	/* NVMEFC_HOST_TRADDR=...*/
> +	char tgtaddr[80];	/* NVMEFC_TRADDR=...*/
> +	char *envp[4] = { "FC_EVENT=nvmediscovery", hostaddr, tgtaddr, NULL };
> +
> +	if (!(rport->remoteport.port_role & FC_PORT_ROLE_NVME_DISCOVERY))
> +		return;
> +
> +	snprintf(hostaddr, sizeof(hostaddr),
> +		"NVMEFC_HOST_TRADDR=nn-0x%016llx:pn-0x%016llx",
> +		lport->localport.node_name, lport->localport.port_name);
> +	snprintf(tgtaddr, sizeof(tgtaddr),
> +		"NVMEFC_TRADDR=nn-0x%016llx:pn-0x%016llx",
> +		rport->remoteport.node_name, rport->remoteport.port_name);
> +	kobject_uevent_env(&nvmefc_device->kobj, KOBJ_CHANGE, envp);

Please use kasprintf so that we have a dynamic allocation and don't
need to hardcode buffer sizes.

> +	spin_lock_irqsave(&rport->lock, flags);
> +	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
> +		if (__nvme_fc_options_match(opts, ctrl)) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&rport->lock, flags);
> +
> +	if (found) {
> +		ret = -EALREADY;
> +		goto out_fail;
> +	}

Thus seems neither related to the intent of the patch, nor does it make
sense at all.

[PATCH v2 0/2] nvme_fc: add uevent to allow dynamic connects

[James Smart]

On 5/13/2017 12:05 PM, James Smart wrote:
> These patches add support for generating a uevent which can be
> caugth and converted to a nvme-cli connect-all request for the
> FC initiator and target ports.
>
> The patches were cut on nvme-4.12 after apply the patches contained in
> http://lists.infradead.org/pipermail/linux-nvme/2017-May/010156.html
>
> James Smart (2):
>   nvme_fc: create fc class and transport device
>   nvme_fc: add uevent for auto-connect
>
>  drivers/nvme/host/fc.c         | 102 +++++++++++++++++++++++++++++++++++++++--
>  include/linux/nvme-fc-driver.h |   2 +
>  2 files changed, 100 insertions(+), 4 deletions(-)
>


Any comments on this series ?

-- james

[PATCH v2 0/2] nvme_fc: add uevent to allow dynamic connects

[Christoph Hellwig]

On Sat, May 13, 2017@12:05:57PM -0700, James Smart wrote:
> These patches add support for generating a uevent which can be
> caugth and converted to a nvme-cli connect-all request for the
> FC initiator and target ports.

Can you post the nvme-cli side as well so that it can be reviewed
together?

> 
> The patches were cut on nvme-4.12 after apply the patches contained in
> http://lists.infradead.org/pipermail/linux-nvme/2017-May/010156.html
> 
> James Smart (2):
>   nvme_fc: create fc class and transport device
>   nvme_fc: add uevent for auto-connect
> 
>  drivers/nvme/host/fc.c         | 102 +++++++++++++++++++++++++++++++++++++++--
>  include/linux/nvme-fc-driver.h |   2 +
>  2 files changed, 100 insertions(+), 4 deletions(-)
> 
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
---end quoted text---

[PATCH v2 0/2] nvme_fc: add uevent to allow dynamic connects

[James Smart]

On 5/23/2017 12:17 AM, Christoph Hellwig wrote:
> On Sat, May 13, 2017@12:05:57PM -0700, James Smart wrote:
>> These patches add support for generating a uevent which can be
>> caugth and converted to a nvme-cli connect-all request for the
>> FC initiator and target ports.
>
> Can you post the nvme-cli side as well so that it can be reviewed
> together?

See the patch description in Patch 2.  It has the udev event rule, which 
invokes nvme cli, that was used to test with.

-- james