Re: [Qemu-devel] [PATCH v4 10/10] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

[John Snow]

On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> The only reason I can imagine for this strange code at the very-end of
> bdrv_reopen_commit is the fact that bs->read_only updated after
> calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same
> time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong
> check for being writable, when actually it only need writable file
> child not self.
> 
> So, as it's fixed, let's move things to correct place.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  include/block/block_int.h |  6 ------
>  block.c                   | 19 -------------------
>  block/qcow2.c             | 15 ++++++++++++++-
>  3 files changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 3aa1e832a8..18a1e81194 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -531,12 +531,6 @@ struct BlockDriver {
>                               uint64_t parent_perm, uint64_t parent_shared,
>                               uint64_t *nperm, uint64_t *nshared);
>  
> -    /**
> -     * Bitmaps should be marked as 'IN_USE' in the image on reopening image
> -     * as rw. This handler should realize it. It also should unset readonly
> -     * field of BlockDirtyBitmap's in case of success.
> -     */
> -    int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
>      bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
>                                              const char *name,
>                                              uint32_t granularity,
> diff --git a/block.c b/block.c
> index d59f9f97cb..395bc88045 100644
> --- a/block.c
> +++ b/block.c
> @@ -3925,16 +3925,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>      BlockDriver *drv;
>      BlockDriverState *bs;
>      BdrvChild *child;
> -    bool old_can_write, new_can_write;
>  
>      assert(reopen_state != NULL);
>      bs = reopen_state->bs;
>      drv = bs->drv;
>      assert(drv != NULL);
>  
> -    old_can_write =
> -        !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
> -
>      /* If there are any driver level actions to take */
>      if (drv->bdrv_reopen_commit) {
>          drv->bdrv_reopen_commit(reopen_state);
> @@ -3978,21 +3974,6 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>      }
>  
>      bdrv_refresh_limits(bs, NULL);
> -
> -    new_can_write =
> -        !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
> -    if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
> -        Error *local_err = NULL;
> -        if (drv->bdrv_reopen_bitmaps_rw(bs, &local_err) < 0) {
> -            /* This is not fatal, bitmaps just left read-only, so all 
> following
> -             * writes will fail. User can remove read-only bitmaps to unblock
> -             * writes.
> -             */
> -            error_reportf_err(local_err,
> -                              "%s: Failed to make dirty bitmaps writable: ",
> -                              bdrv_get_node_name(bs));
> -        }
> -    }
>  }
>  
>  /*
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 5c1187e2f9..9e6210c282 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1828,6 +1828,20 @@ fail:
>  static void qcow2_reopen_commit(BDRVReopenState *state)
>  {
>      qcow2_update_options_commit(state->bs, state->opaque);
> +    if (state->flags & BDRV_O_RDWR) {
> +        Error *local_err = NULL;
> +
> +        if (qcow2_reopen_bitmaps_rw(state->bs, &local_err) < 0) {
> +            /*
> +             * This is not fatal, bitmaps just left read-only, so all 
> following
> +             * writes will fail. User can remove read-only bitmaps to unblock
> +             * writes or retry reopen.
> +             */
> +            error_reportf_err(local_err,
> +                              "%s: Failed to make dirty bitmaps writable: ",
> +                              bdrv_get_node_name(state->bs));
> +        }
> +    }
>      g_free(state->opaque);
>  }
>  
> @@ -5229,7 +5243,6 @@ BlockDriver bdrv_qcow2 = {
>      .bdrv_detach_aio_context  = qcow2_detach_aio_context,
>      .bdrv_attach_aio_context  = qcow2_attach_aio_context,
>  
> -    .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
>      .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
>      .bdrv_remove_persistent_dirty_bitmap = 
> qcow2_remove_persistent_dirty_bitmap,
>  };
> 

Makes sense to me -- bitmap reopen should happen when a specific driver
needs to reopen. It was a weird top-level driver callback.

Reviewed-by: John Snow <address@hidden>

Max, can you please review this one as well?