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