[Qemu-devel] [PATCH] linux-aio: avoid deadlock in nested aio_poll() calls

[Qemu-devel] [PATCH] linux-aio: avoid deadlock in nested aio_poll() calls

[Stefan Hajnoczi]

If two Linux AIO request completions are fetched in the same
io_getevents() call, QEMU will deadlock if request A's callback waits
for request B to complete using an aio_poll() loop.  This was reported
to happen with the mirror blockjob.

This patch moves completion processing into a BH and makes it resumable.
Nested event loops can resume completion processing so that request B
will complete and the deadlock will not occur.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Marcin Gibuła <m.gibula@beyond.pl>
Reported-by: Marcin Gibuła <m.gibula@beyond.pl>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/linux-aio.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 55 insertions(+), 16 deletions(-)

diff --git a/block/linux-aio.c b/block/linux-aio.c
index 7ac7e8c..9aca758 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -51,6 +51,12 @@ struct qemu_laio_state {
 
     /* io queue for submit at batch */
     LaioQueue io_q;
+
+    /* I/O completion processing */
+    QEMUBH *completion_bh;
+    struct io_event events[MAX_EVENTS];
+    int event_idx;
+    int event_max;
 };
 
 static inline ssize_t io_event_ret(struct io_event *ev)
@@ -86,27 +92,58 @@ static void qemu_laio_process_completion(struct qemu_laio_state *s,
     qemu_aio_release(laiocb);
 }
 
-static void qemu_laio_completion_cb(EventNotifier *e)
+/* The completion BH fetches completed I/O requests and invokes their
+ * callbacks.
+ *
+ * The function is somewhat tricky because it supports nested event loops, for
+ * example when a request callback invokes aio_poll().  In order to do this,
+ * the completion events array and index are kept in qemu_laio_state.  The BH
+ * reschedules itself as long as there are completions pending so it will
+ * either be called again in a nested event loop or will be called after all
+ * events have been completed.  When there are no events left to complete, the
+ * BH returns without rescheduling.
+ */
+static void qemu_laio_completion_bh(void *opaque)
 {
-    struct qemu_laio_state *s = container_of(e, struct qemu_laio_state, e);
-
-    while (event_notifier_test_and_clear(&s->e)) {
-        struct io_event events[MAX_EVENTS];
-        struct timespec ts = { 0 };
-        int nevents, i;
+    struct qemu_laio_state *s = opaque;
 
+    /* Fetch more completion events when empty */
+    if (s->event_idx == s->event_max) {
         do {
-            nevents = io_getevents(s->ctx, MAX_EVENTS, MAX_EVENTS, events, &ts);
-        } while (nevents == -EINTR);
+            struct timespec ts = { 0 };
+            s->event_max = io_getevents(s->ctx, MAX_EVENTS, MAX_EVENTS,
+                                        s->events, &ts);
+        } while (s->event_max == -EINTR);
+
+        s->event_idx = 0;
+        if (s->event_max <= 0) {
+            s->event_max = 0;
+            return; /* no more events */
+        }
+    }
 
-        for (i = 0; i < nevents; i++) {
-            struct iocb *iocb = events[i].obj;
-            struct qemu_laiocb *laiocb =
-                    container_of(iocb, struct qemu_laiocb, iocb);
+    /* Reschedule so nested event loops see currently pending completions */
+    qemu_bh_schedule(s->completion_bh);
 
-            laiocb->ret = io_event_ret(&events[i]);
-            qemu_laio_process_completion(s, laiocb);
-        }
+    /* Process completion events */
+    while (s->event_idx < s->event_max) {
+        struct iocb *iocb = s->events[s->event_idx].obj;
+        struct qemu_laiocb *laiocb =
+                container_of(iocb, struct qemu_laiocb, iocb);
+
+        laiocb->ret = io_event_ret(&s->events[s->event_idx]);
+        s->event_idx++;
+
+        qemu_laio_process_completion(s, laiocb);
+    }
+}
+
+static void qemu_laio_completion_cb(EventNotifier *e)
+{
+    struct qemu_laio_state *s = container_of(e, struct qemu_laio_state, e);
+
+    if (event_notifier_test_and_clear(&s->e)) {
+        qemu_bh_schedule(s->completion_bh);
     }
 }
 
@@ -272,12 +309,14 @@ void laio_detach_aio_context(void *s_, AioContext *old_context)
     struct qemu_laio_state *s = s_;
 
     aio_set_event_notifier(old_context, &s->e, NULL);
