[dm-devel] [RFC PATCH 5/5] multipathd: RFC add new polling dmevents waiter thread

[Martin Wilck]

Hi Ben,

On Mon, 2018-02-12 at 17:18 -0600, Benjamin Marzinski wrote:
> On Sat, Feb 10, 2018 at 08:55:53PM +0100, Martin Wilck wrote:
> > Hi Ben,
> > 
> > thanks a lot for this. I have only a few minor nitpicks (see
> > below).
> > I suppose you've tested this already?
> 
> Yes. I do plan on doing some more testing after I look into making
> libdevmapper support re-arming the polling interface and grabbing the
> event number from the names listing, before I repost this without the
> RFC tag. I was also thinking of trying out cmocka by mocking up a
> device-mapper interface that let me test this code in isolation.

Great idea.

Am I understanding correctly that you are working on libdevmapper in
parallel? If yes, would it make sense to have libmultipath use the
newly developed libdevmapper API right away, rather than using a
custom-made ioctl interface until libdevmapper is ready?

> > > I haven't touched any of the existing event waiting code, since
> > > event
> > > polling was only added to device-mapper in version
> > > 4.37.0.  multipathd
> > > checks this version, and defaults to using the polling code if
> > > device-mapper supports it. This can be overridden by running
> > > multipathd
> > > with "-w", to force it to use the old event waiting code.
> > 
> > Why use a command line option here rather than a config file
> > option?
> 
> Mostly because it was faster, and I wanted to get to testing it. The
> other reason is that I don't see any benefit for the work involved in
> making this be changeable in
> 
> # multipathd reconfigure
> 
> However, we already have configuration settings that can't get
> changed
> on reconfigure, so making this another one is not a big deal. I agree
> that it is easier for users to change if it is a configuration
> setting,
> but I'm hoping that this change will be invisible to users. If you
> would
> prefer it as a configuration setting, I have no problem with changing
> that
> .

Right. It doesn't need to be user-configurable. We may want to leave a
compile-time option to disable it for the time being.

> > > 
> > > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > > ---
> > >  multipathd/Makefile   |   3 +-
> > >  multipathd/dmevents.c | 396
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  multipathd/dmevents.h |  13 ++
> > >  multipathd/main.c     |  58 +++++++-
> > >  4 files changed, 461 insertions(+), 9 deletions(-)
> > >  create mode 100644 multipathd/dmevents.c
> > >  create mode 100644 multipathd/dmevents.h
> > > 
> > > diff --git a/multipathd/Makefile b/multipathd/Makefile
> > > index 85f29a7..4c438f0 100644
> > > --- a/multipathd/Makefile
> > > +++ b/multipathd/Makefile
> > > @@ -22,7 +22,8 @@ ifdef SYSTEMD
> > >  	endif
> > >  endif
> > >  
> > > -OBJS = main.o pidfile.o uxlsnr.o uxclnt.o cli.o cli_handlers.o
> > > waiter.o
> > > +OBJS = main.o pidfile.o uxlsnr.o uxclnt.o cli.o cli_handlers.o
> > > waiter.o \
> > > +       dmevents.o
> > >  
> > >  EXEC = multipathd
> > >  
> > > diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
> > > new file mode 100644
> > > index 0000000..a56c055
> > > --- /dev/null
> > > +++ b/multipathd/dmevents.c
> > > 

> > > +
> > > +
> > > +int alloc_dmevent_waiter(struct vectors *vecs)
> > > +{
> > > +	if (!vecs) {
> > > 
> > Nitpick: conventionally, an "alloc"-type function would return the
> > pointer, and NULL on failure.
> 
> Is this a naming complaint, or an interface complaint?  I'm fine with
> changing the names so they follow the lead of checkers and prio, i.e.
> init_dmevents_waiter() and cleanup_dmevents_waiter(). The init and
> cleanup functions for checkers and prio have the same returns as the
> dmevent functions (well the init functions return 1 for failure, and
> I
> can do that as well)

I'm fine with simply changing the names.

Regards
Martin

-- 
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)