NVMe-FC autoconnect, second try
Subsystems (2)
cover NVMe-FC autoconnect, second try
Hi all,
here's my attempt a NVMe-FC autoconnect.
Primarily it's just a patch adding a 'discovery' sysfs attribute to /sys/class/fc/nvme_fc_transport.
Writing '1' (or anything) into it will cause nvme-fc to resend uevents for every known host_traddr:traddr combination.
With that we can then add the simple udev rule:
SUBSYSTEM!="block, GOTO="nvme_autoconnect_end"
ACTION!="add|change", GOTO="nvme_autoconnect_end"
ENV{FC_EVENT}!="nvmediscovery", GOTO="nvme_autoconnect_end" ENV{NVMEFC_HOST_TRADDR}!="?*", GOTO="nvme_autoconnect_end"
ENV{NVMEFC_TRADDR}!="?*", GOTO="nvme_autoconnect_end"
RUN+="/usr/sbin/nvme connect-all -t fc --host-traddr=$env{NVMEFC_HOST_TRADDR} -a $env{NVMEFC_TRADDR}" LABEL="nvme_autoconnect_end"
to facilitate autodiscovery.
This does not address the potential stall in nvme connect-all that James mentioned, but I do think that should be fixed via other means.
Hannes Reinecke (1):
nvme/fc: add 'discovery' sysfs attribute to fc transport device
drivers/nvme/host/fc.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 9 deletions(-)
Signature · 1 line
1.8.5.6
PATCH 1/1 nvme/fc: add 'discovery' sysfs attribute to fc transport device
The fc transport device should allow for a rediscovery, as userspace might have lost the events.
So add a sysfs entry 'discovery' to trigger the discover uevents.
--- drivers/nvme/host/fc.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 9 deletions(-)
drivers/nvme/host/fc.c +54 -9
@@ -219,7 +219,6 @@ struct nvme_fc_ctrl {* 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;
@@ -3275,6 +3274,52 @@ struct nvmet_fc_traddr { .create_ctrl = nvme_fc_create_ctrl, }; +static ssize_t nvme_fc_discovery_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +unsigned long flags; +struct nvme_fc_lport *lport; +struct nvme_fc_rport *rport, *tmp_rport; + +spin_lock_irqsave (&nvme_fc_lock, flags); + +list_for_each_entry (lport, &nvme_fc_lport_list, port_list) { +list_for_each_entry_safe (rport, tmp_rport, + &lport->endp_list, endp_list) { +if (!nvme_fc_rport_get (rport)) +continue ; +spin_unlock_irqrestore (&nvme_fc_lock, flags); +nvme_fc_signal_discovery_scan (lport, rport); +nvme_fc_rport_put (rport); +spin_lock_irqsave (&nvme_fc_lock, flags); + } + } +spin_unlock_irqrestore (&nvme_fc_lock, flags); +return count; +} +static DEVICE_ATTR(discovery, S_IWUSR, NULL, nvme_fc_discovery_store); + +static struct attribute *nvme_fc_attrs[] = { + &dev_attr_discovery.attr, +NULL +}; + +static struct attribute_group nvme_fc_attr_group = { + .attrs = nvme_fc_attrs, +}; + +static const struct attribute_group *nvme_fc_attr_groups[] = { + &nvme_fc_attr_group, +NULL +}; + +static struct class fc_class = { + .name ="fc" , + .dev_groups = nvme_fc_attr_groups, + .owner =THIS_MODULE , +}; +static int __init nvme_fc_init_module (void ) {int ret;
@@ -3293,16 +3338,16 @@ static int __init nvme_fc_init_module(void)* 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)) { + ret =class_register (&fc_class); +if (ret) {pr_err ("couldn't register class fc\n" ); -return PTR_ERR (fc_class); +return ret; }/* * Create a device for the FC-centric udev events */ - fc_udev_device =device_create (fc_class,NULL ,MKDEV (0 ,0 ),NULL , + 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" );
@@ -3317,9 +3362,9 @@ static int __init nvme_fc_init_module(void)return 0 ; out_destroy_device: -device_destroy (fc_class,MKDEV (0 ,0 )); +device_destroy (&fc_class,MKDEV (0 ,0 )); out_destroy_class: -class_destroy (fc_class); +class_unregister (&fc_class);return ret; }
@@ -3334,8 +3379,8 @@ 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); +device_destroy (&fc_class,MKDEV (0 ,0 )); +class_unregister (&fc_class); }module_init (nvme_fc_init_module);
Signature · 1 line
1.8.5.6
PATCH 1/1 nvme/fc: add 'discovery' sysfs attribute to fc transport device
Quoted 7 lines · Hannes Reinecke
The fc transport device should allow for a rediscovery, as userspace might have lost the events.
So add a sysfs entry 'discovery' to trigger the discover uevents.Signed-off-by Hannes Reinecke <hare at suse.com>---
Ok - so I like this as it removes the lpfc-specific script that was there to deal with pre-udev-present events that weren't replayed and makes it a simple script that access this new attribute. It does not change the content or need for the other scripts that service the udev event and starts the systemd script to make makes the nvme connect-all call.
PATCH 1/1 nvme/fc: add 'discovery' sysfs attribute to fc transport device
In reply to Hannes Reineckedrivers/nvme/host/fc.c struct nvmet_fc_traddr { · line 3298
Quoted 12 lines
drivers/nvme/host/fc.c +12
-0+list_for_each_entry (lport, &nvme_fc_lport_list, port_list) { +list_for_each_entry_safe (rport, tmp_rport, + &lport->endp_list, endp_list) { +if (!nvme_fc_rport_get (rport)) +continue ; +spin_unlock_irqrestore (&nvme_fc_lock, flags); +nvme_fc_signal_discovery_scan (lport, rport); +nvme_fc_rport_put (rport); +spin_lock_irqsave (&nvme_fc_lock, flags); + } + } +spin_unlock_irqrestore (&nvme_fc_lock, flags);
What guarantees the list next pointers are still valid after you dropped nvme_fc_lock?
PATCH 1/1 nvme/fc: add 'discovery' sysfs attribute to fc transport device
Quoted 7 lines · Christoph Hellwig
Quoted 7 lines
drivers/nvme/host/fc.c +7
-0+list_for_each_entry (lport, &nvme_fc_lport_list, port_list) { +list_for_each_entry_safe (rport, tmp_rport, + &lport->endp_list, endp_list) { +if (!nvme_fc_rport_get (rport)) +continue ; +spin_unlock_irqrestore (&nvme_fc_lock, flags); +nvme_fc_signal_discovery_scan (lport, rport);
Quoted 9 lines
drivers/nvme/host/fc.c struct nvmet_fc_traddr { · line 3298
Quoted 5 lines
drivers/nvme/host/fc.c +5
-0+nvme_fc_rport_put (rport); +spin_lock_irqsave (&nvme_fc_lock, flags); + } + } +spin_unlock_irqrestore (&nvme_fc_lock, flags);What guarantees the list next pointers are still valid after you dropped
nvme_fc_lock?
You know, I wondered about that, too.
But then I cannot hold the lock when calling nvme_fc_rport_put(). And I'm somewhat reluctant to use list_for_each_entry_safe() in the outer loop.
I'll be checking what we can do here.
Cheers,
Hannes
Signature · 5 lines
Dr. Hannes Reinecke Teamlead Storage & Networking hare at suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG N?rnberg)
PATCH 1/1 nvme/fc: add 'discovery' sysfs attribute to fc transport device
Quoted 17 lines · Hannes Reinecke
Quoted 7 lines · Christoph Hellwig
Quoted 7 lines
drivers/nvme/host/fc.c +7
-0+list_for_each_entry (lport, &nvme_fc_lport_list, port_list) { +list_for_each_entry_safe (rport, tmp_rport, + &lport->endp_list, endp_list) { +if (!nvme_fc_rport_get (rport)) +continue ; +spin_unlock_irqrestore (&nvme_fc_lock, flags); +nvme_fc_signal_discovery_scan (lport, rport);Quoted 9 lines
drivers/nvme/host/fc.c struct nvmet_fc_traddr { · line 3298
Quoted 5 lines
drivers/nvme/host/fc.c +5
-0+nvme_fc_rport_put (rport); +spin_lock_irqsave (&nvme_fc_lock, flags); + } + } +spin_unlock_irqrestore (&nvme_fc_lock, flags);What guarantees the list next pointers are still valid after you dropped
nvme_fc_lock?
Quoted 4 lines
You know, I wondered about that, too.
But then I cannot hold the lock when callingnvme_fc_rport_put(). And I'm somewhat reluctant to uselist_for_each_entry_safe()in the outer loop.
list_for_each_entry_safe isn't going to help you with races - it only helps you when you delete the current node.