Skip to contentSkip to conversation

NVMe-FC autoconnect, second try

PATCH Superseded Hannes Reinecke 3 participants 6 messages Nov 3, 2017 – Nov 8, 2017

Subsystems (2)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

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
#1 lore raw

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.

Signed-off-by Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/fc.c | 63 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 9 deletions(-)
drivers/nvme/host/fc.c +54 -9
index 113c30b..fb95933 100644
@@ -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
#2 lore raw

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.

Signed-off-by James Smart <james.smart at broadcom.com>
#3 lore raw

PATCH 1/1 nvme/fc: add 'discovery' sysfs attribute to fc transport device

In reply to Hannes Reinecke

drivers/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?

#4 lore raw

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)
#5 lore raw

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 calling nvme_fc_rport_put(). And I'm somewhat reluctant to use list_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.