[PATCH 0/6] Misc multipath patches

[PATCH 0/6] Misc multipath patches

[Benjamin Marzinski]

Here are some various patches I've had waiting to go upstream

Benjamin Marzinski (6):
  libmultipath: cleanup rlookup_binding
  Add support for EMC XtremIO
  libmultipath: Add additional path wildcards
  Fix missing frees and null terminations
  multipath.conf: add all_devs device option
  Always put watchdog in the config structure

 kpartx/devmapper.c                       |  3 +-
 libmultipath/alias.c                     | 30 +++++-----
 libmultipath/blacklist.c                 |  7 +++
 libmultipath/config.c                    | 59 ++++++++++++++++++++
 libmultipath/config.h                    |  3 +-
 libmultipath/dict.c                      | 38 +++++++++++++
 libmultipath/hwtable.c                   | 13 +++++
 libmultipath/print.c                     | 95 +++++++++++++++++++++++++++++++-
 libmultipath/prioritizers/iet.c          |  2 +
 libmultipath/prioritizers/weightedpath.c |  5 +-
 libmultipath/propsel.c                   |  2 +-
 libmultipath/util.c                      |  2 +-
 12 files changed, 237 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[PATCH 1/6] libmultipath: cleanup rlookup_binding

[Benjamin Marzinski]

Before the use_existing_alias() code was added, the scan_device() call in
rlookup_binding always failed, complicated the function and was completely
pointless. Now it has a use, but only when called by use_existing_alias().
Instead of doing this extra work in rlookup_binding() this patch makes
use_existing_alias call scan_device() itself when necessary.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/alias.c   | 29 ++++++++++++++---------------
 libmultipath/propsel.c |  2 +-
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index f1879c9..f2859b0 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -149,13 +149,11 @@ rlookup_binding(FILE *f, char *buff, char *map_alias, char *prefix)
 {
 	char line[LINE_MAX];
 	unsigned int line_nr = 0;
-	int id = 0;
 
 	buff[0] = '\0';
 
 	while (fgets(line, LINE_MAX, f)) {
 		char *c, *alias, *wwid;
-		int curr_id;
 
 		line_nr++;
 		c = strpbrk(line, "#\n\r");
@@ -164,9 +162,6 @@ rlookup_binding(FILE *f, char *buff, char *map_alias, char *prefix)
 		alias = strtok(line, " \t");
 		if (!alias) /* blank line */
 			continue;
-		curr_id = scan_devname(alias, prefix);
-		if (curr_id >= id)
-			id = curr_id + 1;
 		wwid = strtok(NULL, " \t");
 		if (!wwid){
 			condlog(3,
@@ -184,16 +179,12 @@ rlookup_binding(FILE *f, char *buff, char *map_alias, char *prefix)
 				"\nSetting wwid to %s", alias, wwid);
 			strncpy(buff, wwid, WWID_SIZE);
 			buff[WWID_SIZE - 1] = '\0';
-			return id;
+			return 0;
 		}
 	}
 	condlog(3, "No matching alias [%s] in bindings file.", map_alias);
 
-	/* Get the theoretical id for this map alias.
-	 * Used by use_existing_alias
-	 */
-	id = scan_devname(map_alias, prefix);
-	return id;
+	return -1;
 }
 
 static char *
