[Top][All Lists]

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

Re: [PATCH 1/2] qcow2: Release read-only bitmaps when inactivated

From: Eric Blake
Subject: Re: [PATCH 1/2] qcow2: Release read-only bitmaps when inactivated
Date: Thu, 30 Jul 2020 09:54:29 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

On 7/30/20 7:02 AM, Max Reitz wrote:
During migration, we release all bitmaps after storing them on disk, as
long as they are (1) stored on disk, (2) not read-only, and (3)

(2) seems arbitrary, though.  The reason we do not release them is
because we do not write them, as there is no need to; and then we just
forget about all bitmaps that we have not written to the file.  However,
read-only persistent bitmaps are still in the file and in sync with
their in-memory representation, so we may as well release them just like
any R/W bitmap that we have updated.

It leads to actual problems, too: After migration, letting the source
continue may result in an error if there were any bitmaps on read-only
nodes (such as backing images), because those have not been released by
bdrv_inactive_all(), but bdrv_invalidate_cache_all() attempts to reload
them (which fails, because they are still present in memory).

I think our alternatives are ensuring no bitmaps are in memory so that reloading the RO bitmap from the file succeeds (which then hits the earlier question about whether releasing ALL bitmaps affects libvirt's ability to query which bitmaps were on the source, but makes reloading on the destination easy), or teaching the reload to special-case a RO bitmap from the disk that is already in memory (either to make the reload a graceful no-op instead of an error that it was already loaded, or to go one step further and validate whether the contents in memory match the contents reloaded from disk). If I understand your patch, you went with the first of these alternatives. And since Peter was able to test that it fixed the libvirt scenario, I'm okay with the approach you took, although I would love a second opinion from Virtuozzo folks.

Signed-off-by: Max Reitz <mreitz@redhat.com>
  block/qcow2-bitmap.c | 23 +++++++++++++++++++----
  1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 1f38806ca6..8c34b2aef7 100644
--- a/block/qcow2-bitmap.c

@@ -1641,6 +1654,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
      if (release_stored) {
          QSIMPLEQ_FOREACH(bm, bm_list, entry) {
              if (bm->dirty_bitmap == NULL) {
@@ -1651,13 +1665,14 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,

Moving the label was an interesting change; I had to look at the file in context to see the real effect: basically, you now reach the line:


for the set of persistent RO bitmaps that were previously ignored.

Reviewed-by: Eric Blake <eblake@redhat.com>

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

reply via email to

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