[dm-devel] [PATCH v5 03/22] libmultipath: should_multipath: keep existing maps

[Martin Wilck]

with find_multipaths "yes" and without the "-n" option to multipathd,
if a path is already multipathed, keep it. The same logic is applied by
"multipath -u -i".

To do this, we need to add a "mpvec" parameter to should_multipath().

Reviewed-by: Benjamin Marzinski 
Signed-off-by: Martin Wilck 
---
 libmultipath/configure.c |  2 +-
 libmultipath/wwids.c | 12 +++-
 libmultipath/wwids.h |  2 +-
 multipathd/main.c|  2 +-
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 51a45d4f..1cd575e4 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -987,7 +987,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, 
char * refwwid,
continue;
 
/* If find_multipaths was selected check if the path is valid */
-   if (!refwwid && !should_multipath(pp1, pathvec)) {
+   if (!refwwid && !should_multipath(pp1, pathvec, curmp)) {
orphan_path(pp1, "only one path");
continue;
}
diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index 8c21b33f..d0256c2c 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -15,6 +15,7 @@
 #include "wwids.h"
 #include "defaults.h"
 #include "config.h"
+#include "devmapper.h"
 
 /*
  * Copyright (c) 2010 Benjamin Marzinski, Redhat
@@ -274,7 +275,7 @@ out:
 }
 
 int
-should_multipath(struct path *pp1, vector pathvec)
+should_multipath(struct path *pp1, vector pathvec, vector mpvec)
 {
int i, ignore_new_devs, find_multipaths;
struct path *pp2;
@@ -289,6 +290,15 @@ should_multipath(struct path *pp1, vector pathvec)
 
condlog(4, "checking if %s should be multipathed", pp1->dev);
if (!ignore_new_devs) {
+   char tmp_wwid[WWID_SIZE];
+   struct multipath *mp = find_mp_by_wwid(mpvec, pp1->wwid);
+
+   if (mp != NULL && dm_get_uuid(mp->alias, tmp_wwid) == 0 &&
+   !strncmp(tmp_wwid, pp1->wwid, WWID_SIZE)) {
+   condlog(3, "wwid %s is already multipathed, keeping it",
+   pp1->wwid);
+   return 1;
+   }
vector_foreach_slot(pathvec, pp2, i) {
if (pp1->dev == pp2->dev)
continue;
diff --git a/libmultipath/wwids.h b/libmultipath/wwids.h
index 95270129..d9a78b38 100644
--- a/libmultipath/wwids.h
+++ b/libmultipath/wwids.h
@@ -12,7 +12,7 @@
 "#\n" \
 "# Valid WWIDs:\n"
 
-int should_multipath(struct path *pp, vector pathvec);
+int should_multipath(struct path *pp, vector pathvec, vector mpvec);
 int remember_wwid(char *wwid);
 int check_wwids_file(char *wwid, int write_wwid);
 int remove_wwid(char *wwid);
diff --git a/multipathd/main.c b/multipathd/main.c
index 17175a85..20b6fe02 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -940,7 +940,7 @@ rescan:
mpp->action = ACT_RELOAD;
extract_hwe_from_path(mpp);
} else {
-   if (!should_multipath(pp, vecs->pathvec)) {
+   if (!should_multipath(pp, vecs->pathvec, vecs->mpvec)) {
orphan_path(pp, "only one path");
return 0;
}
-- 
2.16.1

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

[dm-devel] [PATCH v5 06/22] libmultipath: trigger path uevent only when necessary

[Martin Wilck]

Paths that are already classified as DM_MULTIPATH_DEVICE_PATH don't
need to be retriggered.

Reviewed-by: Benjamin Marzinski 
Signed-off-by: Martin Wilck 
---
 libmultipath/configure.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index c56e972f..2cae9240 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -458,8 +458,20 @@ trigger_paths_udev_change(const struct multipath *mpp)
if (!pgp->paths)
continue;
vector_foreach_slot(pgp->paths, pp, j) {
+   const char *env;
+
if (!pp->udev)
continue;
+   /*
+* Paths that are already classified as multipath
+* members don't need another uevent.
+*/
+   env = udev_device_get_property_value(
+   pp->udev, "DM_MULTIPATH_DEVICE_PATH");
+   if (env != NULL && !strcmp(env, "1"))
+   continue;
+
+   condlog(4, "triggering change uevent for %s", pp->dev);
sysfs_attr_set_value(pp->udev, "uevent", "change",
 strlen("change"));
}
-- 
2.16.1

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

[dm-devel] [PATCH v5 13/22] multipath -u: treat failed wwids as invalid

[Martin Wilck]

If a WWID has been marked as "failed", don't treat it as "valid multipath
device path" in multipath -c/-u. This is key to achieve consistency between
multipathd and udev rule processing.

