Re: [dm-devel] [PATCH v2 30/48] multipathd: uxlsnr: add idle notification

[Benjamin Marzinski <bmarzins@redhat.com>]

On Fri, Nov 26, 2021 at 03:23:18PM +0100, Martin Wilck wrote:
> On Wed, 2021-11-24 at 19:08 -0600, Benjamin Marzinski wrote:
> > On Thu, Nov 18, 2021 at 11:58:22PM +0100, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > The previous patches added the state machine and the timeout
> > > handling,
> > > but there was no wakeup mechanism for the uxlsnr for cases where
> > > client connections were waiting for the vecs lock.
> > > 
> > > This patch uses the previously introduced wakeup mechanism of
> > > struct mutex_lock for this purpose. Processes which unlock the
> > > "global" vecs lock send an event in an eventfd which the uxlsnr
> > > loop is polling for.
> > > 
> > > As we are now woken up for servicing client handlers that don't
> > > wait for input but for the lock, we need to set up the pollfds
> > > differently, and iterate over all clients when handling events,
> > > not only over the ones that are receiving. The hangup handling
> > > is changed, too. We have to look at every client, even if one has
> > > hung up. Note that I don't take client_lock for the loop in
> > > uxsock_listen(), it's not necessary and will be removed elsewhere
> > > in a follow-up patch.
> > > 
> > > With this in place, the lock need not be taken in execute_handler()
> > > any more. The uxlsnr only ever calls trylock() on the vecs lock,
> > > avoiding any waiting for other threads to finish.
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  multipathd/uxlsnr.c | 200 ++++++++++++++++++++++++++++------------
> > > ----
> > >  1 file changed, 129 insertions(+), 71 deletions(-)
> > > 
> > > diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> > > index 87134d5..bf9780d 100644
> > > --- a/multipathd/uxlsnr.c
> > > +++ b/multipathd/uxlsnr.c
> > 
> > Nitpick: This would look clearer to me if, instead of a switch
> > statement, it was just
> > 
> > if (c->state != CLT_RECV)
> >         continue;
> > 
> > polls[i].events = POLLIN;
> > polls[i].fd = c->fd;
> > ...
> 
> That's true if you look at this patch in isolation. The reason I use a
> switch statement is that with patch 32, we get another case to treat
> here (CLT_SEND). At that point, the switch is at least on par wrt
> clarity, IMO.
> 
> No?

Yep. I retrack my objection

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> 
> Martin

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel