sdd scsi-ml event wq

The attached patch just adds a scsi-ml work queue to handle all events. It also converts the transport_scsi_fc class to use it, so drivers using that class do not have to worry about calling the event functions from a process context.

The attached patch was built against my previous patch set.

Mike Christie

diff -Naurp linux-2.6.7-rc1/drivers/scsi/scsi.c linux-2.6.7-rc1-hotplug/drivers/scsi/scsi.c
@@ -1193,6 +1193,10 @@ static int __init init_scsi(void)
 	if (error)
 		goto cleanup_sysctl;
 
+	error = scsi_init_event_daemon();
+	if (error)
+		goto cleanup_sysfs;
+
 	for (i = 0; i < NR_CPUS; i++)
 		INIT_LIST_HEAD(&per_cpu(scsi_done_q, i));
 

@@ -1202,6 +1206,8 @@ static int __init init_scsi(void)
 	printk(KERN_NOTICE "SCSI subsystem initialized\n");
 	return 0;
 
+cleanup_sysfs:
+	scsi_sysfs_unregister();
 cleanup_sysctl:
 	scsi_exit_sysctl();
 cleanup_hosts:

@@ -1219,6 +1225,7 @@ cleanup_queue:
 
 static void __exit exit_scsi(void)
 {
+	scsi_shutdown_event_daemon();
 	scsi_sysfs_unregister();
 	scsi_exit_sysctl();
 	scsi_exit_hosts();

diff -Naurp linux-2.6.7-rc1/drivers/scsi/scsi_priv.h linux-2.6.7-rc1-hotplug/drivers/scsi/scsi_priv.h
@@ -116,6 +116,8 @@ extern struct request_queue *scsi_alloc_
 extern void scsi_free_queue(struct request_queue *q);
 extern int scsi_init_queue(void);
 extern void scsi_exit_queue(void);
+extern int scsi_init_event_daemon(void);
+extern void scsi_shutdown_event_daemon(void);
 
 /* scsi_proc.c */
 #ifdef CONFIG_SCSI_PROC_FS

diff -Naurp linux-2.6.7-rc1/drivers/scsi/scsi_transport_fc.c linux-2.6.7-rc1-hotplug/drivers/scsi/scsi_transport_fc.c
@@ -22,6 +22,7 @@
 #include <linux/kobject.h>
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_event.h>
 #include <scsi/scsi_transport.h>
 #include <scsi/scsi_transport_fc.h>
 

@@ -77,28 +78,30 @@ static void __exit fc_transport_exit(voi
  *
  * Sends a hotplug event to userspace utilizing the
  * kobject infrastructure.
+ *
+ * returns 0 for success or an -Exxx value on error.
  */
-void fc_link_up(struct Scsi_Host *shost)
+int fc_link_up(struct Scsi_Host *shost)
 {
 	struct kobject *kobj;
 
 	if (!shost)
-		return;
+		return -EINVAL;
 
 	kobj = &shost->transport_classdev.kobj;
-	kobject_hotplug("fc_link_up", kobj);
+	return scsi_hotplug_event(kobj, "fc_link_up");
 }
 EXPORT_SYMBOL(fc_link_up);
 
-void fc_link_down(struct Scsi_Host *shost)
+int fc_link_down(struct Scsi_Host *shost)
 {
 	struct kobject *kobj;
 
 	if (!shost)
-		return;
+		return -EINVAL;
 
 	kobj = &shost->transport_classdev.kobj;
-	kobject_hotplug("fc_link_down", kobj);
+	return scsi_hotplug_event(kobj, "fc_link_down");
 }
 EXPORT_SYMBOL(fc_link_down);
 

diff -Naurp linux-2.6.7-rc1/include/scsi/scsi_event.h linux-2.6.7-rc1-hotplug/include/scsi/scsi_event.h
@@ -0,0 +1,8 @@
+#ifndef _SCSI_SCSI_EVENT_H
+#define _SCSI_SCSI_EVENT_H
+
+#include <linux/kobject.h>
+
+int scsi_hotplug_event(struct kobject *, const char *);
+
+#endif

diff -Naurp linux-2.6.7-rc1/include/scsi/scsi_transport_fc.h linux-2.6.7-rc1-hotplug/include/scsi/scsi_transport_fc.h
@@ -52,5 +52,7 @@ struct fc_function_template {
 
 struct scsi_transport_template *fc_attach_transport(struct fc_function_template *);
 void fc_release_transport(struct scsi_transport_template *);
+int fc_link_up(struct Scsi_Host *);
+int fc_link_up(struct Scsi_Host *);
 
 #endif /* SCSI_TRANSPORT_FC_H */

question: it seems you allow only a limited number of outstanding events, fair enough. However, how are users of this API supposed to handle failure? I see you pass the error nicely all the way down, yet I don't quite understand how a driver (the consumer of the API) is supposed to handle failure. Queue the event itself? Sounds ugly/wrong to me. I'm just wondering if the cleanup you propose doesn't just mean that the uglyness gets pushed to correct users of the API.. which isn't quite a nett win (under the assumption that there are more than one users of a subsystem function)

I am not sure I know what you mean. You are referring to when memory allocations fail, right?

However, how are users of this API supposed to

I agree with you, and I do not have a good answer. When the system is failing memory allocations it is difficult to do a lot of things. I was thinking that in such an extreme situation that it would be acceptable for events to fail. Would it be better to have the API set a timer and retry at a later time? I was thinking you could end up with a long backlog of events that may be worthless by the time the system can allocate memory - which is why I put in the mempool (I know that they are not magic and will fail too though). I am open to ideas.

well what would suck is if userspace only sees a last "loop down" event but then never sees the final "loop up".

The problem is that the memory allocation issue is more critical here than in typical code, simply because on loop down you may not be able to do IO to your swap device/filesystem to free memory.

Maybe we need to do a spare allocated event that is ONLY used for "eh the queue overflowed, assume all state is lost, rediscover everything". That way userland CAN know about the loss of state, and perform full discovery/recovery.

I think the kobject hotplug code will already drop events without indication that it failed :( Look at kset_hotplug().

Even if we had the spare event, the kobject hotplug code is kmallocing memory with GFP_KERNEL and will probably fail if we are in a situation where memory is so tight that we went to our event mempool and those reserves failed. I am sure that could all be fixed, but we would need to extend a priority to the hotplug code so it knows that this event is special (Well, you would also have to go through __call_usermodehelper which could fail too).

For the short term would it be best to harden the SCSI side of the API then worry about the API we are using, or is the kobject_hotplug/userspace route not looking to be the best option?

Mike Christie wrote:

[snip]

Passing state change information out to the userspace via hotplug and sysfs has problems which are exacerbated by the sysfs policy of one datapoint per node. If there were multiple state changes in succession then a userspace program that reads several sysfs variables could be getting inconsistent state.

Persistent reservations in SPC-3 handle a similar problem by having a "PRgeneration" variable which is a 32 bit counter that is incremented every time there is a (registration) state change.

Could such an approach (i.e. an appropriately placed "generation" state change counter in sysfs) be useful for the hotplug/userspace route?

Doug Gilbert