[dm-devel] [PATCH v2 0/6] Misc Multipath patches

[dm-devel] [PATCH v2 0/6] Misc Multipath patches

[Benjamin Marzinski]

This is a set of unrelated patches, based on top of my previous "add
library to check if device is a valid path" patchset. The first two
patches add a new config option, eh_deadline, that sets the scsi sysfs
value of the same name for scsi path devices. This has been requested by
multiple customers. Patch 0004 is a change to detecting rdac support
requested by Netapp.

Patch 0005 fixes a rare segfault during shutdown, that happens when the
tur DSO is unloaded while the tur thread is still running. The issue is
that unless we make the tur_checker thread joinable, there is no way to
be sure that it isn't still running when the DSO is unloaded. I tried to
fix this in a way that allowed the DSO to get cleaned up. But without
redoing the tur_thread, that was impossible. Even if the cleanup
happened when the tur_thread was running a cleanup handler function from
libmultipath (not the DSO), while in pthread_exit(), which doesn't
return back to the calling fuction, it would segfault.

I realize that there has just been a lot of work done to make sure that
multipathd is cleaning up before exitting that this code is going
against that, but I'm not sure that the cost of redoing the tur_thread
is worth the benefit of being able to unload the DSO. If people feel
strongly that we should always unload the DSO, I can redo this and make
the tur_thread joinable with pthread_tryjoin_np(), and add code to the
checkerloop or uxlsnrloop to join with orphaned tur_threads.  That
should work.

Changes from v1 to v2:
0002: multiple small fixes suggested by Martin
0004: New patch to setup for checking vpd page 0x00. Just refactoring code,
      with not functional changes.
0005 (was 0004): added checking for vpd page 0xc9 in vpd page 0x00, as
                 suggested by Martin
0006 (was 0005): Added version script update


Benjamin Marzinski (6):
  libmultipath: move fast_io_fail defines to structs.h
  libmultipath: add eh_deadline multipath.conf parameter
  multipathd: remove redundant vector_free() int configure
  libmultipath: factor out code to get vpd page data
  libmultipath: limit reading 0xc9 vpd page
  libmultipath: don't dlclose tur checker DSO

 libmultipath/checkers.c           |  10 ++-
 libmultipath/checkers.h           |   1 +
 libmultipath/checkers/tur.c       |   1 +
 libmultipath/config.c             |   2 +
 libmultipath/config.h             |  10 +--
 libmultipath/configure.c          |   1 +
 libmultipath/dict.c               |  40 +++++++-----
 libmultipath/dict.h               |   2 +-
 libmultipath/discovery.c          | 104 ++++++++++++++++++++++++++----
 libmultipath/discovery.h          |   1 +
 libmultipath/libmultipath.version |   5 ++
 libmultipath/propsel.c            |  29 +++++++--
 libmultipath/propsel.h            |   1 +
 libmultipath/structs.h            |  24 +++++++
 multipath/multipath.conf.5        |  16 +++++
 multipathd/main.c                 |   8 +--
 16 files changed, 208 insertions(+), 47 deletions(-)

-- 
2.17.2

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

[dm-devel] [PATCH v2 1/6] libmultipath: move fast_io_fail defines to structs.h

[Benjamin Marzinski]

Since fast_io_fail is part of the multipath struct, its symbolic values
belong in structs.h. Also, make it an instance of a general enum, which
will be used again in future patches, and change the set/print functions
which use it to use the general enum instead.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.h  |  8 --------
 libmultipath/dict.c    | 30 +++++++++++++++---------------
 libmultipath/dict.h    |  2 +-
 libmultipath/propsel.c |  2 +-
 libmultipath/structs.h | 17 +++++++++++++++++
 5 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index 5d460359..661dd586 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -10,14 +10,6 @@
 #define ORIGIN_DEFAULT 0
 #define ORIGIN_CONFIG  1
 
-/*
- * In kernel, fast_io_fail == 0 means immediate failure on rport delete.
- * OTOH '0' means not-configured in various places in multipath-tools.
- */
-#define MP_FAST_IO_FAIL_UNSET (0)
-#define MP_FAST_IO_FAIL_OFF (-1)
-#define MP_FAST_IO_FAIL_ZERO (-2)
-
 enum devtypes {
 	DEV_NONE,
 	DEV_DEVT,
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index f12c2e5c..f4357da1 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -822,7 +822,7 @@ declare_mp_attr_handler(gid, set_gid)
 declare_mp_attr_snprint(gid, print_gid)
 
 static int
-set_fast_io_fail(vector strvec, void *ptr)
+set_undef_off_zero(vector strvec, void *ptr)
 {
 	char * buff;
 	int *int_ptr = (int *)ptr;
@@ -832,36 +832,36 @@ set_fast_io_fail(vector strvec, void *ptr)
 		return 1;
 
 	if (strcmp(buff, "off") == 0)
-		*int_ptr = MP_FAST_IO_FAIL_OFF;
+		*int_ptr = UOZ_OFF;
 	else if (sscanf(buff, "%d", int_ptr) != 1 ||
-		 *int_ptr < MP_FAST_IO_FAIL_ZERO)
-		*int_ptr = MP_FAST_IO_FAIL_UNSET;
+		 *int_ptr < UOZ_ZERO)
+		*int_ptr = UOZ_UNDEF;
 	else if (*int_ptr == 0)
-		*int_ptr = MP_FAST_IO_FAIL_ZERO;
+		*int_ptr = UOZ_ZERO;
 
 	FREE(buff);
 	return 0;
 }
 
 int
