[PATCH v3 0/3] nvme_fc: add uevent to allow dynamic connects

[PATCH v3 0/3] nvme_fc: add uevent to allow dynamic connects

[James Smart]

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

Also contains a patch to catch connection attempts that duplicate
already-in-existence associations.

Cut on nvme-4.14

James Smart (3):
  nvme_fc: create fc class and transport device
  nvme_fc: add uevent for auto-connect
  nvme_fc: Avoid duplicate associations between same port pairs

 drivers/nvme/host/fc.c         | 132 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/nvme-fc-driver.h |   2 +
 2 files changed, 133 insertions(+), 1 deletion(-)

-- 
2.13.1

[PATCH v3 1/3] nvme_fc: create fc class and transport device

[James Smart]

Added a new fc class and a device node for udev events under it.
I expect the fc class will eventually be the location where the
FC SCSI and FC NVME merge in the future. Therefore names are
kept somewhat generic.

Signed-off-by: James Smart <james.smart at broadcom.com>
---
v3:
  Genericized udev event device name.
  Added comments about future expectations.
  Kept fc class and udev device in the nvme module due to its
     small size. in future, it can move to somewhere else.

 drivers/nvme/host/fc.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index d2e882c0f496..f9e9dddc7e92 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -213,6 +213,13 @@ static DEFINE_IDA(nvme_fc_ctrl_cnt);
 
 
 
+/*
+ * These items are short-term. They will eventually be moved into
+ * a generic FC class. See comments in module init.
+ */
+static struct class *fc_class;
+static struct device *fc_udev_device;
+
 
 /* *********************** FC-NVME Port Management ************************ */
 
@@ -2996,7 +3003,50 @@ static struct nvmf_transport_ops nvme_fc_transport = {
 
 static int __init nvme_fc_init_module(void)
 {
-	return nvmf_register_transport(&nvme_fc_transport);
+	int ret;
+
+	/*
+	 * NOTE:
+	 * It is expected that in the future the kernel will combine
+	 * the FC-isms that are currently under scsi and now being
+	 * added to by NVME into a new standalone FC class. The SCSI
+	 * and NVME protocols and their devices would be under this
+	 * new FC class.
+	 *
+	 * As we need something to post FC-specific udev events to,
+	 * specifically for nvme probe events, start by creating the
+	 * new device class.  When the new standalone FC class is
+	 * put in place, this code will move to a more generic
+	 * location for the class.
+	 */
+	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);
+	}
+
+	/*
+	 * Create a device for the FC-centric udev events
+	 */
+	fc_udev_device = device_create(fc_class, NULL, MKDEV(0, 0), NULL,
+				"fc_udev_device");
+	if (IS_ERR(fc_udev_device)) {
+		pr_err("couldn't create fc_udev device!\n");
+		ret = PTR_ERR(fc_udev_device);
+		goto out_destroy_class;
+	}
+
+	ret = nvmf_register_transport(&nvme_fc_transport);
+	if (ret)
+		goto out_destroy_device;
+
+	return 0;
+
+out_destroy_device:
+	device_destroy(fc_class, MKDEV(0, 0));
+out_destroy_class:
+	class_destroy(fc_class);
+	return ret;
 }
 
 static void __exit nvme_fc_exit_module(void)
@@ -3009,6 +3059,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.13.1

[PATCH v3 1/3] nvme_fc: create fc class and transport device

[Christoph Hellwig]

Looks ok,

although I really wish we could come up with some common FC code,
including making the existing FC drivers use more infrastructure
from libfc..

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH v3 1/3] nvme_fc: create fc class and transport device

[Johannes Thumshirn]

On Mon, Sep 18, 2017@09:13:30AM -0700, Christoph Hellwig wrote:
> Looks ok,
> 
> although I really wish we could come up with some common FC code,
> including making the existing FC drivers use more infrastructure
> from libfc..

the problem with libfc currently is it is made for fcoe, despite being named
libfc. The other hand it is hard-wired to FCP so bringing NVMe into it is a
bit challanging, BTDT.

It's not that it is impossible, it's just not something that can be done in a
day or two of hacking.

Anyways,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH v3 1/3] nvme_fc: create fc class and transport device

[Christoph Hellwig]

On Wed, Sep 20, 2017@12:30:41PM +0200, Johannes Thumshirn wrote:
> the problem with libfc currently is it is made for fcoe, despite being named
> libfc.

Well, that's my point - we should be able to use more bits in a common
FC layer.  I don't really care about the exact name.

> The other hand it is hard-wired to FCP so bringing NVMe into it is a
> bit challanging, BTDT.

FC-NVMe layers on top of FCP.

[PATCH v3 1/3] nvme_fc: create fc class and transport device

[Johannes Thumshirn]

