[Qemu-devel] [PATCH v2 for-2.6 0/5] block: Fix assertion failure at mirror exit

[Qemu-devel] [PATCH v2 for-2.6 0/5] block: Fix assertion failure at mirror exit

[Fam Zheng]

v2: Go along the is_external appraoch. [Kevin]

I tested this series survives the "snapshot + commit" loop reproducer with both
bonnie++ and RHEL installation in the guest.

This supersedes the "virtio: Register host notifier handler as external" patch
from yesterday.

The bug was initially reported by Matthew Schumacher as LaunchPad Bug 1570134,
and nicely bisected by Max Reitz. See patch 2 for the analysis.


Fam Zheng (5):
  iohandler: Introduce iohandler_get_aio_context
  event-notifier: Add "is_external" parameter
  virtio: Mark host notifiers as external
  aio-posix: Skip external nodes in aio_dispatch
  mirror: Workaround for unexpected iohandler events during completion

 aio-posix.c                   |  2 ++
 block/mirror.c                |  9 +++++++++
 hw/usb/ccid-card-emulated.c   |  2 +-
 hw/virtio/virtio.c            |  8 ++++----
 include/qemu/event_notifier.h |  4 +++-
 include/qemu/main-loop.h      |  1 +
 iohandler.c                   |  6 ++++++
 stubs/Makefile.objs           |  1 +
 stubs/iohandler.c             |  8 ++++++++
 stubs/set-fd-handler.c        | 10 ++++++++++
 target-i386/hyperv.c          |  6 +++---
 util/event_notifier-posix.c   |  4 +++-
 util/event_notifier-win32.c   |  1 +
 13 files changed, 52 insertions(+), 10 deletions(-)
 create mode 100644 stubs/iohandler.c

-- 
2.8.0

[Qemu-devel] [PATCH v2 for-2.6 1/5] iohandler: Introduce iohandler_get_aio_context

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 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 9976909..19b5de3 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 3f23433..f2fc8a9 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 b6d1e65..4b258a6 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 0000000..22b0ee5
--- /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();
+}
-- 
2.8.0

Re: [Qemu-devel] [PATCH v2 for-2.6 1/5] iohandler: Introduce iohandler_get_aio_context

[Michael S. Tsirkin]

On Fri, Apr 22, 2016 at 06:56:00PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  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 9976909..19b5de3 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 3f23433..f2fc8a9 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 b6d1e65..4b258a6 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 0000000..22b0ee5
> --- /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();
> +}
> -- 
> 2.8.0

[Qemu-devel] [PATCH v2 for-2.6 2/5] event-notifier: Add "is_external" parameter

