mirrors/qemu
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

block/qcow2-bitmap: do not remove bitmaps on reopen-ro

Browse Source 
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
260 iotests still. Reopening bitmaps rw will be fixed in the following
patches.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190927122355.7344-7-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
This commit is contained in:
Vladimir Sementsov-Ogievskiy 2019-09-27 15:23:52 +03:00 committed by John Snow
parent bd429a884c
commit 644ddbb754
3 changed files with 37 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
block/qcow2-bitmap.c
View File

@ -1440,7 +1440,32 @@ out:
return ret;
}
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
/*
* qcow2_store_persistent_dirty_bitmaps
*
* Stores persistent BdrvDirtyBitmap objects.
*
* @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. Moreover, 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;
@ -1551,21 +1576,15 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
g_free(tb);
}
if (release_stored) {
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;
}
bdrv_release_dirty_bitmap(bm->dirty_bitmap);
}
}
success:
bitmap_list_free(bm_list);
@ -1592,7 +1611,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;

2
block/qcow2.c
View File

@ -2503,7 +2503,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 "

3
block/qcow2.h
View File

@ -742,7 +742,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_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
const char *name,