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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]

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 <vsementsov@virtuozzo.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>  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
present.)

Max

> +        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) {
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]