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

[Hannes Reinecke <hare@suse.de>]

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