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

[Max Reitz]

On 07.08.19 16:12, 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.
> +             */

It’s still my impression that this is absolutely fatal, because that
means the node isn’t actually writable, and that means the reopen
effectively failed.

But again, it doesn’t make things worse.

Assuming the RW -> RW transition is a no-op now (the previous patch
claims to handle that case):

Acked-by: Max Reitz <address@hidden>

> +            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,
>  };
>

signature.asc
Description: OpenPGP digital signature