--- 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);
}
On Sun, 28 Jan 2007 19:47:27 -0600
Robert Hancock <[email protected]> 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);
> }
>
>
>
>
>
>
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 <[email protected]>
--
Robert Hancock Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/
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
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 [email protected]
Home Page: http://www.roberthancock.com/
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/
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 [email protected]
Home Page: http://www.roberthancock.com/