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 <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... >> > > 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 <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! >> > > 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