Series comparison

-[PULL 0/4] Block patches
+[PULL 0/1] Block patches
-The following changes since commit 560009f2c8b57b7cdd31a5693ea86ab369382f49:
+The following changes since commit 887cba855bb6ff4775256f7968409281350b568c:
-  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2019-10-07 15:40:53 +0100)
+  configure: Fix cross-building for RISCV host (v5) (2023-07-11 17:56:09 +0100)
 are available in the Git repository at:
-  https://github.com/stefanha/qemu.git tags/block-pull-request
+  https://gitlab.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to 4d804b5305ffb4d5fa414c38d4f1bdfb987c8d0b:
+for you to fetch changes up to 75dcb4d790bbe5327169fd72b185960ca58e2fa6:
-  iotests/262: Switch source/dest VM launch order (2019-10-08 14:28:25 +0100)
+  virtio-blk: fix host notifier issues during dataplane start/stop (2023-07-12 15:20:32 -0400)
 ----------------------------------------------------------------
 Pull request
-This pull request also contains the two commits from the previous pull request
-that was dropped due to a mingw compilation error.  The compilation should now
-be fixed.
 ----------------------------------------------------------------
-Max Reitz (2):
+Stefan Hajnoczi (1):
-  block: Skip COR for inactive nodes
+  virtio-blk: fix host notifier issues during dataplane start/stop
   iotests/262: Switch source/dest VM launch order
-Sergio Lopez (1):
+ hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++--------------
-  virtio-blk: schedule virtio_notify_config to run on main context
+file changed, 38 insertions(+), 29 deletions(-)
 Vladimir Sementsov-Ogievskiy (1):
   util/ioc.c: try to reassure Coverity about qemu_iovec_init_extended
  block/io.c                 | 41 +++++++++++++++++++++++++-------------
  hw/block/virtio-blk.c      | 16 ++++++++++++++-
  util/iov.c                 |  5 +++--
  tests/qemu-iotests/262     | 12 +++++------
  tests/qemu-iotests/262.out |  6 +++---
 files changed, 54 insertions(+), 26 deletions(-)
 --
-.21.0
+.40.1

