| Message ID | 20190531163202.162543-7-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2-bitmaps: rewrite reopening logic | expand |
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 > + * @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. 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. > +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)
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!
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 <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... > 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 <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! > 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?
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 <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... >> > > 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 <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! >> > > 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.
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. + * + * @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. + */ +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 "
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(-)