[Qemu-commits] [qemu/qemu] bcd82a: iohandler: Introduce iohandler_get_aio_context

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: bcd82a968fbf7d8156eefbae3f3aab59ad576fa2
      
https://github.com/qemu/qemu/commit/bcd82a968fbf7d8156eefbae3f3aab59ad576fa2
  Author: Fam Zheng <address@hidden>
  Date:   2016-04-22 (Fri, 22 Apr 2016)

  Changed paths:
    M include/qemu/main-loop.h
    M iohandler.c
    M stubs/Makefile.objs
    A stubs/iohandler.c

  Log Message:
  -----------
  iohandler: Introduce iohandler_get_aio_context

Signed-off-by: Fam Zheng <address@hidden>
Reviewed-by: Michael S. Tsirkin <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 54e18d35e44c48cf6e13c4ce09962c30b595b72a
      
https://github.com/qemu/qemu/commit/54e18d35e44c48cf6e13c4ce09962c30b595b72a
  Author: Fam Zheng <address@hidden>
  Date:   2016-04-22 (Fri, 22 Apr 2016)

  Changed paths:
    M hw/usb/ccid-card-emulated.c
    M hw/virtio/virtio.c
    M include/qemu/event_notifier.h
    M stubs/set-fd-handler.c
    M target-i386/hyperv.c
    M util/event_notifier-posix.c
    M util/event_notifier-win32.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Michael S. Tsirkin <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 14560d69e7c979d97975c3aa6e7bd1ab3249fe88
      
https://github.com/qemu/qemu/commit/14560d69e7c979d97975c3aa6e7bd1ab3249fe88
  Author: Fam Zheng <address@hidden>
  Date:   2016-04-22 (Fri, 22 Apr 2016)

  Changed paths:
    M hw/virtio/virtio.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Michael S. Tsirkin <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 37989ced44e559dbb1edb8b238ffe221f70214b4
      
https://github.com/qemu/qemu/commit/37989ced44e559dbb1edb8b238ffe221f70214b4
  Author: Fam Zheng <address@hidden>
  Date:   2016-04-22 (Fri, 22 Apr 2016)

  Changed paths:
    M aio-posix.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: ab27c3b5e7408693dde0b565f050aa55c4a1bcef
      
https://github.com/qemu/qemu/commit/ab27c3b5e7408693dde0b565f050aa55c4a1bcef
  Author: Fam Zheng <address@hidden>
  Date:   2016-04-22 (Fri, 22 Apr 2016)

  Changed paths:
    M block/mirror.c

  Log Message:
  -----------
  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
    <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: address@hidden
Signed-off-by: Fam Zheng <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 53343338a6e7b83777b82803398572b40afc8c0f
      
https://github.com/qemu/qemu/commit/53343338a6e7b83777b82803398572b40afc8c0f
  Author: Peter Maydell <address@hidden>
  Date:   2016-04-22 (Fri, 22 Apr 2016)

  Changed paths:
    M aio-posix.c
    M block/mirror.c
    M hw/usb/ccid-card-emulated.c
    M hw/virtio/virtio.c
    M include/qemu/event_notifier.h
    M include/qemu/main-loop.h
    M iohandler.c
    M stubs/Makefile.objs
    A stubs/iohandler.c
    M stubs/set-fd-handler.c
    M target-i386/hyperv.c
    M util/event_notifier-posix.c
    M util/event_notifier-win32.c

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging

Mirror block job fixes for 2.6.0-rc4

# gpg: Signature made Fri 22 Apr 2016 15:46:41 BST using RSA key ID C88F2FD6
# gpg: Good signature from "Kevin Wolf <address@hidden>"

* remotes/kevin/tags/for-upstream:
  mirror: Workaround for unexpected iohandler events during completion
  aio-posix: Skip external nodes in aio_dispatch
  virtio: Mark host notifiers as external
  event-notifier: Add "is_external" parameter
  iohandler: Introduce iohandler_get_aio_context

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/ee1e0f8e5d36...53343338a6e7