Re: PATCH: Shutdown IDE before powering off.

Re: PATCH: Shutdown IDE before powering off.

[Karol Kozimor]

Thus wrote Bartlomiej Zolnierkiewicz:
> 
> On Thursday 22 of January 2004 17:02, Jeff Garzik wrote:
> > I'm either shock or very very worried that the reboot notifier that
> > flushes IDE in 2.4.x, ide_notifier, is nowhere to be seen in 2.6.x :(
> > That seems like the real problem -- the code _used_ to be there.
> 
> Yep, it should be re-added.  I wonder when/why it was removed?

Hi,
What's the current status of this issue?
Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan@hell.org.pl

Broken PM semantics (WAS: PATCH: Shutdown IDE before powering off).

[Benjamin Herrenschmidt]

On Fri, 2004-03-12 at 08:46, Karol Kozimor wrote:
> Thus wrote Bartlomiej Zolnierkiewicz:
> > 
> > On Thursday 22 of January 2004 17:02, Jeff Garzik wrote:
> > > I'm either shock or very very worried that the reboot notifier that
> > > flushes IDE in 2.4.x, ide_notifier, is nowhere to be seen in 2.6.x :(
> > > That seems like the real problem -- the code _used_ to be there.
> > 
> > Yep, it should be re-added.  I wonder when/why it was removed?

Ideally, it should use the same mecanism as the PM requests...

In fact, the shutdown is just a special case of PM request. I think
ultimately, we should drop the various "shutdown()" functions in the
drivers in favor of a "state" selector for PM. That goes along with
the current problem of "state" in PM beeing completely bogus. The
constants defined by linux/pm.h are in no way related to what
the various drivers have come to expect.

enum {
        PM_SUSPEND_ON,
        PM_SUSPEND_STANDBY,
        PM_SUSPEND_MEM,
        PM_SUSPEND_DISK,
        PM_SUSPEND_MAX,
};
                                                                                                                   
Which basically gives is MEM=2 and DISK=3, while drivers usually
expect MEM=3 and DISK=4 while nobody really cares about 2 except
some specific stuffs in the arch code (or radeonfb on pmacs...)

We should get rid of this assumption that we are passing a D-type
anyway. I suggest we define once for all that what we are passing
down the driver is really the overall system state we are getting
to, that is MEM,DISK,KEXEC,SHUTDOWN, eventually STANDBY if we
ever do something like that (useful for handhelds that have a
special idle state and really don't care about scheduling whne
nothing happens for a while).

Ben.

Re: Broken PM semantics (WAS: PATCH: Shutdown IDE before powering off).

[Pavel Machek]

Hi!

> > Thus wrote Bartlomiej Zolnierkiewicz:
> > > 
> > > On Thursday 22 of January 2004 17:02, Jeff Garzik wrote:
> > > > I'm either shock or very very worried that the reboot notifier that
> > > > flushes IDE in 2.4.x, ide_notifier, is nowhere to be seen in 2.6.x :(
> > > > That seems like the real problem -- the code _used_ to be there.
> > > 
> > > Yep, it should be re-added.  I wonder when/why it was removed?
> 
> Ideally, it should use the same mecanism as the PM requests...
> 
> In fact, the shutdown is just a special case of PM request. I think
> ultimately, we should drop the various "shutdown()" functions in the
> drivers in favor of a "state" selector for PM. That goes along with
> the current problem of "state" in PM beeing completely bogus. The
> constants defined by linux/pm.h are in no way related to what
> the various drivers have come to expect.
> 
> enum {
>         PM_SUSPEND_ON,
>         PM_SUSPEND_STANDBY,
>         PM_SUSPEND_MEM,
>         PM_SUSPEND_DISK,
>         PM_SUSPEND_MAX,
> };
>                                                                                                                    
> Which basically gives is MEM=2 and DISK=3, while drivers usually
> expect MEM=3 and DISK=4 while nobody really cares about 2 except
> some specific stuffs in the arch code (or radeonfb on pmacs...)
> 
> We should get rid of this assumption that we are passing a D-type
> anyway. I suggest we define once for all that what we are passing
> down the driver is really the overall system state we are getting
> to, that is MEM,DISK,KEXEC,SHUTDOWN, eventually STANDBY if we
> ever do something like that (useful for handhelds that have a
> special idle state and really don't care about scheduling whne
> nothing happens for a while).

Agreed. Or at least document that it takes D states
and BUG-ON on invalid values...
				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

PATCH: Shutdown IDE before powering off.

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 301 bytes --]

Hi.

Here's a patch Bernard Blackham posted to the Software Suspend mailing
list, which has fixed data-not-being-properly flushed issues for some
people. (Forwarded with Bernard's permission).

Regards,

Nigel
-- 
My work on Software Suspend is graciously brought to you by
LinuxFund.org.

[-- Attachment #1.2: ide-shutdown.diff --]
[-- Type: text/x-patch, Size: 718 bytes --]

diff -ruN linux-2.6.0/drivers/ide/ide.c.orig linux-2.6.0/drivers/ide/ide.c
--- linux-2.6.0/drivers/ide/ide.c.orig	2003-12-18 10:58:38.000000000 +0800
+++ linux-2.6.0/drivers/ide/ide.c	2003-12-28 10:18:47.000000000 +0800
@@ -2493,6 +2493,11 @@
 	return 0;
 }
 
+static void ide_drive_shutdown (struct device * dev)
+{
+	generic_ide_suspend(dev, 5);
+}
+
 int ide_register_driver(ide_driver_t *driver)
 {
 	struct list_head list;
@@ -2519,6 +2524,7 @@
 	driver->gen_driver.name = (char *) driver->name;
 	driver->gen_driver.bus = &ide_bus_type;
 	driver->gen_driver.remove = ide_drive_remove;
+	driver->gen_driver.shutdown = ide_drive_shutdown;
 	return driver_register(&driver->gen_driver);
 }
 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH: Shutdown IDE before powering off.

[Andrew Morton]

Nigel Cunningham <ncunningham@users.sourceforge.net> wrote:
>
> +static void ide_drive_shutdown (struct device * dev)
>  +{
>  +	generic_ide_suspend(dev, 5);
>  +}
>  +
>   int ide_register_driver(ide_driver_t *driver)
>   {
>   	struct list_head list;
>  @@ -2519,6 +2524,7 @@
>   	driver->gen_driver.name = (char *) driver->name;
>   	driver->gen_driver.bus = &ide_bus_type;
>   	driver->gen_driver.remove = ide_drive_remove;
>  +	driver->gen_driver.shutdown = ide_drive_shutdown;

This spins down the disk(s) when you're just doing do a reboot.  That's
fairly irritating and could affect reboot times if one has many disks.

Re: PATCH: Shutdown IDE before powering off.

[John Bradford]

Quote from Andrew Morton <akpm@osdl.org>:
> Nigel Cunningham <ncunningham@users.sourceforge.net> wrote:
> >
> > +static void ide_drive_shutdown (struct device * dev)
> >  +{
> >  +	generic_ide_suspend(dev, 5);
> >  +}
> >  +
> >   int ide_register_driver(ide_driver_t *driver)
> >   {
> >   	struct list_head list;
> >  @@ -2519,6 +2524,7 @@
> >   	driver->gen_driver.name = (char *) driver->name;
> >   	driver->gen_driver.bus = &ide_bus_type;
> >   	driver->gen_driver.remove = ide_drive_remove;
> >  +	driver->gen_driver.shutdown = ide_drive_shutdown;
> 
> This spins down the disk(s) when you're just doing do a reboot.  That's
> fairly irritating and could affect reboot times if one has many disks.

I think it is an attempt to force some broken drives to flush their
cache, but I wonder whether it will simply move the problem from one
set of broken drives to another :-).

John.

Re: PATCH: Shutdown IDE before powering off.

[Nigel Cunningham]

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

Hi.

On Thu, 2004-01-22 at 21:13, John Bradford wrote:
> > This spins down the disk(s) when you're just doing do a reboot.  That's
> > fairly irritating and could affect reboot times if one has many disks.
> 
> I think it is an attempt to force some broken drives to flush their
> cache, but I wonder whether it will simply move the problem from one
> set of broken drives to another :-).

Yes, they were trying to get caches flushed. If this attempt is
misguided, that's fine. Is there a better way?

Regards,

Nigel
-- 
My work on Software Suspend is graciously brought to you by
LinuxFund.org.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH: Shutdown IDE before powering off.

[Andrew Morton]

Nigel Cunningham <ncunningham@users.sourceforge.net> wrote:
>
> Hi.
> 
> On Thu, 2004-01-22 at 21:13, John Bradford wrote:
> > > This spins down the disk(s) when you're just doing do a reboot.  That's
> > > fairly irritating and could affect reboot times if one has many disks.
> > 
> > I think it is an attempt to force some broken drives to flush their
> > cache, but I wonder whether it will simply move the problem from one
> > set of broken drives to another :-).
> 
> Yes, they were trying to get caches flushed. If this attempt is
> misguided, that's fine. Is there a better way?

A couple of thoughts come to mind:

a) Don't do it if the user typed reboot - only do it if we're powering down.

b) Try to do a cache flush instead.  If that fails (do we know?) then
   power down the disk instead.

