[Qemu-devel] [PATCH for-2.4 v3 0/3] AioContext: fix deadlock after aio_context_acquire() race

[Qemu-devel] [PATCH for-2.4 v3 0/3] AioContext: fix deadlock after aio_context_acquire() race

[Paolo Bonzini]

v3: same as v1, but include virtio-blk-dataplane fix and move thread_pool_free
before the loop.  Same result as applying in order:

[PATCH for-2.4 1/2] AioContext: avoid leaking BHs on cleanup
[PATCH for-2.4 2/2] AioContext: force event loop iteration using BH
[PATCH for-2.4] block: delete bottom halves before the AioContext is freed

but bisectable.

Paolo Bonzini (1):
  virtio-blk-dataplane: delete bottom half before the AioContext is freed

Stefan Hajnoczi (2):
  AioContext: avoid leaking BHs on cleanup
  AioContext: force event loop iteration using BH

 async.c                         | 29 +++++++++++++++++++++++++++--
 hw/block/dataplane/virtio-blk.c |  2 +-
 include/block/aio.h             |  3 +++
 3 files changed, 31 insertions(+), 3 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 v3 1/3] virtio-blk-dataplane: delete bottom half before the AioContext is freed

[Paolo Bonzini]

Other uses of aio_bh_new are safe as long as all scheduled bottom
halves are run before an iothread is destroyed, which bdrv_drain will
ensure:

- archipelago_finish_aiocb: BH deletes itself

- inject_error: BH deletes itself

- blkverify_aio_bh: BH deletes itself

- abort_aio_request: BH deletes itself

- curl_aio_readv: BH deletes itself

- gluster_finish_aiocb: BH deletes itself

- bdrv_aio_rw_vector: BH deletes itself

- bdrv_co_maybe_schedule_bh: BH deletes itself

- iscsi_schedule_bh, iscsi_co_generic_cb: BH deletes itself

- laio_attach_aio_context: deleted in laio_detach_aio_context,
called through bdrv_detach_aio_context before deleting the iothread

- nfs_co_generic_cb: BH deletes itself

- null_aio_common: BH deletes itself

- qed_aio_complete: BH deletes itself

- rbd_finish_aiocb: BH deletes itself

- dma_blk_cb: BH deletes itself

- virtio_blk_dma_restart_cb: BH deletes itself

- qemu_bh_new: main loop AioContext is never destroyed

- test-aio.c: bh_delete_cb deletes itself, otherwise deleted in
the same function that calls aio_bh_new

Reported-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <1438086628-13000-1-git-send-email-pbonzini@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 3db139b..6106e46 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -223,8 +223,8 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     virtio_blk_data_plane_stop(s);
     blk_op_unblock_all(s->conf->conf.blk, s->blocker);
     error_free(s->blocker);
-    object_unref(OBJECT(s->iothread));
     qemu_bh_delete(s->bh);
+    object_unref(OBJECT(s->iothread));
     g_free(s);
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 v3 2/3] AioContext: avoid leaking BHs on cleanup

[Paolo Bonzini]

From: Stefan Hajnoczi <stefanha@redhat.com>

BHs are freed during aio_bh_poll().  This leads to memory leaks if there
is no aio_bh_poll() between qemu_bh_delete() and aio_ctx_finalize().

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <1438014819-18125-2-git-send-email-stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v3: place loop after thread_pool_free

 async.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/async.c b/async.c
index 9a98a74..8d23681 100644
--- a/async.c
+++ b/async.c
@@ -231,6 +231,19 @@ aio_ctx_finalize(GSource     *source)
     AioContext *ctx = (AioContext *) source;
 
     thread_pool_free(ctx->thread_pool);
+
+    qemu_mutex_lock(&ctx->bh_lock);
+    while (ctx->first_bh) {
+        QEMUBH *next = ctx->first_bh->next;
+
+        /* qemu_bh_delete() must have been called on BHs in this AioContext */
+        assert(ctx->first_bh->deleted);
+
+        g_free(ctx->first_bh);
+        ctx->first_bh = next;
+    }
+    qemu_mutex_unlock(&ctx->bh_lock);
+
     aio_set_event_notifier(ctx, &ctx->notifier, NULL);
     event_notifier_cleanup(&ctx->notifier);
     rfifolock_destroy(&ctx->lock);
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 v3 3/3] AioContext: force event loop iteration using BH

[Paolo Bonzini]

From: Stefan Hajnoczi <stefanha@redhat.com>

The notify_me optimization introduced in commit eabc97797310
("AioContext: fix broken ctx->dispatching optimization") skips
event_notifier_set() calls when the event loop thread is not blocked in
ppoll(2).

This optimization causes a deadlock if two aio_context_acquire() calls
race.  notify_me = 0 during the race so the winning thread can enter
ppoll(2) unaware that the other thread is waiting its turn to acquire
the AioContext.

This patch forces ppoll(2) to return by scheduling a BH instead of
calling aio_notify().

The following deadlock with virtio-blk dataplane is fixed:

  qemu ... -object iothread,id=iothread0 \
           -drive if=none,id=drive0,file=test.img,... \
           -device virtio-blk-pci,iothread=iothread0,drive=drive0

