14.11.2017 02:32, John Snow wrote:
On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
Make it possible to set bitmap 'frozen' without a successor.
This is needed to protect the bitmap during outgoing bitmap postcopy
migration.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
include/block/dirty-bitmap.h | 1 +
block/dirty-bitmap.c | 22 ++++++++++++++++++++--
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index a9e2a92e4f..ae6d697850 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -39,6 +39,7 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState
*bs);
uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool frozen);
const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 7578863aa1..67fc6bd6e0 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
QemuMutex *mutex;
HBitmap *bitmap; /* Dirty bitmap implementation */
HBitmap *meta; /* Meta dirty bitmap */
+ bool frozen; /* Bitmap is frozen, it can't be modified
+ through QMP */
I hesitate, because this now means that we have two independent bits of
state we need to update and maintain consistency with.
it was your proposal)))
https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01172.html
"
Your new use case here sounds like Frozen to me, but it simply does not
have an anonymous successor to force it to be recognized as "frozen." We
can add a `bool protected` or `bool frozen` field to force recognition
of this status and adjust the json documentation accordingly.
I think then we'd have four recognized states:
FROZEN: Cannot be reset/deleted. Bitmap is in-use by a block job or
other internal process. Bitmap is otherwise ACTIVE.
DISABLED: Not recording any writes (by choice.)
READONLY: Not able to record any writes (by necessity.)
ACTIVE: Normal bitmap status.
Sound right?
"
Before:
Frozen: "Bitmap has a successor and is no longer itself recording
writes, though we are guaranteed to have a successor doing so on our
behalf."
After:
Frozen: "Bitmap may or may not have a successor, but it is disabled with
an even more limited subset of tasks than a traditionally disabled bitmap."
This changes the meaning of "frozen" a little, and I am not sure that is
a problem as such, but it does make the interface seem a little
"fuzzier" than it did prior.
BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
char *name; /* Optional non-empty unique ID */
int64_t size; /* Size of the bitmap, in bytes */
@@ -183,13 +185,22 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap
*bitmap)
/* Called with BQL taken. */
bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
{
- return bitmap->successor;
+ return bitmap->frozen;
+}
Are there any cases where we will be unfrozen, but actually have a
successor now? If so, under what circumstances does that happen?
+
+/* Called with BQL taken. */
+void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool frozen)
+{
+ qemu_mutex_lock(bitmap->mutex);
+ assert(bitmap->successor == NULL);
+ bitmap->frozen = frozen;
+ qemu_mutex_unlock(bitmap->mutex);
}
OK, so we can only "set frozen" (or unset) if we don't have a successor.
/* Called with BQL taken. */
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
{
- return !(bitmap->disabled || bitmap->successor);
+ return !(bitmap->disabled || (bitmap->successor != NULL));
}
I guess this just makes 'successor' more obviously non-boolean, which is
fine.
/* Called with BQL taken. */
@@ -234,6 +245,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
/* Install the successor and freeze the parent */
bitmap->successor = child;
+ bitmap->frozen = true;
return 0;
}
@@ -266,6 +278,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
name = bitmap->name;
bitmap->name = NULL;
successor->name = name;
+ assert(bitmap->frozen);
+ bitmap->frozen = false;
bitmap->successor = NULL;
successor->persistent = bitmap->persistent;
bitmap->persistent = false;
@@ -298,6 +312,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState
*bs,
return NULL;
}
bdrv_release_dirty_bitmap(bs, successor);
+ assert(parent->frozen);
+ parent->frozen = false;
parent->successor = NULL;
return parent;
@@ -439,6 +455,8 @@ void bdrv_dirty_bitmap_release_successor(BlockDriverState
*bs,
if (parent->successor) {
bdrv_release_dirty_bitmap_locked(bs, parent->successor);
+ assert(parent->frozen);
+ parent->frozen = false;
parent->successor = NULL;
}
Tentatively:
Reviewed-by: John Snow <js...@redhat.com>
Fam, any thoughts?
--John
--
Best regards,
Vladimir