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

[Martin Wilck <mwilck@suse.com>]

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)