Signed-off-by: Martin Wilck 
Reviewed-by: Benjamin Marzinski 
---
 multipath/main.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 59a72ed0..942caf4c 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -450,8 +450,12 @@ configure (struct config *conf, enum mpath_cmds cmd,
 * Paths listed in the wwids file are always considered valid.
 */
if (cmd == CMD_VALID_PATH) {
-   if ((!find_multipaths_on(conf) && ignore_wwids_on(conf))
-   || check_wwids_file(refwwid, 0) == 0)
+   if (is_failed_wwid(refwwid) == WWID_IS_FAILED) {
+   r = 1;
+   goto print_valid;
+   } else if ((!find_multipaths_on(conf) &&
+   ignore_wwids_on(conf)) ||
+  check_wwids_file(refwwid, 0) == 0)
r = 0;
if (r == 0 ||
!find_multipaths_on(conf) ||
-- 
2.16.1

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

[dm-devel] [PATCH v5 19/22] multipath -u: don't grab devices already passed to system

[Martin Wilck]

Setting SYSTEMD_READY=0 on a device that has previously been passed to
systemd is dangerous - already mounted file systems might be unmounted by
systemd.

Avoid that by checking for previously set DM_MULTIPATH_DEVICE_PATH
environment variable.

This requires to change the exit status of multipath -u - it needs to exit
with status 0 even if the path is not a multipath device path, otherwise
udev doesn't import the printed key-value pairs. We do this only for
"multipath -u"; legacy "multipath -c", which is more likely to be run in user
scripts, still exits with 1 for non-multipath devices.

The condition ENV{DM_MULTIPATH_DEVICE_PATH}!="1" before the "multipath -u"
statement in multipath.rules needs to be removed. This condition was
pointless anyway, because until this patch, DM_MULTIPATH_DEVICE_PATH hadn't
been imported from the db and thus was never set, and "multipath -u" was
always invoked. We want to keep this behavior.

Signed-off-by: Martin Wilck 
---
 multipath/main.c  | 46 +-
 multipath/multipath.rules |  5 -
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 96e37a7a..573d94f9 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -494,6 +495,25 @@ static int print_cmd_valid(int k, const vector pathvec,
return k == 1;
 }
 
+/*
+ * Returns true if this device has been handled before,
+ * and released to systemd.
+ *
+ * This must be called before get_refwwid(),
+ * otherwise udev_device_new_from_environment() will have
+ * destroyed environ(7).
+ */
+static bool released_to_systemd(void)
+{
+   static const char dmdp[] = "DM_MULTIPATH_DEVICE_PATH";
+   const char *dm_mp_dev_path = getenv(dmdp);
+   bool ret;
+
+   ret = dm_mp_dev_path != NULL && !strcmp(dm_mp_dev_path, "0");
+   condlog(4, "%s: %s=%s -> %d", __func__, dmdp, dm_mp_dev_path, ret);
+   return ret;
+}
+
 /*
  * Return value:
  *  -1: Retry
@@ -511,6 +531,7 @@ 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
@@ -602,6 +623,20 @@ configure (struct config *conf, enum mpath_cmds cmd,
r = 0;
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 = 1;
+   goto print_valid;
+   }
if (r == 0)
goto print_valid;
/* find_multipaths_on: Fall through to path detection */
@@ -641,7 +676,9 @@ configure (struct config *conf, enum mpath_cmds cmd,
 
if (cmd == CMD_VALID_PATH) {
/* This only happens if find_multipaths and
-* ignore_wwids is set.
+* 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 */
@@ -1064,6 +1101,13 @@ 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 == 1)
+   r = 0;
+
if (dev_type == DEV_UEVENT)
closelog();
 
diff --git a/multipath/multipath.rules b/multipath/multipath.rules
index 37f4f6d8..ef857271 100644
--- a/multipath/multipath.rules
+++ b/multipath/multipath.rules
@@ -21,8 +21,11 @@ LABEL="test_dev"
 ENV{MPATH_SBIN_PATH}="/sbin"
 TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
 
+# multipath -u needs to know if this device has ever been exported
+IMPORT{db}="DM_MULTIPATH_DEVICE_PATH"
+
 # multipath -u sets DM_MULTIPATH_DEVICE_PATH
-ENV{DM_MULTIPATH_DEVICE_PATH}!="1", 
IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"

[dm-devel] [PATCH v5 07/22] libmultipath: change find_multipaths option to multi-value

[Martin Wilck]

Change the "find_multipaths" option from yes/no to multi-value. This
option now covers the effects of "find_multipaths" as it used to be,
plus the -i option to multipath (ignore_wwids) and the -n option to
multipathd (ignore_new_devs), excluding such combinations of the former
options that are dangerous or inconsistent.

The possible values for "find_multipaths" are now:

 - strict: strictly stick to the WWIDs file; never add new maps automatically
   (new default; upstream behaviour with with find_multipaths "yes" and
   commit 64e27ec "multipathd: imply -n if find_multipaths is set")
 - off|no: multipath like "strict", multipathd like "greedy" (previous
   upstream default)
 - on|yes: set up multipath if >1 paths are seen (current Red Hat/Ubuntu
   behavior with find_multipaths "yes")
 - greedy: set up multipath for all non-blacklisted devices (current SUSE
   default)
 - smart: Like "yes", but try to avoid inconsistencies between udev processing
   and multipathd processing by waiting for path siblings to
   appear (fully implemented in follow-up patches)

The default has been changed to "strict", because "no" may cause inconsistent
behavior between "multipath -u" and multipathd. This is deliberately a
conservative choice.

The patch also updates the related man pages.

Reviewed-by: Benjamin Marzinski 
Signed-off-by: Martin Wilck 
---
 libmultipath/config.h  |  2 --
 libmultipath/defaults.h|  2 +-
 libmultipath/dict.c| 47 --
 libmultipath/structs.h | 26 +
 libmultipath/wwids.c   |  4 ++--
 multipath/main.c   |  9 +
 multipath/multipath.8  |  9 -
 multipath/multipath.conf.5 | 46 +
 multipathd/main.c  |  6 +-
 multipathd/multipathd.8|  8 +---
 10 files changed, 119 insertions(+), 40 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index 329f3ed3..c71d972b 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -139,7 +139,6 @@ struct config {
int max_fds;
int force_reload;
int queue_without_daemon;
-   int ignore_wwids;
int checker_timeout;
int flush_on_last_del;
int attribute_flags;
@@ -168,7 +167,6 @@ struct config {
int strict_timing;
int retrigger_tries;
int retrigger_delay;
-   int ignore_new_devs;
int delayed_reconfig;
int uev_wait_timeout;
int skip_kpartx;
diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 2b270ca4..690182c3 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -17,7 +17,7 @@
 #define DEFAULT_NO_PATH_RETRY  NO_PATH_RETRY_UNDEF
 #define DEFAULT_VERBOSITY  2
 #define DEFAULT_REASSIGN_MAPS  0
-#define DEFAULT_FIND_MULTIPATHS0
+#define DEFAULT_FIND_MULTIPATHSFIND_MULTIPATHS_STRICT
 #define DEFAULT_FAST_IO_FAIL   5
 #define DEFAULT_DEV_LOSS_TMO   600
 #define DEFAULT_RETAIN_HWHANDLER RETAIN_HWHANDLER_ON
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 1a18337b..b03197b6 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -240,8 +240,51 @@ declare_def_snprint(multipath_dir, print_str)
 declare_def_handler(partition_delim, set_str)
 declare_def_snprint(partition_delim, print_str)
 
-declare_def_handler(find_multipaths, set_yes_no)
-declare_def_snprint(find_multipaths, print_yes_no)
+static const char * const find_multipaths_optvals[] = {
+   [FIND_MULTIPATHS_OFF] = "off",
+   [FIND_MULTIPATHS_ON] = "on",
+   [FIND_MULTIPATHS_STRICT] = "strict",
+   [FIND_MULTIPATHS_GREEDY] = "greedy",
+};
+
+static int
+def_find_multipaths_handler(struct config *conf, vector strvec)
+{
+   char *buff;
+   int i;
+
+   if (set_yes_no_undef(strvec, >find_multipaths) == 0 &&
+   conf->find_multipaths != YNU_UNDEF)
+   return 0;
+
+   buff = set_value(strvec);
+   if (!buff)
+   return 1;
+
+   for (i = FIND_MULTIPATHS_OFF; i < __FIND_MULTIPATHS_LAST; i++) {
+   if (find_multipaths_optvals[i] != NULL &&
+   !strcmp(buff, find_multipaths_optvals[i])) {
+   conf->find_multipaths = i;
+   break;
+   }
+   }
+
+   if (conf->find_multipaths == YNU_UNDEF) {
+   condlog(0, "illegal value for find_multipaths: %s", buff);
+   conf->find_multipaths = DEFAULT_FIND_MULTIPATHS;
+   }
+
+   FREE(buff);
+   return 0;
+}
+
+static int
+snprint_def_find_multipaths(struct config *conf, char *buff, int len,
+   const void *data)
+{
+   return print_str(buff, len,
+find_multipaths_optvals[conf->find_multipaths]);
+}
 
 declare_def_handler(selector, set_str)
 declare_def_snprint_defstr(selector, print_str, DEFAULT_SELECTOR)
diff --git 

[dm-devel] [PATCH v5 08/22] libmultipath: use const char* in open_file()

[Martin Wilck]

Reviewed-by: Benjamin Marzinski 
Signed-off-by: Martin Wilck 
---
 libmultipath/file.c | 6 +++---
 libmultipath/file.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/libmultipath/file.c b/libmultipath/file.c
index e4951c92..d5165ec6 100644
--- a/libmultipath/file.c
+++ b/libmultipath/file.c
@@ -37,7 +37,7 @@
  */
 
 static int
-ensure_directories_exist(char *str, mode_t dir_mode)
+ensure_directories_exist(const char *str, mode_t dir_mode)
 {
char *pathname;
char *end;
@@ -80,7 +80,7 @@ sigalrm(int sig)
 }
 
 static int
-lock_file(int fd, char *file_name)
+lock_file(int fd, const char *file_name)
 {
struct sigaction act, oldact;
sigset_t set, oldset;
@@ -118,7 +118,7 @@ lock_file(int fd, char *file_name)
 }
 
 int
-open_file(char *file, int *can_write, char *header)
+open_file(const char *file, int *can_write, const char *header)
 {
int fd;
struct stat s;
diff --git a/libmultipath/file.h b/libmultipath/file.h
index 4f96dbf5..70bffa53 100644
--- a/libmultipath/file.h
+++ b/libmultipath/file.h
@@ -6,6 +6,6 @@
 #define _FILE_H
 
 #define FILE_TIMEOUT 30
-int open_file(char *file, int *can_write, char *header);
+int open_file(const char *file, int *can_write, const char *header);
 
 #endif /* _FILE_H */
-- 
2.16.1

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

[dm-devel] [PATCH v5 17/22] multipath -u: cleanup logic

[Martin Wilck]

The CMD_VALID_PATH logic is complex and hard to read and understand.
This patch cleans it up a bit (preparing for folluw-up patches).
It doesn't change any logic.

Signed-off-by: Martin Wilck 
---
 multipath/main.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 5d847f08..61ba90a6 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -585,15 +585,17 @@ configure (struct config *conf, enum mpath_cmds cmd,
if (is_failed_wwid(refwwid) == WWID_IS_FAILED) {
r = 1;
goto print_valid;
-   } else if ((!find_multipaths_on(conf) &&
+   }
+   if ((!find_multipaths_on(conf) &&
ignore_wwids_on(conf)) ||
   check_wwids_file(refwwid, 0) == 0)
r = 0;
-   if (r == 0 ||
-   !find_multipaths_on(conf) ||
-   !ignore_wwids_on(conf)) {
+   if (!ignore_wwids_on(conf))
goto print_valid;
-   }
+   /* At this point, either r==0 or find_multipaths_on. */
+   if (r == 0)
+   goto print_valid;
+   /* find_multipaths_on: Fall through to path detection */
}
}
 
-- 
2.16.1

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

[dm-devel] [PATCH v5 18/22] multipath -u: quick check if path is multipathed

[Martin Wilck]

With "find_multipaths smart", we accept paths as valid if they are
already part of a multipath map. This patch avoids doing a full path
and device-mapper map scan for this case, speeding up "multipath -u"
considerably.

Signed-off-by: Martin Wilck 
Reviewed-by: Benjamin Marzinski 
---
 libmultipath/sysfs.c | 66 
 libmultipath/sysfs.h |  2 ++
 multipath/main.c |  9 +++
 3 files changed, 77 insertions(+)

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 97e09977..ee72e6a3 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "checkers.h"
 #include "vector.h"
@@ -287,3 +288,68 @@ int sysfs_check_holders(char * check_devt, char * new_devt)
 
return 0;
 }
+
+static int select_dm_devs(const struct dirent *di)
+{
+   return fnmatch("dm-*", di->d_name, FNM_FILE_NAME) == 0;
+}
+
+static void close_fd(void *arg)
+{
+   close((long)arg);
+}
+
+bool sysfs_is_multipathed(const struct path *pp)
+{
+   char pathbuf[PATH_MAX];
+   struct dirent **di;
+   int n, r, i;
+   bool found = false;
+
+   n = snprintf(pathbuf, sizeof(pathbuf), "/sys/block/%s/holders",
+pp->dev);
+
+   if (n >= sizeof(pathbuf)) {
+   condlog(1, "%s: pathname overflow", __func__);
+   return false;
+   }
+
+   r = scandir(pathbuf, , select_dm_devs, alphasort);
+   if (r == 0)
+   return false;
+   else if (r < 0) {
+   condlog(1, "%s: error scanning %s", __func__, pathbuf);
+   return false;
+   }
+
+   pthread_cleanup_push(free, di);
+   for (i = 0; i < r && !found; i++) {
+   long fd;
+   int nr;
+   char uuid[6];
+
+   if (snprintf(pathbuf + n, sizeof(pathbuf) - n,
+"/%s/dm/uuid", di[i]->d_name)
+   >= sizeof(pathbuf) - n)
+   continue;
+
+   fd = open(pathbuf, O_RDONLY);
+   if (fd == -1) {
+   condlog(1, "%s: error opening %s", __func__, pathbuf);
+   continue;
+   }
+
+   pthread_cleanup_push(close_fd, (void *)fd);
+   nr = read(fd, uuid, sizeof(uuid));
+   if (nr == sizeof(uuid) && !memcmp(uuid, "mpath-", sizeof(uuid)))
+   found = true;
+   else if (nr < 0) {
+   condlog(1, "%s: error reading from %s: %s",
+   __func__, pathbuf, strerror(errno));
+   }
+   pthread_cleanup_pop(1);
+   }
+   pthread_cleanup_pop(1);
+
+   return found;
+}
diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
index 75c0f9c1..9ae30b39 100644
--- a/libmultipath/sysfs.h
+++ b/libmultipath/sysfs.h
@@ -4,6 +4,7 @@
 
 #ifndef _LIBMULTIPATH_SYSFS_H
 #define _LIBMULTIPATH_SYSFS_H
+#include 
 
 ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
 const char * value, size_t value_len);
@@ -13,4 +14,5 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, 
const char *attr_name,
 unsigned char * value, size_t value_len);
 int sysfs_get_size (struct path *pp, unsigned long long * size);
 int sysfs_check_holders(char * check_devt, char * new_devt);
+bool sysfs_is_multipathed(const struct path *pp);
 #endif
diff --git a/multipath/main.c b/multipath/main.c
index 61ba90a6..96e37a7a 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -593,6 +593,15 @@ configure (struct config *conf, enum mpath_cmds cmd,
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))) {
+   r = 0;
+   goto print_valid;
+   }
if (r == 0)
goto print_valid;
/* find_multipaths_on: Fall through to path detection */
-- 
2.16.1

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

Re: [dm-devel] [PATCH ALT] multipath-tools: add licence info to README

[Xose Vazquez Perez]

On 04/13/2018 06:18 PM, Bart Van Assche wrote:

> On Fri, 2018-04-13 at 18:17 +0200, Xose Vazquez Perez wrote:
>> +Licence
> 
> Isn't the preferred spelling "License"?

https://en.oxforddictionaries.com/definition/licence

licence
(US license)

Usage:
Note that in British English licence is the correct spelling for the
noun, and is also an acceptable variant spelling of the verb.
In US English both noun and verb are spelled license.

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

[dm-devel] [PATCH ALT] multipath-tools: add licence info to README

[Xose Vazquez Perez]

Alternative patch to [PATCH v2 2/2] multipath-tools: link LICENSES/LGPL-2.0 to 
LICENSE.default

Cc: Hannes Reinecke 
Cc: Benjamin Marzinski 
Cc: Martin Wilck 
Cc: Bart Van Assche 
Cc: Gris Ge 
Cc: Christophe Varoqui 
Cc: DM ML 
Signed-off-by: Xose Vazquez Perez 
---
 README | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/README b/README
index 89bab74..2fc4a81 100644
--- a/README
+++ b/README
@@ -51,3 +51,10 @@ Maintainer
 ==
 Christophe Varoqui 
 Device-mapper development mailing list 
+
+Licence
+===
+The multipath-tools source code is covered by several different
+licences. Refer to the individual source files for details.
+Source files which do not specify a licence are shipped under
+LGPL-2.0 (see LICENSES/LGPL-2.0).
-- 
2.17.0

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

Re: [dm-devel] [PATCH v4 19/20] libmultipath: enable find_multipaths "smart"

[Martin Wilck]

On Thu, 2018-04-12 at 13:47 -0500, Benjamin Marzinski wrote:
> On Wed, Apr 04, 2018 at 06:16:26PM +0200, Martin Wilck wrote:
> > This activates "smart" path detection. This is similar to
> > "find_multipaths yes", but doesn't generally ignore single paths
> > that are not listed in the WWIDs file. Rather, such paths are
> > temporarily treated like multipath members. If no additional paths
> > are detected after a certain time, the paths are re-added to the
> > system as non-multipath. This needs support by the udev rules, to
> > be added in a follow-up patch.
> > 
> > If a multipath map is successfully successfully created, and paths
> > are
> > in waiting state, trigger path uevents to update their status.
> > 
> > Signed-off-by: Martin Wilck 
> > ---
> >  libmultipath/configure.c   | 15 ---
> >  libmultipath/dict.c|  1 +
> >  multipath/multipath.conf.5 | 13 +
> >  3 files changed, 26 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index 9aa3d21..17c2fa1 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -12,6 +12,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include "mpath_cmd.h"
> > @@ -476,9 +477,17 @@ trigger_paths_udev_change(struct multipath
> > *mpp, bool is_mpath)
> > env = udev_device_get_property_value(
> > pp->udev,
> > "DM_MULTIPATH_DEVICE_PATH");
> >  
> > -   if (is_mpath && env != NULL &&
> > !strcmp(env, "1"))
> > -   continue;
> > -   else if (!is_mpath &&
> 
> If DM_MULTIPATH_DEVICE_PATH=1 then there has already been a uevent
> where
> udev recognized that the device should be claimed.  Wouldn't udev
> have
> stopped the timer then?  I don't see why this is necessary.

I set DM_MULTIPATH_DEVICE_PATH=1 in the "pretend_mpath" section
in multipath.rules in the "maybe" case. The value
DM_MULTIPATH_DEVICE_PATH=2 is never passed on to other udev rules,
and never seen by multipathd.

The reason I did that was that rules may have checks like
'DM_MULTIPATH_DEVICE_PATH!="1", ...' and I wanted to avoid these to be
run in the "maybe" case, which, for all actors above multipath, 
should be temporarily treated like "yes".

I hope this explains it.

Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

Re: [dm-devel] [PATCH v2 2/2] multipath-tools: link LICENSES/LGPL-2.0 to LICENSE.default

[Martin Wilck]

On Thu, 2018-04-12 at 16:57 +0200, Xose Vazquez Perez wrote:
> To indicate that the default licence is LGPL-2.0,
> since there are still a lot of files without a licence header.
> 
> Result:
> multipath-tools/
> └── LICENSE.default -> LICENSES/LGPL-2.0

License.default is better than COPYING, which, as you said yourself,
normally refers to a GPL-type license.

But I'm unsure if this really clarifies matters. If adding my lenghty
review document to the tree is out of the question, a note in the
README might be more helpful than this link:

License
===

The multipath-tools source code is covered by several different
licenses. Refer to the individual source files for details.
Source files which do not have a license header are shipped under 
LGPL-2.0 (see LICENSES/LGPL-2.0).

Regards
Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

Re: [dm-devel] [PATCH v4 09/20] libmultipath: functions to indicate mapping failure in /dev/shm

[Benjamin Marzinski]

On Thu, Apr 12, 2018 at 10:07:13PM +0200, Martin Wilck wrote:
> On Thu, 2018-04-12 at 13:33 -0500, Benjamin Marzinski wrote:
> > On Wed, Apr 04, 2018 at 06:16:16PM +0200, Martin Wilck wrote:
> > > Create a simple API that indicates failure to create a map for a
> > > certain WWID. This will allow multipathd to indicate to other tools
> > > (in particular, "multipath -u" during udev processing) that
> > > an attempt to create a map for a certain wwid failed.
> > > 
> > > The indicator is simply the existence of a file under
> > > /dev/shm/multipath/failed_wwids.
> > 
> > I'm a little confused about the necessity of a lock file here.  What
> > it
> > the race that you are worried about? If two processes try to create a
> > file at the same time, surely one of them will succeed. If two
> > processes
> > try to delete a file at the same time, it will get deleted.  If one
> > process is trying to create a file and one is trying to remove it,
> > the
> > outcome depends on the who wins the race. But this is true whether
> > you
> > add a lock file to make those actions atomic or not. The same goes
> > with
> > stating a file that's being created or removed. As far a I can tell,
> > this should work if you simply create an empty file without any
> > locking.
> > Are you worried about some odd errno due to a race?
> 
> My thinking was that it's generally a good thing to make file system
> operations like this atomic, and the well-tested and mature open_file()
> API was there ready to be used, thus I did.
> 
> You're probably right that the locking not strictly necessary. I don't
> think it hurts, performance-wise the impact is quite low.
> Do you require me to change this, or can we change it later?

That's fine. I don't see how it can hurt.

Reviewed-by: Benjamin Marzinski 

> 
> Martin
> 
> -- 
> Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

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

Re: [dm-devel] [PATCH v4 20/20] multipath.rules: find_multipaths "smart" logic

[Benjamin Marzinski]

On Wed, Apr 04, 2018 at 06:16:27PM +0200, Martin Wilck wrote:
> When the first path to a device appears, we don't know if more paths are going
> to follow. find_multipath "smart" logic attempts to solve this dilemma by
> waiting for additional paths for a configurable time before giving up
> and releasing single paths to upper layers.
> 
> These rules apply only if both find_multipaths is set to "smart" in
> multipath.conf. In this mode, multipath -u sets DM_MULTIPATH_DEVICE_PATH=2 if
> there's no clear evidence wheteher a given device should be a multipath member
> (not blacklisted, not listed as "failed", not in WWIDs file, not member of an
> existing map, only one path seen yet). In this case, "multipath -u" also sets 
> the
> variable FIND_MULTIPATHS_WAIT_UNIL to a relative time stamp (we need to use
> relative "monotonic" time stamps because this may be triggered early during
> boot, before system "calendar" time is correctly initialized).
> 
> In the DM_MULTIPATH_DEVICE_PATH=2 case, pretend that the path is multipath
> member, disallow further processing by systemd (allowing multipathd some time
> to grab the path), and set a systemd timer to check again after the given
> timeout. If the path is still not multipathed by then, pass it on to systemd
> for further processing.
> 


Like I've mentioned, I think multipath -u should be called with a
special option on add events or if FIND_MULTIPATHS_WAIT_UNTIL exists and
is non-zero (or perhaps we should IMPORT DM_MULTIPATH_DEVICE_PATH, and
check if it's 2 or import SYSTEMD_READY and check if it's 0), that smart
mode could use to allow maybes and new claims on existing paths because
another path appeared.

> Signed-off-by: Martin Wilck 
> ---
>  multipath/multipath.rules | 64 
> ---
>  1 file changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/multipath/multipath.rules b/multipath/multipath.rules
> index aab64dc..77b9f16 100644
> --- a/multipath/multipath.rules
> +++ b/multipath/multipath.rules
> @@ -19,9 +19,65 @@ LABEL="test_dev"
>  ENV{MPATH_SBIN_PATH}="/sbin"
>  TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
>  
> -# multipath -u sets DM_MULTIPATH_DEVICE_PATH
> -ENV{DM_MULTIPATH_DEVICE_PATH}!="1", 
> IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
> -ENV{DM_MULTIPATH_DEVICE_PATH}=="1", ENV{ID_FS_TYPE}="mpath_member", \
> - ENV{SYSTEMD_READY}="0"
> +# FIND_MULTIPATHS_WAIT_UNTIL is the timeout (in seconds after the
> +# epoch).
> +IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"
> +ENV{.SAVED_FM_WAIT_UNTIL}="$env{FIND_MULTIPATHS_WAIT_UNTIL}"
> +
> +# multipath -u sets DM_MULTIPATH_DEVICE_PATH and,
> +# if "find_multipaths smart", also FIND_MULTIPATHS_WAIT_UNTIL.
> +ENV{DM_MULTIPATH_DEVICE_PATH}!="1", \
> + IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
> +
> +# case 1: this is definitely multipath
> +ENV{DM_MULTIPATH_DEVICE_PATH}=="1", \
> + ENV{ID_FS_TYPE}="mpath_member", ENV{SYSTEMD_READY}="0", \
> + GOTO="stop_wait"
> +
> +# case 2: this is definitely not multipath, or timeout has expired
> +ENV{DM_MULTIPATH_DEVICE_PATH}!="2", \
> + GOTO="stop_wait"
> +
> +# Code below here is only run in "smart" mode.
> +# multipath -u has indicated this is "maybe" multipath.
> +
> +# This shouldn't happen, just in case.
> +ENV{FIND_MULTIPATHS_WAIT_UNTIL}!="?*", GOTO="end_mpath"
> +
> +# Be careful not to start the timer twice.
> +ACTION!="add", GOTO="pretend_mpath"
> +ENV{.SAVED_FM_WAIT_UNTIL}=="?*", GOTO="pretend_mpath"
> +
> +# At this point, we are seeing this path for the first time, and it's 
> "maybe" multipath.
> +
> +# The actual start command for the timer.
> +#
> +# The purpose of this command is only to make sure we will receive another
> +# uevent eventually. *Any* uevent may cause waiting to finish if it either 
> ends
> +# in case 1-3 above, or if it arrives after FIND_MULTIPATHS_WAIT_UNTIL.
> +#
> +# Note that this will try to activate multipathd if it isn't running yet.
> +# If that fails, the unit starts and expires nonetheless. If multipathd
> +# startup needs to wait for other services, this wait time will add up with
> +# the --on-active timeout.
> +#
> +# We must trigger an "add" event because LVM2 will only act on those.
> +
> +RUN+="/usr/bin/systemd-run --unit=cancel-multipath-wait-$kernel 
> --description 'cancel waiting for multipath siblings of $kernel' --no-block 
> --timer-property DefaultDependencies=no --timer-property 
> Conflicts=shutdown.target --timer-property Before=shutdown.target 
> --timer-property AccuracySec=500ms --property DefaultDependencies=no 
> --property Conflicts=shutdown.target --property Before=shutdown.target 
> --property Wants=multipathd.service --property After=multipathd.service 
> --on-active=$env{FIND_MULTIPATHS_WAIT_UNTIL} /usr/bin/udevadm trigger 
> --action=add $sys$devpath"
> +
> +LABEL="pretend_mpath"
> +ENV{DM_MULTIPATH_DEVICE_PATH}="1"
> +ENV{SYSTEMD_READY}="0"
> +GOTO="end_mpath"

Re: [dm-devel] [PATCH 1/2] libmultipath: hwhandler auto-detection for ALUA

[Benjamin Marzinski]

On Thu, Apr 12, 2018 at 05:43:39PM +0200, Martin Wilck wrote:
> Hi Ben,
> 
> I'm unsure what to do. Do you still reject my patch? Or have you been
> convinced by Hannes and my arguments? 
> Or are you requesting changes? If yes, what? 

I still feel that it's better to make the default config const for
devices that may or may not be ALUA, and let detect_alua figure it out,
rather than allowing multipathd to override a specifically requested
ALUA hardware handler. This is especially true if
get_target_port_group_support() and get_target_port_group succeed, but
get_asymmetric_access_state() fails in detect_alua().  But I don't think
that transient alua errors like this are very likely during multpath
creation, so I not going to reject the patch.

Reviewed-by: Benjmain Marzinski 

> 
> Regards,
> Martin
> 
> -- 
> Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

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

Re: [dm-devel] [PATCH v3 17/20] multipath -u: test if path is busy

[Benjamin Marzinski]

On Mon, Apr 02, 2018 at 09:50:48PM +0200, Martin Wilck wrote:
> For "find_multipaths smart", check if a path is already in use
> before setting DM_MULTIPATH_DEVICE_PATH to 1 or 2 (and thus,
> SYSTEMD_READY=0). If we don't do this, a device which has already been
> mounted (e.g. during initrd processing) may be unmounted by systemd, causing
> havoc to the boot process.

I'm reviewing  v3 of this patch because I don't see patch 17/20 in your
emails from v4. Am I missing an email, or did it not get sent?


> 
> Signed-off-by: Martin Wilck 
> ---
>  multipath/main.c | 31 ++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index d09f117..392d5f0 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -629,16 +629,45 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  
>  
>   if (cmd == CMD_VALID_PATH) {
> + struct path *pp;
> + int fd;
> +
>   /* This only happens if find_multipaths and
>* ignore_wwids is set.
>* 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 || VECTOR_SIZE(pathvec) > 1)
> + if (VECTOR_SIZE(curmp) != 0) {
> + r = 0;
> + goto print_valid;
> + } else if (VECTOR_SIZE(pathvec) > 1)
>   r = 0;
>   else
>   /* Use r=2 as an indication for "maybe" */
>   r = 2;
> +
> + /*
> +  * 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.
> +  *
> +  * 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);

I'm worried about this.  Since we can't be sure that is_failed_wwid()
will really tell us that multipathd has tried to multipath the device
and failed, it is totally possible to get a maybe after multipath has
turned the path device over to the rest of the system. If this is true,
then the exclusive open might race with something else that is trying to
use the device, and cause that to fail.  Or worse, it might win but have
the other process mount the file system on it, only to have multipath go
and claim the device, unmounting it. I still think that the only safe
course is to only do this grab when we know that it is safe, such as on
add events, or if we have already labelled this device as a maybe
device, and we are still waiting on it.

Of course, this means I would exlcude the whole second "if (cmd ==
CMD_VALID_PATH)" section in configure() unless we know that it is safe
to grab the device.  Otherwise, there is nothing to stop us from
claiming a device that is in use. Clearly that exclusive grab check is
racy at any time except on add events or when the device already is set
to SYSTEMD_READY=0.  I'm pretty sure that the coldplug add event after
the switchroot is safe, since nothing will be racing to grab the device
then. 

You've already agreed that it should be fine to allow multipathd to try
to create a multipath device on top of a non-claimed path, since we can
just claim it later by issuing a uevent.  I feel like this is just
another instance of that.  If this isn't a new path, where we have
excluded everyone else from using it, we can't suddenly claim it just
because a second path appears. However, if multipathd manages to create
a multipath device on top of it, then it will add the wwid to the wwids
file, and be able to claim it.  But otherwise, I don't think that the
exclusive grab is safe or reliable enough to allow us to simply do this
on any uevent.

I would add a new option to multipath, that works with -u, to tell it
that maybes are allowed. If find_multipaths == FIND_MULTIPATHS_SMART,
then it should not claim the device if it doesn't get positively claimed
in the first "if (cmd == CMD_VALID_PATH)" section of configure(). That
will save us from claiming devices that are already in use, and speed
the multipath -u calls up.
 

> + if (fd >= 0)
> + close(fd);
> + else {
> + condlog(3, "%s: path %s is in use: %s",
> + __func__, pp->dev,
> + strerror(errno));
> + r = 1;
> +