Patchwork [1/2] block: don't propagate unlisted DISK_EVENTs to userland

login
register
mail settings
Submitter Tejun Heo
Date April 21, 2011, 5:08 p.m.
Message ID <20110421170826.GC15988@htj.dyndns.org>
Download mbox | patch
Permalink /patch/92427/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Tejun Heo - April 21, 2011, 5:08 p.m.
DISK_EVENT_MEDIA_CHANGE is used for both userland visible event and
internal event for revalidation of removeable devices.  Some legacy
drivers don't implement proper event detection and continuously
generate events under certain circumstances.  For example, ide-cd
generates media changed continuously if there's no media in the drive,
which can lead to infinite loop of events jumping back and forth
between the driver and userland event handler.

This patch updates disk event infrastructure such that it never
propagates events not listed in disk->events to userland.  Those
events are processed the same for internal purposes but uevent
generation is suppressed.

This also ensures that userland only gets events which are advertised
in the @events sysfs node lowering risk of confusion.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
These two patches fix infinite MEDIA_CHANGE events problem reported w/
ide-cd.  I tried an alternate patch to implement proper check_events()
for ide-cd but given the deprecated status of ide and the existence of
fallback userland event polling, this minimal approach seems better.

Thank you.

 block/genhd.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds - April 21, 2011, 5:25 p.m.
Should I take these as patches, or through Jens/David/Who?

I'll happily take them as patches, but I'd also like to hear
confirmation from the people who saw the lock-up that it's gone now..

                        Linus

On Thu, Apr 21, 2011 at 10:08 AM, Tejun Heo <tj@kernel.org> wrote:
> DISK_EVENT_MEDIA_CHANGE is used for both userland visible event and
> internal event for revalidation of removeable devices.  Some legacy
> drivers don't implement proper event detection and continuously
> generate events under certain circumstances.  For example, ide-cd
> generates media changed continuously if there's no media in the drive,
> which can lead to infinite loop of events jumping back and forth
> between the driver and userland event handler.
>
> This patch updates disk event infrastructure such that it never
> propagates events not listed in disk->events to userland.  Those
> events are processed the same for internal purposes but uevent
> generation is suppressed.
>
> This also ensures that userland only gets events which are advertised
> in the @events sysfs node lowering risk of confusion.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
> These two patches fix infinite MEDIA_CHANGE events problem reported w/
> ide-cd.  I tried an alternate patch to implement proper check_events()
> for ide-cd but given the deprecated status of ide and the existence of
> fallback userland event polling, this minimal approach seems better.
>
> Thank you.
>
>  block/genhd.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> Index: work/block/genhd.c
> ===================================================================
> --- work.orig/block/genhd.c
> +++ work/block/genhd.c
> @@ -1588,9 +1588,13 @@ static void disk_events_workfn(struct wo
>
>        spin_unlock_irq(&ev->lock);
>
> -       /* tell userland about new events */
> +       /*
> +        * Tell userland about new events.  Only the events listed in
> +        * @disk->events are reported.  Unlisted events are processed the
> +        * same internally but never get reported to userland.
> +        */
>        for (i = 0; i < ARRAY_SIZE(disk_uevents); i++)
> -               if (events & (1 << i))
> +               if (events & disk->events & (1 << i))
>                        envp[nr_events++] = disk_uevents[i];
>
>        if (nr_events)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 21, 2011, 5:27 p.m.
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 21 Apr 2011 10:25:35 -0700

> Should I take these as patches, or through Jens/David/Who?
> 
> I'll happily take them as patches, but I'd also like to hear
> confirmation from the people who saw the lock-up that it's gone now..

I ACK'd the IDE part so that this can get integrated either
directly or via Jens's tree.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jens Axboe - April 21, 2011, 5:31 p.m.
On 2011-04-21 19:25, Linus Torvalds wrote:
> Should I take these as patches, or through Jens/David/Who?
> 
> I'll happily take them as patches, but I'd also like to hear
> confirmation from the people who saw the lock-up that it's gone now..

I'm pulling them in now, with the elevator patch referenced in the IO
scheduler switch email. I've verified that it fixes the ide-cd bug for
me.
Shaun Ruffell - April 21, 2011, 6:02 p.m.
On 04/21/2011 12:31 PM, Jens Axboe wrote:
> On 2011-04-21 19:25, Linus Torvalds wrote:
>> Should I take these as patches, or through Jens/David/Who?
>>
>> I'll happily take them as patches, but I'd also like to hear
>> confirmation from the people who saw the lock-up that it's gone now..
> 
> I'm pulling them in now, with the elevator patch referenced in the IO
> scheduler switch email. I've verified that it fixes the ide-cd bug for
> me.

Jens, Tejun,

I was never seeing a lockup, just the stream of events, but I applied the
two patches on top of mainline (584f790467 from yesterday) and do not see
any issues on the system where I first noticed the constant events [1].

[1] https://bugzilla.kernel.org/show_bug.cgi?id=33342

So,
Tested-by: Shaun Ruffell <sruffell@digium.com> 

Thanks,
Shaun
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jens Axboe - April 21, 2011, 6:10 p.m.
On 2011-04-21 20:02, Shaun Ruffell wrote:
> On 04/21/2011 12:31 PM, Jens Axboe wrote:
>> On 2011-04-21 19:25, Linus Torvalds wrote:
>>> Should I take these as patches, or through Jens/David/Who?
>>>
>>> I'll happily take them as patches, but I'd also like to hear
>>> confirmation from the people who saw the lock-up that it's gone now..
>>
>> I'm pulling them in now, with the elevator patch referenced in the IO
>> scheduler switch email. I've verified that it fixes the ide-cd bug for
>> me.
> 
> Jens, Tejun,
> 
> I was never seeing a lockup, just the stream of events, but I applied the
> two patches on top of mainline (584f790467 from yesterday) and do not see
> any issues on the system where I first noticed the constant events [1].
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=33342

Great, thanks for testing!

Patch

Index: work/block/genhd.c
===================================================================
--- work.orig/block/genhd.c
+++ work/block/genhd.c
@@ -1588,9 +1588,13 @@  static void disk_events_workfn(struct wo
 
 	spin_unlock_irq(&ev->lock);
 
-	/* tell userland about new events */
+	/*
+	 * Tell userland about new events.  Only the events listed in
+	 * @disk->events are reported.  Unlisted events are processed the
+	 * same internally but never get reported to userland.
+	 */
 	for (i = 0; i < ARRAY_SIZE(disk_uevents); i++)
-		if (events & (1 << i))
+		if (events & disk->events & (1 << i))
 			envp[nr_events++] = disk_uevents[i];
 
 	if (nr_events)