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

[Vladimir Sementsov-Ogievskiy]

06.03.2019 18:52, Vladimir Sementsov-Ogievskiy wrote:
> 06.03.2019 18:41, John Snow wrote:
>>
>>
>> On 3/6/19 10:33 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 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?
>>>
>>
>> If you don't flush it to disk, all calls to retrieve the bm_list begin
>> failing because of inconsistent metadata.
>>
>> Future calls to add bitmaps, remove bitmaps, etc will start failing. It
>> seemed safest and easiest to just flush the whole suite to disk. I am
>> trying to avoid premature optimizations at this stage, as I believe
>> resizes will be very infrequent events.
> 
> Hmmm, understand.
> 
> But I think, if we are going to allow resizes, it definitely means that 
> IN_USE flag covers not
> only bitmap data, but size field too. And we must write it to specification. 
> And it is right
> even if you flush here, as resize may fail after your flush, and actual disk 
> size will remain
> unchanged when bitmaps are flushed with new size.

oops, you flush only on success. But, if flush fails, what would be the state 
of qcow2 image?
resized or not?

> 
> And if size is covered by IN_USE, we should ignore it for IN_USE bitmaps (for 
> IN_USE by us too).
> And it means, that we need check it only on qcow2_open. And not on other 
> cases.
> 


-- 
Best regards,
Vladimir