[dm-devel] [PATCH 0/9] Add protocol specific config subsection

[dm-devel] [PATCH 0/9] Add protocol specific config subsection

[Benjamin Marzinski]

Some storage arrays can be accessed using multiple protocols at the same
time.  I've have customers request the ability to set different values
for the path specific timeouts, like fast_io_fail_tmo, based on the
protocol used to access the path. In order to make this possible, this
patchset adds a new protocol subsection to the device subsection and the
overrides section. This allows users to set a device config's path
specific timeouts, such as dev_loss_tmo, fast_io_fail_tmo, and
eh_deadline on a per-protocol basis.

Benjamin Marzinski (9):
  libmultipath: steal the src string pointer in merge_str()
  libmultipath: make protocol_name global
  libmultipath: add a protocol subsection to multipath.conf
  libmultipath: Set the scsi timeout parameters by path
  libmultipath: check the hwentry pctable for path variables
  libmultipath: make snprint_pctable indent a variable amount
  libmultipath: add procotol subsection to overrides
  libmultipath: fix eh_deadline documentation
  libmultipath: Add documentation for the protocol subsection

 libmultipath/config.c      | 116 ++++++++++++++++++++++---
 libmultipath/config.h      |  10 +++
 libmultipath/configure.c   |   5 +-
 libmultipath/dict.c        | 141 ++++++++++++++++++++++++++++++
 libmultipath/discovery.c   | 174 +++++++++++++++++++++----------------
 libmultipath/discovery.h   |   2 +-
 libmultipath/print.c       |  80 +++++++++++++----
 libmultipath/propsel.c     |  89 ++++++++++++++-----
 libmultipath/propsel.h     |   6 +-
 libmultipath/structs.c     |  19 +++-
 libmultipath/structs.h     |   7 +-
 multipath/multipath.conf.5 |  42 +++++++++
 12 files changed, 555 insertions(+), 136 deletions(-)

-- 
2.17.2

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

[dm-devel] [PATCH 1/9] libmultipath: steal the src string pointer in merge_str()

[Benjamin Marzinski]

Instead of allocating a copy when the original string is going to be
freed right after the merge, just steal the pointer. Also, merge_mpe()
can't get called with NULL arguments.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index c595e768..612941b8 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -387,9 +387,9 @@ set_param_str(const char * str)
 }
 
 #define merge_str(s) \
-	if (!dst->s && src->s) { \
-		if (!(dst->s = set_param_str(src->s))) \
-			return 1; \
+	if (!dst->s && src->s && strlen(src->s)) { \
+		dst->s = src->s; \
+		src->s = NULL; \
 	}
 
 #define merge_num(s) \
@@ -397,7 +397,7 @@ set_param_str(const char * str)
 		dst->s = src->s
 
 
-static int
+static void
 merge_hwe (struct hwentry * dst, struct hwentry * src)
 {
 	char id[SCSI_VENDOR_SIZE+PATH_PRODUCT_SIZE];
@@ -449,15 +449,11 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
 	reconcile_features_with_options(id, &dst->features,
 					&dst->no_path_retry,
 					&dst->retain_hwhandler);
-	return 0;
 }
 
-static int
+static void
 merge_mpe(struct mpentry *dst, struct mpentry *src)
 {
-	if (!dst || !src)
-		return 1;
-
 	merge_str(alias);
 	merge_str(uid_attribute);
 	merge_str(getuid);
@@ -499,8 +495,6 @@ merge_mpe(struct mpentry *dst, struct mpentry *src)
 	merge_num(uid);
 	merge_num(gid);
 	merge_num(mode);
-
-	return 0;
 }
 
 void merge_mptable(vector mptable)
-- 
2.17.2

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

Re: [dm-devel] [PATCH 1/9] libmultipath: steal the src string pointer in merge_str()

[Martin Wilck]

On Mon, 2022-04-11 at 20:59 -0500, Benjamin Marzinski wrote:
> Instead of allocating a copy when the original string is going to be
> freed right after the merge, just steal the pointer. Also,
> merge_mpe()
> can't get called with NULL arguments.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

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

[dm-devel] [PATCH 2/9] libmultipath: make protocol_name global

[Benjamin Marzinski]

Future patches will make use of this, so move it out of
snprint_path_protocol()

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/print.c   | 17 -----------------
 libmultipath/structs.c | 18 ++++++++++++++++++
 libmultipath/structs.h |  1 +
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index bf88f301..27c2cf1a 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -754,23 +754,6 @@ snprint_path_failures(struct strbuf *buff, const struct path * pp)
 int
 snprint_path_protocol(struct strbuf *buff, const struct path * pp)
 {
-	static const char * const protocol_name[LAST_BUS_PROTOCOL_ID + 1] = {
-		[SYSFS_BUS_UNDEF] = "undef",
-		[SYSFS_BUS_CCW] = "ccw",
-		[SYSFS_BUS_CCISS] = "cciss",
-		[SYSFS_BUS_NVME] = "nvme",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_FCP] = "scsi:fcp",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_SPI] = "scsi:spi",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_SSA] = "scsi:ssa",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_SBP] = "scsi:sbp",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_SRP] = "scsi:srp",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_ISCSI] = "scsi:iscsi",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_SAS] = "scsi:sas",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_ADT] = "scsi:adt",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_ATA] = "scsi:ata",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_USB] = "scsi:usb",
-		[SYSFS_BUS_SCSI + SCSI_PROTOCOL_UNSPEC] = "scsi:unspec",
-	};
 	const char *pn = protocol_name[bus_protocol_id(pp)];
 
 	assert(pn != NULL);
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 4b62da54..04cfdcdc 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -20,6 +20,24 @@
 #include "dm-generic.h"
 #include "devmapper.h"
 
+const char * const protocol_name[LAST_BUS_PROTOCOL_ID + 1] = {
+	[SYSFS_BUS_UNDEF] = "undef",
+	[SYSFS_BUS_CCW] = "ccw",
+	[SYSFS_BUS_CCISS] = "cciss",
+	[SYSFS_BUS_NVME] = "nvme",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_FCP] = "scsi:fcp",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_SPI] = "scsi:spi",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_SSA] = "scsi:ssa",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_SBP] = "scsi:sbp",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_SRP] = "scsi:srp",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_ISCSI] = "scsi:iscsi",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_SAS] = "scsi:sas",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_ADT] = "scsi:adt",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_ATA] = "scsi:ata",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_USB] = "scsi:usb",
+	[SYSFS_BUS_SCSI + SCSI_PROTOCOL_UNSPEC] = "scsi:unspec",
+};
+
 struct adapter_group *
 alloc_adaptergroup(void)
 {
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index d94f93a0..3722e31b 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -192,6 +192,7 @@ enum scsi_protocol {
  */
 #define LAST_BUS_PROTOCOL_ID (SYSFS_BUS_SCSI + SCSI_PROTOCOL_UNSPEC)
 unsigned int bus_protocol_id(const struct path *pp);
+extern const char * const protocol_name[];
 
 #define SCSI_INVALID_LUN ~0ULL
 
-- 
2.17.2

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

Re: [dm-devel] [PATCH 2/9] libmultipath: make protocol_name global

[Martin Wilck]

On Mon, 2022-04-11 at 20:59 -0500, Benjamin Marzinski wrote:
> Future patches will make use of this, so move it out of
> snprint_path_protocol()
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

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

[dm-devel] [PATCH 3/9] libmultipath: add a protocol subsection to multipath.conf

[Benjamin Marzinski]

Some storage arrays can be accessed using multiple protocols at the same
time.  In these cases, users may want to set path attributes
differently, depending on the protocol that the path is using. To allow
this, add a protocol subsection to the device subsection in
multipath.conf, which allows select path-specific options to be set.
This commit simply adds the subsection, and handles merging matching
entries, both within a hwentry, and when hwentries are merged. Future
patches will make use of the section.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c | 102 ++++++++++++++++++++++++++++++++++++++++++
 libmultipath/config.h |  10 +++++
 libmultipath/dict.c   |  99 ++++++++++++++++++++++++++++++++++++++++
 libmultipath/print.c  |  44 ++++++++++++++++++
 4 files changed, 255 insertions(+)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 612941b8..61d3c182 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -237,6 +237,18 @@ const char *get_mpe_wwid(const struct _vector *mptable, const char *alias)
 	return NULL;
 }
 
+static void
+free_pctable (vector pctable)
+{
+	int i;
+	struct pcentry *pce;
+
+	vector_foreach_slot(pctable, pce, i)
+		free(pce);
+
+	vector_free(pctable);
+}
+
 void
 free_hwe (struct hwentry * hwe)
 {
@@ -282,6 +294,9 @@ free_hwe (struct hwentry * hwe)
 	if (hwe->bl_product)
 		free(hwe->bl_product);
 
+	if (hwe->pctable)
+		free_pctable(hwe->pctable);
+
 	free(hwe);
 }
 
@@ -363,6 +378,15 @@ alloc_hwe (void)
 	return hwe;
 }
 
