| Message ID | 1589507962-6895-4-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath: path validation library prep work | expand |
On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote: > This code pulls the multipath path validation code out of > configure(), > and puts it into its own function, check_path_valid(). This function > calls a new libmultipath function, is_path_valid() to check just path > requested. This seperation exists so that is_path_valid() can be > reused > by future code. This code will give almost the same answer as the > existing code, with the exception that now, if a device is currently > multipathed, it will always be a valid multipath path. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Great job getting the logic right! Readability massively improved. Almost ack, a few comments and questions below. Regards, Martin > --- > libmultipath/Makefile | 2 +- > libmultipath/devmapper.c | 49 +++++++ > libmultipath/devmapper.h | 1 + > libmultipath/structs.h | 24 +--- > libmultipath/valid.c | 118 ++++++++++++++++ > libmultipath/valid.h | 32 +++++ > libmultipath/wwids.c | 10 +- > multipath/main.c | 296 +++++++++++++++++------------------ > ---- > 8 files changed, 337 insertions(+), 195 deletions(-) > create mode 100644 libmultipath/valid.c > create mode 100644 libmultipath/valid.h > > diff --git a/libmultipath/Makefile b/libmultipath/Makefile > index f19b7ad2..e5dac5ea 100644 > --- a/libmultipath/Makefile > +++ b/libmultipath/Makefile > @@ -48,7 +48,7 @@ OBJS = memory.o parser.o vector.o devmapper.o > callout.o \ > log.o configure.o structs_vec.o sysfs.o prio.o checkers.o \ > lock.o file.o wwids.o prioritizers/alua_rtpg.o prkey.o \ > io_err_stat.o dm-generic.o generic.o foreign.o nvme-lib.o \ > - libsg.o > + libsg.o valid.o > > all: $(LIBS) > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > index 7ed494a1..92f61133 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_map_present_by_uuid(const char *uuid) > +{ > + 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; I have nothing against goto for error handling, but here I'd prefer r = !!info.exists; > +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..5ed7edc5 100644 > --- a/libmultipath/devmapper.h > +++ b/libmultipath/devmapper.h > @@ -39,6 +39,7 @@ int dm_simplecmd_noflush (int, const char *, > uint16_t); > int dm_addmap_create (struct multipath *mpp, char *params); > int dm_addmap_reload (struct multipath *mpp, char *params, int > flush); > int dm_map_present (const char *); > +int dm_map_present_by_uuid(const char *uuid); > int dm_get_map(const char *, unsigned long long *, char *); > int dm_get_status(const char *, char *); > int dm_type(const char *, char *); > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > index 9bd39eb1..d69bc2e9 100644 > --- a/libmultipath/structs.h > +++ b/libmultipath/structs.h > @@ -101,29 +101,13 @@ enum yes_no_undef_states { > YNU_YES, > }; > > -#define _FIND_MULTIPATHS_F (1 << 1) > -#define _FIND_MULTIPATHS_I (1 << 2) > -#define _FIND_MULTIPATHS_N (1 << 3) > -/* > - * _FIND_MULTIPATHS_F must have the same value as YNU_YES. > - * Generate a compile time error if that isn't the case. > - */ > -extern char ___error1___[-(_FIND_MULTIPATHS_F != YNU_YES)]; > - > -#define find_multipaths_on(conf) \ > - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_F)) > -#define ignore_wwids_on(conf) \ > - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_I)) > -#define ignore_new_devs_on(conf) \ > - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_N)) > - > enum find_multipaths_states { > FIND_MULTIPATHS_UNDEF = YNU_UNDEF, > FIND_MULTIPATHS_OFF = YNU_NO, > - FIND_MULTIPATHS_ON = _FIND_MULTIPATHS_F, > - FIND_MULTIPATHS_GREEDY = _FIND_MULTIPATHS_I, > - FIND_MULTIPATHS_SMART = _FIND_MULTIPATHS_F|_FIND_MULTIPATHS_I, > - FIND_MULTIPATHS_STRICT = _FIND_MULTIPATHS_F|_FIND_MULTIPATHS_N, > + FIND_MULTIPATHS_ON = YNU_YES, > + FIND_MULTIPATHS_GREEDY, > + FIND_MULTIPATHS_SMART, > + FIND_MULTIPATHS_STRICT, > __FIND_MULTIPATHS_LAST, > }; > What was the reason you changed these definitions? AFAICS you've got the logic right, and I'm not saying it shouldn't be done, but I'd like to see a rationale. Is it just simplification / readability? (Note to self: _FIND_MULTIPATHS_F etc. were introduced to make it obvious at the time that these flags had the same effect as the previous "ignore_wwids", "ignore_new_devs", and "find_multipaths" command line options). > diff --git a/libmultipath/valid.c b/libmultipath/valid.c > new file mode 100644 > index 00000000..456b1f6e > --- /dev/null > +++ b/libmultipath/valid.c > @@ -0,0 +1,118 @@ > +/* > + Copyright (c) 2020 Benjamin Marzinski, IBM > + > + This program is free software; you can redistribute it and/or > + modify it under the terms of the GNU 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 General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see < > https://www.gnu.org/licenses/>;. > + */ > +#include <stddef.h> > +#include <errno.h> > +#include <libudev.h> > + > +#include "vector.h" > +#include "config.h" > +#include "debug.h" > +#include "util.h" > +#include "devmapper.h" > +#include "discovery.h" > +#include "wwids.h" > +#include "sysfs.h" > +#include "blacklist.h" > +#include "mpath_cmd.h" > +#include "valid.h" > + > +int > +is_path_valid(const char *name, struct config *conf, struct path > *pp, > + bool check_multipathd) > +{ > + int r; > + int fd; > + > + if (!pp || !name || !conf) > + return PATH_IS_ERROR; > + > + if (conf->find_multipaths <= FIND_MULTIPATHS_UNDEF || > + conf->find_multipaths >= __FIND_MULTIPATHS_LAST) > + return PATH_IS_ERROR; > + > + if (safe_sprintf(pp->dev, "%s", name)) > + return PATH_IS_ERROR; > + > + if (sysfs_is_multipathed(pp, true)) { > + if (pp->wwid[0] == '\0') > + return PATH_IS_ERROR; > + return PATH_IS_VALID_NO_CHECK; > + } > + > + /* > + * "multipath -u" may be run before the daemon is started. In > this > + * case, systemd might own the socket but might delay > multipathd > + * startup until some other unit (udev settle!) has finished > + * starting. With many LUNs, the listen backlog may be > exceeded, which > + * would cause connect() to block. This causes udev workers > calling > + * "multipath -u" to hang, and thus creates a deadlock, until > "udev > + * settle" times out. To avoid this, call connect() in non- > blocking > + * mode here, and take EAGAIN as indication for a filled-up > systemd > + * backlog. > + */ > + > + if (check_multipathd) { > + fd = __mpath_connect(1); > + if (fd < 0) { > + if (errno != EAGAIN && > !systemd_service_enabled(name)) { > + condlog(3, "multipathd not running or > enabled"); > + return PATH_IS_NOT_VALID; > + } > + } else > + mpath_disconnect(fd); > + } > + > + pp->udev = udev_device_new_from_subsystem_sysname(udev, > "block", name); > + if (!pp->udev) > + return PATH_IS_ERROR; > + > + r = pathinfo(pp, conf, DI_SYSFS | DI_WWID | DI_BLACKLIST); > + if (r == PATHINFO_SKIPPED) > + return PATH_IS_NOT_VALID; > + else if (r) > + return PATH_IS_ERROR; > + > + if (pp->wwid[0] == '\0') > + return PATH_IS_NOT_VALID; > + > + if (pp->udev && pp->uid_attribute && > + filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0) > + return PATH_IS_NOT_VALID; > + > + r = is_failed_wwid(pp->wwid); > + if (r != WWID_IS_NOT_FAILED) { > + if (r == WWID_IS_FAILED) > + return PATH_IS_NOT_VALID; > + return PATH_IS_ERROR; > + } > + > + if (conf->find_multipaths == FIND_MULTIPATHS_GREEDY) > + return PATH_IS_VALID; > + > + if (check_wwids_file(pp->wwid, 0) == 0) > + return PATH_IS_VALID_NO_CHECK; > + > + if (dm_map_present_by_uuid(pp->wwid) == 1) > + return PATH_IS_VALID; > + > + /* all these act like FIND_MULTIPATHS_STRICT for finding if a > + * path is valid */ > + if (conf->find_multipaths != FIND_MULTIPATHS_SMART) > + return PATH_IS_NOT_VALID; > + > + return PATH_IS_MAYBE_VALID; > +} > diff --git a/libmultipath/valid.h b/libmultipath/valid.h > new file mode 100644 > index 00000000..778658ad > --- /dev/null > +++ b/libmultipath/valid.h > @@ -0,0 +1,32 @@ > +/* > + Copyright (c) 2020 Benjamin Marzinski, IBM > + > + This program is free software; you can redistribute it and/or > + modify it under the terms of the GNU 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 General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see < > https://www.gnu.org/licenses/>;. > + */ > +#ifndef _VALID_H > +#define _VALID_H > + > +enum is_path_valid_result { > + PATH_IS_ERROR = -1, > + PATH_IS_NOT_VALID, > + PATH_IS_VALID, > + PATH_IS_VALID_NO_CHECK, I'd like to see the comment explaining the difference between VALID and VALID_NO_CHECK here, too. > + PATH_IS_MAYBE_VALID, > + PATH_MAX_VALID_RESULT, /* only for bounds checking */ > +}; > + > +int is_path_valid(const char *name, struct config *conf, struct path > *pp, > + bool check_multipathd); > + > +#endif /* _VALID_D */ > diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c > index 28a2150d..637cb0ab 100644 > --- a/libmultipath/wwids.c > +++ b/libmultipath/wwids.c > @@ -289,19 +289,19 @@ out: > int > should_multipath(struct path *pp1, vector pathvec, vector mpvec) > { > - int i, ignore_new_devs, find_multipaths; > + int i, find_multipaths; > struct path *pp2; > struct config *conf; > > conf = get_multipath_config(); > - ignore_new_devs = ignore_new_devs_on(conf); > - find_multipaths = find_multipaths_on(conf); > + find_multipaths = conf->find_multipaths; > put_multipath_config(conf); > - if (!find_multipaths && !ignore_new_devs) > + if (find_multipaths == FIND_MULTIPATHS_OFF || > + find_multipaths == FIND_MULTIPATHS_GREEDY) > return 1; > > condlog(4, "checking if %s should be multipathed", pp1->dev); > - if (!ignore_new_devs) { > + if (find_multipaths != FIND_MULTIPATHS_STRICT) { > char tmp_wwid[WWID_SIZE]; > struct multipath *mp = find_mp_by_wwid(mpvec, pp1- > >wwid); Noting explicitly here: you got the complex logic right. Kudos :-) > > diff --git a/multipath/main.c b/multipath/main.c > index 545ead87..953fab27 100644 > --- a/multipath/main.c > +++ b/multipath/main.c > @@ -63,21 +63,18 @@ > #include "propsel.h" > #include "time-util.h" > #include "file.h" > +#include "valid.h" > > int logsink; > struct udev *udev; > struct config *multipath_conf; > > /* > - * Return values of configure(), print_cmd_valid(), and main(). > - * RTVL_{YES,NO} are synonyms for RTVL_{OK,FAIL} for the > CMD_VALID_PATH case. > + * Return values of configure(), check_path_valid(), and main(). > */ > enum { > RTVL_OK = 0, > - RTVL_YES = RTVL_OK, > RTVL_FAIL = 1, > - RTVL_NO = RTVL_FAIL, > - RTVL_MAYBE, /* only used internally, never returned */ > RTVL_RETRY, /* returned by configure(), not by main() */ > }; > > @@ -269,9 +266,6 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, > vector pathvec, char * refwwid) > continue; > } > > - if (cmd == CMD_VALID_PATH) > - continue; > - > dm_get_map(mpp->alias, &mpp->size, params); > condlog(3, "params = %s", params); > dm_get_status(mpp->alias, status); > @@ -491,10 +485,11 @@ static int print_cmd_valid(int k, const vector > pathvec, > struct timespec until; > struct path *pp; > > - if (k != RTVL_YES && k != RTVL_NO && k != RTVL_MAYBE) > - return RTVL_NO; > + if (k != PATH_IS_VALID && k != PATH_IS_NOT_VALID && > + k != PATH_IS_MAYBE_VALID) > + return PATH_IS_NOT_VALID; > > - if (k == RTVL_MAYBE) { > + if (k == PATH_IS_MAYBE_VALID) { > /* > * Caller ensures that pathvec[0] is the path to > * examine. > @@ -504,7 +499,7 @@ static int print_cmd_valid(int k, const vector > pathvec, > wait = find_multipaths_check_timeout( > pp, pp->find_multipaths_timeout, &until); > if (wait != FIND_MULTIPATHS_WAITING) > - k = RTVL_NO; > + k = PATH_IS_NOT_VALID; > } else if (pathvec != NULL && (pp = VECTOR_SLOT(pathvec, 0))) > wait = find_multipaths_check_timeout(pp, 0, &until); > if (wait == FIND_MULTIPATHS_WAITING) > @@ -513,9 +508,9 @@ static int print_cmd_valid(int k, const vector > pathvec, > else if (wait == FIND_MULTIPATHS_WAIT_DONE) > printf("FIND_MULTIPATHS_WAIT_UNTIL=\"0\"\n"); > printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", > - k == RTVL_MAYBE ? 2 : k == RTVL_YES ? 1 : 0); > + k == PATH_IS_MAYBE_VALID ? 2 : k == PATH_IS_VALID ? 1 : > 0); > /* Never return RTVL_MAYBE */ > - return k == RTVL_NO ? RTVL_NO : RTVL_YES; > + return k == PATH_IS_NOT_VALID ? PATH_IS_NOT_VALID : > PATH_IS_VALID; > } > > /* > @@ -548,7 +543,6 @@ configure (struct config *conf, enum mpath_cmds > cmd, > int di_flag = 0; > char * refwwid = NULL; > char * dev = NULL; > - bool released = released_to_systemd(); > > /* > * allocate core vectors to store paths and multipaths > @@ -573,7 +567,7 @@ configure (struct config *conf, enum mpath_cmds > cmd, > cmd != CMD_REMOVE_WWID && > (filter_devnode(conf->blist_devnode, > conf->elist_devnode, dev) > 0)) { > - goto print_valid; > + goto out; > } > > /* > @@ -581,14 +575,10 @@ configure (struct config *conf, enum mpath_cmds > cmd, > * failing the translation is fatal (by policy) > */ > if (devpath) { > - int failed = get_refwwid(cmd, devpath, dev_type, > - pathvec, &refwwid); > + get_refwwid(cmd, devpath, dev_type, pathvec, &refwwid); > if (!refwwid) { > condlog(4, "%s: failed to get wwid", devpath); > - if (failed == 2 && cmd == CMD_VALID_PATH) > - goto print_valid; > - else > - condlog(3, "scope is null"); > + condlog(3, "scope is null"); > goto out; > } > if (cmd == CMD_REMOVE_WWID) { > @@ -614,53 +604,6 @@ configure (struct config *conf, enum mpath_cmds > cmd, > goto out; > } > condlog(3, "scope limited to %s", refwwid); > - /* If you are ignoring the wwids file and > find_multipaths is > - * set, you need to actually check if there are two > available > - * paths to determine if this path should be > multipathed. To > - * do this, we put off the check until after > discovering all > - * the paths. > - * Paths listed in the wwids file are always considered > valid. > - */ > - if (cmd == CMD_VALID_PATH) { > - if (is_failed_wwid(refwwid) == WWID_IS_FAILED) > { > - r = RTVL_NO; > - goto print_valid; > - } > - if ((!find_multipaths_on(conf) && > - ignore_wwids_on(conf)) || > - check_wwids_file(refwwid, 0) == 0) > - r = RTVL_YES; > - if (!ignore_wwids_on(conf)) > - goto print_valid;PATH_IS_VALID_NO_CHECK > > - /* At this point, either r==0 or > find_multipaths_on. */ > - > - /* > - * Shortcut for find_multipaths smart: > - * Quick check if path is already multipathed. > - */ > - if (sysfs_is_multipathed(VECTOR_SLOT(pathvec, > 0), > - false)) { > - r = RTVL_YES; > - goto print_valid; > - } > - > - /* > - * DM_MULTIPATH_DEVICE_PATH=="0" means that we > have > - * been called for this device already, and > have > - * released it to systemd. Unless the device is > now > - * already multipathed (see above), we can't > try to > - * grab it, because setting SYSTEMD_READY=0 > would > - * cause file systems to be unmounted. > - * Leave DM_MULTIPATH_DEVICE_PATH="0". > - */ > - if (released) { > - r = RTVL_NO; > - goto print_valid; > - } > - if (r == RTVL_YES) > - goto print_valid; > - /* find_multipaths_on: Fall through to path > detection */ > - } > } > > /* > @@ -701,59 +644,6 @@ configure (struct config *conf, enum mpath_cmds > cmd, > goto out; > } > > - if (cmd == CMD_VALID_PATH) { > - struct path *pp; > - int fd; > - > - /* This only happens if find_multipaths and > - * ignore_wwids is set, and the path is not in WWIDs > - * file, not currently multipathed, and has > - * never been released to systemd. > - * If there is currently a multipath device matching > - * the refwwid, or there is more than one path matching > - * the refwwid, then the path is valid */ > - if (VECTOR_SIZE(curmp) != 0) { > - r = RTVL_YES; > - goto print_valid; > - } else if (VECTOR_SIZE(pathvec) > 1) > - r = RTVL_YES; > - else > - r = RTVL_MAYBE; > - > - /* > - * If opening the path with O_EXCL fails, the path > - * is in use (e.g. mounted during initramfs > processing). > - * We know that it's not used by dm-multipath. > - * We may not set SYSTEMD_READY=0 on such devices, it > - * might cause systemd to umount the device. > - * Use O_RDONLY, because udevd would trigger another > - * uevent for close-after-write. > - * > - * The O_EXCL check is potentially dangerous, because > it may > - * race with other tasks trying to access the device. > Therefore > - * this code is only executed if the path hasn't been > released > - * to systemd earlier (see above). > - * > - * get_refwwid() above stores the path we examine in > slot 0. > - */ > - pp = VECTOR_SLOT(pathvec, 0); > - fd = open(udev_device_get_devnode(pp->udev), > - O_RDONLY|O_EXCL); > - if (fd >= 0) > - close(fd); > - else { > - condlog(3, "%s: path %s is in use: %s", > - __func__, pp->dev, > - strerror(errno)); > - /* > - * Check if we raced with multipathd > - */ > - r = sysfs_is_multipathed(VECTOR_SLOT(pathvec, > 0), > - false) ? RTVL_YES : > RTVL_NO; > - } > - goto print_valid; > - } > - > if (cmd != CMD_CREATE && cmd != CMD_DRY_RUN) { > r = RTVL_OK; > goto out; > @@ -766,10 +656,6 @@ configure (struct config *conf, enum mpath_cmds > cmd, > conf->force_reload, cmd); > r = rc == CP_RETRY ? RTVL_RETRY : rc == CP_OK ? RTVL_OK : > RTVL_FAIL; > > -print_valid: > - if (cmd == CMD_VALID_PATH) > - r = print_cmd_valid(r, pathvec, conf); > - > out: > if (refwwid) > FREE(refwwid); > @@ -780,6 +666,112 @@ out: > return r; > } > > +static int > +check_path_valid(const char *name, struct config *conf, bool > is_uevent) > +{ > + int fd, r = PATH_IS_ERROR; > + struct path *pp = NULL; > + vector pathvec = NULL; > + > + pp = alloc_path(); > + if (!pp) > + return RTVL_FAIL; > + > + r = is_path_valid(name, conf, pp, is_uevent); > + if (r <= PATH_IS_ERROR || r >= PATH_MAX_VALID_RESULT) > + goto fail; > + About the following block ... > + /* set path values if is_path_valid() didn't */ > + if (!pp->udev) > + pp->udev = udev_device_new_from_subsystem_sysname(udev, > "block", > + name) > ; > + if (!pp->udev) > + goto fail; > + > + if (!strlen(pp->dev_t)) { > + dev_t devt = udev_device_get_devnum(pp->udev); > + if (major(devt) == 0 && minor(devt) == 0) > + goto fail; > + snprintf(pp->dev_t, BLK_DEV_SIZE, "%d:%d", major(devt), > + minor(devt)); > + } > + > + pathvec = vector_alloc(); > + if (!pathvec) > + goto fail; > + > + if (store_path(pathvec, pp) != 0) { > + free_path(pp); > + goto fail; > + } ... why do you do this here, rather than after the (r != PATH_IS_MAYBE_VALID) clause below? AFAICS you don't need pathvec until you run path_discovery(). > + > + if ((r == PATH_IS_VALID || r == PATH_IS_MAYBE_VALID) && > + released_to_systemd()) > + r = PATH_IS_NOT_VALID; > + > + /* This state is only used to skip the released_to_systemd() > check */ > + if (r == PATH_IS_VALID_NO_CHECK) > + r = PATH_IS_VALID; > + > + if (r != PATH_IS_MAYBE_VALID) > + goto out; > + > + /* > + * If opening the path with O_EXCL fails, the path > + * is in use (e.g. mounted during initramfs processing). > + * We know that it's not used by dm-multipath. > + * We may not set SYSTEMD_READY=0 on such devices, it > + * might cause systemd to umount the device. > + * Use O_RDONLY, because udevd would trigger another > + * uevent for close-after-write. > + * > + * The O_EXCL check is potentially dangerous, because it may > + * race with other tasks trying to access the device. Therefore > + * this code is only executed if the path hasn't been released > + * to systemd earlier (see above). > + */ > + fd = open(udev_device_get_devnode(pp->udev), O_RDONLY|O_EXCL); > + if (fd >= 0) > + close(fd); > + else { > + condlog(3, "%s: path %s is in use: %m", __func__, pp- > >dev); > + /* Check if we raced with multipathd */ > + if (sysfs_is_multipathed(pp, false)) > + r = PATH_IS_VALID; > + else > + r = PATH_IS_NOT_VALID; > + goto out; > + } > + > + /* For find_multipaths = SMART, if there is more than one path > + * matching the refwwid, then the path is valid */ > + if (path_discovery(pathvec, DI_SYSFS | DI_WWID) < 0) > + goto fail; > + filter_pathvec(pathvec, pp->wwid); > + if (VECTOR_SIZE(pathvec) > 1) > + r = PATH_IS_VALID; > + else > + r = PATH_IS_MAYBE_VALID; > + > +out: > + r = print_cmd_valid(r, pathvec, conf); > + free_pathvec(pathvec, FREE_PATHS); > + /* > + * multipath -u must exit with status 0, otherwise udev won't > + * import its output. > + */ > + if (!is_uevent && r == PATH_IS_NOT_VALID) > + return RTVL_FAIL; > + return RTVL_OK; > + > +fail: > + if (pathvec) > + free_pathvec(pathvec, FREE_PATHS); > + else > + free_path(pp); > + return RTVL_FAIL; > +} > + > static int > get_dev_type(char *dev) { > struct stat buf; > @@ -861,32 +853,6 @@ out: > return r; > } > > -static int test_multipathd_socket(void) > -{ > - int fd; > - /* > - * "multipath -u" may be run before the daemon is started. In > this > - * case, systemd might own the socket but might delay > multipathd > - * startup until some other unit (udev settle!) has finished > - * starting. With many LUNs, the listen backlog may be > exceeded, which > - * would cause connect() to block. This causes udev workers > calling > - * "multipath -u" to hang, and thus creates a deadlock, until > "udev > - * settle" times out. To avoid this, call connect() in non- > blocking > - * mode here, and take EAGAIN as indication for a filled-up > systemd > - * backlog. > - */ > - > - fd = __mpath_connect(1); > - if (fd == -1) { > - if (errno == EAGAIN) > - condlog(3, "daemon backlog exceeded"); > - else > - return 0; > - } else > - close(fd); > - return 1; > -} > - > int > main (int argc, char *argv[]) > { > @@ -970,7 +936,11 @@ main (int argc, char *argv[]) > conf->force_reload = FORCE_RELOAD_YES; > break; > case 'i': > - conf->find_multipaths |= _FIND_MULTIPATHS_I; > + if (conf->find_multipaths == FIND_MULTIPATHS_ON > || > + conf->find_multipaths == > FIND_MULTIPATHS_STRICT) > + conf->find_multipaths = > FIND_MULTIPATHS_SMART; > + else if (conf->find_multipaths == > FIND_MULTIPATHS_OFF) > + conf->find_multipaths = > FIND_MULTIPATHS_GREEDY; Ok. Previously FIND_MULTIPATHS_SMART was not the same value as FIND_MULTIPATHS_STRICT + _FIND_MULTIPATHS_I. Effectively, this doesn't change logic, but only because the check for ignore_new_devs_on() in should_multipath() is actually redundant. (IIRC in the past we'd determined that "strict" + "ignore_wwids" makes no sense). > break; > case 't': > r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : > RTVL_OK; > @@ -1064,15 +1034,10 @@ main (int argc, char *argv[]) > condlog(0, "the -c option requires a path to check"); > goto out; > } > - if (cmd == CMD_VALID_PATH && > - dev_type == DEV_UEVENT) { > - if (!test_multipathd_socket()) { > - condlog(3, "%s: daemon is not running", dev); > - if (!systemd_service_enabled(dev)) { > - r = print_cmd_valid(RTVL_NO, NULL, > conf); > - goto out; > - } > - } > + if (cmd == CMD_VALID_PATH) { > + char * name = convert_dev(dev, (dev_type == > DEV_DEVNODE)); > + r = check_path_valid(name, conf, dev_type == > DEV_UEVENT); > + goto out; > } > > if (cmd == CMD_REMOVE_WWID && !dev) { > @@ -1136,13 +1101,6 @@ out: > cleanup_prio(); > cleanup_checkers(); > > - /* > - * multipath -u must exit with status 0, otherwise udev won't > - * import its output. > - */ > - if (cmd == CMD_VALID_PATH && dev_type == DEV_UEVENT && r == > RTVL_NO) > - r = RTVL_OK; > - > if (dev_type == DEV_UEVENT) > closelog(); >
On Fri, May 15, 2020 at 08:37:16PM +0000, Martin Wilck wrote: > On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote: > > This code pulls the multipath path validation code out of > > configure(), > > and puts it into its own function, check_path_valid(). This function > > calls a new libmultipath function, is_path_valid() to check just path > > requested. This seperation exists so that is_path_valid() can be > > reused > > by future code. This code will give almost the same answer as the > > existing code, with the exception that now, if a device is currently > > multipathed, it will always be a valid multipath path. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > Great job getting the logic right! Readability massively improved. > Almost ack, a few comments and questions below. > > Regards, > Martin > > > > --- > > libmultipath/Makefile | 2 +- > > libmultipath/devmapper.c | 49 +++++++ > > libmultipath/devmapper.h | 1 + > > libmultipath/structs.h | 24 +--- > > libmultipath/valid.c | 118 ++++++++++++++++ > > libmultipath/valid.h | 32 +++++ > > libmultipath/wwids.c | 10 +- > > multipath/main.c | 296 +++++++++++++++++------------------ > > ---- > > 8 files changed, 337 insertions(+), 195 deletions(-) > > create mode 100644 libmultipath/valid.c > > create mode 100644 libmultipath/valid.h > > > > diff --git a/libmultipath/Makefile b/libmultipath/Makefile > > index f19b7ad2..e5dac5ea 100644 > > --- a/libmultipath/Makefile > > +++ b/libmultipath/Makefile > > @@ -48,7 +48,7 @@ OBJS = memory.o parser.o vector.o devmapper.o > > callout.o \ > > log.o configure.o structs_vec.o sysfs.o prio.o checkers.o \ > > lock.o file.o wwids.o prioritizers/alua_rtpg.o prkey.o \ > > io_err_stat.o dm-generic.o generic.o foreign.o nvme-lib.o \ > > - libsg.o > > + libsg.o valid.o > > > > all: $(LIBS) > > > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > > index 7ed494a1..92f61133 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_map_present_by_uuid(const char *uuid) > > +{ > > + 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; > > I have nothing against goto for error handling, but here I'd prefer > > r = !!info.exists; Sure. > > +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..5ed7edc5 100644 > > --- a/libmultipath/devmapper.h > > +++ b/libmultipath/devmapper.h > > @@ -39,6 +39,7 @@ int dm_simplecmd_noflush (int, const char *, > > uint16_t); > > int dm_addmap_create (struct multipath *mpp, char *params); > > int dm_addmap_reload (struct multipath *mpp, char *params, int > > flush); > > int dm_map_present (const char *); > > +int dm_map_present_by_uuid(const char *uuid); > > int dm_get_map(const char *, unsigned long long *, char *); > > int dm_get_status(const char *, char *); > > int dm_type(const char *, char *); > > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > > index 9bd39eb1..d69bc2e9 100644 > > --- a/libmultipath/structs.h > > +++ b/libmultipath/structs.h > > @@ -101,29 +101,13 @@ enum yes_no_undef_states { > > YNU_YES, > > }; > > > > -#define _FIND_MULTIPATHS_F (1 << 1) > > -#define _FIND_MULTIPATHS_I (1 << 2) > > -#define _FIND_MULTIPATHS_N (1 << 3) > > -/* > > - * _FIND_MULTIPATHS_F must have the same value as YNU_YES. > > - * Generate a compile time error if that isn't the case. > > - */ > > -extern char ___error1___[-(_FIND_MULTIPATHS_F != YNU_YES)]; > > - > > -#define find_multipaths_on(conf) \ > > - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_F)) > > -#define ignore_wwids_on(conf) \ > > - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_I)) > > -#define ignore_new_devs_on(conf) \ > > - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_N)) > > - > > enum find_multipaths_states { > > FIND_MULTIPATHS_UNDEF = YNU_UNDEF, > > FIND_MULTIPATHS_OFF = YNU_NO, > > - FIND_MULTIPATHS_ON = _FIND_MULTIPATHS_F, > > - FIND_MULTIPATHS_GREEDY = _FIND_MULTIPATHS_I, > > - FIND_MULTIPATHS_SMART = _FIND_MULTIPATHS_F|_FIND_MULTIPATHS_I, > > - FIND_MULTIPATHS_STRICT = _FIND_MULTIPATHS_F|_FIND_MULTIPATHS_N, > > + FIND_MULTIPATHS_ON = YNU_YES, > > + FIND_MULTIPATHS_GREEDY, > > + FIND_MULTIPATHS_SMART, > > + FIND_MULTIPATHS_STRICT, > > __FIND_MULTIPATHS_LAST, > > }; > > > > What was the reason you changed these definitions? AFAICS you've got > the logic right, and I'm not saying it shouldn't be done, but I'd like > to see a rationale. Is it just simplification / readability? > > (Note to self: _FIND_MULTIPATHS_F etc. were introduced to make it > obvious at the time that these flags had the same effect as the > previous "ignore_wwids", "ignore_new_devs", and "find_multipaths" > command line options). > Simply because the _FIND_MULTIPATHS_* defines and their checking fuctions where no longer used. And like you mention, since we've moved a ways from the validation setup that we had before the existing one, there's no point in writing the code to reference that old setup. > > diff --git a/libmultipath/valid.c b/libmultipath/valid.c > > new file mode 100644 > > index 00000000..456b1f6e > > --- /dev/null > > +++ b/libmultipath/valid.c > > @@ -0,0 +1,118 @@ > > +/* > > + Copyright (c) 2020 Benjamin Marzinski, IBM > > + > > + This program is free software; you can redistribute it and/or > > + modify it under the terms of the GNU 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 General Public License for more details. > > + > > + You should have received a copy of the GNU General Public License > > + along with this program. If not, see < > > https://www.gnu.org/licenses/>;. > > + */ > > +#include <stddef.h> > > +#include <errno.h> > > +#include <libudev.h> > > + > > +#include "vector.h" > > +#include "config.h" > > +#include "debug.h" > > +#include "util.h" > > +#include "devmapper.h" > > +#include "discovery.h" > > +#include "wwids.h" > > +#include "sysfs.h" > > +#include "blacklist.h" > > +#include "mpath_cmd.h" > > +#include "valid.h" > > + > > +int > > +is_path_valid(const char *name, struct config *conf, struct path > > *pp, > > + bool check_multipathd) > > +{ > > + int r; > > + int fd; > > + > > + if (!pp || !name || !conf) > > + return PATH_IS_ERROR; > > + > > + if (conf->find_multipaths <= FIND_MULTIPATHS_UNDEF || > > + conf->find_multipaths >= __FIND_MULTIPATHS_LAST) > > + return PATH_IS_ERROR; > > + > > + if (safe_sprintf(pp->dev, "%s", name)) > > + return PATH_IS_ERROR; > > + > > + if (sysfs_is_multipathed(pp, true)) { > > + if (pp->wwid[0] == '\0') > > + return PATH_IS_ERROR; > > + return PATH_IS_VALID_NO_CHECK; > > + } > > + > > + /* > > + * "multipath -u" may be run before the daemon is started. In > > this > > + * case, systemd might own the socket but might delay > > multipathd > > + * startup until some other unit (udev settle!) has finished > > + * starting. With many LUNs, the listen backlog may be > > exceeded, which > > + * would cause connect() to block. This causes udev workers > > calling > > + * "multipath -u" to hang, and thus creates a deadlock, until > > "udev > > + * settle" times out. To avoid this, call connect() in non- > > blocking > > + * mode here, and take EAGAIN as indication for a filled-up > > systemd > > + * backlog. > > + */ > > + > > + if (check_multipathd) { > > + fd = __mpath_connect(1); > > + if (fd < 0) { > > + if (errno != EAGAIN && > > !systemd_service_enabled(name)) { > > + condlog(3, "multipathd not running or > > enabled"); > > + return PATH_IS_NOT_VALID; > > + } > > + } else > > + mpath_disconnect(fd); > > + } > > + > > + pp->udev = udev_device_new_from_subsystem_sysname(udev, > > "block", name); > > + if (!pp->udev) > > + return PATH_IS_ERROR; > > + > > + r = pathinfo(pp, conf, DI_SYSFS | DI_WWID | DI_BLACKLIST); > > + if (r == PATHINFO_SKIPPED) > > + return PATH_IS_NOT_VALID; > > + else if (r) > > + return PATH_IS_ERROR; > > + > > + if (pp->wwid[0] == '\0') > > + return PATH_IS_NOT_VALID; > > + > > + if (pp->udev && pp->uid_attribute && > > + filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0) > > + return PATH_IS_NOT_VALID; > > + > > + r = is_failed_wwid(pp->wwid); > > + if (r != WWID_IS_NOT_FAILED) { > > + if (r == WWID_IS_FAILED) > > + return PATH_IS_NOT_VALID; > > + return PATH_IS_ERROR; > > + } > > + > > + if (conf->find_multipaths == FIND_MULTIPATHS_GREEDY) > > + return PATH_IS_VALID; > > + > > + if (check_wwids_file(pp->wwid, 0) == 0) > > + return PATH_IS_VALID_NO_CHECK; > > + > > + if (dm_map_present_by_uuid(pp->wwid) == 1) > > + return PATH_IS_VALID; > > + > > + /* all these act like FIND_MULTIPATHS_STRICT for finding if a > > + * path is valid */ > > + if (conf->find_multipaths != FIND_MULTIPATHS_SMART) > > + return PATH_IS_NOT_VALID; > > + > > + return PATH_IS_MAYBE_VALID; > > +} > > diff --git a/libmultipath/valid.h b/libmultipath/valid.h > > new file mode 100644 > > index 00000000..778658ad > > --- /dev/null > > +++ b/libmultipath/valid.h > > @@ -0,0 +1,32 @@ > > +/* > > + Copyright (c) 2020 Benjamin Marzinski, IBM > > + > > + This program is free software; you can redistribute it and/or > > + modify it under the terms of the GNU 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 General Public License for more details. > > + > > + You should have received a copy of the GNU General Public License > > + along with this program. If not, see < > > https://www.gnu.org/licenses/>;. > > + */ > > +#ifndef _VALID_H > > +#define _VALID_H > > + > > +enum is_path_valid_result { > > + PATH_IS_ERROR = -1, > > + PATH_IS_NOT_VALID, > > + PATH_IS_VALID, > > + PATH_IS_VALID_NO_CHECK, > > I'd like to see the comment explaining the difference between VALID > and VALID_NO_CHECK here, too. Sure. > > + PATH_IS_MAYBE_VALID, > > + PATH_MAX_VALID_RESULT, /* only for bounds checking */ > > +}; > > + > > +int is_path_valid(const char *name, struct config *conf, struct path > > *pp, > > + bool check_multipathd); > > + > > +#endif /* _VALID_D */ > > diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c > > index 28a2150d..637cb0ab 100644 > > --- a/libmultipath/wwids.c > > +++ b/libmultipath/wwids.c > > @@ -289,19 +289,19 @@ out: > > int > > should_multipath(struct path *pp1, vector pathvec, vector mpvec) > > { > > - int i, ignore_new_devs, find_multipaths; > > + int i, find_multipaths; > > struct path *pp2; > > struct config *conf; > > > > conf = get_multipath_config(); > > - ignore_new_devs = ignore_new_devs_on(conf); > > - find_multipaths = find_multipaths_on(conf); > > + find_multipaths = conf->find_multipaths; > > put_multipath_config(conf); > > - if (!find_multipaths && !ignore_new_devs) > > + if (find_multipaths == FIND_MULTIPATHS_OFF || > > + find_multipaths == FIND_MULTIPATHS_GREEDY) > > return 1; > > > > condlog(4, "checking if %s should be multipathed", pp1->dev); > > - if (!ignore_new_devs) { > > + if (find_multipaths != FIND_MULTIPATHS_STRICT) { > > char tmp_wwid[WWID_SIZE]; > > struct multipath *mp = find_mp_by_wwid(mpvec, pp1- > > >wwid); > > Noting explicitly here: you got the complex logic right. Kudos :-) > > > > > diff --git a/multipath/main.c b/multipath/main.c > > index 545ead87..953fab27 100644 > > --- a/multipath/main.c > > +++ b/multipath/main.c > > @@ -63,21 +63,18 @@ > > #include "propsel.h" > > #include "time-util.h" > > #include "file.h" > > +#include "valid.h" > > > > int logsink; > > struct udev *udev; > > struct config *multipath_conf; > > > > /* > > - * Return values of configure(), print_cmd_valid(), and main(). > > - * RTVL_{YES,NO} are synonyms for RTVL_{OK,FAIL} for the > > CMD_VALID_PATH case. > > + * Return values of configure(), check_path_valid(), and main(). > > */ > > enum { > > RTVL_OK = 0, > > - RTVL_YES = RTVL_OK, > > RTVL_FAIL = 1, > > - RTVL_NO = RTVL_FAIL, > > - RTVL_MAYBE, /* only used internally, never returned */ > > RTVL_RETRY, /* returned by configure(), not by main() */ > > }; > > > > @@ -269,9 +266,6 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, > > vector pathvec, char * refwwid) > > continue; > > } > > > > - if (cmd == CMD_VALID_PATH) > > - continue; > > - > > dm_get_map(mpp->alias, &mpp->size, params); > > condlog(3, "params = %s", params); > > dm_get_status(mpp->alias, status); > > @@ -491,10 +485,11 @@ static int print_cmd_valid(int k, const vector > > pathvec, > > struct timespec until; > > struct path *pp; > > > > - if (k != RTVL_YES && k != RTVL_NO && k != RTVL_MAYBE) > > - return RTVL_NO; > > + if (k != PATH_IS_VALID && k != PATH_IS_NOT_VALID && > > + k != PATH_IS_MAYBE_VALID) > > + return PATH_IS_NOT_VALID; > > > > - if (k == RTVL_MAYBE) { > > + if (k == PATH_IS_MAYBE_VALID) { > > /* > > * Caller ensures that pathvec[0] is the path to > > * examine. > > @@ -504,7 +499,7 @@ static int print_cmd_valid(int k, const vector > > pathvec, > > wait = find_multipaths_check_timeout( > > pp, pp->find_multipaths_timeout, &until); > > if (wait != FIND_MULTIPATHS_WAITING) > > - k = RTVL_NO; > > + k = PATH_IS_NOT_VALID; > > } else if (pathvec != NULL && (pp = VECTOR_SLOT(pathvec, 0))) > > wait = find_multipaths_check_timeout(pp, 0, &until); > > if (wait == FIND_MULTIPATHS_WAITING) > > @@ -513,9 +508,9 @@ static int print_cmd_valid(int k, const vector > > pathvec, > > else if (wait == FIND_MULTIPATHS_WAIT_DONE) > > printf("FIND_MULTIPATHS_WAIT_UNTIL=\"0\"\n"); > > printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", > > - k == RTVL_MAYBE ? 2 : k == RTVL_YES ? 1 : 0); > > + k == PATH_IS_MAYBE_VALID ? 2 : k == PATH_IS_VALID ? 1 : > > 0); > > /* Never return RTVL_MAYBE */ > > - return k == RTVL_NO ? RTVL_NO : RTVL_YES; > > + return k == PATH_IS_NOT_VALID ? PATH_IS_NOT_VALID : > > PATH_IS_VALID; > > } > > > > /* > > @@ -548,7 +543,6 @@ configure (struct config *conf, enum mpath_cmds > > cmd, > > int di_flag = 0; > > char * refwwid = NULL; > > char * dev = NULL; > > - bool released = released_to_systemd(); > > > > /* > > * allocate core vectors to store paths and multipaths > > @@ -573,7 +567,7 @@ configure (struct config *conf, enum mpath_cmds > > cmd, > > cmd != CMD_REMOVE_WWID && > > (filter_devnode(conf->blist_devnode, > > conf->elist_devnode, dev) > 0)) { > > - goto print_valid; > > + goto out; > > } > > > > /* > > @@ -581,14 +575,10 @@ configure (struct config *conf, enum mpath_cmds > > cmd, > > * failing the translation is fatal (by policy) > > */ > > if (devpath) { > > - int failed = get_refwwid(cmd, devpath, dev_type, > > - pathvec, &refwwid); > > + get_refwwid(cmd, devpath, dev_type, pathvec, &refwwid); > > if (!refwwid) { > > condlog(4, "%s: failed to get wwid", devpath); > > - if (failed == 2 && cmd == CMD_VALID_PATH) > > - goto print_valid; > > - else > > - condlog(3, "scope is null"); > > + condlog(3, "scope is null"); > > goto out; > > } > > if (cmd == CMD_REMOVE_WWID) { > > @@ -614,53 +604,6 @@ configure (struct config *conf, enum mpath_cmds > > cmd, > > goto out; > > } > > condlog(3, "scope limited to %s", refwwid); > > - /* If you are ignoring the wwids file and > > find_multipaths is > > - * set, you need to actually check if there are two > > available > > - * paths to determine if this path should be > > multipathed. To > > - * do this, we put off the check until after > > discovering all > > - * the paths. > > - * Paths listed in the wwids file are always considered > > valid. > > - */ > > - if (cmd == CMD_VALID_PATH) { > > - if (is_failed_wwid(refwwid) == WWID_IS_FAILED) > > { > > - r = RTVL_NO; > > - goto print_valid; > > - } > > - if ((!find_multipaths_on(conf) && > > - ignore_wwids_on(conf)) || > > - check_wwids_file(refwwid, 0) == 0) > > - r = RTVL_YES; > > - if (!ignore_wwids_on(conf)) > > - goto print_valid;PATH_IS_VALID_NO_CHECK > > > > - /* At this point, either r==0 or > > find_multipaths_on. */ > > - > > - /* > > - * Shortcut for find_multipaths smart: > > - * Quick check if path is already multipathed. > > - */ > > - if (sysfs_is_multipathed(VECTOR_SLOT(pathvec, > > 0), > > - false)) { > > - r = RTVL_YES; > > - goto print_valid; > > - } > > - > > - /* > > - * DM_MULTIPATH_DEVICE_PATH=="0" means that we > > have > > - * been called for this device already, and > > have > > - * released it to systemd. Unless the device is > > now > > - * already multipathed (see above), we can't > > try to > > - * grab it, because setting SYSTEMD_READY=0 > > would > > - * cause file systems to be unmounted. > > - * Leave DM_MULTIPATH_DEVICE_PATH="0". > > - */ > > - if (released) { > > - r = RTVL_NO; > > - goto print_valid; > > - } > > - if (r == RTVL_YES) > > - goto print_valid; > > - /* find_multipaths_on: Fall through to path > > detection */ > > - } > > } > > > > /* > > @@ -701,59 +644,6 @@ configure (struct config *conf, enum mpath_cmds > > cmd, > > goto out; > > } > > > > - if (cmd == CMD_VALID_PATH) { > > - struct path *pp; > > - int fd; > > - > > - /* This only happens if find_multipaths and > > - * ignore_wwids is set, and the path is not in WWIDs > > - * file, not currently multipathed, and has > > - * never been released to systemd. > > - * If there is currently a multipath device matching > > - * the refwwid, or there is more than one path matching > > - * the refwwid, then the path is valid */ > > - if (VECTOR_SIZE(curmp) != 0) { > > - r = RTVL_YES; > > - goto print_valid; > > - } else if (VECTOR_SIZE(pathvec) > 1) > > - r = RTVL_YES; > > - else > > - r = RTVL_MAYBE; > > - > > - /* > > - * If opening the path with O_EXCL fails, the path > > - * is in use (e.g. mounted during initramfs > > processing). > > - * We know that it's not used by dm-multipath. > > - * We may not set SYSTEMD_READY=0 on such devices, it > > - * might cause systemd to umount the device. > > - * Use O_RDONLY, because udevd would trigger another > > - * uevent for close-after-write. > > - * > > - * The O_EXCL check is potentially dangerous, because > > it may > > - * race with other tasks trying to access the device. > > Therefore > > - * this code is only executed if the path hasn't been > > released > > - * to systemd earlier (see above). > > - * > > - * get_refwwid() above stores the path we examine in > > slot 0. > > - */ > > - pp = VECTOR_SLOT(pathvec, 0); > > - fd = open(udev_device_get_devnode(pp->udev), > > - O_RDONLY|O_EXCL); > > - if (fd >= 0) > > - close(fd); > > - else { > > - condlog(3, "%s: path %s is in use: %s", > > - __func__, pp->dev, > > - strerror(errno)); > > - /* > > - * Check if we raced with multipathd > > - */ > > - r = sysfs_is_multipathed(VECTOR_SLOT(pathvec, > > 0), > > - false) ? RTVL_YES : > > RTVL_NO; > > - } > > - goto print_valid; > > - } > > - > > if (cmd != CMD_CREATE && cmd != CMD_DRY_RUN) { > > r = RTVL_OK; > > goto out; > > @@ -766,10 +656,6 @@ configure (struct config *conf, enum mpath_cmds > > cmd, > > conf->force_reload, cmd); > > r = rc == CP_RETRY ? RTVL_RETRY : rc == CP_OK ? RTVL_OK : > > RTVL_FAIL; > > > > -print_valid: > > - if (cmd == CMD_VALID_PATH) > > - r = print_cmd_valid(r, pathvec, conf); > > - > > out: > > if (refwwid) > > FREE(refwwid); > > @@ -780,6 +666,112 @@ out: > > return r; > > } > > > > +static int > > +check_path_valid(const char *name, struct config *conf, bool > > is_uevent) > > +{ > > + int fd, r = PATH_IS_ERROR; > > + struct path *pp = NULL; > > + vector pathvec = NULL; > > + > > + pp = alloc_path(); > > + if (!pp) > > + return RTVL_FAIL; > > + > > + r = is_path_valid(name, conf, pp, is_uevent); > > + if (r <= PATH_IS_ERROR || r >= PATH_MAX_VALID_RESULT) > > + goto fail; > > + > > About the following block ... > > > + /* set path values if is_path_valid() didn't */ > > + if (!pp->udev) > > + pp->udev = udev_device_new_from_subsystem_sysname(udev, > > "block", > > + name) > > ; > > + if (!pp->udev) > > + goto fail; > > + > > + if (!strlen(pp->dev_t)) { > > + dev_t devt = udev_device_get_devnum(pp->udev); > > + if (major(devt) == 0 && minor(devt) == 0) > > + goto fail; > > + snprintf(pp->dev_t, BLK_DEV_SIZE, "%d:%d", major(devt), > > + minor(devt)); > > + } > > + > > + pathvec = vector_alloc(); > > + if (!pathvec) > > + goto fail; > > + > > + if (store_path(pathvec, pp) != 0) { > > + free_path(pp); > > + goto fail; > > + } > > ... why do you do this here, rather than after the (r != > PATH_IS_MAYBE_VALID) clause below? AFAICS you don't need pathvec until > you run path_discovery(). > print_cmd_valid() expects a pathvec, and I didn't bother to change that. You are completely right that I could change that to just take a path, and not need to set up the pathvec early. If you it's important, I can change that for the next verison. > > + > > + if ((r == PATH_IS_VALID || r == PATH_IS_MAYBE_VALID) && > > + released_to_systemd()) > > + r = PATH_IS_NOT_VALID; > > + > > + /* This state is only used to skip the released_to_systemd() > > check */ > > + if (r == PATH_IS_VALID_NO_CHECK) > > + r = PATH_IS_VALID; > > + > > + if (r != PATH_IS_MAYBE_VALID) > > + goto out; > > + > > + /* > > + * If opening the path with O_EXCL fails, the path > > + * is in use (e.g. mounted during initramfs processing). > > + * We know that it's not used by dm-multipath. > > + * We may not set SYSTEMD_READY=0 on such devices, it > > + * might cause systemd to umount the device. > > + * Use O_RDONLY, because udevd would trigger another > > + * uevent for close-after-write. > > + * > > + * The O_EXCL check is potentially dangerous, because it may > > + * race with other tasks trying to access the device. Therefore > > + * this code is only executed if the path hasn't been released > > + * to systemd earlier (see above). > > + */ > > + fd = open(udev_device_get_devnode(pp->udev), O_RDONLY|O_EXCL); > > + if (fd >= 0) > > + close(fd); > > + else { > > + condlog(3, "%s: path %s is in use: %m", __func__, pp- > > >dev); > > + /* Check if we raced with multipathd */ > > + if (sysfs_is_multipathed(pp, false)) > > + r = PATH_IS_VALID; > > + else > > + r = PATH_IS_NOT_VALID; > > + goto out; > > + } > > + > > + /* For find_multipaths = SMART, if there is more than one path > > + * matching the refwwid, then the path is valid */ > > + if (path_discovery(pathvec, DI_SYSFS | DI_WWID) < 0) > > + goto fail; > > + filter_pathvec(pathvec, pp->wwid); > > + if (VECTOR_SIZE(pathvec) > 1) > > + r = PATH_IS_VALID; > > + else > > + r = PATH_IS_MAYBE_VALID; > > + > > +out: > > + r = print_cmd_valid(r, pathvec, conf); > > + free_pathvec(pathvec, FREE_PATHS); > > + /* > > + * multipath -u must exit with status 0, otherwise udev won't > > + * import its output. > > + */ > > + if (!is_uevent && r == PATH_IS_NOT_VALID) > > + return RTVL_FAIL; > > + return RTVL_OK; > > + > > +fail: > > + if (pathvec) > > + free_pathvec(pathvec, FREE_PATHS); > > + else > > + free_path(pp); > > + return RTVL_FAIL; > > +} > > + > > static int > > get_dev_type(char *dev) { > > struct stat buf; > > @@ -861,32 +853,6 @@ out: > > return r; > > } > > > > -static int test_multipathd_socket(void) > > -{ > > - int fd; > > - /* > > - * "multipath -u" may be run before the daemon is started. In > > this > > - * case, systemd might own the socket but might delay > > multipathd > > - * startup until some other unit (udev settle!) has finished > > - * starting. With many LUNs, the listen backlog may be > > exceeded, which > > - * would cause connect() to block. This causes udev workers > > calling > > - * "multipath -u" to hang, and thus creates a deadlock, until > > "udev > > - * settle" times out. To avoid this, call connect() in non- > > blocking > > - * mode here, and take EAGAIN as indication for a filled-up > > systemd > > - * backlog. > > - */ > > - > > - fd = __mpath_connect(1); > > - if (fd == -1) { > > - if (errno == EAGAIN) > > - condlog(3, "daemon backlog exceeded"); > > - else > > - return 0; > > - } else > > - close(fd); > > - return 1; > > -} > > - > > int > > main (int argc, char *argv[]) > > { > > @@ -970,7 +936,11 @@ main (int argc, char *argv[]) > > conf->force_reload = FORCE_RELOAD_YES; > > break; > > case 'i': > > - conf->find_multipaths |= _FIND_MULTIPATHS_I; > > + if (conf->find_multipaths == FIND_MULTIPATHS_ON > > || > > + conf->find_multipaths == > > FIND_MULTIPATHS_STRICT) > > + conf->find_multipaths = > > FIND_MULTIPATHS_SMART; > > + else if (conf->find_multipaths == > > FIND_MULTIPATHS_OFF) > > + conf->find_multipaths = > > FIND_MULTIPATHS_GREEDY; > > Ok. Previously FIND_MULTIPATHS_SMART was not the same value as > FIND_MULTIPATHS_STRICT + _FIND_MULTIPATHS_I. Effectively, this doesn't > change logic, but only because the check for ignore_new_devs_on() in > should_multipath() is actually redundant. (IIRC in the past we'd > determined that "strict" + "ignore_wwids" makes no sense). > And I still feel like it doesn't make any sense, so this was intentional. Are you arguing that we need that state, or are you just pointing this out? > > break; > > case 't': > > r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : > > RTVL_OK; > > @@ -1064,15 +1034,10 @@ main (int argc, char *argv[]) > > condlog(0, "the -c option requires a path to check"); > > goto out; > > } > > - if (cmd == CMD_VALID_PATH && > > - dev_type == DEV_UEVENT) { > > - if (!test_multipathd_socket()) { > > - condlog(3, "%s: daemon is not running", dev); > > - if (!systemd_service_enabled(dev)) { > > - r = print_cmd_valid(RTVL_NO, NULL, > > conf); > > - goto out; > > - } > > - } > > + if (cmd == CMD_VALID_PATH) { > > + char * name = convert_dev(dev, (dev_type == > > DEV_DEVNODE)); > > + r = check_path_valid(name, conf, dev_type == > > DEV_UEVENT); > > + goto out; > > } > > > > if (cmd == CMD_REMOVE_WWID && !dev) { > > @@ -1136,13 +1101,6 @@ out: > > cleanup_prio(); > > cleanup_checkers(); > > > > - /* > > - * multipath -u must exit with status 0, otherwise udev won't > > - * import its output. > > - */ > > - if (cmd == CMD_VALID_PATH && dev_type == DEV_UEVENT && r == > > RTVL_NO) > > - r = RTVL_OK; > > - > > if (dev_type == DEV_UEVENT) > > closelog(); > > > > -- > Dr. Martin Wilck <mwilck@suse.com>, 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@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 2020-05-18 at 13:53 -0500, Benjamin Marzinski wrote: > On Fri, May 15, 2020 at 08:37:16PM +0000, Martin Wilck wrote: > > On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote: > > > This code pulls the multipath path validation code out of > > > configure(), > > > and puts it into its own function, check_path_valid(). This > > > function > > > calls a new libmultipath function, is_path_valid() to check just > > > path > > > requested. This seperation exists so that is_path_valid() can be > > > reused > > > by future code. This code will give almost the same answer as the > > > existing code, with the exception that now, if a device is > > > currently > > > multipathed, it will always be a valid multipath path. > > > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > > Great job getting the logic right! Readability massively improved. > > Almost ack, a few comments and questions below. > > > > Regards, > > Martin > > > > > > > - conf->find_multipaths |= _FIND_MULTIPATHS_I; > > > + if (conf->find_multipaths == FIND_MULTIPATHS_ON > > > + conf->find_multipaths == > > > FIND_MULTIPATHS_STRICT) > > > + conf->find_multipaths = > > > FIND_MULTIPATHS_SMART; > > > + else if (conf->find_multipaths == > > > FIND_MULTIPATHS_OFF) > > > + conf->find_multipaths = > > > FIND_MULTIPATHS_GREEDY; > > > > Ok. Previously FIND_MULTIPATHS_SMART was not the same value as > > FIND_MULTIPATHS_STRICT + _FIND_MULTIPATHS_I. Effectively, this > > doesn't > > change logic, but only because the check for ignore_new_devs_on() > > in > > should_multipath() is actually redundant. (IIRC in the past we'd > > determined that "strict" + "ignore_wwids" makes no sense). > > > > And I still feel like it doesn't make any sense, so this was > intentional. Are you arguing that we need that state, or are you > just > pointing this out? Just pointing it out. That's why I wrote "Ok" in the first place. But I stumbled on it at first sight, and took a while to realize that your patch got it right. Which is worthwhile to note IMO, in order not to stumble on it next time. Sorry for not expressing that clearly enough. Martin
diff --git a/libmultipath/Makefile b/libmultipath/Makefile index f19b7ad2..e5dac5ea 100644 --- a/libmultipath/Makefile +++ b/libmultipath/Makefile @@ -48,7 +48,7 @@ OBJS = memory.o parser.o vector.o devmapper.o callout.o \ log.o configure.o structs_vec.o sysfs.o prio.o checkers.o \ lock.o file.o wwids.o prioritizers/alua_rtpg.o prkey.o \ io_err_stat.o dm-generic.o generic.o foreign.o nvme-lib.o \ - libsg.o + libsg.o valid.o all: $(LIBS) diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c index 7ed494a1..92f61133 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_map_present_by_uuid(const char *uuid) +{ + 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..5ed7edc5 100644 --- a/libmultipath/devmapper.h +++ b/libmultipath/devmapper.h @@ -39,6 +39,7 @@ int dm_simplecmd_noflush (int, const char *, uint16_t); int dm_addmap_create (struct multipath *mpp, char *params); int dm_addmap_reload (struct multipath *mpp, char *params, int flush); int dm_map_present (const char *); +int dm_map_present_by_uuid(const char *uuid); int dm_get_map(const char *, unsigned long long *, char *); int dm_get_status(const char *, char *); int dm_type(const char *, char *); diff --git a/libmultipath/structs.h b/libmultipath/structs.h index 9bd39eb1..d69bc2e9 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -101,29 +101,13 @@ enum yes_no_undef_states { YNU_YES, }; -#define _FIND_MULTIPATHS_F (1 << 1) -#define _FIND_MULTIPATHS_I (1 << 2) -#define _FIND_MULTIPATHS_N (1 << 3) -/* - * _FIND_MULTIPATHS_F must have the same value as YNU_YES. - * Generate a compile time error if that isn't the case. - */ -extern char ___error1___[-(_FIND_MULTIPATHS_F != YNU_YES)]; - -#define find_multipaths_on(conf) \ - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_F)) -#define ignore_wwids_on(conf) \ - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_I)) -#define ignore_new_devs_on(conf) \ - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_N)) - enum find_multipaths_states { FIND_MULTIPATHS_UNDEF = YNU_UNDEF, FIND_MULTIPATHS_OFF = YNU_NO, - FIND_MULTIPATHS_ON = _FIND_MULTIPATHS_F, - FIND_MULTIPATHS_GREEDY = _FIND_MULTIPATHS_I, - FIND_MULTIPATHS_SMART = _FIND_MULTIPATHS_F|_FIND_MULTIPATHS_I, - FIND_MULTIPATHS_STRICT = _FIND_MULTIPATHS_F|_FIND_MULTIPATHS_N, + FIND_MULTIPATHS_ON = YNU_YES, + FIND_MULTIPATHS_GREEDY, + FIND_MULTIPATHS_SMART, + FIND_MULTIPATHS_STRICT, __FIND_MULTIPATHS_LAST, }; diff --git a/libmultipath/valid.c b/libmultipath/valid.c new file mode 100644 index 00000000..456b1f6e --- /dev/null +++ b/libmultipath/valid.c @@ -0,0 +1,118 @@ +/* + Copyright (c) 2020 Benjamin Marzinski, IBM + + This program is free software; you can redistribute it and/or + modify it under the terms of the GNU 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 General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <https://www.gnu.org/licenses/>. + */ +#include <stddef.h> +#include <errno.h> +#include <libudev.h> + +#include "vector.h" +#include "config.h" +#include "debug.h" +#include "util.h" +#include "devmapper.h" +#include "discovery.h" +#include "wwids.h" +#include "sysfs.h" +#include "blacklist.h" +#include "mpath_cmd.h" +#include "valid.h" + +int +is_path_valid(const char *name, struct config *conf, struct path *pp, + bool check_multipathd) +{ + int r; + int fd; + + if (!pp || !name || !conf) + return PATH_IS_ERROR; + + if (conf->find_multipaths <= FIND_MULTIPATHS_UNDEF || + conf->find_multipaths >= __FIND_MULTIPATHS_LAST) + return PATH_IS_ERROR; + + if (safe_sprintf(pp->dev, "%s", name)) + return PATH_IS_ERROR; + + if (sysfs_is_multipathed(pp, true)) { + if (pp->wwid[0] == '\0') + return PATH_IS_ERROR; + return PATH_IS_VALID_NO_CHECK; + } + + /* + * "multipath -u" may be run before the daemon is started. In this + * case, systemd might own the socket but might delay multipathd + * startup until some other unit (udev settle!) has finished + * starting. With many LUNs, the listen backlog may be exceeded, which + * would cause connect() to block. This causes udev workers calling + * "multipath -u" to hang, and thus creates a deadlock, until "udev + * settle" times out. To avoid this, call connect() in non-blocking + * mode here, and take EAGAIN as indication for a filled-up systemd + * backlog. + */ + + if (check_multipathd) { + fd = __mpath_connect(1); + if (fd < 0) { + if (errno != EAGAIN && !systemd_service_enabled(name)) { + condlog(3, "multipathd not running or enabled"); + return PATH_IS_NOT_VALID; + } + } else + mpath_disconnect(fd); + } + + pp->udev = udev_device_new_from_subsystem_sysname(udev, "block", name); + if (!pp->udev) + return PATH_IS_ERROR; + + r = pathinfo(pp, conf, DI_SYSFS | DI_WWID | DI_BLACKLIST); + if (r == PATHINFO_SKIPPED) + return PATH_IS_NOT_VALID; + else if (r) + return PATH_IS_ERROR; + + if (pp->wwid[0] == '\0') + return PATH_IS_NOT_VALID; + + if (pp->udev && pp->uid_attribute && + filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0) + return PATH_IS_NOT_VALID; + + r = is_failed_wwid(pp->wwid); + if (r != WWID_IS_NOT_FAILED) { + if (r == WWID_IS_FAILED) + return PATH_IS_NOT_VALID; + return PATH_IS_ERROR; + } + + if (conf->find_multipaths == FIND_MULTIPATHS_GREEDY) + return PATH_IS_VALID; + + if (check_wwids_file(pp->wwid, 0) == 0) + return PATH_IS_VALID_NO_CHECK; + + if (dm_map_present_by_uuid(pp->wwid) == 1) + return PATH_IS_VALID; + + /* all these act like FIND_MULTIPATHS_STRICT for finding if a + * path is valid */ + if (conf->find_multipaths != FIND_MULTIPATHS_SMART) + return PATH_IS_NOT_VALID; + + return PATH_IS_MAYBE_VALID; +} diff --git a/libmultipath/valid.h b/libmultipath/valid.h new file mode 100644 index 00000000..778658ad --- /dev/null +++ b/libmultipath/valid.h @@ -0,0 +1,32 @@ +/* + Copyright (c) 2020 Benjamin Marzinski, IBM + + This program is free software; you can redistribute it and/or + modify it under the terms of the GNU 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 General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <https://www.gnu.org/licenses/>. + */ +#ifndef _VALID_H +#define _VALID_H + +enum is_path_valid_result { + PATH_IS_ERROR = -1, + PATH_IS_NOT_VALID, + PATH_IS_VALID, + PATH_IS_VALID_NO_CHECK, + PATH_IS_MAYBE_VALID, + PATH_MAX_VALID_RESULT, /* only for bounds checking */ +}; + +int is_path_valid(const char *name, struct config *conf, struct path *pp, + bool check_multipathd); + +#endif /* _VALID_D */ diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c index 28a2150d..637cb0ab 100644 --- a/libmultipath/wwids.c +++ b/libmultipath/wwids.c @@ -289,19 +289,19 @@ out: int should_multipath(struct path *pp1, vector pathvec, vector mpvec) { - int i, ignore_new_devs, find_multipaths; + int i, find_multipaths; struct path *pp2; struct config *conf; conf = get_multipath_config(); - ignore_new_devs = ignore_new_devs_on(conf); - find_multipaths = find_multipaths_on(conf); + find_multipaths = conf->find_multipaths; put_multipath_config(conf); - if (!find_multipaths && !ignore_new_devs) + if (find_multipaths == FIND_MULTIPATHS_OFF || + find_multipaths == FIND_MULTIPATHS_GREEDY) return 1; condlog(4, "checking if %s should be multipathed", pp1->dev); - if (!ignore_new_devs) { + if (find_multipaths != FIND_MULTIPATHS_STRICT) { char tmp_wwid[WWID_SIZE]; struct multipath *mp = find_mp_by_wwid(mpvec, pp1->wwid); diff --git a/multipath/main.c b/multipath/main.c index 545ead87..953fab27 100644 --- a/multipath/main.c +++ b/multipath/main.c @@ -63,21 +63,18 @@ #include "propsel.h" #include "time-util.h" #include "file.h" +#include "valid.h" int logsink; struct udev *udev; struct config *multipath_conf; /* - * Return values of configure(), print_cmd_valid(), and main(). - * RTVL_{YES,NO} are synonyms for RTVL_{OK,FAIL} for the CMD_VALID_PATH case. + * Return values of configure(), check_path_valid(), and main(). */ enum { RTVL_OK = 0, - RTVL_YES = RTVL_OK, RTVL_FAIL = 1, - RTVL_NO = RTVL_FAIL, - RTVL_MAYBE, /* only used internally, never returned */ RTVL_RETRY, /* returned by configure(), not by main() */ }; @@ -269,9 +266,6 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid) continue; } - if (cmd == CMD_VALID_PATH) - continue; - dm_get_map(mpp->alias, &mpp->size, params); condlog(3, "params = %s", params); dm_get_status(mpp->alias, status); @@ -491,10 +485,11 @@ static int print_cmd_valid(int k, const vector pathvec, struct timespec until; struct path *pp; - if (k != RTVL_YES && k != RTVL_NO && k != RTVL_MAYBE) - return RTVL_NO; + if (k != PATH_IS_VALID && k != PATH_IS_NOT_VALID && + k != PATH_IS_MAYBE_VALID) + return PATH_IS_NOT_VALID; - if (k == RTVL_MAYBE) { + if (k == PATH_IS_MAYBE_VALID) { /* * Caller ensures that pathvec[0] is the path to * examine. @@ -504,7 +499,7 @@ static int print_cmd_valid(int k, const vector pathvec, wait = find_multipaths_check_timeout( pp, pp->find_multipaths_timeout, &until); if (wait != FIND_MULTIPATHS_WAITING) - k = RTVL_NO; + k = PATH_IS_NOT_VALID; } else if (pathvec != NULL && (pp = VECTOR_SLOT(pathvec, 0))) wait = find_multipaths_check_timeout(pp, 0, &until); if (wait == FIND_MULTIPATHS_WAITING) @@ -513,9 +508,9 @@ static int print_cmd_valid(int k, const vector pathvec, else if (wait == FIND_MULTIPATHS_WAIT_DONE) printf("FIND_MULTIPATHS_WAIT_UNTIL=\"0\"\n"); printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", - k == RTVL_MAYBE ? 2 : k == RTVL_YES ? 1 : 0); + k == PATH_IS_MAYBE_VALID ? 2 : k == PATH_IS_VALID ? 1 : 0); /* Never return RTVL_MAYBE */ - return k == RTVL_NO ? RTVL_NO : RTVL_YES; + return k == PATH_IS_NOT_VALID ? PATH_IS_NOT_VALID : PATH_IS_VALID; } /* @@ -548,7 +543,6 @@ configure (struct config *conf, enum mpath_cmds cmd, int di_flag = 0; char * refwwid = NULL; char * dev = NULL; - bool released = released_to_systemd(); /* * allocate core vectors to store paths and multipaths @@ -573,7 +567,7 @@ configure (struct config *conf, enum mpath_cmds cmd, cmd != CMD_REMOVE_WWID && (filter_devnode(conf->blist_devnode, conf->elist_devnode, dev) > 0)) { - goto print_valid; + goto out; } /* @@ -581,14 +575,10 @@ configure (struct config *conf, enum mpath_cmds cmd, * failing the translation is fatal (by policy) */ if (devpath) { - int failed = get_refwwid(cmd, devpath, dev_type, - pathvec, &refwwid); + get_refwwid(cmd, devpath, dev_type, pathvec, &refwwid); if (!refwwid) { condlog(4, "%s: failed to get wwid", devpath); - if (failed == 2 && cmd == CMD_VALID_PATH) - goto print_valid; - else - condlog(3, "scope is null"); + condlog(3, "scope is null"); goto out; } if (cmd == CMD_REMOVE_WWID) { @@ -614,53 +604,6 @@ configure (struct config *conf, enum mpath_cmds cmd, goto out; } condlog(3, "scope limited to %s", refwwid); - /* If you are ignoring the wwids file and find_multipaths is - * set, you need to actually check if there are two available - * paths to determine if this path should be multipathed. To - * do this, we put off the check until after discovering all - * the paths. - * Paths listed in the wwids file are always considered valid. - */ - if (cmd == CMD_VALID_PATH) { - if (is_failed_wwid(refwwid) == WWID_IS_FAILED) { - r = RTVL_NO; - goto print_valid; - } - if ((!find_multipaths_on(conf) && - ignore_wwids_on(conf)) || - check_wwids_file(refwwid, 0) == 0) - r = RTVL_YES; - if (!ignore_wwids_on(conf)) - goto print_valid; - /* At this point, either r==0 or find_multipaths_on. */ - - /* - * Shortcut for find_multipaths smart: - * Quick check if path is already multipathed. - */ - if (sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0), - false)) { - r = RTVL_YES; - goto print_valid; - } - - /* - * DM_MULTIPATH_DEVICE_PATH=="0" means that we have - * been called for this device already, and have - * released it to systemd. Unless the device is now - * already multipathed (see above), we can't try to - * grab it, because setting SYSTEMD_READY=0 would - * cause file systems to be unmounted. - * Leave DM_MULTIPATH_DEVICE_PATH="0". - */ - if (released) { - r = RTVL_NO; - goto print_valid; - } - if (r == RTVL_YES) - goto print_valid; - /* find_multipaths_on: Fall through to path detection */ - } } /* @@ -701,59 +644,6 @@ configure (struct config *conf, enum mpath_cmds cmd, goto out; } - if (cmd == CMD_VALID_PATH) { - struct path *pp; - int fd; - - /* This only happens if find_multipaths and - * ignore_wwids is set, and the path is not in WWIDs - * file, not currently multipathed, and has - * never been released to systemd. - * If there is currently a multipath device matching - * the refwwid, or there is more than one path matching - * the refwwid, then the path is valid */ - if (VECTOR_SIZE(curmp) != 0) { - r = RTVL_YES; - goto print_valid; - } else if (VECTOR_SIZE(pathvec) > 1) - r = RTVL_YES; - else - r = RTVL_MAYBE; - - /* - * If opening the path with O_EXCL fails, the path - * is in use (e.g. mounted during initramfs processing). - * We know that it's not used by dm-multipath. - * We may not set SYSTEMD_READY=0 on such devices, it - * might cause systemd to umount the device. - * Use O_RDONLY, because udevd would trigger another - * uevent for close-after-write. - * - * The O_EXCL check is potentially dangerous, because it may - * race with other tasks trying to access the device. Therefore - * this code is only executed if the path hasn't been released - * to systemd earlier (see above). - * - * get_refwwid() above stores the path we examine in slot 0. - */ - pp = VECTOR_SLOT(pathvec, 0); - fd = open(udev_device_get_devnode(pp->udev), - O_RDONLY|O_EXCL); - if (fd >= 0) - close(fd); - else { - condlog(3, "%s: path %s is in use: %s", - __func__, pp->dev, - strerror(errno)); - /* - * Check if we raced with multipathd - */ - r = sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0), - false) ? RTVL_YES : RTVL_NO; - } - goto print_valid; - } - if (cmd != CMD_CREATE && cmd != CMD_DRY_RUN) { r = RTVL_OK; goto out; @@ -766,10 +656,6 @@ configure (struct config *conf, enum mpath_cmds cmd, conf->force_reload, cmd); r = rc == CP_RETRY ? RTVL_RETRY : rc == CP_OK ? RTVL_OK : RTVL_FAIL; -print_valid: - if (cmd == CMD_VALID_PATH) - r = print_cmd_valid(r, pathvec, conf); - out: if (refwwid) FREE(refwwid); @@ -780,6 +666,112 @@ out: return r; } +static int +check_path_valid(const char *name, struct config *conf, bool is_uevent) +{ + int fd, r = PATH_IS_ERROR; + struct path *pp = NULL; + vector pathvec = NULL; + + pp = alloc_path(); + if (!pp) + return RTVL_FAIL; + + r = is_path_valid(name, conf, pp, is_uevent); + if (r <= PATH_IS_ERROR || r >= PATH_MAX_VALID_RESULT) + goto fail; + + /* set path values if is_path_valid() didn't */ + if (!pp->udev) + pp->udev = udev_device_new_from_subsystem_sysname(udev, "block", + name); + if (!pp->udev) + goto fail; + + if (!strlen(pp->dev_t)) { + dev_t devt = udev_device_get_devnum(pp->udev); + if (major(devt) == 0 && minor(devt) == 0) + goto fail; + snprintf(pp->dev_t, BLK_DEV_SIZE, "%d:%d", major(devt), + minor(devt)); + } + + pathvec = vector_alloc(); + if (!pathvec) + goto fail; + + if (store_path(pathvec, pp) != 0) { + free_path(pp); + goto fail; + } + + if ((r == PATH_IS_VALID || r == PATH_IS_MAYBE_VALID) && + released_to_systemd()) + r = PATH_IS_NOT_VALID; + + /* This state is only used to skip the released_to_systemd() check */ + if (r == PATH_IS_VALID_NO_CHECK) + r = PATH_IS_VALID; + + if (r != PATH_IS_MAYBE_VALID) + goto out; + + /* + * If opening the path with O_EXCL fails, the path + * is in use (e.g. mounted during initramfs processing). + * We know that it's not used by dm-multipath. + * We may not set SYSTEMD_READY=0 on such devices, it + * might cause systemd to umount the device. + * Use O_RDONLY, because udevd would trigger another + * uevent for close-after-write. + * + * The O_EXCL check is potentially dangerous, because it may + * race with other tasks trying to access the device. Therefore + * this code is only executed if the path hasn't been released + * to systemd earlier (see above). + */ + fd = open(udev_device_get_devnode(pp->udev), O_RDONLY|O_EXCL); + if (fd >= 0) + close(fd); + else { + condlog(3, "%s: path %s is in use: %m", __func__, pp->dev); + /* Check if we raced with multipathd */ + if (sysfs_is_multipathed(pp, false)) + r = PATH_IS_VALID; + else + r = PATH_IS_NOT_VALID; + goto out; + } + + /* For find_multipaths = SMART, if there is more than one path + * matching the refwwid, then the path is valid */ + if (path_discovery(pathvec, DI_SYSFS | DI_WWID) < 0) + goto fail; + filter_pathvec(pathvec, pp->wwid); + if (VECTOR_SIZE(pathvec) > 1) + r = PATH_IS_VALID; + else + r = PATH_IS_MAYBE_VALID; + +out: + r = print_cmd_valid(r, pathvec, conf); + free_pathvec(pathvec, FREE_PATHS); + /* + * multipath -u must exit with status 0, otherwise udev won't + * import its output. + */ + if (!is_uevent && r == PATH_IS_NOT_VALID) + return RTVL_FAIL; + return RTVL_OK; + +fail: + if (pathvec) + free_pathvec(pathvec, FREE_PATHS); + else + free_path(pp); + return RTVL_FAIL; +} + static int get_dev_type(char *dev) { struct stat buf; @@ -861,32 +853,6 @@ out: return r; } -static int test_multipathd_socket(void) -{ - int fd; - /* - * "multipath -u" may be run before the daemon is started. In this - * case, systemd might own the socket but might delay multipathd - * startup until some other unit (udev settle!) has finished - * starting. With many LUNs, the listen backlog may be exceeded, which - * would cause connect() to block. This causes udev workers calling - * "multipath -u" to hang, and thus creates a deadlock, until "udev - * settle" times out. To avoid this, call connect() in non-blocking - * mode here, and take EAGAIN as indication for a filled-up systemd - * backlog. - */ - - fd = __mpath_connect(1); - if (fd == -1) { - if (errno == EAGAIN) - condlog(3, "daemon backlog exceeded"); - else - return 0; - } else - close(fd); - return 1; -} - int main (int argc, char *argv[]) { @@ -970,7 +936,11 @@ main (int argc, char *argv[]) conf->force_reload = FORCE_RELOAD_YES; break; case 'i': - conf->find_multipaths |= _FIND_MULTIPATHS_I; + if (conf->find_multipaths == FIND_MULTIPATHS_ON || + conf->find_multipaths == FIND_MULTIPATHS_STRICT) + conf->find_multipaths = FIND_MULTIPATHS_SMART; + else if (conf->find_multipaths == FIND_MULTIPATHS_OFF) + conf->find_multipaths = FIND_MULTIPATHS_GREEDY; break; case 't': r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : RTVL_OK; @@ -1064,15 +1034,10 @@ main (int argc, char *argv[]) condlog(0, "the -c option requires a path to check"); goto out; } - if (cmd == CMD_VALID_PATH && - dev_type == DEV_UEVENT) { - if (!test_multipathd_socket()) { - condlog(3, "%s: daemon is not running", dev); - if (!systemd_service_enabled(dev)) { - r = print_cmd_valid(RTVL_NO, NULL, conf); - goto out; - } - } + if (cmd == CMD_VALID_PATH) { + char * name = convert_dev(dev, (dev_type == DEV_DEVNODE)); + r = check_path_valid(name, conf, dev_type == DEV_UEVENT); + goto out; } if (cmd == CMD_REMOVE_WWID && !dev) { @@ -1136,13 +1101,6 @@ out: cleanup_prio(); cleanup_checkers(); - /* - * multipath -u must exit with status 0, otherwise udev won't - * import its output. - */ - if (cmd == CMD_VALID_PATH && dev_type == DEV_UEVENT && r == RTVL_NO) - r = RTVL_OK; - if (dev_type == DEV_UEVENT) closelog();
This code pulls the multipath path validation code out of configure(), and puts it into its own function, check_path_valid(). This function calls a new libmultipath function, is_path_valid() to check just path requested. This seperation exists so that is_path_valid() can be reused by future code. This code will give almost the same answer as the existing code, with the exception that now, if a device is currently multipathed, it will always be a valid multipath path. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/Makefile | 2 +- libmultipath/devmapper.c | 49 +++++++ libmultipath/devmapper.h | 1 + libmultipath/structs.h | 24 +--- libmultipath/valid.c | 118 ++++++++++++++++ libmultipath/valid.h | 32 +++++ libmultipath/wwids.c | 10 +- multipath/main.c | 296 +++++++++++++++++---------------------- 8 files changed, 337 insertions(+), 195 deletions(-) create mode 100644 libmultipath/valid.c create mode 100644 libmultipath/valid.h