Re: [Qemu-devel] [PATCH v20 13/30] block: new bdrv_reopen_bitmaps_rw interface

[John Snow <jsnow@redhat.com>]

On 06/02/2017 07:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add format driver handler, which should mark loaded read-only
> bitmaps as 'IN_USE' in the image and unset read_only field in
> corresponding BdrvDirtyBitmap's.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c                   | 17 +++++++++++++++++
>  include/block/block_int.h |  7 +++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 04af7697dc..161db9e32a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2946,12 +2946,16 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>  {
>      BlockDriver *drv;
>      BlockDriverState *bs;
> +    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);
> @@ -2965,6 +2969,19 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>      bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
>  
>      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_report_err(local_err);
> +        }
> +    }
>  }
>  
>  /*
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8d3724cce6..1dc6f2e90d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -380,6 +380,13 @@ 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);
> +

Hmm, do we need a new top-level hook for this? We already have
.bdrv_reopen_commit and .bdrv_reopen_prepare which inform the
blockdriver that a reopen event is occurring.

Can't we just amend qcow2_reopen_prepare and qcow2_reopen_commit to do
the necessary tasks of either:

(A) Flushing the bitmap in preparation for reopening as RO, or
(B) Writing in_use and removing the RO flag in preparation for reopening
as RW

>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> 


Well, design issues aside, I will at least confirm that I think this
patch should work as designed, and I will leave the design critiques to
Max and Kevin:

Reviewed-by: John Snow <jsnow@redhat.com>