+    qemu_bh_delete(s->completion_bh);
 }
 
 void laio_attach_aio_context(void *s_, AioContext *new_context)
 {
     struct qemu_laio_state *s = s_;
 
+    s->completion_bh = aio_bh_new(new_context, qemu_laio_completion_bh, s);
     aio_set_event_notifier(new_context, &s->e, qemu_laio_completion_cb);
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH] linux-aio: avoid deadlock in nested aio_poll() calls

[Marcin Gibuła]

W dniu 2014-08-04 17:56, Stefan Hajnoczi pisze:
> If two Linux AIO request completions are fetched in the same
> io_getevents() call, QEMU will deadlock if request A's callback waits
> for request B to complete using an aio_poll() loop.  This was reported
> to happen with the mirror blockjob.

s/mirror/commit/

> This patch moves completion processing into a BH and makes it resumable.
> Nested event loops can resume completion processing so that request B
> will complete and the deadlock will not occur.
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Marcin Gibuła <m.gibula@beyond.pl>
> Reported-by: Marcin Gibuła <m.gibula@beyond.pl>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

I'll test it tomorrow.

-- 
mg

Re: [Qemu-devel] [PATCH] linux-aio: avoid deadlock in nested aio_poll() calls

[Ming Lei]

On Mon, Aug 4, 2014 at 11:56 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> If two Linux AIO request completions are fetched in the same
> io_getevents() call, QEMU will deadlock if request A's callback waits
> for request B to complete using an aio_poll() loop.  This was reported
> to happen with the mirror blockjob.
>
> This patch moves completion processing into a BH and makes it resumable.
> Nested event loops can resume completion processing so that request B
> will complete and the deadlock will not occur.
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Marcin Gibuła <m.gibula@beyond.pl>
> Reported-by: Marcin Gibuła <m.gibula@beyond.pl>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/linux-aio.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 55 insertions(+), 16 deletions(-)
>
> diff --git a/block/linux-aio.c b/block/linux-aio.c
> index 7ac7e8c..9aca758 100644
> --- a/block/linux-aio.c
> +++ b/block/linux-aio.c
> @@ -51,6 +51,12 @@ struct qemu_laio_state {
>
>      /* io queue for submit at batch */
>      LaioQueue io_q;
> +
> +    /* I/O completion processing */
> +    QEMUBH *completion_bh;
> +    struct io_event events[MAX_EVENTS];
> +    int event_idx;
> +    int event_max;
>  };
>
>  static inline ssize_t io_event_ret(struct io_event *ev)
> @@ -86,27 +92,58 @@ static void qemu_laio_process_completion(struct qemu_laio_state *s,
>      qemu_aio_release(laiocb);
>  }
>
> -static void qemu_laio_completion_cb(EventNotifier *e)
> +/* The completion BH fetches completed I/O requests and invokes their
> + * callbacks.
> + *
> + * The function is somewhat tricky because it supports nested event loops, for
> + * example when a request callback invokes aio_poll().  In order to do this,

Looks it is a very tricky usage, maybe it is better to change the caller.

> + * the completion events array and index are kept in qemu_laio_state.  The BH
> + * reschedules itself as long as there are completions pending so it will
> + * either be called again in a nested event loop or will be called after all
> + * events have been completed.  When there are no events left to complete, the
> + * BH returns without rescheduling.
> + */
> +static void qemu_laio_completion_bh(void *opaque)
>  {
> -    struct qemu_laio_state *s = container_of(e, struct qemu_laio_state, e);
> -
> -    while (event_notifier_test_and_clear(&s->e)) {
> -        struct io_event events[MAX_EVENTS];
> -        struct timespec ts = { 0 };
> -        int nevents, i;
> +    struct qemu_laio_state *s = opaque;
>
> +    /* Fetch more completion events when empty */
> +    if (s->event_idx == s->event_max) {
>          do {
> -            nevents = io_getevents(s->ctx, MAX_EVENTS, MAX_EVENTS, events, &ts);
> -        } while (nevents == -EINTR);
> +            struct timespec ts = { 0 };
> +            s->event_max = io_getevents(s->ctx, MAX_EVENTS, MAX_EVENTS,
> +                                        s->events, &ts);
> +        } while (s->event_max == -EINTR);
> +
> +        s->event_idx = 0;
> +        if (s->event_max <= 0) {
> +            s->event_max = 0;
> +            return; /* no more events */
> +        }
> +    }
>
> -        for (i = 0; i < nevents; i++) {
> -            struct iocb *iocb = events[i].obj;
> -            struct qemu_laiocb *laiocb =
> -                    container_of(iocb, struct qemu_laiocb, iocb);
> +    /* Reschedule so nested event loops see currently pending completions */
> +    qemu_bh_schedule(s->completion_bh);
>
> -            laiocb->ret = io_event_ret(&events[i]);
> -            qemu_laio_process_completion(s, laiocb);
> -        }
> +    /* Process completion events */
> +    while (s->event_idx < s->event_max) {
> +        struct iocb *iocb = s->events[s->event_idx].obj;
> +        struct qemu_laiocb *laiocb =
> +                container_of(iocb, struct qemu_laiocb, iocb);
> +
> +        laiocb->ret = io_event_ret(&s->events[s->event_idx]);
> +        s->event_idx++;
> +
> +        qemu_laio_process_completion(s, laiocb);

The implementation is same tricky with the usage, :-)

Also using a FIFO style implementation should be more efficient
since IO events can still be read and completed in current BH handler
if the queue isn't full, but becomes more complicated.

Thanks,

Re: [Qemu-devel] [PATCH] linux-aio: avoid deadlock in nested aio_poll() calls

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5582 bytes --]

