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

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

[Hannes Reinecke]

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--;
 
-- 
1.8.5.2

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

[Bart Van Assche]

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.

Thanks,

Bart.

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

[James Bottomley]

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

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

[Hannes Reinecke]

On 07/06/2015 10:57 PM, James Bottomley wrote:
> 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".
I'd rather prefer to have this handled with the existing 'rescan'
functionality. IE if the path becomes available (ie returns a state
of 'optimal' or 'non-optimal' scsi_dh_alua will be invoking 'rescan'
on that device, which should cause the capacity to be updated.

>> - Let multipathd ignore paths for which READ CAPACITY failed until the 
>> capacity becomes known.

I've already sent patches to multipath to simply ignore the results
from a READ CAPACITY; so this is not an issue.
Paths with a capacity of '0' will be happily accepted as multipath
devices with path status 'standby'.

> 
> Yes, I'm with something like this.  The patch you propose will be a busy
> wait for userspace which they may not want.
> 
I'm fully aware that this patch will induce a busy wait if the
device happens to be in transitioning at that time.
This patch is just a simple solution to this problem.
The 'correct' solution is actually a bit more involved, as it would
require the device handler to keep track of the multipath topology:

Currently the device handler are per ITL nexus, and invoked on
activation only (ie for explicit ALUA), so the ALUA state only
reflect the state after the last call to 'activate'.

I have a patchset for moving scsi_dh_alua onto a workqueue, so that
it'll be able to keep track of the ALUA state per TL nexus (note the
missing 'I'). This will not only reduce the number of RTPGs send to
the target (scsi_dh_rdac already does this), but will also allow
scsi_dh_alua to keep track of the ALUA state of the relative target
port of the target port group.

However, to handle the above case correctly we would need to keep
track of the entire multipath topology, to figure out which devices
belong to that relative target port and might need to be updated
(there might be several paths in standby, and we will have sent the
RTPG only for one of them).
Patches for that are not done yet, so I thought the above patch
would be a simple stop-gap measure.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
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

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

[Bart Van Assche]

On 07/06/2015 11:18 PM, Hannes Reinecke wrote:
> However, to handle the above case correctly we would need to keep
> track of the entire multipath topology, to figure out which devices
> belong to that relative target port and might need to be updated
> (there might be several paths in standby, and we will have sent the
> RTPG only for one of them).
> Patches for that are not done yet, so I thought the above patch
> would be a simple stop-gap measure.

Hello Hannes,

Are you sure that keeping track of the entire multipath topology would 
be required to implement what I proposed ? In the patch "scsi_dh_alua: 
Use separate alua_port_group structure" 
(http://thread.gmane.org/gmane.linux.scsi/101388/focus=101380) I see 
that the new scsi_dh_alua code keeps track of the target port group 
(TPG) ID and relative target port (RTP) ID. As you know this information 
can be queried for each LUN via the Device Identification VPD page. How 
about caching the TPG and RTP IDs per LUN such that the scsi_dh_alua 
code can figure out which LUNs are associated with which target ports by 
iterating over the known LUNs ?

Thanks,

Bart.

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

[Hannes Reinecke]

On 07/07/2015 10:48 PM, Bart Van Assche wrote:
> On 07/06/2015 11:18 PM, Hannes Reinecke wrote:
>> However, to handle the above case correctly we would need to keep
>> track of the entire multipath topology, to figure out which devices
>> belong to that relative target port and might need to be updated
>> (there might be several paths in standby, and we will have sent the
>> RTPG only for one of them).
>> Patches for that are not done yet, so I thought the above patch
>> would be a simple stop-gap measure.
> 
> Hello Hannes,
> 
> Are you sure that keeping track of the entire multipath topology
> would be required to implement what I proposed ? In the patch
> "scsi_dh_alua: Use separate alua_port_group structure"
> (http://thread.gmane.org/gmane.linux.scsi/101388/focus=101380) I see
> that the new scsi_dh_alua code keeps track of the target port group
> (TPG) ID and relative target port (RTP) ID. As you know this
> information can be queried for each LUN via the Device
> Identification VPD page. How about caching the TPG and RTP IDs per
> LUN such that the scsi_dh_alua code can figure out which LUNs are
> associated with which target ports by iterating over the known LUNs ?
> 
I did intentionally _not_ store a pointer to the attached LUN in the
alua_port_group structure, as this would induce yet another
potential race condition when devices are being removed.

But meanwhile I've come up with a simpler patch which handles this
rather elegantly (methinks :-), and doesn't require any additional
infrastructure.
I'll be posting it shortly.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
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