[PATCH 0/4] block: skip media change event handling if unsupported

[PATCH 0/4] block: skip media change event handling if unsupported

[Martin Wilck]

The block layer currently can't distinguish between gendisk devices that
don't support any media change events, and devices that do support them,
but only for internal purposes. Therefore the check_events() function is
called e.g. for ordinary non-removable SCSI disks. While these devices are
not polled under normal conditions, the check_events function is called on
certain synchronization points, in particular while the device is opened,
closed, or probed.

Under unfavorable conditions this can lead to processes being stalled on a
blocked queue: a close() schedules a work item for check_events() which
gets blocked in the work queue, and subsequent open() tries to flush the
workqueue. The flush then stalls too, as long as the the blocked work item
can't finish.

In principle, the gendisk->events field would make it very easy for the
block layer to check only for events actually supported by the device.
Currently this is impossible, because there are lots of drivers which don't
set gendisk->events although they implement the check_events() method.
This was introduced in commit 7c88a168da80 ("block: don't propagate unlisted 
DISK_EVENTs to userland") and follow-up patches because uevent generation by
these drivers was found to possibly generate infinite event loops between
kernel and user space. A side effect of these patches was that the
distinction between such devices and devices supporting no events at all
was lost.

This series implements a slightly different approach to event handling and
uevent suppression. The drivers are changed to set the events field again.
Whether or not uevents should be generated is controlled by a separate flag
bit, which is only set by the drivers that are known to generate proper
uevents (sd and sr). Once this is done, devices that don't support any
media change events can be clearly identified, and the whole events
checking code path can be skipped. This simplifies handling of
non-removable SCSI disks.

I have tested this with removable and non-removable SCSI disks, SCSI cdrom,
and ide-cd.

This patch set targets the same problem as Hannes' late submission "sd:
skip non-removable devices in sd_check_events()".

Martin Wilck (4):
  block: disk_events: introduce event flags
  Revert "ide: unexport DISK_EVENT_MEDIA_CHANGE for ide-gd and ide-cd"
  Revert "block: unexport DISK_EVENT_MEDIA_CHANGE for legacy/fringe
    drivers"
  block: check_events: don't bother with events if unsupported

 block/genhd.c              | 50 +++++++++++++++++++++++++-------------
 drivers/block/amiflop.c    |  1 +
 drivers/block/ataflop.c    |  1 +
 drivers/block/floppy.c     |  1 +
 drivers/block/paride/pcd.c |  1 +
 drivers/block/paride/pd.c  |  1 +
 drivers/block/paride/pf.c  |  1 +
 drivers/block/swim.c       |  1 +
 drivers/block/swim3.c      |  1 +
 drivers/block/xsysace.c    |  1 +
 drivers/cdrom/gdrom.c      |  1 +
 drivers/ide/ide-cd.c       |  1 +
 drivers/ide/ide-cd_ioctl.c |  5 ++--
 drivers/ide/ide-gd.c       |  6 +++--
 drivers/scsi/sd.c          |  3 ++-
 drivers/scsi/sr.c          |  3 ++-
 include/linux/genhd.h      |  7 ++++++
 17 files changed, 62 insertions(+), 23 deletions(-)

-- 
2.19.2

[PATCH 1/4] block: disk_events: introduce event flags

[Martin Wilck]

Currently, an empty disk->events field tells the block layer not to forward
media change events to user space. This was done in commit 7c88a168da80 ("block:
don't propagate unlisted DISK_EVENTs to userland") in order to avoid events
from "fringe" drivers to be forwarded to user space. By doing so, the block
layer lost the information which events were supported by a particular
block device, and most importantly, whether or not a given device supports
media change events at all.

Prepare for not interpreting the "events" field this way in the future any
more. This is done by adding two flag bits that can be set to have the
device treated like one that has the "events" field set to a non-zero value
before. This applies only to the sd and sr drivers, which are changed to
set the new flags.

The new flags are DISK_EVENT_FLAG_POLL to enforce polling of the device for
synchronous events, and DISK_EVENT_FLAG_UEVENT to tell the blocklayer to
generate udev events from kernel events. They can easily be fit in the int
reserved for event bits.

This patch doesn't change behavior.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 block/genhd.c         | 22 ++++++++++++++++------
 drivers/scsi/sd.c     |  3 ++-
 drivers/scsi/sr.c     |  3 ++-
 include/linux/genhd.h |  7 +++++++
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 1dd8fd6..bcd16f6 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1631,7 +1631,8 @@ static unsigned long disk_events_poll_jiffies(struct gendisk *disk)
 	 */
 	if (ev->poll_msecs >= 0)
 		intv_msecs = ev->poll_msecs;
-	else if (disk->events & ~disk->async_events)
+	else if (disk->events & DISK_EVENT_FLAG_POLL
+		 && disk->events & ~disk->async_events)
 		intv_msecs = disk_events_dfl_poll_msecs;
 
 	return msecs_to_jiffies(intv_msecs);
@@ -1841,11 +1842,13 @@ static void disk_check_events(struct disk_events *ev,
 
 	/*
 	 * 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.
+	 * @disk->events are reported, and only if DISK_EVENT_FLAG_UEVENT
+	 * is set. Otherwise, events are processed internally but never
+	 * get reported to userland.
 	 */
 	for (i = 0; i < ARRAY_SIZE(disk_uevents); i++)
-		if (events & disk->events & (1 << i))
+		if (events & disk->events & (1 << i) &&
+		    disk->events & DISK_EVENT_FLAG_UEVENT)
 			envp[nr_events++] = disk_uevents[i];
 
 	if (nr_events)
@@ -1882,7 +1885,10 @@ static ssize_t disk_events_show(struct device *dev,
 {
 	struct gendisk *disk = dev_to_disk(dev);
 
-	return __disk_events_show(disk->events, buf);
+	if (!(disk->events & DISK_EVENT_FLAG_UEVENT))
+		return 0;
+
+	return __disk_events_show(disk->events & DISK_EVENT_TYPES_MASK, buf);
 }
 
 static ssize_t disk_events_async_show(struct device *dev,
@@ -1890,7 +1896,11 @@ static ssize_t disk_events_async_show(struct device *dev,
 {
 	struct gendisk *disk = dev_to_disk(dev);
 
-	return __disk_events_show(disk->async_events, buf);
+	if (!(disk->events & DISK_EVENT_FLAG_UEVENT))
+		return 0;
+
+	return __disk_events_show(disk->async_events & DISK_EVENT_TYPES_MASK,
+				  buf);
 }
 
 static ssize_t disk_events_poll_msecs_show(struct device *dev,
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a1a44f5..fee8509 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3256,7 +3256,8 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 	gd->flags = GENHD_FL_EXT_DEVT;
 	if (sdp->removable) {
 		gd->flags |= GENHD_FL_REMOVABLE;
-		gd->events |= DISK_EVENT_MEDIA_CHANGE;
+		gd->events |= DISK_EVENT_MEDIA_CHANGE |
+			DISK_EVENT_FLAG_POLL | DISK_EVENT_FLAG_UEVENT;
 	}
 
 	blk_pm_runtime_init(sdp->request_queue, dev);
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 38ddbbf..3571651 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -716,7 +716,8 @@ static int sr_probe(struct device *dev)
 	sprintf(disk->disk_name, "sr%d", minor);
 	disk->fops = &sr_bdops;
 	disk->flags = GENHD_FL_CD | GENHD_FL_BLOCK_EVENTS_ON_EXCL_WRITE;
-	disk->events = DISK_EVENT_MEDIA_CHANGE | DISK_EVENT_EJECT_REQUEST;
+	disk->events = DISK_EVENT_MEDIA_CHANGE | DISK_EVENT_EJECT_REQUEST
+		| DISK_EVENT_FLAG_POLL | DISK_EVENT_FLAG_UEVENT;
 
 	blk_queue_rq_timeout(sdev->request_queue, SR_TIMEOUT);
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 06c0fd59..1112bcc 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -148,8 +148,15 @@ struct hd_struct {
 enum {
 	DISK_EVENT_MEDIA_CHANGE			= 1 << 0, /* media changed */
 	DISK_EVENT_EJECT_REQUEST		= 1 << 1, /* eject requested */
+	/* Poll even if events_poll_msecs is unset */
+	DISK_EVENT_FLAG_POLL			= 1 << 16,
+	/* Forward events to udev */
+	DISK_EVENT_FLAG_UEVENT			= 1 << 17,
 };
 
+#define DISK_EVENT_TYPES_MASK \
+	(DISK_EVENT_MEDIA_CHANGE | DISK_EVENT_EJECT_REQUEST)
+
 struct disk_part_tbl {
 	struct rcu_head rcu_head;
 	int len;
-- 
2.19.2

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

[Martin Wilck]

This reverts commit 7eec77a1816a7042591a6cbdb4820e9e7ebffe0e.

Instead of leaving disk->events completely empty, we now export the supported
events again, and tell the block layer not to forward events to user space by
not setting DISK_EVENT_FLAG_UEVENT. This allows the block layer to distinguish
between devices that for which events should be handled in kernel only, and
devices which don't support any meda change events at all.

Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/ide/ide-cd.c       | 1 +
 drivers/ide/ide-cd_ioctl.c | 5 +++--
 drivers/ide/ide-gd.c       | 6 ++++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 1f03884..3b15adc 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1797,6 +1797,7 @@ static int ide_cd_probe(ide_drive_t *drive)
 	ide_cd_read_toc(drive);
 	g->fops = &idecd_ops;
 	g->flags |= GENHD_FL_REMOVABLE | GENHD_FL_BLOCK_EVENTS_ON_EXCL_WRITE;
+	g->events = DISK_EVENT_MEDIA_CHANGE;
 	device_add_disk(&drive->gendev, g, NULL);
 	return 0;
 
diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index 4a6e1a4..f9f4bac 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -82,8 +82,9 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
 
 /*
  * 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
+ * makes it impossible to use for proper event reporting, so
+ * DISK_EVENT_FLAG_UEVENT is cleared in disk->events
+ * 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,
diff --git a/drivers/ide/ide-gd.c b/drivers/ide/ide-gd.c
index 04e008e..cab8052 100644
--- a/drivers/ide/ide-gd.c
+++ b/drivers/ide/ide-gd.c
@@ -299,8 +299,9 @@ static unsigned int ide_gd_check_events(struct gendisk *disk,
 	/*
 	 * 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.
+	 * DISK_EVENT_FLAG_UEVENT isn't set in genhd->events.
+	 * 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;
@@ -417,6 +418,7 @@ static int ide_gd_probe(ide_drive_t *drive)
 		g->flags = GENHD_FL_REMOVABLE;
 	g->fops = &ide_gd_ops;
 	device_add_disk(&drive->gendev, g, NULL);
+	g->events = DISK_EVENT_MEDIA_CHANGE;
 	return 0;
 
 out_free_disk:
-- 
2.19.2

[PATCH 3/4] Revert "block: unexport DISK_EVENT_MEDIA_CHANGE for legacy/fringe drivers"

[Martin Wilck]

This reverts commit 9fd097b14918875bd6f125ed699d7bbbba5893ee.

Instead of leaving disk->events completely empty, we now export the supported
events again, and tell the block layer not to forward events to user space by
not setting DISK_EVENT_FLAG_UEVENT. This allows the block layer to distinguish
between devices that for which events should be handled in kernel only, and
devices which don't support any meda change events at all.

Cc: Jiri Kosina <jikos@kernel.org>
Cc: Tim Waugh <tim@cyberelk.net>
Cc: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/block/amiflop.c    | 1 +
 drivers/block/ataflop.c    | 1 +
 drivers/block/floppy.c     | 1 +
 drivers/block/paride/pcd.c | 1 +
 drivers/block/paride/pd.c  | 1 +
 drivers/block/paride/pf.c  | 1 +
 drivers/block/swim.c       | 1 +
 drivers/block/swim3.c      | 1 +
 drivers/block/xsysace.c    | 1 +
 drivers/cdrom/gdrom.c      | 1 +
 10 files changed, 10 insertions(+)

diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
index 0903e08..92b930c 100644
--- a/drivers/block/amiflop.c
+++ b/drivers/block/amiflop.c
@@ -1829,6 +1829,7 @@ static int __init fd_probe_drives(void)
 		disk->major = FLOPPY_MAJOR;
 		disk->first_minor = drive;
 		disk->fops = &floppy_fops;
+		disk->events = DISK_EVENT_MEDIA_CHANGE;
 		sprintf(disk->disk_name, "fd%d", drive);
 		disk->private_data = &unit[drive];
 		set_capacity(disk, 880*2);
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index b0dbbdf..c7b5c46 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -2028,6 +2028,7 @@ static int __init atari_floppy_init (void)
 		unit[i].disk->first_minor = i;
 		sprintf(unit[i].disk->disk_name, "fd%d", i);
 		unit[i].disk->fops = &floppy_fops;
+		unit[i].disk->events = DISK_EVENT_MEDIA_CHANGE;
 		unit[i].disk->private_data = &unit[i];
 		set_capacity(unit[i].disk, MAX_DISK_SIZE * 2);
 		add_disk(unit[i].disk);
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 6f2856c..f95699e 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4543,6 +4543,7 @@ static int __init do_floppy_init(void)
 		disks[drive]->major = FLOPPY_MAJOR;
 		disks[drive]->first_minor = TOMINOR(drive);
 		disks[drive]->fops = &floppy_fops;
+		disks[drive]->events = DISK_EVENT_MEDIA_CHANGE;
 		sprintf(disks[drive]->disk_name, "fd%d", drive);
 
 		timer_setup(&motor_off_timer[drive], motor_off_callback, 0);
diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
index 96670ee..359a0d8 100644
--- a/drivers/block/paride/pcd.c
+++ b/drivers/block/paride/pcd.c
@@ -342,6 +342,7 @@ static void pcd_init_units(void)
 		strcpy(disk->disk_name, cd->name);	/* umm... */
 		disk->fops = &pcd_bdops;
 		disk->flags = GENHD_FL_BLOCK_EVENTS_ON_EXCL_WRITE;
+		disk->events = DISK_EVENT_MEDIA_CHANGE;
 	}
 }
 
diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index 0ff9b12..6f9ad3f 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -897,6 +897,7 @@ static void pd_probe_drive(struct pd_unit *disk)
 	p->fops = &pd_fops;
 	p->major = major;
 	p->first_minor = (disk - pd) << PD_BITS;
+	p->events = DISK_EVENT_MEDIA_CHANGE;
 	disk->gd = p;
 	p->private_data = disk;
 
diff --git a/drivers/block/paride/pf.c b/drivers/block/paride/pf.c
index e92e7a8..52a74f9 100644
--- a/drivers/block/paride/pf.c
+++ b/drivers/block/paride/pf.c
@@ -319,6 +319,7 @@ static void __init pf_init_units(void)
 		disk->first_minor = unit;
 		strcpy(disk->disk_name, pf->name);
 		disk->fops = &pf_fops;
+		disk->events = DISK_EVENT_MEDIA_CHANGE;
 		if (!(*drives[unit])[D_PRT])
 			pf_drive_count++;
 	}
diff --git a/drivers/block/swim.c b/drivers/block/swim.c
index 3fa6fcc..67b5ec2 100644
--- a/drivers/block/swim.c
+++ b/drivers/block/swim.c
@@ -862,6 +862,7 @@ static int swim_floppy_init(struct swim_priv *swd)
 		swd->unit[drive].disk->first_minor = drive;
 		sprintf(swd->unit[drive].disk->disk_name, "fd%d", drive);
 		swd->unit[drive].disk->fops = &floppy_fops;
+		swd->unit[drive].disk->events = DISK_EVENT_MEDIA_CHANGE;
 		swd->unit[drive].disk->private_data = &swd->unit[drive];
 		set_capacity(swd->unit[drive].disk, 2880);
 		add_disk(swd->unit[drive].disk);
diff --git a/drivers/block/swim3.c b/drivers/block/swim3.c
index 1e2ae90d..cf42729 100644
--- a/drivers/block/swim3.c
+++ b/drivers/block/swim3.c
@@ -1216,6 +1216,7 @@ static int swim3_attach(struct macio_dev *mdev,
 	disk->first_minor = floppy_count;
 	disk->fops = &floppy_fops;
 	disk->private_data = fs;
+	disk->events = DISK_EVENT_MEDIA_CHANGE;
 	disk->flags |= GENHD_FL_REMOVABLE;
 	sprintf(disk->disk_name, "fd%d", floppy_count);
 	set_capacity(disk, 2880);
diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index 87ccef4..8d29950 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -1032,6 +1032,7 @@ static int ace_setup(struct ace_device *ace)
 	ace->gd->major = ace_major;
 	ace->gd->first_minor = ace->id * ACE_NUM_MINORS;
 	ace->gd->fops = &ace_fops;
+	ace->gd->events = DISK_EVENT_MEDIA_CHANGE;
 	ace->gd->queue = ace->queue;
 	ace->gd->private_data = ace;
 	snprintf(ace->gd->disk_name, 32, "xs%c", ace->id + 'a');
diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index f8b7345..5cf3bad 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -786,6 +786,7 @@ static int probe_gdrom(struct platform_device *devptr)
 		goto probe_fail_cdrom_register;
 	}
 	gd.disk->fops = &gdrom_bdops;
+	gd.disk->events = DISK_EVENT_MEDIA_CHANGE;
 	/* latch on to the interrupt */
 	err = gdrom_set_interrupt_handlers();
 	if (err)
-- 
2.19.2

[PATCH 4/4] block: check_events: don't bother with events if unsupported

[Martin Wilck]

Drivers now report to the block layer if they support media change
events. If this is not the case, there's no need to allocate
the event structure, and all event handling code can effectively
be skipped. This simplifies code flow in particular for
non-removable sd devices.

This effectively reverts commit 75e3f3ee3c64 ("block: always allocate
genhd->ev if check_events is implemented").

The sysfs files for the events are kept in place even if no events
are supported, as user space may rely on them being present. The only
difference is that an error code is now returned if the user tries
to set poll_msecs.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 block/genhd.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index bcd16f6..55b6570 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1909,6 +1909,9 @@ static ssize_t disk_events_poll_msecs_show(struct device *dev,
 {
 	struct gendisk *disk = dev_to_disk(dev);
 
+	if (!disk->ev)
+		return sprintf(buf, "-1\n");
+
 	return sprintf(buf, "%ld\n", disk->ev->poll_msecs);
 }
 
@@ -1925,6 +1928,9 @@ static ssize_t disk_events_poll_msecs_store(struct device *dev,
 	if (intv < 0 && intv != -1)
 		return -EINVAL;
 
+	if (!disk->ev)
+		return -ENODEV;
+
 	disk_block_events(disk);
 	disk->ev->poll_msecs = intv;
 	__disk_unblock_events(disk, true);
@@ -1989,7 +1995,8 @@ static void disk_alloc_events(struct gendisk *disk)
 {
 	struct disk_events *ev;
 
-	if (!disk->fops->check_events)
+	if (!disk->fops->check_events ||
+	    !(disk->events & DISK_EVENT_TYPES_MASK))
 		return;
 
 	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
@@ -2011,14 +2018,14 @@ static void disk_alloc_events(struct gendisk *disk)
 
 static void disk_add_events(struct gendisk *disk)
 {
-	if (!disk->ev)
-		return;
-
 	/* FIXME: error handling */
 	if (sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs) < 0)
 		pr_warn("%s: failed to create sysfs files for events\n",
 			disk->disk_name);
 
+	if (!disk->ev)
+		return;
+
 	mutex_lock(&disk_events_mutex);
 	list_add_tail(&disk->ev->node, &disk_events);
 	mutex_unlock(&disk_events_mutex);
@@ -2032,14 +2039,13 @@ static void disk_add_events(struct gendisk *disk)
 
 static void disk_del_events(struct gendisk *disk)
 {
-	if (!disk->ev)
-		return;
+	if (disk->ev) {
+		disk_block_events(disk);
 
-	disk_block_events(disk);
-
-	mutex_lock(&disk_events_mutex);
-	list_del_init(&disk->ev->node);
-	mutex_unlock(&disk_events_mutex);
+		mutex_lock(&disk_events_mutex);
+		list_del_init(&disk->ev->node);
+		mutex_unlock(&disk_events_mutex);
+	}
 
 	sysfs_remove_files(&disk_to_dev(disk)->kobj, disk_events_attrs);
 }
-- 
2.19.2

Re: [PATCH 1/4] block: disk_events: introduce event flags

[Hannes Reinecke]

On 1/18/19 10:32 PM, Martin Wilck wrote:
> Currently, an empty disk->events field tells the block layer not to forward
> media change events to user space. This was done in commit 7c88a168da80 ("block:
> don't propagate unlisted DISK_EVENTs to userland") in order to avoid events
> from "fringe" drivers to be forwarded to user space. By doing so, the block
> layer lost the information which events were supported by a particular
> block device, and most importantly, whether or not a given device supports
> media change events at all.
> 
> Prepare for not interpreting the "events" field this way in the future any
> more. This is done by adding two flag bits that can be set to have the
> device treated like one that has the "events" field set to a non-zero value
> before. This applies only to the sd and sr drivers, which are changed to
> set the new flags.
> 
> The new flags are DISK_EVENT_FLAG_POLL to enforce polling of the device for
> synchronous events, and DISK_EVENT_FLAG_UEVENT to tell the blocklayer to
> generate udev events from kernel events. They can easily be fit in the int
> reserved for event bits.
> 
> This patch doesn't change behavior.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   block/genhd.c         | 22 ++++++++++++++++------
>   drivers/scsi/sd.c     |  3 ++-
>   drivers/scsi/sr.c     |  3 ++-
>   include/linux/genhd.h |  7 +++++++
>   4 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 1dd8fd6..bcd16f6 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1631,7 +1631,8 @@ static unsigned long disk_events_poll_jiffies(struct gendisk *disk)
>   	 */
>   	if (ev->poll_msecs >= 0)
>   		intv_msecs = ev->poll_msecs;
> -	else if (disk->events & ~disk->async_events)
> +	else if (disk->events & DISK_EVENT_FLAG_POLL
> +		 && disk->events & ~disk->async_events)
>   		intv_msecs = disk_events_dfl_poll_msecs;
>   
>   	return msecs_to_jiffies(intv_msecs);
Hmm. That is an ... odd condition.
Clearly it's pointless to have the event bit set in the ->events mask if 
it's already part of the ->async_events mask.
But shouldn't we better _prevent_ this from happening, ie refuse to set
DISK_EVENT_FLAG_POLL in events if it's already in ->async_events?
Then the above check would be simplified.

Cheers,

Hannes

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

[Hannes Reinecke]

On 1/18/19 10:32 PM, Martin Wilck wrote:
> This reverts commit 7eec77a1816a7042591a6cbdb4820e9e7ebffe0e.
> 
> Instead of leaving disk->events completely empty, we now export the supported
> events again, and tell the block layer not to forward events to user space by
> not setting DISK_EVENT_FLAG_UEVENT. This allows the block layer to distinguish
> between devices that for which events should be handled in kernel only, and
> devices which don't support any meda change events at all.
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   drivers/ide/ide-cd.c       | 1 +
>   drivers/ide/ide-cd_ioctl.c | 5 +++--
>   drivers/ide/ide-gd.c       | 6 ++++--
>   3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index 1f03884..3b15adc 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1797,6 +1797,7 @@ static int ide_cd_probe(ide_drive_t *drive)
>   	ide_cd_read_toc(drive);
>   	g->fops = &idecd_ops;
>   	g->flags |= GENHD_FL_REMOVABLE | GENHD_FL_BLOCK_EVENTS_ON_EXCL_WRITE;
> +	g->events = DISK_EVENT_MEDIA_CHANGE;
>   	device_add_disk(&drive->gendev, g, NULL);
>   	return 0;
>   
> diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
> index 4a6e1a4..f9f4bac 100644
> --- a/drivers/ide/ide-cd_ioctl.c
> +++ b/drivers/ide/ide-cd_ioctl.c
> @@ -82,8 +82,9 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
>   
>   /*
>    * 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
> + * makes it impossible to use for proper event reporting, so
> + * DISK_EVENT_FLAG_UEVENT is cleared in disk->events
> + * 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,
> diff --git a/drivers/ide/ide-gd.c b/drivers/ide/ide-gd.c
> index 04e008e..cab8052 100644
> --- a/drivers/ide/ide-gd.c
> +++ b/drivers/ide/ide-gd.c
> @@ -299,8 +299,9 @@ static unsigned int ide_gd_check_events(struct gendisk *disk,
>   	/*
>   	 * 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.
> +	 * DISK_EVENT_FLAG_UEVENT isn't set in genhd->events.
> +	 * 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;
> @@ -417,6 +418,7 @@ static int ide_gd_probe(ide_drive_t *drive)
>   		g->flags = GENHD_FL_REMOVABLE;
>   	g->fops = &ide_gd_ops;
>   	device_add_disk(&drive->gendev, g, NULL);
> +	g->events = DISK_EVENT_MEDIA_CHANGE;
>   	return 0;
>   
>   out_free_disk:
> 
Please set the events mask prior to call device_add_disk(), otherwise 
we'll have a race condition here.

Cheers,

Hannes

Re: [PATCH 3/4] Revert "block: unexport DISK_EVENT_MEDIA_CHANGE for legacy/fringe drivers"

[Hannes Reinecke]

On 1/18/19 10:32 PM, Martin Wilck wrote:
> This reverts commit 9fd097b14918875bd6f125ed699d7bbbba5893ee.
> 
> Instead of leaving disk->events completely empty, we now export the supported
> events again, and tell the block layer not to forward events to user space by
> not setting DISK_EVENT_FLAG_UEVENT. This allows the block layer to distinguish
> between devices that for which events should be handled in kernel only, and
> devices which don't support any meda change events at all.
> 
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: Tim Waugh <tim@cyberelk.net>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   drivers/block/amiflop.c    | 1 +
>   drivers/block/ataflop.c    | 1 +
>   drivers/block/floppy.c     | 1 +
>   drivers/block/paride/pcd.c | 1 +
>   drivers/block/paride/pd.c  | 1 +
>   drivers/block/paride/pf.c  | 1 +
>   drivers/block/swim.c       | 1 +
>   drivers/block/swim3.c      | 1 +
>   drivers/block/xsysace.c    | 1 +
>   drivers/cdrom/gdrom.c      | 1 +
>   10 files changed, 10 insertions(+)
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCH 4/4] block: check_events: don't bother with events if unsupported

[Hannes Reinecke]

On 1/18/19 10:32 PM, Martin Wilck wrote:
> Drivers now report to the block layer if they support media change
> events. If this is not the case, there's no need to allocate
> the event structure, and all event handling code can effectively
> be skipped. This simplifies code flow in particular for
> non-removable sd devices.
> 
> This effectively reverts commit 75e3f3ee3c64 ("block: always allocate
> genhd->ev if check_events is implemented").
> 
> The sysfs files for the events are kept in place even if no events
> are supported, as user space may rely on them being present. The only
> difference is that an error code is now returned if the user tries
> to set poll_msecs.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   block/genhd.c | 28 +++++++++++++++++-----------
>   1 file changed, 17 insertions(+), 11 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCH 1/4] block: disk_events: introduce event flags

[Martin Wilck]

On Sat, 2019-01-26 at 11:09 +0100, Hannes Reinecke wrote:
> On 1/18/19 10:32 PM, Martin Wilck wrote:
> > Currently, an empty disk->events field tells the block layer not to
> > forward
> > media change events to user space. This was done in commit
> > 7c88a168da80 ("block:
> > don't propagate unlisted DISK_EVENTs to userland") in order to
> > avoid events
> > from "fringe" drivers to be forwarded to user space. By doing so,
> > the block
> > layer lost the information which events were supported by a
> > particular
> > block device, and most importantly, whether or not a given device
> > supports
> > media change events at all.
> > 
> > Prepare for not interpreting the "events" field this way in the
> > future any
> > more. This is done by adding two flag bits that can be set to have
> > the
> > device treated like one that has the "events" field set to a non-
> > zero value
> > before. This applies only to the sd and sr drivers, which are
> > changed to
> > set the new flags.
> > 
> > The new flags are DISK_EVENT_FLAG_POLL to enforce polling of the
> > device for
> > synchronous events, and DISK_EVENT_FLAG_UEVENT to tell the
> > blocklayer to
> > generate udev events from kernel events. They can easily be fit in
> > the int
> > reserved for event bits.
> > 
> > This patch doesn't change behavior.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >   block/genhd.c         | 22 ++++++++++++++++------
> >   drivers/scsi/sd.c     |  3 ++-
> >   drivers/scsi/sr.c     |  3 ++-
> >   include/linux/genhd.h |  7 +++++++
> >   4 files changed, 27 insertions(+), 8 deletions(-)
> > 
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 1dd8fd6..bcd16f6 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -1631,7 +1631,8 @@ static unsigned long
> > disk_events_poll_jiffies(struct gendisk *disk)
> >   	 */
> >   	if (ev->poll_msecs >= 0)
> >   		intv_msecs = ev->poll_msecs;
> > -	else if (disk->events & ~disk->async_events)
> > +	else if (disk->events & DISK_EVENT_FLAG_POLL
> > +		 && disk->events & ~disk->async_events)
> >   		intv_msecs = disk_events_dfl_poll_msecs;
> >   
> >   	return msecs_to_jiffies(intv_msecs);
> Hmm. That is an ... odd condition.
> Clearly it's pointless to have the event bit set in the ->events mask
> if 
> it's already part of the ->async_events mask.

The "events" bit has to be set in that case. "async_events" is defined
as a subset of "events", see genhd.h. You can trivially verify that
this is currently true, as NO driver that sets any bit in the
"async_events" field. I was wondering if "async_events" can't be
ditched completely, but I didn't want to make that aggressive a change
in this patch set.

> But shouldn't we better _prevent_ this from happening, ie refuse to
> set
> DISK_EVENT_FLAG_POLL in events if it's already in ->async_events?
> Then the above check would be simplified.

Asynchronous events need not be polled for, therefore setting the POLL
flag in async_events makes no sense. My intention was to use these
"flag" bits in the "events" field only. Perhaps I should have expressed
that more clearly?

Anyway, unless I'm really blind, the condition above is actually the
same as before, just that I now require the POLL flag to be set as
well, which is the main point of the patch.

Regards
Martin


> 
> Cheers,
> 
> Hannes
> 

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 1/4] block: disk_events: introduce event flags

[Martin Wilck]

Hannes, all,

On Mon, 2019-01-28 at 14:54 +0100, Martin Wilck wrote:
> On Sat, 2019-01-26 at 11:09 +0100, Hannes Reinecke wrote:
> > On 1/18/19 10:32 PM, Martin Wilck wrote:
> > > Currently, an empty disk->events field tells the block layer not
> > > to
> > > forward
> > > media change events to user space. This was done in commit
> > > 7c88a168da80 ("block:
> > > don't propagate unlisted DISK_EVENTs to userland") in order to
> > > avoid events
> > > from "fringe" drivers to be forwarded to user space. By doing so,
> > > the block
> > > layer lost the information which events were supported by a
> > > particular
> > > block device, and most importantly, whether or not a given device
> > > supports
> > > media change events at all.
> > > 
> > > Prepare for not interpreting the "events" field this way in the
> > > future any
> > > more. This is done by adding two flag bits that can be set to
> > > have
> > > the
> > > device treated like one that has the "events" field set to a non-
> > > zero value
> > > before. This applies only to the sd and sr drivers, which are
> > > changed to
> > > set the new flags.
> > > 
> > > The new flags are DISK_EVENT_FLAG_POLL to enforce polling of the
> > > device for
> > > synchronous events, and DISK_EVENT_FLAG_UEVENT to tell the
> > > blocklayer to
> > > generate udev events from kernel events. They can easily be fit
> > > in
> > > the int
> > > reserved for event bits.
> > > 
> > > This patch doesn't change behavior.
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >   block/genhd.c         | 22 ++++++++++++++++------
> > >   drivers/scsi/sd.c     |  3 ++-
> > >   drivers/scsi/sr.c     |  3 ++-
> > >   include/linux/genhd.h |  7 +++++++
> > >   4 files changed, 27 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/block/genhd.c b/block/genhd.c
> > > index 1dd8fd6..bcd16f6 100644
> > > --- a/block/genhd.c
> > > +++ b/block/genhd.c
> > > @@ -1631,7 +1631,8 @@ static unsigned long
> > > disk_events_poll_jiffies(struct gendisk *disk)
> > >   	 */
> > >   	if (ev->poll_msecs >= 0)
> > >   		intv_msecs = ev->poll_msecs;
> > > -	else if (disk->events & ~disk->async_events)
> > > +	else if (disk->events & DISK_EVENT_FLAG_POLL
> > > +		 && disk->events & ~disk->async_events)
> > >   		intv_msecs = disk_events_dfl_poll_msecs;
> > >   
> > >   	return msecs_to_jiffies(intv_msecs);
> > Hmm. That is an ... odd condition.
> > Clearly it's pointless to have the event bit set in the ->events
> > mask
> > if 
> > it's already part of the ->async_events mask.
> 
> The "events" bit has to be set in that case. "async_events" is
> defined
> as a subset of "events", see genhd.h. You can trivially verify that
> this is currently true, as NO driver that sets any bit in the
> "async_events" field. I was wondering if "async_events" can't be
> ditched completely, but I didn't want to make that aggressive a
> change
> in this patch set.
> 
> > But shouldn't we better _prevent_ this from happening, ie refuse to
> > set
> > DISK_EVENT_FLAG_POLL in events if it's already in ->async_events?
> > Then the above check would be simplified.
> 
> Asynchronous events need not be polled for, therefore setting the
> POLL
> flag in async_events makes no sense. My intention was to use these
> "flag" bits in the "events" field only. Perhaps I should have
> expressed
> that more clearly?
> 
> Anyway, unless I'm really blind, the condition above is actually the
> same as before, just that I now require the POLL flag to be set as
> well, which is the main point of the patch.

Does this response make sense? If yes, can we get the set merged?
If no, what do I need to change? Should I add a patch that does away
with the unused async_events field?

Thanks
Martin


-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)