[PATCH 0/3] REAC CAPACITY fixes

[PATCH 0/3] REAC CAPACITY fixes

[Hannes Reinecke]

Hi all,

here's a small patch series as an attempt to fix up the READ CAPACITY
issue on ALUA devices. The problem is that an ALUA device in state
'standby' or 'transitioning' might return the READ CAPACITY command
with a sense code, causing the sd driver to report an error in
sd_read_capacity.
This patchset fixes up this situation  so that these devices will be
reported with a capacity of '0', but marked with a flag
'invalid_capacity'. So whenever we get a sense code indicating that
the ALUA state has changed or if the 'rescan' attribute was triggered
this flag will be reset if the capacity could be successfully read.

Hannes Reinecke (3):
  sd: Fixup capacity for ALUA standby or transitioning ports
  scsi: rescan device if an invalid capacity had been reported
  sd: do not try to spin-up disks for ALUA 'transitioning' state

 drivers/scsi/scsi_lib.c    |  4 ++++
 drivers/scsi/sd.c          | 35 ++++++++++++++++++++++++++++++++++-
 include/scsi/scsi_device.h |  1 +
 3 files changed, 39 insertions(+), 1 deletion(-)

-- 
1.8.5.2

[PATCH 1/3] sd: Fixup capacity for ALUA standby or transitioning ports

[Hannes Reinecke]

If a target port is in ALUA 'standby' or 'transitioning' state it
might not need to respond to a 'READ CAPACITY' command. So fixup
the initialization for these cases.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/sd.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3b2fcb4..03fdfa9 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1981,6 +1981,16 @@ 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)
+				/* Target port in transition */
+				return 0;
+			if (sense_valid &&
+			    sshdr.sense_key == NOT_READY &&
+			    sshdr.asc == 0x04 && sshdr.ascq == 0x0b)
+				/* Target port in standy state */
+				return 0;
 		}
 		retries--;
 
@@ -2063,6 +2073,16 @@ 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)
+				/* Target port in transition */
+				return 0;
+			if (sense_valid &&
+			    sshdr.sense_key == NOT_READY &&
+			    sshdr.asc == 0x04 && sshdr.ascq == 0x0b)
+				/* Target port in standy state */
+				return 0;
 		}
 		retries--;
 
@@ -2177,6 +2197,8 @@ got_data:
 		sector_size = 512;
 		sd_printk(KERN_NOTICE, sdkp, "Sector size 0 reported, "
 			  "assuming 512.\n");
+		if (!sdkp->physical_block_size)
+			sdkp->physical_block_size = sector_size;
 	}
 
 	if (sector_size != 512 &&
-- 
1.8.5.2

[PATCH 2/3] scsi: rescan device if an invalid capacity had been reported

[Hannes Reinecke]

Device paths in ALUA state 'standby' do not necessarily support
the READ_CAPACITY command. This patch adds a new flag 'invalid_capacity'
to the scsi device, and rescans the device if an ALUA state
change occurred.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_lib.c    |  4 ++++
 drivers/scsi/sd.c          | 19 ++++++++++++++-----
 include/scsi/scsi_device.h |  1 +
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c005e42..d4245f0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2701,6 +2701,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 		envp[idx++] = "SDEV_UA=INQUIRY_DATA_HAS_CHANGED";
 		break;
 	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
+		scsi_rescan_device(&sdev->sdev_gendev);
 		envp[idx++] = "SDEV_UA=CAPACITY_DATA_HAS_CHANGED";
 		break;
 	case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
@@ -2713,6 +2714,9 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 		envp[idx++] = "SDEV_UA=REPORTED_LUNS_DATA_HAS_CHANGED";
 		break;
 	case SDEV_EVT_ALUA_STATE_CHANGE_REPORTED:
+		if (sdev->invalid_capacity)
+			scsi_rescan_device(&sdev->sdev_gendev);
+
 		envp[idx++] = "SDEV_UA=ASYMMETRIC_ACCESS_STATE_CHANGED";
 		break;
 	default:
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 03fdfa9..7c0bdaa 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1983,14 +1983,18 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 					continue;
 			if (sense_valid &&
 			    sshdr.sense_key == NOT_READY &&
-			    sshdr.asc == 0x04 && sshdr.ascq == 0x0a)
+			    sshdr.asc == 0x04 && sshdr.ascq == 0x0a) {
 				/* Target port in transition */
+				sdp->invalid_capacity = 1;
 				return 0;
+			}
 			if (sense_valid &&
 			    sshdr.sense_key == NOT_READY &&
-			    sshdr.asc == 0x04 && sshdr.ascq == 0x0b)
+			    sshdr.asc == 0x04 && sshdr.ascq == 0x0b) {
 				/* Target port in standy state */
+				sdp->invalid_capacity = 1;
 				return 0;
+			}
 		}
 		retries--;
 
