| Message ID | 1436181130-82905-1-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- 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
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 -- 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
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
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. -- 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
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
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--;
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(+)