Re: PATCH: Shutdown IDE before powering off.

[Nigel Cunningham]

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

Hi.

On Thu, 2004-01-22 at 21:45, Andrew Morton wrote:
> A couple of thoughts come to mind:
> 
> a) Don't do it if the user typed reboot - only do it if we're powering down.

You'd think a parameter called shutdown would do that :>

> b) Try to do a cache flush instead.  If that fails (do we know?) then
>    power down the disk instead.

We're calling drivers_suspend and then sys_reboot for powering down, and
only calling sys_reboot when rebooting. Shouldn't cache flushes already
be happening?

Regards,

Nigel
-- 
My work on Software Suspend is graciously brought to you by
LinuxFund.org.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH: Shutdown IDE before powering off.

[Jeff Garzik]

Andrew Morton wrote:
> Nigel Cunningham <ncunningham@users.sourceforge.net> wrote:
> 
>>Hi.
>>
>>On Thu, 2004-01-22 at 21:13, John Bradford wrote:
>>
>>>>This spins down the disk(s) when you're just doing do a reboot.  That's
>>>>fairly irritating and could affect reboot times if one has many disks.
>>>
>>>I think it is an attempt to force some broken drives to flush their
>>>cache, but I wonder whether it will simply move the problem from one
>>>set of broken drives to another :-).
>>
>>Yes, they were trying to get caches flushed. If this attempt is
>>misguided, that's fine. Is there a better way?
> 
> 
> A couple of thoughts come to mind:
> 
> a) Don't do it if the user typed reboot - only do it if we're powering down.
> 
> b) Try to do a cache flush instead.  If that fails (do we know?) then
>    power down the disk instead.


