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

[Eric Blake <eblake@redhat.com>]

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)
> consistent.
> 
> (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,
>           g_free(tb);
>       }
>   
> +success:
>       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,
>           }
>       }
>   
> -success:

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:

bdrv_release_dirty_bitmap(bm->dirty_bitmap);

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