PATCH 1/1] : Spinning up disk is observed on standby paths until timeout, resulting in longer path restoration time.

PATCH 1/1] : Spinning up disk is observed on standby paths until timeout, resulting in longer path restoration time.

[Rengarajan, Narayanan (STSD)]

Sorry if sent already

Hi,

  Spinning up disk is observed on standby paths until timeout, resulting in longer path restoration time in 2.6.27 kernel.

 Steps to reproduce:
 1. present a standby lun to the host
 2. do a discovery from the host (scan the scsi bus)  3. Spinning of disks is  observed in  /var/log/messages

Whenever a device goes offline and comes back, the new sd device takes longer time to get created. This is because of the spinning up of disk in sd_spinup_disk fuction as the standby paths would return device not ready with 0x04/0x0b asc/ascq.

Recommended patch :

  diff -pNaur /usr/src/linux/drivers/scsi/sd.c sd.c
--- /usr/src/linux/drivers/scsi/sd.c    2009-02-09 22:24:56.000000000 +0530
+++ sd.c        2009-02-19 16:39:16.000000000 +0530
@@ -1181,8 +1181,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
                 */
                if (sense_valid &&
                    sshdr.sense_key == NOT_READY &&
-                   sshdr.asc == 4 && sshdr.ascq == 3) {
-                       break;          /* manual intervention required */
+                   sshdr.asc == 4 && (sshdr.ascq == 3 || sshdr.ascq == 
+ 0x0b ||
sshdr.ascq == 0x0c) ) {
+                       break;  /* manual intervention required || 
+ Standby ||
Unavailable */

 signed off : narayanan.rengarajan@hp.com

Narayanan

Re: PATCH 1/1] : Spinning up disk is observed on standby paths until timeout, resulting in longer path restoration time.

[Matthew Wilcox]

On Fri, Feb 20, 2009 at 12:03:58PM +0000, Rengarajan, Narayanan (STSD) wrote:
>   Spinning up disk is observed on standby paths until timeout, resulting in longer path restoration time in 2.6.27 kernel.
> 
>  Steps to reproduce:
>  1. present a standby lun to the host
>  2. do a discovery from the host (scan the scsi bus)  3. Spinning of disks is  observed in  /var/log/messages
> 
> Whenever a device goes offline and comes back, the new sd device takes longer time to get created. This is because of the spinning up of disk in sd_spinup_disk fuction as the standby paths would return device not ready with 0x04/0x0b asc/ascq.

I think your analysis is correct.

It's probably better for you to not use Outlook to send email here; it's
failed at line wrapping and it's mangled your patch quite badly.  See
Documentation/email-clients.txt for more details.

> Recommended patch :
> 
>   diff -pNaur /usr/src/linux/drivers/scsi/sd.c sd.c
> --- /usr/src/linux/drivers/scsi/sd.c    2009-02-09 22:24:56.000000000 +0530
> +++ sd.c        2009-02-19 16:39:16.000000000 +0530
> @@ -1181,8 +1181,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
>                  */
>                 if (sense_valid &&
>                     sshdr.sense_key == NOT_READY &&
> -                   sshdr.asc == 4 && sshdr.ascq == 3) {
> -                       break;          /* manual intervention required */
> +                   sshdr.asc == 4 && (sshdr.ascq == 3 || sshdr.ascq == 
> + 0x0b ||
> sshdr.ascq == 0x0c) ) {
> +                       break;  /* manual intervention required || 
> + Standby ||
> Unavailable */
> 
>  signed off : narayanan.rengarajan@hp.com

I don't like the patch though.  Apart from being badly mangled, it makes
the if condition rather unbalanced and hard to read.  I think the patch
would be better.

Even this patch, I wonder about.  I've looked at the ASC/ASCQ tables for
'NOT_READY', and really only ASC 4, ASCQ 2 seems to require the
START_STOP command to be sent.  Should we invert the sense of these
tests so we only send START_STOP in cases where it's known to do some
good?

---

Don't try to spin up drives that are connected to an inactive port

We currently try to spin up drives connected to standby and unavailable
ports.  This will never succeed and wastes a lot of time.  Fail quickly
if the sense data reports the port is in standby or unavailable state.

Reported-by: Narayanan Rengarajan <narayanan.rengarajan@hp.com>
Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d57566b..289cd46 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1166,23 +1166,19 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 		/*
 		 * The device does not want the automatic start to be issued.
 		 */
-		if (sdkp->device->no_start_on_add) {
+		if (sdkp->device->no_start_on_add)
 			break;
-		}
-
-		/*
-		 * If manual intervention is required, or this is an
-		 * absent USB storage device, a spinup is meaningless.
-		 */
-		if (sense_valid &&
-		    sshdr.sense_key == NOT_READY &&
-		    sshdr.asc == 4 && sshdr.ascq == 3) {
-			break;		/* manual intervention required */
 
-		/*
-		 * Issue command to spin up drive when not ready
-		 */
-		} else if (sense_valid && sshdr.sense_key == NOT_READY) {
+		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 == 0xb)
+				break;	/* standby */
+			if (sshdr.asc == 4 && sshdr.ascq == 0xc)
+				break;	/* unavailable */
+			/*
+			 * Issue command to spin up drive when not ready
+			 */
 			if (!spintime) {
 				sd_printk(KERN_NOTICE, sdkp, "Spinning up disk...");
 				cmd[0] = START_STOP;

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

RE: PATCH 1/1] : Spinning up disk is observed on standby paths until timeout, resulting in longer path restoration time.

[Rengarajan, Narayanan (STSD)]

 I ve verified the patch by Matthew Wilcox under 2.6.29-rc6

 without patch:
 
  #tail -f /var/log/messages
  
Feb 26 17:23:44 xyz kernel: sd 13:0:2:1: [sdca] Spinning up disk.......................................................................................................not responding...
Feb 26 17:23:44 xyz kernel: sd 13:0:2:1: [sdca] 10485760 512-byte hardware sectors: (5.36 GB/5.00 GiB)
Feb 26 17:23:44 xyz kernel: sd 13:0:2:1: [sdca] Write Protect is off
Feb 26 17:23:44 xyz kernel: sd 13:0:2:1: [sdca] Mode Sense: af 00 00 08
Feb 26 17:23:44 xyz kernel: sd 13:0:2:1: [sdca] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
Feb 26 17:25:24 xyz kernel: sd 13:0:2:1: [sdca] Spinning up disk.......................................................................................................not responding...

 With patch:

# tail -f /var/log/messages

Feb 26 18:13:47 xyz kernel: sd 12:0:0:2: [sdcc] 6291456 512-byte hardware sectors: (3.22 GB/3.00 GiB)
Feb 26 18:13:47 xyz kernel: sd 12:0:0:2: [sdcc] Write Protect is off
Feb 26 18:13:47 xyz kernel: sd 12:0:0:2: [sdcc] Mode Sense: af 00 00 08
Feb 26 18:13:47 xyz kernel: sd 12:0:0:2: [sdcc] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
Feb 26 18:13:47 xyz kernel: sd 12:0:0:2: [sdcc] 6291456 512-byte hardware sectors: (3.22 GB/3.00 GiB)


 Test Signed-off-by : narayanan.rengarajan@hp.com

-----Original Message-----
From: Matthew Wilcox [mailto:matthew@wil.cx] 
Sent: Friday, February 20, 2009 7:24 PM
To: Rengarajan, Narayanan (STSD)
Cc: linux-scsi@vger.kernel.org
Subject: Re: PATCH 1/1] : Spinning up disk is observed on standby paths until timeout, resulting in longer path restoration time.