I'm either shock or very very worried that the reboot notifier that 
flushes IDE in 2.4.x, ide_notifier, is nowhere to be seen in 2.6.x :( 
That seems like the real problem -- the code _used_ to be there.

	Jeff

Re: PATCH: Shutdown IDE before powering off.

[Bartlomiej Zolnierkiewicz]

On Thursday 22 of January 2004 17:02, Jeff Garzik wrote:
> I'm either shock or very very worried that the reboot notifier that
> flushes IDE in 2.4.x, ide_notifier, is nowhere to be seen in 2.6.x :(
> That seems like the real problem -- the code _used_ to be there.

Yep, it should be re-added.  I wonder when/why it was removed?

--bart

Re: PATCH: Shutdown IDE before powering off.

[John Bradford]

Quote from Nigel Cunningham <ncunningham@users.sourceforge.net>:
> 
> --=-1L1FlHM683Yn00jU9UMu
> Content-Type: text/plain
> Content-Transfer-Encoding: quoted-printable
> 
> Hi.
> 
> On Thu, 2004-01-22 at 21:13, John Bradford wrote:
> > > This spins down the disk(s) when you're just doing do a reboot.  That's
> > > fairly irritating and could affect reboot times if one has many disks.
> >=20
> > I think it is an attempt to force some broken drives to flush their
> > cache, but I wonder whether it will simply move the problem from one
> > set of broken drives to another :-).
> 
> Yes, they were trying to get caches flushed. If this attempt is
> misguided, that's fine. Is there a better way?

It was discussed at length around the 2.4.20 timeframe, when the
power-off cache-flush and spin down behavior was changed, but I don't
remember any real conclusion being reached.

John.

Re: PATCH: Shutdown IDE before powering off.

[James H. Cloos Jr.]

>>>>> "John" == John Bradford <john@grabjohn.com> writes:

>> This spins down the disk(s) when you're just doing do a reboot.
>> That's fairly irritating and could affect reboot times if one has
>> many disks.

John> I think it is an attempt to force some broken drives to flush
John> their cache, but I wonder whether it will simply move the
John> problem from one set of broken drives to another :-).

