From bcd82a968fbf7d8156eefbae3f3aab59ad576fa2 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 22 Apr 2016 21:53:52 +0800 Subject: iohandler: Introduce iohandler_get_aio_context Signed-off-by: Fam Zheng Reviewed-by: Michael S. Tsirkin Signed-off-by: Kevin Wolf --- include/qemu/main-loop.h | 1 + iohandler.c | 6 ++++++ stubs/Makefile.objs | 1 + stubs/iohandler.c | 8 ++++++++ 4 files changed, 16 insertions(+) create mode 100644 stubs/iohandler.c diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h index 99769093fc..19b5de3dd5 100644 --- a/include/qemu/main-loop.h +++ b/include/qemu/main-loop.h @@ -204,6 +204,7 @@ void qemu_set_fd_handler(int fd, void *opaque); GSource *iohandler_get_g_source(void); +AioContext *iohandler_get_aio_context(void); #ifdef CONFIG_POSIX /** * qemu_add_child_watch: Register a child process for reaping. diff --git a/iohandler.c b/iohandler.c index 3f23433b5a..f2fc8a9bd6 100644 --- a/iohandler.c +++ b/iohandler.c @@ -44,6 +44,12 @@ static void iohandler_init(void) } } +AioContext *iohandler_get_aio_context(void) +{ + iohandler_init(); + return iohandler_ctx; +} + GSource *iohandler_get_g_source(void) { iohandler_init(); diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index b6d1e650db..4b258a6731 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -40,3 +40,4 @@ stub-obj-y += qmp_pc_dimm_device_list.o stub-obj-y += target-monitor-defs.o stub-obj-y += target-get-monitor-def.o stub-obj-y += vhost.o +stub-obj-y += iohandler.o diff --git a/stubs/iohandler.c b/stubs/iohandler.c new file mode 100644 index 0000000000..22b0ee5b0a --- /dev/null +++ b/stubs/iohandler.c @@ -0,0 +1,8 @@ +#include "qemu/osdep.h" +#include "qemu-common.h" +#include "qemu/main-loop.h" + +AioContext *iohandler_get_aio_context(void) +{ + abort(); +} -- cgit v1.2.1 From 54e18d35e44c48cf6e13c4ce09962c30b595b72a Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 22 Apr 2016 21:53:53 +0800 Subject: event-notifier: Add "is_external" parameter All callers pass "false" keeping the old semantics. The windows implementation doesn't distinguish the flag yet. On posix, it is passed down to the underlying aio context. Signed-off-by: Fam Zheng Reviewed-by: Michael S. Tsirkin Signed-off-by: Kevin Wolf --- hw/usb/ccid-card-emulated.c | 2 +- hw/virtio/virtio.c | 8 ++++---- include/qemu/event_notifier.h | 4 +++- stubs/set-fd-handler.c | 10 ++++++++++ target-i386/hyperv.c | 6 +++--- util/event_notifier-posix.c | 4 +++- util/event_notifier-win32.c | 1 + 7 files changed, 25 insertions(+), 10 deletions(-) diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c index 9ddd5ad75b..3213f9f8af 100644 --- a/hw/usb/ccid-card-emulated.c +++ b/hw/usb/ccid-card-emulated.c @@ -407,7 +407,7 @@ static int init_event_notifier(EmulatedState *card) DPRINTF(card, 2, "event notifier creation failed\n"); return -1; } - event_notifier_set_handler(&card->notifier, card_event_handler); + event_notifier_set_handler(&card->notifier, false, card_event_handler); return 0; } diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index f745c4abd0..fffa09f46c 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1775,10 +1775,10 @@ void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign, bool with_irqfd) { if (assign && !with_irqfd) { - event_notifier_set_handler(&vq->guest_notifier, + event_notifier_set_handler(&vq->guest_notifier, false, virtio_queue_guest_notifier_read); } else { - event_notifier_set_handler(&vq->guest_notifier, NULL); + event_notifier_set_handler(&vq->guest_notifier, false, NULL); } if (!assign) { /* Test and clear notifier before closing it, @@ -1829,10 +1829,10 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign, bool set_handler) { if (assign && set_handler) { - event_notifier_set_handler(&vq->host_notifier, + event_notifier_set_handler(&vq->host_notifier, false, virtio_queue_host_notifier_read); } else { - event_notifier_set_handler(&vq->host_notifier, NULL); + event_notifier_set_handler(&vq->host_notifier, false, NULL); } if (!assign) { /* Test and clear notifier before after disabling event, diff --git a/include/qemu/event_notifier.h b/include/qemu/event_notifier.h index a8f28540c9..e326990db4 100644 --- a/include/qemu/event_notifier.h +++ b/include/qemu/event_notifier.h @@ -34,7 +34,9 @@ int event_notifier_init(EventNotifier *, int active); void event_notifier_cleanup(EventNotifier *); int event_notifier_set(EventNotifier *); int event_notifier_test_and_clear(EventNotifier *); -int event_notifier_set_handler(EventNotifier *, EventNotifierHandler *); +int event_notifier_set_handler(EventNotifier *, + bool is_external, + EventNotifierHandler *); #ifdef CONFIG_POSIX void event_notifier_init_fd(EventNotifier *, int fd); diff --git a/stubs/set-fd-handler.c b/stubs/set-fd-handler.c index 26965de4c3..06a5da48f1 100644 --- a/stubs/set-fd-handler.c +++ b/stubs/set-fd-handler.c @@ -9,3 +9,13 @@ void qemu_set_fd_handler(int fd, { abort(); } + +void aio_set_fd_handler(AioContext *ctx, + int fd, + bool is_external, + IOHandler *io_read, + IOHandler *io_write, + void *opaque) +{ + abort(); +} diff --git a/target-i386/hyperv.c b/target-i386/hyperv.c index c4d6a9b2b1..39a230f119 100644 --- a/target-i386/hyperv.c +++ b/target-i386/hyperv.c @@ -88,7 +88,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint, goto err_sint_set_notifier; } - event_notifier_set_handler(&sint_route->sint_ack_notifier, + event_notifier_set_handler(&sint_route->sint_ack_notifier, false, kvm_hv_sint_ack_handler); gsi = kvm_irqchip_add_hv_sint_route(kvm_state, vcpu_id, sint); @@ -112,7 +112,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint, err_irqfd: kvm_irqchip_release_virq(kvm_state, gsi); err_gsi: - event_notifier_set_handler(&sint_route->sint_ack_notifier, NULL); + event_notifier_set_handler(&sint_route->sint_ack_notifier, false, NULL); event_notifier_cleanup(&sint_route->sint_ack_notifier); err_sint_set_notifier: event_notifier_cleanup(&sint_route->sint_set_notifier); @@ -128,7 +128,7 @@ void kvm_hv_sint_route_destroy(HvSintRoute *sint_route) &sint_route->sint_set_notifier, sint_route->gsi); kvm_irqchip_release_virq(kvm_state, sint_route->gsi); - event_notifier_set_handler(&sint_route->sint_ack_notifier, NULL); + event_notifier_set_handler(&sint_route->sint_ack_notifier, false, NULL); event_notifier_cleanup(&sint_route->sint_ack_notifier); event_notifier_cleanup(&sint_route->sint_set_notifier); g_free(sint_route); diff --git a/util/event_notifier-posix.c b/util/event_notifier-posix.c index e150301c33..c1f0d79b34 100644 --- a/util/event_notifier-posix.c +++ b/util/event_notifier-posix.c @@ -91,9 +91,11 @@ int event_notifier_get_fd(const EventNotifier *e) } int event_notifier_set_handler(EventNotifier *e, + bool is_external, EventNotifierHandler *handler) { - qemu_set_fd_handler(e->rfd, (IOHandler *)handler, NULL, e); + aio_set_fd_handler(iohandler_get_aio_context(), e->rfd, is_external, + (IOHandler *)handler, NULL, e); return 0; } diff --git a/util/event_notifier-win32.c b/util/event_notifier-win32.c index 14b4f7d2c0..de87df02d6 100644 --- a/util/event_notifier-win32.c +++ b/util/event_notifier-win32.c @@ -33,6 +33,7 @@ HANDLE event_notifier_get_handle(EventNotifier *e) } int event_notifier_set_handler(EventNotifier *e, + bool is_external, EventNotifierHandler *handler) { if (handler) { -- cgit v1.2.1 From 14560d69e7c979d97975c3aa6e7bd1ab3249fe88 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 22 Apr 2016 21:53:54 +0800 Subject: virtio: Mark host notifiers as external The effect of this change is the block layer drained section can work, for example when mirror job is being completed. Signed-off-by: Fam Zheng Reviewed-by: Michael S. Tsirkin Signed-off-by: Kevin Wolf --- hw/virtio/virtio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index fffa09f46c..30ede3d1cc 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1829,10 +1829,10 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign, bool set_handler) { if (assign && set_handler) { - event_notifier_set_handler(&vq->host_notifier, false, + event_notifier_set_handler(&vq->host_notifier, true, virtio_queue_host_notifier_read); } else { - event_notifier_set_handler(&vq->host_notifier, false, NULL); + event_notifier_set_handler(&vq->host_notifier, true, NULL); } if (!assign) { /* Test and clear notifier before after disabling event, -- cgit v1.2.1 From 37989ced44e559dbb1edb8b238ffe221f70214b4 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 22 Apr 2016 21:53:55 +0800 Subject: aio-posix: Skip external nodes in aio_dispatch aio_poll doesn't poll the external nodes so this should never be true, but aio_ctx_dispatch may get notified by the events from GSource. To make bdrv_drained_begin effective in main loop, we should check the is_external flag here too. Also do the check in aio_pending so aio_dispatch is not called superfluously, when there is no events other than external ones. Signed-off-by: Fam Zheng Reviewed-by: Jeff Cody Signed-off-by: Kevin Wolf --- aio-posix.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/aio-posix.c b/aio-posix.c index 7fd565fbde..6006122e0b 100644 --- a/aio-posix.c +++ b/aio-posix.c @@ -282,10 +282,12 @@ bool aio_pending(AioContext *ctx) int revents; revents = node->pfd.revents & node->pfd.events; - if (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR) && node->io_read) { + if (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR) && node->io_read && + aio_node_check(ctx, node->is_external)) { return true; } - if (revents & (G_IO_OUT | G_IO_ERR) && node->io_write) { + if (revents & (G_IO_OUT | G_IO_ERR) && node->io_write && + aio_node_check(ctx, node->is_external)) { return true; } } @@ -323,6 +325,7 @@ bool aio_dispatch(AioContext *ctx) if (!node->deleted && (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR)) && + aio_node_check(ctx, node->is_external) && node->io_read) { node->io_read(node->opaque); @@ -333,6 +336,7 @@ bool aio_dispatch(AioContext *ctx) } if (!node->deleted && (revents & (G_IO_OUT | G_IO_ERR)) && + aio_node_check(ctx, node->is_external) && node->io_write) { node->io_write(node->opaque); progress = true; -- cgit v1.2.1 From ab27c3b5e7408693dde0b565f050aa55c4a1bcef Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 22 Apr 2016 21:53:56 +0800 Subject: mirror: Workaround for unexpected iohandler events during completion Commit 5a7e7a0ba moved mirror_exit to a BH handler but didn't add any protection against new requests that could sneak in just before the BH is dispatched. For example (assuming a code base at that commit): main_loop_wait # 1 os_host_main_loop_wait g_main_context_dispatch aio_ctx_dispatch aio_dispatch ... mirror_run bdrv_drain (a) block_job_defer_to_main_loop qemu_iohandler_poll virtio_queue_host_notifier_read ... virtio_submit_multiwrite (b) blk_aio_multiwrite main_loop_wait # 2 aio_dispatch aio_bh_poll (c) mirror_exit At (a) we know the BDS has no pending request. However, the same main_loop_wait call is going to dispatch iohandlers (EventNotifier events), which may lead to a new I/O from guest. So the invariant is already broken at (c). Data loss. Commit f3926945c8 made iohandler to use aio API. The order of virtio_queue_host_notifier_read and block_job_defer_to_main_loop within a main_loop_wait becomes unpredictable, and even worse, if the host notifier event arrives at the next main_loop_wait call, the unpredictable order between mirror_exit and virtio_queue_host_notifier_read is also a trouble. As shown below, this commit made the bug easier to trigger: - Bug case 1: main_loop_wait # 1 os_host_main_loop_wait g_main_context_dispatch aio_ctx_dispatch (qemu_aio_context) ... mirror_run bdrv_drain (a) block_job_defer_to_main_loop aio_ctx_dispatch (iohandler_ctx) virtio_queue_host_notifier_read ... virtio_submit_multiwrite (b) blk_aio_multiwrite main_loop_wait # 2 ... aio_dispatch aio_bh_poll (c) mirror_exit - Bug case 2: main_loop_wait # 1 os_host_main_loop_wait g_main_context_dispatch aio_ctx_dispatch (qemu_aio_context) ... mirror_run bdrv_drain (a) block_job_defer_to_main_loop main_loop_wait # 2 ... aio_ctx_dispatch (iohandler_ctx) virtio_queue_host_notifier_read ... virtio_submit_multiwrite (b) blk_aio_multiwrite aio_dispatch aio_bh_poll (c) mirror_exit In both cases, (b) breaks the invariant wanted by (a) and (c). Until then, the request loss has been silent. Later, 3f09bfbc7be added asserts at (c) to check the invariant (in bdrv_replace_in_backing_chain), and Max reported an assertion failure first visible there, by doing active committing while the guest is running bonnie++. 2.5 added bdrv_drained_begin at (a) to protect the dataplane case from similar problems, but we never realize the main loop bug until now. As a bandage, this patch disables iohandler's external events temporarily together with bs->ctx. Launchpad Bug: 1570134 Cc: qemu-stable@nongnu.org Signed-off-by: Fam Zheng Reviewed-by: Jeff Cody Signed-off-by: Kevin Wolf --- block/mirror.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/block/mirror.c b/block/mirror.c index d56e30e472..039f48125e 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -495,6 +495,9 @@ out: block_job_completed(&s->common, data->ret); g_free(data); bdrv_drained_end(src); + if (qemu_get_aio_context() == bdrv_get_aio_context(src)) { + aio_enable_external(iohandler_get_aio_context()); + } bdrv_unref(src); } @@ -716,6 +719,12 @@ immediate_exit: /* Before we switch to target in mirror_exit, make sure data doesn't * change. */ bdrv_drained_begin(s->common.bs); + if (qemu_get_aio_context() == bdrv_get_aio_context(bs)) { + /* FIXME: virtio host notifiers run on iohandler_ctx, therefore the + * above bdrv_drained_end isn't enough to quiesce it. This is ugly, we + * need a block layer API change to achieve this. */ + aio_disable_external(iohandler_get_aio_context()); + } block_job_defer_to_main_loop(&s->common, mirror_exit, data); } -- cgit v1.2.1