[dm-devel] [PATCH 1/3] dm: a basic support for using the select or poll function

[Mikulas Patocka]

On Thu, 11 May 2017, Martin Wilck wrote:

> On Thu, 2017-05-11 at 09:21 -0400, Mike Snitzer wrote:
> > On Thu, May 11 2017 at  5:43am -0400,
> > Martin Wilck <mwilck at suse.com> wrote:
> > 
> > > On Thu, 2017-05-11 at 11:39 +0200, Martin Wilck wrote:
> > > > On Tue, 2017-05-09 at 12:10 -0700, Andy Grover wrote:
> > > > > From: Mikulas Patocka <mpatocka at redhat.com>
> > > > >  
> > > > > This is the very simple patch for polling on the
> > > > > /dev/mapper/control
> > > > > device. The select or poll function waits until any event
> > > > > happens
> > > > > on
> > > > > any
> > > > > dm device since opening the /dev/mapper/control device. When
> > > > > select
> > > > > or
> > > > > poll returns the device as readable, we must close and reopen
> > > > > the
> > > > > device
> > > > > to wait for new dm events.
> > > > 
> > > > Why have you done it that way? Couldn't you just save the
> > > > dm_global_event_nr at the time poll() is called?
> > > 
> > > I should have read your patch 2/3 before posting ... but I'm still
> > > missing why the counter can't simply be set at poll() time.
> > 
> > If you did that then you would have a race where:
> > 
> > 1) userspace has recorded events prior to poll()
> > 2) an event triggers an increment of dm_global_event_nr before
> > userspace
> > calls poll()
> > 3) then userspace calls poll() -- only to find that after poll()
> > returns
> > multiple events have occurred.
> > 
> > Which implies missed handling of events.
> 
> I see - but I don't see yet how the ioctl approach (or the
> close()/open() based one, for that matter) would avoid this race.
> 
>  1) application processes event N
>  2) event N+1 arrives in the kernel
>  3) user space issues ioctl or close()/open() sequence, N+1 is recorded
>     in priv->global_event_nr
>  4) user space runs poll()
>  5) event N+2 arrives 4 weeks later and poll returns
> 
> ... meaning that event N+1 has been left unprocessed for 4 weeks.
> Or what I am missing?

The proper sequence is:
1) open /dev/mapper/control
2) check if the event we are waiting for already happened
3) call poll()

- if you call poll immediatelly after opening, it is incorrect use and 
there is a possibility for a race condition.

> > > > Why have you done it that way? Couldn't you just save the
> > > > dm_global_event_nr at the time poll() is called?

- if we did it this way, the race condition would be unavoidable.

Mikulas

> AFAICS, the only way for user space to make sure it misses no events
> would be passing the number of the last processed event down the kernel
> in the ioctl call (and the kernel would use that value, rather than its
> internal counter, for initializing priv->global_event_nr).
> 
> Regards
> Martin
> 
> 
> 
> > But if I'm wrong I'm sure Andy or Mikulas will correct me.
> > 
> > Thanks,
> > Mike
> > 
> 
> -- 
> Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
>