[PATCH 0/1] NVMe-FC autoconnect, second try

[PATCH 0/1] NVMe-FC autoconnect, second try

[Hannes Reinecke]

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(-)

-- 
1.8.5.6

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

[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>
---
 drivers/nvme/host/fc.c | 63 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 113c30b..fb95933 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -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);
-- 
1.8.5.6

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

[James Smart]

On 11/3/2017 8:19 AM, Hannes Reinecke wrote:
> 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>

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

[Christoph Hellwig]

> +	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

[Hannes Reinecke]

On 11/07/2017 08:25 PM, Christoph Hellwig wrote:
>> +	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?
> 
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
-- 
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

[Christoph Hellwig]

On Wed, Nov 08, 2017@05:31:23PM +0100, Hannes Reinecke wrote:
> On 11/07/2017 08:25 PM, Christoph Hellwig wrote:
> >> +	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?
> > 
> 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.