On Fri, Feb 20, 2009 at 12:03:58PM +0000, Rengarajan, Narayanan (STSD) wrote:
>   Spinning up disk is observed on standby paths until timeout, resulting in longer path restoration time in 2.6.27 kernel.
> 
>  Steps to reproduce:
>  1. present a standby lun to the host
>  2. do a discovery from the host (scan the scsi bus)  3. Spinning of 
> disks is  observed in  /var/log/messages
> 
> Whenever a device goes offline and comes back, the new sd device takes longer time to get created. This is because of the spinning up of disk in sd_spinup_disk fuction as the standby paths would return device not ready with 0x04/0x0b asc/ascq.

I think your analysis is correct.

It's probably better for you to not use Outlook to send email here; it's failed at line wrapping and it's mangled your patch quite badly.  See Documentation/email-clients.txt for more details.

> Recommended patch :
> 
>   diff -pNaur /usr/src/linux/drivers/scsi/sd.c sd.c
> --- /usr/src/linux/drivers/scsi/sd.c    2009-02-09 22:24:56.000000000 +0530
> +++ sd.c        2009-02-19 16:39:16.000000000 +0530
> @@ -1181,8 +1181,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
>                  */
>                 if (sense_valid &&
>                     sshdr.sense_key == NOT_READY &&
> -                   sshdr.asc == 4 && sshdr.ascq == 3) {
> -                       break;          /* manual intervention required */
> +                   sshdr.asc == 4 && (sshdr.ascq == 3 || sshdr.ascq 
> + == 0x0b ||
> sshdr.ascq == 0x0c) ) {
> +                       break;  /* manual intervention required || 
> + Standby ||
> Unavailable */
> 
>  signed off : narayanan.rengarajan@hp.com

I don't like the patch though.  Apart from being badly mangled, it makes the if condition rather unbalanced and hard to read.  I think the patch would be better.

Even this patch, I wonder about.  I've looked at the ASC/ASCQ tables for 'NOT_READY', and really only ASC 4, ASCQ 2 seems to require the START_STOP command to be sent.  Should we invert the sense of these tests so we only send START_STOP in cases where it's known to do some good?

---

Don't try to spin up drives that are connected to an inactive port

We currently try to spin up drives connected to standby and unavailable ports.  This will never succeed and wastes a lot of time.  Fail quickly if the sense data reports the port is in standby or unavailable state.

Reported-by: Narayanan Rengarajan <narayanan.rengarajan@hp.com>
Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index d57566b..289cd46 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1166,23 +1166,19 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 		/*
 		 * The device does not want the automatic start to be issued.
 		 */
-		if (sdkp->device->no_start_on_add) {
+		if (sdkp->device->no_start_on_add)
 			break;
-		}
-
-		/*
-		 * If manual intervention is required, or this is an
-		 * absent USB storage device, a spinup is meaningless.
-		 */
-		if (sense_valid &&
-		    sshdr.sense_key == NOT_READY &&
-		    sshdr.asc == 4 && sshdr.ascq == 3) {
-			break;		/* manual intervention required */
 
-		/*
-		 * Issue command to spin up drive when not ready
-		 */
-		} else if (sense_valid && sshdr.sense_key == NOT_READY) {
+		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 == 0xb)
+				break;	/* standby */
+			if (sshdr.asc == 4 && sshdr.ascq == 0xc)
+				break;	/* unavailable */
+			/*
+			 * Issue command to spin up drive when not ready
+			 */
 			if (!spintime) {
 				sd_printk(KERN_NOTICE, sdkp, "Spinning up disk...");
 				cmd[0] = START_STOP;

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours.  We can't possibly take such a retrograde step."