On Tue, Aug 05, 2014 at 06:44:25PM +0800, Ming Lei wrote:
> On Mon, Aug 4, 2014 at 11:56 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > If two Linux AIO request completions are fetched in the same
> > io_getevents() call, QEMU will deadlock if request A's callback waits
> > for request B to complete using an aio_poll() loop.  This was reported
> > to happen with the mirror blockjob.
> >
> > This patch moves completion processing into a BH and makes it resumable.
> > Nested event loops can resume completion processing so that request B
> > will complete and the deadlock will not occur.
> >
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Ming Lei <ming.lei@canonical.com>
> > Cc: Marcin Gibuła <m.gibula@beyond.pl>
> > Reported-by: Marcin Gibuła <m.gibula@beyond.pl>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  block/linux-aio.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 55 insertions(+), 16 deletions(-)
> >
> > diff --git a/block/linux-aio.c b/block/linux-aio.c
> > index 7ac7e8c..9aca758 100644
> > --- a/block/linux-aio.c
> > +++ b/block/linux-aio.c
> > @@ -51,6 +51,12 @@ struct qemu_laio_state {
> >
> >      /* io queue for submit at batch */
> >      LaioQueue io_q;
> > +
> > +    /* I/O completion processing */
> > +    QEMUBH *completion_bh;
> > +    struct io_event events[MAX_EVENTS];
> > +    int event_idx;
> > +    int event_max;
> >  };
> >
> >  static inline ssize_t io_event_ret(struct io_event *ev)
> > @@ -86,27 +92,58 @@ static void qemu_laio_process_completion(struct qemu_laio_state *s,
> >      qemu_aio_release(laiocb);
> >  }
> >
> > -static void qemu_laio_completion_cb(EventNotifier *e)
> > +/* The completion BH fetches completed I/O requests and invokes their
> > + * callbacks.
> > + *
> > + * The function is somewhat tricky because it supports nested event loops, for
> > + * example when a request callback invokes aio_poll().  In order to do this,
> 
> Looks it is a very tricky usage, maybe it is better to change the caller.

This comment is not about usage.  It's just for people reading the
implementation.  I can move it inside the function body, if you like.

I like the idea of eliminating nested event loops, but it requires a
huge change: making all callers either async (using callbacks) or
coroutines so they can yield.

There are many callers so this is a lot of work and will have
side-effects too.

BTW, here is the thread-pool.c fix which is analogous to this patch:
https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg02437.html

> > + * the completion events array and index are kept in qemu_laio_state.  The BH
> > + * reschedules itself as long as there are completions pending so it will
> > + * either be called again in a nested event loop or will be called after all
> > + * events have been completed.  When there are no events left to complete, the
> > + * BH returns without rescheduling.
> > + */
> > +static void qemu_laio_completion_bh(void *opaque)
> >  {
> > -    struct qemu_laio_state *s = container_of(e, struct qemu_laio_state, e);
> > -
> > -    while (event_notifier_test_and_clear(&s->e)) {
> > -        struct io_event events[MAX_EVENTS];
> > -        struct timespec ts = { 0 };
> > -        int nevents, i;
> > +    struct qemu_laio_state *s = opaque;
> >
> > +    /* Fetch more completion events when empty */
> > +    if (s->event_idx == s->event_max) {
> >          do {
> > -            nevents = io_getevents(s->ctx, MAX_EVENTS, MAX_EVENTS, events, &ts);
> > -        } while (nevents == -EINTR);
> > +            struct timespec ts = { 0 };
> > +            s->event_max = io_getevents(s->ctx, MAX_EVENTS, MAX_EVENTS,
> > +                                        s->events, &ts);
> > +        } while (s->event_max == -EINTR);
> > +
> > +        s->event_idx = 0;
> > +        if (s->event_max <= 0) {
> > +            s->event_max = 0;
> > +            return; /* no more events */
> > +        }
> > +    }
> >
> > -        for (i = 0; i < nevents; i++) {
> > -            struct iocb *iocb = events[i].obj;
> > -            struct qemu_laiocb *laiocb =
> > -                    container_of(iocb, struct qemu_laiocb, iocb);
> > +    /* Reschedule so nested event loops see currently pending completions */
> > +    qemu_bh_schedule(s->completion_bh);
> >
> > -            laiocb->ret = io_event_ret(&events[i]);
> > -            qemu_laio_process_completion(s, laiocb);
> > -        }
> > +    /* Process completion events */
> > +    while (s->event_idx < s->event_max) {
> > +        struct iocb *iocb = s->events[s->event_idx].obj;
> > +        struct qemu_laiocb *laiocb =
> > +                container_of(iocb, struct qemu_laiocb, iocb);
> > +
> > +        laiocb->ret = io_event_ret(&s->events[s->event_idx]);
> > +        s->event_idx++;
> > +
> > +        qemu_laio_process_completion(s, laiocb);
> 
> The implementation is same tricky with the usage, :-)
> 
> Also using a FIFO style implementation should be more efficient
> since IO events can still be read and completed in current BH handler
> if the queue isn't full, but becomes more complicated.

That might help but should be benchmarked.

Another trick is calling qemu_laio_completion_bh() directly from
qemu_laio_completion_cb() to avoid a BH iteration.

I think they are premature optimization.  Let's first agree whether this
fix is correct or not :).

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] linux-aio: avoid deadlock in nested aio_poll() calls

