From fed105e2756dde98efa5e80baca02ae516dd1e51 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 28 Jul 2015 18:34:07 +0200 Subject: [PATCH 1/3] virtio-blk-dataplane: delete bottom half before the AioContext is freed 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 Signed-off-by: Paolo Bonzini Message-id: 1438101249-25166-2-git-send-email-pbonzini@redhat.com Message-Id: <1438086628-13000-1-git-send-email-pbonzini@redhat.com> Signed-off-by: Stefan Hajnoczi --- 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 3db139b8a4ab..6106e46158e9 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); } From a076972a4d36381d610a854f0c336507650a1d34 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 28 Jul 2015 18:34:08 +0200 Subject: [PATCH 2/3] AioContext: avoid leaking BHs on cleanup 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 Signed-off-by: Stefan Hajnoczi Signed-off-by: Paolo Bonzini Message-id: 1438101249-25166-3-git-send-email-pbonzini@redhat.com Message-Id: <1438014819-18125-2-git-send-email-stefanha@redhat.com> Signed-off-by: Paolo Bonzini Signed-off-by: Stefan Hajnoczi --- async.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/async.c b/async.c index 9a98a74acb99..8d23681526ce 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); From ca96ac44dcd290566090b2435bc828fded356ad9 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 28 Jul 2015 18:34:09 +0200 Subject: [PATCH 3/3] AioContext: force event loop iteration using BH 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 for investigating this bug with me. Cc: Christian Borntraeger Cc: Cornelia Huck Cc: Paolo Bonzini Signed-off-by: Stefan Hajnoczi Signed-off-by: Paolo Bonzini Message-id: 1438101249-25166-4-git-send-email-pbonzini@redhat.com Message-Id: <1438014819-18125-3-git-send-email-stefanha@redhat.com> Signed-off-by: Paolo Bonzini Signed-off-by: Stefan Hajnoczi --- async.c | 16 ++++++++++++++-- include/block/aio.h | 3 +++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/async.c b/async.c index 8d23681526ce..efce14b63a84 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 9dd32e0f137c..400b1b002082 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;