[Qemu-devel] [PATCH] block/qcow2: save dirty_bitmaps_loaded during reopen

[Qemu-devel] [PATCH] block/qcow2: save dirty_bitmaps_loaded during reopen

[Vladimir Sementsov-Ogievskiy]

This variable was introduced to handle reopens. We need it on the
following qcow2_do_open, to don't try load bitmaps again.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 549fee9b69..29f7eab576 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2178,6 +2178,7 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
     QDict *options;
     Error *local_err = NULL;
     int ret;
+    bool dirty_bitmaps_loaded = s->dirty_bitmaps_loaded;
 
     /*
      * Backing files are read-only which makes all of their metadata immutable,
@@ -2190,6 +2191,7 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
     qcow2_close(bs);
 
     memset(s, 0, sizeof(BDRVQcow2State));
+    s->dirty_bitmaps_loaded = dirty_bitmaps_loaded;
     options = qdict_clone_shallow(bs->options);
 
     flags &= ~BDRV_O_INACTIVE;
-- 
2.11.1

Re: [Qemu-devel] [PATCH] block/qcow2: save dirty_bitmaps_loaded during reopen

[Vladimir Sementsov-Ogievskiy]

08.06.2018 15:32, Vladimir Sementsov-Ogievskiy wrote:
> This variable was introduced to handle reopens. We need it on the
> following qcow2_do_open, to don't try load bitmaps again.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/qcow2.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 549fee9b69..29f7eab576 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2178,6 +2178,7 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>       QDict *options;
>       Error *local_err = NULL;
>       int ret;
> +    bool dirty_bitmaps_loaded = s->dirty_bitmaps_loaded;
>   
>       /*
>        * Backing files are read-only which makes all of their metadata immutable,
> @@ -2190,6 +2191,7 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>       qcow2_close(bs);
>   
>       memset(s, 0, sizeof(BDRVQcow2State));
> +    s->dirty_bitmaps_loaded = dirty_bitmaps_loaded;
>       options = qdict_clone_shallow(bs->options);
>   
>       flags &= ~BDRV_O_INACTIVE;

Haha, this breaks 169 test.. Because, logic around dirty_bitmaps_loaded, 
which is fixed here, actually breaks it.

context:

static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
                                       int flags, Error **errp)
{

[...]

     if (s->dirty_bitmaps_loaded) {
         /* It's some kind of reopen. There are no known cases where we 
need to
          * reload bitmaps in such a situation, so it's safer to skip them.
*
          * Moreover, if we have some readonly bitmaps and we are 
reopening for
          * rw we should reopen bitmaps correspondingly.
*/
         if (bdrv_has_readonly_bitmaps(bs) &&
             !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & 
BDRV_O_INACTIVE))
{
             qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err);
}
     } else {
         header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
         s->dirty_bitmaps_loaded = true;
     }

[...]

- First time, we go into "else" block, but there no bitmaps yet, as 
target is opened before source closed. On second time, we go into "if" 
block, but there is nothing to reopen.

I'll resend the patch, with additional fixing.

-- 
Best regards,
Vladimir