Re: [Qemu-devel] [PATCH v2 6/9] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[Vladimir Sementsov-Ogievskiy]

18.06.2019 17:30, John Snow wrote:
> 
> 
> On 6/3/19 6:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 01.06.2019 3:06, John Snow wrote:
>>>
>>>
>>> On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
>>>> readonly. But the latter don't work, as
>>>> qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
>>>> It's OK for inactivation but bad idea for reopen-ro. And this leads to
>>>> the following bug:
>>>>
>>>> Assume we have persistent bitmap 'bitmap0'.
>>>> Create external snapshot
>>>>     bitmap0 is stored and therefore removed
>>>> Commit snapshot
>>>>     now we have no bitmaps
>>>> Do some writes from guest (*)
>>>>     they are not marked in bitmap
>>>> Shutdown
>>>> Start
>>>>     bitmap0 is loaded as valid, but it is actually broken! It misses
>>>>     writes (*)
>>>> Incremental backup
>>>>     it will be inconsistent
>>>>
>>>> So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
>>>> reopening bitmaps to rw is broken too, so the whole scenario will not
>>>> work after this patch and we can't enable corresponding test cases in
>>>> 255 iotests still. Reopening bitmaps rw will be fixed in the following
>>>> patches.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>> ---
>>>>    block/qcow2.h        |  3 ++-
>>>>    block/qcow2-bitmap.c | 46 +++++++++++++++++++++++++++++---------------
>>>>    block/qcow2.c        |  2 +-
>>>>    3 files changed, 34 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>> index 88a2030f54..4c8435141b 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -734,7 +734,8 @@ Qcow2BitmapInfoList 
>>>> *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>>>                                                    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_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>> +                                          bool release_stored, Error 
>>>> **errp);
>>>>    int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>>>    bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>>                                          const char *name,
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> index fbeee37243..25b1e069a7 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -1432,7 +1432,29 @@ fail:
>>>>        bitmap_list_free(bm_list);
>>>>    }
>>>>    
>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error 
>>>> **errp)
>>>> +/*
>>>> + * qcow2_store_persistent_dirty_bitmaps
>>>> + *
>>>> + * Stores persistent BdrvDirtyBitmap's.
>>>> + *
>>>
>>> No apostrophe for plural's
>>
>> I always do so, as it seems strange to me to append 's' to identifiers..
>> Should I write it BdrvDirtyBitmaps? It sounds as some other identifier...
>>
> 
> This is a recurring problem with English. The term "CD's" is in common
> usage for this reason, even though it's grammatically incorrect.
> Honestly, I don't have an answer for you, but you could try to avoid it:
> 
> "Stores persistent BdrvDirtyBitmap objects"
> 
> It's clunkier, but it avoids adding a plural to an identifier. In marked
> up text, it's not uncommon to see `BdrvDirtyBitmap`s, but that would
> look silly here.
> 
>>>
>>>> + * @release_stored: if true, release BdrvDirtyBitmap's after storing to 
>>>> the
>>>> + * image. This is used in two cases, both via qcow2_inactivate:
>>>> + * 1. bdrv_close: It's correct to remove bitmaps on close.
>>>> + * 2. migration: If bitmaps are migrated through migration channel via
>>>> + *    'dirty-bitmaps' migration capability they are not handled by this 
>>>> code.
>>>> + *    Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
>>>> + *    invalidation.
>>>> + *
>>>> + * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
>>>> + * inactivation means that we lose control on disk, and therefore on 
>>>> bitmaps,
>>>> + * we should sync them and do not touch more.
>>>> + *
>>>> + * Contrariwise, we don't want to release any bitmaps on just 
>>>> reopen-to-ro,
>>>> + * when we need to store them, as image is still under our control, and 
>>>> it's
>>>> + * good to keep all the bitmaps in read-only mode.
>>>> + */
>>>
>>> I have to admit that 'Contrariwise' is not an everyday term for me. You
>>> should keep it in here just for fun, in my opinion.
>>
>> Ahaha, I've just used it in my previous reply.
>>
>>>
>>> Regarding "it's good to keep all the bitmaps in read-only mode":
>>> More directly, keeping them read-only is correct because this is what
>>> would happen if we opened the node readonly to begin with, and whether
>>> we opened directly or reopened to that state shouldn't matter for the
>>> state we get afterward.
>>
>> Agree, this is better reasoning.
>>
>>>
>>>> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>> +                                          bool release_stored, Error 
>>>> **errp)
>>>>    {
>>>>        BdrvDirtyBitmap *bitmap;
>>>>        BDRVQcow2State *s = bs->opaque;
>>>> @@ -1545,20 +1567,14 @@ void 
>>>> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>>>            g_free(tb);
>>>>        }
>>>>    
>>>> -    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>>> -        /* For safety, we remove bitmap after storing.
>>>> -         * We may be here in two cases:
>>>> -         * 1. bdrv_close. It's ok to drop bitmap.
>>>> -         * 2. inactivation. It means migration without 'dirty-bitmaps'
>>>> -         *    capability, so bitmaps are not marked with
>>>> -         *    BdrvDirtyBitmap.migration flags. It's not bad to drop them 
>>>> too,
>>>> -         *    and reload on invalidation.
>>>> -         */
>>>> -        if (bm->dirty_bitmap == NULL) {
>>>> -            continue;
>>>> -        }
>>>> +    if (release_stored) {
>>>> +        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>>> +            if (bm->dirty_bitmap == NULL) {
>>>> +                continue;
>>>> +            }
>>>>    
>>>> -        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>>>> +            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>>>> +        }
>>>>        }
>>>>    
>>>>    success:
>>>> @@ -1586,7 +1602,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, 
>>>> Error **errp)
>>>>        BdrvDirtyBitmap *bitmap;
>>>>        Error *local_err = NULL;
>>>>    
>>>> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
>>>> +    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
>>>>        if (local_err != NULL) {
>>>>            error_propagate(errp, local_err);
>>>>            return -EINVAL;
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index f2cb131048..02d8ce7534 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -2344,7 +2344,7 @@ static int qcow2_inactivate(BlockDriverState *bs)
>>>>        int ret, result = 0;
>>>>        Error *local_err = NULL;
>>>>    
>>>> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
>>>> +    qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
>>>>        if (local_err != NULL) {
>>>>            result = -EINVAL;
>>>>            error_reportf_err(local_err, "Lost persistent bitmaps during "
>>>>
>>>
>>> code:
>>> Reviewed-by: John Snow <address@hidden>
>>>
>>> (You can adjust the docs as you need to on further review, if any, and
>>> keep that RB. --js)
>>>
>>
>> OK, thank you!
>>
> 
> I'll get back to the rest of this soon, it looks like you haven't gotten
> review on the core block layer pieces, or maybe I've missed it if you have?
> 

Hmm, no, I haven't.. Seems I forget about these series, it should have been 
pinged
several days ago.

-- 
Best regards,
Vladimir