[PATCH v3 00/10] multipath-tools: avoid I/O during uevent processing

[PATCH v3 00/10] multipath-tools: avoid I/O during uevent processing

[Martin Wilck]

Hi Christophe, hi Ben,

the bulk of this series attempts to avoid unnecessary I/O for "multipath -u"
calls from udev rules. I came up with it in a recent attempt to debug
avoid "multipath -u" hangups during boot. I found that the detect_alua()
call in the get_refwwid->pathinfo->scsi_ioctl_pathinfo call sequence would
cause an sg ioctl to be sent for every "multipath -u" invocation. That's
obvious nonsense, we don't need to detect ALUA capabilities to find out
if a device is a potential multipath member.

So, rather than setting pp->tpgs in pathinfo, I resorted to a "lazy"
approach where ALUA capabilities are probed when they are needed, no
sooner. I also cleaned up the ALUA code a bit (still some more to go),
and made it attempt to read INQUIRY data from sysfs.

Regards,
Martin

Changes v2->v3:

 - added 8/10 "tidy up do_set_from_hwe() with statement expression"
   (Hannes), renumber subsequent patches accordingly.
 - 9/10: cleanup ugly macro usage; do retrieve VPD for "unknown"
   SCSI storage devices
 
Changes v1->v2:

 - 6/9: remove one remaining direct access to pp->tpgs
 - 9/9: use non-personal email address for maintainer

Martin Wilck (10):
  libmultipath: add sysfs_get_inquiry()
  libmultipath: alua: make API more consistent
  libmultipath: alua: try to retrieve inquiry data from sysfs
  libmultipath: constify sysfs_get_timeout()
  libmultipath: detect_alua(): use system timeout
  libmultipath: lazy tpgs probing
  libmultipath: don't link alua_rtpg.o twice
  libmultipath: tidy up do_set_from_hwe() with statement expression
  libmultipath: check_rdac(): pre-check in hwtable
  libmultipath: hwtable: add Lenovo DE series

 libmultipath/discovery.c              | 46 +++++++++++++++++-------
 libmultipath/discovery.h              | 10 +++---
 libmultipath/hwtable.c                | 20 +++++++++++
 libmultipath/prioritizers/Makefile    |  3 --
 libmultipath/prioritizers/alua.c      |  4 +--
 libmultipath/prioritizers/alua_rtpg.c | 45 +++++++++++++++++------
 libmultipath/prioritizers/alua_rtpg.h |  7 ++--
 libmultipath/propsel.c                | 52 ++++++++++++++++++---------
 multipathd/main.c                     |  7 ++--
 9 files changed, 139 insertions(+), 55 deletions(-)

-- 
2.21.0

[PATCH v3 01/10] libmultipath: add sysfs_get_inquiry()

[Martin Wilck]

Provide a utility function to retrieve inquiry data from
sysfs, like we do for VPDs.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 23 ++++++++++++++++++-----
 libmultipath/discovery.h |  6 ++++--
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 10bd8cd6..65d651d4 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -218,12 +218,11 @@ declare_sysfs_get_str(vendor);
 declare_sysfs_get_str(model);
 declare_sysfs_get_str(rev);
 