+struct pcentry *
+alloc_pce (void)
+{
+	struct pcentry *pce = (struct pcentry *)
+				calloc(1, sizeof(struct pcentry));
+	pce->type = -1;
+	return pce;
+}
+
 static char *
 set_param_str(const char * str)
 {
@@ -396,6 +420,41 @@ set_param_str(const char * str)
 	if (!dst->s && src->s) \
 		dst->s = src->s
 
+static void
+merge_pce(struct pcentry *dst, struct pcentry *src)
+{
+	merge_num(fast_io_fail);
+	merge_num(dev_loss);
+	merge_num(eh_deadline);
+}
+
+static int
+merge_pctable(vector d_pctable, vector s_pctable)
+{
+	struct pcentry *d_pce, *s_pce;
+	int i, j, start = 0;
+
+	vector_foreach_slot_backwards(s_pctable, s_pce, i) {
+		bool found = false;
+		j = start;
+		vector_foreach_slot_after(d_pctable, d_pce, j) {
+			if (s_pce->type != d_pce->type)
+				continue;
+			found = true;
+			merge_pce(d_pce, s_pce);
+			vector_del_slot(s_pctable, i);
+			free(s_pce);
+			break;
+		}
+		if (found)
+			continue;
+		if (!vector_insert_slot(d_pctable, 0, s_pce))
+			return 1;
+		vector_del_slot(s_pctable, i);
+		start++;
+	}
+	return 0;
+}
 
 static void
 merge_hwe (struct hwentry * dst, struct hwentry * src)
@@ -445,6 +504,13 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
 	merge_num(marginal_path_err_recheck_gap_time);
 	merge_num(marginal_path_double_failed_time);
 
+	if (src->pctable) {
+		if (!dst->pctable)
+			dst->pctable = steal_ptr(src->pctable);
+		else
+			merge_pctable(dst->pctable, src->pctable);
+	}
+
 	snprintf(id, sizeof(id), "%s/%s", dst->vendor, dst->product);
 	reconcile_features_with_options(id, &dst->features,
 					&dst->no_path_retry,
@@ -602,6 +668,41 @@ out:
 	return 1;
 }
 
+static void
+factorize_pctable(struct hwentry *hwe, const char *table_desc)
+{
+	struct pcentry *pce1, *pce2;
+	int i, j;
+
+	if (!hwe->pctable)
+		return;
+
+	vector_foreach_slot(hwe->pctable, pce1, i) {
+		if (pce1->type < 0) {
+			condlog(0, "protocol section from %s:%s in %s missing type",
+				hwe->vendor, hwe->product, table_desc);
+			vector_del_slot(hwe->pctable, i--);
+			free(pce1);
+			continue;
+		}
+		j = i + 1;
+		vector_foreach_slot_after(hwe->pctable, pce2, j) {
+			if (pce1->type != pce2->type)
+				continue;
+			merge_pce(pce2,pce1);
+			vector_del_slot(hwe->pctable, i--);
+			free(pce1);
+			break;
+		}
+
+	}
+
+	if (VECTOR_SIZE(hwe->pctable) == 0) {
+		vector_free(hwe->pctable);
+		hwe->pctable = NULL;
+	}
+}
+
 static void
 factorize_hwtable (vector hw, int n, const char *table_desc)
 {
@@ -618,6 +719,7 @@ restart:
 			free_hwe(hwe1);
 			continue;
 		}
+		factorize_pctable(hwe1, table_desc);
 		j = n > i + 1 ? n : i + 1;
 		vector_foreach_slot_after(hw, hwe2, j) {
 			if (hwe_strmatch(hwe2, hwe1) == 0) {
diff --git a/libmultipath/config.h b/libmultipath/config.h
index c73389b5..b7bca9a8 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -40,6 +40,13 @@ enum force_reload_types {
 	FORCE_RELOAD_WEAK,
 };
 
+struct pcentry {
+	int type;
+	int fast_io_fail;
+	unsigned int dev_loss;
+	int eh_deadline;
+};
+
 struct hwentry {
 	char * vendor;
 	char * product;
@@ -85,6 +92,8 @@ struct hwentry {
 	int vpd_vendor_id;
 	int recheck_wwid;
 	char * bl_product;
+
+	vector pctable;
 };
 
 struct mpentry {
@@ -284,6 +293,7 @@ const char *get_mpe_wwid (const struct _vector *mptable, const char *alias);
 
 struct hwentry * alloc_hwe (void);
 struct mpentry * alloc_mpe (void);
+struct pcentry * alloc_pce (void);
 
 void free_hwe (struct hwentry * hwe);
 void free_hwtable (vector hwtable);
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 26cbe785..04d86ee3 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -417,6 +417,30 @@ snprint_mp_ ## option (struct config *conf, struct strbuf *buff,	\
 	return function(buff, mpe->option);				\
 }
 
+#define declare_pc_handler(option, function)				\
+static int								\
+pc_ ## option ## _handler (struct config *conf, vector strvec,		\
+			   const char *file, int line_nr)		\
+{									\
+	struct pcentry *pce;						\
+	struct hwentry * hwe = VECTOR_LAST_SLOT(conf->hwtable);		\
+	if (!hwe || !hwe->pctable)					\
+		return 1;						\
+	pce = VECTOR_LAST_SLOT(hwe->pctable);				\
+	if (!pce)							\
+		return 1;						\
+	return function (strvec, &pce->option, file, line_nr);		\
+}
+
+#define declare_pc_snprint(option, function)				\
+static int								\
+snprint_pc_ ## option (struct config *conf, struct strbuf *buff,	\
+		       const void *data)				\
+{									\
+	const struct pcentry *pce  = (const struct pcentry *)data;	\
+	return function(buff, pce->option);				\
+}
+
 static int checkint_handler(struct config *conf, vector strvec,
 			    const char *file, int line_nr)
 {
@@ -1046,6 +1070,8 @@ declare_ovr_handler(fast_io_fail, set_undef_off_zero)
 declare_ovr_snprint(fast_io_fail, print_undef_off_zero)
 declare_hw_handler(fast_io_fail, set_undef_off_zero)
 declare_hw_snprint(fast_io_fail, print_undef_off_zero)
+declare_pc_handler(fast_io_fail, set_undef_off_zero)
+declare_pc_snprint(fast_io_fail, print_undef_off_zero)
 
 static int
 set_dev_loss(vector strvec, void *ptr, const char *file, int line_nr)
@@ -1083,6 +1109,8 @@ declare_ovr_handler(dev_loss, set_dev_loss)
 declare_ovr_snprint(dev_loss, print_dev_loss)
 declare_hw_handler(dev_loss, set_dev_loss)
 declare_hw_snprint(dev_loss, print_dev_loss)
+declare_pc_handler(dev_loss, set_dev_loss)
+declare_pc_snprint(dev_loss, print_dev_loss)
 
 declare_def_handler(eh_deadline, set_undef_off_zero)
 declare_def_snprint(eh_deadline, print_undef_off_zero)
@@ -1090,6 +1118,8 @@ declare_ovr_handler(eh_deadline, set_undef_off_zero)
 declare_ovr_snprint(eh_deadline, print_undef_off_zero)
 declare_hw_handler(eh_deadline, set_undef_off_zero)
 declare_hw_snprint(eh_deadline, print_undef_off_zero)
+declare_pc_handler(eh_deadline, set_undef_off_zero)
+declare_pc_snprint(eh_deadline, print_undef_off_zero)
 
 static int
 set_pgpolicy(vector strvec, void *ptr, const char *file, int line_nr)
@@ -1897,6 +1927,68 @@ declare_mp_snprint(wwid, print_str)
 declare_mp_handler(alias, set_str_noslash)
 declare_mp_snprint(alias, print_str)
 
+
+static int
+protocol_handler(struct config *conf, vector strvec, const char *file,
+               int line_nr)
+{
+	struct pcentry *pce;
+	struct hwentry *hwe = VECTOR_LAST_SLOT(conf->hwtable);
+	if (!hwe)
+		return 1;
+
+	if (!hwe->pctable && !(hwe->pctable = vector_alloc()))
+		return 1;
+
+	if (!(pce = alloc_pce()))
+		return 1;
+
+	if (!vector_alloc_slot(hwe->pctable)) {
+		free(pce);
+		return 1;
+	}
+	vector_set_slot(hwe->pctable, pce);
+
+	return 0;
+}
+
+static int
+set_protocol_type(vector strvec, void *ptr, const char *file, int line_nr)
+{
+	int *int_ptr = (int *)ptr;
+	char *buff;
+	int i;
+
+	buff = set_value(strvec);
+
+	if (!buff)
+		return 1;
+
+	for (i = 0; i <= LAST_BUS_PROTOCOL_ID; i++) {
+		if (protocol_name[i] && !strcmp(buff, protocol_name[i])) {
+			*int_ptr = i;
+			break;
+		}
+	}
+	if (i > LAST_BUS_PROTOCOL_ID)
+		condlog(1, "%s line %d, invalid value for type: \"%s\"",
+			file, line_nr, buff);
+
+	free(buff);
+	return 0;
+}
+
+static int
+print_protocol_type(struct strbuf *buff, int type)
+{
+	if (type < 0)
+		return 0;
+	return append_strbuf_quoted(buff, protocol_name[type]);
+}
+
+declare_pc_handler(type, set_protocol_type)
+declare_pc_snprint(type, print_protocol_type)
+
 /*
  * deprecated handlers
  */
@@ -2096,6 +2188,13 @@ init_keywords(vector keywords)
 	install_keyword("all_tg_pt", &hw_all_tg_pt_handler, &snprint_hw_all_tg_pt);
 	install_keyword("vpd_vendor", &hw_vpd_vendor_handler, &snprint_hw_vpd_vendor);
 	install_keyword("recheck_wwid", &hw_recheck_wwid_handler, &snprint_hw_recheck_wwid);
+	install_keyword_multi("protocol", &protocol_handler, NULL);
+	install_sublevel();
+	install_keyword("type", &pc_type_handler, &snprint_pc_type);
+	install_keyword("fast_io_fail_tmo", &pc_fast_io_fail_handler, &snprint_pc_fast_io_fail);
+	install_keyword("dev_loss_tmo", &pc_dev_loss_handler, &snprint_pc_dev_loss);
+	install_keyword("eh_deadline", &pc_eh_deadline_handler, &snprint_pc_eh_deadline);
+	install_sublevel_end();
 	install_sublevel_end();
 
 	install_keyword_root("overrides", &overrides_handler);
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 27c2cf1a..46d231ed 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -1406,6 +1406,45 @@ int snprint_multipath_topology_json (struct strbuf *buff,
 	return get_strbuf_len(buff) - initial_len;
 }
 
+static int
+snprint_pcentry (struct strbuf *buff, const struct pcentry *pce,
+		 const struct keyword *rootkw)
+{
+	int i, rc;
+	struct keyword *kw;
+	size_t initial_len = get_strbuf_len(buff);
+
+	if ((rc = append_strbuf_str(buff, "\t\tprotocol {\n")) < 0)
+		return rc;
+
+	iterate_sub_keywords(rootkw, kw, i) {
+		if ((rc = snprint_keyword(buff, "\t\t\t%k %v\n", kw, pce)) < 0)
+			return rc;
+	}
+
+	if ((rc = append_strbuf_str(buff, "\t\t}\n")) < 0)
+		return rc;
+	return get_strbuf_len(buff) - initial_len;
+}
+
+static int
+snprint_pctable (const struct config *conf, struct strbuf *buff,
+		 const struct _vector *pctable, const struct keyword *rootkw)
+{
+	int i, rc;
+	struct pcentry *pce;
+	size_t initial_len = get_strbuf_len(buff);
+
+	rootkw = find_keyword(conf->keywords, rootkw->sub, "protocol");
+	assert(rootkw);
+
+	vector_foreach_slot(pctable, pce, i) {
+		if ((rc = snprint_pcentry(buff, pce, rootkw)) < 0)
+			return rc;
+	}
+	return get_strbuf_len(buff) - initial_len;
+}
+
 static int
 snprint_hwentry (const struct config *conf,
 		 struct strbuf *buff, const struct hwentry * hwe)
@@ -1427,6 +1466,11 @@ snprint_hwentry (const struct config *conf,
 		if ((rc = snprint_keyword(buff, "\t\t%k %v\n", kw, hwe)) < 0)
 			return rc;
 	}
+
+	if (hwe->pctable &&
+	    (rc = snprint_pctable(conf, buff, hwe->pctable, rootkw)) < 0)
+		return rc;
+
 	if ((rc = append_strbuf_str(buff, "\t}\n")) < 0)
 		return rc;
 	return get_strbuf_len(buff) - initial_len;
-- 
2.17.2

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

[dm-devel] [PATCH 4/9] libmultipath: Set the scsi timeout parameters by path

[Benjamin Marzinski]

Instead of dev_loss, fast_io_fail, and eh_deadline belonging to the
multipath structure, have them belong to the path structure. This means
that they are selected per path, and that sysfs_set_scsi_tmo() doesn't
assume that all paths of a multipath device will have the same value.
Currently they will all be the same, but a future patch will make it
possible for paths to have different values based on their protocol.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c |   5 +-
 libmultipath/discovery.c | 174 ++++++++++++++++++++++-----------------
 libmultipath/discovery.h |   2 +-
 libmultipath/propsel.c   |  42 +++++-----
 libmultipath/propsel.h   |   6 +-
 libmultipath/structs.c   |   1 -
 libmultipath/structs.h   |   6 +-
 7 files changed, 127 insertions(+), 109 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index eca11ba0..09ae708d 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -329,9 +329,6 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
 	select_mode(conf, mpp);
 	select_uid(conf, mpp);
 	select_gid(conf, mpp);
-	select_fast_io_fail(conf, mpp);
-	select_dev_loss(conf, mpp);
-	select_eh_deadline(conf, mpp);
 	select_reservation_key(conf, mpp);
 	select_deferred_remove(conf, mpp);
 	select_marginal_path_err_sample_time(conf, mpp);
@@ -347,7 +344,7 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
 	select_ghost_delay(conf, mpp);
 	select_flush_on_last_del(conf, mpp);
 
-	sysfs_set_scsi_tmo(mpp, conf->checkint);
+	sysfs_set_scsi_tmo(conf, mpp);
 	marginal_pathgroups = conf->marginal_pathgroups;
 	pthread_cleanup_pop(1);
 
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index b969fba1..c6ba1967 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -598,13 +598,13 @@ sysfs_get_asymmetric_access_state(struct path *pp, char *buff, int buflen)
 }
 
 static int
-sysfs_set_eh_deadline(struct multipath *mpp, struct path *pp)
+sysfs_set_eh_deadline(struct path *pp)
 {
 	struct udev_device *hostdev;
 	char host_name[HOST_NAME_LEN], value[16];
 	int ret, len;
 
-	if (mpp->eh_deadline == EH_DEADLINE_UNSET)
+	if (pp->eh_deadline == EH_DEADLINE_UNSET)
 		return 0;
 
 	sprintf(host_name, "host%d", pp->sg_id.host_no);
@@ -613,12 +613,12 @@ sysfs_set_eh_deadline(struct multipath *mpp, struct path *pp)
 	if (!hostdev)
 		return 1;
 
-	if (mpp->eh_deadline == EH_DEADLINE_OFF)
+	if (pp->eh_deadline == EH_DEADLINE_OFF)
 		len = sprintf(value, "off");
-	else if (mpp->eh_deadline == EH_DEADLINE_ZERO)
+	else if (pp->eh_deadline == EH_DEADLINE_ZERO)
 		len = sprintf(value, "0");
 	else
-		len = sprintf(value, "%d", mpp->eh_deadline);
+		len = sprintf(value, "%d", pp->eh_deadline);
 
 	ret = sysfs_attr_set_value(hostdev, "eh_deadline",
 				   value, len + 1);
@@ -642,8 +642,8 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp)
 	unsigned int tmo;
 	int ret;
 
-	if (mpp->dev_loss == DEV_LOSS_TMO_UNSET &&
-	    mpp->fast_io_fail == MP_FAST_IO_FAIL_UNSET)
+	if (pp->dev_loss == DEV_LOSS_TMO_UNSET &&
+	    pp->fast_io_fail == MP_FAST_IO_FAIL_UNSET)
 		return;
 
 	sprintf(rport_id, "rport-%d:%d-%d",
@@ -685,14 +685,14 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp)
 	 * then set fast_io_fail, and _then_ set dev_loss_tmo
 	 * to the correct value.
 	 */
-	if (mpp->fast_io_fail != MP_FAST_IO_FAIL_UNSET &&
-	    mpp->fast_io_fail != MP_FAST_IO_FAIL_ZERO &&
-	    mpp->fast_io_fail != MP_FAST_IO_FAIL_OFF) {
+	if (pp->fast_io_fail != MP_FAST_IO_FAIL_UNSET &&
+	    pp->fast_io_fail != MP_FAST_IO_FAIL_ZERO &&
+	    pp->fast_io_fail != MP_FAST_IO_FAIL_OFF) {
 		/* Check if we need to temporarily increase dev_loss_tmo */
-		if ((unsigned int)mpp->fast_io_fail >= tmo) {
+		if ((unsigned int)pp->fast_io_fail >= tmo) {
 			/* Increase dev_loss_tmo temporarily */
 			snprintf(value, sizeof(value), "%u",
-				 (unsigned int)mpp->fast_io_fail + 1);
+				 (unsigned int)pp->fast_io_fail + 1);
 			ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo",
 						   value, strlen(value));
 			if (ret <= 0) {
@@ -706,20 +706,20 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp)
 				goto out;
 			}
 		}
-	} else if (mpp->dev_loss > DEFAULT_DEV_LOSS_TMO &&
-		mpp->no_path_retry != NO_PATH_RETRY_QUEUE) {
+	} else if (pp->dev_loss > DEFAULT_DEV_LOSS_TMO &&
+		   mpp->no_path_retry != NO_PATH_RETRY_QUEUE) {
 		condlog(2, "%s: limiting dev_loss_tmo to %d, since "
 			"fast_io_fail is not set",
 			rport_id, DEFAULT_DEV_LOSS_TMO);
-		mpp->dev_loss = DEFAULT_DEV_LOSS_TMO;
+		pp->dev_loss = DEFAULT_DEV_LOSS_TMO;
 	}
-	if (mpp->fast_io_fail != MP_FAST_IO_FAIL_UNSET) {
-		if (mpp->fast_io_fail == MP_FAST_IO_FAIL_OFF)
+	if (pp->fast_io_fail != MP_FAST_IO_FAIL_UNSET) {
+		if (pp->fast_io_fail == MP_FAST_IO_FAIL_OFF)
 			sprintf(value, "off");
-		else if (mpp->fast_io_fail == MP_FAST_IO_FAIL_ZERO)
+		else if (pp->fast_io_fail == MP_FAST_IO_FAIL_ZERO)
 			sprintf(value, "0");
 		else
-			snprintf(value, 16, "%u", mpp->fast_io_fail);
+			snprintf(value, 16, "%u", pp->fast_io_fail);
 		ret = sysfs_attr_set_value(rport_dev, "fast_io_fail_tmo",
 					   value, strlen(value));
 		if (ret <= 0) {
@@ -730,8 +730,8 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp)
 					rport_id, value, -ret);
 		}
 	}
