sd: spin down disks on removal or power-down

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.

drivers/scsi/sd.c +29 -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);

drivers/scsi/sd.c +2 -0

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

triviata:

drivers/scsi/sd.c static int sd_sync_cache(struct scsi_dev · line 824

Quoted 7 lines

drivers/scsi/sd.c +1 -0

@@ -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/* / */

drivers/scsi/sd.c static int sd_sync_cache(struct scsi_dev · line 828

Quoted 4 lines

drivers/scsi/sd.c +4 -0

+{
+	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?

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.

Quoted 7 lines · Andrew Morton

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

Quoted 1 line

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).

Quoted 5 lines · Robert Hancock

Quoted 1 line

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?