[Fam Zheng]

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 <famz@redhat.com>
---
 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 9ddd5ad..3213f9f 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 f745c4a..fffa09f 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 a8f2854..e326990 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 26965de..06a5da4 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 c4d6a9b..39a230f 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 e150301..c1f0d79 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 14b4f7d..de87df0 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) {
-- 
2.8.0

Re: [Qemu-devel] [PATCH v2 for-2.6 2/5] event-notifier: Add "is_external" parameter

[Michael S. Tsirkin]

On Fri, Apr 22, 2016 at 06:56:01PM +0800, Fam Zheng wrote:
> 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 <famz@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  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 9ddd5ad..3213f9f 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 f745c4a..fffa09f 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 a8f2854..e326990 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 26965de..06a5da4 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 c4d6a9b..39a230f 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 e150301..c1f0d79 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 14b4f7d..de87df0 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) {
> -- 
> 2.8.0

[Qemu-devel] [PATCH v2 for-2.6 3/5] virtio: Mark host notifiers as external

[Fam Zheng]

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 <famz@redhat.com>
---
 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 fffa09f..30ede3d 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,
-- 
2.8.0

Re: [Qemu-devel] [PATCH v2 for-2.6 3/5] virtio: Mark host notifiers as external

[Michael S. Tsirkin]

On Fri, Apr 22, 2016 at 06:56:02PM +0800, Fam Zheng wrote:
> 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 <famz@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> ---
>  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 fffa09f..30ede3d 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,
> -- 
> 2.8.0

[Qemu-devel] [PATCH v2 for-2.6 4/5] aio-posix: Skip external nodes in aio_dispatch

[Fam Zheng]

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.

This could result in a few busy polls because the fd is left unhandled,
but the drained section is only transient and shouldn't be longer than
one or two event loop iterations.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 aio-posix.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/aio-posix.c b/aio-posix.c
index 7fd565f..a7c9304 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -323,6 +323,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 +334,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;
-- 
2.8.0

Re: [Qemu-devel] [PATCH v2 for-2.6 4/5] aio-posix: Skip external nodes in aio_dispatch

[Kevin Wolf]

Am 22.04.2016 um 12:56 hat Fam Zheng geschrieben:
> 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.
> 
> This could result in a few busy polls because the fd is left unhandled,
> but the drained section is only transient and shouldn't be longer than
> one or two event loop iterations.

Are the busy polls because aio_ctx_check() calls aio_pending(), which
still returns true, even if only disabled AioHandlers are ready? If so,
should we just add the is_external check to aio_pending(), too?

Kevin

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  aio-posix.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/aio-posix.c b/aio-posix.c
> index 7fd565f..a7c9304 100644
> --- a/aio-posix.c
> +++ b/aio-posix.c
> @@ -323,6 +323,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 +334,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;
> -- 
> 2.8.0
> 

Re: [Qemu-devel] [PATCH v2 for-2.6 4/5] aio-posix: Skip external nodes in aio_dispatch

[Fam Zheng]

On Fri, 04/22 13:38, Kevin Wolf wrote:
> Am 22.04.2016 um 12:56 hat Fam Zheng geschrieben:
> > 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.
> > 
> > This could result in a few busy polls because the fd is left unhandled,
> > but the drained section is only transient and shouldn't be longer than
> > one or two event loop iterations.
> 
> Are the busy polls because aio_ctx_check() calls aio_pending(), which
> still returns true, even if only disabled AioHandlers are ready? If so,
> should we just add the is_external check to aio_pending(), too?

Good point, that is certainly better! Will add that.

Fam

> 
> Kevin
> 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  aio-posix.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/aio-posix.c b/aio-posix.c
> > index 7fd565f..a7c9304 100644
> > --- a/aio-posix.c
> > +++ b/aio-posix.c
> > @@ -323,6 +323,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 +334,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;
> > -- 
> > 2.8.0
> > 

[Qemu-devel] [PATCH v2 for-2.6 5/5] mirror: Workaround for unexpected iohandler events during completion

[Fam Zheng]

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
          <snip>
                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 <famz@redhat.com>
---
 block/mirror.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index d56e30e..039f481 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);
 }
 
-- 
2.8.0

Re: [Qemu-devel] [PATCH v2 for-2.6 0/5] block: Fix assertion failure at mirror exit

[Michael S. Tsirkin]

On Fri, Apr 22, 2016 at 06:55:59PM +0800, Fam Zheng wrote:
> v2: Go along the is_external appraoch. [Kevin]
> 
> I tested this series survives the "snapshot + commit" loop reproducer with both
> bonnie++ and RHEL installation in the guest.
> 
> This supersedes the "virtio: Register host notifier handler as external" patch
> from yesterday.
> 
> The bug was initially reported by Matthew Schumacher as LaunchPad Bug 1570134,
> and nicely bisected by Max Reitz. See patch 2 for the analysis.

I reviewed patches 1-3 now.
I could merge them but I'd rather not merge patches 4-5 myself though.
Is there a reason why patches 4-5 are in the same series?
If not, pls submit them separately so block maintainers can merge them.
Alternative have one of the block maintainers merge 1-3,
I sent reviewed-by tags in that case.

> 
> Fam Zheng (5):
>   iohandler: Introduce iohandler_get_aio_context
>   event-notifier: Add "is_external" parameter
>   virtio: Mark host notifiers as external
>   aio-posix: Skip external nodes in aio_dispatch
>   mirror: Workaround for unexpected iohandler events during completion
> 
>  aio-posix.c                   |  2 ++
>  block/mirror.c                |  9 +++++++++
>  hw/usb/ccid-card-emulated.c   |  2 +-
>  hw/virtio/virtio.c            |  8 ++++----
>  include/qemu/event_notifier.h |  4 +++-
>  include/qemu/main-loop.h      |  1 +
>  iohandler.c                   |  6 ++++++
>  stubs/Makefile.objs           |  1 +
>  stubs/iohandler.c             |  8 ++++++++
>  stubs/set-fd-handler.c        | 10 ++++++++++
>  target-i386/hyperv.c          |  6 +++---
>  util/event_notifier-posix.c   |  4 +++-
>  util/event_notifier-win32.c   |  1 +
>  13 files changed, 52 insertions(+), 10 deletions(-)
>  create mode 100644 stubs/iohandler.c
> 
> -- 
> 2.8.0