This command-line results in a hang early on without this patch.

Thanks to Paolo Bonzini <pbonzini@redhat.com> for investigating this bug
with me.

Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <1438014819-18125-3-git-send-email-stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        This will be mostly reverted in 2.5 when the callback goes away.

 async.c             | 16 ++++++++++++++--
 include/block/aio.h |  3 +++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/async.c b/async.c
index 8d23681..efce14b 100644
--- a/async.c
+++ b/async.c
@@ -79,8 +79,10 @@ int aio_bh_poll(AioContext *ctx)
          * aio_notify again if necessary.
          */
         if (!bh->deleted && atomic_xchg(&bh->scheduled, 0)) {
-            if (!bh->idle)
+            /* Idle BHs and the notify BH don't count as progress */
+            if (!bh->idle && bh != ctx->notify_dummy_bh) {
                 ret = 1;
+            }
             bh->idle = 0;
             bh->cb(bh->opaque);
         }
@@ -230,6 +232,7 @@ aio_ctx_finalize(GSource     *source)
 {
     AioContext *ctx = (AioContext *) source;
 
+    qemu_bh_delete(ctx->notify_dummy_bh);
     thread_pool_free(ctx->thread_pool);
 
     qemu_mutex_lock(&ctx->bh_lock);
@@ -298,8 +301,15 @@ static void aio_timerlist_notify(void *opaque)
 
 static void aio_rfifolock_cb(void *opaque)
 {
+    AioContext *ctx = opaque;
+
     /* Kick owner thread in case they are blocked in aio_poll() */
-    aio_notify(opaque);
+    qemu_bh_schedule(ctx->notify_dummy_bh);
+}
+
+static void notify_dummy_bh(void *opaque)
+{
+    /* Do nothing, we were invoked just to force the event loop to iterate */
 }
 
 static void event_notifier_dummy_cb(EventNotifier *e)
@@ -326,6 +336,8 @@ AioContext *aio_context_new(Error **errp)
     rfifolock_init(&ctx->lock, aio_rfifolock_cb, ctx);
     timerlistgroup_init(&ctx->tlg, aio_timerlist_notify, ctx);
 
+    ctx->notify_dummy_bh = aio_bh_new(ctx, notify_dummy_bh, NULL);
+
     return ctx;
 }
 
diff --git a/include/block/aio.h b/include/block/aio.h
index 9dd32e0..400b1b0 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -114,6 +114,9 @@ struct AioContext {
     bool notified;
     EventNotifier notifier;
 
+    /* Scheduling this BH forces the event loop it iterate */
+    QEMUBH *notify_dummy_bh;
+
     /* Thread pool for performing work and receiving completion callbacks */
     struct ThreadPool *thread_pool;
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.4 v3 0/3] AioContext: fix deadlock after aio_context_acquire() race

[Stefan Hajnoczi]

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

On Tue, Jul 28, 2015 at 06:34:06PM +0200, Paolo Bonzini wrote:
> v3: same as v1, but include virtio-blk-dataplane fix and move thread_pool_free
> before the loop.  Same result as applying in order:
> 
> [PATCH for-2.4 1/2] AioContext: avoid leaking BHs on cleanup
> [PATCH for-2.4 2/2] AioContext: force event loop iteration using BH
> [PATCH for-2.4] block: delete bottom halves before the AioContext is freed
> 
> but bisectable.
> 
> Paolo Bonzini (1):
>   virtio-blk-dataplane: delete bottom half before the AioContext is freed
> 
> Stefan Hajnoczi (2):
>   AioContext: avoid leaking BHs on cleanup
>   AioContext: force event loop iteration using BH
> 
>  async.c                         | 29 +++++++++++++++++++++++++++--
>  hw/block/dataplane/virtio-blk.c |  2 +-
>  include/block/aio.h             |  3 +++
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 
> -- 
> 2.4.3
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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

Re: [Qemu-devel] [PATCH for-2.4 v3 0/3] AioContext: fix deadlock after aio_context_acquire() race

[Cornelia Huck]

On Tue, 28 Jul 2015 18:34:06 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> v3: same as v1, but include virtio-blk-dataplane fix and move thread_pool_free
> before the loop.  Same result as applying in order:
> 
> [PATCH for-2.4 1/2] AioContext: avoid leaking BHs on cleanup
> [PATCH for-2.4 2/2] AioContext: force event loop iteration using BH
> [PATCH for-2.4] block: delete bottom halves before the AioContext is freed
> 
> but bisectable.
> 
> Paolo Bonzini (1):
>   virtio-blk-dataplane: delete bottom half before the AioContext is freed
> 
> Stefan Hajnoczi (2):
>   AioContext: avoid leaking BHs on cleanup
>   AioContext: force event loop iteration using BH
> 
>  async.c                         | 29 +++++++++++++++++++++++++++--
>  hw/block/dataplane/virtio-blk.c |  2 +-
>  include/block/aio.h             |  3 +++
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 

Did a quick verification on top of current master; starting,
managedsave and device_del with dataplane devices all seem to work fine.