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

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

[Tejun Heo]

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)

[PATCH 2/2] ide: unexport DISK_EVENT_MEDIA_CHANGE for ide-gd and ide-cd

[Tejun Heo]

check_events() implementations in both ide-gd and ide-cd are
inadequate for in-kernel event polling.  Both generate media change
events continuously when certain conditions are met causing infinite
event loop between the driver and userland event handler.

As disk event now supports suppression of unlisted events, simply
de-listing DISK_EVENT_MEDIA_CHANGE from disk->events resolves the
problem.  Internal handling around media revalidation will behave the
same while userland will fall back to userland event polling after
detecting the device doesn't support disk events.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Jens Axboe <jaxboe@fusionio.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
 drivers/ide/ide-cd.c       |    1 -
 drivers/ide/ide-cd_ioctl.c |    6 ++++++
 drivers/ide/ide-gd.c       |    7 ++++++-
 3 files changed, 12 insertions(+), 2 deletions(-)

Index: work/drivers/ide/ide-gd.c
===================================================================
--- work.orig/drivers/ide/ide-gd.c
+++ work/drivers/ide/ide-gd.c
@@ -298,6 +298,12 @@ static unsigned int ide_gd_check_events(
 		return 0;
 	}
 
+	/*
+	 * The following is used to force revalidation on the first open on
+	 * removeable devices, and never gets reported to userland as
+	 * genhd->events is 0.  This is intended as removeable ide disk
+	 * can't really detect MEDIA_CHANGE events.
+	 */
 	ret = drive->dev_flags & IDE_DFLAG_MEDIA_CHANGED;
 	drive->dev_flags &= ~IDE_DFLAG_MEDIA_CHANGED;
 
@@ -413,7 +419,6 @@ static int ide_gd_probe(ide_drive_t *dri
 	if (drive->dev_flags & IDE_DFLAG_REMOVABLE)
 		g->flags = GENHD_FL_REMOVABLE;
 	g->fops = &ide_gd_ops;
-	g->events = DISK_EVENT_MEDIA_CHANGE;
 	add_disk(g);
 	return 0;
 
Index: work/drivers/ide/ide-cd.c
===================================================================
--- work.orig/drivers/ide/ide-cd.c
+++ work/drivers/ide/ide-cd.c
@@ -1782,7 +1782,6 @@ static int ide_cd_probe(ide_drive_t *dri
 	ide_cd_read_toc(drive, &sense);
 	g->fops = &idecd_ops;
 	g->flags |= GENHD_FL_REMOVABLE;
-	g->events = DISK_EVENT_MEDIA_CHANGE;
 	add_disk(g);
 	return 0;
 
Index: work/drivers/ide/ide-cd_ioctl.c
===================================================================
--- work.orig/drivers/ide/ide-cd_ioctl.c
+++ work/drivers/ide/ide-cd_ioctl.c
@@ -79,6 +79,12 @@ int ide_cdrom_drive_status(struct cdrom_
 	return CDS_DRIVE_NOT_READY;
 }
 
+/*
+ * ide-cd always generates media changed event if media is missing, which
+ * makes it impossible to use for proper event reporting, so disk->events
+ * is cleared to 0 and the following function is used only to trigger
+ * revalidation and never propagated to userland.
+ */
 unsigned int ide_cdrom_check_events_real(struct cdrom_device_info *cdi,
 					 unsigned int clearing, int slot_nr)
 {

Re: [PATCH 2/2] ide: unexport DISK_EVENT_MEDIA_CHANGE for ide-gd and ide-cd

[David Miller]

From: Tejun Heo <tj@kernel.org>
Date: Thu, 21 Apr 2011 19:09:25 +0200

> check_events() implementations in both ide-gd and ide-cd are
> inadequate for in-kernel event polling.  Both generate media change
> events continuously when certain conditions are met causing infinite
> event loop between the driver and userland event handler.
> 
> As disk event now supports suppression of unlisted events, simply
> de-listing DISK_EVENT_MEDIA_CHANGE from disk->events resolves the
> problem.  Internal handling around media revalidation will behave the
> same while userland will fall back to userland event polling after
> detecting the device doesn't support disk events.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Jens Axboe <jaxboe@fusionio.com>

Acked-by: David S. Miller <davem@davemloft.net>

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

[Linus Torvalds]

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

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

[David Miller]

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.

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

[Jens Axboe]

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 Axboe

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

[Jens Axboe]

On 2011-04-21 19:31, 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.

- Fix the oops on IO scheduler switch. Verified locally that the patch I
  proposed does fix the issue.

- The ide notification fixes from Tejun. Also verified locally to fix
  the boot hang on UP && !CONFIG_PREEMPT.

Please pull.


  git://git.kernel.dk/linux-2.6-block.git for-linus

Jens Axboe (1):
      elevator: check for ELEVATOR_INSERT_SORT_MERGE in !elvpriv case too

Tejun Heo (2):
      block: don't propagate unlisted DISK_EVENTs to userland
      ide: unexport DISK_EVENT_MEDIA_CHANGE for ide-gd and ide-cd

 block/elevator.c           |    3 ++-
 block/genhd.c              |    8 ++++++--
 drivers/ide/ide-cd.c       |    1 -
 drivers/ide/ide-cd_ioctl.c |    6 ++++++
 drivers/ide/ide-gd.c       |    7 ++++++-
 5 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 6f6abc0..45ca1e3 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -671,7 +671,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 			q->boundary_rq = rq;
 		}
 	} else if (!(rq->cmd_flags & REQ_ELVPRIV) &&
-		    where == ELEVATOR_INSERT_SORT)
+		    (where == ELEVATOR_INSERT_SORT ||
+		     where == ELEVATOR_INSERT_SORT_MERGE))
 		where = ELEVATOR_INSERT_BACK;
 
 	switch (where) {
diff --git a/block/genhd.c b/block/genhd.c
index b364bd0..2dd9887 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1588,9 +1588,13 @@ static void disk_events_workfn(struct work_struct *work)
 
 	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)
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index fd1e117..a5ec5a7 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1782,7 +1782,6 @@ static int ide_cd_probe(ide_drive_t *drive)
 	ide_cd_read_toc(drive, &sense);
 	g->fops = &idecd_ops;
 	g->flags |= GENHD_FL_REMOVABLE;
