[PATCH] fix media change events for polled devices

[PATCH] fix media change events for polled devices

[Kay Sievers]

Hi James,
we like to see this in 2.6.25, as "change" events for polled devices are
broken at the moment.

Thanks,
Kay


From: Kay Sievers <kay.sievers@vrfy.org>
Subject: [SCSI] fix media change events for polled devices

Commit:
  a341cd0f (SCSI: add asynchronous event notification API)
breaks:
  285e9670 (sr,sd: send media state change modification events)
by introducing an event filter, which is removed here, to make
events, we are depending on, happen again.

Cc: David Zeuthen <david@fubar.dk>
Cc: kristen accardi <kaccardi@gmail.com>
Cc: Jeff Garzik <jeff@garzik.org>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ba21d97..ad1dc03 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2162,11 +2162,6 @@ void sdev_evt_send(struct scsi_device *sdev, struct scsi_event *evt)
 {
 	unsigned long flags;
 
-	if (!test_bit(evt->evt_type, sdev->supported_events)) {
-		kfree(evt);
-		return;
-	}
-
 	spin_lock_irqsave(&sdev->list_lock, flags);
 	list_add_tail(&evt->node, &sdev->event_list);
 	schedule_work(&sdev->event_work);

Re: [PATCH] fix media change events for polled devices

[James Bottomley]

On Wed, 2008-03-19 at 13:04 +0100, Kay Sievers wrote:
> Hi James,
> we like to see this in 2.6.25, as "change" events for polled devices are
> broken at the moment.

I'm not happy just killing the code, since when the AN infrastructure
gets fixed, it will work.  How about this instead as a fix for 2.6.25?

Also, acks from Jeff and Kristin would be nice.

James

---
>From 4d1566ed2100d074ccc654e5cf2e44cdea3a01d0 Mon Sep 17 00:00:00 2001
From: Kay Sievers <kay.sievers@vrfy.org>
Date: Wed, 19 Mar 2008 13:04:47 +0100
Subject: [SCSI] fix media change events for polled devices

Commit:
  a341cd0f (SCSI: add asynchronous event notification API)
breaks:
  285e9670 (sr,sd: send media state change modification events)
by introducing an event filter, which is removed here, to make
events, we are depending on, happen again.

Fix this by removing the event filter.  It's pretty much broken at the
moment, since a user can't set it (the attribute being read only).  A
proper fix will be to make the event discriminator distinguish between
AN and Polled media change events.

Cc: David Zeuthen <david@fubar.dk>
Cc: kristen accardi <kaccardi@gmail.com>
Cc: Jeff Garzik <jeff@garzik.org>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/scsi_lib.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ba21d97..f40898d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2162,10 +2162,15 @@ void sdev_evt_send(struct scsi_device *sdev, struct scsi_event *evt)
 {
 	unsigned long flags;
 
+#if 0
+	/* FIXME: currently this check eliminates all media change events
+	 * for polled devices.  Need to update to discriminate between AN
+	 * and polled events */
 	if (!test_bit(evt->evt_type, sdev->supported_events)) {
 		kfree(evt);
 		return;
 	}
+#endif
 
 	spin_lock_irqsave(&sdev->list_lock, flags);
 	list_add_tail(&evt->node, &sdev->event_list);
-- 
1.5.4.1

Re: [PATCH] fix media change events for polled devices

[Kay Sievers]

On Wed, 2008-03-19 at 11:54 -0500, James Bottomley wrote:
> On Wed, 2008-03-19 at 13:04 +0100, Kay Sievers wrote:
> > we like to see this in 2.6.25, as "change" events for polled devices are
> > broken at the moment.
> 
> I'm not happy just killing the code, since when the AN infrastructure
> gets fixed, it will work.

Who is going to fix what and when? Jeff did not even reply the last two
weeks to the simple question why he added this filter, and why events
need to dropped if something wants to send them. :)

These events are for non-AN devices, which we still poll for media
changes. Userspace wants the same kind of events for both types of
devices.

> How about this instead as a fix for 2.6.25?

We are currently working on a the next generation of the storage part of
the hal daemon, which needs these events. So it would be nice, if we can
make them working now, like they worked with Kristens original patches.

Thanks,
Kay

Re: [PATCH] fix media change events for polled devices

[James Bottomley]

On Thu, 2008-03-20 at 02:18 +0100, Kay Sievers wrote:
> On Wed, 2008-03-19 at 11:54 -0500, James Bottomley wrote:
> > On Wed, 2008-03-19 at 13:04 +0100, Kay Sievers wrote:
> > > we like to see this in 2.6.25, as "change" events for polled devices are
> > > broken at the moment.
> > 
> > I'm not happy just killing the code, since when the AN infrastructure
> > gets fixed, it will work.
> 
> Who is going to fix what and when? Jeff did not even reply the last two
> weeks to the simple question why he added this filter, and why events
> need to dropped if something wants to send them. :)