@@ -264,9 +255,7 @@ use_existing_alias (char *wwid, char *file, char *alias_old,
 	/* lookup the binding. if it exsists, the wwid will be in buff
 	 * either way, id contains the id for the alias
 	 */
-	id = rlookup_binding(f , buff,  alias_old, prefix);
-	if (id < 0)
-		goto out;
+	rlookup_binding(f, buff, alias_old, prefix);
 
 	if (strlen(buff) > 0) {
 		/* if buff is our wwid, it's already
@@ -283,7 +272,17 @@ use_existing_alias (char *wwid, char *file, char *alias_old,
 	}
 
 	/* allocate the existing alias in the bindings file */
-	if (can_write && id && !bindings_read_only) {
+	id = scan_devname(alias_old, prefix);
+	if (id <= 0)
+		goto out;
+
+	if (fflush(f) != 0) {
+		condlog(0, "cannot fflush bindings file stream : %s",
+			strerror(errno));
+		goto out;
+	}
+
+	if (can_write && !bindings_read_only) {
 		alias = allocate_binding(fd, wwid, id, prefix);
 		condlog(0, "Allocated existing binding [%s] for WWID [%s]",
 			alias, wwid);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index e23d65c..7a966bb 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -269,7 +269,7 @@ select_alias (struct multipath * mp)
 				mp->alias_old, mp->alias_prefix,
 				conf->bindings_read_only);
 		memset (mp->alias_old, 0, WWID_SIZE);
-	} 
+	}
 
 	if (mp->alias == NULL)
 		mp->alias = get_user_friendly_alias(mp->wwid,
-- 
1.8.3.1

[PATCH 2/6] Add support for EMC XtremIO

[Benjamin Marzinski]

This is a built-in configuration I got from EMC for the XtremIO storage
array.

Signed-off-by: Vincent Chen <vincent.y.chen@emc.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/hwtable.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
index b774453..a9dde7f 100644
--- a/libmultipath/hwtable.c
+++ b/libmultipath/hwtable.c
@@ -1121,6 +1121,19 @@ static struct hwentry default_hw[] = {
 		.prio_name     = PRIO_ALUA,
 		.prio_args     = NULL,
 	},
+	{
+		.vendor        = "XtremIO",
+		.product       = "XtremApp",
+		.features      = DEFAULT_FEATURES,
+		.hwhandler     = DEFAULT_HWHANDLER,
+		.selector      = "queue-length 0",
+		.pgpolicy      = MULTIBUS,
+		.pgfailback    = -FAILBACK_IMMEDIATE,
+		.checker_name  = DIRECTIO,
+		.fast_io_fail  = 15,
+		.prio_name     = DEFAULT_PRIO,
+		.prio_args     = NULL,
+	},
 	/*
 	 * EOL
 	 */
-- 
1.8.3.1

[PATCH 3/6] libmultipath: Add additional path wildcards

[Benjamin Marzinski]

Customers have been asking if multipath can make it easier to see how various
paths in your multipath device are connected.  To help with this I've added
5 new path wildcards

%N  host WWNN
%n  target WWNN
%R  host WWPN
%r  target WWPN
%a  host adapter

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/print.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 1 deletion(-)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index 3c526c2..383eae4 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -10,6 +10,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <errno.h>
+#include <libudev.h>
 
 #include "checkers.h"
 #include "vector.h"
@@ -44,7 +45,7 @@
  * information printing helpers
  */
 static int
-snprint_str (char * buff, size_t len, char * str)
+snprint_str (char * buff, size_t len, const char * str)
 {
 	return snprintf(buff, len, "%s", str);
 }
@@ -436,6 +437,93 @@ snprint_path_mpp (char * buff, size_t len, struct path * pp)
 }
 
 static int
+snprint_host_attr (char * buff, size_t len, struct path * pp, char *attr)
+{
+	struct udev_device *host_dev = NULL;
+	char host_id[32];
+	const char *value = NULL;
+	int ret;
+
+	if (pp->sg_id.proto_id != SCSI_PROTOCOL_FCP)
+		return snprintf(buff, len, "[undef]");
+	sprintf(host_id, "host%d", pp->sg_id.host_no);
+	host_dev = udev_device_new_from_subsystem_sysname(conf->udev, "fc_host",
+							  host_id);
+	if (!host_dev) {
+		condlog(1, "%s: No fc_host device for '%s'", pp->dev, host_id);
+		goto out;
+	}
+	value = udev_device_get_sysattr_value(host_dev, attr);
+	if (value)
+		ret = snprint_str(buff, len, value);
+	udev_device_unref(host_dev);
+out:
+	if (!value)
+		ret = snprintf(buff, len, "[unknown]");
+	return ret;
+}
+
+static int
+snprint_host_wwnn (char * buff, size_t len, struct path * pp)
+{
+	return snprint_host_attr(buff, len, pp, "node_name");
+}
+
+static int
+snprint_host_wwpn (char * buff, size_t len, struct path * pp)
+{
+	return snprint_host_attr(buff, len, pp, "port_name");
+}
+
+static int
+snprint_tgt_wwpn (char * buff, size_t len, struct path * pp)
+{
+	struct udev_device *rport_dev = NULL;
+	char rport_id[32];
+	const char *value = NULL;
+	int ret;
+
+	if (pp->sg_id.proto_id != SCSI_PROTOCOL_FCP)
+		return snprintf(buff, len, "[undef]");
+	sprintf(rport_id, "rport-%d:%d-%d",
+		pp->sg_id.host_no, pp->sg_id.channel, pp->sg_id.transport_id);
+	rport_dev = udev_device_new_from_subsystem_sysname(conf->udev,
+				"fc_remote_ports", rport_id);
+	if (!rport_dev) {
+		condlog(1, "%s: No fc_remote_port device for '%s'", pp->dev,
+			rport_id);
+		goto out;
+	}
+	value = udev_device_get_sysattr_value(rport_dev, "port_name");
+	if (value)
+		ret = snprint_str(buff, len, value);
+	udev_device_unref(rport_dev);
+out:
+	if (!value)
+		ret = snprintf(buff, len, "[unknown]");
+	return ret;
+}
+
+
+static int
+snprint_tgt_wwnn (char * buff, size_t len, struct path * pp)
+{
+	if (pp->tgt_node_name[0] == '\0')
+		return snprintf(buff, len, "[undef]");
+	return snprint_str(buff, len, pp->tgt_node_name);
+}
+
+static int
+snprint_host_adapter (char * buff, size_t len, struct path * pp)
+{
+	char adapter[SLOT_NAME_SIZE];
+
+	if (sysfs_get_host_adapter_name(pp, adapter))
+		return snprintf(buff, len, "[undef]");
+	return snprint_str(buff, len, adapter);
+}
+
+static int
 snprint_path_checker (char * buff, size_t len, struct path * pp)
 {
 	struct checker * c = &pp->checker;
@@ -479,6 +567,11 @@ struct path_data pd[] = {
 	{'S', "size",          0, snprint_path_size},
 	{'z', "serial",        0, snprint_path_serial},
 	{'m', "multipath",     0, snprint_path_mpp},
+	{'N', "host WWNN",     0, snprint_host_wwnn},
+	{'n', "target WWNN",   0, snprint_tgt_wwnn},
+	{'R', "host WWPN",     0, snprint_host_wwpn},
+	{'r', "target WWPN",   0, snprint_tgt_wwpn},
+	{'a', "host adapter",  0, snprint_host_adapter},
 	{0, NULL, 0 , NULL}
 };
 
-- 
1.8.3.1

[PATCH 4/6] Fix missing frees and null terminations

[Benjamin Marzinski]

There were a number of error code paths where multipath wasn't correctly
freeing memory.  Also, readlink doesn't null terminate strings, so
multipath needs to make sure that they get terminated.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/devmapper.c                       | 3 ++-
 libmultipath/alias.c                     | 1 +
 libmultipath/blacklist.c                 | 7 +++++++
 libmultipath/prioritizers/iet.c          | 2 ++
 libmultipath/prioritizers/weightedpath.c | 5 ++++-
 libmultipath/util.c                      | 2 +-
 6 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 7879a09..a3272d4 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -132,8 +132,9 @@ dm_addmap (int task, const char *name, const char *target,
 		goto addout;
 	r = dm_task_run (dmt);
 
-	addout:
+addout:
 	dm_task_destroy (dmt);
+	free(prefixed_uuid);
 
 	return r;
 }
diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index f2859b0..7d12a0c 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -328,6 +328,7 @@ get_user_friendly_alias(char *wwid, char *file, char *prefix,
 	if (fflush(f) != 0) {
 		condlog(0, "cannot fflush bindings file stream : %s",
 			strerror(errno));
+		free(alias);
 		fclose(f);
 		return NULL;
 	}
diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index 651bd7e..3f9e80b 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -80,6 +80,8 @@ set_ble_device (vector blist, char * vendor, char * product, int origin)
 		if (regcomp(&ble->vendor_reg, vendor,
 			    REG_EXTENDED|REG_NOSUB)) {
 			FREE(vendor);
+			if (product)
+				FREE(product);
 			return 1;
 		}
 		ble->vendor = vendor;
@@ -88,6 +90,10 @@ set_ble_device (vector blist, char * vendor, char * product, int origin)
 		if (regcomp(&ble->product_reg, product,
 			    REG_EXTENDED|REG_NOSUB)) {
 			FREE(product);
+			if (vendor) {
+				ble->vendor = NULL;
+				FREE(vendor);
+			}
 			return 1;
 		}
 		ble->product = product;
@@ -202,6 +208,7 @@ setup_default_blist (struct config * conf)
 					   STRDUP(hwe->bl_product),
 					   ORIGIN_DEFAULT)) {
 				FREE(ble);
+				vector_del_slot(conf->blist_device, VECTOR_SIZE(conf->blist_device) - 1);
 				return 1;
 			}
 		}
