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

[John Snow]

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?