[PATCH] sdd scsi-ml event wq

[PATCH] sdd scsi-ml event wq

[Mike Christie]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

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

[-- Attachment #2: scsi-ml-event.patch --]
[-- Type: text/plain, Size: 6566 bytes --]

diff -Naurp linux-2.6.7-rc1/drivers/scsi/scsi.c linux-2.6.7-rc1-hotplug/drivers/scsi/scsi.c
--- linux-2.6.7-rc1/drivers/scsi/scsi.c	2004-05-27 00:36:22.000000000 -0700
+++ linux-2.6.7-rc1-hotplug/drivers/scsi/scsi.c	2004-05-27 20:46:30.000000000 -0700
@@ -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_lib.c linux-2.6.7-rc1-hotplug/drivers/scsi/scsi_lib.c
--- linux-2.6.7-rc1/drivers/scsi/scsi_lib.c	2004-05-09 19:32:37.000000000 -0700
+++ linux-2.6.7-rc1-hotplug/drivers/scsi/scsi_lib.c	2004-05-27 23:53:36.000000000 -0700
@@ -15,9 +15,11 @@
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/kobject.h>
 
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_event.h>
 #include "scsi.h"
 
 #include "scsi_priv.h"
@@ -1691,3 +1693,88 @@ scsi_device_resume(struct scsi_device *s
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
+static struct workqueue_struct *scsi_eventd;
+static kmem_cache_t *scsi_event_cache;
+static mempool_t *scsi_event_pool;
+#define EVENT_POOL_SIZE 64
+
+struct scsi_event {
+	struct work_struct work;
+	const char *action;
+	struct kobject *kobj;
+};
+
+int __init scsi_init_event_daemon(void)
+{
+	scsi_eventd = create_workqueue("scsi_event");
+	if (!scsi_eventd)
+		return -ENOMEM;
+
+	scsi_event_cache = kmem_cache_create("scsi_event",
+					     sizeof(struct scsi_event),
+					     0, 0, NULL, NULL);
+	if (!scsi_event_cache)
+		goto destroy_queue;
+
+	scsi_event_pool = mempool_create(EVENT_POOL_SIZE, mempool_alloc_slab,
+					 mempool_free_slab, scsi_event_cache);
+	if (scsi_event_pool)
+		return 0;
+
+	kmem_cache_destroy(scsi_event_cache);
+ destroy_queue:
+	destroy_workqueue(scsi_eventd);
+
+	return -ENOMEM;
+}
+
+void scsi_shutdown_event_daemon(void)
+{
+	mempool_destroy(scsi_event_pool);
+	kmem_cache_destroy(scsi_event_cache);
+	destroy_workqueue(scsi_eventd);
+}
+
+static void __scsi_hotplug_event(void *data)
+{
+	struct scsi_event *event = data;
+
+	kobject_hotplug(event->action, event->kobj);
+	kobject_put(event->kobj);
+	mempool_free(event, scsi_event_pool);
+}
+
+/**
+ *	scsi_hotplug_event - raises a userspace event.
+ *	@kobj: kobject from scsi data structure.
+ *	@action: the event type.
+ *
+ *	Sends an event through the kobject infrastructure.
+ *	returns 0 for success or an -Exxx value on error.
+ **/
+int scsi_hotplug_event(struct kobject *kobj, const char *action)
+{
+	struct scsi_event *event;
+
+	if (!kobj || !action)
+		return -EINVAL;
+
+	/* this handle is released in __scsi_hotplug_event */
+	if (!kobject_get(kobj))
+		return -EINVAL;
+
+	event = mempool_alloc(scsi_event_pool, GFP_ATOMIC);
+	if (!event) {
+		kobject_put(kobj);
+		return -ENOMEM;
+	}
+
+	event->kobj = kobj;
+	event->action = action;
+	INIT_WORK(&event->work, __scsi_hotplug_event, event);
+
+	queue_work(scsi_eventd, &event->work);
+
+	return 0;
+}
+EXPORT_SYMBOL(scsi_hotplug_event);
diff -Naurp linux-2.6.7-rc1/drivers/scsi/scsi_priv.h linux-2.6.7-rc1-hotplug/drivers/scsi/scsi_priv.h
--- linux-2.6.7-rc1/drivers/scsi/scsi_priv.h	2004-05-09 19:32:53.000000000 -0700
+++ linux-2.6.7-rc1-hotplug/drivers/scsi/scsi_priv.h	2004-05-27 21:09:48.000000000 -0700
@@ -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
--- linux-2.6.7-rc1/drivers/scsi/scsi_transport_fc.c	2004-05-27 23:31:31.000000000 -0700
+++ linux-2.6.7-rc1-hotplug/drivers/scsi/scsi_transport_fc.c	2004-05-27 22:24:53.000000000 -0700
@@ -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
--- linux-2.6.7-rc1/include/scsi/scsi_event.h	1969-12-31 16:00:00.000000000 -0800
+++ linux-2.6.7-rc1-hotplug/include/scsi/scsi_event.h	2004-05-27 23:53:19.000000000 -0700
@@ -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
--- linux-2.6.7-rc1/include/scsi/scsi_transport_fc.h	2004-05-09 19:32:27.000000000 -0700
+++ linux-2.6.7-rc1-hotplug/include/scsi/scsi_transport_fc.h	2004-05-27 22:02:35.000000000 -0700
@@ -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 */

Re: [PATCH] sdd scsi-ml event wq

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On Fri, 2004-05-28 at 09:59, Mike Christie wrote:
> 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.

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)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] sdd scsi-ml event wq

[Mike Christie]

Arjan van de Ven wrote:
> On Fri, 2004-05-28 at 09:59, Mike Christie wrote:
> 
>>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.
> 
> 
> question: it seems you allow only a limited number of outstanding
> events, fair enough. 

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

Re: [PATCH] sdd scsi-ml event wq

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

On Fri, May 28, 2004 at 02:07:03AM -0700, Mike Christie wrote:
> 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.



[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] sdd scsi-ml event wq

[Mike Christie]

Arjan van de Ven wrote:
> On Fri, May 28, 2004 at 02:07:03AM -0700, Mike Christie wrote:
> 
>>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".

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

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

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?

Re: [PATCH] sdd scsi-ml event wq

[Martin Peschke3]

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

I am not sure for what other events this interface is going to be used.
However, for link down/up events there might be a feasible solution.
The minimum requirement is to ensure that paths are re-enabled on
link-up. If we use a flag in scsi_device for the link status, a flag in
scsi_device that indicates whether an event could be propagated,
and one spare buffer for an event then there should always be a way
to report a link-up event. If the spare buffer is being used when a
link-up event occurs, just set the scsi_device->linkup flag as well as
the scsi_device->lostevent flag and report the event later when the
spare buffer has been freed, provided that scsi_device->lostevent
was set. Which means that we might skip a few events in case of a
rapidly changing link status. But we would always report - with a minor
delay - the last, and therewith most important link event.

Martin

Re: [PATCH] sdd scsi-ml event wq

[Douglas Gilbert]

Mike Christie wrote:

[snip]

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

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