[PATCH RFC] sd: spin down disks on removal or power-down

[PATCH RFC] sd: spin down disks on removal or power-down

[Robert Hancock]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

Here's a patch for sd.c I've cooked up which issues a START STOP UNIT
command to stop the drive when the SCSI disk is removed or the machine
is powered down. The rationale behind this is that apparently on many
drives, simply cutting power to the spinning disk forces it to do an
emergency head park/unload which creates more wear on the drive then a
controlled park/unload with power still applied. This change ensures
that the drive will be spun down if the user shuts down the machine or
if they are about to hot-unplug the drive and have done "scsiadd -r" first.

The main potential concern I have about this implementation is that if
the drive is used in a multi-initiator, iSCSI, etc. environment,
stopping the drive may be undesirable as another initiator may still be
accessing it. I'm not familiar enough with these setups to know if this
problem is likely to come up or not. For this and other reasons we may
want to make this behavior controllable - I'm open to suggestions on how
to do this or whether it's needed.

I've tested that this does work on SATA disks through libata (with my
patch "libata: fix translation for START STOP UNIT" applied). I also
tested with some external USB-to-IDE drive enclosures. The support for
START STOP UNIT on those seems rather poor though, on one enclosure with
a Genesys bridge chip it returned a check condition with "Invalid field
in CDB", and on another with a JMicron chip the request succeeded but it
didn't actually spin the drive down.

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/





[-- Attachment #2: sd-stop-unit-on-shutdown.patch --]
[-- Type: text/plain, Size: 1792 bytes --]

--- linux-2.6.20-rc6nv/drivers/scsi/sd.c	2007-01-28 17:00:00.000000000 -0600
+++ linux-2.6.20-rc6nvedit/drivers/scsi/sd.c	2007-01-28 18:08:53.000000000 -0600
@@ -821,6 +821,39 @@ static int sd_sync_cache(struct scsi_dev
 	return res;
 }
 
+static int sd_stop_unit(struct scsi_device *sdp, struct scsi_disk* sdkp)
+{
+	int res;
+	struct scsi_sense_hdr sshdr;
+	unsigned char cmd[10] = { 0 };
+
+	if (!scsi_device_online(sdp))
+		return -ENODEV;
+
+	cmd[0] = START_STOP;
+	/*
+	 * Leave the rest of the command zero to indicate
+	 * transition to stopped power condition and return
+	 * on completion.
+	 */
+	printk(KERN_NOTICE "Stopping SCSI disk %s\n",
+		sdkp->disk->disk_name);
+	res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
+			       SD_TIMEOUT, SD_MAX_RETRIES);
+
+	if (res) {
+		printk(KERN_WARNING "sd stop failed: status = %x, message = %02x, "
+				    "host = %d, driver = %02x\n",
+				    status_byte(res), msg_byte(res),
+				    host_byte(res), driver_byte(res));
+		if (driver_byte(res) & DRIVER_SENSE)
+			scsi_print_sense_hdr("sd", &sshdr);
+	}
+	
+	return res;
+}
+
+
 static int sd_issue_flush(struct device *dev, sector_t *error_sector)
 {
 	int ret = 0;
@@ -1727,11 +1760,13 @@ static int sd_probe(struct device *dev)
  **/
 static int sd_remove(struct device *dev)
 {
+	struct scsi_device *sdp = to_scsi_device(dev);
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 
 	class_device_del(&sdkp->cdev);
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);
+	sd_stop_unit(sdp,sdkp);
 
 	mutex_lock(&sd_ref_mutex);
 	dev_set_drvdata(dev, NULL);
@@ -1784,6 +1819,9 @@ static void sd_shutdown(struct device *d
 				sdkp->disk->disk_name);
 		sd_sync_cache(sdp);
 	}
+	if(system_state == SYSTEM_POWER_OFF)
+		sd_stop_unit(sdp,sdkp);
+		
 	scsi_disk_put(sdkp);
 }
 

Re: [PATCH RFC] sd: spin down disks on removal or power-down

[Andrew Morton]

On Sun, 28 Jan 2007 19:47:27 -0600
Robert Hancock <hancockr@shaw.ca> wrote:

> Here's a patch for sd.c I've cooked up which issues a START STOP UNIT
> command to stop the drive when the SCSI disk is removed or the machine
> is powered down. The rationale behind this is that apparently on many
> drives, simply cutting power to the spinning disk forces it to do an
> emergency head park/unload which creates more wear on the drive then a
> controlled park/unload with power still applied. This change ensures
> that the drive will be spun down if the user shuts down the machine or
> if they are about to hot-unplug the drive and have done "scsiadd -r" first.
> 
> The main potential concern I have about this implementation is that if
> the drive is used in a multi-initiator, iSCSI, etc. environment,
> stopping the drive may be undesirable as another initiator may still be
> accessing it. I'm not familiar enough with these setups to know if this
> problem is likely to come up or not. For this and other reasons we may
> want to make this behavior controllable - I'm open to suggestions on how
> to do this or whether it's needed.
> 
> I've tested that this does work on SATA disks through libata (with my
> patch "libata: fix translation for START STOP UNIT" applied). I also
> tested with some external USB-to-IDE drive enclosures. The support for
> START STOP UNIT on those seems rather poor though, on one enclosure with
> a Genesys bridge chip it returned a check condition with "Invalid field
> in CDB", and on another with a JMicron chip the request succeeded but it
> didn't actually spin the drive down.
> 

What we don't want to happen is for those disks to spin down during a reboot.
It seems that this is OK with this patch.

Also, we probably don't want them to be spun down during a kexec_load, but
I expect that's OK too.

triviata:

> --- linux-2.6.20-rc6nv/drivers/scsi/sd.c	2007-01-28 17:00:00.000000000 -0600
> +++ linux-2.6.20-rc6nvedit/drivers/scsi/sd.c	2007-01-28 18:08:53.000000000 -0600
> @@ -821,6 +821,39 @@ static int sd_sync_cache(struct scsi_dev
>  	return res;
>  }
>  
> +static int sd_stop_unit(struct scsi_device *sdp, struct scsi_disk* sdkp)

s/* / */