-print_fast_io_fail(char * buff, int len, long v)
+print_undef_off_zero(char * buff, int len, long v)
 {
-	if (v == MP_FAST_IO_FAIL_UNSET)
+	if (v == UOZ_UNDEF)
 		return 0;
-	if (v == MP_FAST_IO_FAIL_OFF)
+	if (v == UOZ_OFF)
 		return snprintf(buff, len, "\"off\"");
-	if (v == MP_FAST_IO_FAIL_ZERO)
+	if (v == UOZ_ZERO)
 		return snprintf(buff, len, "0");
 	return snprintf(buff, len, "%ld", v);
 }
 
-declare_def_handler(fast_io_fail, set_fast_io_fail)
-declare_def_snprint_defint(fast_io_fail, print_fast_io_fail,
+declare_def_handler(fast_io_fail, set_undef_off_zero)
+declare_def_snprint_defint(fast_io_fail, print_undef_off_zero,
 			   DEFAULT_FAST_IO_FAIL)
-declare_ovr_handler(fast_io_fail, set_fast_io_fail)
-declare_ovr_snprint(fast_io_fail, print_fast_io_fail)
-declare_hw_handler(fast_io_fail, set_fast_io_fail)
-declare_hw_snprint(fast_io_fail, print_fast_io_fail)
+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)
 
 static int
 set_dev_loss(vector strvec, void *ptr)
diff --git a/libmultipath/dict.h b/libmultipath/dict.h
index a40ac66f..a917e1ca 100644
--- a/libmultipath/dict.h
+++ b/libmultipath/dict.h
@@ -13,7 +13,7 @@ int print_rr_weight(char *buff, int len, long v);
 int print_pgfailback(char *buff, int len, long v);
 int print_pgpolicy(char *buff, int len, long v);
 int print_no_path_retry(char *buff, int len, long v);
-int print_fast_io_fail(char *buff, int len, long v);
+int print_undef_off_zero(char *buff, int len, long v);
 int print_dev_loss(char *buff, int len, unsigned long v);
 int print_reservation_key(char * buff, int len, struct be64 key, uint8_t
 			  flags, int source);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 3f2c2cfa..67d025cf 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -754,7 +754,7 @@ int select_fast_io_fail(struct config *conf, struct multipath *mp)
 	mp_set_conf(fast_io_fail);
 	mp_set_default(fast_io_fail, DEFAULT_FAST_IO_FAIL);
 out:
-	print_fast_io_fail(buff, 12, mp->fast_io_fail);
+	print_undef_off_zero(buff, 12, mp->fast_io_fail);
 	condlog(3, "%s: fast_io_fail_tmo = %s %s", mp->alias, buff, origin);
 	return 0;
 }
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 4ce30551..cfa7b649 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -219,6 +219,23 @@ enum vpd_vendor_ids {
 	VPD_VP_ARRAY_SIZE, /* This must remain the last entry */
 };
 