On Wed, Sep 20, 2017@10:50:18AM -0700, Christoph Hellwig wrote:
> On Wed, Sep 20, 2017@12:30:41PM +0200, Johannes Thumshirn wrote:
> > the problem with libfc currently is it is made for fcoe, despite being named
> > libfc.
> 
> Well, that's my point - we should be able to use more bits in a common
> FC layer.  I don't really care about the exact name.

Yes, that's on my long term TODO list, getting rid of the SKBs used internally
so ibmvfc can be switched over, allow the setting of the NVMe Type code (or
even arbitrary typecodes) yada, yada.  It'll benefit fnic, bnx2fc and qedfc as
well, but I haven't been able to do much in this regard. Hannes and I have
internal patchsets flying around solving the fist little bits for it but
they're far from completion.

Byte,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH v3 2/3] 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 fc_udev device.

patch set 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'"

I will post proposed udev/systemd scripts for possible kernel support.

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().
v3:
  Stayed with snprintf over kasprintf. The xxaddr fields, per FC-NVME
    spec, are fixed length, so hardcoding their length is acceptable.
    Also, if allocation was to fail, it potentially causes a
    critical loss in connectivity as the event wouldn't be posted.
    Better to allocate on the stack and ensure the event can be
    posted. Added define and comments on string length.
  Split duplicate connection check into separate patch.

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

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index f9e9dddc7e92..cb0cd4f5c4b5 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -459,6 +459,36 @@ nvme_fc_unregister_localport(struct nvme_fc_local_port *portptr)
 }
 EXPORT_SYMBOL_GPL(nvme_fc_unregister_localport);
 
+/*
+ * TRADDR strings, per FC-NVME are fixed format:
+ *   "nn-0x<16hexdigits>:pn-0x<16hexdigits>" - 43 characters
+ * udev event will only differ by prefix of what field is
+ * being specified:
+ *    "NVMEFC_HOST_TRADDR=" or "NVMEFC_TRADDR=" - 19 max characters
+ *  19 + 43 + null_fudge = 64 characters
+ */
+#define FCNVME_TRADDR_LENGTH		64
+
+static void
+nvme_fc_signal_discovery_scan(struct nvme_fc_lport *lport,
+		struct nvme_fc_rport *rport)
+{
+	char hostaddr[FCNVME_TRADDR_LENGTH];	/* NVMEFC_HOST_TRADDR=...*/
+	char tgtaddr[FCNVME_TRADDR_LENGTH];	/* 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(&fc_udev_device->kobj, KOBJ_CHANGE, envp);
+}
+
 /**
  * nvme_fc_register_remoteport - transport entry point called by an
  *                              LLDD to register the existence of a NVME
@@ -523,6 +553,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;
 
@@ -641,6 +673,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 **************************** */
 
diff --git a/include/linux/nvme-fc-driver.h b/include/linux/nvme-fc-driver.h
index 9c5cb4480806..1af80055053a 100644
--- a/include/linux/nvme-fc-driver.h
+++ b/include/linux/nvme-fc-driver.h
@@ -451,6 +451,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.13.1

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

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch at lst.de>

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

[James Smart]

On 9/18/2017 9:14 AM, Christoph Hellwig wrote:
> Looks fine,
> 
> Reviewed-by: Christoph Hellwig <hch at lst.de>
> 

ok can you merge patches 1 and 2 while I replace patch 3 with something 
that applies to all transports ?

-- james

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

[Christoph Hellwig]

On Tue, Sep 19, 2017@07:53:08AM -0700, James Smart wrote:
> On 9/18/2017 9:14 AM, Christoph Hellwig wrote:
> > Looks fine,
> > 
> > Reviewed-by: Christoph Hellwig <hch at lst.de>
> > 
> 
> ok can you merge patches 1 and 2 while I replace patch 3 with something that
> applies to all transports ?

Sure, I'll add them to the nvme-4.15 branch once that opens.

[PATCH v3 3/3] nvme_fc: Avoid duplicate associations between same port pairs

[James Smart]

Rescans can occur due to subsystem list changes on an FC remoteport.
Rescans may attempt to reconnect to subsystems where there is already
an association in place. There should only be 1 association in place
at a time for the following tuple:
  <hostnqn, hostid, host FC port, target FC port, subnqn>

Catch connection attempts to associations that already exist and
fail the attempts.

Signed-off-by: James Smart <james.smart at broadcom.com>

---
v3:
  this patch created by content movement
  slight mod for uuid type

 drivers/nvme/host/fc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index cb0cd4f5c4b5..e7c1d767fb4c 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2783,6 +2783,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_t)))
