[PATCH] libata: track spindown status and skip spindown_compat if possible

[PATCH] libata: track spindown status and skip spindown_compat if possible

[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 <htejun@gmail.com>
---

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.

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 07b5a3d..b6a1de8 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -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);
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 27d9362..a52734b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -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),

Re: [PATCH] libata: track spindown status and skip spindown_compat if possible

[Francesco Pretto]

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.

Re: [PATCH] libata: track spindown status and skip spindown_compat if possible

[Tejun Heo]

Francesco Pretto wrote:
> 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.

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?

-- 
tejun

Re: [PATCH] libata: track spindown status and skip spindown_compat if possible

[Stephen Clark]

Tejun Heo wrote:

>Francesco Pretto wrote:
>  
>
>>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.
>>    
>>
>
>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?
>
>  
>
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

-- 

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

Re: [PATCH] libata: track spindown status and skip spindown_compat if possible

[Tejun Heo]

Stephen Clark wrote:
> 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.  :-)

-- 
tejun

Re: [PATCH] libata: track spindown status and skip spindown_compat if possible

[Francesco Pretto]

2007/5/15, Tejun Heo <htejun@gmail.com>:
> 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:
> We have an inherent preference for the cleanest and simplest
> implementation, obviously :-)  If that means no userspace code, and
> just call reboot(), WIN!

Re: [PATCH] libata: track spindown status and skip spindown_compat if possible

[Jeff Garzik]

Tejun Heo wrote:
> 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 <htejun@gmail.com>
> ---
> 
> 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.

applied to #upstream-fixes