suspend.c: This is broken, fixme

PATCH Pavel Machek 3 participants 5 messages Jun 3, 2002

lore

Share & tools

lore

Pavel Machek

author

2002-06-03 10:01 UTC #0 lore raw

Hi!

I found this in 2.5.20...

kernel/suspend.c Sun Jun 2 18:44:56 2002 +3 -1

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

Signature · 2 lines

(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

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.

Signature · 1 line

Jens Axboe

Pavel Machek

2002-06-03 11:32 UTC #2 lore raw

In reply to Pavel Machek

Hi!

Quoted 10 lines

kernel/suspend.c Sun Jun 2 18:44:56 2002 +2 -1

@@ -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;

Quoted 4 lines

Quoted 1 line
. 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.

Quoted 8 lines

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

Signature · 2 lines

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

Jens Axboe

2002-06-03 11:36 UTC #3 lore raw

In reply to Pavel Machek

Quoted 28 lines · Pavel Machek

Hi!
Quoted 10 lines
Quoted 10 lines
kernel/suspend.c Sun Jun 2 18:44:56 2002 +2 -1
@@ -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;
 
Quoted 4 lines
Quoted 1 line
. 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.
Quoted 8 lines
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.

Quoted 3 lines

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

Quoted 1 line

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.

Signature · 1 line

Jens Axboe

Pavel Machek

2002-06-03 12:13 UTC #4 lore raw

In reply to Pavel Machek

Hi!

Quoted 4 lines

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

Quoted 8 lines

Ok, then at least the very last point I made can be disregarded. However... ->
Quoted 1 line
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

Signature · 2 lines

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