+/*
+ * Multipath treats 0 as undefined for optional config parameters.
+ * Use this for cases where 0 is a valid option for systems multipath
+ * is communicating with
+ */
+enum undefined_off_zero {
+	UOZ_UNDEF = 0,
+	UOZ_OFF = -1,
+	UOZ_ZERO = -2,
+};
+
+enum fast_io_fail_states {
+	MP_FAST_IO_FAIL_UNSET = UOZ_UNDEF,
+	MP_FAST_IO_FAIL_OFF = UOZ_OFF,
+	MP_FAST_IO_FAIL_ZERO = UOZ_ZERO,
+};
+
 struct vpd_vendor_page {
 	int pg;
 	const char *name;
-- 
2.17.2

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

[dm-devel] [PATCH v2 2/6] libmultipath: add eh_deadline multipath.conf parameter

[Benjamin Marzinski]

There are times a fc rport is never lost, meaning that fast_io_fail_tmo
and dev_loss_tmo never trigger, but scsi commands still hang. This can
cause problems in cases where users have strict timing requirements, and
the easiest way to solve these issues is to set eh_deadline. Since it's
already possible to set fast_io_fail_tmo and dev_loss_tmo from
multipath.conf, and have multipath take care of setting it correctly for
the scsi devices in sysfs, it makes sense to allow users to set
eh_deadline here as well.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c      |  2 ++
 libmultipath/config.h      |  2 ++
 libmultipath/configure.c   |  1 +
 libmultipath/dict.c        | 10 +++++++
 libmultipath/discovery.c   | 60 +++++++++++++++++++++++++++++++++-----
 libmultipath/propsel.c     | 17 +++++++++++
 libmultipath/propsel.h     |  1 +
 libmultipath/structs.h     |  7 +++++
 multipath/multipath.conf.5 | 16 ++++++++++
 9 files changed, 109 insertions(+), 7 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 49e7fb81..9f3cb38d 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -424,6 +424,7 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
 	merge_num(flush_on_last_del);
 	merge_num(fast_io_fail);
 	merge_num(dev_loss);
+	merge_num(eh_deadline);
 	merge_num(user_friendly_names);
 	merge_num(retain_hwhandler);
 	merge_num(detect_prio);
@@ -579,6 +580,7 @@ store_hwe (vector hwtable, struct hwentry * dhwe)
 	hwe->flush_on_last_del = dhwe->flush_on_last_del;
 	hwe->fast_io_fail = dhwe->fast_io_fail;
 	hwe->dev_loss = dhwe->dev_loss;
+	hwe->eh_deadline = dhwe->eh_deadline;
 	hwe->user_friendly_names = dhwe->user_friendly_names;
 	hwe->retain_hwhandler = dhwe->retain_hwhandler;
 	hwe->detect_prio = dhwe->detect_prio;
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 661dd586..9ce37f16 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -63,6 +63,7 @@ struct hwentry {
 	int flush_on_last_del;
 	int fast_io_fail;
 	unsigned int dev_loss;
+	int eh_deadline;
 	int user_friendly_names;
 	int retain_hwhandler;
 	int detect_prio;
@@ -148,6 +149,7 @@ struct config {
 	int attribute_flags;
 	int fast_io_fail;
 	unsigned int dev_loss;
+	int eh_deadline;
 	int log_checker_err;
 	int allow_queueing;
 	int allow_usb_devices;
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 1c8aac08..a878d870 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -368,6 +368,7 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
 	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);
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index f4357da1..bab96146 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -899,6 +899,13 @@ 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_def_handler(eh_deadline, set_undef_off_zero)
+declare_def_snprint(eh_deadline, print_undef_off_zero)
+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)
+
 static int
 set_pgpolicy(vector strvec, void *ptr)
 {
@@ -1771,6 +1778,7 @@ init_keywords(vector keywords)
 	install_keyword("gid", &def_gid_handler, &snprint_def_gid);
 	install_keyword("fast_io_fail_tmo", &def_fast_io_fail_handler, &snprint_def_fast_io_fail);
 	install_keyword("dev_loss_tmo", &def_dev_loss_handler, &snprint_def_dev_loss);
+	install_keyword("eh_deadline", &def_eh_deadline_handler, &snprint_def_eh_deadline);
 	install_keyword("bindings_file", &def_bindings_file_handler, &snprint_def_bindings_file);
 	install_keyword("wwids_file", &def_wwids_file_handler, &snprint_def_wwids_file);
 	install_keyword("prkeys_file", &def_prkeys_file_handler, &snprint_def_prkeys_file);
@@ -1880,6 +1888,7 @@ init_keywords(vector keywords)
 	install_keyword("flush_on_last_del", &hw_flush_on_last_del_handler, &snprint_hw_flush_on_last_del);
 	install_keyword("fast_io_fail_tmo", &hw_fast_io_fail_handler, &snprint_hw_fast_io_fail);
 	install_keyword("dev_loss_tmo", &hw_dev_loss_handler, &snprint_hw_dev_loss);
+	install_keyword("eh_deadline", &hw_eh_deadline_handler, &snprint_hw_eh_deadline);
 	install_keyword("user_friendly_names", &hw_user_friendly_names_handler, &snprint_hw_user_friendly_names);
 	install_keyword("retain_attached_hw_handler", &hw_retain_hwhandler_handler, &snprint_hw_retain_hwhandler);
 	install_keyword("detect_prio", &hw_detect_prio_handler, &snprint_hw_detect_prio);
@@ -1920,6 +1929,7 @@ init_keywords(vector keywords)
 	install_keyword("flush_on_last_del", &ovr_flush_on_last_del_handler, &snprint_ovr_flush_on_last_del);
 	install_keyword("fast_io_fail_tmo", &ovr_fast_io_fail_handler, &snprint_ovr_fast_io_fail);
 	install_keyword("dev_loss_tmo", &ovr_dev_loss_handler, &snprint_ovr_dev_loss);
+	install_keyword("eh_deadline", &ovr_eh_deadline_handler, &snprint_ovr_eh_deadline);
 	install_keyword("user_friendly_names", &ovr_user_friendly_names_handler, &snprint_ovr_user_friendly_names);
 	install_keyword("retain_attached_hw_handler", &ovr_retain_hwhandler_handler, &snprint_ovr_retain_hwhandler);
 	install_keyword("detect_prio", &ovr_detect_prio_handler, &snprint_ovr_detect_prio);
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 950b1586..a97d2998 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -585,6 +585,42 @@ sysfs_get_asymmetric_access_state(struct path *pp, char *buff, int buflen)
 	return !!preferred;
 }
 
+static int
+sysfs_set_eh_deadline(struct multipath *mpp, 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)
+		return 0;
+
+	sprintf(host_name, "host%d", pp->sg_id.host_no);
+	hostdev = udev_device_new_from_subsystem_sysname(udev,
+			"scsi_host", host_name);
+	if (!hostdev)
+		return 1;
+
+	if (mpp->eh_deadline == EH_DEADLINE_OFF)
+		len = sprintf(value, "off");
+	else if (mpp->eh_deadline == EH_DEADLINE_ZERO)
+		len = sprintf(value, "0");
+	else
+		len = sprintf(value, "%d", mpp->eh_deadline);
+
+	ret = sysfs_attr_set_value(hostdev, "eh_deadline",
+				   value, len + 1);
+	/*
+	 * not all scsi drivers support setting eh_deadline, so failing
+	 * is totally reasonable
+	 */
+	if (ret <= 0)
+		condlog(3, "%s: failed to set eh_deadline to %s, error %d", udev_device_get_sysname(hostdev), value, -ret);
+
+	udev_device_unref(hostdev);
+	return (ret <= 0);
+}
+
 static void
 sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp)
 {
@@ -594,6 +630,10 @@ 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)
+		return;
+
 	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(udev,
@@ -701,6 +741,11 @@ sysfs_set_session_tmo(struct multipath *mpp, struct path *pp)
 	char session_id[64];
 	char value[11];
 
+	if (mpp->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)
+		return;
+
 	sprintf(session_id, "session%d", pp->sg_id.transport_id);
 	session_dev = udev_device_new_from_subsystem_sysname(udev,
 				"iscsi_session", session_id);
@@ -712,9 +757,6 @@ 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->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 (mpp->fast_io_fail == MP_FAST_IO_FAIL_OFF) {
 			condlog(3, "%s: can't switch off fast_io_fail_tmo "
@@ -742,6 +784,8 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path *pp)
 	char end_dev_id[64];
 	char value[11];
 
+	if (mpp->dev_loss == DEV_LOSS_TMO_UNSET)
+		return;
 	sprintf(end_dev_id, "end_device-%d:%d",
 		pp->sg_id.host_no, pp->sg_id.transport_id);
 	sas_dev = udev_device_new_from_subsystem_sysname(udev,
@@ -799,7 +843,8 @@ sysfs_set_scsi_tmo (struct multipath *mpp, unsigned int checkint)
 		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->fast_io_fail == MP_FAST_IO_FAIL_UNSET &&
+	    mpp->eh_deadline == EH_DEADLINE_UNSET)
 		return 0;
 
 	vector_foreach_slot(mpp->paths, pp, i) {
@@ -812,17 +857,18 @@ sysfs_set_scsi_tmo (struct multipath *mpp, unsigned int checkint)
 		switch (pp->sg_id.proto_id) {
 		case SCSI_PROTOCOL_FCP:
 			sysfs_set_rport_tmo(mpp, pp);
-			continue;
+			break;
 		case SCSI_PROTOCOL_ISCSI:
 			sysfs_set_session_tmo(mpp, pp);
-			continue;
+			break;
 		case SCSI_PROTOCOL_SAS:
 			sysfs_set_nexus_loss_tmo(mpp, pp);
-			continue;
+			break;
 		default:
 			if (!err_path)
 				err_path = pp;
 		}
+		sysfs_set_eh_deadline(mpp, pp);
 	}
 
 	if (err_path) {
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 67d025cf..fa4ac5d9 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -775,6 +775,23 @@ out:
 	return 0;
 }
 
+int select_eh_deadline(struct config *conf, struct multipath *mp)
+{
+	const char *origin;
+	char buff[12];
+
+	mp_set_ovr(eh_deadline);
+	mp_set_hwe(eh_deadline);
+	mp_set_conf(eh_deadline);
+	mp->eh_deadline = EH_DEADLINE_UNSET;
+	/* not changing sysfs in default cause, so don't print anything */
+	return 0;
+out:
+	print_undef_off_zero(buff, 12, mp->eh_deadline);
+	condlog(3, "%s: eh_deadline = %s %s", mp->alias, buff, origin);
+	return 0;
+}
+
 int select_flush_on_last_del(struct config *conf, struct multipath *mp)
 {
 	const char *origin;
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index 3d6edd8a..a68bacf0 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -17,6 +17,7 @@ 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_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.h b/libmultipath/structs.h
index cfa7b649..d6ff6762 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -236,6 +236,12 @@ enum fast_io_fail_states {
 	MP_FAST_IO_FAIL_ZERO = UOZ_ZERO,
 };
 
+enum eh_deadline_states {
+	EH_DEADLINE_UNSET = UOZ_UNDEF,
+	EH_DEADLINE_OFF = UOZ_OFF,
+	EH_DEADLINE_ZERO = UOZ_ZERO,
+};
+
 struct vpd_vendor_page {
 	int pg;
 	const char *name;
@@ -356,6 +362,7 @@ struct multipath {
 	int ghost_delay;
 	int ghost_delay_tick;
 	unsigned int dev_loss;
+	int eh_deadline;
 	uid_t uid;
 	gid_t gid;
 	mode_t mode;
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index d2101ed6..cf05c1ab 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -717,6 +717,22 @@ The default is: \fB600\fR
 .
 .
 .TP
+.B eh_deadline
+Specify the maximum number of seconds the SCSI layer will spend doing error
+handling when scsi devices fail. After this timeout the scsi layer will perform
+a full HBA reset. Setting this may be necessary in cases where the rport is
+never lost, so \fIfast_io_fail_tmo\fR and \fIdev_loss_tmo\fR will never
+trigger, but (frequently do to load) scsi commands still hang. \fBNote:\fR when
+the scsi error handler performs the HBA reset, all target paths on that HBA
+will be affected. eh_deadline should only be set in cases where all targets on
+the affected HBAs are multipathed.
+.RS
+.TP
+The default is: \fB<unset>\fR
+.RE
+.
+.
+.TP
 .B bindings_file
 The full pathname of the binding file to be used when the user_friendly_names
 option is set.
-- 
2.17.2

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

[dm-devel] [PATCH v2 3/6] multipathd: remove redundant vector_free() int configure

[Benjamin Marzinski]

remove_maps(vecs) already calls vector_free(vecs->mpvec)

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 0cf8a264..e114bab7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2634,14 +2634,10 @@ configure (struct vectors * vecs)
 	}
 
 	/*
-	 * purge dm of old maps
+	 * purge dm of old maps and save new set of maps formed by
+	 * considering current path state
 	 */
 	remove_maps(vecs);
-
-	/*
-	 * save new set of maps formed by considering current path state
-	 */
-	vector_free(vecs->mpvec);
 	vecs->mpvec = mpvec;
 
 	/*
-- 
2.17.2

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

[dm-devel] [PATCH v2 4/6] libmultipath: factor out code to get vpd page data

[Benjamin Marzinski]

A future patch will reuse the code to get the vpd page data, so factor
it out from get_vpd_sgio().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index a97d2998..95ddbbbd 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1319,11 +1319,10 @@ get_vpd_sysfs (struct udev_device *parent, int pg, char * str, int maxlen)
 	return len;
 }
 
-int
-get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
+static int
+fetch_vpd_page(int fd, int pg, unsigned char *buff)
 {
-	int len, buff_len;
-	unsigned char buff[4096];
+	int buff_len;
 
 	memset(buff, 0x0, 4096);
 	if (sgio_get_vpd(buff, 4096, fd, pg) < 0) {
@@ -1344,6 +1343,18 @@ get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
 		condlog(3, "vpd pg%02x page truncated", pg);
 		buff_len = 4096;
 	}
+	return buff_len;
+}
+
+int
+get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
+{
+	int len, buff_len;
+	unsigned char buff[4096];
+
+	buff_len = fetch_vpd_page(fd, pg, buff);
+	if (buff_len < 0)
+		return buff_len;
 	if (pg == 0x80)
 		len = parse_vpd_pg80(buff, str, maxlen);
 	else if (pg == 0x83)
-- 
2.17.2

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

[dm-devel] [PATCH v2 5/6] libmultipath: limit reading 0xc9 vpd page

[Benjamin Marzinski]

Only rdac arrays support 0xC9 vpd page inquiries. All other arrays will
return a failure. Only do the rdac inquiry when detecting array
capabilities if the array's path checker is explicitly set to rdac, or
the path checker is not set, and the array reports that it supports vpd
page 0xC9 in the Supported VPD Pages (0x00) vpd page.

Multipath was doing the check if either the path checker was set to
rdac, or no path checker was set.  This means that for almost all
non-rdac arrays, multipath was issuing a bad inquiry. This was annoying
users.

Cc: Steve Schremmer <sschremm@netapp.com>
Cc: NetApp RDAC team <ng-eseries-upstream-maintainers@netapp.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 25 +++++++++++++++++++++++++
 libmultipath/discovery.h |  1 +
 libmultipath/propsel.c   | 10 ++++++----
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 95ddbbbd..5669690d 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1346,6 +1346,31 @@ fetch_vpd_page(int fd, int pg, unsigned char *buff)
 	return buff_len;
 }
 
+/* heavily based on sg_inq.c from sg3_utils */
+bool
+is_vpd_page_supported(int fd, int pg)
+{
+	int i, len, buff_len;
+	unsigned char buff[4096];
+
+	buff_len = fetch_vpd_page(fd, 0x00, buff);
+	if (buff_len < 0)
+		return false;
+	if (buff_len < 4) {
+		condlog(3, "VPD page 00h too short");
+		return false;
+	}
+
+	len = buff[3] + 4;
+	if (len > buff_len)
+		condlog(3, "vpd page 00h trucated, expected %d, have %d",
+			len, buff_len);
+	for (i = 4; i < len; ++i)
+		if (buff[i] == pg)
+			return true;
+	return false;
+}
+
 int
 get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
 {
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index 6444887d..d3193daf 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -56,6 +56,7 @@ int sysfs_get_asymmetric_access_state(struct path *pp,
 				      char *buff, int buflen);
 int get_uid(struct path * pp, int path_state, struct udev_device *udev,
 	    int allow_fallback);
+bool is_vpd_page_supported(int fd, int pg);
 
 /*
  * discovery bitmask
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index fa4ac5d9..f771a830 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -496,13 +496,15 @@ check_rdac(struct path * pp)
 {
 	int len;
 	char buff[44];
-	const char *checker_name;
+	const char *checker_name = NULL;
 
 	if (pp->bus != SYSFS_BUS_SCSI)
 		return 0;
-	/* Avoid ioctl if this is likely not an RDAC array */
-	if (__do_set_from_hwe(checker_name, pp, checker_name) &&
-	    strcmp(checker_name, RDAC))
+	/* Avoid checking 0xc9 if this is likely not an RDAC array */
+	if (!__do_set_from_hwe(checker_name, pp, checker_name) &&
+	    !is_vpd_page_supported(pp->fd, 0xC9))
+		return 0;
+	if (checker_name && strcmp(checker_name, RDAC))
 		return 0;
 	len = get_vpd_sgio(pp->fd, 0xC9, 0, buff, 44);
 	if (len <= 0)
-- 
2.17.2

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

[dm-devel] [PATCH v2 6/6] libmultipath: don't dlclose tur checker DSO

[Benjamin Marzinski]

The multipathd tur checker thread is designed to be able to finish at
any time, even after the tur checker itself has been freed. The
multipathd shutdown code makes sure all the checkers have been freed
before freeing the checker_class and calling dlclose() to unload the
DSO, but this doesn't guarantee that the checker threads have finished.
If one hasn't, the DSO will get unloaded while the thread still running
code from it, causing a segfault. Unfortunately, it's not possible to be
sure that all tur checker threads have ended during shutdown, without
making them joinable.

However, since libmultipath will never be reinitialized after it has
been uninitialzed, not dlclosing the tur checker DSO once a thread is
started has minimal cost (keeping the DSO code around until the program
exits, which usually happens right after freeing the checkers).

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers.c           | 10 +++++++++-
 libmultipath/checkers.h           |  1 +
 libmultipath/checkers/tur.c       |  1 +
 libmultipath/libmultipath.version |  5 +++++
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 18b1f5eb..35a17f8c 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -22,6 +22,7 @@ struct checker_class {
 	void (*reset)(void);		     /* to reset the global variables */
 	const char **msgtable;
 	short msgtable_size;
+	int keep_dso;
 };
 
 static const char *checker_state_names[PATH_MAX_STATE] = {
@@ -74,7 +75,7 @@ void free_checker_class(struct checker_class *c)
 	list_del(&c->node);
 	if (c->reset)
 		c->reset();
-	if (c->handle) {
+	if (c->handle && !c->keep_dso) {
 		if (dlclose(c->handle) != 0) {
 			condlog(0, "Cannot unload checker %s: %s",
 				c->name, dlerror());
@@ -197,6 +198,13 @@ out:
 	return NULL;
 }
 
+void checker_keep_dso(struct checker * c)
+{
+	if (!c || !c->cls)
+		return;
+	c->cls->keep_dso = 1;
+}
+
 void checker_set_fd (struct checker * c, int fd)
 {
 	if (!c)
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index 9d5f90b9..af5a4006 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -146,6 +146,7 @@ void checker_reset (struct checker *);
 void checker_set_sync (struct checker *);
 void checker_set_async (struct checker *);
 void checker_set_fd (struct checker *, int);
+void checker_keep_dso(struct checker *c);
 void checker_enable (struct checker *);
 void checker_disable (struct checker *);
 int checker_check (struct checker *, int);
diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index e886fcf8..fd58d62a 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -394,6 +394,7 @@ int libcheck_check(struct checker * c)
 		uatomic_set(&ct->running, 1);
 		tur_set_async_timeout(c);
 		setup_thread_attr(&attr, 32 * 1024, 1);
+		checker_keep_dso(c);
 		r = pthread_create(&ct->thread, &attr, tur_thread, ct);
 		pthread_attr_destroy(&attr);
 		if (r) {
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 2e3583f5..04eea300 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -270,3 +270,8 @@ global:
 	dm_prereq;
 	skip_libmp_dm_init;
 } LIBMULTIPATH_4.1.0;
+
+LIBMULTIPATH_4.3.0 {
+global:
+	checker_keep_dso;
+} LIBMULTIPATH_4.2.0;
-- 
2.17.2

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

Re: [dm-devel] [PATCH v2 2/6] libmultipath: add eh_deadline multipath.conf parameter

[Martin Wilck]

On Wed, 2020-11-04 at 00:54 -0600, Benjamin Marzinski wrote:
> There are times a fc rport is never lost, meaning that
> fast_io_fail_tmo
> and dev_loss_tmo never trigger, but scsi commands still hang. This
> can
> cause problems in cases where users have strict timing requirements,
> and
> the easiest way to solve these issues is to set eh_deadline. Since
> it's
> already possible to set fast_io_fail_tmo and dev_loss_tmo from
> multipath.conf, and have multipath take care of setting it correctly
> for
> the scsi devices in sysfs, it makes sense to allow users to set
> eh_deadline here as well.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

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


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

Re: [dm-devel] [PATCH v2 4/6] libmultipath: factor out code to get vpd page data

[Martin Wilck]

On Wed, 2020-11-04 at 00:54 -0600, Benjamin Marzinski wrote:
> A future patch will reuse the code to get the vpd page data, so
> factor
> it out from get_vpd_sgio().
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/discovery.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index a97d2998..95ddbbbd 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1319,11 +1319,10 @@ get_vpd_sysfs (struct udev_device *parent,
> int pg, char * str, int maxlen)
>  	return len;
>  }
>  
> -int
> -get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
> +static int
> +fetch_vpd_page(int fd, int pg, unsigned char *buff)
>  {
> -	int len, buff_len;
> -	unsigned char buff[4096];
> +	int buff_len;
>  
>  	memset(buff, 0x0, 4096);

I don't know ... I think we shouldn't write any new functions making
assumptions about the size of buffers passed to them, even if the
caller is directly next to them in the code.

>  	if (sgio_get_vpd(buff, 4096, fd, pg) < 0) {
> @@ -1344,6 +1343,18 @@ get_vpd_sgio (int fd, int pg, int vend_id,
> char * str, int maxlen)
>  		condlog(3, "vpd pg%02x page truncated", pg);
>  		buff_len = 4096;
>  	}
> +	return buff_len;
> +}
> +
> +int
> +get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
> +{
> +	int len, buff_len;
> +	unsigned char buff[4096];
> +
> +	buff_len = fetch_vpd_page(fd, pg, buff);
> +	if (buff_len < 0)
> +		return buff_len;
>  	if (pg == 0x80)
>  		len = parse_vpd_pg80(buff, str, maxlen);
>  	else if (pg == 0x83)

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer



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

Re: [dm-devel] [PATCH v2 5/6] libmultipath: limit reading 0xc9 vpd page

[Martin Wilck]

On Wed, 2020-11-04 at 00:54 -0600, Benjamin Marzinski wrote:
> Only rdac arrays support 0xC9 vpd page inquiries. All other arrays
> will
> return a failure. Only do the rdac inquiry when detecting array
> capabilities if the array's path checker is explicitly set to rdac,
> or
> the path checker is not set, and the array reports that it supports
> vpd
> page 0xC9 in the Supported VPD Pages (0x00) vpd page.
> 
> Multipath was doing the check if either the path checker was set to
> rdac, or no path checker was set.  This means that for almost all
> non-rdac arrays, multipath was issuing a bad inquiry. This was
> annoying
> users.
> 
> Cc: Steve Schremmer <sschremm@netapp.com>
> Cc: NetApp RDAC team <ng-eseries-upstream-maintainers@netapp.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/discovery.c | 25 +++++++++++++++++++++++++
>  libmultipath/discovery.h |  1 +
>  libmultipath/propsel.c   | 10 ++++++----
>  3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 95ddbbbd..5669690d 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1346,6 +1346,31 @@ fetch_vpd_page(int fd, int pg, unsigned char
> *buff)
>  	return buff_len;
>  }
>  
> +/* heavily based on sg_inq.c from sg3_utils */
> +bool
> +is_vpd_page_supported(int fd, int pg)
> +{
> +	int i, len, buff_len;
> +	unsigned char buff[4096];
> +
> +	buff_len = fetch_vpd_page(fd, 0x00, buff);
> +	if (buff_len < 0)
> +		return false;
> +	if (buff_len < 4) {
> +		condlog(3, "VPD page 00h too short");
> +		return false;
> +	}
> +
> +	len = buff[3] + 4;

SPC-4 says that the page length is a 16-bit value.
You may also want to check buff[1] == 0.

> +	if (len > buff_len)
> +		condlog(3, "vpd page 00h trucated, expected %d, have
> %d",
> +			len, buff_len);
> +	for (i = 4; i < len; ++i)
> +		if (buff[i] == pg)
> +			return true;
> +	return false;
> +}
> +
>  int
>  get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
>  {
> diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
> index 6444887d..d3193daf 100644
> --- a/libmultipath/discovery.h
> +++ b/libmultipath/discovery.h
> @@ -56,6 +56,7 @@ int sysfs_get_asymmetric_access_state(struct path
> *pp,
>  				      char *buff, int buflen);
>  int get_uid(struct path * pp, int path_state, struct udev_device
> *udev,
>  	    int allow_fallback);
> +bool is_vpd_page_supported(int fd, int pg);
>  
>  /*
>   * discovery bitmask
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index fa4ac5d9..f771a830 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -496,13 +496,15 @@ check_rdac(struct path * pp)
>  {
>  	int len;
>  	char buff[44];
> -	const char *checker_name;
> +	const char *checker_name = NULL;
>  
>  	if (pp->bus != SYSFS_BUS_SCSI)
>  		return 0;
> -	/* Avoid ioctl if this is likely not an RDAC array */
> -	if (__do_set_from_hwe(checker_name, pp, checker_name) &&
> -	    strcmp(checker_name, RDAC))
> +	/* Avoid checking 0xc9 if this is likely not an RDAC array */
> +	if (!__do_set_from_hwe(checker_name, pp, checker_name) &&
> +	    !is_vpd_page_supported(pp->fd, 0xC9))
> +		return 0;
> +	if (checker_name && strcmp(checker_name, RDAC))

Do we still need the name check after testing whether 0xc9 is
supported? Well I guess it doesn't harm.

Regards
Martin

>  		return 0;
>  	len = get_vpd_sgio(pp->fd, 0xC9, 0, buff, 44);
>  	if (len <= 0)

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer



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

Re: [dm-devel] [PATCH v2 4/6] libmultipath: factor out code to get vpd page data

[Benjamin Marzinski]

On Wed, Dec 16, 2020 at 09:13:49PM +0000, Martin Wilck wrote:
> On Wed, 2020-11-04 at 00:54 -0600, Benjamin Marzinski wrote:
> > A future patch will reuse the code to get the vpd page data, so
> > factor
> > it out from get_vpd_sgio().
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/discovery.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index a97d2998..95ddbbbd 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1319,11 +1319,10 @@ get_vpd_sysfs (struct udev_device *parent,
> > int pg, char * str, int maxlen)
> >  	return len;
> >  }
> >  
> > -int
> > -get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
> > +static int
> > +fetch_vpd_page(int fd, int pg, unsigned char *buff)
> >  {
> > -	int len, buff_len;
> > -	unsigned char buff[4096];
> > +	int buff_len;
> >  
> >  	memset(buff, 0x0, 4096);
> 
> I don't know ... I think we shouldn't write any new functions making
> assumptions about the size of buffers passed to them, even if the
> caller is directly next to them in the code.
> 

Fair enough. I'll change that.

-Ben

> >  	if (sgio_get_vpd(buff, 4096, fd, pg) < 0) {
> > @@ -1344,6 +1343,18 @@ get_vpd_sgio (int fd, int pg, int vend_id,
> > char * str, int maxlen)
> >  		condlog(3, "vpd pg%02x page truncated", pg);
> >  		buff_len = 4096;
> >  	}
> > +	return buff_len;
> > +}
> > +
> > +int
> > +get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
> > +{
> > +	int len, buff_len;
> > +	unsigned char buff[4096];
> > +
> > +	buff_len = fetch_vpd_page(fd, pg, buff);
> > +	if (buff_len < 0)
> > +		return buff_len;
> >  	if (pg == 0x80)
> >  		len = parse_vpd_pg80(buff, str, maxlen);
> >  	else if (pg == 0x83)
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE  Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix
> Imendörffer
> 

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

Re: [dm-devel] [PATCH v2 5/6] libmultipath: limit reading 0xc9 vpd page

[Benjamin Marzinski]

On Wed, Dec 16, 2020 at 09:18:01PM +0000, Martin Wilck wrote:
> On Wed, 2020-11-04 at 00:54 -0600, Benjamin Marzinski wrote:
> > Only rdac arrays support 0xC9 vpd page inquiries. All other arrays
> > will
> > return a failure. Only do the rdac inquiry when detecting array
> > capabilities if the array's path checker is explicitly set to rdac,
> > or
> > the path checker is not set, and the array reports that it supports
> > vpd
> > page 0xC9 in the Supported VPD Pages (0x00) vpd page.
> > 
> > Multipath was doing the check if either the path checker was set to
> > rdac, or no path checker was set.  This means that for almost all
> > non-rdac arrays, multipath was issuing a bad inquiry. This was
> > annoying
> > users.
> > 
> > Cc: Steve Schremmer <sschremm@netapp.com>
> > Cc: NetApp RDAC team <ng-eseries-upstream-maintainers@netapp.com>
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/discovery.c | 25 +++++++++++++++++++++++++
> >  libmultipath/discovery.h |  1 +
> >  libmultipath/propsel.c   | 10 ++++++----
> >  3 files changed, 32 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index 95ddbbbd..5669690d 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1346,6 +1346,31 @@ fetch_vpd_page(int fd, int pg, unsigned char
> > *buff)
> >  	return buff_len;
> >  }
> >  
> > +/* heavily based on sg_inq.c from sg3_utils */
> > +bool
> > +is_vpd_page_supported(int fd, int pg)
> > +{
> > +	int i, len, buff_len;
> > +	unsigned char buff[4096];
> > +
> > +	buff_len = fetch_vpd_page(fd, 0x00, buff);
> > +	if (buff_len < 0)
> > +		return false;
> > +	if (buff_len < 4) {
> > +		condlog(3, "VPD page 00h too short");
> > +		return false;
> > +	}
> > +
> > +	len = buff[3] + 4;
> 
> SPC-4 says that the page length is a 16-bit value.
> You may also want to check buff[1] == 0.

Makes sense. I'll check these.
 
> > +	if (len > buff_len)
> > +		condlog(3, "vpd page 00h trucated, expected %d, have
> > %d",
> > +			len, buff_len);
> > +	for (i = 4; i < len; ++i)
> > +		if (buff[i] == pg)
> > +			return true;
> > +	return false;
> > +}
> > +
> >  int
> >  get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
> >  {
> > diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
> > index 6444887d..d3193daf 100644
> > --- a/libmultipath/discovery.h
> > +++ b/libmultipath/discovery.h
> > @@ -56,6 +56,7 @@ int sysfs_get_asymmetric_access_state(struct path
> > *pp,
> >  				      char *buff, int buflen);
> >  int get_uid(struct path * pp, int path_state, struct udev_device
> > *udev,
> >  	    int allow_fallback);
> > +bool is_vpd_page_supported(int fd, int pg);
> >  
> >  /*
> >   * discovery bitmask
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index fa4ac5d9..f771a830 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -496,13 +496,15 @@ check_rdac(struct path * pp)
> >  {
> >  	int len;
> >  	char buff[44];
> > -	const char *checker_name;
> > +	const char *checker_name = NULL;
> >  
> >  	if (pp->bus != SYSFS_BUS_SCSI)
> >  		return 0;
> > -	/* Avoid ioctl if this is likely not an RDAC array */
> > -	if (__do_set_from_hwe(checker_name, pp, checker_name) &&
> > -	    strcmp(checker_name, RDAC))
> > +	/* Avoid checking 0xc9 if this is likely not an RDAC array */
> > +	if (!__do_set_from_hwe(checker_name, pp, checker_name) &&
> > +	    !is_vpd_page_supported(pp->fd, 0xC9))
> > +		return 0;
> > +	if (checker_name && strcmp(checker_name, RDAC))
> 
> Do we still need the name check after testing whether 0xc9 is
> supported? Well I guess it doesn't harm.

I understand that people could want to use the device section checker
configuration as the fallback. But giving priority to an explicit
checker configuration hasn't caused any problems here so far, so I think
we should continue to do so. 

-Ben

> Regards
> Martin
> 
> >  		return 0;
> >  	len = get_vpd_sgio(pp->fd, 0xC9, 0, buff, 44);
> >  	if (len <= 0)
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE  Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix
> Imendörffer
> 

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

Re: [dm-devel] [PATCH v2 5/6] libmultipath: limit reading 0xc9 vpd page

[Martin Wilck]

On Wed, 2020-12-16 at 17:56 -0600, Benjamin Marzinski wrote:
> On Wed, Dec 16, 2020 at 09:18:01PM +0000, Martin Wilck wrote:
> > 
> > Do we still need the name check after testing whether 0xc9 is
> > supported? Well I guess it doesn't harm.
> 
> I understand that people could want to use the device section checker
> configuration as the fallback. But giving priority to an explicit
> checker configuration hasn't caused any problems here so far, so I
> think
> we should continue to do so. 

ok.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer



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