Re: [Qemu-block] [PATCH 4/5] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

06.03.2019 2:43, John Snow wrote:
> Since we now load all bitmaps into memory anyway, we can just truncate them
> in-memory and then flush them back to disk. Just in case, we will still check
> and enforce that this shortcut is valid -- i.e. that any bitmap described
> on-disk is indeed in-memory and can be modified.
> 
> If there are any inconsistent bitmaps, we refuse to allow the truncate as
> we do not actually load these bitmaps into memory, and it isn't safe or
> reasonable to attempt to truncate corrupted data.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>   block/qcow2.h        |  1 +
>   block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   block/qcow2.c        | 27 ++++++++++++++++++++-------
>   3 files changed, 63 insertions(+), 7 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 4c1fdfcbec..b9227a72cc 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -689,6 +689,7 @@ Qcow2BitmapInfoList 
> *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>                                    Error **errp);
>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error 
> **errp);
>   void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error 
> **errp);
>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 9373055d3b..24f280bccc 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, 
> Error **errp)
>       return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
>   }
>   
> +/* Checks to see if it's safe to resize bitmaps */
> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    Qcow2BitmapList *bm_list;
> +    Qcow2Bitmap *bm;
> +    int ret = 0;
> +
> +    if (s->nb_bitmaps == 0) {
> +        return 0;
> +    }
> +
> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> +                               s->bitmap_directory_size, false, errp);
> +    if (bm_list == NULL) {
> +        return -EINVAL;
> +    }
> +
> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
> +        if (bitmap == NULL) {
> +            /* We rely on all bitmaps being in-memory to be able to resize 
> them,
> +             * Otherwise, we'd need to resize them on disk explicitly */
> +            error_setg(errp, "Cannot resize qcow2 with persistent bitmaps 
> that "
> +                       "were not loaded into memory");
> +            ret = -ENOTSUP;
> +            goto out;
> +        }
> +
> +        /* The checks against readonly and busy are redundant, but certainly
> +         * do no harm. checks against inconsistent are crucial: */
> +        if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
> +            ret = -ENOTSUP;
> +            goto out;
> +        }
> +    }
> +
> +out:
> +    bitmap_list_free(bm_list);
> +    return ret;
> +}
> +
>   /* store_bitmap_data()
>    * Store bitmap to image, filling bitmap table accordingly.
>    */
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7fb2730f09..6fd9252494 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3472,7 +3472,7 @@ static int coroutine_fn 
> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>   {
>       BDRVQcow2State *s = bs->opaque;
>       uint64_t old_length;
> -    int64_t new_l1_size;
> +    int64_t new_l1_size, offset_be;
>       int ret;
>       QDict *options;
>   
> @@ -3498,10 +3498,8 @@ static int coroutine_fn 
> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>           goto fail;
>       }
>   
> -    /* cannot proceed if image has bitmaps */
> -    if (s->nb_bitmaps) {
> -        /* TODO: resize bitmaps in the image */
> -        error_setg(errp, "Can't resize an image which has bitmaps");
> +    /* Only certain persistent bitmaps can be resized: */
> +    if (qcow2_truncate_bitmaps_check(bs, errp)) {
>           ret = -ENOTSUP;
>           goto fail;
>       }
> @@ -3702,9 +3700,9 @@ static int coroutine_fn 
> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>       bs->total_sectors = offset / BDRV_SECTOR_SIZE;
>   
>       /* write updated header.size */
> -    offset = cpu_to_be64(offset);
> +    offset_be = cpu_to_be64(offset);
>       ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
> -                           &offset, sizeof(uint64_t));
> +                           &offset_be, sizeof(uint64_t));
>       if (ret < 0) {
>           error_setg_errno(errp, -ret, "Failed to update the image size");
>           goto fail;
> @@ -3719,6 +3717,21 @@ static int coroutine_fn 
> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>       if (ret < 0) {
>           goto fail;
>       }
> +
> +    /* Flush bitmaps */
> +    if (s->nb_bitmaps) {
> +        Error *local_err = NULL;
> +
> +        /* Usually, bitmaps get resized after this call.
> +         * Force it earlier so we can make the metadata consistent. */
> +        bdrv_dirty_bitmap_truncate(bs, offset);
> +        qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
> +        if (local_err) {
> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +    }

Why to flush after resize? Bitmaps will be IN_USE in the image anyway...

Could we implement resize without flush first,  it would be one patch + test? 
And then consider
flushing in separate?

> +
>       ret = 0;
>   fail:
>       qemu_co_mutex_unlock(&s->lock);
> 


-- 
Best regards,
Vladimir