[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 6/9] block/qcow2-bitmap: do not
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 6/9] block/qcow2-bitmap: do not remove bitmaps on reopen-ro |
Date: |
Tue, 18 Jun 2019 14:38:48 +0000 |
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