[Marcin Gibuła]

On 04.08.2014 17:56, Stefan Hajnoczi wrote:
> If two Linux AIO request completions are fetched in the same
> io_getevents() call, QEMU will deadlock if request A's callback waits
> for request B to complete using an aio_poll() loop.  This was reported
> to happen with the mirror blockjob.
>
> This patch moves completion processing into a BH and makes it resumable.
> Nested event loops can resume completion processing so that request B
> will complete and the deadlock will not occur.
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Marcin Gibuła <m.gibula@beyond.pl>
> Reported-by: Marcin Gibuła <m.gibula@beyond.pl>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Still hangs...

Backtrace still looks like this:

Thread 1 (Thread 0x7f3d5313a900 (LWP 17440)):
#0  0x00007f3d4f38f286 in ppoll () from /lib64/libc.so.6
#1  0x00007f3d5347465b in ppoll (__ss=0x0, __timeout=0x0, 
__nfds=<optimized out>, __fds=<optimized out>) at 
/usr/include/bits/poll2.h:77
#2  qemu_poll_ns (fds=<optimized out>, nfds=<optimized out>, 
timeout=<optimized out>)
     at 
/var/tmp/portage/app-emulation/qemu-2.1.0/work/qemu-2.1.0/qemu-timer.c:314
#3  0x00007f3d53475970 in aio_poll (ctx=ctx@entry=0x7f3d54270c00, 
blocking=blocking@entry=true)
     at 