-[PULL 1/4] util/ioc.c: try to reassure Coverity about qemu_iovec_init_extended
+Deleted patch
-From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
-Make it more obvious, that filling qiov corresponds to qiov allocation,
-which in turn corresponds to total_niov calculation, based on mid_niov
-(not mid_len). Still add an assertion to show that there should be no
-difference.
-[Added mingw "error: 'mid_iov' may be used uninitialized in this
-function" compiler error fix suggested by Vladimir.
---Stefan]
-Reported-by: Coverity (CID 1405302)
-Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
-Message-id: 20190910090310.14032-1-vsementsov@virtuozzo.com
-Suggested-by: Peter Maydell <peter.maydell@linaro.org>
-Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
-Message-Id: <20190910090310.14032-1-vsementsov@virtuozzo.com>
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
-fixup! util/ioc.c: try to reassure Coverity about qemu_iovec_init_extended
----
- util/iov.c | 5 +++--
-file changed, 3 insertions(+), 2 deletions(-)
-diff --git a/util/iov.c b/util/iov.c
-index XXXXXXX..XXXXXXX 100644
---- a/util/iov.c
-+++ b/util/iov.c
-@@ -XXX,XX +XXX,XX @@ void qemu_iovec_init_extended(
- {
-     size_t mid_head, mid_tail;
-     int total_niov, mid_niov = 0;
--    struct iovec *p, *mid_iov;
-+    struct iovec *p, *mid_iov = NULL;
-     if (mid_len) {
-         mid_iov = qiov_slice(mid_qiov, mid_offset, mid_len,
-@@ -XXX,XX +XXX,XX @@ void qemu_iovec_init_extended(
-         p++;
-     }
--    if (mid_len) {
-+    assert(!mid_niov == !mid_len);
-+    if (mid_niov) {
-         memcpy(p, mid_iov, mid_niov * sizeof(*p));
-         p[0].iov_base = (uint8_t *)p[0].iov_base + mid_head;
-         p[0].iov_len -= mid_head;
---
-.21.0

-[PULL 2/4] virtio-blk: schedule virtio_notify_config to run on main context
+Deleted patch
-From: Sergio Lopez <slp@redhat.com>
-virtio_notify_config() needs to acquire the global mutex, which isn't
-allowed from an iothread, and may lead to a deadlock like this:
- - main thead
-  * Has acquired: qemu_global_mutex.
-  * Is trying the acquire: iothread AioContext lock via
-    AIO_WAIT_WHILE (after aio_poll).
- - iothread
-  * Has acquired: AioContext lock.
-  * Is trying to acquire: qemu_global_mutex (via
-    virtio_notify_config->prepare_mmio_access).
-If virtio_blk_resize() is called from an iothread, schedule
-virtio_notify_config() to be run in the main context BH.
-[Removed unnecessary newline as suggested by Kevin Wolf
-<kwolf@redhat.com>.
---Stefan]
-Signed-off-by: Sergio Lopez <slp@redhat.com>
-Reviewed-by: Kevin Wolf <kwolf@redhat.com>
-Message-id: 20190916112411.21636-1-slp@redhat.com
-Message-Id: <20190916112411.21636-1-slp@redhat.com>
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
----
- hw/block/virtio-blk.c | 16 +++++++++++++++-
-file changed, 15 insertions(+), 1 deletion(-)
-diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
-index XXXXXXX..XXXXXXX 100644
---- a/hw/block/virtio-blk.c
-+++ b/hw/block/virtio-blk.c
-@@ -XXX,XX +XXX,XX @@
- #include "qemu/iov.h"
- #include "qemu/module.h"
- #include "qemu/error-report.h"
-+#include "qemu/main-loop.h"
- #include "trace.h"
- #include "hw/block/block.h"
- #include "hw/qdev-properties.h"
-@@ -XXX,XX +XXX,XX @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
-     return 0;
- }
-+static void virtio_resize_cb(void *opaque)
-+{
-+    VirtIODevice *vdev = opaque;
-+
-+    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
-+    virtio_notify_config(vdev);
-+}
-+
- static void virtio_blk_resize(void *opaque)
- {
-     VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
--    virtio_notify_config(vdev);
-+    /*
-+     * virtio_notify_config() needs to acquire the global mutex,
-+     * so it can't be called from an iothread. Instead, schedule
-+     * it to be run in the main context BH.
-+     */
-+    aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
- }
- static const BlockDevOps virtio_block_ops = {
---
-.21.0

-[PULL 3/4] block: Skip COR for inactive nodes
+[PULL 1/1] virtio-blk: fix host notifier issues during dataplane start/stop
-From: Max Reitz <mreitz@redhat.com>
+The main loop thread can consume 100% CPU when using --device
 virtio-blk-pci,iothread=<iothread>. ppoll() constantly returns but
 reading virtqueue host notifiers fails with EAGAIN. The file descriptors
 are stale and remain registered with the AioContext because of bugs in
 the virtio-blk dataplane start/stop code.
-We must not write data to inactive nodes, and a COR is certainly
+The problem is that the dataplane start/stop code involves drain
-something we can simply not do without upsetting anyone.  So skip COR
+operations, which call virtio_blk_drained_begin() and
-operations on inactive nodes.
+virtio_blk_drained_end() at points where the host notifier is not
 operational:
 - In virtio_blk_data_plane_start(), blk_set_aio_context() drains after
   vblk->dataplane_started has been set to true but the host notifier has
   not been attached yet.
 - In virtio_blk_data_plane_stop(), blk_drain() and blk_set_aio_context()
   drain after the host notifier has already been detached but with
   vblk->dataplane_started still set to true.
-Signed-off-by: Max Reitz <mreitz@redhat.com>
+I would like to simplify ->ioeventfd_start/stop() to avoid interactions
-Reviewed-by: Eric Blake <eblake@redhat.com>
+with drain entirely, but couldn't find a way to do that. Instead, this
-Message-id: 20191001174827.11081-2-mreitz@redhat.com
+patch accepts the fragile nature of the code and reorders it so that
-Message-Id: <20191001174827.11081-2-mreitz@redhat.com>
+vblk->dataplane_started is false during drain operations. This way the
 virtio_blk_drained_begin() and virtio_blk_drained_end() calls don't
 touch the host notifier. The result is that
 virtio_blk_data_plane_start() and virtio_blk_data_plane_stop() have
 complete control over the host notifier and stale file descriptors are
 no longer left in the AioContext.
 This patch fixes the 100% CPU consumption in the main loop thread and
 correctly moves host notifier processing to the IOThread.
 Fixes: 1665d9326fd2 ("virtio-blk: implement BlockDevOps->drained_begin()")
 Reported-by: Lukáš Doktor <ldoktor@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 Tested-by: Lukas Doktor <ldoktor@redhat.com>
 Message-id: 20230704151527.193586-1-stefanha@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- block/io.c | 41 +++++++++++++++++++++++++++--------------
+ hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++--------------
-file changed, 27 insertions(+), 14 deletions(-)
+file changed, 38 insertions(+), 29 deletions(-)
-diff --git a/block/io.c b/block/io.c
+diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
 index XXXXXXX..XXXXXXX 100644
---- a/block/io.c
+--- a/hw/block/dataplane/virtio-blk.c
-+++ b/block/io.c
++++ b/hw/block/dataplane/virtio-blk.c
-@@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
+@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
-     int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
-                                     BDRV_REQUEST_MAX_BYTES);
+     memory_region_transaction_commit();
-     unsigned int progress = 0;
-+    bool skip_write;
+-    /*
+-     * These fields are visible to the IOThread so we rely on implicit barriers
-     if (!drv) {
+-     * in aio_context_acquire() on the write side and aio_notify_accept() on
-         return -ENOMEDIUM;
+-     * the read side.
 -     */
 -    s->starting = false;
 -    vblk->dataplane_started = true;
      trace_virtio_blk_data_plane_start(s);
      old_context = blk_get_aio_context(s->conf->conf.blk);
@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
          event_notifier_set(virtio_queue_get_host_notifier(vq));
      }
 +    /*
-+     * Do not write anything when the BDS is inactive.  That is not
++     * These fields must be visible to the IOThread when it processes the
-+     * allowed, and it would not help.
++     * virtqueue, otherwise it will think dataplane has not started yet.
 +     *
 +     * Make sure ->dataplane_started is false when blk_set_aio_context() is
 +     * called above so that draining does not cause the host notifier to be
 +     * detached/attached prematurely.
 +     */
-+    skip_write = (bs->open_flags & BDRV_O_INACTIVE);
++    s->starting = false;
 +    vblk->dataplane_started = true;
 +    smp_wmb(); /* paired with aio_notify_accept() on the read side */
 +
-     /* FIXME We cannot require callers to have write permissions when all they
+     /* Get this show started by hooking up our callbacks */
-      * are doing is a read request. If we did things right, write permissions
+     if (!blk_in_drain(s->conf->conf.blk)) {
-      * would be obtained anyway, but internally by the copy-on-read code. As
+         aio_context_acquire(s->ctx);
-@@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
+@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
-     while (cluster_bytes) {
+   fail_guest_notifiers:
-         int64_t pnum;
+     vblk->dataplane_disabled = true;
+     s->starting = false;
--        ret = bdrv_is_allocated(bs, cluster_offset,
+-    vblk->dataplane_started = true;
--                                MIN(cluster_bytes, max_transfer), &pnum);
+     return -ENOSYS;
--        if (ret < 0) {
+ }
--            /* Safe to treat errors in querying allocation as if
--             * unallocated; we'll probably fail again soon on the
+@@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
--             * read, but at least that will set a decent errno.
+         aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
--             */
+     }
-+        if (skip_write) {
-+            ret = 1; /* "already allocated", so nothing will be copied */
++    /*
-             pnum = MIN(cluster_bytes, max_transfer);
++     * Batch all the host notifiers in a single transaction to avoid
--        }
++     * quadratic time complexity in address_space_update_ioeventfds().
-+        } else {
++     */
-+            ret = bdrv_is_allocated(bs, cluster_offset,
++    memory_region_transaction_begin();
-+                                    MIN(cluster_bytes, max_transfer), &pnum);
++
-+            if (ret < 0) {
++    for (i = 0; i < nvqs; i++) {
-+                /*
++        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
-+                 * Safe to treat errors in querying allocation as if
++    }
-+                 * unallocated; we'll probably fail again soon on the
++
-+                 * read, but at least that will set a decent errno.
++    /*
-+                 */
++     * The transaction expects the ioeventfds to be open when it
-+                pnum = MIN(cluster_bytes, max_transfer);
++     * commits. Do it now, before the cleanup loop.
-+            }
++     */
++    memory_region_transaction_commit();
--        /* Stop at EOF if the image ends in the middle of the cluster */
++
--        if (ret == 0 && pnum == 0) {
++    for (i = 0; i < nvqs; i++) {
--            assert(progress >= bytes);
++        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
--            break;
++    }
--        }
++
-+            /* Stop at EOF if the image ends in the middle of the cluster */
++    /*
-+            if (ret == 0 && pnum == 0) {
++     * Set ->dataplane_started to false before draining so that host notifiers
-+                assert(progress >= bytes);
++     * are not detached/attached anymore.
-+                break;
++     */
-+            }
++    vblk->dataplane_started = false;
++
--        assert(skip_bytes < pnum);
+     aio_context_acquire(s->ctx);
-+            assert(skip_bytes < pnum);
-+        }
+     /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
+@@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
-         if (ret <= 0) {
-             QEMUIOVector local_qiov;
+     aio_context_release(s->ctx);
 -    /*
 -     * Batch all the host notifiers in a single transaction to avoid
 -     * quadratic time complexity in address_space_update_ioeventfds().
 -     */
 -    memory_region_transaction_begin();
 -
 -    for (i = 0; i < nvqs; i++) {
 -        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
 -    }
 -
 -    /*
 -     * The transaction expects the ioeventfds to be open when it
 -     * commits. Do it now, before the cleanup loop.
 -     */
 -    memory_region_transaction_commit();
 -
 -    for (i = 0; i < nvqs; i++) {
 -        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
 -    }
 -
      qemu_bh_cancel(s->bh);
      notify_guest_bh(s); /* final chance to notify guest */
      /* Clean up guest notifier (irq) */
      k->set_guest_notifiers(qbus->parent, nvqs, false);
 -    vblk->dataplane_started = false;
      s->stopping = false;
  }
 --
-.21.0
+.40.1

-[PULL 4/4] iotests/262: Switch source/dest VM launch order
+Deleted patch
-From: Max Reitz <mreitz@redhat.com>
-Launching the destination VM before the source VM gives us a regression
-test for HEAD^:
-The guest device causes a read from the disk image through
-guess_disk_lchs().  This will not work if the first sector (containing
-the partition table) is yet unallocated, we use COR, and the node is
-inactive.
-By launching the source VM before the destination, however, the COR
-filter on the source will allocate that area in the image shared between
-both VMs, thus the problem will not become apparent.
-Switching the launch order causes the sector to still be unallocated
-when guess_disk_lchs() runs on the inactive node in the destination VM,
-and thus we get our test case.
-Signed-off-by: Max Reitz <mreitz@redhat.com>
-Reviewed-by: Eric Blake <eblake@redhat.com>
-Message-id: 20191001174827.11081-3-mreitz@redhat.com
-Message-Id: <20191001174827.11081-3-mreitz@redhat.com>
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
----
- tests/qemu-iotests/262     | 12 ++++++------
- tests/qemu-iotests/262.out |  6 +++---
-files changed, 9 insertions(+), 9 deletions(-)
-diff --git a/tests/qemu-iotests/262 b/tests/qemu-iotests/262
-index XXXXXXX..XXXXXXX 100755
---- a/tests/qemu-iotests/262
-+++ b/tests/qemu-iotests/262
-@@ -XXX,XX +XXX,XX @@ with iotests.FilePath('img') as img_path, \
-     os.mkfifo(fifo)
--    iotests.log('Launching source VM...')
--    add_opts(vm_a)
--    vm_a.launch()
--
--    vm_a.enable_migration_events('A')
--
-     iotests.log('Launching destination VM...')
-     add_opts(vm_b)
-     vm_b.add_incoming("exec: cat '%s'" % (fifo))
-@@ -XXX,XX +XXX,XX @@ with iotests.FilePath('img') as img_path, \
-     vm_b.enable_migration_events('B')
-+    iotests.log('Launching source VM...')
-+    add_opts(vm_a)
-+    vm_a.launch()
-+
-+    vm_a.enable_migration_events('A')
-+
-     iotests.log('Starting migration to B...')
-     iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo)))
-     with iotests.Timeout(3, 'Migration does not complete'):
-diff --git a/tests/qemu-iotests/262.out b/tests/qemu-iotests/262.out
-index XXXXXXX..XXXXXXX 100644
---- a/tests/qemu-iotests/262.out
-+++ b/tests/qemu-iotests/262.out
-@@ -XXX,XX +XXX,XX @@
--Launching source VM...
--Enabling migration QMP events on A...
--{"return": {}}
- Launching destination VM...
- Enabling migration QMP events on B...
- {"return": {}}
-+Launching source VM...
-+Enabling migration QMP events on A...
-+{"return": {}}
- Starting migration to B...
- {"return": {}}
- {"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
---
-.21.0

The following changes since commit 560009f2c8b57b7cdd31a5693ea86ab369382f49:

Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2019-10-07 15:40:53 +0100)

are available in the Git repository at:

https://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 4d804b5305ffb4d5fa414c38d4f1bdfb987c8d0b:

iotests/262: Switch source/dest VM launch order (2019-10-08 14:28:25 +0100)

----------------------------------------------------------------
Pull request

This pull request also contains the two commits from the previous pull request
that was dropped due to a mingw compilation error.  The compilation should now
be fixed.

----------------------------------------------------------------

Max Reitz (2):
  block: Skip COR for inactive nodes
  iotests/262: Switch source/dest VM launch order

Sergio Lopez (1):
  virtio-blk: schedule virtio_notify_config to run on main context

Vladimir Sementsov-Ogievskiy (1):
  util/ioc.c: try to reassure Coverity about qemu_iovec_init_extended

-- 
2.21.0

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Make it more obvious, that filling qiov corresponds to qiov allocation,
which in turn corresponds to total_niov calculation, based on mid_niov
(not mid_len). Still add an assertion to show that there should be no
difference.

[Added mingw "error: 'mid_iov' may be used uninitialized in this
function" compiler error fix suggested by Vladimir.
--Stefan]

Reported-by: Coverity (CID 1405302)
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20190910090310.14032-1-vsementsov@virtuozzo.com
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20190910090310.14032-1-vsementsov@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

fixup! util/ioc.c: try to reassure Coverity about qemu_iovec_init_extended
---
 util/iov.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/util/iov.c b/util/iov.c
index XXXXXXX..XXXXXXX 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -XXX,XX +XXX,XX @@ void qemu_iovec_init_extended(
 {
     size_t mid_head, mid_tail;
     int total_niov, mid_niov = 0;
-    struct iovec *p, *mid_iov;
+    struct iovec *p, *mid_iov = NULL;
 
     if (mid_len) {
         mid_iov = qiov_slice(mid_qiov, mid_offset, mid_len,
@@ -XXX,XX +XXX,XX @@ void qemu_iovec_init_extended(
         p++;
     }
 
-    if (mid_len) {
+    assert(!mid_niov == !mid_len);
+    if (mid_niov) {
         memcpy(p, mid_iov, mid_niov * sizeof(*p));
         p[0].iov_base = (uint8_t *)p[0].iov_base + mid_head;
         p[0].iov_len -= mid_head;
-- 
2.21.0

From: Sergio Lopez <slp@redhat.com>

virtio_notify_config() needs to acquire the global mutex, which isn't
allowed from an iothread, and may lead to a deadlock like this:

- main thead
  * Has acquired: qemu_global_mutex.
  * Is trying the acquire: iothread AioContext lock via
    AIO_WAIT_WHILE (after aio_poll).

- iothread
  * Has acquired: AioContext lock.
  * Is trying to acquire: qemu_global_mutex (via
    virtio_notify_config->prepare_mmio_access).

If virtio_blk_resize() is called from an iothread, schedule
virtio_notify_config() to be run in the main context BH.

[Removed unnecessary newline as suggested by Kevin Wolf
<kwolf@redhat.com>.
--Stefan]

Signed-off-by: Sergio Lopez <slp@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-id: 20190916112411.21636-1-slp@redhat.com
Message-Id: <20190916112411.21636-1-slp@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -XXX,XX +XXX,XX @@
 #include "qemu/iov.h"
 #include "qemu/module.h"
 #include "qemu/error-report.h"
+#include "qemu/main-loop.h"
 #include "trace.h"
 #include "hw/block/block.h"
 #include "hw/qdev-properties.h"
@@ -XXX,XX +XXX,XX @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
     return 0;
 }
 
+static void virtio_resize_cb(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+
+    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+    virtio_notify_config(vdev);
+}
+
 static void virtio_blk_resize(void *opaque)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
 
-    virtio_notify_config(vdev);
+    /*
+     * virtio_notify_config() needs to acquire the global mutex,
+     * so it can't be called from an iothread. Instead, schedule
+     * it to be run in the main context BH.
+     */
+    aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
 }
 
 static const BlockDevOps virtio_block_ops = {
-- 
2.21.0

From: Max Reitz <mreitz@redhat.com>

We must not write data to inactive nodes, and a COR is certainly
something we can simply not do without upsetting anyone.  So skip COR
operations on inactive nodes.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 20191001174827.11081-2-mreitz@redhat.com
Message-Id: <20191001174827.11081-2-mreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/io.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/block/io.c b/block/io.c
index XXXXXXX..XXXXXXX 100644
--- a/block/io.c
+++ b/block/io.c
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
     int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
                                     BDRV_REQUEST_MAX_BYTES);
     unsigned int progress = 0;
+    bool skip_write;
 
     if (!drv) {
         return -ENOMEDIUM;
     }
 
+    /*
+     * Do not write anything when the BDS is inactive.  That is not
+     * allowed, and it would not help.
+     */
+    skip_write = (bs->open_flags & BDRV_O_INACTIVE);
+
     /* FIXME We cannot require callers to have write permissions when all they
      * are doing is a read request. If we did things right, write permissions
      * would be obtained anyway, but internally by the copy-on-read code. As
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
     while (cluster_bytes) {
         int64_t pnum;
 
-        ret = bdrv_is_allocated(bs, cluster_offset,
-                                MIN(cluster_bytes, max_transfer), &pnum);
-        if (ret < 0) {
-            /* Safe to treat errors in querying allocation as if
-             * unallocated; we'll probably fail again soon on the
-             * read, but at least that will set a decent errno.
-             */
+        if (skip_write) {
+            ret = 1; /* "already allocated", so nothing will be copied */
             pnum = MIN(cluster_bytes, max_transfer);
-        }
+        } else {
+            ret = bdrv_is_allocated(bs, cluster_offset,
+                                    MIN(cluster_bytes, max_transfer), &pnum);
+            if (ret < 0) {
+                /*
+                 * Safe to treat errors in querying allocation as if
+                 * unallocated; we'll probably fail again soon on the
+                 * read, but at least that will set a decent errno.
+                 */
+                pnum = MIN(cluster_bytes, max_transfer);
+            }
 
-        /* Stop at EOF if the image ends in the middle of the cluster */
-        if (ret == 0 && pnum == 0) {
-            assert(progress >= bytes);
-            break;
-        }
+            /* Stop at EOF if the image ends in the middle of the cluster */
+            if (ret == 0 && pnum == 0) {
+                assert(progress >= bytes);
+                break;
+            }
 
-        assert(skip_bytes < pnum);
+            assert(skip_bytes < pnum);
+        }
 
         if (ret <= 0) {
             QEMUIOVector local_qiov;
-- 
2.21.0

From: Max Reitz <mreitz@redhat.com>

Launching the destination VM before the source VM gives us a regression
test for HEAD^:

The guest device causes a read from the disk image through
guess_disk_lchs().  This will not work if the first sector (containing
the partition table) is yet unallocated, we use COR, and the node is
inactive.

By launching the source VM before the destination, however, the COR
filter on the source will allocate that area in the image shared between
both VMs, thus the problem will not become apparent.

Switching the launch order causes the sector to still be unallocated
when guess_disk_lchs() runs on the inactive node in the destination VM,
and thus we get our test case.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 20191001174827.11081-3-mreitz@redhat.com
Message-Id: <20191001174827.11081-3-mreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/262     | 12 ++++++------
 tests/qemu-iotests/262.out |  6 +++---
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/262 b/tests/qemu-iotests/262
index XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/262
+++ b/tests/qemu-iotests/262
@@ -XXX,XX +XXX,XX @@ with iotests.FilePath('img') as img_path, \
 
     os.mkfifo(fifo)
 
-    iotests.log('Launching source VM...')
-    add_opts(vm_a)
-    vm_a.launch()
-
-    vm_a.enable_migration_events('A')
-
     iotests.log('Launching destination VM...')
     add_opts(vm_b)
     vm_b.add_incoming("exec: cat '%s'" % (fifo))
@@ -XXX,XX +XXX,XX @@ with iotests.FilePath('img') as img_path, \
 
     vm_b.enable_migration_events('B')
 
+    iotests.log('Launching source VM...')
+    add_opts(vm_a)
+    vm_a.launch()
+
+    vm_a.enable_migration_events('A')
+
     iotests.log('Starting migration to B...')
     iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo)))
     with iotests.Timeout(3, 'Migration does not complete'):
diff --git a/tests/qemu-iotests/262.out b/tests/qemu-iotests/262.out
index XXXXXXX..XXXXXXX 100644
--- a/tests/qemu-iotests/262.out
+++ b/tests/qemu-iotests/262.out
@@ -XXX,XX +XXX,XX @@
-Launching source VM...
-Enabling migration QMP events on A...
-{"return": {}}
 Launching destination VM...
 Enabling migration QMP events on B...
 {"return": {}}
+Launching source VM...
+Enabling migration QMP events on A...
+{"return": {}}
 Starting migration to B...
 {"return": {}}
 {"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-- 
2.21.0

The main loop thread can consume 100% CPU when using --device
virtio-blk-pci,iothread=<iothread>. ppoll() constantly returns but
reading virtqueue host notifiers fails with EAGAIN. The file descriptors
are stale and remain registered with the AioContext because of bugs in
the virtio-blk dataplane start/stop code.

The problem is that the dataplane start/stop code involves drain
operations, which call virtio_blk_drained_begin() and
virtio_blk_drained_end() at points where the host notifier is not
operational:
- In virtio_blk_data_plane_start(), blk_set_aio_context() drains after
  vblk->dataplane_started has been set to true but the host notifier has
  not been attached yet.
- In virtio_blk_data_plane_stop(), blk_drain() and blk_set_aio_context()
  drain after the host notifier has already been detached but with
  vblk->dataplane_started still set to true.

I would like to simplify ->ioeventfd_start/stop() to avoid interactions
with drain entirely, but couldn't find a way to do that. Instead, this
patch accepts the fragile nature of the code and reorders it so that
vblk->dataplane_started is false during drain operations. This way the
virtio_blk_drained_begin() and virtio_blk_drained_end() calls don't
touch the host notifier. The result is that
virtio_blk_data_plane_start() and virtio_blk_data_plane_stop() have
complete control over the host notifier and stale file descriptors are
no longer left in the AioContext.

This patch fixes the 100% CPU consumption in the main loop thread and
correctly moves host notifier processing to the IOThread.

Fixes: 1665d9326fd2 ("virtio-blk: implement BlockDevOps->drained_begin()")
Reported-by: Lukáš Doktor <ldoktor@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-by: Lukas Doktor <ldoktor@redhat.com>
Message-id: 20230704151527.193586-1-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 29 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
 
     memory_region_transaction_commit();
 
-    /*
-     * These fields are visible to the IOThread so we rely on implicit barriers
-     * in aio_context_acquire() on the write side and aio_notify_accept() on
-     * the read side.
-     */
-    s->starting = false;
-    vblk->dataplane_started = true;
     trace_virtio_blk_data_plane_start(s);
 
     old_context = blk_get_aio_context(s->conf->conf.blk);
@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
         event_notifier_set(virtio_queue_get_host_notifier(vq));
     }
 
+    /*
+     * These fields must be visible to the IOThread when it processes the
+     * virtqueue, otherwise it will think dataplane has not started yet.
+     *
+     * Make sure ->dataplane_started is false when blk_set_aio_context() is
+     * called above so that draining does not cause the host notifier to be
+     * detached/attached prematurely.
+     */
+    s->starting = false;
+    vblk->dataplane_started = true;
+    smp_wmb(); /* paired with aio_notify_accept() on the read side */
+
     /* Get this show started by hooking up our callbacks */
     if (!blk_in_drain(s->conf->conf.blk)) {
         aio_context_acquire(s->ctx);
@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
   fail_guest_notifiers:
     vblk->dataplane_disabled = true;
     s->starting = false;
-    vblk->dataplane_started = true;
     return -ENOSYS;
 }
 
@@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
         aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
     }
 
+    /*
+     * Batch all the host notifiers in a single transaction to avoid
+     * quadratic time complexity in address_space_update_ioeventfds().
+     */
+    memory_region_transaction_begin();
+
+    for (i = 0; i < nvqs; i++) {
+        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+    }
+
+    /*
+     * The transaction expects the ioeventfds to be open when it
+     * commits. Do it now, before the cleanup loop.
+     */
+    memory_region_transaction_commit();
+
+    for (i = 0; i < nvqs; i++) {
+        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
+    }
+
+    /*
+     * Set ->dataplane_started to false before draining so that host notifiers
+     * are not detached/attached anymore.
+     */
+    vblk->dataplane_started = false;
+
     aio_context_acquire(s->ctx);
 
     /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
@@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
 
     aio_context_release(s->ctx);
 
-    /*
-     * Batch all the host notifiers in a single transaction to avoid
-     * quadratic time complexity in address_space_update_ioeventfds().
-     */
-    memory_region_transaction_begin();
-
-    for (i = 0; i < nvqs; i++) {
-        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
-    }
-
-    /*
-     * The transaction expects the ioeventfds to be open when it
-     * commits. Do it now, before the cleanup loop.
-     */
-    memory_region_transaction_commit();
-
-    for (i = 0; i < nvqs; i++) {
-        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
-    }
-
     qemu_bh_cancel(s->bh);
     notify_guest_bh(s); /* final chance to notify guest */
 
     /* Clean up guest notifier (irq) */
     k->set_guest_notifiers(qbus->parent, nvqs, false);
 
-    vblk->dataplane_started = false;
     s->stopping = false;
 }
-- 
2.40.1