-ssize_t
-sysfs_get_vpd (struct udev_device * udev, int pg,
-	       unsigned char * buff, size_t len)
+static ssize_t
+sysfs_get_binary (struct udev_device * udev, const char *attrname,
+		  unsigned char *buff, size_t len)
 {
 	ssize_t attr_len;
-	char attrname[9];
 	const char * devname;
 
 	if (!udev) {
@@ -232,7 +231,6 @@ sysfs_get_vpd (struct udev_device * udev, int pg,
 	}
 
 	devname = udev_device_get_sysname(udev);
-	sprintf(attrname, "vpd_pg%02x", pg);
 	attr_len = sysfs_bin_attr_get_value(udev, attrname, buff, len);
 	if (attr_len < 0) {
 		condlog(3, "%s: attribute %s not found in sysfs",
@@ -242,6 +240,21 @@ sysfs_get_vpd (struct udev_device * udev, int pg,
 	return attr_len;
 }
 
+ssize_t sysfs_get_vpd(struct udev_device * udev, unsigned char pg,
+		      unsigned char *buff, size_t len)
+{
+	char attrname[9];
+
+	snprintf(attrname, sizeof(attrname), "vpd_pg%02x", pg);
+	return sysfs_get_binary(udev, attrname, buff, len);
+}
+
+ssize_t sysfs_get_inquiry(struct udev_device * udev,
+			  unsigned char *buff, size_t len)
+{
+	return sysfs_get_binary(udev, "inquiry", buff, len);
+}
+
 int
 sysfs_get_timeout(struct path *pp, unsigned int *timeout)
 {
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index 9aacf75b..7cacbb6c 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -48,8 +48,10 @@ int sysfs_get_host_pci_name(const struct path *pp, char *pci_name);
 int sysfs_get_iscsi_ip_address(const struct path *pp, char *ip_address);
 int sysfs_get_host_adapter_name(const struct path *pp,
 				char *adapter_name);
-ssize_t sysfs_get_vpd (struct udev_device * udev, int pg, unsigned char * buff,
-		       size_t len);
+ssize_t sysfs_get_vpd (struct udev_device *udev, unsigned char pg,
+		       unsigned char *buff, size_t len);
+ssize_t sysfs_get_inquiry(struct udev_device *udev,
+			  unsigned char *buff, size_t len);
 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);
-- 
2.21.0

[PATCH v3 02/10] libmultipath: alua: make API more consistent

[Martin Wilck]

Let all alua functions take "const struct path *" as first
argument.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c              |  4 ++--
 libmultipath/prioritizers/alua.c      |  4 ++--
 libmultipath/prioritizers/alua_rtpg.c | 28 +++++++++++++++++----------
 libmultipath/prioritizers/alua_rtpg.h |  7 ++++---
 libmultipath/propsel.c                |  2 +-
 5 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 65d651d4..6b4a420b 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -839,12 +839,12 @@ detect_alua(struct path * pp, struct config *conf)
 	int tpgs;
 	unsigned int timeout = conf->checker_timeout;
 
-	if ((tpgs = get_target_port_group_support(pp->fd, timeout)) <= 0) {
+	if ((tpgs = get_target_port_group_support(pp, timeout)) <= 0) {
 		pp->tpgs = TPGS_NONE;
 		return;
 	}
 	ret = get_target_port_group(pp, timeout);
-	if (ret < 0 || get_asymmetric_access_state(pp->fd, ret, timeout) < 0) {
+	if (ret < 0 || get_asymmetric_access_state(pp, ret, timeout) < 0) {
 		pp->tpgs = TPGS_NONE;
 		return;
 	}
diff --git a/libmultipath/prioritizers/alua.c b/libmultipath/prioritizers/alua.c
index b24e2d48..0ab06e2b 100644
--- a/libmultipath/prioritizers/alua.c
+++ b/libmultipath/prioritizers/alua.c
@@ -58,7 +58,7 @@ get_alua_info(struct path * pp, unsigned int timeout)
 
 	tpg = get_target_port_group(pp, timeout);
 	if (tpg < 0) {
-		rc = get_target_port_group_support(pp->fd, timeout);
+		rc = get_target_port_group_support(pp, timeout);
 		if (rc < 0)
 			return -ALUA_PRIO_TPGS_FAILED;
 		if (rc == TPGS_NONE)
@@ -66,7 +66,7 @@ get_alua_info(struct path * pp, unsigned int timeout)
 		return -ALUA_PRIO_RTPG_FAILED;
 	}
 	condlog(3, "%s: reported target port group is %i", pp->dev, tpg);
-	rc = get_asymmetric_access_state(pp->fd, tpg, timeout);
+	rc = get_asymmetric_access_state(pp, tpg, timeout);
 	if (rc < 0) {
 		condlog(2, "%s: get_asymmetric_access_state returned %d",
 			__func__, rc);
diff --git a/libmultipath/prioritizers/alua_rtpg.c b/libmultipath/prioritizers/alua_rtpg.c
index 811ce7a2..d9215a88 100644
--- a/libmultipath/prioritizers/alua_rtpg.c
+++ b/libmultipath/prioritizers/alua_rtpg.c
@@ -135,9 +135,9 @@ scsi_error(struct sg_io_hdr *hdr, int opcode)
 /*
  * Helper function to setup and run a SCSI inquiry command.
  */
-int
-do_inquiry(int fd, int evpd, unsigned int codepage,
-	   void *resp, int resplen, unsigned int timeout)
+static int
+do_inquiry_sg(int fd, int evpd, unsigned int codepage,
+	      void *resp, int resplen, unsigned int timeout)
 {
 	struct inquiry_command	cmd;
 	struct sg_io_hdr	hdr;
@@ -185,18 +185,24 @@ retry:
 	return 0;
 }
 
+int do_inquiry(const struct path *pp, int evpd, unsigned int codepage,
+	       void *resp, int resplen, unsigned int timeout)
+{
+	return do_inquiry_sg(pp->fd, evpd, codepage, resp, resplen, timeout);
+}
+
 /*
  * This function returns the support for target port groups by evaluating the
  * data returned by the standard inquiry command.
  */
 int
-get_target_port_group_support(int fd, unsigned int timeout)
+get_target_port_group_support(const struct path *pp, unsigned int timeout)
 {
 	struct inquiry_data	inq;
 	int			rc;
 
 	memset((unsigned char *)&inq, 0, sizeof(inq));
-	rc = do_inquiry(fd, 0, 0x00, &inq, sizeof(inq), timeout);
+	rc = do_inquiry(pp, 0, 0x00, &inq, sizeof(inq), timeout);
 	if (!rc) {
 		rc = inquiry_data_get_tpgs(&inq);
 	}
@@ -205,7 +211,7 @@ get_target_port_group_support(int fd, unsigned int timeout)
 }
 
 static int
-get_sysfs_pg83(struct path *pp, unsigned char *buff, int buflen)
+get_sysfs_pg83(const struct path *pp, unsigned char *buff, int buflen)
 {
 	struct udev_device *parent = pp->udev;
 
@@ -224,7 +230,7 @@ get_sysfs_pg83(struct path *pp, unsigned char *buff, int buflen)
 }
 
 int
-get_target_port_group(struct path * pp, unsigned int timeout)
+get_target_port_group(const struct path * pp, unsigned int timeout)
 {
 	unsigned char		*buf;
 	struct vpd83_data *	vpd83;
@@ -245,7 +251,7 @@ get_target_port_group(struct path * pp, unsigned int timeout)
 	rc = get_sysfs_pg83(pp, buf, buflen);
 
 	if (rc < 0) {
-		rc = do_inquiry(pp->fd, 1, 0x83, buf, buflen, timeout);
+		rc = do_inquiry(pp, 1, 0x83, buf, buflen, timeout);
 		if (rc < 0)
 			goto out;
 
@@ -263,7 +269,7 @@ get_target_port_group(struct path * pp, unsigned int timeout)
 			}
 			buflen = scsi_buflen;
 			memset(buf, 0, buflen);
-			rc = do_inquiry(pp->fd, 1, 0x83, buf, buflen, timeout);
+			rc = do_inquiry(pp, 1, 0x83, buf, buflen, timeout);
 			if (rc < 0)
 				goto out;
 		}
@@ -341,7 +347,8 @@ retry:
 }
 
 int
-get_asymmetric_access_state(int fd, unsigned int tpg, unsigned int timeout)
+get_asymmetric_access_state(const struct path *pp, unsigned int tpg,
+			    unsigned int timeout)
 {
 	unsigned char		*buf;
 	struct rtpg_data *	tpgd;
@@ -349,6 +356,7 @@ get_asymmetric_access_state(int fd, unsigned int tpg, unsigned int timeout)
 	int			rc;
 	int			buflen;
 	uint64_t		scsi_buflen;
+	int fd = pp->fd;
 
 	buflen = 4096;
 	buf = (unsigned char *)malloc(buflen);
diff --git a/libmultipath/prioritizers/alua_rtpg.h b/libmultipath/prioritizers/alua_rtpg.h
index 35cffaf3..675709ff 100644
--- a/libmultipath/prioritizers/alua_rtpg.h
+++ b/libmultipath/prioritizers/alua_rtpg.h
@@ -22,8 +22,9 @@
 #define RTPG_RTPG_FAILED			3
 #define RTPG_TPG_NOT_FOUND			4
 
-int get_target_port_group_support(int fd, unsigned int timeout);
-int get_target_port_group(struct path * pp, unsigned int timeout);
-int get_asymmetric_access_state(int fd, unsigned int tpg, unsigned int timeout);
+int get_target_port_group_support(const struct path *pp, unsigned int timeout);
+int get_target_port_group(const struct path *pp, unsigned int timeout);
+int get_asymmetric_access_state(const struct path *pp,
+				unsigned int tpg, unsigned int timeout);
 
 #endif /* __RTPG_H__ */
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 98068f34..624dc6ef 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -632,7 +632,7 @@ out:
 		unsigned int timeout = conf->checker_timeout;
 
 		if(!pp->tpgs &&
-		   (tpgs = get_target_port_group_support(pp->fd, timeout)) >= 0)
+		   (tpgs = get_target_port_group_support(pp, timeout)) >= 0)
 			pp->tpgs = tpgs;
 	}
 	condlog(3, "%s: prio = %s %s", pp->dev, prio_name(p), origin);
-- 
2.21.0

[PATCH v3 03/10] libmultipath: alua: try to retrieve inquiry data from sysfs

[Martin Wilck]

This can avoid IO while configuring the path prioritizer.
The alua prioritizer avoids reading from sysfs for a reason
(see commit 7e2f46d3), but this applies only to RTPG / STPG,
not to INQUIRY calls.

Cc: Steve.Schremmer@netapp.com
Cc: NetApp RDAC team <ng-eseries-upstream-maintainers@netapp.com>
Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/prioritizers/alua_rtpg.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/libmultipath/prioritizers/alua_rtpg.c b/libmultipath/prioritizers/alua_rtpg.c
index d9215a88..271a019d 100644
--- a/libmultipath/prioritizers/alua_rtpg.c
+++ b/libmultipath/prioritizers/alua_rtpg.c
@@ -188,6 +188,23 @@ retry:
 int do_inquiry(const struct path *pp, int evpd, unsigned int codepage,
 	       void *resp, int resplen, unsigned int timeout)
 {
+	struct udev_device *ud;
+
+	ud = udev_device_get_parent_with_subsystem_devtype(pp->udev, "scsi",
+							   "scsi_device");
+	if (ud != NULL) {
+		int rc;
+
+		if (!evpd)
+			rc = sysfs_get_inquiry(ud, resp, resplen);
+		else
+			rc = sysfs_get_vpd(ud, codepage, resp, resplen);
+
+		if (rc >= 0) {
+			PRINT_HEX((unsigned char *) resp, resplen);
+			return 0;
+		}
+	}
 	return do_inquiry_sg(pp->fd, evpd, codepage, resp, resplen, timeout);
 }
 
-- 
2.21.0

[PATCH v3 04/10] libmultipath: constify sysfs_get_timeout()

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 2 +-
 libmultipath/discovery.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 6b4a420b..270dedc9 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -256,7 +256,7 @@ ssize_t sysfs_get_inquiry(struct udev_device * udev,
 }
 
 int
-sysfs_get_timeout(struct path *pp, unsigned int *timeout)
+sysfs_get_timeout(const struct path *pp, unsigned int *timeout)
 {
 	const char *attr = NULL;
 	const char *subsys;
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index 7cacbb6c..a9a28add 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -43,7 +43,7 @@ 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, int checkint);
-int sysfs_get_timeout(struct path *pp, unsigned int *timeout);
+int sysfs_get_timeout(const struct path *pp, unsigned int *timeout);
 int sysfs_get_host_pci_name(const struct path *pp, char *pci_name);
 int sysfs_get_iscsi_ip_address(const struct path *pp, char *ip_address);
 int sysfs_get_host_adapter_name(const struct path *pp,
-- 
2.21.0

[PATCH v3 05/10] libmultipath: detect_alua(): use system timeout

[Martin Wilck]

This is not the path checker - we don't need to use the
configured checker timeout here. This makes it possible to
call this function without a current (struct config *).

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 270dedc9..b2a43e86 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -833,11 +833,14 @@ get_serial (char * str, int maxlen, int fd)
 }
 
 static void
-detect_alua(struct path * pp, struct config *conf)
+detect_alua(struct path * pp)
 {
 	int ret;
 	int tpgs;
-	unsigned int timeout = conf->checker_timeout;
+	unsigned int timeout;
+
+	if (sysfs_get_timeout(pp, &timeout) <= 0)
+		timeout = DEF_TIMEOUT;
 
 	if ((tpgs = get_target_port_group_support(pp, timeout)) <= 0) {
 		pp->tpgs = TPGS_NONE;
@@ -1519,7 +1522,7 @@ scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
 	const char *attr_path = NULL;
 
 	if (pp->tpgs == TPGS_UNDEF)
-		detect_alua(pp, conf);
+		detect_alua(pp);
 
 	if (!(mask & DI_SERIAL))
 		return;
-- 
2.21.0

Re: [PATCH v3 05/10] libmultipath: detect_alua(): use system timeout

[Benjamin Marzinski]

On Mon, Mar 18, 2019 at 12:24:40PM +0100, Martin Wilck wrote:
> This is not the path checker - we don't need to use the
> configured checker timeout here. This makes it possible to
> call this function without a current (struct config *).

I feel like checker_timeout is badly named, since we use if for a lot of
timeouts that aren't in the checker.  In general, I feel like we should
allow users to configure how long they want these sorts of calls to wait
for a response. I get why you changed this, but it may be worth thinking
about if we should deal with this timeout like we deal with verbosity,
where, we track it outside of the config structure.

But that doesn't need to be dealt with now, so

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> 
 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 270dedc9..b2a43e86 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -833,11 +833,14 @@ get_serial (char * str, int maxlen, int fd)
>  }
>  
>  static void
> -detect_alua(struct path * pp, struct config *conf)
> +detect_alua(struct path * pp)
>  {
>  	int ret;
>  	int tpgs;
> -	unsigned int timeout = conf->checker_timeout;
> +	unsigned int timeout;
> +
> +	if (sysfs_get_timeout(pp, &timeout) <= 0)
> +		timeout = DEF_TIMEOUT;
>  
>  	if ((tpgs = get_target_port_group_support(pp, timeout)) <= 0) {
>  		pp->tpgs = TPGS_NONE;
> @@ -1519,7 +1522,7 @@ scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
>  	const char *attr_path = NULL;
>  
>  	if (pp->tpgs == TPGS_UNDEF)
> -		detect_alua(pp, conf);
> +		detect_alua(pp);
>  
>  	if (!(mask & DI_SERIAL))
>  		return;
> -- 
> 2.21.0

[PATCH v3 06/10] libmultipath: lazy tpgs probing

[Martin Wilck]

Provide a "getter" function that can be used to probe tpgs lazily.
This way we don't need to send an RTPG in the pathinfo() call
chain (e.g. in "multipath -u"). With this in place, no "user"
code should access pp->tpgs directly any more.

Moreover, in select_prio(), in the case where the alua checker
was statically configured, rather then calling into the alua
code directly, use get_tpgs(), which does all the proper error
checking, and fall back to const prio if it fails.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 10 +++++++---
 libmultipath/discovery.h |  2 +-
 libmultipath/propsel.c   | 25 +++++++++++++++----------
 multipathd/main.c        |  7 ++++---
 4 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index b2a43e86..b51a37b2 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -854,6 +854,13 @@ detect_alua(struct path * pp)
 	pp->tpgs = tpgs;
 }
 
+int path_get_tpgs(struct path *pp)
+{
+	if (pp->tpgs == TPGS_UNDEF)
+		detect_alua(pp);
+	return pp->tpgs;
+}
+
 #define DEFAULT_SGIO_LEN 254
 
 /* Query VPD page @pg. Returns number of INQUIRY bytes
@@ -1521,9 +1528,6 @@ scsi_ioctl_pathinfo (struct path * pp, struct config *conf, int mask)
 	struct udev_device *parent;
 	const char *attr_path = NULL;
 
-	if (pp->tpgs == TPGS_UNDEF)
-		detect_alua(pp);
-
 	if (!(mask & DI_SERIAL))
 		return;
 
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index a9a28add..01789339 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -31,7 +31,7 @@
 struct config;
 
 int path_discovery (vector pathvec, int flag);
-
+int path_get_tpgs(struct path *pp); /* This function never returns TPGS_UNDEF */
 int do_tur (char *);
 int path_offline (struct path *);
 int get_state (struct path * pp, struct config * conf, int daemon, int state);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 624dc6ef..27474f05 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -425,7 +425,7 @@ int select_hwhandler(struct config *conf, struct multipath *mp)
 	dh_state = &handler[2];
 
 	vector_foreach_slot(mp->paths, pp, i)
-		all_tpgs = all_tpgs && (pp->tpgs > 0);
+		all_tpgs = all_tpgs && (path_get_tpgs(pp) > 0);
 	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
 		vector_foreach_slot(mp->paths, pp, i) {
 			if (get_dh_state(pp, dh_state, sizeof(handler) - 2) > 0
@@ -490,7 +490,7 @@ int select_checker(struct config *conf, struct path *pp)
 		if (check_rdac(pp)) {
 			ckr_name = RDAC;
 			goto out;
-		} else if (pp->tpgs > 0) {
+		} else if (path_get_tpgs(pp) != TPGS_NONE) {
 			ckr_name = TUR;
 			goto out;
 		}
@@ -552,6 +552,7 @@ detect_prio(struct config *conf, struct path * pp)
 	struct prio *p = &pp->prio;
 	char buff[512];
 	char *default_prio;
+	int tpgs;
 
 	switch(pp->bus) {
 	case SYSFS_BUS_NVME:
@@ -560,9 +561,10 @@ detect_prio(struct config *conf, struct path * pp)
 		default_prio = PRIO_ANA;
 		break;
 	case SYSFS_BUS_SCSI:
-		if (pp->tpgs <= 0)
+		tpgs = path_get_tpgs(pp);
+		if (tpgs == TPGS_NONE)
 			return;
-		if ((pp->tpgs == 2 || !check_rdac(pp)) &&
+		if ((tpgs == TPGS_EXPLICIT || !check_rdac(pp)) &&
 		    sysfs_get_asymmetric_access_state(pp, buff, 512) >= 0)
 			default_prio = PRIO_SYSFS;
 		else
@@ -607,6 +609,7 @@ int select_prio(struct config *conf, struct path *pp)
 	const char *origin;
 	struct mpentry * mpe;
 	struct prio * p = &pp->prio;
+	int log_prio = 3;
 
 	if (pp->detect_prio == DETECT_PRIO_ON) {
 		detect_prio(conf, pp);
@@ -628,14 +631,16 @@ out:
 	 * fetch tpgs mode for alua, if its not already obtained
 	 */
 	if (!strncmp(prio_name(p), PRIO_ALUA, PRIO_NAME_LEN)) {
-		int tpgs = 0;
-		unsigned int timeout = conf->checker_timeout;
+		int tpgs = path_get_tpgs(pp);
 
-		if(!pp->tpgs &&
-		   (tpgs = get_target_port_group_support(pp, timeout)) >= 0)
-			pp->tpgs = tpgs;
+		if (tpgs == TPGS_NONE) {
+			prio_get(conf->multipath_dir,
+				 p, DEFAULT_PRIO, DEFAULT_PRIO_ARGS);
+			origin = "(setting: emergency fallback - alua failed)";
+			log_prio = 1;
+		}
 	}
-	condlog(3, "%s: prio = %s %s", pp->dev, prio_name(p), origin);
+	condlog(log_prio, "%s: prio = %s %s", pp->dev, prio_name(p), origin);
 	condlog(3, "%s: prio args = \"%s\" %s", pp->dev, prio_args(p), origin);
 	return 0;
 }
diff --git a/multipathd/main.c b/multipathd/main.c
index fb520b64..269a96ec 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -936,7 +936,8 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
 	}
 	if (mpp && mpp->wait_for_udev &&
 	    (pathcount(mpp, PATH_UP) > 0 ||
-	     (pathcount(mpp, PATH_GHOST) > 0 && pp->tpgs != TPGS_IMPLICIT &&
+	     (pathcount(mpp, PATH_GHOST) > 0 &&
+	      path_get_tpgs(pp) != TPGS_IMPLICIT &&
 	      mpp->ghost_delay_tick <= 0))) {
 		/* if wait_for_udev is set and valid paths exist */
 		condlog(3, "%s: delaying path addition until %s is fully initialized",
@@ -2106,8 +2107,8 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	 * paths if there are no other active paths in map.
 	 */
 	disable_reinstate = (newstate == PATH_GHOST &&
-			    pp->mpp->nr_active == 0 &&
-			    pp->tpgs == TPGS_IMPLICIT) ? 1 : 0;
+			     pp->mpp->nr_active == 0 &&
+			     path_get_tpgs(pp) == TPGS_IMPLICIT) ? 1 : 0;
 
 	pp->chkrstate = newstate;
 	if (newstate != pp->state) {
-- 
2.21.0

[PATCH v3 07/10] libmultipath: don't link alua_rtpg.o twice

[Martin Wilck]

We link this already to libmultipath.so. Therefore, no need
to link ti to libprioalua.so, too.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/prioritizers/Makefile | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/libmultipath/prioritizers/Makefile b/libmultipath/prioritizers/Makefile
index 4d80c20c..9d0fe03c 100644
--- a/libmultipath/prioritizers/Makefile
+++ b/libmultipath/prioritizers/Makefile
@@ -28,9 +28,6 @@ endif
 
 all: $(LIBS)
 
-libprioalua.so: alua.o alua_rtpg.o
-	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
-
 libpriopath_latency.so: path_latency.o  ../checkers/libsg.o
 	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ -lm
 
-- 
2.21.0

[PATCH v3 08/10] libmultipath: tidy up do_set_from_hwe() with statement expression

[Martin Wilck]

propsel.c has a lot of "funky" macros making assumptions about
variable and label names in callers. This one is particularly
ugly. As a first tidy-up step, split out a statement expression
which can be called cleanly from code that doesn't have said
variables and labels.

Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/propsel.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 27474f05..caf55b68 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -45,22 +45,30 @@ do {									\
 	}								\
 } while(0)
 
-#define do_set_from_vec(type, var, src, dest, msg)			\
-do {									\
+#define __do_set_from_vec(type, var, src, dest)				\
+({									\
 	type *_p;							\
+	bool _found = false;						\
 	int i;								\
 									\
 	vector_foreach_slot(src, _p, i) {				\
 		if (_p->var) {						\
 			dest = _p->var;					\
-			origin = msg;					\
-			goto out;					\
+			_found = true;					\
+			break;						\
 		}							\
 	}								\
-} while (0)
+	_found;								\
+})
+
+#define __do_set_from_hwe(var, src, dest) \
+	__do_set_from_vec(struct hwentry, var, (src)->hwe, dest)
 
-#define do_set_from_hwe(var, src, dest, msg) \
-	do_set_from_vec(struct hwentry, var, src->hwe, dest, msg)
+#define do_set_from_hwe(var, src, dest, msg)				\
+	if (__do_set_from_hwe(var, src, dest)) {			\
+		origin = msg;						\
+		goto out;						\
+	}
 
 static const char default_origin[] = "(setting: multipath internal)";
 static const char hwe_origin[] =
-- 
2.21.0

[PATCH v3 09/10] libmultipath: check_rdac(): pre-check in hwtable

[Martin Wilck]

Currently check_rdac() always runs an SG_IO for VPD 0xc9 to check
if the storage supports RDAC. This is an extra IO, and may cause
annoying error messages on the storage side for non-RDAC arrays.
Do not use the RDAC override for arrays that have legacy configuration
to use a checker other than "rdac". For "unkown" devices with no checker
configured in either the hwtable or multipath.conf, the VPD call will
be tried.

Cc: Steve.Schremmer@netapp.com
Cc: NetApp RDAC team <ng-eseries-upstream-maintainers@netapp.com>
Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/propsel.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index caf55b68..e6263e9b 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -478,9 +478,14 @@ check_rdac(struct path * pp)
 {
 	int len;
 	char buff[44];
+	const char *checker_name;
 
 	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))
+		return 0;
 	len = get_vpd_sgio(pp->fd, 0xC9, buff, 44);
 	if (len <= 0)
 		return 0;
-- 
2.21.0

[PATCH v3 10/10] libmultipath: hwtable: add Lenovo DE series

[Martin Wilck]

I got this from Steven.

Cc: Steve.Schremmer@netapp.com
Cc: NetApp RDAC team <ng-eseries-upstream-maintainers@netapp.com>
Cc: xose.vazquez@gmail.com
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/hwtable.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
index d3a8d9ba..b8285818 100644
--- a/libmultipath/hwtable.c
+++ b/libmultipath/hwtable.c
@@ -701,6 +701,26 @@ static struct hwentry default_hw[] = {
 		.no_path_retry = (300 / DEFAULT_CHECKINT),
 		.prio_name     = PRIO_ALUA,
 	},
+        /*
+         * Lenovo
+         */
+        {
+                /*
+		 * DE Series
+		 *
+		 * Maintainer: ng-eseries-upstream-maintainers@netapp.com
+		 */
+                .vendor        = "LENOVO",
+                .product       = "DE_Series",
+                .bl_product    = "Universal Xport",
+                .pgpolicy      = GROUP_BY_PRIO,
+                .checker_name  = RDAC,
+                .features      = "2 pg_init_retries 50",
+                .hwhandler     = "1 rdac",
+                .prio_name     = PRIO_RDAC,
+                .pgfailback    = -FAILBACK_IMMEDIATE,
+                .no_path_retry = 30,
+        },
 	/*
 	 * NetApp
 	 */
-- 
2.21.0

Re: [PATCH v3 10/10] libmultipath: hwtable: add Lenovo DE series

[Schremmer, Steven]

> From: Martin Wilck <mwilck@suse.com>
> Sent: Monday, March 18, 2019 6:25 AM
> To: Christophe Varoqui <christophe.varoqui@opensvc.com>; Benjamin Marzinski <bmarzins@redhat.com>
> Cc: dm-devel@redhat.com; Martin Wilck <mwilck@suse.com>; Schremmer, Steven
> <Steve.Schremmer@netapp.com>; ng-eseries-upstream-maintainers <ng-eseries-upstream-
> maintainers@netapp.com>; xose.vazquez@gmail.com
> Subject: [PATCH v3 10/10] libmultipath: hwtable: add Lenovo DE series
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/hwtable.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
> index d3a8d9ba..b8285818 100644
> --- a/libmultipath/hwtable.c
> +++ b/libmultipath/hwtable.c
> @@ -701,6 +701,26 @@ static struct hwentry default_hw[] = {
>                 .no_path_retry = (300 / DEFAULT_CHECKINT),
>                 .prio_name     = PRIO_ALUA,
>         },
> +        /*
> +         * Lenovo
> +         */
> +        {
> +                /*
> +                * DE Series
> +                *
> +                * Maintainer: ng-eseries-upstream-maintainers@netapp.com
> +                */
> +                .vendor        = "LENOVO",
> +                .product       = "DE_Series",
> +                .bl_product    = "Universal Xport",
> +                .pgpolicy      = GROUP_BY_PRIO,
> +                .checker_name  = RDAC,
> +                .features      = "2 pg_init_retries 50",
> +                .hwhandler     = "1 rdac",
> +                .prio_name     = PRIO_RDAC,
> +                .pgfailback    = -FAILBACK_IMMEDIATE,
> +                .no_path_retry = 30,
> +        },
>         /*
>          * NetApp
>          */
> --
> 2.21.0

Looks good.

Reviewed-by: Steven Schremmer <steven.schremmer@netapp.com>

Thanks for submitting it,
Steve

Re: [PATCH v3 10/10] libmultipath: hwtable: add Lenovo DE series

[Schremmer, Steven]

> From: Schremmer, Steven <Steve.Schremmer@netapp.com>
> > From: Martin Wilck <mwilck@suse.com>
> > Sent: Monday, March 18, 2019 6:25 AM
> > To: Christophe Varoqui <christophe.varoqui@opensvc.com>; Benjamin Marzinski <bmarzins@redhat.com>
> > Cc: dm-devel@redhat.com; Martin Wilck <mwilck@suse.com>; Schremmer, Steven
> > <Steve.Schremmer@netapp.com>; ng-eseries-upstream-maintainers <ng-eseries-upstream-
> > maintainers@netapp.com>; xose.vazquez@gmail.com
> > Subject: [PATCH v3 10/10] libmultipath: hwtable: add Lenovo DE series
> >
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/hwtable.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
> > index d3a8d9ba..b8285818 100644
> > --- a/libmultipath/hwtable.c
> > +++ b/libmultipath/hwtable.c
> > @@ -701,6 +701,26 @@ static struct hwentry default_hw[] = {
> >                 .no_path_retry = (300 / DEFAULT_CHECKINT),
> >                 .prio_name     = PRIO_ALUA,
> >         },
> > +        /*
> > +         * Lenovo
> > +         */
> > +        {
> > +                /*
> > +                * DE Series
> > +                *
> > +                * Maintainer: ng-eseries-upstream-maintainers@netapp.com
> > +                */
> > +                .vendor        = "LENOVO",
> > +                .product       = "DE_Series",
> > +                .bl_product    = "Universal Xport",
> > +                .pgpolicy      = GROUP_BY_PRIO,
> > +                .checker_name  = RDAC,
> > +                .features      = "2 pg_init_retries 50",
> > +                .hwhandler     = "1 rdac",
> > +                .prio_name     = PRIO_RDAC,
> > +                .pgfailback    = -FAILBACK_IMMEDIATE,
> > +                .no_path_retry = 30,
> > +        },
> >         /*
> >          * NetApp
> >          */
> > --
> > 2.21.0
> 
> Looks good.
> 
> Reviewed-by: Steven Schremmer <steven.schremmer@netapp.com>
> 
> Thanks for submitting it,
> Steve

Did this patch get accepted?

Thanks,
Steve

Re: [PATCH v3 00/10] multipath-tools: avoid I/O during uevent processing

[Benjamin Marzinski]

On Mon, Mar 18, 2019 at 12:24:35PM +0100, Martin Wilck wrote:
> Hi Christophe, hi Ben,
> 
> the bulk of this series attempts to avoid unnecessary I/O for "multipath -u"
> calls from udev rules. I came up with it in a recent attempt to debug
> avoid "multipath -u" hangups during boot. I found that the detect_alua()
> call in the get_refwwid->pathinfo->scsi_ioctl_pathinfo call sequence would
> cause an sg ioctl to be sent for every "multipath -u" invocation. That's
> obvious nonsense, we don't need to detect ALUA capabilities to find out
> if a device is a potential multipath member.
> 
> So, rather than setting pp->tpgs in pathinfo, I resorted to a "lazy"
> approach where ALUA capabilities are probed when they are needed, no
> sooner. I also cleaned up the ALUA code a bit (still some more to go),
> and made it attempt to read INQUIRY data from sysfs.
> 
> Regards,
> Martin

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

for the set.
 
> Changes v2->v3:
> 
>  - added 8/10 "tidy up do_set_from_hwe() with statement expression"
>    (Hannes), renumber subsequent patches accordingly.
>  - 9/10: cleanup ugly macro usage; do retrieve VPD for "unknown"
>    SCSI storage devices
>  
> Changes v1->v2:
> 
>  - 6/9: remove one remaining direct access to pp->tpgs
>  - 9/9: use non-personal email address for maintainer
> 
> Martin Wilck (10):
>   libmultipath: add sysfs_get_inquiry()
>   libmultipath: alua: make API more consistent
>   libmultipath: alua: try to retrieve inquiry data from sysfs
>   libmultipath: constify sysfs_get_timeout()
>   libmultipath: detect_alua(): use system timeout
>   libmultipath: lazy tpgs probing
>   libmultipath: don't link alua_rtpg.o twice
>   libmultipath: tidy up do_set_from_hwe() with statement expression
>   libmultipath: check_rdac(): pre-check in hwtable
>   libmultipath: hwtable: add Lenovo DE series
> 
>  libmultipath/discovery.c              | 46 +++++++++++++++++-------
>  libmultipath/discovery.h              | 10 +++---
>  libmultipath/hwtable.c                | 20 +++++++++++
>  libmultipath/prioritizers/Makefile    |  3 --
>  libmultipath/prioritizers/alua.c      |  4 +--
>  libmultipath/prioritizers/alua_rtpg.c | 45 +++++++++++++++++------
>  libmultipath/prioritizers/alua_rtpg.h |  7 ++--
>  libmultipath/propsel.c                | 52 ++++++++++++++++++---------
>  multipathd/main.c                     |  7 ++--
>  9 files changed, 139 insertions(+), 55 deletions(-)
> 
> -- 
> 2.21.0