On Tue, Mar 31, 2020 at 03:38:25PM +0000, Martin Wilck wrote:
> On Mon, 2020-03-30 at 21:00 -0500, Benjamin Marzinski wrote:
> > This library allows other programs to check if a path should be
> > claimed
> > by multipath. Currently, it only includes one function,
> > mpath_is_path(), which takes a device name, mode to for checking the
> > path, and an optional info structure, to return the wwid. The modes
> > work
> > mostly like they do for "multipath -c/-u", with a few exceptions.
> >
> > 1. If a device is currently multipathed, it is always VALID. This
> > perhaps should be true for the existing path valid code.
>
> AFAICS, this is already the case, except if:
>
> 1 WWID_IS_FAILED is set, i.e. dm_addmap() failed to set up
> the multipath with this WWID last time we tried. In this case it's
> unlikely that the path is currently multipathed. But there may be
> some corner case in which your new code would return "valid"
> while the current code would not; possibly if someone set up a
> multipath device with "dmsetup" directly, or because of some race
> condition. I just realized that we don't check for -EBUSY when we
> create a map...
>
> I agree that perhaps the "is_multipath" test should be done before
> the "failed" test in multipath -u, too.
>
> 2 ignore_wwids is off, and check_wwids_file returned a negative
> result. IMO this logic is correct. But you are the inventor of
> the wwids file, so fine with me to change it.
>
> And also if multipathd is neither running nor enabled, but see below.
See below for my thoughts on its placement
>
> >
> > 2. There is no seperate "on" mode. It is instead treated like
> > "smart".
> > This is because the library only looks at a single path, so it can
> > only
> > say that a device could be multpathed, if there was another path. It
> > is
> > the caller's job to check if another path exists, or to wait for
> > another
> > path appear.
>
> I'm not sure if I like this. Returning "no" for the first path and
> "yes" for the second and later paths is exactly how
> "find_multipaths=yes" is supposed to behave. If that's not useful for
> an application, the application should choose a different mode; and
> that's what I believe SID should do (assuming that SID will be the main
> / only user of this API for some time).
SID just wants to be able to look at one path at a time. It will be
storing the information from previous path runs, so it will be able to
check if there are other paths with that wwid. I really don't want to
run coalesce_paths in the library, to search repeatedly for all the
paths. It's completely unnecessary, since the information will have
already been gotten by previous calls to the library.
To deal with SID, I could have the function take an array of path_info
structures holding all the known wwids. This would allow the function to
return "No" for the first path in the "find_multipaths=yes" case.
However, this would make the library worse for use by "multipath -u",
since multipath would have to gather the existing path wwids before it
knew if it needed them.
Assuming we are moving the main part of this into libmultipath, I could
make that run in two modes. One would be like it currently works and not
require the array of wwids, which would be for the "multipath -u" code.
The other would require the array, and that would be what libmpathvalid
used. Perhaps something as simply as passing -1 for the array length
could mean make no assumptions about other paths, and return "maybe", if
this path needs a second path in order to be claimed. Then the
"multipath -u" could run coalesce_paths() later, if necessary.
> >
> > 3. The library does check if there already is an existing multipath
> > device with the same wwid, and if so, will declare the path valid.
>
> What if there are other paths, not multipathed (yet) but with the same
> wwid as the path in question? The current code checks that by calling
> coalesce_paths() in "dry-run" mode, which would cover both this case
> and your case 3.
>
>
> >
> > In order to act more library-like, libmpathvalid doesn't set its own
> > device-mapper log functions. It leaves this to the caller. It
> > currently
> > keeps condlog() from printing anything, but should eventually include
> > a
> > function to allow the caller set the logging. It also uses a
> > statically
> > compiled version of libmultipath, both to keep that library from
> > polluting the namespace of the caller, and to avoid the caller
> > needing
> > to set up the variables and functions (like logsink, and
> > get_multipath_config) that it expects.
>
>
> All fair, but I'd prefer a solution where we use as much common code
> as possible, to avoid bit rot of either code path in the future. Your
> new function has the advantage to be much better readable than the
> current code in multipath/main.c. Maybe we can find a way to use it
> from "multipath -u"? The mentioned semantic changes are minor and could
> be resolved (not sure about the coalesce_paths() call, I guess you had
> a reason to skip it).
>
> The namespace issue could be fixed by using
> "-fvisibility=hidden" and using explicit visibility attributes for
> those functions we want to export. The logsink and get_multipath_config
> issues should be solvable by using a sane default implementation and
> allowing applications to change it.
>
> Both would be improvements for libmultipath that we should have made
> long ago.
Having libmultipath store logsink and get_multipath_config, and making
getter and setter functions would be great. But I'm worried that making
that change might mess with current users of libmpathpersist.
There are still a number of functions that multipath, multipathd, and
libmpathpersist need from libmultipath, that libmpathvalid doesn't, but
-fvisibility=hidden would fix a large part of my dislike with the
namesapce polution. I'm still not crazy about telling people to link
against a library with a number of exposed, undocumented functions, but
we already been doing it for a while with libmpathpersist.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> > ---
> > Makefile | 1 +
> > Makefile.inc | 1 +
> > libmpathvalid/Makefile | 38 +++++++
> > libmpathvalid/libmpathvalid.version | 6 +
> > libmpathvalid/mpath_valid.c | 171
> > ++++++++++++++++++++++++++++
> > libmpathvalid/mpath_valid.h | 53 +++++++++
> > libmultipath/Makefile | 1 +
> > libmultipath/devmapper.c | 49 ++++++++
> > libmultipath/devmapper.h | 1 +
> > 9 files changed, 321 insertions(+)
> > create mode 100644 libmpathvalid/Makefile
> > create mode 100644 libmpathvalid/libmpathvalid.version
> > create mode 100644 libmpathvalid/mpath_valid.c
> > create mode 100644 libmpathvalid/mpath_valid.h
> >
> > diff --git a/Makefile b/Makefile
> > index 1dee3680..462d6655 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -9,6 +9,7 @@ BUILDDIRS := \
> > libmultipath/checkers \
> > libmultipath/foreign \
> > libmpathpersist \
> > + libmpathvalid \
> > multipath \
> > multipathd \
> > mpathpersist \
> > diff --git a/Makefile.inc b/Makefile.inc
> > index d4d1e0dd..7e0e8203 100644
> > --- a/Makefile.inc
> > +++ b/Makefile.inc
> > @@ -66,6 +66,7 @@ libdir = $(prefix)/$(LIB)/multipath
> > unitdir = $(prefix)/$(SYSTEMDPATH)/systemd/system
> > mpathpersistdir = $(TOPDIR)/libmpathpersist
> > mpathcmddir = $(TOPDIR)/libmpathcmd
> > +mpathvaliddir = $(TOPDIR)/libmpathvalid
> > thirdpartydir = $(TOPDIR)/third-party
> > libdmmpdir = $(TOPDIR)/libdmmp
> > nvmedir = $(TOPDIR)/libmultipath/nvme
> > diff --git a/libmpathvalid/Makefile b/libmpathvalid/Makefile
> > new file mode 100644
> > index 00000000..5fc97022
> > --- /dev/null
> > +++ b/libmpathvalid/Makefile
> > @@ -0,0 +1,38 @@
> > +include ../Makefile.inc
> > +
> > +SONAME = 0
> > +DEVLIB = libmpathvalid.so
> > +LIBS = $(DEVLIB).$(SONAME)
> > +
> > +CFLAGS += $(LIB_CFLAGS) -I$(multipathdir) -I$(mpathcmddir)
> > +
> > +LIBDEPS += -lpthread -ldevmapper -ldl -L$(multipathdir) \
> > + -lmultipath_nonshared -L$(mpathcmddir) -lmpathcmd -ludev
> > +
> > +OBJS = mpath_valid.o
> > +
> > +all: $(LIBS)
> > +
> > +$(LIBS): $(OBJS)
> > + $(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS)
> > $(LIBDEPS) -Wl,--version-script=libmpathvalid.version
> > + $(LN) $(LIBS) $(DEVLIB)
> > +
> > +install: $(LIBS)
> > + $(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(syslibdir)
> > + $(INSTALL_PROGRAM) -m 755 $(LIBS)
> > $(DESTDIR)$(syslibdir)/$(LIBS)
> > + $(LN) $(LIBS) $(DESTDIR)$(syslibdir)/$(DEVLIB)
> > + $(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(includedir)
> > + $(INSTALL_PROGRAM) -m 644 mpath_valid.h $(DESTDIR)$(includedir)
> > +
> > +uninstall:
> > + $(RM) $(DESTDIR)$(syslibdir)/$(LIBS)
> > + $(RM) $(DESTDIR)$(syslibdir)/$(DEVLIB)
> > + $(RM) $(DESTDIR)$(includedir)/mpath_valid.h
> > +
> > +clean: dep_clean
> > + $(RM) core *.a *.o *.so *.so.* *.gz
> > +
> > +include $(wildcard $(OBJS:.o=.d))
> > +
> > +dep_clean:
> > + $(RM) $(OBJS:.o=.d)
> > diff --git a/libmpathvalid/libmpathvalid.version
> > b/libmpathvalid/libmpathvalid.version
> > new file mode 100644
> > index 00000000..e8967951
> > --- /dev/null
> > +++ b/libmpathvalid/libmpathvalid.version
> > @@ -0,0 +1,6 @@
> > +MPATH_1.0 {
> > + global:
> > + mpath_is_path;
> > + local:
> > + *;
> > +};
> > diff --git a/libmpathvalid/mpath_valid.c
> > b/libmpathvalid/mpath_valid.c
> > new file mode 100644
> > index 00000000..3d96c32f
> > --- /dev/null
> > +++ b/libmpathvalid/mpath_valid.c
> > @@ -0,0 +1,171 @@
> > +#include <stdio.h>
> > +#include <stdint.h>
> > +#include <libdevmapper.h>
> > +#include <libudev.h>
> > +#include <errno.h>
> > +
> > +#include "devmapper.h"
> > +#include "structs.h"
> > +#include "util.h"
> > +#include "config.h"
> > +#include "discovery.h"
> > +#include "wwids.h"
> > +#include "sysfs.h"
> > +#include "mpath_cmd.h"
> > +#include "mpath_valid.h"
> > +
> > +struct udev *udev;
> > +int logsink = -1;
> > +static struct config default_config = { .verbosity = -1 };
> > +static struct config *multipath_conf;
> > +
> > +struct config *get_multipath_config(void)
> > +{
> > + return (multipath_conf)? multipath_conf : &default_config;
> > +}
> > +
> > +void put_multipath_config(__attribute__((unused))void *conf)
> > +{
> > + /* Noop */
> > +}
> > +
> > +static int get_conf_mode(struct config *conf)
> > +{
> > + if (conf->find_multipaths == FIND_MULTIPATHS_ON ||
> > + conf->find_multipaths == FIND_MULTIPATHS_SMART)
> > + return MPATH_SMART;
> > + if (conf->find_multipaths == FIND_MULTIPATHS_GREEDY)
> > + return MPATH_GREEDY;
> > + return MPATH_STRICT;
> > +}
> > +
> > +
> > +/*
> > + * Return
> > + * 2: possible path (only in MPATH_GREEDY mode)
> > + * 1: mpath path
> > + * 0: not mpath path
> > + * -1: Failure
> > + */
> > +int
> > +mpath_is_path(const char *name, unsigned int mode, struct mpath_info
> > *info)
> > +{
> > + struct config *conf;
> > + struct path * pp;
> > + int r = MPATH_IS_ERROR;
> > + int fd = -1;
> > + unsigned int version[3];
> > + bool already_multipathed = false;
> > +
> > + if (info)
> > + memset(info, 0, sizeof(struct mpath_info));
> > +
> > + if (!name || mode >= MPATH_MAX_MODE)
> > + return r;
> > +
> > + skip_libmp_dm_init();
> > + udev = udev_new();
> > + if (!udev)
> > + goto out;
> > + conf = load_config(DEFAULT_CONFIGFILE);
> > + if (!conf)
> > + goto out_udev;
> > + conf->verbosity = -1;
> > + if (mode == MPATH_DEFAULT)
> > + mode = get_conf_mode(conf);
> > +
> > + if (dm_prereq(version))
> > + goto out_config;
> > + memcpy(conf->version, version, sizeof(version));
> > + multipath_conf = conf;
> > +
> > + pp = alloc_path();
> > + if (!pp)
> > + goto out_config;
> > +
> > + if (safe_sprintf(pp->dev, "%s", name))
> > + goto out_path;
> > +
> > + if (sysfs_is_multipathed(pp, true)) {
> > + if (!info || pp->wwid[0] != '\0') {
> > + r = MPATH_IS_VALID;
> > + goto out_path;
> > + }
> > + already_multipathed = true;
> > + }
> > +
> > + fd = __mpath_connect(1);
> > + if (fd < 0) {
> > + if (errno != EAGAIN && !systemd_service_enabled(name))
> > {
> > + r = MPATH_IS_NOT_VALID;
> > + goto out_path;
> > + }
> > + } else
> > + mpath_disconnect(fd);
>
> So "multipathd not running" takes precedence over "is already
> multipathed"? That contradicts your statement 1. above. Would SID
> (or any other external application using your API) really require
> multipathd?
I've waffled on this. SID will require multipathd, but if a device
actually is part of a multipath device, then the reality of the
situation is that it has been claimed by multipath, and there will be
things that won't be able to use it. For instance, LVM will not be able
to be set up on a partition, and filesystems won't mount. So, should
the code return "not a multipath path" for a device that IS a multipath
path, just because it shouldn't be, especially when there is nothing
that is going to un-multipath it.
I think that the best answer is to report that the path has been, in
fact, claimed by multipath, even if we don't know why, or think it
shouldn't be. But this is a corner case, and I'm open to being
convinced otherwise.
> > +
> > + pp->udev = udev_device_new_from_subsystem_sysname(udev,
> > "block", name);
> > + if (!pp->udev)
> > + goto out_path;
> > +
> > + r = pathinfo(pp, conf, DI_SYSFS | DI_WWID | DI_BLACKLIST);
> > + if (r) {
> > + if (r == PATHINFO_SKIPPED)
> > + r = MPATH_IS_NOT_VALID;
> > + else
> > + r = MPATH_IS_ERROR;
> > + goto out_path;
> > + }
> > +
> > + if (pp->wwid[0] == '\0') {
> > + r = MPATH_IS_NOT_VALID;
> > + goto out_path;
> > + }
> > +
> > + if (already_multipathed)
> > + goto out_path;
> > +
> > + if (dm_is_mpath_uuid(pp->wwid) == 1) {
> > + r = MPATH_IS_VALID;
> > + goto out_path;
> > + }
> > +
> > + r = is_failed_wwid(pp->wwid);
> > + if (r != WWID_IS_NOT_FAILED) {
> > + if (r == WWID_IS_FAILED)
> > + r = MPATH_IS_NOT_VALID;
> > + else
> > + r = MPATH_IS_ERROR;
> > + goto out_path;
> > + }
> > +
> > + if (mode == MPATH_GREEDY) {
> > + r = MPATH_IS_VALID;
> > + goto out_path;
> > + }
> > +
> > + if (check_wwids_file(pp->wwid, 0) == 0) {
> > + r = MPATH_IS_VALID;
> > + goto out_path;
> > + }
> > +
> > + if (mode == MPATH_STRICT) {
> > + r = MPATH_IS_NOT_VALID;
> > + goto out_path;
> > + }
> > +
> > + /* mode == SMART */
> > + r = MPATH_IS_MAYBE_VALID;
> > +
> > +out_path:
> > + if (already_multipathed)
> > + r = MPATH_IS_VALID;
> > + if (info && (r == MPATH_IS_VALID || r == MPATH_IS_MAYBE_VALID))
> > + strlcpy(info->wwid, pp->wwid, 128);
> > + free_path(pp);
> > +out_config:
> > + free_config(conf);
> > +out_udev:
> > + udev_unref(udev);
> > +out:
> > + return r;
> > +}
> > diff --git a/libmpathvalid/mpath_valid.h
> > b/libmpathvalid/mpath_valid.h
> > new file mode 100644
> > index 00000000..b9e420a8
> > --- /dev/null
> > +++ b/libmpathvalid/mpath_valid.h
> > @@ -0,0 +1,53 @@
> > +/*
> > + * Copyright (C) 2015 Red Hat, Inc.
> > + *
> > + * This file is part of the device-mapper multipath userspace tools.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > License
> > + * along with this program. If not, see <
> > http://www.gnu.org/licenses/>;.
> > + */
> > +
> > +#ifndef LIB_MPATH_VALID_H
> > +#define LIB_MPATH_VALID_H
> > +
> > +#ifdef __cpluscplus
> > +extern "C" {
> > +#endif
> > +
> > +enum mpath_valid_mode {
> > + MPATH_DEFAULT,
> > + MPATH_STRICT,
> > + MPATH_SMART,
> > + MPATH_GREEDY,
> > + MPATH_MAX_MODE, /* used only for bounds checking */
> > +};
> > +
> > +enum mpath_valid_result {
> > + MPATH_IS_ERROR = -1,
> > + MPATH_IS_NOT_VALID,
> > + MPATH_IS_VALID,
> > + MPATH_IS_MAYBE_VALID,
> > +};
> > +
> > +struct mpath_info {
> > + char wwid[128];
> > +};
> > +
> > +int mpath_is_path(const char *name, unsigned int mode, struct
> > mpath_info *info);
> > +
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +#endif /* LIB_PATH_VALID_H */
> > +
> > diff --git a/libmultipath/Makefile b/libmultipath/Makefile
> > index ad690a49..7e2c00cf 100644
> > --- a/libmultipath/Makefile
> > +++ b/libmultipath/Makefile
> > @@ -69,6 +69,7 @@ nvme-ioctl.h: nvme/nvme-ioctl.h
> >
> > $(LIBS): $(OBJS)
> > $(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS)
> > $(LIBDEPS)
> > + ar rcs libmultipath_nonshared.a $(OBJS)
> > $(LN) $@ $(DEVLIB)
> >
> > install:
> > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > index 7ed494a1..27d4f61f 100644
> > --- a/libmultipath/devmapper.c
> > +++ b/libmultipath/devmapper.c
> > @@ -770,6 +770,55 @@ out:
> > return r;
> > }
> >
> > +/*
> > + * Return
> > + * 1 : map with uuid exists
> > + * 0 : map with uuid doesn't exist
> > + * -1 : error
> > + */
> > +int
> > +dm_is_mpath_uuid(const char *uuid)
>
> We should probably replace our current dm_map_present() implementation
> with something like this, checking for the UUID. The name of the
> function irritated me, perhaps it should be called
> dm_map_present_by_uuid() or so.
Sure. There is one thing that I'm conflicted about with this function,
and with sysfs_is_multipathed(). They both return success if the wwid
looks good. But they don't bother to check the the device target is
actually a multipath type. It would add an extra libdm command to both
of them to deal with an unlikely corner case, but nothing stops anyone
from creating a dm device with the mpath- prefix.
> Regards,
> Martin
>
>
> > +{
> > + struct dm_task *dmt;
> > + struct dm_info info;
> > + char prefixed_uuid[WWID_SIZE + UUID_PREFIX_LEN];
> > + int r = -1;
> > +
> > + if (!uuid || uuid[0] == '\0')
> > + return 0;
> > +
> > + if (safe_sprintf(prefixed_uuid, UUID_PREFIX "%s", uuid))
> > + goto out;
> > +
> > + if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> > + goto out;
> > +
> > + dm_task_no_open_count(dmt);
> > +
> > + if (!dm_task_set_uuid(dmt, prefixed_uuid))
> > + goto out_task;
> > +
> > + if (!dm_task_run(dmt))
> > + goto out_task;
> > +
> > + if (!dm_task_get_info(dmt, &info))
> > + goto out_task;
> > +
> > + r = 0;
> > +
> > + if (!info.exists)
> > + goto out_task;
> > +
> > + r = 1;
> > +out_task:
> > + dm_task_destroy(dmt);
> > +out:
> > + if (r < 0)
> > + condlog(3, "%s: dm command failed in %s: %s", uuid,
> > + __FUNCTION__, strerror(errno));
> > + return r;
> > +}
> > +
> > static int
> > dm_dev_t (const char * mapname, char * dev_t, int len)
> > {
> > diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> > index 17fc9faf..fa2513c2 100644
> > --- a/libmultipath/devmapper.h
> > +++ b/libmultipath/devmapper.h
> > @@ -43,6 +43,7 @@ int dm_get_map(const char *, unsigned long long *,
> > char *);
> > int dm_get_status(const char *, char *);
> > int dm_type(const char *, char *);
> > int dm_is_mpath(const char *);
> > +int dm_is_mpath_uuid(const char *uuid);
> > int _dm_flush_map (const char *, int, int, int, int);
> > int dm_flush_map_nopaths(const char * mapname, int deferred_remove);
> > #define dm_flush_map(mapname) _dm_flush_map(mapname, 1, 0, 0, 0)
>
> --
> Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
> SUSE Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix
> Imendörffer
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
