libata: remove libata.spindown_compat

PATCH Applied Tejun Heo 4 participants 7 messages May 17, 2007 – May 19, 2007

Subsystems (2)

Documentation libata

lore

Share & tools

lore

Tejun Heo

author

2007-05-17 14:44 UTC #0 lore raw

PATCH 1/1 libata: remove libata.spindown_compat

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 <[email protected]>

---

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.

Documentation/feature-removal-schedule.txt +12 ~~-13~~

index 498ff31..5c8695a 100644
@@ -328,21 +328,20 @@ Who:   Adrian Bunk <[email protected]>
 
 ---------------------------
 
-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 <[email protected]>
 
 ---------------------------

drivers/ata/libata-core.c +0 -6

index d5939e6..d3ea7f5 100644
@@ -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");

drivers/ata/libata-scsi.c +4 -5

index b6a1de8..242c43e 100644
@@ -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;

drivers/ata/libata.h +0 -1

index 13cb0c9..5e24666 100644
@@ -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,

Tejun Heo

author

2007-05-17 14:55 UTC #1 lore raw

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.

Signature · 1 line

tejun

Jeff Garzik

Applied

2007-05-18 00:58 UTC #2 lore raw

In reply to Tejun Heo

Quoted 50 lines · 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 <[email protected]>
---
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.
Documentation/feature-removal-schedule.txt +7 ~~-11~~
index 498ff31..5c8695a 100644
@@ -328,21 +328,20 @@ Who:   Adrian Bunk <[email protected]>
 
 ---------------------------
 
-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

Documentation/feature-removal-schedule.txt Who: Adrian Bunk <[email protected]> · line 347

Quoted 10 lines

Documentation/feature-removal-schedule.txt +5 -2

-      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 <[email protected]>
 
 ---------------------------

applied

you forgot diffstat

Miquel van Smoorenburg

2007-05-19 11:06 UTC #3 lore raw

In reply to Tejun Heo

Quoted 3 lines · Tejun Heo

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

It says:

Quoted 8 lines

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.

Quoted 9 lines

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.

Tejun Heo

author

2007-05-19 11:14 UTC #4 lore raw

Quoted 19 lines · Miquel van Smoorenburg

Quoted 9 lines
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.

Quoted 3 lines

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.

Signature · 1 line

tejun

Henrique de Moraes Holschuh

2007-05-19 13:48 UTC #5 lore raw

Quoted 7 lines · Tejun Heo

Quoted 3 lines
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.

Signature · 4 lines

  "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

Tejun Heo

author

2007-05-19 16:48 UTC #6 lore raw

Quoted 8 lines · Henrique de Moraes Holschuh

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.

Quoted 12 lines

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

Quoted 8 lines

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

Signature · 1 line

tejun