+		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)
@@ -2790,6 +2803,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))) {
@@ -2797,6 +2811,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;
-- 
2.13.1

[PATCH v3 3/3] nvme_fc: Avoid duplicate associations between same port pairs

[Christoph Hellwig]

On Thu, Sep 14, 2017@10:38:43AM -0700, James Smart wrote:
> Rescans can occur due to subsystem list changes on an FC remoteport.
> Rescans may attempt to reconnect to subsystems where there is already
> an association in place. There should only be 1 association in place
> at a time for the following tuple:
>   <hostnqn, hostid, host FC port, target FC port, subnqn>

There is absolutely no requirement for this limitation in the NVMe
spec.

[PATCH v3 3/3] nvme_fc: Avoid duplicate associations between same port pairs

[James Smart]

On 9/18/2017 9:12 AM, Christoph Hellwig wrote:
> On Thu, Sep 14, 2017@10:38:43AM -0700, James Smart wrote:
>> Rescans can occur due to subsystem list changes on an FC remoteport.
>> Rescans may attempt to reconnect to subsystems where there is already
>> an association in place. There should only be 1 association in place
>> at a time for the following tuple:
>>    <hostnqn, hostid, host FC port, target FC port, subnqn>
> There is absolutely no requirement for this limitation in the NVMe
> spec.

True. But it makes little sense to allow it as it creates a 2nd 
controller, thus multiple devices for the namespaces, along the same "hw 
path".? I.e. now multipath has 2 nvme devices for the same hw path with 
no changes in availability and consuming more resources on host and target.

The desire for FC is to have the same dynamic discovery that it has for 
SCSI. Thus the udev events that kick off scanning. The events are posted 
when connectivity is first detected as well as conditions where the 
discovery server content may have changed (new subsystems added for 
example).? Since those events are ignorant of what's already connected, 
it make no sense to create additional controllers/devices if there's 
already a controller connected.

The alternative is to drive all connections manually which makes a very 
difficult to manage system for an administrator, especially on 
"cable-pull" scenarios. There is no need for this with FC.

-- james

[PATCH v3 3/3] nvme_fc: Avoid duplicate associations between same port pairs

[Christoph Hellwig]

On Mon, Sep 18, 2017@09:28:35AM -0700, James Smart wrote:
> The desire for FC is to have the same dynamic discovery that it has for
> SCSI. Thus the udev events that kick off scanning. The events are posted
> when connectivity is first detected as well as conditions where the
> discovery server content may have changed (new subsystems added for
> example).? Since those events are ignorant of what's already connected, it
> make no sense to create additional controllers/devices if there's already a
> controller connected.
> 
> The alternative is to drive all connections manually which makes a very
> difficult to manage system for an administrator, especially on "cable-pull"
> scenarios. There is no need for this with FC.

The add a connect options that checks for duplicates instead of making
it the only options.  And please make it a global check for all
fabrics transports.

[PATCH v3 3/3] nvme_fc: Avoid duplicate associations between same port pairs

[James Smart]

On 9/18/2017 4:17 PM, Christoph Hellwig wrote:
> On Mon, Sep 18, 2017@09:28:35AM -0700, James Smart wrote:
>> The desire for FC is to have the same dynamic discovery that it has for
>> SCSI. Thus the udev events that kick off scanning. The events are posted
>> when connectivity is first detected as well as conditions where the
>> discovery server content may have changed (new subsystems added for
>> example).? Since those events are ignorant of what's already connected, it
>> make no sense to create additional controllers/devices if there's already a
>> controller connected.
>>
>> The alternative is to drive all connections manually which makes a very
>> difficult to manage system for an administrator, especially on "cable-pull"
>> scenarios. There is no need for this with FC.
> 
> The add a connect options that checks for duplicates instead of making
> it the only options.  And please make it a global check for all
> fabrics transports.
> 

So we're on the same page - I'll add the option for "check for 
duplicates" to the cli, which will pass it on to the kernel, but the 
implementation of the check will be in the kernel in each transport.

By keeping the check in the kernel it will:
- avoid race conditions of what the "active controller" list is between 
user and kernel space
- keep transport-port specific knowledge of what connect fields are 
meaningful in the transport,
- avoids adding any infrastructure to get the connect fields into user 
space if the cli were to do the checking.

-- james

[PATCH v3 3/3] nvme_fc: Avoid duplicate associations between same port pairs

[Sagi Grimberg]

> So we're on the same page - I'll add the option for "check for 
> duplicates" to the cli,

Agree its useful to have

> which will pass it on to the kernel, but the 
> implementation of the check will be in the kernel in each transport.
> 
> By keeping the check in the kernel it will:
> - avoid race conditions of what the "active controller" list is between 
> user and kernel space
> - keep transport-port specific knowledge of what connect fields are 
> meaningful in the transport,
> - avoids adding any infrastructure to get the connect fields into user 
> space if the cli were to do the checking.

Also agree here.

[PATCH v3 0/3] nvme_fc: add uevent to allow dynamic connects

[Christoph Hellwig]

Applied patches 1 and 2 to nvme-4.15.