It will.  I've had to work with a few drives or drive combos over
the years that would not spin up reliably.  It was vital to keep
them spinning once they were (all) up.  Adding this would make
reboot unnecessarily unuseable in such cases.  Perhaps just
flush, pause, flush would work as well?

Or even the logical equivilent to sync;sync;sync;reboot?

-JimC

Re: PATCH: Shutdown IDE before powering off.

[Nigel Cunningham]

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

Well, as far as Suspend can see, all the data has been written. By the
time we reach this point, the end_buffer_io_async routine has been
called for every write submitted, and the last write submitted by
anything else was at least a few seconds ago (all other processes are
frozen and drivers suspended), so all data _should_ be on disk already
and sync should do nothing. Actually, we wouldn't want to call sync
anyway for reasons I won't go into here (unnecessary complication). I'm
sure you'd agree that we would want to delay for an arbitrary length of
time either (no guarantees that that would cut the mustard). We need to
flush caches properly.

Regards,

Nigel

On Fri, 2004-01-23 at 08:19, James H. Cloos Jr. wrote:
> John> I think it is an attempt to force some broken drives to flush
> John> their cache, but I wonder whether it will simply move the
> John> problem from one set of broken drives to another :-).
> 
> It will.  I've had to work with a few drives or drive combos over
> the years that would not spin up reliably.  It was vital to keep
> them spinning once they were (all) up.  Adding this would make
> reboot unnecessarily unuseable in such cases.  Perhaps just
> flush, pause, flush would work as well?
> 
> Or even the logical equivilent to sync;sync;sync;reboot?
> 
> -JimC
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-- 
My work on Software Suspend is graciously brought to you by
LinuxFund.org.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH: Shutdown IDE before powering off.

[James H. Cloos Jr.]

>>>>> "Nigel" == Nigel Cunningham <ncunningham@users.sourceforge.net> writes:

Nigel> Actually, we wouldn't want to call sync
Nigel> anyway for reasons I won't go into here

Sorry for the confusion; I didn't mean call sync so much as flush
synchronously (ie wait for the drive to ack) thrice before the reboot.

-JimC

Re: PATCH: Shutdown IDE before powering off.

[Jeff Garzik]

James H. Cloos Jr. wrote:
>>>>>>"John" == John Bradford <john@grabjohn.com> writes:
> 
> 
>>>This spins down the disk(s) when you're just doing do a reboot.
>>>That's fairly irritating and could affect reboot times if one has
>>>many disks.
> 
> 
> John> I think it is an attempt to force some broken drives to flush
> John> their cache, but I wonder whether it will simply move the
> John> problem from one set of broken drives to another :-).
> 
> It will.  I've had to work with a few drives or drive combos over
> the years that would not spin up reliably.  It was vital to keep
> them spinning once they were (all) up.  Adding this would make
> reboot unnecessarily unuseable in such cases.  Perhaps just
> flush, pause, flush would work as well?


Flush is what is needed, flush is what it does in 2.4, and flush is what 
it should do in 2.6 :)

Rebooting does not shut down nor unload the IDE driver, so it is 
-critical- that a flush occurs before reboot, otherwise it is entirely 
possible that writes the drive has ack'd back to the OS will not 
actually get written to the media.

	Jeff