/var/tmp/portage/app-emulation/qemu-2.1.0/work/qemu-2.1.0/aio-posix.c:250
#4  0x00007f3d534695e7 in bdrv_drain_all () at 
/var/tmp/portage/app-emulation/qemu-2.1.0/work/qemu-2.1.0/block.c:1924
#5  0x00007f3d5346fe1f in bdrv_close (bs=bs@entry=0x7f3d5579b340) at 
/var/tmp/portage/app-emulation/qemu-2.1.0/work/qemu-2.1.0/block.c:1820
#6  0x00007f3d53470047 in bdrv_delete (bs=0x7f3d5579b340) at 
/var/tmp/portage/app-emulation/qemu-2.1.0/work/qemu-2.1.0/block.c:2094
#7  bdrv_unref (bs=0x7f3d5579b340) at 
/var/tmp/portage/app-emulation/qemu-2.1.0/work/qemu-2.1.0/block.c:5376
#8  0x00007f3d5347030b in bdrv_drop_intermediate 
(active=active@entry=0x7f3d54635e20, top=top@entry=0x7f3d5579b340, 
base=base@entry=0x7f3d54d956b0,
     backing_file_str=0x7f3d54d95700 
"/mnt/nfs/volumes/7c13c27f-0c48-4676-b075-6e8a3325383e/3785abe6-d2df-49da-9cba-e15cfce8e2af.qcow2")
     at 
/var/tmp/portage/app-emulation/qemu-2.1.0/work/qemu-2.1.0/block.c:2643
#9  0x00007f3d5335121a in commit_run (opaque=0x7f3d545cdac0) at 
/var/tmp/portage/app-emulation/qemu-2.1.0/work/qemu-2.1.0/block/commit.c:145
#10 0x00007f3d5347ebca in coroutine_trampoline (i0=<optimized out>, 
i1=<optimized out>)
     at 
/var/tmp/portage/app-emulation/qemu-2.1.0/work/qemu-2.1.0/coroutine-ucontext.c:118
#11 0x00007f3d4f2f49f0 in ?? () from /lib64/libc.so.6
#12 0x00007fff27d5ef50 in ?? ()
#13 0x0000000000000000 in ?? ()


-- 
mg

Re: [Qemu-devel] [PATCH] linux-aio: avoid deadlock in nested aio_poll() calls

[Marcin Gibuła]

On 05.08.2014 16:26, Marcin Gibuła wrote:
> On 04.08.2014 17:56, Stefan Hajnoczi wrote:
>> If two Linux AIO request completions are fetched in the same
>> io_getevents() call, QEMU will deadlock if request A's callback waits
>> for request B to complete using an aio_poll() loop.  This was reported
>> to happen with the mirror blockjob.
>>
>> This patch moves completion processing into a BH and makes it resumable.
>> Nested event loops can resume completion processing so that request B
>> will complete and the deadlock will not occur.
>>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Ming Lei <ming.lei@canonical.com>
>> Cc: Marcin Gibuła <m.gibula@beyond.pl>
>> Reported-by: Marcin Gibuła <m.gibula@beyond.pl>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> Still hangs...

I'm sorry, ignore this comment.

I've built my test qemu without aio support. Retesting now.

-- 
mg

Re: [Qemu-devel] [PATCH] linux-aio: avoid deadlock in nested aio_poll() calls

[Marcin Gibuła]

W dniu 2014-08-04 17:56, Stefan Hajnoczi pisze:
> If two Linux AIO request completions are fetched in the same
> io_getevents() call, QEMU will deadlock if request A's callback waits
> for request B to complete using an aio_poll() loop.  This was reported
> to happen with the mirror blockjob.
>
> This patch moves completion processing into a BH and makes it resumable.
> Nested event loops can resume completion processing so that request B
> will complete and the deadlock will not occur.
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Marcin Gibuła <m.gibula@beyond.pl>
> Reported-by: Marcin Gibuła <m.gibula@beyond.pl>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

This patch fixes the block-commit hang when using linux-aio, so:

Tested-by: Marcin Gibuła <m.gibula@beyond.pl>

-- 
mg

Re: [Qemu-devel] [PATCH] linux-aio: avoid deadlock in nested aio_poll() calls

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

On Mon, Aug 04, 2014 at 04:56:33PM +0100, Stefan Hajnoczi wrote:
> If two Linux AIO request completions are fetched in the same
> io_getevents() call, QEMU will deadlock if request A's callback waits
> for request B to complete using an aio_poll() loop.  This was reported
> to happen with the mirror blockjob.
> 
> This patch moves completion processing into a BH and makes it resumable.
> Nested event loops can resume completion processing so that request B
> will complete and the deadlock will not occur.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Marcin Gibuła <m.gibula@beyond.pl>
> Reported-by: Marcin Gibuła <m.gibula@beyond.pl>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/linux-aio.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 55 insertions(+), 16 deletions(-)

Applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]