qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Fiona Ebner <f.ebner@proxmox.com>
To: qemu-devel@nongnu.org
Cc: stefanha@redhat.com, fam@euphon.net, eblake@redhat.com,
	vsementsov@yandex-team.ru, jsnow@redhat.com, quintela@redhat.com,
	peterx@redhat.com, leobras@redhat.com, qemu-block@nongnu.org
Subject: [RFC] migration/block-dirty-bitmap: make loading bitmap for device with iothread future-proof
Date: Fri, 28 Jul 2023 15:39:28 +0200	[thread overview]
Message-ID: <20230728133928.256898-1-f.ebner@proxmox.com> (raw)

The bdrv_create_dirty_bitmap() function (which is also called by
bdrv_dirty_bitmap_create_successor()) uses bdrv_getlength(bs). This is
a wrapper around a coroutine, and when not called in coroutine context
would use bdrv_poll_co(). Such a call would trigger an assert() if the
correct AioContext hasn't been acquired before, because polling would
try to release the AioContext.

The issue does not happen for migration, because the call happens
from process_incoming_migration_co(), i.e. in coroutine context. So
the bdrv_getlength() wrapper will just call bdrv_co_getlength()
directly without polling.

The issue would happen for snapshots, but won't in practice, because
saving a snapshot with a block dirty bitmap is currently not possible.
The reason is that dirty_bitmap_save_iterate() returns whether it has
completed the bulk phase, which only happens in postcopy, so
qemu_savevm_state_iterate() will always return 0, meaning the call
to iterate will be repeated over and over again without ever reaching
the completion phase.

Still, this would make the code more robust for the future.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

We ran into this issue downstream, because we have a custom snapshot
mechanism which does support dirty bitmaps and does not run in
coroutine context during load.

 migration/block-dirty-bitmap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 032fc5f405..e1ae3b7316 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -805,8 +805,11 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
                      "destination", bdrv_dirty_bitmap_name(s->bitmap));
         return -EINVAL;
     } else {
+        AioContext *ctx = bdrv_get_aio_context(s->bs);
+        aio_context_acquire(ctx);
         s->bitmap = bdrv_create_dirty_bitmap(s->bs, granularity,
                                              s->bitmap_name, &local_err);
+        aio_context_release(ctx);
         if (!s->bitmap) {
             error_report_err(local_err);
             return -EINVAL;
@@ -833,7 +836,10 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
 
     bdrv_disable_dirty_bitmap(s->bitmap);
     if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
+        AioContext *ctx = bdrv_get_aio_context(s->bs);
+        aio_context_acquire(ctx);
         bdrv_dirty_bitmap_create_successor(s->bitmap, &local_err);
+        aio_context_release(ctx);
         if (local_err) {
             error_report_err(local_err);
             return -EINVAL;
-- 
2.39.2




             reply	other threads:[~2023-07-28 13:54 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-28 13:39 Fiona Ebner [this message]
2023-07-31  7:35 ` [RFC] migration/block-dirty-bitmap: make loading bitmap for device with iothread future-proof Juan Quintela
2023-08-01  7:57   ` Fiona Ebner
2023-10-04 16:04   ` Vladimir Sementsov-Ogievskiy
2023-10-04 16:13 ` Vladimir Sementsov-Ogievskiy
2023-10-05  7:19   ` Fiona Ebner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230728133928.256898-1-f.ebner@proxmox.com \
    --to=f.ebner@proxmox.com \
    --cc=eblake@redhat.com \
    --cc=fam@euphon.net \
    --cc=jsnow@redhat.com \
    --cc=leobras@redhat.com \
    --cc=peterx@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    --cc=stefanha@redhat.com \
    --cc=vsementsov@yandex-team.ru \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).