-	g->events = DISK_EVENT_MEDIA_CHANGE;
 	add_disk(g);
 	return 0;
 
diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index 2a6bc50..02caa7d 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -79,6 +79,12 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
 	return CDS_DRIVE_NOT_READY;
 }
 
+/*
+ * ide-cd always generates media changed event if media is missing, which
+ * makes it impossible to use for proper event reporting, so disk->events
+ * is cleared to 0 and the following function is used only to trigger
+ * revalidation and never propagated to userland.
+ */
 unsigned int ide_cdrom_check_events_real(struct cdrom_device_info *cdi,
 					 unsigned int clearing, int slot_nr)
 {
diff --git a/drivers/ide/ide-gd.c b/drivers/ide/ide-gd.c
index c4ffd48..70ea876 100644
--- a/drivers/ide/ide-gd.c
+++ b/drivers/ide/ide-gd.c
@@ -298,6 +298,12 @@ static unsigned int ide_gd_check_events(struct gendisk *disk,
 		return 0;
 	}
 
+	/*
+	 * The following is used to force revalidation on the first open on
+	 * removeable devices, and never gets reported to userland as
+	 * genhd->events is 0.  This is intended as removeable ide disk
+	 * can't really detect MEDIA_CHANGE events.
+	 */
 	ret = drive->dev_flags & IDE_DFLAG_MEDIA_CHANGED;
 	drive->dev_flags &= ~IDE_DFLAG_MEDIA_CHANGED;
 
@@ -413,7 +419,6 @@ static int ide_gd_probe(ide_drive_t *drive)
 	if (drive->dev_flags & IDE_DFLAG_REMOVABLE)
 		g->flags = GENHD_FL_REMOVABLE;
 	g->fops = &ide_gd_ops;
-	g->events = DISK_EVENT_MEDIA_CHANGE;
 	add_disk(g);
 	return 0;
 
> 


-- 
Jens Axboe

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

[Shaun Ruffell]

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

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

[Jens Axboe]

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!

-- 
Jens Axboe