[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache
Date: Tue, 20 Mar 2018 19:32:44 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

20.03.2018 16:44, Max Reitz wrote:
On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
Consider migration with shared storage. Persistent bitmaps are stored
on bdrv_inactivate. Then, on destination
process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
are already loaded on destination start. In this case we should call
qcow2_reopen_bitmaps_rw instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
Reviewed-by: John Snow <address@hidden>
  block/qcow2.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7472af6931..6219666d4a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1480,7 +1480,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
          s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
- if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
+    if (bdrv_has_readonly_bitmaps(bs)) {
So if there are any read-only bitmaps, we'll skip
qcow2_load_dirty_bitmaps() altogether.  That doesn't seem so bad because
qcow2_load_dirty_bitmaps() seems to assume no bitmaps have been loaded yet.

But if that's the case, shouldn't we skip that function if any bitmap
has been loaded already, RO or R/W?  (And we can call
qcow2_reopen_bitmaps_rw_hint() even if there aren't any RO bitmaps, it
just won't do anything then.)

This doesn't make this patch really wrong, I'm just wondering whether it
can do better.  (To add to that, I don't even know whether there is a
case where qcow2_do_open() would be called with any R/W bitmaps already


hm reasonable. So, we don't know about such cases, it's better to don't allow them.

+        if (!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) 
+            bool header_updated = false;
+            qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err);
+            update_header = update_header && !header_updated;
+        }
+    } else if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
          update_header = false;
      if (local_err != NULL) {

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]