Re: [dm-devel] [RFC PATCH 5/5] multipathd: RFC add new polling dmevents waiter thread
On Tue, 2018-02-13 at 10:49 -0600, Benjamin Marzinski wrote: > On Tue, Feb 13, 2018 at 09:50:19AM +0100, Martin Wilck wrote: > > Hi Ben, > > > > 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 been working on adding the re-arming support to > libdevmapper. > I just started looking into that now that I have all of these > multipath > patches posted. > > I'm not sure I understand you suggestion. There's a large amount of > code > that can get executed when you call dm_task_run(). But the core bit > of > code that it would execute for the DM_DEV_ARM_POLL command is that > ioctl. Also, the calculation to find the offset of the event number > in > the dm_names structure will be the same when libdevmapper does them. > I > have no problem with moving the functions I wrote (arm_dev_event_poll > and dm_event_nr) to libmultipath/devmapper.c, where they will > eventually > use libdevmapper to do their work, but the actual code they will > execute > as part of libdevmapper will be functionally the same. OK. I think I misunderstood your remark about libdevmapper support. Just go ahead according to your initial plan, fine with me. Regards Martin -- Dr. Martin Wilck , Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [RFC PATCH 5/5] multipathd: RFC add new polling dmevents waiter thread
On Tue, Feb 13, 2018 at 09:50:19AM +0100, Martin Wilck wrote:
> 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 been working on adding the re-arming support to libdevmapper.
I just started looking into that now that I have all of these multipath
patches posted.
I'm not sure I understand you suggestion. There's a large amount of code
that can get executed when you call dm_task_run(). But the core bit of
code that it would execute for the DM_DEV_ARM_POLL command is that
ioctl. Also, the calculation to find the offset of the event number in
the dm_names structure will be the same when libdevmapper does them. I
have no problem with moving the functions I wrote (arm_dev_event_poll
and dm_event_nr) to libmultipath/devmapper.c, where they will eventually
use libdevmapper to do their work, but the actual code they will execute
as part of libdevmapper will be functionally the same.
> > > > 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.
>
I'm fine with adding a compile-time option. When this option is
compiled in, we do want to make multipathd able to use either method,
since not everyone will be running on a recent enough kernel. Since we
are doing that, I do want to keep some way to force the old method, even
if it is just for testing and debuging purposes. So I would like to keep
the -w option.
> > > >
> > > > Signed-off-by: Benjamin Marzinski
> > > > ---
> > > > 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 000..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_dmeven
Re: [dm-devel] [RFC PATCH 5/5] multipathd: RFC add new polling dmevents waiter thread
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
> > > ---
> > > 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 000..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 , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [RFC PATCH 5/5] multipathd: RFC add new polling dmevents waiter thread
On Mon, Feb 12, 2018 at 05:18:02PM -0600, Benjamin Marzinski wrote: > It is the DM_EXISTS_FLAG. It's defined in libdm/ioctl/libdm-iface.c in > the lvm2 source. It is unconditionally set on all dm control ioctls > from libdevmapper by _do_dm_ioctl() (also in libdm/ioctl/libdm-iface.c). > I don't know the reason for this. I don't see anything that uses it in > driver/md/dm-ioctl.c, and I see that line in the libdm source has a > /* FIXME */ next to it. On the other hand, all I'm trying to do here is > run the same ioctl that libdevmapper would if it supported this command, > and there may well be a reason for it that I'm missing. DM_EXISTS_FLAG indicates that the device the ioctl referenced exists. If you performed certain queries and the device was not found, you got a successful return but without this flag. In the dim and distant past this flag was handled kernel-side. Later, we dropped it but left that userspace code emulating it. The FIXMEs were just saying that the library code could be cleaned up too one day. Ref. https://lwn.net/Articles/38512/ Alasdair -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [RFC PATCH 5/5] multipathd: RFC add new polling dmevents waiter thread
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.
>
> Regards
> Martin
>
> On Fri, 2018-02-09 at 23:07 -0600, Benjamin Marzinski wrote:
> > The current method of waiting for dmevents on multipath devices
> > involves
> > creating a seperate thread for each device. This can become very
> > wasteful when there are large numbers of multipath devices. Also,
> > since
> > multipathd needs to grab the vecs lock to update the devices, the
> > additional threads don't actually provide much parallelism.
> >
> > The patch adds a new method of updating multipath devices on
> > dmevents,
> > which uses the new device-mapper event polling interface. This means
> > that there is only one dmevent waiting thread which will wait for
> > events
> > on all of the multipath devices. Currently the code to get the event
> > number from the list of device names and to re-arm the polling
> > interface
> > is not in libdevmapper, so the patch does that work. Obviously, these
> > bits need to go into libdevmapper, so that multipathd can use a
> > standard
> > interface.
> >
> > 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.
> >
> > Signed-off-by: Benjamin Marzinski
> > ---
> > 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 000..a56c055
> > --- /dev/null
> > +++ b/multipathd/dmevents.c
> > @@ -0,0 +1,396 @@
> > +/*
> > + * Copyright (c) 2004, 2005 Christophe Varoqui
> > + * Copyright (c) 2005 Kiyoshi Ueda, NEC
> > + * Copyright (c) 2005 Edward Goggin, EMC
> > + * Copyright (c) 2005, 2018 Benjamin Marzinski, Redhat
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "vector.h"
> > +#include "structs.h"
> > +#include "structs_vec.h"
> > +#include "devmapper.h"
> > +#include "debug.h"
> > +#include "dmevents.h"
> > +
> > +#ifndef DM_DEV_ARM_POLL
> > +#define DM_DEV_ARM_POLL _IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD + 1,
> > struct dm_ioctl)
> > +#endif
> > +
> > +enum event_actions {
> > + EVENT_NOTHING,
> > + EVENT_REMOVE,
> > + EVENT_UPDATE,
> > +};
> > +
> > +struct dev_event {
> > + char name[WWID_SIZE];
> > + uint32_t evt_nr;
> > + enum event_actions action;
> > +};
> > +
> > +struct dmevent_waiter {
> > + int fd;
> > + struct vectors *vecs;
> > + vector events;
> > + pthread_mutex_t events_lock;
> > +};
> > +
> > +static struct dmevent_waiter *waiter;
> > +
> > +int dmevent_poll_supported(void)
> > +{
> > + unsigned int minv[3] = {4, 37, 0};
> > + unsigned int v[3];
> > +
> > + if (dm_drv_version(v))
> > + return 0;
> > +
> > + if (VERSION_GE(v, minv))
> > + return 1;
> > + return 0;
> > +}
> > +
>
Re: [dm-devel] [RFC PATCH 5/5] multipathd: RFC add new polling dmevents waiter thread
Hi Ben,
thanks a lot for this. I have only a few minor nitpicks (see below).
I suppose you've tested this already?
Regards
Martin
On Fri, 2018-02-09 at 23:07 -0600, Benjamin Marzinski wrote:
> The current method of waiting for dmevents on multipath devices
> involves
> creating a seperate thread for each device. This can become very
> wasteful when there are large numbers of multipath devices. Also,
> since
> multipathd needs to grab the vecs lock to update the devices, the
> additional threads don't actually provide much parallelism.
>
> The patch adds a new method of updating multipath devices on
> dmevents,
> which uses the new device-mapper event polling interface. This means
> that there is only one dmevent waiting thread which will wait for
> events
> on all of the multipath devices. Currently the code to get the event
> number from the list of device names and to re-arm the polling
> interface
> is not in libdevmapper, so the patch does that work. Obviously, these
> bits need to go into libdevmapper, so that multipathd can use a
> standard
> interface.
>
> 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?
>
> Signed-off-by: Benjamin Marzinski
> ---
> 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 000..a56c055
> --- /dev/null
> +++ b/multipathd/dmevents.c
> @@ -0,0 +1,396 @@
> +/*
> + * Copyright (c) 2004, 2005 Christophe Varoqui
> + * Copyright (c) 2005 Kiyoshi Ueda, NEC
> + * Copyright (c) 2005 Edward Goggin, EMC
> + * Copyright (c) 2005, 2018 Benjamin Marzinski, Redhat
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "vector.h"
> +#include "structs.h"
> +#include "structs_vec.h"
> +#include "devmapper.h"
> +#include "debug.h"
> +#include "dmevents.h"
> +
> +#ifndef DM_DEV_ARM_POLL
> +#define DM_DEV_ARM_POLL _IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD + 1,
> struct dm_ioctl)
> +#endif
> +
> +enum event_actions {
> + EVENT_NOTHING,
> + EVENT_REMOVE,
> + EVENT_UPDATE,
> +};
> +
> +struct dev_event {
> + char name[WWID_SIZE];
> + uint32_t evt_nr;
> + enum event_actions action;
> +};
> +
> +struct dmevent_waiter {
> + int fd;
> + struct vectors *vecs;
> + vector events;
> + pthread_mutex_t events_lock;
> +};
> +
> +static struct dmevent_waiter *waiter;
> +
> +int dmevent_poll_supported(void)
> +{
> + unsigned int minv[3] = {4, 37, 0};
> + unsigned int v[3];
> +
> + if (dm_drv_version(v))
> + return 0;
> +
> + if (VERSION_GE(v, minv))
> + return 1;
> + return 0;
> +}
> +
> +
> +int alloc_dmevent_waiter(struct vectors *vecs)
> +{
> + if (!vecs) {
> + condlog(0, "can't create waiter structure. invalid
> vectors");
> + goto fail;
> + }
> + waiter = (struct dmevent_waiter *)malloc(sizeof(struct
> dmevent_waiter));
> + if (!waiter) {
> + condlog(0, "failed to allocate waiter structure");
> + goto fail;
> + }
> + memset(waiter, 0, sizeof(struct dmevent_waiter));
> + waiter->events = vector_alloc();
> + if (!waiter->events) {
> + condlog(0, "failed to allocate waiter events
> vector");
> + goto fail_waiter;
> + }
> + waiter->fd = open("/dev/mapper/control", O_RDWR);
> + if (waiter->fd < 0) {
> + condlog(0, "failed to open /dev/mapper/control for
> waiter");
> + goto fail_events;
> + }
> + pthread_mutex_init(&waiter->events_lock, NULL);
> + waiter->vecs = vecs;
> +
> + return 0;
> +fail_events:
> + vector_free(waiter->events);
> +fail_waiter:
> + free(waiter);
> +fail:
> + waiter = NULL;
> + return -1;
> +}
Nitpick: conventionally, an "alloc"-type function would return the
pointer, and N