-	if (mpp->dev_loss != DEV_LOSS_TMO_UNSET) {
-		snprintf(value, 16, "%u", mpp->dev_loss);
+	if (pp->dev_loss != DEV_LOSS_TMO_UNSET) {
+		snprintf(value, 16, "%u", pp->dev_loss);
 		ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo",
 					   value, strlen(value));
 		if (ret <= 0) {
@@ -747,15 +747,15 @@ out:
 }
 
 static void
-sysfs_set_session_tmo(struct multipath *mpp, struct path *pp)
+sysfs_set_session_tmo(struct path *pp)
 {
 	struct udev_device *session_dev = NULL;
 	char session_id[64];
 	char value[11];
 
-	if (mpp->dev_loss != DEV_LOSS_TMO_UNSET)
+	if (pp->dev_loss != DEV_LOSS_TMO_UNSET)
 		condlog(3, "%s: ignoring dev_loss_tmo on iSCSI", pp->dev);
-	if (mpp->fast_io_fail == MP_FAST_IO_FAIL_UNSET)
+	if (pp->fast_io_fail == MP_FAST_IO_FAIL_UNSET)
 		return;
 
 	sprintf(session_id, "session%d", pp->sg_id.transport_id);
@@ -769,15 +769,15 @@ sysfs_set_session_tmo(struct multipath *mpp, struct path *pp)
 	condlog(4, "target%d:%d:%d -> %s", pp->sg_id.host_no,
 		pp->sg_id.channel, pp->sg_id.scsi_id, session_id);
 
-	if (mpp->fast_io_fail != MP_FAST_IO_FAIL_UNSET) {
-		if (mpp->fast_io_fail == MP_FAST_IO_FAIL_OFF) {
+	if (pp->fast_io_fail != MP_FAST_IO_FAIL_UNSET) {
+		if (pp->fast_io_fail == MP_FAST_IO_FAIL_OFF) {
 			condlog(3, "%s: can't switch off fast_io_fail_tmo "
 				"on iSCSI", pp->dev);
-		} else if (mpp->fast_io_fail == MP_FAST_IO_FAIL_ZERO) {
+		} else if (pp->fast_io_fail == MP_FAST_IO_FAIL_ZERO) {
 			condlog(3, "%s: can't set fast_io_fail_tmo to '0'"
 				"on iSCSI", pp->dev);
 		} else {
-			snprintf(value, 11, "%u", mpp->fast_io_fail);
+			snprintf(value, 11, "%u", pp->fast_io_fail);
 			if (sysfs_attr_set_value(session_dev, "recovery_tmo",
 						 value, strlen(value)) <= 0) {
 				condlog(3, "%s: Failed to set recovery_tmo, "
@@ -790,14 +790,14 @@ sysfs_set_session_tmo(struct multipath *mpp, struct path *pp)
 }
 
 static void
-sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
+sysfs_set_nexus_loss_tmo(struct path *pp)
 {
 	struct udev_device *parent, *sas_dev = NULL;
 	const char *end_dev_id = NULL;
 	char value[11];
 	static const char ed_str[] = "end_device-";
 
-	if (!pp->udev || mpp->dev_loss == DEV_LOSS_TMO_UNSET)
+	if (!pp->udev || pp->dev_loss == DEV_LOSS_TMO_UNSET)
 		return;
 
 	for (parent = udev_device_get_parent(pp->udev);
@@ -824,8 +824,8 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
 	condlog(4, "target%d:%d:%d -> %s", pp->sg_id.host_no,
 		pp->sg_id.channel, pp->sg_id.scsi_id, end_dev_id);
 
-	if (mpp->dev_loss != DEV_LOSS_TMO_UNSET) {
-		snprintf(value, 11, "%u", mpp->dev_loss);
+	if (pp->dev_loss != DEV_LOSS_TMO_UNSET) {
+		snprintf(value, 11, "%u", pp->dev_loss);
 		if (sysfs_attr_set_value(sas_dev, "I_T_nexus_loss_timeout",
 					 value, strlen(value)) <= 0)
 			condlog(3, "%s: failed to update "
@@ -836,76 +836,98 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
 	return;
 }
 
+static void
+scsi_tmo_error_msg(struct path *pp)
+{
+	STATIC_BITFIELD(bf, LAST_BUS_PROTOCOL_ID + 1);
+	STRBUF_ON_STACK(proto_buf);
+	unsigned int proto_id = bus_protocol_id(pp);
+
+	snprint_path_protocol(&proto_buf, pp);
+	condlog(2, "%s: setting scsi timeouts is unsupported for protocol %s",
+		pp->dev, get_strbuf_str(&proto_buf));
+	set_bit_in_bitfield(proto_id, bf);
+}
+
 int
-sysfs_set_scsi_tmo (struct multipath *mpp, unsigned int checkint)
+sysfs_set_scsi_tmo (struct config *conf, struct multipath *mpp)
 {
 	struct path *pp;
 	int i;
-	unsigned int dev_loss_tmo = mpp->dev_loss;
-	struct path *err_path = NULL;
-	STATIC_BITFIELD(bf, LAST_BUS_PROTOCOL_ID + 1);
+	unsigned int min_dev_loss = 0;
+	bool warn_dev_loss = false;
+	bool warn_fast_io_fail = false;
 
 	if (mpp->no_path_retry > 0) {
 		uint64_t no_path_retry_tmo =
-			(uint64_t)mpp->no_path_retry * checkint;
+			(uint64_t)mpp->no_path_retry * conf->checkint;
 
 		if (no_path_retry_tmo > MAX_DEV_LOSS_TMO)
-			no_path_retry_tmo = MAX_DEV_LOSS_TMO;
-		if (no_path_retry_tmo > dev_loss_tmo)
-			dev_loss_tmo = no_path_retry_tmo;
-	} else if (mpp->no_path_retry == NO_PATH_RETRY_QUEUE) {
-		dev_loss_tmo = MAX_DEV_LOSS_TMO;
-	}
-	if (mpp->dev_loss != DEV_LOSS_TMO_UNSET &&
-	    mpp->dev_loss != dev_loss_tmo) {
-		condlog(2, "%s: Using dev_loss_tmo=%u instead of %u because of no_path_retry setting",
-			mpp->alias, dev_loss_tmo, mpp->dev_loss);
-		mpp->dev_loss = dev_loss_tmo;
-	}
-	if (mpp->dev_loss != DEV_LOSS_TMO_UNSET &&
-	    mpp->fast_io_fail != MP_FAST_IO_FAIL_UNSET &&
-	    (unsigned int)mpp->fast_io_fail >= mpp->dev_loss) {
-		condlog(3, "%s: turning off fast_io_fail (%d is not smaller than dev_loss_tmo)",
-			mpp->alias, mpp->fast_io_fail);
-		mpp->fast_io_fail = MP_FAST_IO_FAIL_OFF;
-	}
-	if (mpp->dev_loss == DEV_LOSS_TMO_UNSET &&
-	    mpp->fast_io_fail == MP_FAST_IO_FAIL_UNSET &&
-	    mpp->eh_deadline == EH_DEADLINE_UNSET)
-		return 0;
+			min_dev_loss = MAX_DEV_LOSS_TMO;
+		else
+			min_dev_loss = no_path_retry_tmo;
+	} else if (mpp->no_path_retry == NO_PATH_RETRY_QUEUE)
+		min_dev_loss = MAX_DEV_LOSS_TMO;
 
 	vector_foreach_slot(mpp->paths, pp, i) {
+		select_fast_io_fail(conf, pp);
+		select_dev_loss(conf, pp);
+		select_eh_deadline(conf, pp);
+
+		if (pp->dev_loss == DEV_LOSS_TMO_UNSET &&
+		    pp->fast_io_fail == MP_FAST_IO_FAIL_UNSET &&
+		    pp->eh_deadline == EH_DEADLINE_UNSET)
+			continue;
+
 		if (pp->bus != SYSFS_BUS_SCSI) {
-			if (!err_path)
-				err_path = pp;
+			scsi_tmo_error_msg(pp);
 			continue;
 		}
+		sysfs_set_eh_deadline(pp);
+
+		if (pp->dev_loss == DEV_LOSS_TMO_UNSET &&
+		    pp->fast_io_fail == MP_FAST_IO_FAIL_UNSET)
+			continue;
+
+		if (pp->sg_id.proto_id != SCSI_PROTOCOL_FCP &&
+		    pp->sg_id.proto_id != SCSI_PROTOCOL_ISCSI &&
+		    pp->sg_id.proto_id != SCSI_PROTOCOL_SAS) {
+			scsi_tmo_error_msg(pp);
+			continue;
+		}
+
+		if (pp->dev_loss != DEV_LOSS_TMO_UNSET &&
+		    pp->dev_loss < min_dev_loss) {
+			warn_dev_loss = true;
+			pp->dev_loss = min_dev_loss;
+		}
+		if (pp->dev_loss != DEV_LOSS_TMO_UNSET &&
+		    pp->fast_io_fail > 0 &&
+		    (unsigned int)pp->fast_io_fail >= pp->dev_loss) {
+			warn_fast_io_fail = true;
+			pp->fast_io_fail = MP_FAST_IO_FAIL_OFF;
+		}
 
 		switch (pp->sg_id.proto_id) {
 		case SCSI_PROTOCOL_FCP:
 			sysfs_set_rport_tmo(mpp, pp);
 			break;
 		case SCSI_PROTOCOL_ISCSI:
-			sysfs_set_session_tmo(mpp, pp);
+			sysfs_set_session_tmo(pp);
 			break;
 		case SCSI_PROTOCOL_SAS:
-			sysfs_set_nexus_loss_tmo(mpp, pp);
+			sysfs_set_nexus_loss_tmo(pp);
 			break;
 		default:
-			if (!err_path)
-				err_path = pp;
+			break;
 		}
-		sysfs_set_eh_deadline(mpp, pp);
-	}
-
-	if (err_path && !is_bit_set_in_bitfield(bus_protocol_id(pp), bf)) {
-		STRBUF_ON_STACK(proto_buf);
-
-		snprint_path_protocol(&proto_buf, err_path);
-		condlog(2, "%s: setting dev_loss_tmo is unsupported for protocol %s",
-			mpp->alias, get_strbuf_str(&proto_buf));
-		set_bit_in_bitfield(bus_protocol_id(pp), bf);
 	}
+	if (warn_dev_loss)
+		condlog(2, "%s: Raising dev_loss_tmo to %u because of no_path_retry setting",
+			mpp->alias, min_dev_loss);
+	if (warn_fast_io_fail)
+		condlog(3, "%s: turning off fast_io_fail (not smaller than dev_loss_tmo)",
+			mpp->alias);
 	return 0;
 }
 
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index 466af345..acd51792 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -42,7 +42,7 @@ int alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,
 int store_pathinfo (vector pathvec, struct config *conf,
 		    struct udev_device *udevice, int flag,
 		    struct path **pp_ptr);
-int sysfs_set_scsi_tmo (struct multipath *mpp, unsigned int checkint);
+int sysfs_set_scsi_tmo (struct config *conf, struct multipath *mpp);
 int sysfs_get_timeout(const struct path *pp, unsigned int *timeout);
 int sysfs_get_iscsi_ip_address(const struct path *pp, char *ip_address);
 int sysfs_get_host_adapter_name(const struct path *pp,
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 1419ec6f..d2d70090 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -769,53 +769,53 @@ int select_minio(struct config *conf, struct multipath *mp)
 		return select_minio_bio(conf, mp);
 }
 
-int select_fast_io_fail(struct config *conf, struct multipath *mp)
+int select_fast_io_fail(struct config *conf, struct path *pp)
 {
 	const char *origin;
 	STRBUF_ON_STACK(buff);
 
-	mp_set_ovr(fast_io_fail);
-	mp_set_hwe(fast_io_fail);
-	mp_set_conf(fast_io_fail);
-	mp_set_default(fast_io_fail, DEFAULT_FAST_IO_FAIL);
+	pp_set_ovr(fast_io_fail);
+	pp_set_hwe(fast_io_fail);
+	pp_set_conf(fast_io_fail);
+	pp_set_default(fast_io_fail, DEFAULT_FAST_IO_FAIL);
 out:
-	print_undef_off_zero(&buff, mp->fast_io_fail);
-	condlog(3, "%s: fast_io_fail_tmo = %s %s", mp->alias,
+	print_undef_off_zero(&buff, pp->fast_io_fail);
+	condlog(3, "%s: fast_io_fail_tmo = %s %s", pp->dev,
 		get_strbuf_str(&buff), origin);
 	return 0;
 }
 
-int select_dev_loss(struct config *conf, struct multipath *mp)
+int select_dev_loss(struct config *conf, struct path *pp)
 {
 	const char *origin;
 	STRBUF_ON_STACK(buff);
 
-	mp_set_ovr(dev_loss);
-	mp_set_hwe(dev_loss);
-	mp_set_conf(dev_loss);
-	mp->dev_loss = DEV_LOSS_TMO_UNSET;
+	pp_set_ovr(dev_loss);
+	pp_set_hwe(dev_loss);
+	pp_set_conf(dev_loss);
+	pp->dev_loss = DEV_LOSS_TMO_UNSET;
 	return 0;
 out:
-	print_dev_loss(&buff, mp->dev_loss);
-	condlog(3, "%s: dev_loss_tmo = %s %s", mp->alias,
+	print_dev_loss(&buff, pp->dev_loss);
+	condlog(3, "%s: dev_loss_tmo = %s %s", pp->dev,
 		get_strbuf_str(&buff), origin);
 	return 0;
 }
 
-int select_eh_deadline(struct config *conf, struct multipath *mp)
+int select_eh_deadline(struct config *conf, struct path *pp)
 {
 	const char *origin;
 	STRBUF_ON_STACK(buff);
 
-	mp_set_ovr(eh_deadline);
-	mp_set_hwe(eh_deadline);
-	mp_set_conf(eh_deadline);
-	mp->eh_deadline = EH_DEADLINE_UNSET;
+	pp_set_ovr(eh_deadline);
+	pp_set_hwe(eh_deadline);
+	pp_set_conf(eh_deadline);
+	pp->eh_deadline = EH_DEADLINE_UNSET;
 	/* not changing sysfs in default cause, so don't print anything */
 	return 0;
 out:
-	print_undef_off_zero(&buff, mp->eh_deadline);
-	condlog(3, "%s: eh_deadline = %s %s", mp->alias,
+	print_undef_off_zero(&buff, pp->eh_deadline);
+	condlog(3, "%s: eh_deadline = %s %s", pp->dev,
 		get_strbuf_str(&buff), origin);
 	return 0;
 }
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index 72a7e33c..152ca44c 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -16,9 +16,9 @@ int select_minio(struct config *conf, struct multipath *mp);
 int select_mode(struct config *conf, struct multipath *mp);
 int select_uid(struct config *conf, struct multipath *mp);
 int select_gid(struct config *conf, struct multipath *mp);
-int select_fast_io_fail(struct config *conf, struct multipath *mp);
-int select_dev_loss(struct config *conf, struct multipath *mp);
-int select_eh_deadline(struct config *conf, struct multipath *mp);
+int select_fast_io_fail(struct config *conf, struct path *pp);
+int select_dev_loss(struct config *conf, struct path *pp);
+int select_eh_deadline(struct config *conf, struct path *pp);
 int select_reservation_key(struct config *conf, struct multipath *mp);
 int select_retain_hwhandler (struct config *conf, struct multipath * mp);
 int select_detect_prio(struct config *conf, struct path * pp);
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 04cfdcdc..2c9be041 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -246,7 +246,6 @@ alloc_multipath (void)
 		mpp->bestpg = 1;
 		mpp->mpcontext = NULL;
 		mpp->no_path_retry = NO_PATH_RETRY_UNDEF;
-		mpp->fast_io_fail = MP_FAST_IO_FAIL_UNSET;
 		dm_multipath_to_gen(mpp)->ops = &dm_gen_multipath_ops;
 	}
 	return mpp;
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 3722e31b..a6749367 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -349,6 +349,9 @@ struct path {
 	int marginal;
 	int vpd_vendor_id;
 	int recheck_wwid;
+	int fast_io_fail;
+	unsigned int dev_loss;
+	int eh_deadline;
 	/* configlet pointers */
 	vector hwe;
 	struct gen_path generic_path;
@@ -376,7 +379,6 @@ struct multipath {
 	int minio;
 	int flush_on_last_del;
 	int attribute_flags;
-	int fast_io_fail;
 	int retain_hwhandler;
 	int deferred_remove;
 	bool in_recovery;
@@ -395,8 +397,6 @@ struct multipath {
 	int needs_paths_uevent;
 	int ghost_delay;
 	int ghost_delay_tick;
-	unsigned int dev_loss;
-	int eh_deadline;
 	uid_t uid;
 	gid_t gid;
 	mode_t mode;
-- 
2.17.2

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

[dm-devel] [PATCH 5/9] libmultipath: check the hwentry pctable for path variables

[Benjamin Marzinski]

For the config values that exist in the proctol subsection of the device
configs, paths will now also check the pctable when checking a hwentry
for a value. Values in a matching pcentry will be used in preference to
values in that hwentry.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/propsel.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index d2d70090..762b7fcb 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -73,6 +73,8 @@ do {									\
 static const char default_origin[] = "(setting: multipath internal)";
 static const char hwe_origin[] =
 	"(setting: storage device configuration)";
+static const char hwe_pce_origin[] =
+	"(setting: storage device procotol section)";
 static const char multipaths_origin[] =
 	"(setting: multipath.conf multipaths section)";
 static const char conf_origin[] =
@@ -146,6 +148,28 @@ do {									\
 	}								\
 } while (0)
 
+#define pp_set_hwe_pce(var)						\
+do {									\
+	struct hwentry *_hwe;						\
+	struct pcentry *_pce;						\
+	int _i, _j;							\
+									\
+	vector_foreach_slot(pp->hwe, _hwe, _i) {			\
+		vector_foreach_slot(_hwe->pctable, _pce, _j) {		\
+			if (_pce->type == (int)bus_protocol_id(pp) && _pce->var) {	\
+				pp->var = _pce->var;			\
+				origin = hwe_pce_origin;		\
+				goto out;				\
+			}						\
+		}							\
+		if (_hwe->var) {					\
+			pp->var = _hwe->var;				\
+			origin = hwe_origin;				\
+			goto out;					\
+		}							\
+	}								\
+} while (0)
+
 int select_mode(struct config *conf, struct multipath *mp)
 {
 	const char *origin;
@@ -775,7 +799,7 @@ int select_fast_io_fail(struct config *conf, struct path *pp)
 	STRBUF_ON_STACK(buff);
 
 	pp_set_ovr(fast_io_fail);
-	pp_set_hwe(fast_io_fail);
+	pp_set_hwe_pce(fast_io_fail);
 	pp_set_conf(fast_io_fail);
 	pp_set_default(fast_io_fail, DEFAULT_FAST_IO_FAIL);
 out:
@@ -791,7 +815,7 @@ int select_dev_loss(struct config *conf, struct path *pp)
 	STRBUF_ON_STACK(buff);
 
 	pp_set_ovr(dev_loss);
-	pp_set_hwe(dev_loss);
+	pp_set_hwe_pce(dev_loss);
 	pp_set_conf(dev_loss);
 	pp->dev_loss = DEV_LOSS_TMO_UNSET;
 	return 0;
@@ -808,7 +832,7 @@ int select_eh_deadline(struct config *conf, struct path *pp)
 	STRBUF_ON_STACK(buff);
 
 	pp_set_ovr(eh_deadline);
-	pp_set_hwe(eh_deadline);
+	pp_set_hwe_pce(eh_deadline);
 	pp_set_conf(eh_deadline);
 	pp->eh_deadline = EH_DEADLINE_UNSET;
 	/* not changing sysfs in default cause, so don't print anything */
-- 
2.17.2

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

[dm-devel] [PATCH 6/9] libmultipath: make snprint_pctable indent a variable amount

[Benjamin Marzinski]

Instead of always indenting two tabs, which is what is needed in for
the protocol subsection of the device subsection, indent a variable
amount. This will be needed in a future patch.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/print.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index 46d231ed..093e43aa 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -1407,28 +1407,42 @@ int snprint_multipath_topology_json (struct strbuf *buff,
 }
 
 static int
-snprint_pcentry (struct strbuf *buff, const struct pcentry *pce,
+snprint_pcentry (struct strbuf *buff, int indent, const struct pcentry *pce,
 		 const struct keyword *rootkw)
 {
 	int i, rc;
 	struct keyword *kw;
 	size_t initial_len = get_strbuf_len(buff);
+	STRBUF_ON_STACK(fmt);
 
-	if ((rc = append_strbuf_str(buff, "\t\tprotocol {\n")) < 0)
+	for (i = 0; i < indent; i++)
+		if ((rc = append_strbuf_str(&fmt, "\t")) < 0)
+			return rc;
+	if ((rc = append_strbuf_str(&fmt, "\t%k %v\n")) < 0)
+			return rc;
+
+	for (i = 0; i < indent; i++)
+		if ((rc = append_strbuf_str(buff, "\t")) < 0)
+			return rc;
+	if ((rc = append_strbuf_str(buff, "protocol {\n")) < 0)
 		return rc;
 
 	iterate_sub_keywords(rootkw, kw, i) {
-		if ((rc = snprint_keyword(buff, "\t\t\t%k %v\n", kw, pce)) < 0)
+		if ((rc = snprint_keyword(buff, get_strbuf_str(&fmt), kw,
+					  pce)) < 0)
 			return rc;
 	}
 
-	if ((rc = append_strbuf_str(buff, "\t\t}\n")) < 0)
+	for (i = 0; i < indent; i++)
+		if ((rc = append_strbuf_str(buff, "\t")) < 0)
+			return rc;
+	if ((rc = append_strbuf_str(buff, "}\n")) < 0)
 		return rc;
 	return get_strbuf_len(buff) - initial_len;
 }
 
 static int
-snprint_pctable (const struct config *conf, struct strbuf *buff,
+snprint_pctable (const struct config *conf, struct strbuf *buff, int indent,
 		 const struct _vector *pctable, const struct keyword *rootkw)
 {
 	int i, rc;
@@ -1439,7 +1453,7 @@ snprint_pctable (const struct config *conf, struct strbuf *buff,
 	assert(rootkw);
 
 	vector_foreach_slot(pctable, pce, i) {
-		if ((rc = snprint_pcentry(buff, pce, rootkw)) < 0)
+		if ((rc = snprint_pcentry(buff, indent, pce, rootkw)) < 0)
 			return rc;
 	}
 	return get_strbuf_len(buff) - initial_len;
@@ -1468,7 +1482,7 @@ snprint_hwentry (const struct config *conf,
 	}
 
 	if (hwe->pctable &&
-	    (rc = snprint_pctable(conf, buff, hwe->pctable, rootkw)) < 0)
+	    (rc = snprint_pctable(conf, buff, 2, hwe->pctable, rootkw)) < 0)
 		return rc;
 
 	if ((rc = append_strbuf_str(buff, "\t}\n")) < 0)
-- 
2.17.2

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

[dm-devel] [PATCH 7/9] libmultipath: add procotol subsection to overrides

[Benjamin Marzinski]

Add a procotol subsection to the overrides section, just like the one in
the device subsection. This allows users to a protocol specific
parameters to all device configurations.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/dict.c    | 76 ++++++++++++++++++++++++++++++++----------
 libmultipath/print.c   |  5 +++
 libmultipath/propsel.c | 29 ++++++++++++++--
 3 files changed, 90 insertions(+), 20 deletions(-)

diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 04d86ee3..4923b8d2 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -386,6 +386,20 @@ snprint_ovr_ ## option (struct config *conf, struct strbuf *buff,	\
 	return function (buff, conf->overrides->option);		\
 }
 
+#define declare_ovr_pc_handler(option, function)			\
+static int								\
+ovr_pc_ ## option ## _handler (struct config *conf, vector strvec,	\
+			       const char *file, int line_nr)		\
+{									\
+	struct pcentry *pce;						\
+	if (!conf->overrides || !conf->overrides->pctable)		\
+		return 1;						\
+	pce = VECTOR_LAST_SLOT(conf->overrides->pctable);		\
+	if (!pce)							\
+		return 1;						\
+	return function (strvec, &pce->option, file, line_nr);		\
+}
+
 #define declare_mp_handler(option, function)				\
 static int								\
 mp_ ## option ## _handler (struct config *conf, vector strvec,		\
@@ -417,10 +431,10 @@ snprint_mp_ ## option (struct config *conf, struct strbuf *buff,	\
 	return function(buff, mpe->option);				\
 }
 
-#define declare_pc_handler(option, function)				\
+#define declare_hw_pc_handler(option, function)				\
 static int								\
-pc_ ## option ## _handler (struct config *conf, vector strvec,		\
-			   const char *file, int line_nr)		\
+hw_pc_ ## option ## _handler (struct config *conf, vector strvec,	\
+			       const char *file, int line_nr)		\
 {									\
 	struct pcentry *pce;						\
 	struct hwentry * hwe = VECTOR_LAST_SLOT(conf->hwtable);		\
@@ -1070,7 +1084,8 @@ declare_ovr_handler(fast_io_fail, set_undef_off_zero)
 declare_ovr_snprint(fast_io_fail, print_undef_off_zero)
 declare_hw_handler(fast_io_fail, set_undef_off_zero)
 declare_hw_snprint(fast_io_fail, print_undef_off_zero)
-declare_pc_handler(fast_io_fail, set_undef_off_zero)
+declare_hw_pc_handler(fast_io_fail, set_undef_off_zero)
+declare_ovr_pc_handler(fast_io_fail, set_undef_off_zero)
 declare_pc_snprint(fast_io_fail, print_undef_off_zero)
 
 static int
@@ -1109,7 +1124,8 @@ declare_ovr_handler(dev_loss, set_dev_loss)
 declare_ovr_snprint(dev_loss, print_dev_loss)
 declare_hw_handler(dev_loss, set_dev_loss)
 declare_hw_snprint(dev_loss, print_dev_loss)
-declare_pc_handler(dev_loss, set_dev_loss)
+declare_hw_pc_handler(dev_loss, set_dev_loss)
+declare_ovr_pc_handler(dev_loss, set_dev_loss)
 declare_pc_snprint(dev_loss, print_dev_loss)
 
 declare_def_handler(eh_deadline, set_undef_off_zero)
@@ -1118,7 +1134,8 @@ declare_ovr_handler(eh_deadline, set_undef_off_zero)
 declare_ovr_snprint(eh_deadline, print_undef_off_zero)
 declare_hw_handler(eh_deadline, set_undef_off_zero)
 declare_hw_snprint(eh_deadline, print_undef_off_zero)
-declare_pc_handler(eh_deadline, set_undef_off_zero)
+declare_hw_pc_handler(eh_deadline, set_undef_off_zero)
+declare_ovr_pc_handler(eh_deadline, set_undef_off_zero)
 declare_pc_snprint(eh_deadline, print_undef_off_zero)
 
 static int
@@ -1929,13 +1946,9 @@ declare_mp_snprint(alias, print_str)
 
 
 static int
-protocol_handler(struct config *conf, vector strvec, const char *file,
-               int line_nr)
+_protocol_handler(struct hwentry *hwe)
 {
 	struct pcentry *pce;
-	struct hwentry *hwe = VECTOR_LAST_SLOT(conf->hwtable);
-	if (!hwe)
-		return 1;
 
 	if (!hwe->pctable && !(hwe->pctable = vector_alloc()))
 		return 1;
@@ -1952,6 +1965,27 @@ protocol_handler(struct config *conf, vector strvec, const char *file,
 	return 0;
 }
 
+static int
+hw_protocol_handler(struct config *conf, vector strvec, const char *file,
+		    int line_nr)
+{
+	struct hwentry *hwe = VECTOR_LAST_SLOT(conf->hwtable);
+	if (!hwe)
+		return 1;
+
+	return _protocol_handler(hwe);
+}
+
+static int
+ovr_protocol_handler(struct config *conf, vector strvec, const char *file,
+		     int line_nr)
+{
+	if (!conf->overrides)
+		return 1;
+
+	return _protocol_handler(conf->overrides);
+}
+
 static int
 set_protocol_type(vector strvec, void *ptr, const char *file, int line_nr)
 {
@@ -1986,7 +2020,8 @@ print_protocol_type(struct strbuf *buff, int type)
 	return append_strbuf_quoted(buff, protocol_name[type]);
 }
 
-declare_pc_handler(type, set_protocol_type)
+declare_hw_pc_handler(type, set_protocol_type)
+declare_ovr_pc_handler(type, set_protocol_type)
 declare_pc_snprint(type, print_protocol_type)
 
 /*
@@ -2188,12 +2223,12 @@ init_keywords(vector keywords)
 	install_keyword("all_tg_pt", &hw_all_tg_pt_handler, &snprint_hw_all_tg_pt);
 	install_keyword("vpd_vendor", &hw_vpd_vendor_handler, &snprint_hw_vpd_vendor);
 	install_keyword("recheck_wwid", &hw_recheck_wwid_handler, &snprint_hw_recheck_wwid);
-	install_keyword_multi("protocol", &protocol_handler, NULL);
+	install_keyword_multi("protocol", &hw_protocol_handler, NULL);
 	install_sublevel();
-	install_keyword("type", &pc_type_handler, &snprint_pc_type);
-	install_keyword("fast_io_fail_tmo", &pc_fast_io_fail_handler, &snprint_pc_fast_io_fail);
-	install_keyword("dev_loss_tmo", &pc_dev_loss_handler, &snprint_pc_dev_loss);
-	install_keyword("eh_deadline", &pc_eh_deadline_handler, &snprint_pc_eh_deadline);
+	install_keyword("type", &hw_pc_type_handler, &snprint_pc_type);
+	install_keyword("fast_io_fail_tmo", &hw_pc_fast_io_fail_handler, &snprint_pc_fast_io_fail);
+	install_keyword("dev_loss_tmo", &hw_pc_dev_loss_handler, &snprint_pc_dev_loss);
+	install_keyword("eh_deadline", &hw_pc_eh_deadline_handler, &snprint_pc_eh_deadline);
 	install_sublevel_end();
 	install_sublevel_end();
 
@@ -2237,6 +2272,13 @@ init_keywords(vector keywords)
 	install_keyword("ghost_delay", &ovr_ghost_delay_handler, &snprint_ovr_ghost_delay);
 	install_keyword("all_tg_pt", &ovr_all_tg_pt_handler, &snprint_ovr_all_tg_pt);
 	install_keyword("recheck_wwid", &ovr_recheck_wwid_handler, &snprint_ovr_recheck_wwid);
+	install_keyword_multi("protocol", &ovr_protocol_handler, NULL);
+	install_sublevel();
+	install_keyword("type", &ovr_pc_type_handler, &snprint_pc_type);
+	install_keyword("fast_io_fail_tmo", &ovr_pc_fast_io_fail_handler, &snprint_pc_fast_io_fail);
+	install_keyword("dev_loss_tmo", &ovr_pc_dev_loss_handler, &snprint_pc_dev_loss);
+	install_keyword("eh_deadline", &ovr_pc_eh_deadline_handler, &snprint_pc_eh_deadline);
+	install_sublevel_end();
 
 	install_keyword_root("multipaths", &multipaths_handler);
 	install_keyword_multi("multipath", &multipath_handler, NULL);
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 093e43aa..93df0b0c 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -1618,6 +1618,11 @@ static int snprint_overrides(const struct config *conf, struct strbuf *buff,
 		if ((rc = snprint_keyword(buff, "\t%k %v\n", kw, NULL)) < 0)
 			return rc;
 	}
+
+	if (overrides->pctable &&
+	    (rc = snprint_pctable(conf, buff, 1, overrides->pctable,
+				   rootkw)) < 0)
+		return rc;
 out:
 	if ((rc = append_strbuf_str(buff, "}\n")) < 0)
 		return rc;
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 762b7fcb..90a160f1 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -81,6 +81,8 @@ static const char conf_origin[] =
 	"(setting: multipath.conf defaults/devices section)";
 static const char overrides_origin[] =
 	"(setting: multipath.conf overrides section)";
+static const char overrides_pce_origin[] =
+	"(setting: multipath.conf overrides protocol section)";
 static const char cmdline_origin[] =
 	"(setting: multipath command line [-p] flag)";
 static const char autodetect_origin[] =
@@ -170,6 +172,27 @@ do {									\
 	}								\
 } while (0)
 
+#define pp_set_ovr_pce(var)						\
+do {									\
+	struct pcentry *_pce;						\
+	int _i;								\
+									\
+	if (conf->overrides) {						\
+		vector_foreach_slot(conf->overrides->pctable, _pce, _i) {	\
+			if (_pce->type == (int)bus_protocol_id(pp) && _pce->var) {	\
+				pp->var = _pce->var;			\
+				origin = overrides_pce_origin;		\
+				goto out;				\
+			}						\
+		}							\
+		if (conf->overrides->var) {				\
+			pp->var = conf->overrides->var;			\
+			origin = overrides_origin;			\
+			goto out;					\
+		}							\
+	}								\
+} while (0)
+
 int select_mode(struct config *conf, struct multipath *mp)
 {
 	const char *origin;
@@ -798,7 +821,7 @@ int select_fast_io_fail(struct config *conf, struct path *pp)
 	const char *origin;
 	STRBUF_ON_STACK(buff);
 
-	pp_set_ovr(fast_io_fail);
+	pp_set_ovr_pce(fast_io_fail);
 	pp_set_hwe_pce(fast_io_fail);
 	pp_set_conf(fast_io_fail);
 	pp_set_default(fast_io_fail, DEFAULT_FAST_IO_FAIL);
@@ -814,7 +837,7 @@ int select_dev_loss(struct config *conf, struct path *pp)
 	const char *origin;
 	STRBUF_ON_STACK(buff);
 
-	pp_set_ovr(dev_loss);
+	pp_set_ovr_pce(dev_loss);
 	pp_set_hwe_pce(dev_loss);
 	pp_set_conf(dev_loss);
 	pp->dev_loss = DEV_LOSS_TMO_UNSET;
@@ -831,7 +854,7 @@ int select_eh_deadline(struct config *conf, struct path *pp)
 	const char *origin;
 	STRBUF_ON_STACK(buff);
 
-	pp_set_ovr(eh_deadline);
+	pp_set_ovr_pce(eh_deadline);
 	pp_set_hwe_pce(eh_deadline);
 	pp_set_conf(eh_deadline);
 	pp->eh_deadline = EH_DEADLINE_UNSET;
-- 
2.17.2

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

[dm-devel] [PATCH 8/9] libmultipath: fix eh_deadline documentation

[Benjamin Marzinski]

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/multipath.conf.5 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 605b46e0..97695da4 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -1636,6 +1636,8 @@ section:
 .TP
 .B dev_loss_tmo
 .TP
+.B eh_deadline
+.TP
 .B flush_on_last_del
 .TP
 .B user_friendly_names
@@ -1722,6 +1724,8 @@ the values are taken from the \fIdevices\fR or \fIdefaults\fR sections:
 .TP
 .B dev_loss_tmo
 .TP
+.B eh_deadline
+.TP
 .B user_friendly_names
 .TP
 .B retain_attached_hw_handler
-- 
2.17.2

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

[dm-devel] [PATCH 9/9] libmultipath: Add documentation for the protocol subsection

[Benjamin Marzinski]

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/multipath.conf.5 | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 97695da4..dab1d58f 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -1678,6 +1678,41 @@ section:
 .RE
 .PD
 .LP
+The device subsection also recognizes the optional \fIprotocol\fR subsection. A
+single device subsection can contain multiple protocol subsections. Devices are
+matched against the protocol subsection using the mandatory \fItype\fR
+attribute.  Attributes in a matching protocol subsection take precedence over
+attributes in the containing device subsection. If there are multiple matching
+protocol subsections within a device subsection, later entries take precedence.
+If there are multiple matching device subsections, attributes in later device
+subsections still take precedence over attributes in a protocol subsection of
+an earlier device subsection.
+.TP
+.B protocol subsection
+The protocol subsection recognizes the following mandatory attribute:
+.RS
+.TP
+.B type
+The protocol string of the path device. The possible values are \fIscsi:fcp\fR,
+\fIscsi:spi\fR, \fIscsi:ssa\fR, \fIscsi:sbp\fR, \fIscsi:srp\fR,
+\fIscsi:iscsi\fR, \fIscsi:sas\fR, \fIscsi:adt\fR, \fIscsi:ata\fR,
+\fIscsi:unspec\fR, \fIccw\fR, \fIcciss\fR, \fInvme\fR, and \fIundef\fR. This is
+\fBnot\fR a regular expression. the path device protcol string must match
+exactly. The protocol that a path is using can be viewed by running
+\fBmultipathd show paths format "%d %P"\fR
+.LP
+The following attributes are optional; if not set, the default values are taken
+from the containing \fIdevice\fR subsection:
+.sp 1
+.PD .1v
+.RS
+.TP
+.B fast_io_fail_tmo
+.TP
+.B dev_loss_tmo
+.TP
+.B eh_deadline
+.PD
 .
 .
 .\" ----------------------------------------------------------------------------
@@ -1764,6 +1799,9 @@ the values are taken from the \fIdevices\fR or \fIdefaults\fR sections:
 .RE
 .PD
 .LP
+The overrides section also recognizes the optional \fIprotocol\fR subsection
+and can contain multiple protocol subsections. For a full description of this
+subsection please see its description in the \fIdevices\fR section.
 .
 .
 .\" ----------------------------------------------------------------------------
-- 
2.17.2

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

Re: [dm-devel] [PATCH 0/9] Add protocol specific config subsection

[Martin Wilck]

On Mon, 2022-04-11 at 20:59 -0500, Benjamin Marzinski wrote:
> Some storage arrays can be accessed using multiple protocols at the
> same
> time.  I've have customers request the ability to set different
> values
> for the path specific timeouts, like fast_io_fail_tmo, based on the
> protocol used to access the path. In order to make this possible,
> this
> patchset adds a new protocol subsection to the device subsection and
> the
> overrides section. This allows users to set a device config's path
> specific timeouts, such as dev_loss_tmo, fast_io_fail_tmo, and
> eh_deadline on a per-protocol basis.

Sigh... I am not happy about this amount of additional complexity in
the multipath configuration. It is already so complicated that hardly
anyone really understands how it works.

I fully agree that some properties, in particular fast_io_fail_tmo [1],
are rather properties of protocol or fabrics than a storage array.
But do we really need to differentiate by both "device" and "protocol"?
IOW, do users need different fast_io_fail_tmo value for the same
protocol, but different arrays? My feeling is that making this property
depend on a 2-D matrix of (protocol x storage) is overcomplicating
matters. And _if_ this is necessary, what do we gain by adding an
"overrides" on top? [2]

What about adding simply adding "protocols" as a new top section in the
conf file, and have the per-protocol settings override built-in hwtable
settings for any array, but not explicit storage-device settings in the
conf file?

Regards,
Martin

[1]: wrt dev_loss_tmo, I tend to think that the best value must be
found based on neither protocol nor array, but data center staff.
[2]: I strongly dislike "overrides", in general. I wonder what we need
it for, except for quick experiments where people are too lazy to add
explicit settings for devices and/or multipaths.

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

Re: [dm-devel] [PATCH 0/9] Add protocol specific config subsection

[Martin Wilck]

On Tue, 2022-04-12 at 12:31 +0200, Martin Wilck wrote:
> On Mon, 2022-04-11 at 20:59 -0500, Benjamin Marzinski wrote:
> > Some storage arrays can be accessed using multiple protocols at the
> > same
> > time.  I've have customers request the ability to set different
> > values
> > for the path specific timeouts, like fast_io_fail_tmo, based on the
> > protocol used to access the path. In order to make this possible,
> > this
> > patchset adds a new protocol subsection to the device subsection
> > and
> > the
> > overrides section. This allows users to set a device config's path
> > specific timeouts, such as dev_loss_tmo, fast_io_fail_tmo, and
> > eh_deadline on a per-protocol basis.
> 
> Sigh... I am not happy about this amount of additional complexity in
> the multipath configuration.

I'll postpone a detailed review of patch 03 ff. of this series until
we agree how the user-facing side should look like. 

Martin

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

Re: [dm-devel] [PATCH 0/9] Add protocol specific config subsection

[Benjamin Marzinski]

On Tue, Apr 12, 2022 at 10:31:50AM +0000, Martin Wilck wrote:
> On Mon, 2022-04-11 at 20:59 -0500, Benjamin Marzinski wrote:
> > Some storage arrays can be accessed using multiple protocols at the
> > same
> > time.  I've have customers request the ability to set different
> > values
> > for the path specific timeouts, like fast_io_fail_tmo, based on the
> > protocol used to access the path. In order to make this possible,
> > this
> > patchset adds a new protocol subsection to the device subsection and
> > the
> > overrides section. This allows users to set a device config's path
> > specific timeouts, such as dev_loss_tmo, fast_io_fail_tmo, and
> > eh_deadline on a per-protocol basis.
> 
> Sigh... I am not happy about this amount of additional complexity in
> the multipath configuration. It is already so complicated that hardly
> anyone really understands how it works.
> 
> I fully agree that some properties, in particular fast_io_fail_tmo [1],
> are rather properties of protocol or fabrics than a storage array.
> But do we really need to differentiate by both "device" and "protocol"?
> IOW, do users need different fast_io_fail_tmo value for the same
> protocol, but different arrays? My feeling is that making this property
> depend on a 2-D matrix of (protocol x storage) is overcomplicating
> matters. And _if_ this is necessary, what do we gain by adding an
> "overrides" on top? [2]

I agree that setting fast_io_fail_tmo to different values based on both
array and protocol is usually overkill. This is why I added it to the
overrides section as well. If you just put it there, it will work for
all devices. I assumed that would be the common case. It wouldn't make
sense to have it be in the defaults section and override things in
devices section, but it's a natural fit for the overrides section. Plus,
since I added the protocol table to the hwentry, enabling it in the
overrides section wasn't much new code.
 
> What about adding simply adding "protocols" as a new top section in the
> conf file, and have the per-protocol settings override built-in hwtable
> settings for any array, but not explicit storage-device settings in the
> conf file?

I'm not really enamored with the idea of only working on the built-in
hwtable. I feel like the people that want to tune things based on the
protocol are the same sort of people that want tune things per array.
More importantly, we don't have anything else in multipath that only
applies to some of the device config entries. That change seems more
confusing to me than adding a new subsection. The protocol subsection is
visually part of the device config it is modifying. A separate protocol
section that only modified some of the device configs seems less
obvious. Plus we don't have a good way to code that.  Also, what happens
to merged configs, where some of the timeouts came from a builtin
config, and some came from the user config.  If you are really against
the subsection idea, I would rather have the protocol section override
all the device configs. Users would need to be o.k. with picking a
protocol for which all their arrays have the same timeout values. But
again, that should be the common case.

> Regards,
> Martin
> 
> [1]: wrt dev_loss_tmo, I tend to think that the best value must be
> found based on neither protocol nor array, but data center staff.

I do agree that fast_io_fail_tmo and eh_deadline make more sense than
dev_loss_tmo (especially since FC/iSCSI setups seem the most common, and
iSCSI doesn't support dev_loss_tmo), but since the section is there, and
dev_loss_tmo is a per-path timeout setting, I put it in.  I thought
about letting people change the checker type per protocol, but figured
that could wait till someone asked for it. This would make less sense if
we had a seperate top level protocol section. So would things like
per-protocol manginal path handling, and other path specific things
which could reasonably go in a protocol subsection of a device config.

> [2]: I strongly dislike "overrides", in general. I wonder what we need
> it for, except for quick experiments where people are too lazy to add
> explicit settings for devices and/or multipaths.

The biggest reason is that some of the builtin device configs do things
like set no_path_retry to "queue". I know people have used it to
override dev_loss_tmo and fast_io_fail_tmo, but the big one is
no_path_retry. But in general, some builtin device configs make choices
that aren't applicable for all environments, but are what the vendors
have validated and want for the default.

While you can call it lazy, it's always possible that a new builtin
config will be added, or and existing one changed, and suddenly your
devices are hanging instead of failing like expected when all the paths
go down, because the devices are now configured with a different
no_path_retry value.  It's safer to simply disallow this possiblity.

Also, the overrides section exists so that it is possible to set up a
multipath config that will work within some environment's constraints,
regardless of the hardware that is attached. For instance, in
virtualized setups, no_path_retry "queue" can be really annoying.  So
virtualization solutions that use multipath can distribute a
multipath.conf that overrides these settings on all types of possible
devices, without having to go through and modify every problematic
builtin device entry, and update their config as new entries are added.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/9] Add protocol specific config subsection

[Martin Wilck]

On Tue, 2022-04-12 at 13:47 -0500, Benjamin Marzinski wrote:
> On Tue, Apr 12, 2022 at 10:31:50AM +0000, Martin Wilck wrote:
> > 
> I agree that setting fast_io_fail_tmo to different values based on
> both
> array and protocol is usually overkill.

If we doubt that there is a serious use case for this level of
differentiation, I think we shouldn't implement it. Doing that would
violate the KISS principle.

> > 
> > What about adding simply adding "protocols" as a new top section in
> > the
> > conf file, and have the per-protocol settings override built-in
> > hwtable
> > settings for any array, but not explicit storage-device settings in
> > the
> > conf file?
> 
> I'm not really enamored with the idea of only working on the built-in
> hwtable.
> I feel like the people that want to tune things based on the
> protocol are the same sort of people that want tune things per array.

Hm, maybe we are misunderstanding each other. Let me give an example
what I meant. We've got dev_loss = inifinity for ONTAP in the builtin
hwtable. My idea would be that users could override this in the
protocols section for all devices using a certain protocol:

protocols {
    protocol {
    	type scsi:fcp
	dev_loss_tmo 120
    }
}

This would take precedence over the built-in hwtable, but not over an
explicit config file entry:

devices {
    device {
        vendor NETAPP
        product LUN
        # With the dev_loss_tmo line below, RDAC would use 300 for
        # every protocol. Without it, it would use 120 for FC and
        # "infinity" for other protocols.
        dev_loss_tmo 300
    }
}      

Admittedly, the problem on the implementation side is that we don't
distinguish between built-in hwentries and those from explicit
configuration. Changing this would be considerable effort; perhaps more
effort than what you did in your patch set. I haven't thought it
through.

> More importantly, we don't have anything else in multipath that only
> applies to some of the device config entries. That change seems more
> confusing to me than adding a new subsection.

The main change would be to differentiate between built-in and user-
configured hardware properties. I hope most users would be able to
understand the concept.

>  The protocol subsection is
> visually part of the device config it is modifying. A separate
> protocol
> section that only modified some of the device configs
>  seems less
> obvious. Plus we don't have a good way to code that.  Also, what
> happens
> to merged configs, where some of the timeouts came from a builtin
> config, and some came from the user config.

To clarify once more: this is what I meant, built-in configs would be
overridden, user configs wouldn't be. This is different from
"defaults", as "defaults" don't override hardware-specific built-ins.

>   If you are really agains the subsection idea,
>  I would rather have the protocol section override
> all the device configs. Users would need to be o.k. with picking a
> protocol for which all their arrays have the same timeout values. But
> again, that should be the common case.

The question is whether a "protocol" entry should override device
settings, or vice versa (as in my hypothetical ONTAP example above). We
seem to agree that that device subsection would implement a level of
complexity that hardly any user needs. If this is the case, we'd just
need to determine the precedence between "devices" and "protocols"
sections. If we determine that "protocols" should always take
precedence, we might as well allow it under "overrides" only. We'd need
no "protocols" section in that case, and no differentiation between
built-in and user-configured hwentries.

> > [1]: wrt dev_loss_tmo, I tend to think that the best value must be
> > found based on neither protocol nor array, but data center staff.
> 
> I do agree that fast_io_fail_tmo and eh_deadline make more sense than
> dev_loss_tmo (especially since FC/iSCSI setups seem the most common,
> and
> iSCSI doesn't support dev_loss_tmo), but since the section is there,
> and
> dev_loss_tmo is a per-path timeout setting, I put it in.
> ...

I'm fine with treating dev_loss_tmo as a protocol/path property.

> > [2]: I strongly dislike "overrides", in general. I wonder what we
> > need
> > it for, except for quick experiments where people are too lazy to
> > add
> > explicit settings for devices and/or multipaths.
> 
> The biggest reason is that some of the builtin device configs do
> things
> like set no_path_retry to "queue". 

You don't need to use "overrides" for that:

devices {
        device {
                vendor .*
                product .*
                no_path_retry 75
        }
}
You can follow up with more device entries that define exceptions for
the general rule above. Am I missing something?

AFAICT the only thing you can do with "overrides" but not with the
trick above is override actual hardware-specific user configs, and I
have a hard time figuring out why someone would work out detailed
device-specific configs just to override them again with a big hammer.

Regards,
Martin

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

Re: [dm-devel] [PATCH 0/9] Add protocol specific config subsection

[Benjamin Marzinski]

On Tue, Apr 12, 2022 at 08:47:38PM +0000, Martin Wilck wrote:
> On Tue, 2022-04-12 at 13:47 -0500, Benjamin Marzinski wrote:
> > On Tue, Apr 12, 2022 at 10:31:50AM +0000, Martin Wilck wrote:
> > > 
> > I agree that setting fast_io_fail_tmo to different values based on
> > both
> > array and protocol is usually overkill.
> 
> If we doubt that there is a serious use case for this level of
> differentiation, I think we shouldn't implement it. Doing that would
> violate the KISS principle.
> 
> > > 
> > > What about adding simply adding "protocols" as a new top section in
> > > the
> > > conf file, and have the per-protocol settings override built-in
> > > hwtable
> > > settings for any array, but not explicit storage-device settings in
> > > the
> > > conf file?
> > 
> > I'm not really enamored with the idea of only working on the built-in
> > hwtable.
> > I feel like the people that want to tune things based on the
> > protocol are the same sort of people that want tune things per array.
> 
> Hm, maybe we are misunderstanding each other. Let me give an example
> what I meant. We've got dev_loss = inifinity for ONTAP in the builtin
> hwtable. My idea would be that users could override this in the
> protocols section for all devices using a certain protocol:
> 
> protocols {
>     protocol {
>     	type scsi:fcp
> 	dev_loss_tmo 120
>     }
> }
> 
> This would take precedence over the built-in hwtable, but not over an
> explicit config file entry:
> 
> devices {
>     device {
>         vendor NETAPP
>         product LUN
>         # With the dev_loss_tmo line below, RDAC would use 300 for
>         # every protocol. Without it, it would use 120 for FC and
>         # "infinity" for other protocols.
>         dev_loss_tmo 300
>     }
> }      
> 
> Admittedly, the problem on the implementation side is that we don't
> distinguish between built-in hwentries and those from explicit
> configuration. Changing this would be considerable effort; perhaps more
> effort than what you did in your patch set. I haven't thought it
> through.
> 
> > More importantly, we don't have anything else in multipath that only
> > applies to some of the device config entries. That change seems more
> > confusing to me than adding a new subsection.
> 
> The main change would be to differentiate between built-in and user-
> configured hardware properties. I hope most users would be able to
> understand the concept.
> 
> >  The protocol subsection is
> > visually part of the device config it is modifying. A separate
> > protocol
> > section that only modified some of the device configs
> >  seems less
> > obvious. Plus we don't have a good way to code that.  Also, what
> > happens
> > to merged configs, where some of the timeouts came from a builtin
> > config, and some came from the user config.
> 
> To clarify once more: this is what I meant, built-in configs would be
> overridden, user configs wouldn't be. This is different from
> "defaults", as "defaults" don't override hardware-specific built-ins.

But what do you call a device config that is the result of merging (via
merge_hwe()) a built-in and a non-built-in config.  Do we really want to
track that some of the values of this merged config need to check the
protocol section, and some don't? We could remove merging identical
configs, but that simply makes it harder for users to figure out how
their device will be configured from the configuration output.

I understand your idea. I'd just rather that it worked on all the device
configs, instead of only the built-in ones. I think overriding only the
built-in configs is needlessly complicated, both from a coding and from
an explaining point of view.

> >   If you are really agains the subsection idea,
> >  I would rather have the protocol section override
> > all the device configs. Users would need to be o.k. with picking a
> > protocol for which all their arrays have the same timeout values. But
> > again, that should be the common case.
> 
> The question is whether a "protocol" entry should override device
> settings, or vice versa (as in my hypothetical ONTAP example above). We
> seem to agree that that device subsection would implement a level of
> complexity that hardly any user needs. If this is the case, we'd just
> need to determine the precedence between "devices" and "protocols"
> sections. If we determine that "protocols" should always take
> precedence, we might as well allow it under "overrides" only. We'd need
> no "protocols" section in that case, and no differentiation between
> built-in and user-configured hwentries.

That seems reasonable.

> > > [1]: wrt dev_loss_tmo, I tend to think that the best value must be
> > > found based on neither protocol nor array, but data center staff.
> > 
> > I do agree that fast_io_fail_tmo and eh_deadline make more sense than
> > dev_loss_tmo (especially since FC/iSCSI setups seem the most common,
> > and
> > iSCSI doesn't support dev_loss_tmo), but since the section is there,
> > and
> > dev_loss_tmo is a per-path timeout setting, I put it in.
> > ...
> 
> I'm fine with treating dev_loss_tmo as a protocol/path property.
> 
> > > [2]: I strongly dislike "overrides", in general. I wonder what we
> > > need
> > > it for, except for quick experiments where people are too lazy to
> > > add
> > > explicit settings for devices and/or multipaths.
> > 
> > The biggest reason is that some of the builtin device configs do
> > things
> > like set no_path_retry to "queue". 
> 
> You don't need to use "overrides" for that:
> 
> devices {
>         device {
>                 vendor .*
>                 product .*
>                 no_path_retry 75
>         }
> }
> You can follow up with more device entries that define exceptions for
> the general rule above. Am I missing something?
> 
> AFAICT the only thing you can do with "overrides" but not with the
> trick above is override actual hardware-specific user configs, and I
> have a hard time figuring out why someone would work out detailed
> device-specific configs just to override them again with a big hammer.

Fair enough. I added the overrides section before you made paths have a
vector of device configs. Back then, there was no way to have a device
config that would work like your above example.  My original idea was to
be able to have a special device section like this:

device {
        all_devs yes
	no_path_retry 75
}

that would get merged with all the device sections. The overrides
section was the compromise after my original idea was NAKed. We probably
could deprecate the overrides section now that we have a vector of
device configs. But then we shouldn't go putting the protocol stuff
there. 

-Ben

> Regards,
> Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/9] Add protocol specific config subsection

[Martin Wilck]

On Tue, 2022-04-12 at 17:01 -0500, Benjamin Marzinski wrote:
> On Tue, Apr 12, 2022 at 08:47:38PM +0000, Martin Wilck wrote:
> > 
> > To clarify once more: this is what I meant, built-in configs would
> > be
> > overridden, user configs wouldn't be. This is different from
> > "defaults", as "defaults" don't override hardware-specific built-
> > ins.
> 
> But what do you call a device config that is the result of merging
> (via
> merge_hwe()) a built-in and a non-built-in config.  Do we really want
> to
> track that some of the values of this merged config need to check the
> protocol section, and some don't? We could remove merging identical
> configs, but that simply makes it harder for users to figure out how
> their device will be configured from the configuration output.

Yes, I figured this might be tricky. My vague idea was to track the
origin with sort of a bit field inside the hwe's. Never mind now.

> I understand your idea. I'd just rather that it worked on all the
> device
> configs, instead of only the built-in ones. I think overriding only
> the
> built-in configs is needlessly complicated, both from a coding and
> from
> an explaining point of view.

I think that users are able to distinguish between built-in defaults
and settings they made explicitly. Being able to differentiate between
these in the "origin" log messages would also have a certain value.

But I said to you not to over-complicate matters, so I suppose I
shouldn't do that, either. If you think just using "overrides" is
sufficient, I'm fine with that.

> > > The biggest reason is that some of the builtin device configs do
> > > things
> > > like set no_path_retry to "queue". 
> > 
> > You don't need to use "overrides" for that:
> > 
> > devices {
> >         device {
> >                 vendor .*
> >                 product .*
> >                 no_path_retry 75
> >         }
> > }
> > You can follow up with more device entries that define exceptions
> > for
> > the general rule above. Am I missing something?
> > 
> > AFAICT the only thing you can do with "overrides" but not with the
> > trick above is override actual hardware-specific user configs, and
> > I
> > have a hard time figuring out why someone would work out detailed
> > device-specific configs just to override them again with a big
> > hammer.
> 
> Fair enough. I added the overrides section before you made paths have
> a
> vector of device configs. Back then, there was no way to have a
> device
> config that would work like your above example.  My original idea was
> to
> be able to have a special device section like this:
> 

Right. Didn't think about that.

> device {
>         all_devs yes
>         no_path_retry 75
> }
> 
> that would get merged with all the device sections. The overrides
> section was the compromise after my original idea was NAKed. We
> probably
> could deprecate the overrides section now that we have a vector of
> device configs. But then we shouldn't go putting the protocol stuff
> there. 

Well I guess as we have had the section for decades, we might as well
just leave it in. Sorry for distracting you with my rant.

Still to be determined whether "protocol" should simply go into
"overrides", or into a separate section (with the precedence semantics
you consider appropriate). I fine with either, so I guess it'll be
"overrides".

Regards
Martin

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