aboutsummaryrefslogtreecommitdiff
path: root/block
diff options
context:
space:
mode:
authorVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>2019-09-27 15:23:52 +0300
committerJohn Snow <jsnow@redhat.com>2019-10-17 17:02:32 -0400
commit644ddbb7540cfb69941f33629281819f53770666 (patch)
tree4b418533bc83ae4ae272c888dc1256dd41b87d47 /block
parentbd429a884cebde851e0fcfd144a8f8ca7185bd09 (diff)
downloadqemu-644ddbb7540cfb69941f33629281819f53770666.zip
qemu-644ddbb7540cfb69941f33629281819f53770666.tar.gz
qemu-644ddbb7540cfb69941f33629281819f53770666.tar.bz2
block/qcow2-bitmap: do not remove bitmaps on reopen-ro
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>
Diffstat (limited to 'block')
-rw-r--r--block/qcow2-bitmap.c49
-rw-r--r--block/qcow2.c2
-rw-r--r--block/qcow2.h3
3 files changed, 37 insertions, 17 deletions
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index ebc1afc..f7dfb40 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -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,20 +1576,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(bm->dirty_bitmap);
+ bdrv_release_dirty_bitmap(bm->dirty_bitmap);
+ }
}
success:
@@ -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;
diff --git a/block/qcow2.c b/block/qcow2.c
index 7062ecc..53a0257 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -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 "
diff --git a/block/qcow2.h b/block/qcow2.h
index 23a9898..5cccd87 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -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,