Fix qemu-iotests failures

This reverts commit 6133b39f3c36623425a6ede9e89d93175fde15cd.

The commit checked conditions that would expose a bug, but there is no real reason to forbid them apart from the bug, which we'll fix in a minute.

In particular, reentering a coroutine during co_aio_sleep_ns() is fine; the function is explicitly written to allow this.

aio_co_schedule() can indeed conflict with direct coroutine invocations, but this is exactky what we want to fix, so remove that check again,

too.

Signed-off-by Kevin Wolf <[email protected]>

---
 include/qemu/coroutine_int.h | 13 +++----------
 util/async.c                 | 13 -------------
 util/qemu-coroutine-sleep.c  | 12 ------------
 util/qemu-coroutine.c        | 14 --------------
 4 files changed, 3 insertions(+), 49 deletions(-)

include/qemu/coroutine_int.h +3 -10

index 59e8406398..cb98892bba 100644
@@ -46,21 +46,14 @@ struct Coroutine {
 
     size_t locks_held;
 
-    /* Only used when the coroutine has yielded.  */
-    AioContext *ctx;
-
-    /* Used to catch and abort on illegal co-routine entry.
-     * Will contain the name of the function that had first
-     * scheduled the coroutine. */
-    const char *scheduled;
-
-    QSIMPLEQ_ENTRY(Coroutine) co_queue_next;
-
     /* Coroutines that should be woken up when we yield or terminate.
      * Only used when the coroutine is running.
      */
     QSIMPLEQ_HEAD(, Coroutine) co_queue_wakeup;
 
+    /* Only used when the coroutine has yielded.  */
+    AioContext *ctx;
+    QSIMPLEQ_ENTRY(Coroutine) co_queue_next;
     QSLIST_ENTRY(Coroutine) co_scheduled_next;
 };
 

util/async.c +0 -13

index 4dd9d95a9e..0e1bd8780a 100644
@@ -388,9 +388,6 @@ static void co_schedule_bh_cb(void *opaque)
         QSLIST_REMOVE_HEAD(&straight, co_scheduled_next);
         trace_aio_co_schedule_bh_cb(ctx, co);
         aio_context_acquire(ctx);
-
-        /* Protected by write barrier in qemu_aio_coroutine_enter */
-        atomic_set(&co->scheduled, NULL);
         qemu_coroutine_enter(co);
         aio_context_release(ctx);
     }

@@ -441,16 +438,6 @@ fail:
 void aio_co_schedule(AioContext *ctx, Coroutine *co)
 {
     trace_aio_co_schedule(ctx, co);
-    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL,
-                                           __func__);
-
-    if (scheduled) {
-        fprintf(stderr,
-                "%s: Co-routine was already scheduled in '%s'\n",
-                __func__, scheduled);
-        abort();
-    }
-
     QSLIST_INSERT_HEAD_ATOMIC(&ctx->scheduled_coroutines,
                               co, co_scheduled_next);
     qemu_bh_schedule(ctx->co_schedule_bh);

util/qemu-coroutine-sleep.c +0 -12

index 254349cdbb..9c5655041b 100644
@@ -13,7 +13,6 @@
 
 #include "qemu/osdep.h"
 #include "qemu/coroutine.h"
-#include "qemu/coroutine_int.h"
 #include "qemu/timer.h"
 #include "block/aio.h"
 

@@ -26,8 +25,6 @@ static void co_sleep_cb(void *opaque)
 {
     CoSleepCB *sleep_cb = opaque;
 
-    /* Write of schedule protected by barrier write in aio_co_schedule */
-    atomic_set(&sleep_cb->co->scheduled, NULL);
     aio_co_wake(sleep_cb->co);
 }
 

@@ -37,15 +34,6 @@ void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type,
     CoSleepCB sleep_cb = {
         .co = qemu_coroutine_self(),
     };
-
-    const char *scheduled = atomic_cmpxchg(&sleep_cb.co->scheduled, NULL,
-                                           __func__);
-    if (scheduled) {
-        fprintf(stderr,
-                "%s: Co-routine was already scheduled in '%s'\n",
-                __func__, scheduled);
-        abort();
-    }
     sleep_cb.ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &sleep_cb);
     timer_mod(sleep_cb.ts, qemu_clock_get_ns(type) + ns);
     qemu_coroutine_yield();

util/qemu-coroutine.c +0 -14

index 9eff7fd450..d6095c1d5a 100644
@@ -107,22 +107,8 @@ void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)
     Coroutine *self = qemu_coroutine_self();
     CoroutineAction ret;
 
