libata: track spindown status and skip spindown_compat if possible

PATCH Applied Tejun Heo 4 participants 7 messages May 15, 2007 – May 16, 2007 libata

Share & tools

Tejun Heo

author

2007-05-15 10:29 UTC #0 lore raw

PATCH 1/1 libata: track spindown status and skip spindown_compat if possible

Our assumption that most distros issue STANDBYNOW seems wrong. The upstream sysvinit and thus many distros including gentoo and opensuse don't take any action for libata disks on spindown. We can skip compat handling for these distros so that they don't need to update anything to take advantage of kernel-side shutdown.

Signed-off-by Tejun Heo <[email protected]>

---

This patch is generated on top of fix-shutdown-warning-message-printing.

http://article.gmane.org/gmane.linux.ide/18823

Jeff, I think we can avoid a lot of problems with this. Those distros which didn't do anything on shutdown would still cause emergency unload on older kernels but at least it isn't a regression and things should just work on newer kernels.

If you ack this, I'll update the shutdown.html page accordingly. Thanks.

drivers/ata/libata-scsi.c +9 -0

index 07b5a3d..b6a1de8 100644
@@ -967,6 +967,7 @@ static unsigned int ata_scsi_start_stop_
 		 * for more info.
 		 */
 		if (ata_spindown_compat &&
+		    (qc->dev->flags & ATA_DFLAG_SPUNDOWN) &&
 		    (system_state == SYSTEM_HALT ||
 		     system_state == SYSTEM_POWER_OFF)) {
 			static unsigned long warned = 0;

@@ -1394,6 +1395,14 @@ static void ata_scsi_qc_complete(struct
 		}
 	}
 
+	/* XXX: track spindown state for spindown_compat */
+	if (unlikely(qc->tf.command == ATA_CMD_STANDBY ||
+		     qc->tf.command == ATA_CMD_STANDBYNOW1))
+		qc->dev->flags |= ATA_DFLAG_SPUNDOWN;
+	else if (likely(system_state != SYSTEM_HALT &&
+			system_state != SYSTEM_POWER_OFF))
+		qc->dev->flags &= ~ATA_DFLAG_SPUNDOWN;
+
 	if (need_sense && !ap->ops->error_handler)
 		ata_dump_status(ap->print_id, &qc->result_tf);

include/linux/libata.h +1 -0

