[PATCH v2 0/9] multipath-tools: avoid I/O during uevent processing

[PATCH v2 0/9] 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 wrt v1:

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

Martin Wilck (9):
  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: 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                | 33 +++++++++++++------
 multipathd/main.c                     |  7 ++--
 9 files changed, 127 insertions(+), 48 deletions(-)

-- 
2.21.0

[PATCH v2 1/9] 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 v2 2/9] 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 v2 3/9] 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

Re: [PATCH v2 3/9] libmultipath: alua: try to retrieve inquiry data from sysfs

[Hannes Reinecke]

On 3/15/19 6:19 PM, Martin Wilck wrote:
> 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);
>   }
>   
> 
Don't you need to do a udev_device_put() or somesuch?

Other than that:

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

Re: [PATCH v2 3/9] libmultipath: alua: try to retrieve inquiry data from sysfs

[Martin Wilck]

On Mon, 2019-03-18 at 08:08 +0100, Hannes Reinecke wrote:
> On 3/15/19 6:19 PM, Martin Wilck wrote:
> > 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);
> >   }
> >   
> > 
> Don't you need to do a udev_device_put() or somesuch?

No. The man page of udev_device_get_parent... says "No additional
reference to this device is acquired".

Thanks,
Martin


> 
> Other than that:
> 
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> 
> Cheers,
> 
> Hannes

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


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

[PATCH v2 4/9] 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 v2 5/9] 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

[PATCH v2 6/9] 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 v2 7/9] 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 v2 8/9] 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 the RDAC override only for arrays that have legacy configuration
to use the rdac checker.

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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 27474f05..8c08a5cc 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -470,9 +470,17 @@ check_rdac(struct path * pp)
 {
 	int len;
 	char buff[44];
+	const char *checker_name = NULL;
+	/* dummy, for do_set_from_hwe */
+	const char *origin __attribute__((unused));
 
 	if (pp->bus != SYSFS_BUS_SCSI)
 		return 0;
+	/* Avoid ioctl if this is likely not an RDAC array */
+	do_set_from_hwe(checker_name, pp, checker_name, NULL);
+out:    /* for do_set_from_hwe */
+	if (!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

Re: [PATCH v2 8/9] libmultipath: check_rdac(): pre-check in hwtable

[Hannes Reinecke]

On 3/15/19 6:19 PM, Martin Wilck wrote:
> 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 the RDAC override only for arrays that have legacy configuration
> to use the rdac checker.
> 
> 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 | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 27474f05..8c08a5cc 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -470,9 +470,17 @@ check_rdac(struct path * pp)
>   {
>   	int len;
>   	char buff[44];
> +	const char *checker_name = NULL;
> +	/* dummy, for do_set_from_hwe */
> +	const char *origin __attribute__((unused));
>   
>   	if (pp->bus != SYSFS_BUS_SCSI)
>   		return 0;
> +	/* Avoid ioctl if this is likely not an RDAC array */
> +	do_set_from_hwe(checker_name, pp, checker_name, NULL);
> +out:    /* for do_set_from_hwe */
> +	if (!checker_name || strcmp(checker_name, RDAC))
> +		return 0;
>   	len = get_vpd_sgio(pp->fd, 0xC9, buff, 44);
>   	if (len <= 0)
>   		return 0;
> 
*origin? dummy for do_set_from_hwe()?
What funky interface is that?

Please redesign do_set_from_hwe() so as _not_ to require 'magic' variables.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

Re: [PATCH v2 8/9] libmultipath: check_rdac(): pre-check in hwtable

[Martin Wilck]

On Mon, 2019-03-18 at 08:10 +0100, Hannes Reinecke wrote:
> On 3/15/19 6:19 PM, Martin Wilck wrote:
> > 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 the RDAC override only for arrays that have legacy configuration
> > to use the rdac checker.
> > 
> > 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 | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index 27474f05..8c08a5cc 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -470,9 +470,17 @@ check_rdac(struct path * pp)
> >   {
> >   	int len;
> >   	char buff[44];
> > +	const char *checker_name = NULL;
> > +	/* dummy, for do_set_from_hwe */
> > +	const char *origin __attribute__((unused));
> >   
> >   	if (pp->bus != SYSFS_BUS_SCSI)
> >   		return 0;
> > +	/* Avoid ioctl if this is likely not an RDAC array */
> > +	do_set_from_hwe(checker_name, pp, checker_name, NULL);
> > +out:    /* for do_set_from_hwe */
> > +	if (!checker_name || strcmp(checker_name, RDAC))
> > +		return 0;
> >   	len = get_vpd_sgio(pp->fd, 0xC9, buff, 44);
> >   	if (len <= 0)
> >   		return 0;
> > 
> *origin? dummy for do_set_from_hwe()?
> What funky interface is that?
> 
> Please redesign do_set_from_hwe() so as _not_ to require 'magic'
> variables.

It's the truly "funky" way the macros are written in dict.c :-/
(Admittedly I wrote that one myself, but I was following the style of
the file).

But you're right, I don't like this "dummy" stuff either. As this patch
set is meant as a bug fix and not a major redesign of dict.c, I guess
I'll rather not use the do_set_from_hwe() macro there. And put the
redesign on my to-do list.

Thanks,
Martin

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


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

Re: [PATCH v2 8/9] libmultipath: check_rdac(): pre-check in hwtable

[Martin Wilck]

On Mon, 2019-03-18 at 10:52 +0100, Martin Wilck wrote:
> On Mon, 2019-03-18 at 08:10 +0100, Hannes Reinecke w
> > *origin? dummy for do_set_from_hwe()?
> > What funky interface is that?
> > 
> > Please redesign do_set_from_hwe() so as _not_ to require 'magic'
> > variables.
> 
> It's the truly "funky" way the macros are written in dict.c :-/
> (Admittedly I wrote that one myself, but I was following the style of
> the file).
> 
> But you're right, I don't like this "dummy" stuff either. As this
> patch
> set is meant as a bug fix and not a major redesign of dict.c, I guess
> I'll rather not use the do_set_from_hwe() macro there. And put the
> redesign on my to-do list.

... I meant propsel.c, of course. Sorry.

Martin

[PATCH v2 9/9] 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