diff --git a/libmultipath/prioritizers/iet.c b/libmultipath/prioritizers/iet.c
index 94406df..0bcc48b 100644
--- a/libmultipath/prioritizers/iet.c
+++ b/libmultipath/prioritizers/iet.c
@@ -109,6 +109,7 @@ int iet_prio(const char *dev, char * args)
 			ssize_t nchars = readlink(path, buffer, sizeof(buffer)-1);
 			if (nchars != -1) {
 				char *device;
+				buffer[nchars] = '\0';
 				device = find_regex(buffer,"(sd[a-z]+)");
 				// if device parsed is the right one
 				if (device!=NULL && strncmp(device, dev, strlen(device)) == 0) {
@@ -118,6 +119,7 @@ int iet_prio(const char *dev, char * args)
 					if (ip!=NULL && strncmp(ip, preferredip, strlen(ip)) == 0) {
 						// high prio
 						free(ip);
+						free(device);
 						closedir(dir_p);
 						return 20;
 					}
diff --git a/libmultipath/prioritizers/weightedpath.c b/libmultipath/prioritizers/weightedpath.c
index 54c9039..13bc52f 100644
--- a/libmultipath/prioritizers/weightedpath.c
+++ b/libmultipath/prioritizers/weightedpath.c
@@ -61,8 +61,10 @@ int prio_path_weight(struct path *pp, char *prio_args)
 	regex = get_next_string(&temp, split_char);
 
 	/* Return default priority if the argument is not parseable */
-	if (!regex)
+	if (!regex) {
+		FREE(arg);
 		return priority;
+	}
 
 	if (!strcmp(regex, HBTL)) {
 		sprintf(path, "%d:%d:%d:%d", pp->sg_id.host_no,
@@ -72,6 +74,7 @@ int prio_path_weight(struct path *pp, char *prio_args)
 	} else {
 		condlog(0, "%s: %s - Invalid arguments", pp->dev,
 			pp->prio.name);
+		FREE(arg);
 		return priority;
 	}
 
diff --git a/libmultipath/util.c b/libmultipath/util.c
index 06a6311..b8487ac 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -165,7 +165,7 @@ devt2devname (char *devname, int devname_len, char *devt)
 		sprintf(block_path,"/sys/dev/block/%u:%u", major, minor);
 		if (lstat(block_path, &statbuf) == 0) {
 			if (S_ISLNK(statbuf.st_mode) &&
-			    readlink(block_path, dev, FILE_NAME_SIZE) > 0) {
+			    readlink(block_path, dev, FILE_NAME_SIZE-1) > 0) {
 				char *p = strrchr(dev, '/');
 
 				if (!p) {
-- 
1.8.3.1

[PATCH 5/6] multipath.conf: add all_devs device option

[Benjamin Marzinski]

Sometimes users want to to be able to set a configuration value for all
their devices (for instance, they may want all devices to set no_path_retry
to fail). The builtin device configurations make this tricky, since
they need to change each one device configuration individually.  To avoid
that, this patch adds a new device config option, "all_devs". When this is
set to "yes", the options set in this devices section will override those
values in all the builtin configs.

For instance, to make all builtin configs set no_path_retry to fail, you
could add:

devices {
	device {
		all_devs yes
		no_path_retry fail
	}
}


Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/config.h |  1 +
 libmultipath/dict.c   | 38 +++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 39963b4..d326765 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -112,6 +112,8 @@ find_hwe (vector hwtable, char * vendor, char * product, char * revision)
 	 * continuing to the generic entries
 	 */
 	vector_foreach_slot_backwards (hwtable, tmp, i) {
+		if (tmp->all_devs == 1)
+			continue;
 		if (hwe_regmatch(tmp, &hwe))
 			continue;
 		ret = tmp;
@@ -353,6 +355,59 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
 	return 0;
 }
 
+#define overwrite_str(s) \
+do { \
+	if (src->s) { \
+		if (dst->s) \
+			FREE(dst->s); \
+		if (!(dst->s = set_param_str(src->s))) \
+			return 1; \
+	} \
+} while(0)
+
+#define overwrite_num(s) \
+do { \
+	if (src->s) \
+		dst->s = src->s; \
+} while(0)
+
+static int
+overwrite_hwe (struct hwentry * dst, struct hwentry * src)
+{
+	/* don't overwrite vendor, product, revision or all_devs */
+	overwrite_str(uid_attribute);
+	overwrite_str(features);
+	overwrite_str(hwhandler);
+	overwrite_str(selector);
+	overwrite_str(checker_name);
+	overwrite_str(prio_name);
+	overwrite_str(prio_args);
+	overwrite_str(alias_prefix);
+	overwrite_str(bl_product);
+	overwrite_num(pgpolicy);
+	overwrite_num(pgfailback);
+	overwrite_num(rr_weight);
+	overwrite_num(no_path_retry);
+	overwrite_num(minio);
+	overwrite_num(minio_rq);
+	overwrite_num(flush_on_last_del);
+	overwrite_num(fast_io_fail);
+	overwrite_num(dev_loss);
+	overwrite_num(user_friendly_names);
+	overwrite_num(retain_hwhandler);
+	overwrite_num(detect_prio);
+
+	/*
+	 * Make sure features is consistent with
+	 * no_path_retry
+	 */
+	if (dst->no_path_retry == NO_PATH_RETRY_FAIL)
+		remove_feature(&dst->features, "queue_if_no_path");
+	else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF)
+		add_feature(&dst->features, "queue_if_no_path");
+	return 0;
+}
+
 int
 store_hwe (vector hwtable, struct hwentry * dhwe)
 {
@@ -438,6 +493,10 @@ restart:
 			break;
 		j = n;
 		vector_foreach_slot_after(hw, hwe2, j) {
+			if (hwe2->all_devs == 1) {
+				overwrite_hwe(hwe1, hwe2);
+				continue;
+			}
 			if (hwe_regmatch(hwe1, hwe2))
 				continue;
 			/* dup */
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 844ee12..2097879 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -47,6 +47,7 @@ struct hwentry {
 	char * prio_args;
 	char * alias_prefix;
 
+	int all_devs;
 	int pgpolicy;
 	int pgfailback;
 	int rr_weight;
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 7de7a67..5b4fbe6 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -918,6 +918,32 @@ device_handler(vector strvec)
 }
 
 static int
+all_devs_handler(vector strvec)
+{
+	char * buff;
+	struct hwentry *hwe = VECTOR_LAST_SLOT(conf->hwtable);
+
+	if (!hwe)
+		return 1;
+
+	buff = set_value(strvec);
+	if (!buff)
+		return 1;
+
+	if ((strlen(buff) == 2 && !strcmp(buff, "no")) ||
+	    (strlen(buff) == 1 && !strcmp(buff, "0")))
+		hwe->all_devs = 0;
+	else if ((strlen(buff) == 3 && !strcmp(buff, "yes")) ||
+		 (strlen(buff) == 1 && !strcmp(buff, "1")))
+		hwe->all_devs = 1;
+	else
+		hwe->all_devs = 0;
+
+	FREE(buff);
+	return 0;
+}
+
+static int
 vendor_handler(vector strvec)
 {
 	struct hwentry * hwe = VECTOR_LAST_SLOT(conf->hwtable);
@@ -2151,6 +2177,17 @@ snprint_hw_dev_loss(char * buff, int len, void * data)
 }
 
 static int
+snprint_hw_all_devs (char *buff, int len, void *data)
+{
+	struct hwentry * hwe = (struct hwentry *)data;
+
+	if (!hwe->all_devs)
+		return 0;
+
+	return snprintf(buff, len, "yes");
+}
+
+static int
 snprint_hw_vendor (char * buff, int len, void * data)
 {
 	struct hwentry * hwe = (struct hwentry *)data;
@@ -2922,6 +2959,7 @@ init_keywords(void)
 	install_keyword_root("devices", &devices_handler);
 	install_keyword_multi("device", &device_handler, NULL);
 	install_sublevel();
+	install_keyword("all_devs", &all_devs_handler, &snprint_hw_all_devs);
 	install_keyword("vendor", &vendor_handler, &snprint_hw_vendor);
 	install_keyword("product", &product_handler, &snprint_hw_product);
 	install_keyword("revision", &revision_handler, &snprint_hw_revision);
-- 
1.8.3.1

[PATCH 6/6] Always put watchdog in the config structure

[Benjamin Marzinski]

Only multipathd and libmultipath ever get compiled with USE_SYSTEMD
enabled. If the watchdog variable is only in the config structure when
USE_SYSTEMD is enabled, it means that the multipath and libmultipath
code have different views of the config structure. This causes all sorts
of errors when multipath calls a libmultipath function.

The other alternative would be to leave the preprocessor #ifdef and
make sure to compile all tools with

CFLAGS += -DUSE_SYSTEMD=$(SYSTEMD)

Removing the #ifdef in the config structure means we don't have to worry
about this mistake happening again in the future, and only takes up 8
extra bytes in the structure.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index 2097879..5b6d107 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -110,9 +110,7 @@ struct config {
 	int ignore_wwids;
 	int checker_timeout;
 	int daemon;
-#ifdef USE_SYSTEMD
 	int watchdog;
-#endif
 	int flush_on_last_del;
 	int attribute_flags;
 	int fast_io_fail;
-- 
1.8.3.1

Re: [PATCH 0/6] Misc multipath patches

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 1659 bytes --]

Hi,

this patchset is now all merged upstream, except the "all_devs device
option" part.
I'd like other's comments on this.
I personnality think the goal is fine, but the configuration plug is not so
intuitive.
We already have a top-level "default" section, would defining a top-level
"override" section be more intuitive ?

I'll release 0.5.1, once this feature is merged in the current form or
another.

Best regards,
Christophe Varoqui
OpenSVC


On Fri, Sep 12, 2014 at 7:44 PM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:

> Here are some various patches I've had waiting to go upstream
>
> Benjamin Marzinski (6):
>   libmultipath: cleanup rlookup_binding
>   Add support for EMC XtremIO
>   libmultipath: Add additional path wildcards
>   Fix missing frees and null terminations
>   multipath.conf: add all_devs device option
>   Always put watchdog in the config structure
>
>  kpartx/devmapper.c                       |  3 +-
>  libmultipath/alias.c                     | 30 +++++-----
>  libmultipath/blacklist.c                 |  7 +++
>  libmultipath/config.c                    | 59 ++++++++++++++++++++
>  libmultipath/config.h                    |  3 +-
>  libmultipath/dict.c                      | 38 +++++++++++++
>  libmultipath/hwtable.c                   | 13 +++++
>  libmultipath/print.c                     | 95
> +++++++++++++++++++++++++++++++-
>  libmultipath/prioritizers/iet.c          |  2 +
>  libmultipath/prioritizers/weightedpath.c |  5 +-
>  libmultipath/propsel.c                   |  2 +-
>  libmultipath/util.c                      |  2 +-
>  12 files changed, 237 insertions(+), 22 deletions(-)
>
> --
> 1.8.3.1
>
>

[-- Attachment #1.2: Type: text/html, Size: 2492 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/6] Misc multipath patches

[Nir Soffer]

----- Original Message -----
> From: "Christophe Varoqui" <christophe.varoqui@opensvc.com>
> To: "Benjamin Marzinski" <bmarzins@redhat.com>
> Cc: "device-mapper development" <dm-devel@redhat.com>
> Sent: Saturday, September 13, 2014 9:57:00 AM
> Subject: Re: [dm-devel] [PATCH 0/6] Misc multipath patches
> 
> Hi,
> 
> this patchset is now all merged upstream, except the " all_devs device
> option" part.
> I'd like other's comments on this.
> I personnality think the goal is fine, but the configuration plug is not so
> intuitive.
> We already have a top-level "default" section, would defining a top-level
> "override" section be more intuitive ?

Ben, can you share the url of your repository?

Nir

Re: [PATCH 0/6] Misc multipath patches

[Nir Soffer]

----- Original Message -----
> From: "Christophe Varoqui" <christophe.varoqui@opensvc.com>
> To: "Benjamin Marzinski" <bmarzins@redhat.com>
> Cc: "device-mapper development" <dm-devel@redhat.com>
> Sent: Saturday, September 13, 2014 9:57:00 AM
> Subject: Re: [dm-devel] [PATCH 0/6] Misc multipath patches
> 
> Hi,
> 
> this patchset is now all merged upstream, except the " all_devs device
> option" part.
> I'd like other's comments on this.
> I personnality think the goal is fine, 

The goal is critical for us (ovirt/rhev). We need a way to configure any
any device on multiple (100's) hosts to use no_path_retry fail.
We have several bugs caused by unwanted queuing in multipath.

> but the configuration plug is not so
> intuitive.

I agree that the suggested patch is not intuitive.

all_devs overrides only devices with built-in configuration. Other 
devices will use the defaults.

So practically this is not enough:

    devices {
        device {
            all_devs yes
            no_path_retry fail
        }
    }

You must do this:

    defaults {
        no_path_retry fail         # keep in sync with all_devs bellow
    }

    devices {
        device {
            all_devs yes
            no_path_retry fail     # keep in sync with defaults above
        }
    }

It would be nice if we could add a device that match any device,
and use it to override *some* settings:

    devices {
        device {
            vendor ".*"
            no_path_retry fail
        }
    }

I tried this, and it seem to remove settings not defined in this device
for example, hwhandler. Looks like multipath does not use the built-in
configuration for any device when such device exists.

Making this work will probably be backward incompatible since
people may assume the old behavior.

Another option that may be useful is:

    multipaths {
        multiapth {
            wwid ".*"
            no_path_retry fail
        }
    }

This does not work because multipath sections are matched using strcmp.
Changing this may be also not backward compatible.

> We already have a top-level "default" section, would defining a top-level
> "override" section be more intuitive ?

I think that would be much better from the user point of view, and
should be simpler to implement.

What I would expect from this configuration is to be able to set:

    overrides {
        no_path_retry fail
    }

And have it override all devices, both those with built-in configuration
and those that have no configuration.

Not sure it this should override also devices with device section
in multipath configuration, since this will prevent an admin to 
configure specific device.

Nir

Re: [PATCH 5/6] multipath.conf: add all_devs device option

[Nir Soffer]

----- Original Message -----
> From: "Benjamin Marzinski" <bmarzins@redhat.com>
> To: "device-mapper development" <dm-devel@redhat.com>
> Cc: "Christophe Varoqui" <christophe.varoqui@gmail.com>
> Sent: Friday, September 12, 2014 8:44:51 PM
> Subject: [dm-devel] [PATCH 5/6] multipath.conf: add all_devs device option
> 
> Sometimes users want to to be able to set a configuration value for all
> their devices (for instance, they may want all devices to set no_path_retry
> to fail). The builtin device configurations make this tricky, since
> they need to change each one device configuration individually.  To avoid
> that, this patch adds a new device config option, "all_devs". When this is
> set to "yes", the options set in this devices section will override those
> values in all the builtin configs.
> 
> For instance, to make all builtin configs set no_path_retry to fail, you
> could add:
> 
> devices {
> 	device {
> 		all_devs yes
> 		no_path_retry fail
> 	}
> }
> 
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/config.c | 59
>  +++++++++++++++++++++++++++++++++++++++++++++++++++
>  libmultipath/config.h |  1 +
>  libmultipath/dict.c   | 38 +++++++++++++++++++++++++++++++++
>  3 files changed, 98 insertions(+)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 39963b4..d326765 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -112,6 +112,8 @@ find_hwe (vector hwtable, char * vendor, char * product,
> char * revision)
>  	 * continuing to the generic entries
>  	 */
>  	vector_foreach_slot_backwards (hwtable, tmp, i) {
> +		if (tmp->all_devs == 1)
> +			continue;
>  		if (hwe_regmatch(tmp, &hwe))
>  			continue;
>  		ret = tmp;
> @@ -353,6 +355,59 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
>  	return 0;
>  }
>  
> +#define overwrite_str(s) \
> +do { \
> +	if (src->s) { \
> +		if (dst->s) \
> +			FREE(dst->s); \
> +		if (!(dst->s = set_param_str(src->s))) \
> +			return 1; \
> +	} \
> +} while(0)
> +
> +#define overwrite_num(s) \
> +do { \
> +	if (src->s) \
> +		dst->s = src->s; \
> +} while(0)
> +
> +static int
> +overwrite_hwe (struct hwentry * dst, struct hwentry * src)
> +{
> +	/* don't overwrite vendor, product, revision or all_devs */
> +	overwrite_str(uid_attribute);
> +	overwrite_str(features);
> +	overwrite_str(hwhandler);
> +	overwrite_str(selector);
> +	overwrite_str(checker_name);
> +	overwrite_str(prio_name);
> +	overwrite_str(prio_args);
> +	overwrite_str(alias_prefix);
> +	overwrite_str(bl_product);
> +	overwrite_num(pgpolicy);
> +	overwrite_num(pgfailback);
> +	overwrite_num(rr_weight);
> +	overwrite_num(no_path_retry);
> +	overwrite_num(minio);
> +	overwrite_num(minio_rq);
> +	overwrite_num(flush_on_last_del);
> +	overwrite_num(fast_io_fail);
> +	overwrite_num(dev_loss);
> +	overwrite_num(user_friendly_names);
> +	overwrite_num(retain_hwhandler);
> +	overwrite_num(detect_prio);
> +
> +	/*
> +	 * Make sure features is consistent with
> +	 * no_path_retry
> +	 */
> +	if (dst->no_path_retry == NO_PATH_RETRY_FAIL)
> +		remove_feature(&dst->features, "queue_if_no_path");
> +	else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF)
> +		add_feature(&dst->features, "queue_if_no_path");

This duplicates the same code in merge_hwe(). It would be a good idea
to extract a function for this and call it from both places.

> +	return 0;
> +}
> +
>  int
>  store_hwe (vector hwtable, struct hwentry * dhwe)
>  {
> @@ -438,6 +493,10 @@ restart:
>  			break;
>  		j = n;
>  		vector_foreach_slot_after(hw, hwe2, j) {
> +			if (hwe2->all_devs == 1) {
> +				overwrite_hwe(hwe1, hwe2);
> +				continue;
> +			}

This code is already little tricky, and adding it here
means you will overwrite again when the loop restarts.

Why not do this as separate step before factorizing?

1. read config
2. override built-in devices with all_devs settings
3. factorize (remove duplicates)

>  			if (hwe_regmatch(hwe1, hwe2))
>  				continue;
>  			/* dup */
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 844ee12..2097879 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -47,6 +47,7 @@ struct hwentry {
>  	char * prio_args;
>  	char * alias_prefix;
>  
> +	int all_devs;
>  	int pgpolicy;
>  	int pgfailback;
>  	int rr_weight;
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index 7de7a67..5b4fbe6 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -918,6 +918,32 @@ device_handler(vector strvec)
>  }
>  
>  static int
> +all_devs_handler(vector strvec)
> +{
> +	char * buff;
> +	struct hwentry *hwe = VECTOR_LAST_SLOT(conf->hwtable);
> +
> +	if (!hwe)
> +		return 1;
> +
> +	buff = set_value(strvec);
> +	if (!buff)
> +		return 1;
> +
> +	if ((strlen(buff) == 2 && !strcmp(buff, "no")) ||
> +	    (strlen(buff) == 1 && !strcmp(buff, "0")))
> +		hwe->all_devs = 0;
> +	else if ((strlen(buff) == 3 && !strcmp(buff, "yes")) ||
> +		 (strlen(buff) == 1 && !strcmp(buff, "1")))
> +		hwe->all_devs = 1;
> +	else
> +		hwe->all_devs = 0;

Why not:

    hwe->all_devs = !strcmp(buff, "1") || !strcmp(buff, "yes");

> +
> +	FREE(buff);
> +	return 0;
> +}

Nir