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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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 <vsementsov@virtuozzo.com>
>> ---
>>   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...

> 
>> + * @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 <jsnow@redhat.com>
> 
> (You can adjust the docs as you need to on further review, if any, and
> keep that RB. --js)
> 

OK, thank you!

-- 
Best regards,
Vladimir