[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persist
|
From: |
Vladimir Sementsov-Ogievskiy |
|
Subject: |
Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination |
|
Date: |
Fri, 5 Feb 2021 11:01:05 +0300 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 |
03.02.2021 16:00, Peter Krempa wrote:
Bitmap's source persistence is transported over the migration stream and
the destination mirrors it. In some cases the destination might want to
persist bitmaps which are not persistent on the source (e.g. the result
of merge of bitmaps from a number of layers on the source when migrating
into a squashed image) but currently it would need to create another set
of persistent bitmaps and merge them.
This adds 'dest-persistent' optional property to
'BitmapMigrationBitmapAlias' which when present overrides the bitmap
presence state from the source.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
migration/block-dirty-bitmap.c | 30 +++++++++++++++++++++++++++++-
qapi/migration.json | 7 ++++++-
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index b0403dd00c..1876c94c45 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -149,6 +149,9 @@ typedef struct DBMLoadState {
bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start
*/
+ bool has_dest_persistent;
+ bool dest_persistent;
+
/*
* cancelled
* Incoming migration is cancelled for some reason. That means that we
@@ -171,6 +174,10 @@ static DBMState dbm_state;
typedef struct AliasMapInnerBitmap {
char *string;
+
+ /* for destination's properties setting bitmap state */
+ bool has_dest_persistent;
+ bool dest_persistent;
} AliasMapInnerBitmap;
static void free_alias_map_inner_bitmap(void *amin_ptr)
@@ -289,6 +296,8 @@ static GHashTable *construct_alias_map(const
BitmapMigrationNodeAliasList *bbm,
for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
const BitmapMigrationBitmapAlias *bmba = bmbal->value;
const char *bmap_map_from, *bmap_map_to;
+ bool bmap_has_dest_persistent = false;
+ bool bmap_dest_persistent = false;
AliasMapInnerBitmap *bmap_inner;
if (strlen(bmba->alias) > UINT8_MAX) {
@@ -317,6 +326,9 @@ static GHashTable *construct_alias_map(const
BitmapMigrationNodeAliasList *bbm,
bmap_map_from = bmba->alias;
bmap_map_to = bmba->name;
+ bmap_has_dest_persistent = bmba->has_dest_persistent;
+ bmap_dest_persistent = bmba->dest_persistent;
+
if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
error_setg(errp, "The bitmap alias '%s'/'%s' is used
twice",
bmna->alias, bmba->alias);
@@ -326,6 +338,8 @@ static GHashTable *construct_alias_map(const
BitmapMigrationNodeAliasList *bbm,
bmap_inner = g_new0(AliasMapInnerBitmap, 1);
bmap_inner->string = g_strdup(bmap_map_to);
+ bmap_inner->has_dest_persistent = bmap_has_dest_persistent;
+ bmap_inner->dest_persistent = bmap_dest_persistent;
g_hash_table_insert(bitmaps_map,
g_strdup(bmap_map_from), bmap_inner);
@@ -798,6 +812,7 @@ static int dirty_bitmap_load_start(QEMUFile *f,
DBMLoadState *s)
uint32_t granularity = qemu_get_be32(f);
uint8_t flags = qemu_get_byte(f);
LoadBitmapState *b;
+ bool persistent;
if (s->cancelled) {
return 0;
@@ -822,7 +837,13 @@ static int dirty_bitmap_load_start(QEMUFile *f,
DBMLoadState *s)
return -EINVAL;
}
- if (flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT) {
+ if (s->has_dest_persistent) {
+ persistent = s->dest_persistent;
+ } else {
+ persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+ }
+
+ if (persistent) {
bdrv_dirty_bitmap_set_persistence(s->bitmap, true);
}
@@ -1087,6 +1108,8 @@ static int dirty_bitmap_load_header(QEMUFile *f,
DBMLoadState *s,
if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
const char *bitmap_name;
+ bool bitmap_has_dest_persistent = false;
+ bool bitmap_dest_persistent = false;
if (!qemu_get_counted_string(f, s->bitmap_alias)) {
error_report("Unable to read bitmap alias string");
@@ -1107,12 +1130,17 @@ static int dirty_bitmap_load_header(QEMUFile *f,
DBMLoadState *s,
}
bitmap_name = bmap_inner->string;
+ bitmap_has_dest_persistent = bmap_inner->has_dest_persistent;
+ bitmap_dest_persistent = bmap_inner->dest_persistent;
}
if (!s->cancelled) {
g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
+ s->has_dest_persistent = bitmap_has_dest_persistent;
+ s->dest_persistent = bitmap_dest_persistent;
+
/*
* bitmap may be NULL here, it wouldn't be an error if it is the
* first occurrence of the bitmap
diff --git a/qapi/migration.json b/qapi/migration.json
index d1d9632c2a..32e64dbce6 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -533,12 +533,17 @@
# @alias: An alias name for migration (for example the bitmap name on
# the opposite site).
#
+# @dest-persistent: If populated set the bitmap will be turned persistent
+# or transient depending on this parameter.
+# (since 6.0)
+#
Thinking of future: will we want modify other bitmap properties "in-flight"?
enabled? maybe granularity? Then we'll add additional dest-* properties. Not bad, but
probably better to make it nested, like
transform: { persistent: bool }
So, than we'll can add other properties:
transform: { *persistent: bool, *enable: bool, *granularity: bool }
Also, in code I see you just ignore dest-persistent if it is set on source. I think we
should either error-out, or support it. Why not to allow setup property change on source
alias-mapping? (and that's why I suggest "transform" which a bit more generic
for using both on source and target)
Also, I don't like duplication of AliasMapInnerBitmap in DBMLoadState. Could we
instead just add a pointer to bmap_inner, like this:
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 1876c94c45..93ae76734e 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -138,6 +138,14 @@ typedef struct LoadBitmapState {
bool enabled;
} LoadBitmapState;
+typedef struct AliasMapInnerBitmap {
+ char *string;
+
+ /* for destination's properties setting bitmap state */
+ bool has_dest_persistent;
+ bool dest_persistent;
+} AliasMapInnerBitmap;
+
/* State of the dirty bitmap migration (DBM) during load process */
typedef struct DBMLoadState {
uint32_t flags;
@@ -149,8 +157,7 @@ typedef struct DBMLoadState {
bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
- bool has_dest_persistent;
- bool dest_persistent;
+ AliasMapInnerBitmap *bmap_inner;
/*
* cancelled
@@ -172,14 +179,6 @@ typedef struct DBMState {
static DBMState dbm_state;
-typedef struct AliasMapInnerBitmap {
- char *string;
-
- /* for destination's properties setting bitmap state */
- bool has_dest_persistent;
- bool dest_persistent;
-} AliasMapInnerBitmap;
-
static void free_alias_map_inner_bitmap(void *amin_ptr)
{
AliasMapInnerBitmap *amin = amin_ptr;
@@ -837,8 +836,8 @@ static int dirty_bitmap_load_start(QEMUFile *f,
DBMLoadState *s)
return -EINVAL;
}
- if (s->has_dest_persistent) {
- persistent = s->dest_persistent;
+ if (s->bmap_inner && s->bmap_inner->has_dest_persistent) {
+ persistent = s->bmap_inner->dest_persistent;
} else {
persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
}
@@ -1108,8 +1107,6 @@ static int dirty_bitmap_load_header(QEMUFile *f,
DBMLoadState *s,
if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
const char *bitmap_name;
- bool bitmap_has_dest_persistent = false;
- bool bitmap_dest_persistent = false;
if (!qemu_get_counted_string(f, s->bitmap_alias)) {
error_report("Unable to read bitmap alias string");
@@ -1130,17 +1127,13 @@ static int dirty_bitmap_load_header(QEMUFile *f,
DBMLoadState *s,
}
bitmap_name = bmap_inner->string;
- bitmap_has_dest_persistent = bmap_inner->has_dest_persistent;
- bitmap_dest_persistent = bmap_inner->dest_persistent;
+ s->bmap_inner = bmap_inner;
}
if (!s->cancelled) {
g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
- s->has_dest_persistent = bitmap_has_dest_persistent;
- s->dest_persistent = bitmap_dest_persistent;
-
/*
* bitmap may be NULL here, it wouldn't be an error if it is the
* first occurrence of the bitmap
--
Best regards,
Vladimir