Series comparison

-[Qemu-devel] [PULL 0/4] Block patches
+[PULL 0/1] Block patches
-The following changes since commit 20d6c7312f1b812bb9c750f4087f69ac8485cc90:
+The following changes since commit 887cba855bb6ff4775256f7968409281350b568c:
-  Merge remote-tracking branch 'remotes/palmer/tags/riscv-for-master-3.2-part1' into staging (2019-01-03 13:26:30 +0000)
+  configure: Fix cross-building for RISCV host (v5) (2023-07-11 17:56:09 +0100)
 are available in the Git repository at:
-  git://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 39a0408e768cd00142f5b57d27ab234282bf4df5:
+for you to fetch changes up to 75dcb4d790bbe5327169fd72b185960ca58e2fa6:
-  dmg: don't skip zero chunk (2019-01-04 11:15:09 +0000)
+  virtio-blk: fix host notifier issues during dataplane start/stop (2023-07-12 15:20:32 -0400)
 ----------------------------------------------------------------
 Pull request
-Bug fixes for the .dmg image file format.
 ----------------------------------------------------------------
-Julio Faracco (1):
+Stefan Hajnoczi (1):
-  dmg: Fixing wrong dmg block type value for block terminator.
+  virtio-blk: fix host notifier issues during dataplane start/stop
-yuchenlin (3):
+ hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++--------------
-  dmg: fix binary search
+file changed, 38 insertions(+), 29 deletions(-)
   dmg: use enumeration type instead of hard coding number
   dmg: don't skip zero chunk
  block/dmg.c | 31 ++++++++++++++++++++-----------
 file changed, 20 insertions(+), 11 deletions(-)
 --
-.20.1
+.40.1

-[Qemu-devel] [PULL 1/4] dmg: Fixing wrong dmg block type value for block terminator.
+Deleted patch
-From: Julio Faracco <jcfaracco@gmail.com>
-This is a trivial patch to fix a wrong value for block terminator.
-The old value was 0x7fffffff which is wrong. It was not affecting the
-code because QEMU dmg block is not handling block terminator right now.
-Neverthless, it should be fixed.
-Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
-Reviewed-by: yuchenlin <yuchenlin@synology.com>
-Message-id: 20181228145055.18039-1-jcfaracco@gmail.com
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
----
- block/dmg.c | 2 +-
-file changed, 1 insertion(+), 1 deletion(-)
-diff --git a/block/dmg.c b/block/dmg.c
-index XXXXXXX..XXXXXXX 100644
---- a/block/dmg.c
-+++ b/block/dmg.c
-@@ -XXX,XX +XXX,XX @@ enum {
-     UDBZ,
-     ULFO,
-     UDCM = 0x7ffffffe, /* Comments */
--    UDLE               /* Last Entry */
-+    UDLE = 0xffffffff  /* Last Entry */
- };
- static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
---
-.20.1

-[Qemu-devel] [PULL 2/4] dmg: fix binary search
+[PULL 1/1] virtio-blk: fix host notifier issues during dataplane start/stop
-From: yuchenlin <npes87184@gmail.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.
-There is a possible hang in original binary search implementation. That is
+The problem is that the dataplane start/stop code involves drain
-if chunk1 = 4, chunk2 = 5, chunk3 = 4, and we go else case.
+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.
-The chunk1 will be still 4, and so on.
+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.
-Signed-off-by: yuchenlin <npes87184@gmail.com>
+This patch fixes the 100% CPU consumption in the main loop thread and
-Message-id: 20190103114700.9686-2-npes87184@gmail.com
+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/dmg.c | 10 +++++++---
+ hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++--------------
-file changed, 7 insertions(+), 3 deletions(-)
+file changed, 38 insertions(+), 29 deletions(-)
-diff --git a/block/dmg.c b/block/dmg.c
+diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
 index XXXXXXX..XXXXXXX 100644
