| Message ID | 20190118213207.2309-2-mwilck@suse.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | block: skip media change event handling if unsupported | expand |
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
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 >
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
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;
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(-)