suspend.c: This is broken, fixme

suspend.c: This is broken, fixme

[Pavel Machek]

Hi!

I found this in 2.5.20...

--- a/kernel/suspend.c  Sun Jun  2 18:44:56 2002
+++ b/kernel/suspend.c  Sun Jun  2 18:44:56 2002
@@ -64,6 +64,7 @@
 #include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 #include <asm/io.h>
+#include <linux/swapops.h>

 unsigned char software_suspend_enabled = 0;

@@ -300,7 +301,8 @@
 static void do_suspend_sync(void)
 {
        while (1) {
-               run_task_queue(&tq_disk);
+               blk_run_queues();
+#error this is broken, FIXME
                if (!TQ_ACTIVE(tq_disk))
                        break;

. Why is it broken?
									Pavel
-- 
(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Re: suspend.c: This is broken, fixme

[Jens Axboe]

On Mon, Jun 03 2002, Pavel Machek wrote:
> Hi!
> 
> I found this in 2.5.20...
> 
> --- a/kernel/suspend.c  Sun Jun  2 18:44:56 2002
> +++ b/kernel/suspend.c  Sun Jun  2 18:44:56 2002
> @@ -64,6 +64,7 @@
>  #include <asm/mmu_context.h>
>  #include <asm/pgtable.h>
>  #include <asm/io.h>
> +#include <linux/swapops.h>
> 
>  unsigned char software_suspend_enabled = 0;
> 
> @@ -300,7 +301,8 @@
>  static void do_suspend_sync(void)
>  {
>         while (1) {
> -               run_task_queue(&tq_disk);
> +               blk_run_queues();
> +#error this is broken, FIXME
>                 if (!TQ_ACTIVE(tq_disk))
>                         break;
> 
> . Why is it broken?

Hey, I even cc'ed you on the patch when it went to Linus... Lets look at
what happened before: run tq_disk, then check if it is active. What
prevents tq_disk from being active right after you issue the TQ_ACTIVE
check? Nothing. And I'm not sure exactly what semantics you think
running tq_disk has. I suspect you are looking for a 'start any pending
i/o and return when it has completed', which is far from what happens.
Running tq_disk will _try_ to start _some_ I/O, and eventually, in time,
the currently pending requests will have completed. In the mean time,
more I/O could have been added though.

-- 
Jens Axboe

Re: suspend.c: This is broken, fixme

[Pavel Machek]

Hi!

> > @@ -300,7 +301,8 @@
> >  static void do_suspend_sync(void)
> >  {
> >         while (1) {
> > -               run_task_queue(&tq_disk);
> > +               blk_run_queues();
> > +#error this is broken, FIXME
> >                 if (!TQ_ACTIVE(tq_disk))
> >                         break;
> > 
> > . Why is it broken?
> 
> Hey, I even cc'ed you on the patch when it went to Linus... Lets
> look at

Okay; I thought I corrected it in the meantime, that's why I got confused.

> what happened before: run tq_disk, then check if it is active. What
> prevents tq_disk from being active right after you issue the TQ_ACTIVE
> check? Nothing. And I'm not sure exactly what semantics you think
> running tq_disk has. I suspect you are looking for a 'start any pending
> i/o and return when it has completed', which is far from what happens.
> Running tq_disk will _try_ to start _some_ I/O, and eventually, in time,
> the currently pending requests will have completed. In the mean time,
> more I/O could have been added though.

I'm alone at the system at that point. All user tasks are stopped and
I'm only thread running. There's noone that could submit requests at
that point.

In such case, killing #error is right solution, right?
								Pavel



-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: suspend.c: This is broken, fixme

[Jens Axboe]

On Mon, Jun 03 2002, Pavel Machek wrote:
> Hi!
> 
> > > @@ -300,7 +301,8 @@
> > >  static void do_suspend_sync(void)
> > >  {
> > >         while (1) {
> > > -               run_task_queue(&tq_disk);
> > > +               blk_run_queues();
> > > +#error this is broken, FIXME
> > >                 if (!TQ_ACTIVE(tq_disk))
> > >                         break;
> > > 
> > > . Why is it broken?
> > 
> > Hey, I even cc'ed you on the patch when it went to Linus... Lets
> > look at
> 
> Okay; I thought I corrected it in the meantime, that's why I got confused.
> 
> > what happened before: run tq_disk, then check if it is active. What
> > prevents tq_disk from being active right after you issue the TQ_ACTIVE
> > check? Nothing. And I'm not sure exactly what semantics you think
> > running tq_disk has. I suspect you are looking for a 'start any pending
> > i/o and return when it has completed', which is far from what happens.
> > Running tq_disk will _try_ to start _some_ I/O, and eventually, in time,
> > the currently pending requests will have completed. In the mean time,
> > more I/O could have been added though.
> 
> I'm alone at the system at that point. All user tasks are stopped and
> I'm only thread running. There's noone that could submit requests at
> that point.

Ok, then at least the very last point I made can be disregarded.
However... ->

> In such case, killing #error is right solution, right?

Not at all. The tq_disk/blk_run_queues() semantics are the same, they
will only start i/o (which may not even be right when you run it) and
that is it. When all i/o is completed is not known.

-- 
Jens Axboe

Re: suspend.c: This is broken, fixme

[Pavel Machek]

Hi!

> > I'm alone at the system at that point. All user tasks are stopped and
> > I'm only thread running. There's noone that could submit requests at
> > that point.
> 
> Ok, then at least the very last point I made can be disregarded.
> However... ->
> 
> > In such case, killing #error is right solution, right?
> 
> Not at all. The tq_disk/blk_run_queues() semantics are the same, they
> will only start i/o (which may not even be right when you run it) and
> that is it. When all i/o is completed is not known.

Is there some way to wait for all I/O to compelte? How do we do that
on system shutdown?
									Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.