[PATCH] libata: remove libata.spindown_compat

[PATCH] libata: remove libata.spindown_compat

[Tejun Heo]

With STANDBYDOWN tracking added, libata.spindown_compat isn't
necessary anymore.  If userspace shutdown(8) issues STANDBYNOW, libata
warns.  If userspace shutdown(8) doesn't issue STANDBYNOW, libata does
the right thing.  Userspace can tell whether kernel supports spindown
by testing whether sysfs node manage_start_stop exists as before.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---

As many distros don't do anything to libata devices on shutdown which
means we can avoid a lot of trouble by tracking spindown status (done)
and which, in turn, obsoletes libata.spindown_compat.  I should have
researched better before staring this mess.  Sorry but the situation
is much better now.  :-)

I'll attach the updated version of http://linux-ata.org/shutdown.html
as a reply to this mail.

If you find any problem in the updated scheme, please let me know.

Thanks.

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index 498ff31..5c8695a 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -328,21 +328,20 @@ Who:   Adrian Bunk <bunk@stusta.de>
 
 ---------------------------
 
-What: libata.spindown_compat module parameter
+What: libata spindown skipping and warning
 When: Dec 2008
-Why:  halt(8) synchronizes caches for and spins down libata disks
-      because libata didn't use to spin down disk on system halt
-      (only synchronized caches).
-      Spin down on system halt is now implemented and can be tested
-      using sysfs node /sys/class/scsi_disk/h:c:i:l/manage_start_stop.
+Why:  Some halt(8) implementations synchronize caches for and spin
+      down libata disks because libata didn't use to spin down disk on
+      system halt (only synchronized caches).
+      Spin down on system halt is now implemented.  sysfs node
+      /sys/class/scsi_disk/h:c:i:l/manage_start_stop is present if
+      spin down support is available.
       Because issuing spin down command to an already spun down disk
-      makes some disks spin up just to spin down again, the old
-      behavior needs to be maintained till userspace tool is updated
-      to check the sysfs node and not to spin down disks with the
-      node set to one.
-      This module parameter is to give userspace tool the time to
-      get updated and should be removed after userspace is
-      reasonably updated.
+      makes some disks spin up just to spin down again, libata tracks
+      device spindown status to skip the extra spindown command and
+      warn about it.
+      This is to give userspace tools the time to get updated and will
+      be removed after userspace is reasonably updated.
 Who:  Tejun Heo <htejun@gmail.com>
 
 ---------------------------
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d5939e6..d3ea7f5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -101,12 +101,6 @@ int libata_noacpi = 1;
 module_param_named(noacpi, libata_noacpi, int, 0444);
 MODULE_PARM_DESC(noacpi, "Disables the use of ACPI in suspend/resume when set");
 
-int ata_spindown_compat = 1;
-module_param_named(spindown_compat, ata_spindown_compat, int, 0644);
-MODULE_PARM_DESC(spindown_compat, "Enable backward compatible spindown "
-		 "behavior.  Will be removed.  More info can be found in "
-		 "Documentation/feature-removal-schedule.txt\n");
-
 MODULE_AUTHOR("Jeff Garzik");
 MODULE_DESCRIPTION("Library module for ATA devices");
 MODULE_LICENSE("GPL");
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b6a1de8..242c43e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -893,7 +893,7 @@ int ata_scsi_change_queue_depth(struct s
 	return queue_depth;
 }
 
