On Wed, 2013-01-23 at 13:06 +0000, James Bottomley wrote:
> On Tue, 2013-01-22 at 10:08 -0500, Ewan Milne wrote:
> > On Fri, 2013-01-18 at 16:46 +0000, 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.
> >
> > Unfortunately, I am not sure that the allocation length that was sent
> > to the device is always available.
>
> Well, yes it is, we just don't store it. scsi_normalize_sense() takes
> as input the length of the sense buffer we gave it. If we want an
> overflow indication, we can certainly compare that against the
> additional length (assuming we have enough bytes to get the additional
> length).
>
> > I will look into this more closely
> > but it appeared to me that e.g. FC drivers like qla2xxx get the sense
> > data automatically from the HBA firmware. In the case of that driver
> > the host sense buffer size looks like it is hard-coded to 32 bytes,
> > for all I know the firmware might only asking for 18 bytes.
>
> You mean for their ACA emulation in the transport? They have to be
> picking up at least the standard minimum in order not to be in
> violation, surely?
I'm sure that's the case, I just don't know if the minimum is big
enough. See below.
>
> > Of course, for a normal REQUEST SENSE command where the allocation
> > length is in the CDB, it would indeed be easy to add a check against
> > the additional sense length.
> >
> > >
> > > 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.
> >
> > That would indeed be a problem. I didn't see this behavior when testing
> > the changes but I'll need to investigate this further.
> >
> > The purpose of detecting the sense data overflow was to provide some
> > visibility that a device is returning a large amount of sense data that
> > is currently being silently ignored. In the case of descriptor format
> > sense data, it is possible that a descriptor we want to examine is
> > located after one or more other descriptors, and we might not get it
> > at all if the buffer isn't large enough.
>
> But why should we care? A lot of the time it will be spewing
> descriptors with irrelevant FRU information. I really think printing
> there's been an overflow isn't a good idea. I'm not sure there's much
> use in the sshdr indicating there's been one. It *may* be useful to
> indicate how big the allocation length should have been, but I'm not
> even convinced of that, since the data is now lost.
My thinking was that it was important to log the fact that the device
had reported a Unit Attention queue overflow, which is reported in a
sense key specific descriptor, so I was concerned that if the sense
buffer wasn't big enough, the host would never see that there had
been a UA queue overflow. That was why I had added the logging of
a sense buffer overflow. I wasn't so much interested in reporting
sense buffer overflow for its own sake.
So, as it stands right now, I think I'll remove the 1/9 component
of the patch set, unless there is consensus that it is really needed.
>
> James
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
