nvme_fc: add uevent to allow dynamic connects
Subsystems (3)
v3 cover nvme_fc: add uevent to allow dynamic connects
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(-)
Signature · 1 line
2.13.1
PATCH v3 1/3 nvme_fc: create fc class and transport device
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.
---
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(-)
drivers/nvme/host/fc.c +54 -1
@@ -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);
Signature · 1 line
2.13.1
PATCH v3 1/3 nvme_fc: create fc class and transport device
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..
PATCH v3 1/3 nvme_fc: create fc class and transport device
Quoted 5 lines · 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..
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>
Signature · 6 lines
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
Quoted 2 lines · Johannes Thumshirn
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.
Quoted 2 lines
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
Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Johannes Thumshirn
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
Signature · 6 lines
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
In reply to James SmartTo 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.
---
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(+)
drivers/nvme/host/fc.c +49 -0
@@ -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 **************************** */
include/linux/nvme-fc-driver.h +2 -0
@@ -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); +/*
Signature · 1 line
2.13.1
PATCH v3 2/3 nvme_fc: add uevent for auto-connect
Looks fine,
PATCH v3 2/3 nvme_fc: add uevent for auto-connect
Quoted 4 lines · Christoph Hellwig
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
Quoted 8 lines · James Smart
Quoted 4 lines · Christoph Hellwig
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
In reply to James SmartRescans 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.
---
v3:
this patch created by content movement
slight mod for uuid type
drivers/nvme/host/fc.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
drivers/nvme/host/fc.c +28 -0
@@ -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 ;
Signature · 1 line
2.13.1
PATCH v3 3/3 nvme_fc: Avoid duplicate associations between same port pairs
Quoted 5 lines · 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>
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
Quoted 8 lines · Christoph Hellwig
Quoted 5 lines · 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>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
Quoted 11 lines · James Smart
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
Quoted 9 lines · Christoph Hellwig
Quoted 8 lines · James Smart
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.
Quoted 8 lines
Quoted 3 lines
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
Sagi Grimberg
maintainerPATCH v3 3/3 nvme_fc: Avoid duplicate associations between same port pairs
Quoted 3 lines
So we're on the same page - I'll add the option for "check for duplicates" to the cli,
Agree its useful to have
Quoted 10 lines
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.
Christoph Hellwig
Appliedv3 cover nvme_fc: add uevent to allow dynamic connects
In reply to James SmartApplied patches 1 and 2 to nvme-4.15.