[PATCH 0/3] VPD page check consolidation

[PATCH 0/3] VPD page check consolidation

[Hannes Reinecke]

Hi all,

as per recent discussion this patchset consolidates the
VPD page checks. With it VPD page scan will be disabled
if the initial scan encounters an error, and also the
checks for VPD pages are consolidated in the function
scsi_device_supports_vpd().

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  scsi: Move 'scsi_attach_vpd()' prototype to scsi_priv.h
  scsi: disable VPD page check on error
  scsi: consolidate checking for VPD pages

 drivers/scsi/scsi.c               | 14 ++++++++++----
 drivers/scsi/scsi_priv.h          |  1 +
 drivers/scsi/scsi_scan.c          |  4 ++--
 drivers/scsi/scsi_transport_sas.c |  3 ++-
 include/scsi/scsi_device.h        |  1 -
 5 files changed, 15 insertions(+), 8 deletions(-)

-- 
1.8.5.6

[PATCH 1/3] scsi: Move 'scsi_attach_vpd()' prototype to scsi_priv.h

[Hannes Reinecke]

scsi_attach_vpd() is not exported, so it should be in scsi_priv.h.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_priv.h   | 1 +
 include/scsi/scsi_device.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 57a4b99..6c3db56 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -31,6 +31,7 @@ extern void scsi_exit_hosts(void);
 /* scsi.c */
 extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
 extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
+void scsi_attach_vpd(struct scsi_device *sdev);
 #ifdef CONFIG_SCSI_LOGGING
 void scsi_log_send(struct scsi_cmnd *cmd);
 void scsi_log_completion(struct scsi_cmnd *cmd, int disposition);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index a6c346d..3f2f147 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -314,7 +314,6 @@ extern int scsi_add_device(struct Scsi_Host *host, uint channel,
 extern int scsi_register_device_handler(struct scsi_device_handler *scsi_dh);
 extern void scsi_remove_device(struct scsi_device *);
 extern int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh);
-void scsi_attach_vpd(struct scsi_device *sdev);
 
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
-- 
1.8.5.6

[PATCH 2/3] scsi: disable VPD page check on error

[Hannes Reinecke]

If we encounter an error during initial VPD page scan we should be
setting the 'skip_vpd_pages' bit to avoid further accesses.
Any errors during rescan should be ignored, as we might encounter
temporary I/O errors during rescan.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi.c      | 9 ++++++++-
 drivers/scsi/scsi_priv.h | 2 +-
 drivers/scsi/scsi_scan.c | 4 ++--
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1deb6ad..1ff4a0b 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -770,13 +770,14 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
 /**
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
+ * @first_scan: true if called during initial scan
  *
  * Attach the 'Device Identification' VPD page (0x83) and the
  * 'Unit Serial Number' VPD page (0x80) to a SCSI device
  * structure. This information can be used to identify the device
  * uniquely.
  */
