Re: [Qemu-block] [Qemu-devel] [PATCH 3/6] bloc/qcow2: drop dirty_bitmaps_loaded state variable

[John Snow]

On 06/26/2018 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> This variable doesn't work as it should, because it is actually cleared
> in qcow2_co_invalidate_cache() by memset(). Drop it, as the following
> patch will introduce new behavior.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/qcow2.h |  1 -
>  block/qcow2.c | 19 ++-----------------
>  2 files changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 01b5250415..c9df19b6e6 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -294,7 +294,6 @@ typedef struct BDRVQcow2State {
>      uint32_t nb_bitmaps;
>      uint64_t bitmap_directory_size;
>      uint64_t bitmap_directory_offset;
> -    bool dirty_bitmaps_loaded;
>  
>      int flags;
>      int qcow_version;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 46194a33ca..0044ff58e7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1149,7 +1149,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>      uint64_t ext_end;
>      uint64_t l1_vm_state_index;
>      bool update_header = false;
> -    bool header_updated = false;
>  
>      ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
>      if (ret < 0) {
> @@ -1488,23 +1487,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>          s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>      }
>  
> -    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;
> +    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
> +        update_header = false;
>      }
> -    update_header = update_header && !header_updated;
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>          ret = -EINVAL;
> 

Usually I'd like to see the new behavior introduced before removing the
old, imperfect solution... (it doesn't make any tests fail!? ...)

At this point in the series I just have to trust that you will fix it to
be better :)

--js