Re: [Qemu-devel] [PATCH v2 for-2.6 0/5] block: Fix assertion failure at mirror exit

[Fam Zheng]

On Fri, 04/22 16:49, Michael S. Tsirkin wrote:
> On Fri, Apr 22, 2016 at 06:55:59PM +0800, Fam Zheng wrote:
> > v2: Go along the is_external appraoch. [Kevin]
> > 
> > I tested this series survives the "snapshot + commit" loop reproducer with both
> > bonnie++ and RHEL installation in the guest.
> > 
> > This supersedes the "virtio: Register host notifier handler as external" patch
> > from yesterday.
> > 
> > The bug was initially reported by Matthew Schumacher as LaunchPad Bug 1570134,
> > and nicely bisected by Max Reitz. See patch 2 for the analysis.
> 
> I reviewed patches 1-3 now.
> I could merge them but I'd rather not merge patches 4-5 myself though.
> Is there a reason why patches 4-5 are in the same series?
> If not, pls submit them separately so block maintainers can merge them.
> Alternative have one of the block maintainers merge 1-3,
> I sent reviewed-by tags in that case.

I've just sent v3 with patch 4 updated as Kevin suggested.  Sorry for not
having noticed your reply before hitting the send button, but the first 3
patches are unchanged.  Perhaps Kevin can pick up your reviewed-by tags and
apply all the series if he is happy with v3.

Thanks for reviewing!

Fam

> 
> > 
> > Fam Zheng (5):
> >   iohandler: Introduce iohandler_get_aio_context
> >   event-notifier: Add "is_external" parameter
> >   virtio: Mark host notifiers as external
> >   aio-posix: Skip external nodes in aio_dispatch
> >   mirror: Workaround for unexpected iohandler events during completion
> > 
> >  aio-posix.c                   |  2 ++
> >  block/mirror.c                |  9 +++++++++
> >  hw/usb/ccid-card-emulated.c   |  2 +-
> >  hw/virtio/virtio.c            |  8 ++++----
> >  include/qemu/event_notifier.h |  4 +++-
> >  include/qemu/main-loop.h      |  1 +
> >  iohandler.c                   |  6 ++++++
> >  stubs/Makefile.objs           |  1 +
> >  stubs/iohandler.c             |  8 ++++++++
> >  stubs/set-fd-handler.c        | 10 ++++++++++
> >  target-i386/hyperv.c          |  6 +++---
> >  util/event_notifier-posix.c   |  4 +++-
> >  util/event_notifier-win32.c   |  1 +
> >  13 files changed, 52 insertions(+), 10 deletions(-)
> >  create mode 100644 stubs/iohandler.c
> > 
> > -- 
> > 2.8.0

Re: [Qemu-devel] [PATCH v2 for-2.6 0/5] block: Fix assertion failure at mirror exit

[Kevin Wolf]

Am 22.04.2016 um 15:49 hat Michael S. Tsirkin geschrieben:
> On Fri, Apr 22, 2016 at 06:55:59PM +0800, Fam Zheng wrote:
> > v2: Go along the is_external appraoch. [Kevin]
> > 
> > I tested this series survives the "snapshot + commit" loop reproducer with both
> > bonnie++ and RHEL installation in the guest.
> > 
> > This supersedes the "virtio: Register host notifier handler as external" patch
> > from yesterday.
> > 
> > The bug was initially reported by Matthew Schumacher as LaunchPad Bug 1570134,
> > and nicely bisected by Max Reitz. See patch 2 for the analysis.
> 
> I reviewed patches 1-3 now.
> I could merge them but I'd rather not merge patches 4-5 myself though.
> Is there a reason why patches 4-5 are in the same series?

The fix doesn't work without them because the is_external handling in
the main loop is incomplete (i.e. even after aio_disable_external() it
would still process the AioHandlers sometimes)

> If not, pls submit them separately so block maintainers can merge them.
> Alternative have one of the block maintainers merge 1-3,
> I sent reviewed-by tags in that case.

I think it makes more sense to merge them in a single batch. I can
either take v3 through my tree as soon as Fam posts it or Peter can
apply the series directly.

Kevin