-/* XXX: for ata_spindown_compat */
+/* XXX: for spindown warning */
 static void ata_delayed_done_timerfn(unsigned long arg)
 {
 	struct scsi_cmnd *scmd = (void *)arg;
@@ -901,7 +901,7 @@ static void ata_delayed_done_timerfn(uns
 	scmd->scsi_done(scmd);
 }
 
-/* XXX: for ata_spindown_compat */
+/* XXX: for spindown warning */
 static void ata_delayed_done(struct scsi_cmnd *scmd)
 {
 	static struct timer_list timer;
@@ -966,8 +966,7 @@ static unsigned int ata_scsi_start_stop_
 		 * removed.  Read Documentation/feature-removal-schedule.txt
 		 * for more info.
 		 */
-		if (ata_spindown_compat &&
-		    (qc->dev->flags & ATA_DFLAG_SPUNDOWN) &&
+		if ((qc->dev->flags & ATA_DFLAG_SPUNDOWN) &&
 		    (system_state == SYSTEM_HALT ||
 		     system_state == SYSTEM_POWER_OFF)) {
 			static unsigned long warned = 0;
@@ -1395,7 +1394,7 @@ static void ata_scsi_qc_complete(struct
 		}
 	}
 
-	/* XXX: track spindown state for spindown_compat */
+	/* XXX: track spindown state for spindown skipping and warning */
 	if (unlikely(qc->tf.command == ATA_CMD_STANDBY ||
 		     qc->tf.command == ATA_CMD_STANDBYNOW1))
 		qc->dev->flags |= ATA_DFLAG_SPUNDOWN;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 13cb0c9..5e24666 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -58,7 +58,6 @@ extern int atapi_enabled;
 extern int atapi_dmadir;
 extern int libata_fua;
 extern int libata_noacpi;
-extern int ata_spindown_compat;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev);
 extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 			   u64 block, u32 n_block, unsigned int tf_flags,

Re: [PATCH] libata: remove libata.spindown_compat

[Tejun Heo]

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

Here's the updated shutdown.html.  As linux-ide doesn't like html
attachment, I gzipped it.  Let's see if this gets through.  The updated
version is also available at

	http://htj.dyndns.org/shutdown.html

Thanks.

-- 
tejun

[-- Attachment #2: shutdown.html.gz --]
[-- Type: application/x-gzip, Size: 3117 bytes --]

Re: [PATCH] libata: remove libata.spindown_compat

[Jeff Garzik]

Tejun Heo wrote:
> With STANDBYDOWN tracking added, libata.spindown_compat isn't
> necessary anymore.  If userspace shutdown(8) issues STANDBYNOW, libata
> warns.  If userspace shutdown(8) doesn't issue STANDBYNOW, libata does
> the right thing.  Userspace can tell whether kernel supports spindown
> by testing whether sysfs node manage_start_stop exists as before.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> 
> As many distros don't do anything to libata devices on shutdown which
> means we can avoid a lot of trouble by tracking spindown status (done)
> and which, in turn, obsoletes libata.spindown_compat.  I should have
> researched better before staring this mess.  Sorry but the situation
> is much better now.  :-)
> 
> I'll attach the updated version of http://linux-ata.org/shutdown.html
> as a reply to this mail.
> 
> If you find any problem in the updated scheme, please let me know.
> 
> Thanks.
> 
> diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
> index 498ff31..5c8695a 100644
> --- a/Documentation/feature-removal-schedule.txt
> +++ b/Documentation/feature-removal-schedule.txt
> @@ -328,21 +328,20 @@ Who:   Adrian Bunk <bunk@stusta.de>
>  
>  ---------------------------
>  
> -What: libata.spindown_compat module parameter
> +What: libata spindown skipping and warning
>  When: Dec 2008
> -Why:  halt(8) synchronizes caches for and spins down libata disks
> -      because libata didn't use to spin down disk on system halt
> -      (only synchronized caches).
> -      Spin down on system halt is now implemented and can be tested
> -      using sysfs node /sys/class/scsi_disk/h:c:i:l/manage_start_stop.
> +Why:  Some halt(8) implementations synchronize caches for and spin
> +      down libata disks because libata didn't use to spin down disk on
> +      system halt (only synchronized caches).
> +      Spin down on system halt is now implemented.  sysfs node
> +      /sys/class/scsi_disk/h:c:i:l/manage_start_stop is present if
> +      spin down support is available.
>        Because issuing spin down command to an already spun down disk
> -      makes some disks spin up just to spin down again, the old
> -      behavior needs to be maintained till userspace tool is updated
> -      to check the sysfs node and not to spin down disks with the
> -      node set to one.
> -      This module parameter is to give userspace tool the time to
> -      get updated and should be removed after userspace is
> -      reasonably updated.
> +      makes some disks spin up just to spin down again, libata tracks
> +      device spindown status to skip the extra spindown command and
> +      warn about it.
> +      This is to give userspace tools the time to get updated and will
> +      be removed after userspace is reasonably updated.
>  Who:  Tejun Heo <htejun@gmail.com>
>  
>  ---------------------------


applied

you forgot diffstat

Re: [PATCH] libata: remove libata.spindown_compat

[Miquel van Smoorenburg]

On Thu, 2007-05-17 at 16:43 +0200, Tejun Heo wrote:
> 
> I'll attach the updated version of http://linux-ata.org/shutdown.html
> as a reply to this mail.

It says:

>When a disk is powered off, it needs to flush its write cache and then
>unload its heads so that they don't crash onto the recording surfaces.
>Most disks have mechanical mechanism to unload its heads even when
>power is abruptly cut. This is usually called emergency unload and
>causes popping sound on some disks and may have negative effect on the
>lifetime of the disk, so the operating system must tell the disks to
>unload the heads prior to shutting the system down.

>In ATA, this is achieved by issuing FLUSH CACHE followed by STANDBYNOW
>and the IDE drivers (drivers/ide/*) have always issued the sequence
>prior to powering off. libata uses SCSI sd driver as its high level
>disk driver and the sd driver, unfortunately, issues only the cache
>flush command during shutdown. This is mainly because SCSI disks can be
>accessed by multiple initiators (hosts) and spinning down disks because
>one initiator goes down can disturb others. Because of this
>implementation detail, libata drivers up to kernel version 2.6.21 don't
>issue the STANDBYNOW command before powering off.

This is not quite correct.

The reason halt(8) spins down IDE disks is that, way back when (november
2001 I guess, from the changelog), the IDE drivers did not flush the
cache on poweroff. The result was that on some machines, poweroff was so
fast that the power of the drive got turned off while it still had some
unflushed data in the write-cache, resulting in filesystem corruption.
This problem did not exist with SCSI, apparently.

To fix this issue, halt(8) started issueing WIN_STANDBYNOW1 (0xE0) and
WIN_STANDBYNOW2 (0x94) ioctls before halt and poweroff, as that was more
reliable than "flush cache" and the effect was the same.

Mike.

Re: [Pkg-sysvinit-devel] [PATCH] libata: remove libata.spindown_compat

[Tejun Heo]

Miquel van Smoorenburg wrote:
>> In ATA, this is achieved by issuing FLUSH CACHE followed by STANDBYNOW
>> and the IDE drivers (drivers/ide/*) have always issued the sequence
>> prior to powering off. libata uses SCSI sd driver as its high level
>> disk driver and the sd driver, unfortunately, issues only the cache
>> flush command during shutdown. This is mainly because SCSI disks can be
>> accessed by multiple initiators (hosts) and spinning down disks because
>> one initiator goes down can disturb others. Because of this
>> implementation detail, libata drivers up to kernel version 2.6.21 don't
>> issue the STANDBYNOW command before powering off.
> 
> This is not quite correct.
> 
> The reason halt(8) spins down IDE disks is that, way back when (november
> 2001 I guess, from the changelog), the IDE drivers did not flush the
> cache on poweroff. The result was that on some machines, poweroff was so
> fast that the power of the drive got turned off while it still had some
> unflushed data in the write-cache, resulting in filesystem corruption.
> This problem did not exist with SCSI, apparently.
> 
> To fix this issue, halt(8) started issueing WIN_STANDBYNOW1 (0xE0) and
> WIN_STANDBYNOW2 (0x94) ioctls before halt and poweroff, as that was more
> reliable than "flush cache" and the effect was the same.

One culprit there is that, according to the spec, STANDBYNOW doesn't
necessarily imply cache flush and AFAIK issuing STANDBYNOW to libata
devices is to avoid emergency unload.  Can you comment on this Henrique?

Would changing "the IDE drivers have always" to "recent IDE drivers
issue" be enough?

Thanks.

-- 
tejun

Re: [Pkg-sysvinit-devel] [PATCH] libata: remove libata.spindown_compat

[Henrique de Moraes Holschuh]

On Sat, 19 May 2007, Tejun Heo wrote:
> > To fix this issue, halt(8) started issueing WIN_STANDBYNOW1 (0xE0) and
> > WIN_STANDBYNOW2 (0x94) ioctls before halt and poweroff, as that was more
> > reliable than "flush cache" and the effect was the same.
> 
> One culprit there is that, according to the spec, STANDBYNOW doesn't
> necessarily imply cache flush and AFAIK issuing STANDBYNOW to libata
> devices is to avoid emergency unload.  Can you comment on this Henrique?

Well, the reason I raised the ruckus in the first place was just the
emergency unload, yes.  I didn't know about any missing cache flushes (and
AFAIK we never had any reported to us).  We will have to fix that in halt(8)
IMHO, just in case.  And probably hdparm/sdparm should also know to not
standby or sleep disks without a cache flush, either.

I also don't know the history of halt(8), Miquel does... so I have not much
else to comment.

At least now it is clear what we need halt(8) to do (in Debian, anyway):

1. check if we have a responsible kernel or not through sysfs.

	1a: kernel is responsible, and can spin down disks at shutdown

		- if given -h on command line, use sysfs to tell kernel
		  to spin down *all* disk devices we can find in sysfs,
		  and don't send any taskfiles or IOCTLs to the disks.

	1b: kernel is missing this essential feature

		- keep doing what we are doing now, but issue a cache
		  flush before we send any standby commands.

2. push that to Debian unstable, and also to stable-proposed-updates, or at
the very least to backports.org.

This will give maximum compatibility with the kernel, and fix the
cache-not-flushed-before-spindown issue.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [Pkg-sysvinit-devel] [PATCH] libata: remove libata.spindown_compat

[Tejun Heo]

Henrique de Moraes Holschuh wrote:
> Well, the reason I raised the ruckus in the first place was just the
> emergency unload, yes.  I didn't know about any missing cache flushes (and
> AFAIK we never had any reported to us).  We will have to fix that in halt(8)
> IMHO, just in case.  And probably hdparm/sdparm should also know to not
> standby or sleep disks without a cache flush, either.
> 
> I also don't know the history of halt(8), Miquel does... so I have not much
> else to comment.

OIC, hmm.. STANDBYNOW for cache flushing.  Interesting.

> At least now it is clear what we need halt(8) to do (in Debian, anyway):
> 
> 1. check if we have a responsible kernel or not through sysfs.
> 
> 	1a: kernel is responsible, and can spin down disks at shutdown
> 
> 		- if given -h on command line, use sysfs to tell kernel
> 		  to spin down *all* disk devices we can find in sysfs,
> 		  and don't send any taskfiles or IOCTLs to the disks.
> 
> 	1b: kernel is missing this essential feature
> 
> 		- keep doing what we are doing now, but issue a cache
> 		  flush before we send any standby commands.
> 
> 2. push that to Debian unstable, and also to stable-proposed-updates, or at
> the very least to backports.org.
> 
> This will give maximum compatibility with the kernel, and fix the
> cache-not-flushed-before-spindown issue.

Yeap, right.  It would be great if you can push the updates upstream so
that other distros can use it too.  Also, some configurations might
prefer halt not touching the sysfs nodes so that the user can configure
mange_start_stop, but I guess that kind of stuff is up to distro.

Thanks.

-- 
tejun