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@xxxxxxxx>
> > ---
> > 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@xxxxxxxx>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