> +{
> +	int res;
> +	struct scsi_sense_hdr sshdr;
> +	unsigned char cmd[10] = { 0 };

I don't think this initialisation-to-all-zeroes is needed, is it?

> +	if (!scsi_device_online(sdp))
> +		return -ENODEV;
> +
> +	cmd[0] = START_STOP;
> +	/*
> +	 * Leave the rest of the command zero to indicate
> +	 * transition to stopped power condition and return
> +	 * on completion.
> +	 */
> +	printk(KERN_NOTICE "Stopping SCSI disk %s\n",
> +		sdkp->disk->disk_name);
> +	res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
> +			       SD_TIMEOUT, SD_MAX_RETRIES);
> +
> +	if (res) {
> +		printk(KERN_WARNING "sd stop failed: status = %x, message = %02x, "
> +				    "host = %d, driver = %02x\n",
> +				    status_byte(res), msg_byte(res),
> +				    host_byte(res), driver_byte(res));
> +		if (driver_byte(res) & DRIVER_SENSE)
> +			scsi_print_sense_hdr("sd", &sshdr);
> +	}
> +	
> +	return res;
> +}
> +
> +
>  static int sd_issue_flush(struct device *dev, sector_t *error_sector)
>  {
>  	int ret = 0;
> @@ -1727,11 +1760,13 @@ static int sd_probe(struct device *dev)
>   **/
>  static int sd_remove(struct device *dev)
>  {
> +	struct scsi_device *sdp = to_scsi_device(dev);
>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
>  
>  	class_device_del(&sdkp->cdev);
>  	del_gendisk(sdkp->disk);
>  	sd_shutdown(dev);
> +	sd_stop_unit(sdp,sdkp);
>  
>  	mutex_lock(&sd_ref_mutex);
>  	dev_set_drvdata(dev, NULL);
> @@ -1784,6 +1819,9 @@ static void sd_shutdown(struct device *d
>  				sdkp->disk->disk_name);
>  		sd_sync_cache(sdp);
>  	}
> +	if(system_state == SYSTEM_POWER_OFF)

s/if(/if (/

> +		sd_stop_unit(sdp,sdkp);
> +		
>  	scsi_disk_put(sdkp);
>  }
>  
> 
> 
> 
> 
> 

Re: [PATCH RFC] sd: spin down disks on removal or power-down

[Robert Hancock]

Andrew Morton wrote:
> triviata:
> 
>> --- linux-2.6.20-rc6nv/drivers/scsi/sd.c	2007-01-28 17:00:00.000000000 -0600
>> +++ linux-2.6.20-rc6nvedit/drivers/scsi/sd.c	2007-01-28 18:08:53.000000000 -0600
>> @@ -821,6 +821,39 @@ static int sd_sync_cache(struct scsi_dev
>>  	return res;
>>  }
>>  
>> +static int sd_stop_unit(struct scsi_device *sdp, struct scsi_disk* sdkp)
> 
> s/* / */
> 
>> +{
>> +	int res;
>> +	struct scsi_sense_hdr sshdr;
>> +	unsigned char cmd[10] = { 0 };
> 
> I don't think this initialisation-to-all-zeroes is needed, is it?

Pretty sure it is, the rest of the command needs to be set to 0. Without 
it the other 9 bytes will contain uninitialized junk.

For the other cleanup changes, though:

Signed-off-by: Robert Hancock <hancockr@shaw.ca>

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: [PATCH RFC] sd: spin down disks on removal or power-down

[James Bottomley]

On Mon, 2007-01-29 at 15:47 -0800, Andrew Morton wrote:
> What we don't want to happen is for those disks to spin down during a
> reboot.
> It seems that this is OK with this patch.
> 
> Also, we probably don't want them to be spun down during a kexec_load,
> but
> I expect that's OK too.

Actually, there's another case where we don't want to do spin down, and
that's when we don't own the disc (think external array on a SAN).
Telling this has always been the biggest stumbling block to spin down of
SCSI devices.

There's a patch similar to yours which takes this into account, which is
currently under discussion:

http://marc.theaimsgroup.com/?t=116922621200002

James

Re: [PATCH RFC] sd: spin down disks on removal or power-down

[Robert Hancock]

James Bottomley wrote:
> On Mon, 2007-01-29 at 15:47 -0800, Andrew Morton wrote:
>> What we don't want to happen is for those disks to spin down during a
>> reboot.
>> It seems that this is OK with this patch.
>>
>> Also, we probably don't want them to be spun down during a kexec_load,
>> but
>> I expect that's OK too.
> 
> Actually, there's another case where we don't want to do spin down, and
> that's when we don't own the disc (think external array on a SAN).
> Telling this has always been the biggest stumbling block to spin down of
> SCSI devices.
> 
> There's a patch similar to yours which takes this into account, which is
> currently under discussion:
> 
> http://marc.theaimsgroup.com/?t=116922621200002

It looks like Tejun's patch essentially does the same thing as mine with 
the addition of the control from userspace. There is one exception 
though, my patch also does the stop on removal of the SCSI disk (i.e. 
writing 1 to its "delete" file in sysfs, what scsiadd -r does). I think 
it makes sense if the user selected the disk to be spun down on shutdown 
to do it on removal as well, as it is potentially about to be physically 
removed/powered down (especially for USB or FireWire disks).

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: [PATCH RFC] sd: spin down disks on removal or power-down

[Stefan Richter]

Robert Hancock wrote:
>> http://marc.theaimsgroup.com/?t=116922621200002
> 
> It looks like Tejun's patch essentially does the same thing as mine with
> the addition of the control from userspace. There is one exception
> though, my patch also does the stop on removal of the SCSI disk (i.e.
> writing 1 to its "delete" file in sysfs, what scsiadd -r does).

Isn't sd_shutdown called at this occasion?
-- 
Stefan Richter
-=====-=-=== ---= =====
http://arcgraph.de/sr/

Re: [PATCH RFC] sd: spin down disks on removal or power-down

[Robert Hancock]

Stefan Richter wrote:
> Robert Hancock wrote:
>>> http://marc.theaimsgroup.com/?t=116922621200002
>> It looks like Tejun's patch essentially does the same thing as mine with
>> the addition of the control from userspace. There is one exception
>> though, my patch also does the stop on removal of the SCSI disk (i.e.
>> writing 1 to its "delete" file in sysfs, what scsiadd -r does).
> 
> Isn't sd_shutdown called at this occasion?

Ah, indeed it is. I was thinking of the way I had written my patch, 
where it only does the spindown if the system state is SYSTEM_POWER_OFF 
so I needed an extra call to force this on removal. Tejun's patch just 
checks for not SYSTEM_RESTART, so it should happen on removal as well.

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/