Re: [PATCHv2] sd: retry READ CAPACITY for ALUA state transition

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Mon, 2015-07-06 at 08:13 -0700, Bart Van Assche wrote:
> On 07/06/2015 04:12 AM, Hannes Reinecke wrote:
> > During ALUA state transitions the device might return
> > a sense code 02/04/0a (Logical unit not accessible, asymmetric
> > access state transition). As this is a transient error
> > we should just retry the READ CAPACITY call after 1 second
> > until the state transition finishes and the correct
> > capacity can be returned.
> > At the same time we should break out of the loop after
> > 2 minutes to avoid unbounded retries.
> >
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > ---
> >   drivers/scsi/sd.c | 21 +++++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 3b2fcb4..f45b8fe 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -1934,6 +1934,7 @@ static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
> >   #endif
> >
> >   #define READ_CAPACITY_RETRIES_ON_RESET	10
> > +#define READ_CAPACITY_RETRIES_ON_TRANSITION 120
> >
> >   static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
> >   						unsigned char *buffer)
> > @@ -1943,6 +1944,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
> >   	int sense_valid = 0;
> >   	int the_result;
> >   	int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
> > +	int transition_retries = READ_CAPACITY_RETRIES_ON_TRANSITION;
> >   	unsigned int alignment;
> >   	unsigned long long lba;
> >   	unsigned sector_size;
> > @@ -1981,6 +1983,15 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
> >   				 * give it one more chance */
> >   				if (--reset_retries > 0)
> >   					continue;
> > +			if (sense_valid &&
> > +			    sshdr.sense_key == NOT_READY &&
> > +			    sshdr.asc == 0x04 && sshdr.ascq == 0x0A) {
> > +				/* ALUA state transition; retry after delay */
> > +				if (--transition_retries > 0) {
> > +					msleep(1000);
> > +					continue;
> > +				}
> > +			}
> >   		}
> >   		retries--;
> >
> > @@ -2039,6 +2050,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
> >   	int sense_valid = 0;
> >   	int the_result;
> >   	int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
> > +	int transition_retries = READ_CAPACITY_RETRIES_ON_TRANSITION;
> >   	sector_t lba;
> >   	unsigned sector_size;
> >
> > @@ -2063,6 +2075,15 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
> >   				 * give it one more chance */
> >   				if (--reset_retries > 0)
> >   					continue;
> > +			if (sense_valid &&
> > +			    sshdr.sense_key == NOT_READY &&
> > +			    sshdr.asc == 0x04 && sshdr.ascq == 0x0A) {
> > +				/* ALUA state transition; retry after delay */
> > +				if (--transition_retries > 0) {
> > +					msleep(1000);
> > +					continue;
> > +				}
> > +			}
> >   		}
> >   		retries--;
> 
> Hello Hannes,
> 
> Although I agree that multipathd should handle arrays correctly that 
> fail READ CAPACITY commands while transitioning, seeing that a new 
> hard-coded timeout is added in the SCSI initiator code does not make me 
> enthusiast. The READ_CAPACITY_RETRIES_ON_TRANSITION timeout added 
> through this patch is independent of the IMPLICIT TRANSITION TIME in the 
> REPORT TARGET PORT GROUPS response. Has the following already been 
> considered ?
> - If the capacity is not known, let the scsi_dh_alua handler submit a 
> READ CAPACITY command asynchronously after certain target port group 
> state transitions. Also let the scsi_dh_alua handler submit a 
> notification to user space after the capacity changes from "unknown" to 
> "known".
> - Let multipathd ignore paths for which READ CAPACITY failed until the 
> capacity becomes known.

Yes, I'm with something like this.  The patch you propose will be a busy
wait for userspace which they may not want.

James