diff mbox series

[v2,5/6] libmultipath: limit reading 0xc9 vpd page

Message ID 1604472849-22422-6-git-send-email-bmarzins@redhat.com (mailing list archive)
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show
Series Misc Multipath patches | expand

Commit Message

Benjamin Marzinski Nov. 4, 2020, 6:54 a.m. UTC 
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(-)

Comments

Martin Wilck Dec. 16, 2020, 9:18 p.m. UTC | #1 
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)
Benjamin Marzinski Dec. 16, 2020, 11:56 p.m. UTC | #2 
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
Martin Wilck Dec. 17, 2020, 12:06 a.m. UTC | #3 
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
diff mbox series

Patch

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)