---- a/block/dmg.c
+--- a/hw/block/dataplane/virtio-blk.c
-+++ b/block/dmg.c
++++ b/hw/block/dataplane/virtio-blk.c
-@@ -XXX,XX +XXX,XX @@ static inline uint32_t search_chunk(BDRVDMGState *s, uint64_t sector_num)
+@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
- {
-     /* binary search */
+     memory_region_transaction_commit();
-     uint32_t chunk1 = 0, chunk2 = s->n_chunks, chunk3;
--    while (chunk1 != chunk2) {
+-    /*
-+    while (chunk1 <= chunk2) {
+-     * These fields are visible to the IOThread so we rely on implicit barriers
-         chunk3 = (chunk1 + chunk2) / 2;
+-     * in aio_context_acquire() on the write side and aio_notify_accept() on
-         if (s->sectors[chunk3] > sector_num) {
+-     * the read side.
--            chunk2 = chunk3;
+-     */
-+            if (chunk3 == 0) {
+-    s->starting = false;
-+                goto err;
+-    vblk->dataplane_started = true;
-+            }
+     trace_virtio_blk_data_plane_start(s);
-+            chunk2 = chunk3 - 1;
-         } else if (s->sectors[chunk3] + s->sectorcounts[chunk3] > sector_num) {
+     old_context = blk_get_aio_context(s->conf->conf.blk);
-             return chunk3;
+@@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
-         } else {
+         event_notifier_set(virtio_queue_get_host_notifier(vq));
 -            chunk1 = chunk3;
 +            chunk1 = chunk3 + 1;
          }
      }