index 27d9362..a52734b 100644
@@ -140,6 +140,7 @@ enum {
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
 	ATA_DFLAG_NCQ_OFF	= (1 << 9), /* device limited to non-NCQ mode */
+	ATA_DFLAG_SPUNDOWN	= (1 << 10), /* XXX: for spindown_compat */
 	ATA_DFLAG_INIT_MASK	= (1 << 16) - 1,
 
 	ATA_DFLAG_DETACH	= (1 << 16),

Francesco Pretto

2007-05-15 11:53 UTC #1 lore raw

Quoted 7 lines · Tejun Heo <htejun <at> gmail.com> writes:

Our assumption that most distros issue STANDBYNOW seems wrong. The upstream sysvinit and thus many distros including gentoo and opensuse don't take any action for libata disks on spindown. We can skip compat handling for these distros so that they don't need to update anything to take advantage of kernel-side shutdown.

Working around the workaround won't make confusion? Won't this break again implementations that was actually issuing STANDBYNOW, assuming that they actually exist? Only wondering, can't say for any init implementation in particular.

Tejun Heo

author

2007-05-15 12:03 UTC #2 lore raw

Quoted 8 lines · Francesco Pretto

Quoted 6 lines · Tejun Heo <htejun <at> gmail.com> writes:
Our assumption that most distros issue STANDBYNOW seems wrong. The upstream sysvinit and thus many distros including gentoo and opensuse don't take any action for libata disks on spindown. We can skip compat handling for these distros so that they don't need to update anything to take advantage of kernel-side shutdown.

Quoted 4 lines

Working around the workaround won't make confusion? Won't this break again implementations that was actually issuing STANDBYNOW, assuming that they actually exist? Only wondering, can't say for any init implementation in particular.

Yeah, it's a big mess. With this patch applied, what happens is...

* If your shutdown(8) does issue STANDBYNOW : you get the big fat warning and kernel won't issue STANDBYNOW.

* If your shutdown(8) doesn't issue STANDBYNOW : kernel issues FLUSH CACHE followed by STANDBYNOW and all is well and dandy without any userland modification.

I think it isn't too bad. Any better ideas?

Signature · 1 line

tejun

Stephen Clark

2007-05-15 12:12 UTC #3 lore raw

Quoted 23 lines · Tejun Heo

Quoted 11 lines · Francesco Pretto
Quoted 8 lines · Tejun Heo <htejun <at> gmail.com> writes:
Our assumption that most distros issue STANDBYNOW seems wrong. The upstream sysvinit and thus many distros including gentoo and opensuse don't take any action for libata disks on spindown. We can skip compat handling for these distros so that they don't need to update anything to take advantage of kernel-side shutdown.
Quoted 6 lines
Working around the workaround won't make confusion? Won't this break again implementations that was actually issuing STANDBYNOW, assuming that they actually exist? Only wondering, can't say for any init implementation in particular.
Yeah, it's a big mess. With this patch applied, what happens is...

Quoted 11 lines

* If your shutdown(8) does issue STANDBYNOW : you get the big fat warning and kernel won't issue STANDBYNOW.
* If your shutdown(8) doesn't issue STANDBYNOW : kernel issues FLUSH CACHE followed by STANDBYNOW and all is well and dandy without any userland modification.
I think it isn't too bad. Any better ideas?

Ok, I am running fc6 with kernel.org 2.6.21 what do I need to do to keep my laptop from popping, and having my retract_count increment, every time I shutdown?

Thanks,
Steve

Signature · 3 lines

"They that give up essential liberty to obtain temporary safety, deserve neither liberty nor safety."  (Ben Franklin)

"The course of history shows that as a government grows, liberty decreases."  (Thomas Jefferson)

Tejun Heo

author

2007-05-15 12:18 UTC #4 lore raw

Quoted 3 lines · Stephen Clark

Ok, I am running fc6 with kernel.org 2.6.21 what do I need to do to keep my laptop from
popping, and having my retract_count increment, every time I shutdown?

Boot with the updated kernel and shutdown. If kernel doesn't complain, you're all set. If kernel complains, you need to update shutdown(8). See, easy. :-)

Signature · 1 line

tejun

Francesco Pretto

2007-05-15 16:44 UTC #5 lore raw

In reply to Tejun Heo

2007/5/15, Tejun Heo <[email protected]>:

Quoted 11 lines

Yeah, it's a big mess. With this patch applied, what happens is...
* If your shutdown(8) does issue STANDBYNOW : you get the big fat warning and kernel won't issue STANDBYNOW.
* If your shutdown(8) doesn't issue STANDBYNOW : kernel issues FLUSH CACHE followed by STANDBYNOW and all is well and dandy without any userland modification.
I think it isn't too bad. Any better ideas?

Seems good for a transitional period. However, as it appeared evident (and with a bit of luck), probably no distributions are trying to issuing STANDBYNOW after all.
Eventually, IMO, even this tracking for spindown status could be removed as only one shutdown(8) behavior should be supported: that of doing nothing and leaving all responsibility of spindown only to kernel.

As upstart author pointed in the concerning bug report:

Quoted 3 lines

We have an inherent preference for the cleanest and simplest implementation, obviously :-) If that means no userspace code, and just call reboot(), WIN!

Jeff Garzik

Applied

2007-05-16 05:22 UTC #6 lore raw

In reply to Tejun Heo

Quoted 13 lines · Tejun Heo

Our assumption that most distros issue STANDBYNOW seems wrong. The upstream sysvinit and thus many distros including gentoo and opensuse don't take any action for libata disks on spindown. We can skip compat handling for these distros so that they don't need to update anything to take advantage of kernel-side shutdown.
Signed-off-by Tejun Heo <[email protected]>
---
This patch is generated on top of fix-shutdown-warning-message-printing.
http://article.gmane.org/gmane.linux.ide/18823

Quoted 7 lines

Jeff, I think we can avoid a lot of problems with this. Those distros which didn't do anything on shutdown would still cause emergency unload on older kernels but at least it isn't a regression and things should just work on newer kernels.
If you ack this, I'll update the shutdown.html page accordingly. Thanks.

applied to #upstream-fixes