@@ -2075,14 +2079,18 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 					continue;
 			if (sense_valid &&
 			    sshdr.sense_key == NOT_READY &&
-			    sshdr.asc == 0x04 && sshdr.ascq == 0x0a)
+			    sshdr.asc == 0x04 && sshdr.ascq == 0x0a) {
 				/* Target port in transition */
+				sdp->invalid_capacity = 1;
 				return 0;
+			}
 			if (sense_valid &&
 			    sshdr.sense_key == NOT_READY &&
-			    sshdr.asc == 0x04 && sshdr.ascq == 0x0b)
+			    sshdr.asc == 0x04 && sshdr.ascq == 0x0b) {
 				/* Target port in standy state */
+				sdp->invalid_capacity = 1;
 				return 0;
+			}
 		}
 		retries--;
 
@@ -2199,7 +2207,8 @@ got_data:
 			  "assuming 512.\n");
 		if (!sdkp->physical_block_size)
 			sdkp->physical_block_size = sector_size;
-	}
+	} else
+		sdp->invalid_capacity = 0;
 
 	if (sector_size != 512 &&
 	    sector_size != 1024 &&
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 50c2a36..99bde5a 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -175,6 +175,7 @@ struct scsi_device {
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
 	unsigned broken_fua:1;		/* Don't set FUA bit */
 	unsigned lun_in_cdb:1;		/* Store LUN bits in CDB[1] */
+	unsigned invalid_capacity:1;	/* READ_CAPACITY not supported */
 
 	atomic_t disk_events_disable_depth; /* disable depth for disk events */
 
-- 
1.8.5.2

[PATCH 3/3] sd: do not try to spin-up disks for ALUA 'transitioning' state

[Hannes Reinecke]

If a disk reports an ALUA 'transitioning' state we should not
try to spin up the device.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/sd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 7c0bdaa..180a6e8 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1801,6 +1801,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 		if (sense_valid && sshdr.sense_key == NOT_READY) {
 			if (sshdr.asc == 4 && sshdr.ascq == 3)
 				break;	/* manual intervention required */
+			if (sshdr.asc == 4 && sshdr.ascq == 0xa)
+				break;  /* transitioning */
 			if (sshdr.asc == 4 && sshdr.ascq == 0xb)
 				break;	/* standby */
 			if (sshdr.asc == 4 && sshdr.ascq == 0xc)
-- 
1.8.5.2

Re: [PATCH 3/3] sd: do not try to spin-up disks for ALUA 'transitioning' state

[Sagi Grimberg]

On 7/8/2015 10:41 AM, Hannes Reinecke wrote:
> If a disk reports an ALUA 'transitioning' state we should not
> try to spin up the device.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/sd.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 7c0bdaa..180a6e8 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1801,6 +1801,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
>   		if (sense_valid && sshdr.sense_key == NOT_READY) {
>   			if (sshdr.asc == 4 && sshdr.ascq == 3)
>   				break;	/* manual intervention required */
> +			if (sshdr.asc == 4 && sshdr.ascq == 0xa)
> +				break;  /* transitioning */
>   			if (sshdr.asc == 4 && sshdr.ascq == 0xb)
>   				break;	/* standby */
>   			if (sshdr.asc == 4 && sshdr.ascq == 0xc)
>


Hi Hannes,

Just nit-picking, but do you think that these four if statements can be
re-organized to condition (asc == 4) once and OR on the rest?

Re: [PATCH 3/3] sd: do not try to spin-up disks for ALUA 'transitioning' state

[Hannes Reinecke]

On 07/08/2015 10:41 AM, Sagi Grimberg wrote:
> On 7/8/2015 10:41 AM, Hannes Reinecke wrote:
>> If a disk reports an ALUA 'transitioning' state we should not
>> try to spin up the device.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/sd.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 7c0bdaa..180a6e8 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -1801,6 +1801,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
>>           if (sense_valid && sshdr.sense_key == NOT_READY) {
>>               if (sshdr.asc == 4 && sshdr.ascq == 3)
>>                   break;    /* manual intervention required */
>> +            if (sshdr.asc == 4 && sshdr.ascq == 0xa)
>> +                break;  /* transitioning */
>>               if (sshdr.asc == 4 && sshdr.ascq == 0xb)
>>                   break;    /* standby */
>>               if (sshdr.asc == 4 && sshdr.ascq == 0xc)
>>
> 
> 
> Hi Hannes,
> 
> Just nit-picking, but do you think that these four if statements can be
> re-organized to condition (asc == 4) once and OR on the rest?
Sure they can, I don't mind.
Once we have a general agreement about these patches (hint, hint :-)
I can update it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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: [PATCH 2/3] scsi: rescan device if an invalid capacity had been reported

[Lee Duncan]

On 07/08/2015 12:41 AM, Hannes Reinecke wrote:
> Device paths in ALUA state 'standby' do not necessarily support
> the READ_CAPACITY command. This patch adds a new flag 'invalid_capacity'
> to the scsi device, and rescans the device if an ALUA state
> change occurred.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_lib.c    |  4 ++++
>  drivers/scsi/sd.c          | 19 ++++++++++++++-----
>  include/scsi/scsi_device.h |  1 +
>  3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index c005e42..d4245f0 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2701,6 +2701,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>  		envp[idx++] = "SDEV_UA=INQUIRY_DATA_HAS_CHANGED";
>  		break;
>  	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
> +		scsi_rescan_device(&sdev->sdev_gendev);
>  		envp[idx++] = "SDEV_UA=CAPACITY_DATA_HAS_CHANGED";
>  		break;
>  	case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
> @@ -2713,6 +2714,9 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>  		envp[idx++] = "SDEV_UA=REPORTED_LUNS_DATA_HAS_CHANGED";
>  		break;
>  	case SDEV_EVT_ALUA_STATE_CHANGE_REPORTED:
> +		if (sdev->invalid_capacity)
> +			scsi_rescan_device(&sdev->sdev_gendev);
> +
>  		envp[idx++] = "SDEV_UA=ASYMMETRIC_ACCESS_STATE_CHANGED";
>  		break;
>  	default:
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 03fdfa9..7c0bdaa 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1983,14 +1983,18 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  					continue;
>  			if (sense_valid &&
>  			    sshdr.sense_key == NOT_READY &&
> -			    sshdr.asc == 0x04 && sshdr.ascq == 0x0a)
> +			    sshdr.asc == 0x04 && sshdr.ascq == 0x0a) {
>  				/* Target port in transition */
> +				sdp->invalid_capacity = 1;
>  				return 0;
> +			}
>  			if (sense_valid &&
>  			    sshdr.sense_key == NOT_READY &&
> -			    sshdr.asc == 0x04 && sshdr.ascq == 0x0b)
> +			    sshdr.asc == 0x04 && sshdr.ascq == 0x0b) {
>  				/* Target port in standy state */
> +				sdp->invalid_capacity = 1;
>  				return 0;
> +			}
>  		}
>  		retries--;
>  
> @@ -2075,14 +2079,18 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  					continue;
>  			if (sense_valid &&
>  			    sshdr.sense_key == NOT_READY &&
> -			    sshdr.asc == 0x04 && sshdr.ascq == 0x0a)
> +			    sshdr.asc == 0x04 && sshdr.ascq == 0x0a) {
>  				/* Target port in transition */
> +				sdp->invalid_capacity = 1;
>  				return 0;
> +			}
>  			if (sense_valid &&
>  			    sshdr.sense_key == NOT_READY &&
> -			    sshdr.asc == 0x04 && sshdr.ascq == 0x0b)
> +			    sshdr.asc == 0x04 && sshdr.ascq == 0x0b) {
>  				/* Target port in standy state */
> +				sdp->invalid_capacity = 1;
>  				return 0;
> +			}
>  		}
>  		retries--;
>  
> @@ -2199,7 +2207,8 @@ got_data:
>  			  "assuming 512.\n");
>  		if (!sdkp->physical_block_size)
>  			sdkp->physical_block_size = sector_size;
> -	}
> +	} else
> +		sdp->invalid_capacity = 0;
>  
>  	if (sector_size != 512 &&
>  	    sector_size != 1024 &&
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 50c2a36..99bde5a 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -175,6 +175,7 @@ struct scsi_device {
>  	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
>  	unsigned broken_fua:1;		/* Don't set FUA bit */
>  	unsigned lun_in_cdb:1;		/* Store LUN bits in CDB[1] */
> +	unsigned invalid_capacity:1;	/* READ_CAPACITY not supported */

Not sure I like the comment for the new field. How about something like
"current capacity unknown"?

>  
>  	atomic_t disk_events_disable_depth; /* disable depth for disk events */
>  
> 

-- 
Lee Duncan
SUSE Labs