LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrew Morton <akpm@osdl.org>
To: Robert Hancock <hancockr@shaw.ca>
Cc: linux-kernel <linux-kernel@vger.kernel.org>,
	linux-ide@vger.kernel.org, linux-scsi@vger.kernel.org
Subject: Re: [PATCH RFC] sd: spin down disks on removal or power-down
Date: Mon, 29 Jan 2007 15:47:06 -0800	[thread overview]
Message-ID: <20070129154706.dfb3edab.akpm@osdl.org> (raw)
In-Reply-To: <45BD522F.3070706@shaw.ca>

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);
>  }
>  
> 
> 
> 
> 
> 

  reply	other threads:[~2007-01-29 23:47 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-01-29  1:47 Robert Hancock
2007-01-29 23:47 ` Andrew Morton [this message]
2007-01-29 23:55   ` Robert Hancock
2007-01-30  0:16   ` James Bottomley
2007-01-30  0:33     ` Robert Hancock
2007-01-31 20:21       ` Stefan Richter
2007-01-31 23:45         ` Robert Hancock

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070129154706.dfb3edab.akpm@osdl.org \
    --to=akpm@osdl.org \
    --cc=hancockr@shaw.ca \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --subject='Re: [PATCH RFC] sd: spin down disks on removal or power-down' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).