Re: [PATCH] scsi: retry MODE SENSE on unit attention

[Ewan Milne <emilne@redhat.com>]

On Fri, 2015-06-12 at 08:27 +0200, Hannes Reinecke wrote:
> On 06/11/2015 05:07 PM, Ewan Milne wrote:
> > On Thu, 2015-06-11 at 13:01 +0200, Hannes Reinecke wrote:
> >> The 'sd' driver is calling scsi_mode_sense() to figure out
> >> internal details. But scsi_mode_sense() never checks for
> >> any pending unit attentions, so we're getting annoying error
> >> messages like:
> >>
> >> MODE SENSE: unimplemented page/subpage: 0x00/0x00
> >>
> >> and a possible wrong decision for device cache handling.
> >>
> >> Signed-off-by: Hannes Reinecke <hare@suse.de>
> >> ---
> >>  drivers/scsi/scsi_lib.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> >> index 2428d96..d7915c8 100644
> >> --- a/drivers/scsi/scsi_lib.c
> >> +++ b/drivers/scsi/scsi_lib.c
> >> @@ -2423,7 +2423,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
> >>  	unsigned char cmd[12];
> >>  	int use_10_for_ms;
> >>  	int header_length;
> >> -	int result;
> >> +	int result, retry_count = retries;
> >>  	struct scsi_sense_hdr my_sshdr;
> >>  
> >>  	memset(data, 0, sizeof(*data));
> >> @@ -2502,6 +2502,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
> >>  			data->block_descriptor_length = buffer[3];
> >>  		}
> >>  		data->header_length = header_length;
> >> +	} else if ((status_byte(result) == CHECK_CONDITION) &&
> >> +		   scsi_sense_valid(sshdr) &&
> >> +		   sshdr->sense_key == UNIT_ATTENTION && retry_count) {
> >> +		retry_count--;
> >> +		goto retry;
> >>  	}
> >>  
> >>  	return result;
> > 
> > Great, but shouldn't we be doing this more generally?  What about
> > scsi_mode_select()?
> > 
> I haven't seen any issues with scsi_mode_select() as of now, so I
> didn't do anything about this :-)
> 
> > (And, with the number of status changes that can get reported by
> >  UAs, we might want to think about increasing the retry count on
> >  these commands up from 3 at some point.)
> > 
> Hmm. _Actually_, we're not getting _more_ UAs (neither the number
> nor the situation at which UAs are being send has changed).
> It's just that we're trying to _use_ UAs so these things pop up.
> But yeah, raising the number or retries to eg 5 is probably a good idea.

Yeah, or maybe add a time limit to we don't drag out the boot time.
We can do this later, though, I'm fine with your patch as it is.

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