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 <vsement...@virtuozzo.com>
> ---
> 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 <js...@redhat.com>
Max, can you please review this one as well?
