aeebdca142
drive-mirror: Fix coroutine reentrance (bz #1266936) Fix udp socket 'localaddr' (bz #1268708)
118 lines
4.6 KiB
Diff
118 lines
4.6 KiB
Diff
From: Kevin Wolf <kwolf@redhat.com>
|
|
Date: Thu, 13 Aug 2015 10:41:50 +0200
|
|
Subject: [PATCH] mirror: Fix coroutine reentrance
|
|
|
|
This fixes a regression introduced by commit dcfb3beb ("mirror: Do zero
|
|
write on target if sectors not allocated"), which was reported to cause
|
|
aborts with the message "Co-routine re-entered recursively".
|
|
|
|
The cause for this bug is the following code in mirror_iteration_done():
|
|
|
|
if (s->common.busy) {
|
|
qemu_coroutine_enter(s->common.co, NULL);
|
|
}
|
|
|
|
This has always been ugly because - unlike most places that reenter - it
|
|
doesn't have a specific yield that it pairs with, but is more
|
|
uncontrolled. What we really mean here is "reenter the coroutine if
|
|
it's in one of the four explicit yields in mirror.c".
|
|
|
|
This used to be equivalent with s->common.busy because neither
|
|
mirror_run() nor mirror_iteration() call any function that could yield.
|
|
However since commit dcfb3beb this doesn't hold true any more:
|
|
bdrv_get_block_status_above() can yield.
|
|
|
|
So what happens is that bdrv_get_block_status_above() wants to take a
|
|
lock that is already held, so it adds itself to the queue of waiting
|
|
coroutines and yields. Instead of being woken up by the unlock function,
|
|
however, it gets woken up by mirror_iteration_done(), which is obviously
|
|
wrong.
|
|
|
|
In most cases the code actually happens to cope fairly well with such
|
|
cases, but in this specific case, the unlock must already have scheduled
|
|
the coroutine for wakeup when mirror_iteration_done() reentered it. And
|
|
then the coroutine happened to process the scheduled restarts and tried
|
|
to reenter itself recursively.
|
|
|
|
This patch fixes the problem by pairing the reenter in
|
|
mirror_iteration_done() with specific yields instead of abusing
|
|
s->common.busy.
|
|
|
|
Cc: qemu-stable@nongnu.org
|
|
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
|
|
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
|
|
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
|
|
Reviewed-by: Jeff Cody <jcody@redhat.com>
|
|
Message-id: 1439455310-11263-1-git-send-email-kwolf@redhat.com
|
|
Signed-off-by: Jeff Cody <jcody@redhat.com>
|
|
(cherry picked from commit e424aff5f307227b1c2512bbb8ece891bb895cef)
|
|
---
|
|
block/mirror.c | 15 ++++++++++-----
|
|
1 file changed, 10 insertions(+), 5 deletions(-)
|
|
|
|
diff --git a/block/mirror.c b/block/mirror.c
|
|
index 6f1bc3c..d06a0be 100644
|
|
--- a/block/mirror.c
|
|
+++ b/block/mirror.c
|
|
@@ -58,6 +58,7 @@ typedef struct MirrorBlockJob {
|
|
int sectors_in_flight;
|
|
int ret;
|
|
bool unmap;
|
|
+ bool waiting_for_io;
|
|
} MirrorBlockJob;
|
|
|
|
typedef struct MirrorOp {
|
|
@@ -112,11 +113,7 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
|
|
qemu_iovec_destroy(&op->qiov);
|
|
g_slice_free(MirrorOp, op);
|
|
|
|
- /* Enter coroutine when it is not sleeping. The coroutine sleeps to
|
|
- * rate-limit itself. The coroutine will eventually resume since there is
|
|
- * a sleep timeout so don't wake it early.
|
|
- */
|
|
- if (s->common.busy) {
|
|
+ if (s->waiting_for_io) {
|
|
qemu_coroutine_enter(s->common.co, NULL);
|
|
}
|
|
}
|
|
@@ -206,7 +203,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
|
|
/* Wait for I/O to this cluster (from a previous iteration) to be done. */
|
|
while (test_bit(next_chunk, s->in_flight_bitmap)) {
|
|
trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
|
|
+ s->waiting_for_io = true;
|
|
qemu_coroutine_yield();
|
|
+ s->waiting_for_io = false;
|
|
}
|
|
|
|
do {
|
|
@@ -242,7 +241,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
|
|
*/
|
|
while (nb_chunks == 0 && s->buf_free_count < added_chunks) {
|
|
trace_mirror_yield_buf_busy(s, nb_chunks, s->in_flight);
|
|
+ s->waiting_for_io = true;
|
|
qemu_coroutine_yield();
|
|
+ s->waiting_for_io = false;
|
|
}
|
|
if (s->buf_free_count < nb_chunks + added_chunks) {
|
|
trace_mirror_break_buf_busy(s, nb_chunks, s->in_flight);
|
|
@@ -341,7 +342,9 @@ static void mirror_free_init(MirrorBlockJob *s)
|
|
static void mirror_drain(MirrorBlockJob *s)
|
|
{
|
|
while (s->in_flight > 0) {
|
|
+ s->waiting_for_io = true;
|
|
qemu_coroutine_yield();
|
|
+ s->waiting_for_io = false;
|
|
}
|
|
}
|
|
|
|
@@ -516,7 +519,9 @@ static void coroutine_fn mirror_run(void *opaque)
|
|
if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
|
|
(cnt == 0 && s->in_flight > 0)) {
|
|
trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
|
|
+ s->waiting_for_io = true;
|
|
qemu_coroutine_yield();
|
|
+ s->waiting_for_io = false;
|
|
continue;
|
|
} else if (cnt != 0) {
|
|
delay_ns = mirror_iteration(s);
|