[Top][All Lists]

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

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH v2 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache
Date: Fri, 22 Dec 2017 17:25:11 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0

22.12.2017 16:39, Kevin Wolf wrote:
Am 12.12.2017 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
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>
qcow2_invalidate_cache() calls qcow2_close() first, so why are there
still any bitmaps loaded? Isn't this a bug? Do we leak bitmaps when a
qcow2 image is closed?


Interesting point.

Now persistent dirty bitmaps are released at the end of bdrv_inactivate_recurse,
which is not called here. It was a separate patch

commit 615b5dcf2decbc5f0abb512d13d7e5db2385fa23
Author: Vladimir Sementsov-Ogievskiy <address@hidden>
Date:   Wed Jun 28 15:05:30 2017 +0300

    block: release persistent bitmaps on inactivate

May be it is more correct to release them immediately after storing, in
qcow2_inactivete. But it will not fix the issue, because qcow2_close will
call qcow2_inactivate only if (!(s->flags & BDRV_O_INACTIVE)), but it is
not the case.

or we can do like this, it fixes the new test:
  static void qcow2_close(BlockDriverState *bs)
      BDRVQcow2State *s = bs->opaque;
      /* else pre-write overlap checks in cache_destroy may crash */
      s->l1_table = NULL;

      if (!(s->flags & BDRV_O_INACTIVE)) {
+     bdrv_release_persistent_dirty_bitmaps(bs);

What do you think?

Best regards,

reply via email to

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