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 <vsement...@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 <js...@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