-void scsi_attach_vpd(struct scsi_device *sdev)
+void scsi_attach_vpd(struct scsi_device *sdev, bool first_scan)
 {
 	int result, i;
 	int vpd_len = SCSI_VPD_PG_LEN;
@@ -796,6 +797,8 @@ retry_pg0:
 	result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
 	if (result < 0) {
 		kfree(vpd_buf);
+		if (first_scan)
+			sdev->skip_vpd_pages = 1;
 		return;
 	}
 	if (result > vpd_len) {
@@ -822,6 +825,8 @@ retry_pg80:
 		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
 		if (result < 0) {
 			kfree(vpd_buf);
+			if (first_scan)
+				sdev->skip_vpd_pages = 1;
 			return;
 		}
 		if (result > vpd_len) {
@@ -851,6 +856,8 @@ retry_pg83:
 		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
 		if (result < 0) {
 			kfree(vpd_buf);
+			if (first_scan)
+				sdev->skip_vpd_pages = 1;
 			return;
 		}
 		if (result > vpd_len) {
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 6c3db56..0c4cc6d 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -31,7 +31,7 @@ extern void scsi_exit_hosts(void);
 /* scsi.c */
 extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
 extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
-void scsi_attach_vpd(struct scsi_device *sdev);
+void scsi_attach_vpd(struct scsi_device *sdev, bool first_scan);
 #ifdef CONFIG_SCSI_LOGGING
 void scsi_log_send(struct scsi_cmnd *cmd);
 void scsi_log_completion(struct scsi_cmnd *cmd, int disposition);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e0a78f5..dcc08ad 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -997,7 +997,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	}
 
 	if (sdev->scsi_level >= SCSI_3)
-		scsi_attach_vpd(sdev);
+		scsi_attach_vpd(sdev, true);
 
 	sdev->max_queue_depth = sdev->queue_depth;
 
@@ -1536,7 +1536,7 @@ void scsi_rescan_device(struct device *dev)
 
 	device_lock(dev);
 
-	scsi_attach_vpd(sdev);
+	scsi_attach_vpd(sdev, false);
 
 	if (sdev->handler && sdev->handler->rescan)
 		sdev->handler->rescan(sdev);
-- 
1.8.5.6

[PATCH 3/3] scsi: consolidate checking for VPD pages

[Hannes Reinecke]

scsi_get_vpd_page() has a check for 'skip_vpd_pages'. However,
this is not sufficient to test if the device really supports
VPD pages. At the same time, most sites were already calling
scsi_device_supports_vpd() prior to calling this function.
So this patchs updates all callers to use scsi_device_supports_vpd()
prior to calling scsi_get_vpd_page() and remove the check
to skip_vpd_pages.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi.c               | 5 ++---
 drivers/scsi/scsi_transport_sas.c | 3 ++-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1ff4a0b..f054878 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -727,15 +727,14 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
  * to a buffer containing the data from that page.  The caller is
  * responsible for calling kfree() on this pointer when it is no longer
  * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
+ * The caller is responsible for checking if the device supports VPD
+ * pages prior to calling this function.
  */
 int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 		      int buf_len)
 {
 	int i, result;
 
-	if (sdev->skip_vpd_pages)
-		goto fail;
-
 	/* Ask for all the pages supported by this device */
 	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
 	if (result < 4)
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 3f0ff07..b2616eb 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -412,7 +412,8 @@ sas_tlr_supported(struct scsi_device *sdev)
 	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
 	int ret = 0;
 
-	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
+	if (!scsi_device_supports_vpd(sdev) ||
+	    scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
 		goto out;
 
 	/*
-- 
1.8.5.6

Re: [PATCH 1/3] scsi: Move 'scsi_attach_vpd()' prototype to scsi_priv.h

[Ewan D. Milne]

On Mon, 2016-06-20 at 08:57 +0200, Hannes Reinecke wrote:
> scsi_attach_vpd() is not exported, so it should be in scsi_priv.h.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi_priv.h   | 1 +
>  include/scsi/scsi_device.h | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 57a4b99..6c3db56 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -31,6 +31,7 @@ extern void scsi_exit_hosts(void);
>  /* scsi.c */
>  extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
>  extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
> +void scsi_attach_vpd(struct scsi_device *sdev);
>  #ifdef CONFIG_SCSI_LOGGING
>  void scsi_log_send(struct scsi_cmnd *cmd);
>  void scsi_log_completion(struct scsi_cmnd *cmd, int disposition);
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index a6c346d..3f2f147 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -314,7 +314,6 @@ extern int scsi_add_device(struct Scsi_Host *host, uint channel,
>  extern int scsi_register_device_handler(struct scsi_device_handler *scsi_dh);
>  extern void scsi_remove_device(struct scsi_device *);
>  extern int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh);
> -void scsi_attach_vpd(struct scsi_device *sdev);
>  
>  extern int scsi_device_get(struct scsi_device *);
>  extern void scsi_device_put(struct scsi_device *);

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH 2/3] scsi: disable VPD page check on error

[Ewan D. Milne]

On Mon, 2016-06-20 at 08:57 +0200, Hannes Reinecke wrote:
> If we encounter an error during initial VPD page scan we should be
> setting the 'skip_vpd_pages' bit to avoid further accesses.
> Any errors during rescan should be ignored, as we might encounter
> temporary I/O errors during rescan.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi.c      | 9 ++++++++-
>  drivers/scsi/scsi_priv.h | 2 +-
>  drivers/scsi/scsi_scan.c | 4 ++--
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 1deb6ad..1ff4a0b 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -770,13 +770,14 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
>  /**
>   * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
>   * @sdev: The device to ask
> + * @first_scan: true if called during initial scan
>   *
>   * Attach the 'Device Identification' VPD page (0x83) and the
>   * 'Unit Serial Number' VPD page (0x80) to a SCSI device
>   * structure. This information can be used to identify the device
>   * uniquely.
>   */
> -void scsi_attach_vpd(struct scsi_device *sdev)
> +void scsi_attach_vpd(struct scsi_device *sdev, bool first_scan)
>  {
>  	int result, i;
>  	int vpd_len = SCSI_VPD_PG_LEN;
> @@ -796,6 +797,8 @@ retry_pg0:
>  	result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
>  	if (result < 0) {
>  		kfree(vpd_buf);
> +		if (first_scan)
> +			sdev->skip_vpd_pages = 1;
>  		return;
>  	}
>  	if (result > vpd_len) {
> @@ -822,6 +825,8 @@ retry_pg80:
>  		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
>  		if (result < 0) {
>  			kfree(vpd_buf);
> +			if (first_scan)
> +				sdev->skip_vpd_pages = 1;
>  			return;
>  		}
>  		if (result > vpd_len) {
> @@ -851,6 +856,8 @@ retry_pg83:
>  		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
>  		if (result < 0) {
>  			kfree(vpd_buf);
> +			if (first_scan)
> +				sdev->skip_vpd_pages = 1;
>  			return;
>  		}
>  		if (result > vpd_len) {
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 6c3db56..0c4cc6d 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -31,7 +31,7 @@ extern void scsi_exit_hosts(void);
>  /* scsi.c */
>  extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
>  extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
> -void scsi_attach_vpd(struct scsi_device *sdev);
> +void scsi_attach_vpd(struct scsi_device *sdev, bool first_scan);
>  #ifdef CONFIG_SCSI_LOGGING
>  void scsi_log_send(struct scsi_cmnd *cmd);
>  void scsi_log_completion(struct scsi_cmnd *cmd, int disposition);
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index e0a78f5..dcc08ad 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -997,7 +997,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  	}
>  
>  	if (sdev->scsi_level >= SCSI_3)
> -		scsi_attach_vpd(sdev);
> +		scsi_attach_vpd(sdev, true);
>  
>  	sdev->max_queue_depth = sdev->queue_depth;
>  
> @@ -1536,7 +1536,7 @@ void scsi_rescan_device(struct device *dev)
>  
>  	device_lock(dev);
>  
> -	scsi_attach_vpd(sdev);
> +	scsi_attach_vpd(sdev, false);
>  
>  	if (sdev->handler && sdev->handler->rescan)
>  		sdev->handler->rescan(sdev);

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH 3/3] scsi: consolidate checking for VPD pages

[Ewan D. Milne]

On Mon, 2016-06-20 at 08:57 +0200, Hannes Reinecke wrote:
> scsi_get_vpd_page() has a check for 'skip_vpd_pages'. However,
> this is not sufficient to test if the device really supports
> VPD pages. At the same time, most sites were already calling
> scsi_device_supports_vpd() prior to calling this function.
> So this patchs updates all callers to use scsi_device_supports_vpd()
> prior to calling scsi_get_vpd_page() and remove the check
> to skip_vpd_pages.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi.c               | 5 ++---
>  drivers/scsi/scsi_transport_sas.c | 3 ++-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 1ff4a0b..f054878 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -727,15 +727,14 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>   * to a buffer containing the data from that page.  The caller is
>   * responsible for calling kfree() on this pointer when it is no longer
>   * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
> + * The caller is responsible for checking if the device supports VPD
> + * pages prior to calling this function.
>   */
>  int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
>  		      int buf_len)
>  {
>  	int i, result;
>  
> -	if (sdev->skip_vpd_pages)
> -		goto fail;
> -
>  	/* Ask for all the pages supported by this device */
>  	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
>  	if (result < 4)
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index 3f0ff07..b2616eb 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -412,7 +412,8 @@ sas_tlr_supported(struct scsi_device *sdev)
>  	char *buffer = kzalloc(vpd_len, GFP_KERNEL);
>  	int ret = 0;
>  
> -	if (scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
> +	if (!scsi_device_supports_vpd(sdev) ||
> +	    scsi_get_vpd_page(sdev, 0x90, buffer, vpd_len))
>  		goto out;
>  
>  	/*

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH 2/3] scsi: disable VPD page check on error

[James Bottomley]

On Mon, 2016-06-20 at 09:25 -0400, Ewan D. Milne wrote:
> On Mon, 2016-06-20 at 08:57 +0200, Hannes Reinecke wrote:
> > If we encounter an error during initial VPD page scan we should be
> > setting the 'skip_vpd_pages' bit to avoid further accesses.
> > Any errors during rescan should be ignored, as we might encounter
> > temporary I/O errors during rescan.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.com>
> > ---
> >  drivers/scsi/scsi.c      | 9 ++++++++-
> >  drivers/scsi/scsi_priv.h | 2 +-
> >  drivers/scsi/scsi_scan.c | 4 ++--
> >  3 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index 1deb6ad..1ff4a0b 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -770,13 +770,14 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
> >  /**
> >   * scsi_attach_vpd - Attach Vital Product Data to a SCSI device
> > structure
> >   * @sdev: The device to ask
> > + * @first_scan: true if called during initial scan
> >   *
> >   * Attach the 'Device Identification' VPD page (0x83) and the
> >   * 'Unit Serial Number' VPD page (0x80) to a SCSI device
> >   * structure. This information can be used to identify the device
> >   * uniquely.
> >   */
> > -void scsi_attach_vpd(struct scsi_device *sdev)
> > +void scsi_attach_vpd(struct scsi_device *sdev, bool first_scan)
> >  {
> >  	int result, i;
> >  	int vpd_len = SCSI_VPD_PG_LEN;
> > @@ -796,6 +797,8 @@ retry_pg0:
> >  	result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
> >  	if (result < 0) {
> >  		kfree(vpd_buf);
> > +		if (first_scan)
> > +			sdev->skip_vpd_pages = 1;
> >  		return;
> >  	}
> >  	if (result > vpd_len) {
> > @@ -822,6 +825,8 @@ retry_pg80:
> >  		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80,
> > vpd_len);
> >  		if (result < 0) {
> >  			kfree(vpd_buf);
> > +			if (first_scan)
> > +				sdev->skip_vpd_pages = 1;
> >  			return;
> >  		}
> >  		if (result > vpd_len) {
> > @@ -851,6 +856,8 @@ retry_pg83:
> >  		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83,
> > vpd_len);
> >  		if (result < 0) {
> >  			kfree(vpd_buf);
> > +			if (first_scan)
> > +				sdev->skip_vpd_pages = 1;
> >  			return;
> >  		}
> >  		if (result > vpd_len) {
> > diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> > index 6c3db56..0c4cc6d 100644
> > --- a/drivers/scsi/scsi_priv.h
> > +++ b/drivers/scsi/scsi_priv.h
> > @@ -31,7 +31,7 @@ extern void scsi_exit_hosts(void);
> >  /* scsi.c */
> >  extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
> >  extern void scsi_destroy_command_freelist(struct Scsi_Host
> > *shost);
> > -void scsi_attach_vpd(struct scsi_device *sdev);
> > +void scsi_attach_vpd(struct scsi_device *sdev, bool first_scan);
> >  #ifdef CONFIG_SCSI_LOGGING
> >  void scsi_log_send(struct scsi_cmnd *cmd);
> >  void scsi_log_completion(struct scsi_cmnd *cmd, int disposition);
> > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > index e0a78f5..dcc08ad 100644
> > --- a/drivers/scsi/scsi_scan.c
> > +++ b/drivers/scsi/scsi_scan.c
> > @@ -997,7 +997,7 @@ static int scsi_add_lun(struct scsi_device
> > *sdev, unsigned char *inq_result,
> >  	}
> >  
> >  	if (sdev->scsi_level >= SCSI_3)
> > -		scsi_attach_vpd(sdev);
> > +		scsi_attach_vpd(sdev, true);
> >  
> >  	sdev->max_queue_depth = sdev->queue_depth;
> >  
> > @@ -1536,7 +1536,7 @@ void scsi_rescan_device(struct device *dev)
> >  
> >  	device_lock(dev);
> >  
> > -	scsi_attach_vpd(sdev);
> > +	scsi_attach_vpd(sdev, false);
> >  
> >  	if (sdev->handler && sdev->handler->rescan)
> >  		sdev->handler->rescan(sdev);
> 
> Reviewed-by: Ewan D. Milne <emilne@redhat.com>

I take it your concerns about never asking the device for VPD pages
again are addressed?

I also don't really like this.  If the device has a failure like the
QEMU one where it just hangs up, this won't help because the problem
already happened.  Conversely, if the device replies in the negative,
it should always do so, so I can't see what this buys us, except the
possibility of doing the wrong thing on a transient error (which can
happen on your ALUA devices during path switch, I believe?)

I'd be much happier if you can point to a problem that this would
solve.

James

Re: [PATCH 2/3] scsi: disable VPD page check on error

[Ewan D. Milne]

On Mon, 2016-06-20 at 06:44 -0700, James Bottomley wrote:
> On Mon, 2016-06-20 at 09:25 -0400, Ewan D. Milne wrote:
> > On Mon, 2016-06-20 at 08:57 +0200, Hannes Reinecke wrote:
> > > If we encounter an error during initial VPD page scan we should be
> > > setting the 'skip_vpd_pages' bit to avoid further accesses.
> > > Any errors during rescan should be ignored, as we might encounter
> > > temporary I/O errors during rescan.
> > > 
> > > Signed-off-by: Hannes Reinecke <hare@suse.com>
> > > ---
> > >  drivers/scsi/scsi.c      | 9 ++++++++-
> > >  drivers/scsi/scsi_priv.h | 2 +-
> > >  drivers/scsi/scsi_scan.c | 4 ++--
> > >  3 files changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > > index 1deb6ad..1ff4a0b 100644
> > > --- a/drivers/scsi/scsi.c
> > > +++ b/drivers/scsi/scsi.c
> > > @@ -770,13 +770,14 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
> > >  /**
> > >   * scsi_attach_vpd - Attach Vital Product Data to a SCSI device
> > > structure
> > >   * @sdev: The device to ask
> > > + * @first_scan: true if called during initial scan
> > >   *
> > >   * Attach the 'Device Identification' VPD page (0x83) and the
> > >   * 'Unit Serial Number' VPD page (0x80) to a SCSI device
> > >   * structure. This information can be used to identify the device
> > >   * uniquely.
> > >   */
> > > -void scsi_attach_vpd(struct scsi_device *sdev)
> > > +void scsi_attach_vpd(struct scsi_device *sdev, bool first_scan)
> > >  {
> > >  	int result, i;
> > >  	int vpd_len = SCSI_VPD_PG_LEN;
> > > @@ -796,6 +797,8 @@ retry_pg0:
> > >  	result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
> > >  	if (result < 0) {
> > >  		kfree(vpd_buf);
> > > +		if (first_scan)
> > > +			sdev->skip_vpd_pages = 1;
> > >  		return;
> > >  	}
> > >  	if (result > vpd_len) {
> > > @@ -822,6 +825,8 @@ retry_pg80:
> > >  		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80,
> > > vpd_len);
> > >  		if (result < 0) {
> > >  			kfree(vpd_buf);
> > > +			if (first_scan)
> > > +				sdev->skip_vpd_pages = 1;
> > >  			return;
> > >  		}
> > >  		if (result > vpd_len) {
> > > @@ -851,6 +856,8 @@ retry_pg83:
> > >  		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83,
> > > vpd_len);
> > >  		if (result < 0) {
> > >  			kfree(vpd_buf);
> > > +			if (first_scan)
> > > +				sdev->skip_vpd_pages = 1;
> > >  			return;
> > >  		}
> > >  		if (result > vpd_len) {
> > > diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> > > index 6c3db56..0c4cc6d 100644
> > > --- a/drivers/scsi/scsi_priv.h
> > > +++ b/drivers/scsi/scsi_priv.h
> > > @@ -31,7 +31,7 @@ extern void scsi_exit_hosts(void);
> > >  /* scsi.c */
> > >  extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
> > >  extern void scsi_destroy_command_freelist(struct Scsi_Host
> > > *shost);
> > > -void scsi_attach_vpd(struct scsi_device *sdev);
> > > +void scsi_attach_vpd(struct scsi_device *sdev, bool first_scan);
> > >  #ifdef CONFIG_SCSI_LOGGING
> > >  void scsi_log_send(struct scsi_cmnd *cmd);
> > >  void scsi_log_completion(struct scsi_cmnd *cmd, int disposition);
> > > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > > index e0a78f5..dcc08ad 100644
> > > --- a/drivers/scsi/scsi_scan.c
> > > +++ b/drivers/scsi/scsi_scan.c
> > > @@ -997,7 +997,7 @@ static int scsi_add_lun(struct scsi_device
> > > *sdev, unsigned char *inq_result,
> > >  	}
> > >  
> > >  	if (sdev->scsi_level >= SCSI_3)
> > > -		scsi_attach_vpd(sdev);
> > > +		scsi_attach_vpd(sdev, true);
> > >  
> > >  	sdev->max_queue_depth = sdev->queue_depth;
> > >  
> > > @@ -1536,7 +1536,7 @@ void scsi_rescan_device(struct device *dev)
> > >  
> > >  	device_lock(dev);
> > >  
> > > -	scsi_attach_vpd(sdev);
> > > +	scsi_attach_vpd(sdev, false);
> > >  
> > >  	if (sdev->handler && sdev->handler->rescan)
> > >  		sdev->handler->rescan(sdev);
> > 
> > Reviewed-by: Ewan D. Milne <emilne@redhat.com>
> 
> I take it your concerns about never asking the device for VPD pages
> again are addressed?
> 
> I also don't really like this.  If the device has a failure like the
> QEMU one where it just hangs up, this won't help because the problem
> already happened.  Conversely, if the device replies in the negative,
> it should always do so, so I can't see what this buys us, except the
> possibility of doing the wrong thing on a transient error (which can
> happen on your ALUA devices during path switch, I believe?)
> 
> I'd be much happier if you can point to a problem that this would
> solve.
> 
> James

My concern with the earlier patch was that any time a rescan was
performed, and the VPD inquiry failed, we might have VPD data that
had been obtained on the initial scan, but might now be stale.
And then the failed inquiry would result in it never being updated
again, ever.  (This could happen if a path was down, for example.)

With this approach, if the VPD inquiry info isn't obtained initially,
it won't be attempted in the future.  So there shouldn't be a problem
with stale data, since we didn't obtain it in the first place.

This is predicated on the assumption that stale data is an issue,
but that was the whole point of Hannes' change to re-probe the VPD
data on rescan.

-Ewan

> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] scsi: disable VPD page check on error

[Martin K. Petersen]

>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:

James> I also don't really like this.  If the device has a failure like
James> the QEMU one where it just hangs up, this won't help because the
James> problem already happened.  Conversely, if the device replies in
James> the negative, it should always do so, so I can't see what this
James> buys us, except the possibility of doing the wrong thing on a
James> transient error

Yeah.

James> I'd be much happier if you can point to a problem that this would
James> solve.

I'd like more details as well.

If we are going to entertain skipping VPD pages on error I would like it
to be as a result of a clear indication that it's the VPD that's the
problem and not just any error returned while querying the page.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/3] scsi: Move 'scsi_attach_vpd()' prototype to scsi_priv.h

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] scsi: disable VPD page check on error

[Christoph Hellwig]

On Mon, Jun 20, 2016 at 08:57:47AM +0200, Hannes Reinecke wrote:
> If we encounter an error during initial VPD page scan we should be
> setting the 'skip_vpd_pages' bit to avoid further accesses.
> Any errors during rescan should be ignored, as we might encounter
> temporary I/O errors during rescan.

Looks fine in general, but why disable this for a rescan?  Doing
the check there as well at least doesn't seem harmful, and for the
unlikely case of a botched firmware upgrade it might even be useful.

Re: [PATCH 3/3] scsi: consolidate checking for VPD pages

[Christoph Hellwig]

Looks fine, although I wonder if simple calling scsi_device_supports_vpd
in scsi_get_vpd_page wouldn't be even better.