[PATCH] scsi: disable VPD page check on error

[PATCH] scsi: disable VPD page check on error

[Hannes Reinecke]

If we encounter an error during VPD page scanning we should be
setting the 'skip_vpd_pages' bit to avoid further accesses.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1deb6ad..0359864 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -796,6 +796,7 @@ retry_pg0:
 	result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
 	if (result < 0) {
 		kfree(vpd_buf);
+		sdev->skip_vpd_pages = 1;
 		return;
 	}
 	if (result > vpd_len) {
@@ -822,6 +823,7 @@ retry_pg80:
 		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
 		if (result < 0) {
 			kfree(vpd_buf);
+			sdev->skip_vpd_pages = 1;
 			return;
 		}
 		if (result > vpd_len) {
@@ -851,6 +853,7 @@ retry_pg83:
 		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
 		if (result < 0) {
 			kfree(vpd_buf);
+			sdev->skip_vpd_pages = 1;
 			return;
 		}
 		if (result > vpd_len) {
-- 
1.8.5.6

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

[Ewan D. Milne]

On Mon, 2016-06-13 at 14:48 +0200, Hannes Reinecke wrote:
> If we encounter an error during VPD page scanning we should be
> setting the 'skip_vpd_pages' bit to avoid further accesses.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 1deb6ad..0359864 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -796,6 +796,7 @@ retry_pg0:
>  	result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
>  	if (result < 0) {
>  		kfree(vpd_buf);
> +		sdev->skip_vpd_pages = 1;
>  		return;
>  	}
>  	if (result > vpd_len) {
> @@ -822,6 +823,7 @@ retry_pg80:
>  		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
>  		if (result < 0) {
>  			kfree(vpd_buf);
> +			sdev->skip_vpd_pages = 1;
>  			return;
>  		}
>  		if (result > vpd_len) {
> @@ -851,6 +853,7 @@ retry_pg83:
>  		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
>  		if (result < 0) {
>  			kfree(vpd_buf);
> +			sdev->skip_vpd_pages = 1;
>  			return;
>  		}
>  		if (result > vpd_len) {

So, this changes scsi_attach_vpd() but not scsi_get_vpd_page() ?

This particular implementation worries me.  If we get an error
performing a VPD inquiry, we will never, ever, attempt one again?  What
happens if a path is down at the time?  The idea behind getting updated
VPD info was that it might change, so if that does happen we don't want
to stop updating after an isolated error.

I think what we want to do is check if the VPD inquiry is supported on
the *initial* inquiry, and if that fails then suppress further updates.

-Ewan

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

[Hannes Reinecke]

On 06/13/2016 03:50 PM, Ewan D. Milne wrote:
> On Mon, 2016-06-13 at 14:48 +0200, Hannes Reinecke wrote:
>> If we encounter an error during VPD page scanning we should be
>> setting the 'skip_vpd_pages' bit to avoid further accesses.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/scsi.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index 1deb6ad..0359864 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -796,6 +796,7 @@ retry_pg0:
>>  	result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
>>  	if (result < 0) {
>>  		kfree(vpd_buf);
>> +		sdev->skip_vpd_pages = 1;
>>  		return;
>>  	}
>>  	if (result > vpd_len) {
>> @@ -822,6 +823,7 @@ retry_pg80:
>>  		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
>>  		if (result < 0) {
>>  			kfree(vpd_buf);
>> +			sdev->skip_vpd_pages = 1;
>>  			return;
>>  		}
>>  		if (result > vpd_len) {
>> @@ -851,6 +853,7 @@ retry_pg83:
>>  		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
>>  		if (result < 0) {
>>  			kfree(vpd_buf);
>> +			sdev->skip_vpd_pages = 1;
>>  			return;
>>  		}
>>  		if (result > vpd_len) {
> 
> So, this changes scsi_attach_vpd() but not scsi_get_vpd_page() ?
> 
Yes.
scsi_get_vpd_page() is just checking 'skip_vpd_pages', but none of the
other settings (ie it's not using scsi_device_supports_vpd()).
So we already come to different results when asking for VPD pages via
scsi_get_vpd_page() and scsi_attach_vpd().
Ideally we would be using scsi_device_supports_vpd() in both instances,
but that would require a further audit and I deemed it beyond the scope
of this patch.

> This particular implementation worries me.  If we get an error
> performing a VPD inquiry, we will never, ever, attempt one again?  What
> happens if a path is down at the time?  The idea behind getting updated
> VPD info was that it might change, so if that does happen we don't want
> to stop updating after an isolated error.
> 
> I think what we want to do is check if the VPD inquiry is supported on
> the *initial* inquiry, and if that fails then suppress further updates.
> 
Fair point. Will be updating the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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