-+err:
-     return s->n_chunks; /* error */
++    /*
 +     * 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;
+ }
 --
-.20.1
+.40.1

-[Qemu-devel] [PULL 3/4] dmg: use enumeration type instead of hard coding number
+Deleted patch
-From: yuchenlin <npes87184@gmail.com>
-Signed-off-by: yuchenlin <npes87184@gmail.com>
-Reviewed-by: Julio Faracco <jcfaracco@gmail.com>
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
-Message-id: 20190103114700.9686-3-npes87184@gmail.com
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
----
- block/dmg.c | 4 ++--
-file changed, 2 insertions(+), 2 deletions(-)
-diff --git a/block/dmg.c b/block/dmg.c
-index XXXXXXX..XXXXXXX 100644
---- a/block/dmg.c
-+++ b/block/dmg.c
-@@ -XXX,XX +XXX,XX @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
-         /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
-          * therefore be unbounded. */
--        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
-+        if (s->types[i] != UDIG && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
-             error_report("sector count %" PRIu64 " for chunk %" PRIu32
-                          " is larger than max (%u)",
-                          s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
-@@ -XXX,XX +XXX,XX @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
-         /* Special case: current chunk is all zeroes. Do not perform a memcpy as
-          * s->uncompressed_chunk may be too small to cover the large all-zeroes
-          * section. dmg_read_chunk is called to find s->current_chunk */
--        if (s->types[s->current_chunk] == 2) { /* all zeroes block entry */
-+        if (s->types[s->current_chunk] == UDIG) { /* all zeroes block entry */
-             qemu_iovec_memset(qiov, i * 512, 0, 512);
-             continue;
-         }
---
-.20.1

-[Qemu-devel] [PULL 4/4] dmg: don't skip zero chunk
+Deleted patch
-From: yuchenlin <npes87184@gmail.com>
-The dmg file has many tables which describe: "start from sector XXX to
-sector XXX, the compression method is XXX and where the compressed data
-resides on".
-Each sector in the expanded file should be covered by a table. The table
-will describe the offset of compressed data (or raw depends on the type)
-in the dmg.
-For example:
-[-----------The expanded file------------]
-[---bzip table ---]/* zeros */[---zlib---]
-    ^
-    | if we want to read this sector.
-we will find bzip table which contains this sector, and get the
-compressed data offset, read it from dmg, uncompress it, finally write to
-expanded file.
-If we skip zero chunk (table), some sector cannot find the table which
-will cause search_chunk() return s->n_chunks, dmg_read_chunk() return -1
-and finally causing dmg_co_preadv() return EIO.
-See:
-[-----------The expanded file------------]
-[---bzip table ---]/* zeros */[---zlib---]
-                    ^
-                    | if we want to read this sector.
-Oops, we cannot find the table contains it...
-In the original implementation, we don't have zero table. When we try to
-read sector inside the zero chunk. We will get EIO, and skip reading.
-After this patch, we treat zero chunk the same as ignore chunk, it will
-directly write zero and avoid some sector may not find the table.
-After this patch:
-[-----------The expanded file------------]
-[---bzip table ---][--zeros--][---zlib---]
-Signed-off-by: yuchenlin <npes87184@gmail.com>
-Reviewed-by: Julio Faracco <jcfaracco@gmail.com>
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
-Message-id: 20190103114700.9686-4-npes87184@gmail.com
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
----
- block/dmg.c | 19 ++++++++++++-------
-file changed, 12 insertions(+), 7 deletions(-)
-diff --git a/block/dmg.c b/block/dmg.c
-index XXXXXXX..XXXXXXX 100644
---- a/block/dmg.c
-+++ b/block/dmg.c
-@@ -XXX,XX +XXX,XX @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
-     case UDRW: /* copy */
-         uncompressed_sectors = DIV_ROUND_UP(s->lengths[chunk], 512);
-         break;
--    case UDIG: /* zero */
-+    case UDZE: /* zero */
-+    case UDIG: /* ignore */
-         /* as the all-zeroes block may be large, it is treated specially: the
-          * sector is not copied from a large buffer, a simple memset is used
-          * instead. Therefore uncompressed_sectors does not need to be set. */
-@@ -XXX,XX +XXX,XX @@ typedef struct DmgHeaderState {
- static bool dmg_is_known_block_type(uint32_t entry_type)
- {
-     switch (entry_type) {
-+    case UDZE:    /* zeros */
-     case UDRW:    /* uncompressed */
--    case UDIG:    /* zeroes */
-+    case UDIG:    /* ignore */
-     case UDZO:    /* zlib */
-         return true;
-     case UDBZ:    /* bzip2 */
-@@ -XXX,XX +XXX,XX @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
-         /* sector count */
-         s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
--        /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
--         * therefore be unbounded. */
--        if (s->types[i] != UDIG && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
-+        /* all-zeroes sector (type UDZE and UDIG) does not need to be
-+         * "uncompressed" and can therefore be unbounded. */
-+        if (s->types[i] != UDZE && s->types[i] != UDIG
-+            && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
-             error_report("sector count %" PRIu64 " for chunk %" PRIu32
-                          " is larger than max (%u)",
-                          s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
-@@ -XXX,XX +XXX,XX @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
-                 return -1;
-             }
-             break;
--        case UDIG: /* zero */
-+        case UDZE: /* zeros */
-+        case UDIG: /* ignore */
-             /* see dmg_read, it is treated specially. No buffer needs to be
-              * pre-filled, the zeroes can be set directly. */
-             break;
-@@ -XXX,XX +XXX,XX @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
-         /* Special case: current chunk is all zeroes. Do not perform a memcpy as
-          * s->uncompressed_chunk may be too small to cover the large all-zeroes
-          * section. dmg_read_chunk is called to find s->current_chunk */
--        if (s->types[s->current_chunk] == UDIG) { /* all zeroes block entry */
-+        if (s->types[s->current_chunk] == UDZE
-+            || s->types[s->current_chunk] == UDIG) { /* all zeroes block entry */
-             qemu_iovec_memset(qiov, i * 512, 0, 512);
-             continue;
-         }
---
-.20.1

From: yuchenlin <npes87184@gmail.com>

There is a possible hang in original binary search implementation. That is
if chunk1 = 4, chunk2 = 5, chunk3 = 4, and we go else case.

The chunk1 will be still 4, and so on.

Signed-off-by: yuchenlin <npes87184@gmail.com>
Message-id: 20190103114700.9686-2-npes87184@gmail.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/dmg.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index XXXXXXX..XXXXXXX 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -XXX,XX +XXX,XX @@ static inline uint32_t search_chunk(BDRVDMGState *s, uint64_t sector_num)
 {
     /* binary search */
     uint32_t chunk1 = 0, chunk2 = s->n_chunks, chunk3;
-    while (chunk1 != chunk2) {
+    while (chunk1 <= chunk2) {
         chunk3 = (chunk1 + chunk2) / 2;
         if (s->sectors[chunk3] > sector_num) {
-            chunk2 = chunk3;
+            if (chunk3 == 0) {
+                goto err;
+            }
+            chunk2 = chunk3 - 1;
         } else if (s->sectors[chunk3] + s->sectorcounts[chunk3] > sector_num) {
             return chunk3;
         } else {
-            chunk1 = chunk3;
+            chunk1 = chunk3 + 1;
         }
     }
+err:
     return s->n_chunks; /* error */
 }
 
-- 
2.20.1

From: yuchenlin <npes87184@gmail.com>

Signed-off-by: yuchenlin <npes87184@gmail.com>
Reviewed-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 20190103114700.9686-3-npes87184@gmail.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/dmg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index XXXXXXX..XXXXXXX 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -XXX,XX +XXX,XX @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
 
         /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
          * therefore be unbounded. */
-        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+        if (s->types[i] != UDIG && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
             error_report("sector count %" PRIu64 " for chunk %" PRIu32
                          " is larger than max (%u)",
                          s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
@@ -XXX,XX +XXX,XX @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         /* Special case: current chunk is all zeroes. Do not perform a memcpy as
          * s->uncompressed_chunk may be too small to cover the large all-zeroes
          * section. dmg_read_chunk is called to find s->current_chunk */
-        if (s->types[s->current_chunk] == 2) { /* all zeroes block entry */
+        if (s->types[s->current_chunk] == UDIG) { /* all zeroes block entry */
             qemu_iovec_memset(qiov, i * 512, 0, 512);
             continue;
         }
-- 
2.20.1

From: yuchenlin <npes87184@gmail.com>

The dmg file has many tables which describe: "start from sector XXX to
sector XXX, the compression method is XXX and where the compressed data
resides on".

Each sector in the expanded file should be covered by a table. The table
will describe the offset of compressed data (or raw depends on the type)
in the dmg.

For example:

[-----------The expanded file------------]
[---bzip table ---]/* zeros */[---zlib---]
    ^
    | if we want to read this sector.

we will find bzip table which contains this sector, and get the
compressed data offset, read it from dmg, uncompress it, finally write to
expanded file.

If we skip zero chunk (table), some sector cannot find the table which
will cause search_chunk() return s->n_chunks, dmg_read_chunk() return -1
and finally causing dmg_co_preadv() return EIO.

See:

[-----------The expanded file------------]
[---bzip table ---]/* zeros */[---zlib---]
                    ^
                    | if we want to read this sector.

Oops, we cannot find the table contains it...

In the original implementation, we don't have zero table. When we try to
read sector inside the zero chunk. We will get EIO, and skip reading.

After this patch, we treat zero chunk the same as ignore chunk, it will
directly write zero and avoid some sector may not find the table.

After this patch:

[-----------The expanded file------------]
[---bzip table ---][--zeros--][---zlib---]

Signed-off-by: yuchenlin <npes87184@gmail.com>
Reviewed-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 20190103114700.9686-4-npes87184@gmail.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/dmg.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index XXXXXXX..XXXXXXX 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -XXX,XX +XXX,XX @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
     case UDRW: /* copy */
         uncompressed_sectors = DIV_ROUND_UP(s->lengths[chunk], 512);
         break;
-    case UDIG: /* zero */
+    case UDZE: /* zero */
+    case UDIG: /* ignore */
         /* as the all-zeroes block may be large, it is treated specially: the
          * sector is not copied from a large buffer, a simple memset is used
          * instead. Therefore uncompressed_sectors does not need to be set. */
@@ -XXX,XX +XXX,XX @@ typedef struct DmgHeaderState {
 static bool dmg_is_known_block_type(uint32_t entry_type)
 {
     switch (entry_type) {
+    case UDZE:    /* zeros */
     case UDRW:    /* uncompressed */
-    case UDIG:    /* zeroes */
+    case UDIG:    /* ignore */
     case UDZO:    /* zlib */
         return true;
     case UDBZ:    /* bzip2 */
@@ -XXX,XX +XXX,XX @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         /* sector count */
         s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
 
-        /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
-         * therefore be unbounded. */
-        if (s->types[i] != UDIG && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+        /* all-zeroes sector (type UDZE and UDIG) does not need to be
+         * "uncompressed" and can therefore be unbounded. */
+        if (s->types[i] != UDZE && s->types[i] != UDIG
+            && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
             error_report("sector count %" PRIu64 " for chunk %" PRIu32
                          " is larger than max (%u)",
                          s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
@@ -XXX,XX +XXX,XX @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
                 return -1;
             }
             break;
-        case UDIG: /* zero */
+        case UDZE: /* zeros */
+        case UDIG: /* ignore */
             /* see dmg_read, it is treated specially. No buffer needs to be
              * pre-filled, the zeroes can be set directly. */
             break;
@@ -XXX,XX +XXX,XX @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         /* Special case: current chunk is all zeroes. Do not perform a memcpy as
          * s->uncompressed_chunk may be too small to cover the large all-zeroes
          * section. dmg_read_chunk is called to find s->current_chunk */
-        if (s->types[s->current_chunk] == UDIG) { /* all zeroes block entry */
+        if (s->types[s->current_chunk] == UDZE
+            || s->types[s->current_chunk] == UDIG) { /* all zeroes block entry */
             qemu_iovec_memset(qiov, i * 512, 0, 512);
             continue;
         }
-- 
2.20.1

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