[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PULL 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro
From: |
John Snow |
Subject: |
[PULL 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro |
Date: |
Fri, 11 Oct 2019 17:25:45 -0400 |
From: Vladimir Sementsov-Ogievskiy <address@hidden>
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 <address@hidden>
Reviewed-by: John Snow <address@hidden>
Message-id: address@hidden
Signed-off-by: John Snow <address@hidden>
---
block/qcow2.h | 3 ++-
block/qcow2-bitmap.c | 49 ++++++++++++++++++++++++++++++--------------
block/qcow2.c | 2 +-
3 files changed, 37 insertions(+), 17 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index b3398a13c20..8d293f2b64e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -739,7 +739,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,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index ebc1afccd3d..f7dfb40256e 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 43a6cc423de..7ab1aad9779 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2359,7 +2359,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 "
--
2.21.0
- [PULL 04/19] block/qcow2: proper locking on bitmap add/remove paths, (continued)
- [PULL 04/19] block/qcow2: proper locking on bitmap add/remove paths, John Snow, 2019/10/11
- [PULL 05/19] block/dirty-bitmap: drop meta, John Snow, 2019/10/11
- [PULL 07/19] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex, John Snow, 2019/10/11
- [PULL 06/19] block/dirty-bitmap: add bs link, John Snow, 2019/10/11
- [PULL 08/19] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next, John Snow, 2019/10/11
- [PULL 09/19] block: switch reopen queue from QSIMPLEQ to QTAILQ, John Snow, 2019/10/11
- [PULL 10/19] block: reverse order for reopen commits, John Snow, 2019/10/11
- [PULL 11/19] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW, John Snow, 2019/10/11
- [PULL 12/19] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps, John Snow, 2019/10/11
- [PULL 13/19] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint(), John Snow, 2019/10/11
- [PULL 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro,
John Snow <=
- [PULL 16/19] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw, John Snow, 2019/10/11
- [PULL 15/19] iotests: add test 260 to check bitmap life after snapshot + commit, John Snow, 2019/10/11
- [PULL 17/19] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit, John Snow, 2019/10/11
- [PULL 18/19] MAINTAINERS: Add Vladimir as a reviewer for bitmaps, John Snow, 2019/10/11
- [PULL 19/19] dirty-bitmaps: remove deprecated autoload parameter, John Snow, 2019/10/11
- Re: [PULL 00/19] Bitmaps patches, Peter Maydell, 2019/10/14