Oh ... who else ... I'll fix it.  It looks pretty simple: we need two
separate events for the filter: polled and AN media change notifies.  We
should be able to turn either of them off, so two separate flags.  They
need to be unified into the single media change event at the top of the
stack before we call udev.

This will allow us to turn off the AN notifies if we get a problem,
which was the original idea.

> These events are for non-AN devices, which we still poll for media
> changes. Userspace wants the same kind of events for both types of
> devices.
> 
> > How about this instead as a fix for 2.6.25?
> 
> We are currently working on a the next generation of the storage part of
> the hal daemon, which needs these events. So it would be nice, if we can
> make them working now, like they worked with Kristens original patches.

If we emit the same event at the top of the stack, what more needs be
done?  I take it all you want is an indication that we will be using AN
instead of polling, which you will be able to get from the sysfs file.

James

Re: [PATCH] fix media change events for polled devices

[Kay Sievers]

On Thu, Mar 20, 2008 at 2:53 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Thu, 2008-03-20 at 02:18 +0100, Kay Sievers wrote:
> > On Wed, 2008-03-19 at 11:54 -0500, James Bottomley wrote:
>  > > On Wed, 2008-03-19 at 13:04 +0100, Kay Sievers wrote:
>
> > > > we like to see this in 2.6.25, as "change" events for polled devices are
>  > > > broken at the moment.
>  > >
>  > > I'm not happy just killing the code, since when the AN infrastructure
>  > > gets fixed, it will work.
>  >
>  > Who is going to fix what and when? Jeff did not even reply the last two
>  > weeks to the simple question why he added this filter, and why events
>  > need to dropped if something wants to send them. :)
>
>  Oh ... who else ... I'll fix it.

Ah, great! :)

>  It looks pretty simple: we need two
>  separate events for the filter: polled and AN media change notifies.  We
>  should be able to turn either of them off, so two separate flags.  They
>  need to be unified into the single media change event at the top of the
>  stack before we call udev.

Yes, they should be both simple "change" uevents.

>  This will allow us to turn off the AN notifies if we get a problem,
>  which was the original idea.

Sure, that sounds useful.

>  > These events are for non-AN devices, which we still poll for media
>  > changes. Userspace wants the same kind of events for both types of
>  > devices.
>
> >
>  > > How about this instead as a fix for 2.6.25?
>  >
>  > We are currently working on a the next generation of the storage part of
>  > the hal daemon, which needs these events. So it would be nice, if we can
>  > make them working now, like they worked with Kristens original patches.
>
>  If we emit the same event at the top of the stack, what more needs be
>  done?

We just want "change" uevents when the media state for the LUN
changes, regardless if it is an AN event, or an event caused by
revalidating the device after open(). Userspace will look at the
device with every change event. If possible, we could carry a variable
in the event environment to distinguish both event sources.

>  I take it all you want is an indication that we will be using AN
>  instead of polling, which you will be able to get from the sysfs file.

The sysfs flag will be read and if it indicates a non-AN device, we
will not start polling the device periodically, if we don't need to.

That way the polling process will be completely dumb and optional, and
the media handling side will be the same code for all types of
devices.

Thanks,
Kay