Re: [PATCH RFC 1/9] [SCSI] Detect overflow of sense data buffer

[Hannes Reinecke <hare@suse.de>]

On 01/22/2013 04:10 PM, Ewan Milne wrote:
> On Mon, 2013-01-21 at 08:26 +0100, Hannes Reinecke wrote:
>> On 01/18/2013 05:46 PM, James Bottomley wrote:
>>> On Fri, 2013-01-18 at 11:27 -0500, Ewan D. Milne wrote:
>>>> --- a/drivers/scsi/scsi_error.c
>>>> +++ b/drivers/scsi/scsi_error.c
>>>> @@ -241,6 +241,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>>>>    	if (! scsi_command_normalize_sense(scmd, &sshdr))
>>>>    		return FAILED;	/* no valid sense data */
>>>>
>>>> +	if (sshdr.overflow)
>>>> +		scmd_printk(KERN_WARNING, scmd, "Sense data overflow");
>>>> +
>>>>    	if (scsi_sense_is_deferred(&sshdr))
>>>>    		return NEEDS_RETRY;
>>>>
>>>> @@ -2059,14 +2062,18 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
>>>>    			sshdr->asc = sense_buffer[2];
>>>>    		if (sb_len > 3)
>>>>    			sshdr->ascq = sense_buffer[3];
>>>> +		if (sb_len > 4)
>>>> +			sshdr->overflow = ((sense_buffer[4] & 0x80) != 0);
>>>>    		if (sb_len > 7)
>>>>    			sshdr->additional_length = sense_buffer[7];
>>>>    	} else {
>>>>    		/*
>>>>    		 * fixed format
>>>>    		 */
>>>> -		if (sb_len > 2)
>>>> +		if (sb_len > 2) {
>>>> +			sshdr->overflow = ((sense_buffer[2] & 0x10) != 0);
>>>>    			sshdr->sense_key = (sense_buffer[2] & 0xf);
>>>> +		}
>>>>    		if (sb_len > 7) {
>>>>    			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
>>>>    					 sb_len : (sense_buffer[7] + 8);
>>>
>>> This isn't the right way to do it:  The overflow bit is a recent
>>> introduction in SPC-4.  The correct way to tell if we have an overflow
>>> or not is to look at the additional sense length and compare it to the
>>> allocation length; this will work for everything.
>>>
>>> I'm not even convinced that overflow is important: for a lot of the
>>> sense probes, we deliberately induce overflows by giving the request
>>> sense command a short buffer.  Printing a warning in scsi_check_sense
>>> will get very noisy very fast.
>>>
>> And indeed I would rather prefer to have it the other way round;
>> we're using a fixed sense_buffer within the SCSI stack, which might
>> not be large enough to hold all sense data.
>> So I would prefer to have an indicator on whether _the internal_
>> sense buffer overflowed; this would even give us some valid use-case
>> now.
>
> So, if I understand what you're saying, we could check for overflow if
> (sense_data[7] + 8) > SCSI_SENSE_BUFFERSIZE
>
Precisely.

> We could do that, certainly.  I think, though, that overflow of sense
> data is more likely to occur if a sense buffer smaller than
> SCSI_SENSE_BUFFERSIZE bytes is used, e.g. by a call to
> scsi_eh_prep_cmnd(), which is an exported symbol.
>
> The existing 96-byte SCSI_SENSE_BUFFERSIZE may well be big enough.
> (I did increase it to the SPC-4 defined value of 252 bytes in a later
> patch in the series if the appropriate kernel config option is enabled.)
>
Indeed, it _may_.

I just would like to have an indicator on whether it actually _is_.
Currently we have no way of telling.
Seeing that we want to implement _real_ sense code handling we
really want to know whether we've missed some information.

Relying on the SPC-defined values doesn't really cut it, as it's 
only in the very latest draft and it'll be ages until we're seeing
these devices in the field. So we have to find a way of handling
older devices, which may yet send us large sense data.
(Think of referrals ...)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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