-    /* Cannot rely on the read barrier for co in aio_co_wake(), as there are
-     * callers outside of aio_co_wake() */
-    const char *scheduled = atomic_mb_read(&co->scheduled);
-
     trace_qemu_aio_coroutine_enter(ctx, self, co, co->entry_arg);
 
-    /* if the Coroutine has already been scheduled, entering it again will
-     * cause us to enter it twice, potentially even after the coroutine has
-     * been deleted */
-    if (scheduled) {
-        fprintf(stderr,
-                "%s: Co-routine was already scheduled in '%s'\n",
-                __func__, scheduled);
-        abort();
-    }
-
     if (co->caller) {
         fprintf(stderr, "Co-routine re-entered recursively\n");
         abort();

Signature · 1 line

2.13.6

This reverts commit 6133b39f3c36623425a6ede9e89d93175fde15cd.

The commit checked conditions that would expose a bug, but there is no real reason to forbid them apart from the bug, which we'll fix in a minute.

In particular, reentering a coroutine during co_aio_sleep_ns() is fine; the function is explicitly written to allow this.

aio_co_schedule() can indeed conflict with direct coroutine invocations, but this is exactky what we want to fix, so remove that check again, too.

Quoted 8 lines

This reverts commit 6133b39f3c36623425a6ede9e89d93175fde15cd.

The commit checked conditions that would expose a bug, but there is no real reason to forbid them apart from the bug, which we'll fix in a minute.

In particular, reentering a coroutine during co_aio_sleep_ns() is fine; the function is explicitly written to allow this.

This is true.

Quoted 3 lines

aio_co_schedule() can indeed conflict with direct coroutine invocations, but this is exactky what we want to fix, so remove that check again, too.

I'm not sure this is a good idea, as I answered in patch 3.

It can also conflict badly with another aio_co_schedule(). Your patch here removes the assertion in this case, and patch 3 makes it easier to get into the situation where two aio_co_schedule()s conflict with each other.

For example, say you have a coroutine that calls aio_co_schedule on itself, like

	while (true) {
		aio_co_schedule(qemu_get_current_aio_context(),
				qemu_coroutine_self());
	}

If somebody else calls qemu_coroutine_enter on this coroutine, *that* is the bug. These patches would just cause some random corruption or (perhaps worse) hang.

Quoted 5 lines

It can also conflict badly with another aio_co_schedule(). Your patch here removes the assertion in this case, and patch 3 makes it easier to get into the situation where two aio_co_schedule()s conflict with each other.

I don't see how they conflict. If the second aio_co_schedule() comes before the coroutine is actually entered, they are effectively simply merged into a single one. Which is exactly what was intended.

Quoted 8 lines

Quoted 5 lines

It can also conflict badly with another aio_co_schedule(). Your patch here removes the assertion in this case, and patch 3 makes it easier to get into the situation where two aio_co_schedule()s conflict with each other.

I don't see how they conflict. If the second aio_co_schedule() comes before the coroutine is actually entered, they are effectively simply merged into a single one. Which is exactly what was intended.

Suppose you have

	ctx->scheduled_coroutines
	  |
	  '---> co
                |
                '---> NULL

Then aio_co_schedule(ctx, co) does QSLIST_INSERT_HEAD_ATOMIC, which is

	/* On entry, ctx->scheduled_coroutines == co.  */
	co->next = ctx->scheduled_coroutines
	change ctx->scheduled_coroutines from co->next to co

This results in a loop:

	ctx->scheduled_coroutines
	  |
	  '---> co <-.
                |    |
                '----'

This is an obvious hang due to list corruption. In other more complicated cases it can skip a wakeup, which is a more subtle kind of hang. You can also get memory corruption if the coroutine terminates (and is freed) before aio_co_schedule executes.

Basically, once you do aio_co_schedule or aio_co_wake the coroutine is not any more yours. It's owned by the context that will run it and you should not do anything with it.

The same is true for co_aio_sleep_ns. Just because in practice it works (and it seemed like a clever idea at the time) it doesn't mean it *is* a good idea...

Am 28.11.2017 um 18:01 hat Paolo Bonzini geschrieben:

Quoted 32 lines · Kevin Wolf

Quoted 8 lines

Quoted 5 lines

It can also conflict badly with another aio_co_schedule(). Your patch here removes the assertion in this case, and patch 3 makes it easier to get into the situation where two aio_co_schedule()s conflict with each other.

I don't see how they conflict. If the second aio_co_schedule() comes before the coroutine is actually entered, they are effectively simply merged into a single one. Which is exactly what was intended.

Suppose you have

	ctx->scheduled_coroutines
	  |
	  '---> co
                |
                '---> NULL

Then aio_co_schedule(ctx, co) does QSLIST_INSERT_HEAD_ATOMIC, which is

	/* On entry, ctx->scheduled_coroutines == co.  */
	co->next = ctx->scheduled_coroutines
	change ctx->scheduled_coroutines from co->next to co

This results in a loop:

	ctx->scheduled_coroutines
	  |
	  '---> co <-.
                |    |
                '----'

Quoted 4 lines

This is an obvious hang due to list corruption. In other more complicated cases it can skip a wakeup, which is a more subtle kind of hang. You can also get memory corruption if the coroutine terminates (and is freed) before aio_co_schedule executes.

Okay, I can see that. I thought you were talking about the logic introduced by this series, but you're actually talking about preexisting behaviour which limits the usefulness of the patches.

Quoted 7 lines

Basically, once you do aio_co_schedule or aio_co_wake the coroutine is not any more yours. It's owned by the context that will run it and you should not do anything with it.

The same is true for co_aio_sleep_ns. Just because in practice it works (and it seemed like a clever idea at the time) it doesn't mean it *is* a good idea...

Well, but that poses the question how you can implement any coroutine that can be reentered from more than one place. Not being able to do that would be a severe restriction.

For example, take quorum, which handles requests by spawning a coroutine for every child and then yielding until acb->count == s->num_children. This means that the main request coroutine can be reentered from the child request coroutines in any order and timing.

What saves us currently is that they are all in the same AioContext, so we won't actually end up in aio_co_schedule(), but I'm sure that sooner or later we'll find cases where we're waiting for several workers that are spread across different I/O threads.

I don't think "don't do that" is a good answer to this.

And yes, reentering co_aio_sleep_ns() early is a real requirement, too. The test case that used speed=1 and would have waited a few hours before actually cancelling the job is an extreme example, but even just delaying cancellation for a second is bad if this is a second of migration downtime.

Am 28.11.2017 um 18:01 hat Paolo Bonzini geschrieben:

Quoted 3 lines

Basically, once you do aio_co_schedule or aio_co_wake the coroutine is not any more yours. It's owned by the context that will run it and you should not do anything with it.

Well, but that poses the question how you can implement any coroutine that can be reentered from more than one place. Not being able to do that would be a severe restriction.

For example, take quorum, which handles requests by spawning a coroutine for every child and then yielding until acb->count == s->num_children. This means that the main request coroutine can be reentered from the child request coroutines in any order and timing.

What saves us currently is that they are all in the same AioContext, so we won't actually end up in aio_co_schedule(), but I'm sure that sooner or later we'll find cases where we're waiting for several workers that are spread across different I/O threads.

I don't think "don't do that" is a good answer to this.

And yes, reentering co_aio_sleep_ns() early is a real requirement, too. The test case that used speed=1 and would have waited a few hours before actually cancelling the job is an extreme example, but even just delaying cancellation for a second is bad if this is a second of migration downtime.

This reverts commit 4afeffc8572f40d8844b946a30c00b10da4442b1.

This fixed the symptom of a bug rather than the root cause. Waking up a sleeping coroutine is generally fine, we just need to make it work correctly across AioContexts, which we'll do in a minute.

Signed-off-by Kevin Wolf <[email protected]>

---
 include/block/blockjob_int.h | 3 +--
 blockjob.c                   | 7 ++-----
 2 files changed, 3 insertions(+), 7 deletions(-)

include/block/blockjob_int.h +1 -2

index 43f3be2965..f13ad05c0d 100644
@@ -143,8 +143,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
  * @ns: How many nanoseconds to stop for.
  *
  * Put the job to sleep (assuming that it wasn't canceled) for @ns
- * nanoseconds.  Canceling the job will not interrupt the wait, so the
- * cancel will not process until the coroutine wakes up.
+ * nanoseconds.  Canceling the job will interrupt the wait immediately.
  */
 void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
 

blockjob.c +2 -5

index ff9a614531..3a0c49137e 100644
@@ -797,14 +797,11 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
         return;
     }
 
-    /* We need to leave job->busy set here, because when we have
-     * put a coroutine to 'sleep', we have scheduled it to run in
-     * the future.  We cannot enter that same coroutine again before
-     * it wakes and runs, otherwise we risk double-entry or entry after
-     * completion. */
+    job->busy = false;
     if (!block_job_should_pause(job)) {
         co_aio_sleep_ns(blk_get_aio_context(job->blk), type, ns);
     }
+    job->busy = true;
 
     block_job_pause_point(job);
 }

Signature · 1 line

2.13.6

If a coroutine can be reentered from multiple possible sources, we need to be careful in the case that two sources try to reenter it at the same time.

There are two different cases where this can happen:

1. A coroutine spawns multiple asynchronous jobs and waits for all of them to complete. In this case, multiple reentries are expected and the coroutine is probably looping around a yield, so entering it twice is generally fine (but entering it just once after all jobs have completed would be enough, too).

Exception: When the loop condition becomes false and the first reenter already leaves the loop, we must not do a second reenter.

2. A coroutine yields and provides multiple alternative ways to be reentered. In this case, we must always only process the first reenter.

Direct entry is not a problem. It requires that the AioContext locks for the coroutine context are held, which means that the coroutine has enough time to set some state that simply prevents the second caller from reentering the coroutine, too.

Things get more interesting with aio_co_schedule() because the coroutine may be scheduled before it is (directly) entered from a second place. Then changing some state inside the coroutine doesn't help because it is already scheduled and the state won't be checked again.

For this case, we'll cancel any pending aio_co_schedule() when the coroutine is actually entered. Reentering it once is enough for all cases explained above, and a requirement for part of them.

Signed-off-by Kevin Wolf <[email protected]>

---
 include/qemu/coroutine_int.h |  1 +
 util/async.c                 | 15 ++++++++++++++-
 util/qemu-coroutine.c        | 17 +++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

include/qemu/coroutine_int.h +1 -0

index cb98892bba..73fc35e54b 100644
@@ -40,6 +40,7 @@ struct Coroutine {
     CoroutineEntry *entry;
     void *entry_arg;
     Coroutine *caller;
+    AioContext *scheduled;
 
     /* Only used when the coroutine has terminated.  */
     QSLIST_ENTRY(Coroutine) pool_next;

util/async.c +11 -1

index 0e1bd8780a..dc249e9404 100644
@@ -372,6 +372,7 @@ static bool event_notifier_poll(void *opaque)
 static void co_schedule_bh_cb(void *opaque)
 {
     AioContext *ctx = opaque;
+    AioContext *scheduled_ctx;
     QSLIST_HEAD(, Coroutine) straight, reversed;
 
     QSLIST_MOVE_ATOMIC(&reversed, &ctx->scheduled_coroutines);

@@ -388,7 +389,16 @@ static void co_schedule_bh_cb(void *opaque)
         QSLIST_REMOVE_HEAD(&straight, co_scheduled_next);
         trace_aio_co_schedule_bh_cb(ctx, co);
         aio_context_acquire(ctx);
-        qemu_coroutine_enter(co);
+        scheduled_ctx = atomic_mb_read(&co->scheduled);
+        if (scheduled_ctx == ctx) {
+            qemu_coroutine_enter(co);
+        } else {
+            /* This must be a cancelled aio_co_schedule() because the coroutine
+             * was already entered before this BH had a chance to run. If a
+             * coroutine is scheduled for two different AioContexts, something
+             * is very wrong. */
+            assert(scheduled_ctx == NULL);
+        }
         aio_context_release(ctx);
     }
 }

util/async.c +2 -0

+    /* Set co->scheduled before the coroutine becomes visible in the list */
+    atomic_mb_set(&co->scheduled, ctx);
 
 
     QSLIST_INSERT_HEAD_ATOMIC(&ctx->scheduled_coroutines,

                               co, co_scheduled_next);
     qemu_bh_schedule(ctx->co_schedule_bh);

util/qemu-coroutine.c +5 -0

index d6095c1d5a..c515b3cb4a 100644
@@ -122,6 +122,23 @@ void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)
      */
     smp_wmb();
 
+    /* Make sure that a coroutine that can alternatively reentered from two
+     * different sources isn't reentered more than once when the first caller
+     * uses aio_co_schedule() and the other one enters to coroutine directly.
+     * This is achieved by cancelling the pending aio_co_schedule().
+     *

util/qemu-coroutine.c +2 -0

+    atomic_set(&co->scheduled, NULL);
+
     ret = qemu_coroutine_switch(self, co, COROUTINE_ENTER);
 
     qemu_co_queue_run_restart(co);

Signature · 1 line

2.13.6

+

     ret = qemu_coroutine_switch(self, co, COROUTINE_ENTER);
 
     qemu_co_queue_run_restart(co);

util/qemu-coroutine.c void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)

Quoted 9 lines

util/qemu-coroutine.c +9 -0

+    /* Make sure that a coroutine that can alternatively reentered from two
+     * different sources isn't reentered more than once when the first caller
+     * uses aio_co_schedule() and the other one enters to coroutine directly.
+     * This is achieved by cancelling the pending aio_co_schedule().
+     *
+     * The other way round, if aio_co_schedule() would be called after this
+     * point, this would be a problem, too, but in practice it doesn't happen
+     * because we're holding the AioContext lock here and aio_co_schedule()
+     * callers must do the same.

No, this is not true. aio_co_schedule is thread-safe.

util/qemu-coroutine.c void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co) · line 140

Quoted 8 lines

This means that the coroutine just needs to

util/qemu-coroutine.c +7 -0

+     * prevent other callers from calling aio_co_schedule() before it yields
+     * (e.g. block job coroutines by setting job->busy = true).
+     *
+     * We still want to ensure that the second case doesn't happen, so reset
+     * co->scheduled only after setting co->caller to make the above check
+     * effective for the co_schedule_bh_cb() case. */
+    atomic_set(&co->scheduled, NULL);

This doesn't work. The coroutine is still in the list, and if someone calls aio_co_schedule again now, any coroutines linked from "co" via co_scheduled_next are lost.

(Also, the AioContext lock is by design not protecting any state in AioContext itself; the AioContext lock is only protecting things that run in an AioContext but do not have their own lock).

Am 28.11.2017 um 17:14 hat Paolo Bonzini geschrieben:

Quoted 11 lines · Kevin Wolf

util/qemu-coroutine.c void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)

Quoted 9 lines

util/qemu-coroutine.c +9 -0

+    /* Make sure that a coroutine that can alternatively reentered from two
+     * different sources isn't reentered more than once when the first caller
+     * uses aio_co_schedule() and the other one enters to coroutine directly.
+     * This is achieved by cancelling the pending aio_co_schedule().
+     *
+     * The other way round, if aio_co_schedule() would be called after this
+     * point, this would be a problem, too, but in practice it doesn't happen
+     * because we're holding the AioContext lock here and aio_co_schedule()
+     * callers must do the same.

Quoted 1 line

No, this is not true. aio_co_schedule is thread-safe.

Hm... With the reproducer we were specfically looking at qmp_block_job_cancel(), which does take the AioContext locks. But it might not be as universal as I thought.

To be honest, I just wasn't sure what to do with this case anyway. It means that the coroutine is already running when someone else schedules it. We don't really know whether we have to enter it a second time or not.

So if it can indeed happen in practice, we need to think a bit more about this.

Quoted 10 lines

util/qemu-coroutine.c void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co) · line 140

Quoted 9 lines

This means that the coroutine just needs to

util/qemu-coroutine.c +7 -0

+     * prevent other callers from calling aio_co_schedule() before it yields
+     * (e.g. block job coroutines by setting job->busy = true).
+     *
+     * We still want to ensure that the second case doesn't happen, so reset
+     * co->scheduled only after setting co->caller to make the above check
+     * effective for the co_schedule_bh_cb() case. */
+    atomic_set(&co->scheduled, NULL);

Quoted 3 lines

This doesn't work. The coroutine is still in the list, and if someone calls aio_co_schedule again now, any coroutines linked from "co" via co_scheduled_next are lost.

Why would they? We still iterate the whole list in co_schedule_bh_cb(), we just skip the single qemu_coroutine_enter().

Quoted 3 lines

(Also, the AioContext lock is by design not protecting any state in AioContext itself; the AioContext lock is only protecting things that run in an AioContext but do not have their own lock).

Such as the coroutine we want to enter, no?

Kevin

Quoted 23 lines · Kevin Wolf

Am 28.11.2017 um 17:14 hat Paolo Bonzini geschrieben:

Quoted 11 lines · Kevin Wolf

util/qemu-coroutine.c void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)

Quoted 9 lines

util/qemu-coroutine.c +9 -0

+    /* Make sure that a coroutine that can alternatively reentered from two
+     * different sources isn't reentered more than once when the first caller
+     * uses aio_co_schedule() and the other one enters to coroutine directly.
+     * This is achieved by cancelling the pending aio_co_schedule().
+     *
+     * The other way round, if aio_co_schedule() would be called after this
+     * point, this would be a problem, too, but in practice it doesn't happen
+     * because we're holding the AioContext lock here and aio_co_schedule()
+     * callers must do the same.

Quoted 1 line

No, this is not true. aio_co_schedule is thread-safe.

Hm... With the reproducer we were specfically looking at qmp_block_job_cancel(), which does take the AioContext locks. But it might not be as universal as I thought.

To be honest, I just wasn't sure what to do with this case anyway. It means that the coroutine is already running when someone else schedules it. We don't really know whether we have to enter it a second time or not.

Quoted 2 lines

So if it can indeed happen in practice, we need to think a bit more about this.

It would be nice if, on coroutine termination, we could unschedule all pending executions for that coroutine. I think use-after-free is the main concern for someone else calling aio_co_schedule() while the coroutine is currently running.

Quoted 27 lines · Jeff Cody

Quoted 25 lines · Kevin Wolf

Am 28.11.2017 um 17:14 hat Paolo Bonzini geschrieben:

Quoted 11 lines · Kevin Wolf

util/qemu-coroutine.c void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)

Quoted 9 lines

util/qemu-coroutine.c +9 -0

+    /* Make sure that a coroutine that can alternatively reentered from two
+     * different sources isn't reentered more than once when the first caller
+     * uses aio_co_schedule() and the other one enters to coroutine directly.
+     * This is achieved by cancelling the pending aio_co_schedule().
+     *
+     * The other way round, if aio_co_schedule() would be called after this
+     * point, this would be a problem, too, but in practice it doesn't happen
+     * because we're holding the AioContext lock here and aio_co_schedule()
+     * callers must do the same.

Quoted 1 line

No, this is not true. aio_co_schedule is thread-safe.

Hm... With the reproducer we were specfically looking at qmp_block_job_cancel(), which does take the AioContext locks. But it might not be as universal as I thought.

To be honest, I just wasn't sure what to do with this case anyway. It means that the coroutine is already running when someone else schedules it. We don't really know whether we have to enter it a second time or not.

So if it can indeed happen in practice, we need to think a bit more about this.

Quoted 4 lines

It would be nice if, on coroutine termination, we could unschedule all pending executions for that coroutine. I think use-after-free is the main concern for someone else calling aio_co_schedule() while the coroutine is currently running.

Yes, terminating a scheduled coroutine is a bug; same for scheduling a terminated coroutine, both orders are wrong. However, "unscheduling" is not the solution; you would just be papering over the issue.

aio_co_schedule() on a running coroutine can only happen when the coroutine is going to yield soon.

Quoted 3 lines

Yes, terminating a scheduled coroutine is a bug; same for scheduling a terminated coroutine, both orders are wrong. However, "unscheduling" is not the solution; you would just be papering over the issue.

Maybe we should at least add an abort on coroutine termination if there are still outstanding schedules, as that is preferable to operating in the weeds.

Quoted 3 lines

aio_co_schedule() on a running coroutine can only happen when the coroutine is going to yield soon.

That is a bit vague. What is "soon", and how does an external caller know if a coroutine is going to yield in this timeframe?

Quoted 23 lines

Am 28.11.2017 um 17:14 hat Paolo Bonzini geschrieben:

Quoted 11 lines · Kevin Wolf

util/qemu-coroutine.c void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)

Quoted 9 lines

util/qemu-coroutine.c +9 -0

+    /* Make sure that a coroutine that can alternatively reentered from two
+     * different sources isn't reentered more than once when the first caller
+     * uses aio_co_schedule() and the other one enters to coroutine directly.
+     * This is achieved by cancelling the pending aio_co_schedule().
+     *
+     * The other way round, if aio_co_schedule() would be called after this
+     * point, this would be a problem, too, but in practice it doesn't happen
+     * because we're holding the AioContext lock here and aio_co_schedule()
+     * callers must do the same.

Quoted 1 line

No, this is not true. aio_co_schedule is thread-safe.

Hm... With the reproducer we were specfically looking at qmp_block_job_cancel(), which does take the AioContext locks. But it might not be as universal as I thought.

To be honest, I just wasn't sure what to do with this case anyway. It means that the coroutine is already running when someone else schedules it. We don't really know whether we have to enter it a second time or not.

Quoted 2 lines

So if it can indeed happen in practice, we need to think a bit more about this.

It would be nice if, on coroutine termination, we could unschedule all pending executions for that coroutine. I think use-after-free is the main concern for someone else calling aio_co_schedule() while the coroutine is currently running.

To be honest, I just wasn't sure what to do with this case anyway. It means that the coroutine is already running when someone else schedules it. We don't really know whether we have to enter it a second time or not.

So if it can indeed happen in practice, we need to think a bit more about this.

Quoted 9 lines

Quoted 8 lines

This means that the coroutine just needs to

util/qemu-coroutine.c +7 -0

+     * prevent other callers from calling aio_co_schedule() before it yields
+     * (e.g. block job coroutines by setting job->busy = true).
+     *
+     * We still want to ensure that the second case doesn't happen, so reset
+     * co->scheduled only after setting co->caller to make the above check
+     * effective for the co_schedule_bh_cb() case. */
+    atomic_set(&co->scheduled, NULL);

Quoted 3 lines

This doesn't work. The coroutine is still in the list, and if someone calls aio_co_schedule again now, any coroutines linked from "co" via co_scheduled_next are lost.

Why would they? We still iterate the whole list in co_schedule_bh_cb(), we just skip the single qemu_coroutine_enter().

Quoted 3 lines

(Also, the AioContext lock is by design not protecting any state in AioContext itself; the AioContext lock is only protecting things that run in an AioContext but do not have their own lock).

Such as the coroutine we want to enter, no?

If a coroutine can be reentered from multiple possible sources, we need to be careful in the case that two sources try to reenter it at the same time.

There are two different cases where this can happen:

1. A coroutine spawns multiple asynchronous jobs and waits for all of them to complete. In this case, multiple reentries are expected and the coroutine is probably looping around a yield, so entering it twice is generally fine (but entering it just once after all jobs have completed would be enough, too).

Exception: When the loop condition becomes false and the first reenter already leaves the loop, we must not do a second reenter.

2. A coroutine yields and provides multiple alternative ways to be reentered. In this case, we must always only process the first reenter.

Direct entry is not a problem. It requires that the AioContext locks for the coroutine context are held, which means that the coroutine has enough time to set some state that simply prevents the second caller from reentering the coroutine, too.

Things get more interesting with aio_co_schedule() because the coroutine may be scheduled before it is (directly) entered from a second place. Then changing some state inside the coroutine doesn't help because it is already scheduled and the state won't be checked again.

For this case, we'll cancel any pending aio_co_schedule() when the coroutine is actually entered. Reentering it once is enough for all cases explained above, and a requirement for part of them.

Signed-off-by Kevin Wolf <[email protected]>

---
 include/qemu/coroutine_int.h |  1 +
 util/async.c                 | 15 ++++++++++++++-
 util/qemu-coroutine.c        | 17 +++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

include/qemu/coroutine_int.h +1 -0

index cb98892bba..73fc35e54b 100644
@@ -40,6 +40,7 @@ struct Coroutine {
     CoroutineEntry *entry;
     void *entry_arg;
     Coroutine *caller;
+    AioContext *scheduled;
 
     /* Only used when the coroutine has terminated.  */
     QSLIST_ENTRY(Coroutine) pool_next;

util/async.c +14 -1

index 0e1bd8780a..dc249e9404 100644
@@ -372,6 +372,7 @@ static bool event_notifier_poll(void *opaque)
 static void co_schedule_bh_cb(void *opaque)
 {
     AioContext *ctx = opaque;
+    AioContext *scheduled_ctx;
     QSLIST_HEAD(, Coroutine) straight, reversed;
 
     QSLIST_MOVE_ATOMIC(&reversed, &ctx->scheduled_coroutines);

@@ -388,7 +389,16 @@ static void co_schedule_bh_cb(void *opaque)
         QSLIST_REMOVE_HEAD(&straight, co_scheduled_next);
         trace_aio_co_schedule_bh_cb(ctx, co);
         aio_context_acquire(ctx);
-        qemu_coroutine_enter(co);
+        scheduled_ctx = atomic_mb_read(&co->scheduled);
+        if (scheduled_ctx == ctx) {
+            qemu_coroutine_enter(co);
+        } else {
+            /* This must be a cancelled aio_co_schedule() because the coroutine
+             * was already entered before this BH had a chance to run. If a
+             * coroutine is scheduled for two different AioContexts, something
+             * is very wrong. */
+            assert(scheduled_ctx == NULL);
+        }
         aio_context_release(ctx);
     }
 }

@@ -438,6 +448,9 @@ fail:
 void aio_co_schedule(AioContext *ctx, Coroutine *co)
 {
     trace_aio_co_schedule(ctx, co);
+
+    /* Set co->scheduled before the coroutine becomes visible in the list */
+    atomic_mb_set(&co->scheduled, ctx);
     QSLIST_INSERT_HEAD_ATOMIC(&ctx->scheduled_coroutines,
                               co, co_scheduled_next);
     qemu_bh_schedule(ctx->co_schedule_bh);

util/qemu-coroutine.c +0 -0

index d6095c1d5a..c515b3cb4a 100644

util/qemu-coroutine.c +5 -0

+     * different sources isn't reentered more than once when the first caller
+     * uses aio_co_schedule() and the other one enters to coroutine directly.
+     * This is achieved by cancelling the pending aio_co_schedule().
+     *
+     * The other way round, if aio_co_schedule() would be called after this

util/qemu-coroutine.c +1 -0

+
     ret = qemu_coroutine_switch(self, co, COROUTINE_ENTER);
 
     qemu_co_queue_run_restart(co);

Signature · 1 line

2.13.6

If a coroutine can be reentered from multiple possible sources, we need to be careful in the case that two sources try to reenter it at the same time.

For this case, we'll cancel any pending aio_co_schedule() when the coroutine is actually entered. Reentering it once is enough for all cases explained above, and a requirement for part of them.

Signed-off-by Kevin Wolf <[email protected]>

---
  include/qemu/coroutine_int.h |  1 +

+++ b/util/qemu-coroutine.c

util/qemu-coroutine.c +6 -0

+     *
+     * We still want to ensure that the second case doesn't happen, so reset
+     * co->scheduled only after setting co->caller to make the above check
+     * effective for the co_schedule_bh_cb() case. */
+    atomic_set(&co->scheduled, NULL);
+
      ret = qemu_coroutine_switch(self, co, COROUTINE_ENTER);
 
      qemu_co_queue_run_restart(co);

The .drained_begin/end callbacks can (directly or indirectly via aio_poll()) cause block nodes to be removed or the current BdrvChild to point to a different child node.

Use QLIST_FOREACH_SAFE() to make sure we don't access invalid BlockDriverStates or accidentally continue iterating the parents of the new child node instead of the node we actually came from.

Signed-off-by Kevin Wolf <[email protected]>

---
 block/io.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

block/io.c +4 -4

index 4fdf93a014..6773926fc1 100644
@@ -42,9 +42,9 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
 
 void bdrv_parent_drained_begin(BlockDriverState *bs)
 {
-    BdrvChild *c;
+    BdrvChild *c, *next;
 
-    QLIST_FOREACH(c, &bs->parents, next_parent) {
+    QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {
         if (c->role->drained_begin) {
             c->role->drained_begin(c);
         }

@@ -53,9 +53,9 @@ void bdrv_parent_drained_begin(BlockDriverState *bs)
 
 void bdrv_parent_drained_end(BlockDriverState *bs)
 {
-    BdrvChild *c;
+    BdrvChild *c, *next;
 
-    QLIST_FOREACH(c, &bs->parents, next_parent) {
+    QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {
         if (c->role->drained_end) {
             c->role->drained_end(c);
         }

Signature · 1 line

2.13.6

This reverts the recent fix for the case of qemu-iotests 200 (block job completion races when using a separate I/O thread) that only fixed a symptom and caused many regressions, and replaces it with a new solution that hopefully attacks the right root cause.

The series also contains a bonus fix for a bug that was uncovered by the different timing with the patches that are reverted here.

Kevin Wolf (4):
Revert "coroutine: abort if we try to schedule or enter a pending
coroutine"
Revert "blockjob: do not allow coroutine double entry or
entry-after-completion"
coroutine: Cancel aio_co_schedule() on direct entry
block: Expect graph changes in bdrv_parent_drained_begin/end

 include/block/blockjob_int.h |  3 +--
 include/qemu/coroutine_int.h | 14 ++++----------
 block/io.c                   |  8 ++++----
 blockjob.c                   |  7 ++-----
 util/async.c                 | 26 +++++++++++++-------------
 util/qemu-coroutine-sleep.c  | 12 ------------
 util/qemu-coroutine.c        | 31 +++++++++++++++++--------------
 7 files changed, 41 insertions(+), 60 deletions(-)

Signature · 1 line

2.13.6

This reverts the recent fix for the case of qemu-iotests 200 (block job completion races when using a separate I/O thread) that only fixed a symptom and caused many regressions, and replaces it with a new solution that hopefully attacks the right root cause.

The series also contains a bonus fix for a bug that was uncovered by the different timing with the patches that are reverted here.

Kevin Wolf (4):
Revert "coroutine: abort if we try to schedule or enter a pending
coroutine"
Revert "blockjob: do not allow coroutine double entry or
entry-after-completion"
coroutine: Cancel aio_co_schedule() on direct entry
block: Expect graph changes in bdrv_parent_drained_begin/end

 include/block/blockjob_int.h |  3 +--
 include/qemu/coroutine_int.h | 14 ++++----------
 block/io.c                   |  8 ++++----
 blockjob.c                   |  7 ++-----
 util/async.c                 | 26 +++++++++++++-------------
 util/qemu-coroutine-sleep.c  | 12 ------------
 util/qemu-coroutine.c        | 31 +++++++++++++++++--------------
 7 files changed, 41 insertions(+), 60 deletions(-)

Signature · 1 line

2.13.6