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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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)





[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux