On Thu, Aug 13, 2015 at 10:41:50AM +0200, Kevin Wolf wrote: > 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-sta...@nongnu.org > Signed-off-by: Kevin Wolf <kw...@redhat.com> > --- > block/mirror.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index fc4d8f5..b2fb4b9 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -60,6 +60,7 @@ typedef struct MirrorBlockJob { > int sectors_in_flight; > int ret; > bool unmap; > + bool waiting_for_io; > } MirrorBlockJob; > > typedef struct MirrorOp { > @@ -114,11 +115,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); > } > } > @@ -203,7 +200,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 { > @@ -239,7 +238,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); > @@ -333,7 +334,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; > } > } > > @@ -506,7 +509,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); > -- > 1.8.3.1 >
Reviewed-by: Jeff Cody <jc...@redhat.com>