* [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter
@ 2020-07-16 13:53 Max Reitz
2020-07-16 13:53 ` [PATCH v2 1/3] " Max Reitz
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Max Reitz @ 2020-07-16 13:53 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Juan Quintela,
John Snow, qemu-devel, Dr . David Alan Gilbert, Peter Krempa,
Max Reitz
RFC v1: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00912.html
RFC v2: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00915.html
v1: https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg09792.html
Branch: https://github.com/XanClic/qemu.git migration-bitmap-mapping-v2
Branch: https://git.xanclic.moe/XanClic/qemu.git migration-bitmap-mapping-v2
Based-on: <20200626130658.76498-1-vsementsov@virtuozzo.com>
(“migration/block-dirty-bitmap: fix add_bitmaps_to_list”)
Hi,
This new migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.
This way, management tools can use different node names on the source
and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).
v2:
- Dropped what used to be patch 1 (the memleak fix), I see the exact
same fix has been sent concurrently and has been merged as
9728ebfb77f0159f4
- Patch 1:
- Changed documentation to clarify the default behavior
- s/5.1/5.2/
- Dropped dead assignment
- Fixed bitmaps_map memleak
- Assert that the bs_name given to add_bitmaps_to_list() must be the
BDS’s node name if an alias_map is given
- On the source side, unmapped bitmaps are simply dropped without
error
- On the destination side, unmapped aliases still result in errors
(see patch 1 for a short explanation on my reasoning)
- Fixed a bug in qmp_query_migrate_parameters(): We have to clone
s->parameters.block_bitmap_mapping, not
params->block_bitmap_mapping, or the latter will just stay NULL (and
so qmp_query_migrate_parameters() won’t return a result for the
block-bitmap-mapping)
- Emit the mapping through HMP’s “info migrate_parameters”
- Return an error when trying to set the mapping through HMP (instead
of just crashing because of an “assert(0)” signalling an unhandled
migration parameter)
- Patch 3:
- Type alias for BlockBitmapMapping
- Check the result of “info migrate_parameters” whenever setting the
block-bitmap-mapping (just to test the new formatting code)
- Catch the qemu.machine.AbnormalShutdown exception on the destination
VM whenever the migration is expected to fail
(necessary since commit ef5d474472426eda6abf81)
- Cases where we don’t set up a mapping for some bitmap on the source
are now expected to succeed (without the bitmap being migrated)
git-backport-diff against v1:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/3:[0117] [FC] 'migration: Add block-bitmap-mapping parameter'
002/3:[----] [--] 'iotests.py: Add wait_for_runstate()'
003/3:[0202] [FC] 'iotests: Test node/bitmap aliases during migration'
Max Reitz (3):
migration: Add block-bitmap-mapping parameter
iotests.py: Add wait_for_runstate()
iotests: Test node/bitmap aliases during migration
qapi/migration.json | 92 +++++-
migration/migration.h | 3 +
migration/block-dirty-bitmap.c | 373 ++++++++++++++++++++----
migration/migration.c | 30 ++
monitor/hmp-cmds.c | 30 ++
tests/qemu-iotests/300 | 511 +++++++++++++++++++++++++++++++++
tests/qemu-iotests/300.out | 5 +
tests/qemu-iotests/group | 1 +
tests/qemu-iotests/iotests.py | 4 +
9 files changed, 994 insertions(+), 55 deletions(-)
create mode 100755 tests/qemu-iotests/300
create mode 100644 tests/qemu-iotests/300.out
--
2.26.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] migration: Add block-bitmap-mapping parameter
2020-07-16 13:53 [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter Max Reitz
@ 2020-07-16 13:53 ` Max Reitz
2020-07-20 16:31 ` Vladimir Sementsov-Ogievskiy
2020-07-22 9:06 ` Dr. David Alan Gilbert
2020-07-16 13:53 ` [PATCH v2 2/3] iotests.py: Add wait_for_runstate() Max Reitz
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Max Reitz @ 2020-07-16 13:53 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Juan Quintela,
John Snow, qemu-devel, Dr . David Alan Gilbert, Peter Krempa,
Max Reitz
This migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.
This way, management tools can use different node and bitmap names on
the source and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
Vladimir noted in v1 that it would be better to ignore bitmaps whose
names aren't mapped, or that are on nodes whose names aren't mapped.
One of the reasons he gave was that bitmap migration is inherently a
form of postcopy migration, and we should not break the target when it
is already running because of a bitmap issue.
So in this version, I changed the behavior to just ignore bitmaps
without a mapping on the source side. On the destination, however, I
kept it an error when an incoming bitmap or node alias is unknown.
This is in violation of Vladimir's (rightful) reasoning that such
errors should never break the already running target, but I decided to
do so anyway for a couple of reasons:
- Ignoring unmapped bitmaps on the source is trivial: We just have to
not put them into the migration stream.
On the destination, it isn't so easy: We (I think) would have to
modify the code to actively skip the bitmap in the stream.
(On the other hand, erroring out is always easy.)
- Incoming bitmaps with unknown names are already an error before this
series. So this isn't introducing new breakage.
- I think it makes sense to drop all bitmaps you don't want to migrate
(or implicitly drop them by just not specifying them if you don't care
about them) on the source. I can't imagine a scenario where it would
be really useful if the destination could silently drop unknown
bitmaps. Unknown bitmaps should just be dropped on the source.
- Choosing to make it an error now doesn't prevent us from relaxing that
restriction in the future.
---
qapi/migration.json | 92 +++++++-
migration/migration.h | 3 +
migration/block-dirty-bitmap.c | 373 ++++++++++++++++++++++++++++-----
migration/migration.c | 30 +++
monitor/hmp-cmds.c | 30 +++
5 files changed, 473 insertions(+), 55 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index d5000558c6..1b0fbcef96 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -507,6 +507,44 @@
'data': [ 'none', 'zlib',
{ 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
+##
+# @BitmapMigrationBitmapAlias:
+#
+# @name: The name of the bitmap.
+#
+# @alias: An alias name for migration (for example the bitmap name on
+# the opposite site).
+#
+# Since: 5.1
+##
+{ 'struct': 'BitmapMigrationBitmapAlias',
+ 'data': {
+ 'name': 'str',
+ 'alias': 'str'
+ } }
+
+##
+# @BitmapMigrationNodeAlias:
+#
+# Maps a block node name and the bitmaps it has to aliases for dirty
+# bitmap migration.
+#
+# @node-name: A block node name.
+#
+# @alias: An alias block node name for migration (for example the
+# node name on the opposite site).
+#
+# @bitmaps: Mappings for the bitmaps on this node.
+#
+# Since: 5.1
+##
+{ 'struct': 'BitmapMigrationNodeAlias',
+ 'data': {
+ 'node-name': 'str',
+ 'alias': 'str',
+ 'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
+ } }
+
##
# @MigrationParameter:
#
@@ -641,6 +679,21 @@
# will consume more CPU.
# Defaults to 1. (Since 5.0)
#
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+# aliases for the purpose of dirty bitmap migration. Such
+# aliases may for example be the corresponding names on the
+# opposite site.
+# The mapping must be one-to-one, and on the destination also
+# complete: On the source, bitmaps on nodes where either the
+# bitmap or the node is not mapped will be ignored. In
+# contrast, on the destination, receiving a bitmap (by alias)
+# from a node (by alias) when either alias is not mapped will
+# result in an error.
+# By default (when this parameter has never been set), bitmap
+# names are mapped to themselves. Nodes are mapped to their
+# block device name if there is one, and to their node name
+# otherwise. (Since 5.2)
+#
# Since: 2.4
##
{ 'enum': 'MigrationParameter',
@@ -655,7 +708,8 @@
'multifd-channels',
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
- 'multifd-zlib-level' ,'multifd-zstd-level' ] }
+ 'multifd-zlib-level' ,'multifd-zstd-level',
+ 'block-bitmap-mapping' ] }
##
# @MigrateSetParameters:
@@ -781,6 +835,21 @@
# will consume more CPU.
# Defaults to 1. (Since 5.0)
#
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+# aliases for the purpose of dirty bitmap migration. Such
+# aliases may for example be the corresponding names on the
+# opposite site.
+# The mapping must be one-to-one, and on the destination also
+# complete: On the source, bitmaps on nodes where either the
+# bitmap or the node is not mapped will be ignored. In
+# contrast, on the destination, receiving a bitmap (by alias)
+# from a node (by alias) when either alias is not mapped will
+# result in an error.
+# By default (when this parameter has never been set), bitmap
+# names are mapped to themselves. Nodes are mapped to their
+# block device name if there is one, and to their node name
+# otherwise. (Since 5.2)
+#
# Since: 2.4
##
# TODO either fuse back into MigrationParameters, or make
@@ -811,7 +880,8 @@
'*max-cpu-throttle': 'int',
'*multifd-compression': 'MultiFDCompression',
'*multifd-zlib-level': 'int',
- '*multifd-zstd-level': 'int' } }
+ '*multifd-zstd-level': 'int',
+ '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
##
# @migrate-set-parameters:
@@ -957,6 +1027,21 @@
# will consume more CPU.
# Defaults to 1. (Since 5.0)
#
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+# aliases for the purpose of dirty bitmap migration. Such
+# aliases may for example be the corresponding names on the
+# opposite site.
+# The mapping must be one-to-one, and on the destination also
+# complete: On the source, bitmaps on nodes where either the
+# bitmap or the node is not mapped will be ignored. In
+# contrast, on the destination, receiving a bitmap (by alias)
+# from a node (by alias) when either alias is not mapped will
+# result in an error.
+# By default (when this parameter has never been set), bitmap
+# names are mapped to themselves. Nodes are mapped to their
+# block device name if there is one, and to their node name
+# otherwise. (Since 5.2)
+#
# Since: 2.4
##
{ 'struct': 'MigrationParameters',
@@ -985,7 +1070,8 @@
'*max-cpu-throttle': 'uint8',
'*multifd-compression': 'MultiFDCompression',
'*multifd-zlib-level': 'uint8',
- '*multifd-zstd-level': 'uint8' } }
+ '*multifd-zstd-level': 'uint8',
+ '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
##
# @query-migrate-parameters:
diff --git a/migration/migration.h b/migration/migration.h
index f617960522..038c318b92 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -336,6 +336,9 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
void dirty_bitmap_mig_before_vm_start(void);
void init_dirty_bitmap_incoming_migration(void);
+bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
+ Error **errp);
+
void migrate_add_address(SocketAddress *address);
int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index b0dbf9eeed..c7945c631b 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -29,10 +29,10 @@
*
* # Header (shared for different chunk types)
* 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
- * [ 1 byte: node name size ] \ flags & DEVICE_NAME
- * [ n bytes: node name ] /
- * [ 1 byte: bitmap name size ] \ flags & BITMAP_NAME
- * [ n bytes: bitmap name ] /
+ * [ 1 byte: node alias size ] \ flags & DEVICE_NAME
+ * [ n bytes: node alias ] /
+ * [ 1 byte: bitmap alias size ] \ flags & BITMAP_NAME
+ * [ n bytes: bitmap alias ] /
*
* # Start of bitmap migration (flags & START)
* header
@@ -72,7 +72,9 @@
#include "migration/register.h"
#include "qemu/hbitmap.h"
#include "qemu/cutils.h"
+#include "qemu/id.h"
#include "qapi/error.h"
+#include "qapi/qapi-commands-migration.h"
#include "trace.h"
#define CHUNK_SIZE (1 << 10)
@@ -103,7 +105,8 @@
typedef struct DirtyBitmapMigBitmapState {
/* Written during setup phase. */
BlockDriverState *bs;
- const char *node_name;
+ char *node_alias;
+ char *bitmap_alias;
BdrvDirtyBitmap *bitmap;
uint64_t total_sectors;
uint64_t sectors_per_chunk;
@@ -128,7 +131,8 @@ typedef struct DirtyBitmapMigState {
typedef struct DirtyBitmapLoadState {
uint32_t flags;
- char node_name[256];
+ char node_alias[256];
+ char bitmap_alias[256];
char bitmap_name[256];
BlockDriverState *bs;
BdrvDirtyBitmap *bitmap;
@@ -144,6 +148,162 @@ typedef struct DirtyBitmapLoadBitmapState {
static GSList *enabled_bitmaps;
QemuMutex finish_lock;
+/* For hash tables that map node/bitmap names to aliases */
+typedef struct AliasMapInnerNode {
+ char *string;
+ GHashTable *subtree;
+} AliasMapInnerNode;
+
+static void free_alias_map_inner_node(void *amin_ptr)
+{
+ AliasMapInnerNode *amin = amin_ptr;
+
+ g_free(amin->string);
+ g_hash_table_unref(amin->subtree);
+ g_free(amin);
+}
+
+/**
+ * Construct an alias map based on the given QMP structure.
+ *
+ * (Note that we cannot store such maps in the MigrationParameters
+ * object, because that struct is defined by the QAPI schema, which
+ * makes it basically impossible to have dicts with arbitrary keys.
+ * Therefore, we instead have to construct these maps when migration
+ * starts.)
+ *
+ * @bbm is the block_bitmap_mapping from the migration parameters.
+ *
+ * If @name_to_alias is true, the returned hash table will map node
+ * and bitmap names to their respective aliases (for outgoing
+ * migration).
+ *
+ * If @name_to_alias is false, the returned hash table will map node
+ * and bitmap aliases to their respective names (for incoming
+ * migration).
+ *
+ * The hash table maps node names/aliases to AliasMapInnerNode
+ * objects, whose .string is the respective node alias/name, and whose
+ * .subtree table maps bitmap names/aliases to the respective bitmap
+ * alias/name.
+ */
+static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
+ bool name_to_alias,
+ Error **errp)
+{
+ GHashTable *alias_map;
+
+ alias_map = g_hash_table_new_full(g_str_hash, g_str_equal,
+ g_free, free_alias_map_inner_node);
+
+ for (; bbm; bbm = bbm->next) {
+ const BitmapMigrationNodeAlias *bmna = bbm->value;
+ const BitmapMigrationBitmapAliasList *bmbal;
+ AliasMapInnerNode *amin;
+ GHashTable *bitmaps_map;
+ const char *node_map_from, *node_map_to;
+
+ if (!id_wellformed(bmna->alias)) {
+ error_setg(errp, "The node alias %s is not well-formed",
+ bmna->alias);
+ goto fail;
+ }
+
+ if (name_to_alias) {
+ if (g_hash_table_contains(alias_map, bmna->node_name)) {
+ error_setg(errp, "The node name %s is mapped twice",
+ bmna->node_name);
+ goto fail;
+ }
+
+ node_map_from = bmna->node_name;
+ node_map_to = bmna->alias;
+ } else {
+ if (g_hash_table_contains(alias_map, bmna->alias)) {
+ error_setg(errp, "The node alias %s is used twice",
+ bmna->alias);
+ goto fail;
+ }
+
+ node_map_from = bmna->alias;
+ node_map_to = bmna->node_name;
+ }
+
+ bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
+ g_free, g_free);
+
+ amin = g_new(AliasMapInnerNode, 1);
+ *amin = (AliasMapInnerNode){
+ .string = g_strdup(node_map_to),
+ .subtree = bitmaps_map,
+ };
+
+ g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
+
+ for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
+ const BitmapMigrationBitmapAlias *bmba = bmbal->value;
+ const char *bmap_map_from, *bmap_map_to;
+
+ if (name_to_alias) {
+ bmap_map_from = bmba->name;
+ bmap_map_to = bmba->alias;
+
+ if (g_hash_table_contains(bitmaps_map, bmba->name)) {
+ error_setg(errp, "The bitmap %s/%s is mapped twice",
+ bmna->node_name, bmba->name);
+ goto fail;
+ }
+ } else {
+ bmap_map_from = bmba->alias;
+ bmap_map_to = bmba->name;
+
+ if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
+ error_setg(errp, "The bitmap alias %s/%s is used twice",
+ bmna->alias, bmba->alias);
+ goto fail;
+ }
+ }
+
+ g_hash_table_insert(bitmaps_map,
+ g_strdup(bmap_map_from), g_strdup(bmap_map_to));
+ }
+ }
+
+ return alias_map;
+
+fail:
+ g_hash_table_destroy(alias_map);
+ return NULL;
+}
+
+/**
+ * Run construct_alias_map() in both directions to check whether @bbm
+ * is valid.
+ * (This function is to be used by migration/migration.c to validate
+ * the user-specified block-bitmap-mapping migration parameter.)
+ *
+ * Returns true if and only if the mapping is valid.
+ */
+bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
+ Error **errp)
+{
+ GHashTable *alias_map;
+
+ alias_map = construct_alias_map(bbm, true, errp);
+ if (!alias_map) {
+ return false;
+ }
+ g_hash_table_destroy(alias_map);
+
+ alias_map = construct_alias_map(bbm, false, errp);
+ if (!alias_map) {
+ return false;
+ }
+ g_hash_table_destroy(alias_map);
+
+ return true;
+}
+
void init_dirty_bitmap_incoming_migration(void)
{
qemu_mutex_init(&finish_lock);
@@ -191,11 +351,11 @@ static void send_bitmap_header(QEMUFile *f, DirtyBitmapMigBitmapState *dbms,
qemu_put_bitmap_flags(f, flags);
if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
- qemu_put_counted_string(f, dbms->node_name);
+ qemu_put_counted_string(f, dbms->node_alias);
}
if (flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
- qemu_put_counted_string(f, bdrv_dirty_bitmap_name(bitmap));
+ qemu_put_counted_string(f, dbms->bitmap_alias);
}
}
@@ -263,17 +423,25 @@ static void dirty_bitmap_mig_cleanup(void)
QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
bdrv_unref(dbms->bs);
+ g_free(dbms->node_alias);
+ g_free(dbms->bitmap_alias);
g_free(dbms);
}
}
/* Called with iothread lock taken. */
-static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
+static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name,
+ GHashTable *alias_map)
{
BdrvDirtyBitmap *bitmap;
DirtyBitmapMigBitmapState *dbms;
+ GHashTable *bitmap_aliases;
+ const char *node_alias, *bitmap_name, *bitmap_alias;
Error *local_err = NULL;
+ /* When an alias map is given, @bs_name must be @bs's node name */
+ assert(!alias_map || !strcmp(bs_name, bdrv_get_node_name(bs)));
+
FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
if (bdrv_dirty_bitmap_name(bitmap)) {
break;
@@ -283,21 +451,39 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
return 0;
}
+ bitmap_name = bdrv_dirty_bitmap_name(bitmap);
+
if (!bs_name || strcmp(bs_name, "") == 0) {
error_report("Bitmap '%s' in unnamed node can't be migrated",
- bdrv_dirty_bitmap_name(bitmap));
+ bitmap_name);
return -1;
}
- if (bs_name[0] == '#') {
+ if (alias_map) {
+ const AliasMapInnerNode *amin = g_hash_table_lookup(alias_map, bs_name);
+
+ if (!amin) {
+ /* Skip bitmaps on nodes with no alias */
+ return 0;
+ }
+
+ node_alias = amin->string;
+ bitmap_aliases = amin->subtree;
+ } else {
+ node_alias = bs_name;
+ bitmap_aliases = NULL;
+ }
+
+ if (node_alias[0] == '#') {
error_report("Bitmap '%s' in a node with auto-generated "
"name '%s' can't be migrated",
- bdrv_dirty_bitmap_name(bitmap), bs_name);
+ bitmap_name, node_alias);
return -1;
}
FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
- if (!bdrv_dirty_bitmap_name(bitmap)) {
+ bitmap_name = bdrv_dirty_bitmap_name(bitmap);
+ if (!bitmap_name) {
continue;
}
@@ -306,12 +492,23 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
return -1;
}
+ if (bitmap_aliases) {
+ bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
+ if (!bitmap_alias) {
+ /* Skip bitmaps with no alias */
+ continue;
+ }
+ } else {
+ bitmap_alias = bitmap_name;
+ }
+
bdrv_ref(bs);
bdrv_dirty_bitmap_set_busy(bitmap, true);
dbms = g_new0(DirtyBitmapMigBitmapState, 1);
dbms->bs = bs;
- dbms->node_name = bs_name;
+ dbms->node_alias = g_strdup(node_alias);
+ dbms->bitmap_alias = g_strdup(bitmap_alias);
dbms->bitmap = bitmap;
dbms->total_sectors = bdrv_nb_sectors(bs);
dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
@@ -337,43 +534,52 @@ static int init_dirty_bitmap_migration(void)
DirtyBitmapMigBitmapState *dbms;
GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
BlockBackend *blk;
+ const MigrationParameters *mig_params = &migrate_get_current()->parameters;
+ GHashTable *alias_map = NULL;
+
+ if (mig_params->has_block_bitmap_mapping) {
+ alias_map = construct_alias_map(mig_params->block_bitmap_mapping, true,
+ &error_abort);
+ }
dirty_bitmap_mig_state.bulk_completed = false;
dirty_bitmap_mig_state.prev_bs = NULL;
dirty_bitmap_mig_state.prev_bitmap = NULL;
dirty_bitmap_mig_state.no_bitmaps = false;
- /*
- * Use blockdevice name for direct (or filtered) children of named block
- * backends.
- */
- for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
- const char *name = blk_name(blk);
-
- if (!name || strcmp(name, "") == 0) {
- continue;
- }
+ if (!alias_map) {
+ /*
+ * Use blockdevice name for direct (or filtered) children of named block
+ * backends.
+ */
+ for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+ const char *name = blk_name(blk);
- bs = blk_bs(blk);
+ if (!name || strcmp(name, "") == 0) {
+ continue;
+ }
- /* Skip filters without bitmaps */
- while (bs && bs->drv && bs->drv->is_filter &&
- !bdrv_has_named_bitmaps(bs))
- {
- if (bs->backing) {
- bs = bs->backing->bs;
- } else if (bs->file) {
- bs = bs->file->bs;
- } else {
- bs = NULL;
+ bs = blk_bs(blk);
+
+ /* Skip filters without bitmaps */
+ while (bs && bs->drv && bs->drv->is_filter &&
+ !bdrv_has_named_bitmaps(bs))
+ {
+ if (bs->backing) {
+ bs = bs->backing->bs;
+ } else if (bs->file) {
+ bs = bs->file->bs;
+ } else {
+ bs = NULL;
+ }
}
- }
- if (bs && bs->drv && !bs->drv->is_filter) {
- if (add_bitmaps_to_list(bs, name)) {
- goto fail;
+ if (bs && bs->drv && !bs->drv->is_filter) {
+ if (add_bitmaps_to_list(bs, name, NULL)) {
+ goto fail;
+ }
+ g_hash_table_add(handled_by_blk, bs);
}
- g_hash_table_add(handled_by_blk, bs);
}
}
@@ -382,7 +588,7 @@ static int init_dirty_bitmap_migration(void)
continue;
}
- if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs))) {
+ if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs), alias_map)) {
goto fail;
}
}
@@ -397,11 +603,17 @@ static int init_dirty_bitmap_migration(void)
}
g_hash_table_destroy(handled_by_blk);
+ if (alias_map) {
+ g_hash_table_destroy(alias_map);
+ }
return 0;
fail:
g_hash_table_destroy(handled_by_blk);
+ if (alias_map) {
+ g_hash_table_destroy(alias_map);
+ }
dirty_bitmap_mig_cleanup();
return -1;
@@ -666,8 +878,10 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DirtyBitmapLoadState *s)
return 0;
}
-static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
+static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s,
+ GHashTable *alias_map)
{
+ GHashTable *bitmap_alias_map = NULL;
Error *local_err = NULL;
bool nothing;
s->flags = qemu_get_bitmap_flags(f);
@@ -676,25 +890,68 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
nothing = s->flags == (s->flags & DIRTY_BITMAP_MIG_FLAG_EOS);
if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
- if (!qemu_get_counted_string(f, s->node_name)) {
- error_report("Unable to read node name string");
+ if (!qemu_get_counted_string(f, s->node_alias)) {
+ error_report("Unable to read node alias string");
return -EINVAL;
}
- s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
+
+ if (alias_map) {
+ const AliasMapInnerNode *amin;
+
+ amin = g_hash_table_lookup(alias_map, s->node_alias);
+ if (!amin) {
+ error_report("Error: Unknown node alias '%s'", s->node_alias);
+ return -EINVAL;
+ }
+
+ bitmap_alias_map = amin->subtree;
+ s->bs = bdrv_lookup_bs(NULL, amin->string, &local_err);
+ } else {
+ s->bs = bdrv_lookup_bs(s->node_alias, s->node_alias, &local_err);
+ }
if (!s->bs) {
error_report_err(local_err);
return -EINVAL;
}
- } else if (!s->bs && !nothing) {
+ } else if (s->bs) {
+ if (alias_map) {
+ const AliasMapInnerNode *amin;
+
+ /* Must be present in the map, or s->bs would not be set */
+ amin = g_hash_table_lookup(alias_map, s->node_alias);
+ assert(amin != NULL);
+
+ bitmap_alias_map = amin->subtree;
+ }
+ } else if (!nothing) {
error_report("Error: block device name is not set");
return -EINVAL;
}
+ assert(!!alias_map == !!bitmap_alias_map);
+
if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
- if (!qemu_get_counted_string(f, s->bitmap_name)) {
+ const char *bitmap_name;
+
+ if (!qemu_get_counted_string(f, s->bitmap_alias)) {
error_report("Unable to read bitmap name string");
return -EINVAL;
}
+
+ if (bitmap_alias_map) {
+ bitmap_name = g_hash_table_lookup(bitmap_alias_map,
+ s->bitmap_alias);
+ if (!bitmap_name) {
+ error_report("Error: Unknown bitmap alias '%s' on node '%s' "
+ "(alias '%s')", s->bitmap_alias, s->bs->node_name,
+ s->node_alias);
+ return -EINVAL;
+ }
+ } else {
+ bitmap_name = s->bitmap_alias;
+ }
+
+ g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
/* bitmap may be NULL here, it wouldn't be an error if it is the
@@ -702,7 +959,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
error_report("Error: unknown dirty bitmap "
"'%s' for block device '%s'",
- s->bitmap_name, s->node_name);
+ s->bitmap_name, s->bs->node_name);
return -EINVAL;
}
} else if (!s->bitmap && !nothing) {
@@ -715,6 +972,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
{
+ GHashTable *alias_map = NULL;
+ const MigrationParameters *mig_params = &migrate_get_current()->parameters;
static DirtyBitmapLoadState s;
int ret = 0;
@@ -724,10 +983,15 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
return -EINVAL;
}
+ if (mig_params->has_block_bitmap_mapping) {
+ alias_map = construct_alias_map(mig_params->block_bitmap_mapping,
+ false, &error_abort);
+ }
+
do {
- ret = dirty_bitmap_load_header(f, &s);
+ ret = dirty_bitmap_load_header(f, &s, alias_map);
if (ret < 0) {
- return ret;
+ goto fail;
}
if (s.flags & DIRTY_BITMAP_MIG_FLAG_START) {
@@ -743,12 +1007,17 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
}
if (ret) {
- return ret;
+ goto fail;
}
} while (!(s.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
trace_dirty_bitmap_load_success();
- return 0;
+ ret = 0;
+fail:
+ if (alias_map) {
+ g_hash_table_destroy(alias_map);
+ }
+ return ret;
}
static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
diff --git a/migration/migration.c b/migration/migration.c
index 2ed9923227..bcf7c3bf76 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -36,6 +36,7 @@
#include "block/block.h"
#include "qapi/error.h"
#include "qapi/clone-visitor.h"
+#include "qapi/qapi-visit-migration.h"
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qapi-commands-migration.h"
#include "qapi/qapi-events-migration.h"
@@ -832,6 +833,13 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
params->has_announce_step = true;
params->announce_step = s->parameters.announce_step;
+ if (s->parameters.has_block_bitmap_mapping) {
+ params->has_block_bitmap_mapping = true;
+ params->block_bitmap_mapping =
+ QAPI_CLONE(BitmapMigrationNodeAliasList,
+ s->parameters.block_bitmap_mapping);
+ }
+
return params;
}
@@ -1297,6 +1305,13 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
"is invalid, it must be in the range of 1 to 10000 ms");
return false;
}
+
+ if (params->has_block_bitmap_mapping &&
+ !check_dirty_bitmap_mig_alias_map(params->block_bitmap_mapping, errp)) {
+ error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
+ return false;
+ }
+
return true;
}
@@ -1391,6 +1406,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
if (params->has_announce_step) {
dest->announce_step = params->announce_step;
}
+
+ if (params->has_block_bitmap_mapping) {
+ dest->has_block_bitmap_mapping = true;
+ dest->block_bitmap_mapping = params->block_bitmap_mapping;
+ }
}
static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
@@ -1503,6 +1523,16 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
if (params->has_announce_step) {
s->parameters.announce_step = params->announce_step;
}
+
+ if (params->has_block_bitmap_mapping) {
+ qapi_free_BitmapMigrationNodeAliasList(
+ s->parameters.block_bitmap_mapping);
+
+ s->parameters.has_block_bitmap_mapping = true;
+ s->parameters.block_bitmap_mapping =
+ QAPI_CLONE(BitmapMigrationNodeAliasList,
+ params->block_bitmap_mapping);
+ }
}
void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ae4b6a4246..7711726fd2 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -469,6 +469,32 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "%s: '%s'\n",
MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
params->tls_authz);
+
+ if (params->has_block_bitmap_mapping) {
+ const BitmapMigrationNodeAliasList *bmnal;
+
+ monitor_printf(mon, "%s:\n",
+ MigrationParameter_str(
+ MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING));
+
+ for (bmnal = params->block_bitmap_mapping;
+ bmnal;
+ bmnal = bmnal->next)
+ {
+ const BitmapMigrationNodeAlias *bmna = bmnal->value;
+ const BitmapMigrationBitmapAliasList *bmbal;
+
+ monitor_printf(mon, " '%s' -> '%s'\n",
+ bmna->node_name, bmna->alias);
+
+ for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
+ const BitmapMigrationBitmapAlias *bmba = bmbal->value;
+
+ monitor_printf(mon, " '%s' -> '%s'\n",
+ bmba->name, bmba->alias);
+ }
+ }
+ }
}
qapi_free_MigrationParameters(params);
@@ -1384,6 +1410,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
p->has_announce_step = true;
visit_type_size(v, param, &p->announce_step, &err);
break;
+ case MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING:
+ error_setg(&err, "The block-bitmap-mapping parameter can only be set "
+ "through QMP");
+ break;
default:
assert(0);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] iotests.py: Add wait_for_runstate()
2020-07-16 13:53 [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter Max Reitz
2020-07-16 13:53 ` [PATCH v2 1/3] " Max Reitz
@ 2020-07-16 13:53 ` Max Reitz
2020-07-20 16:46 ` Vladimir Sementsov-Ogievskiy
2020-07-16 13:53 ` [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration Max Reitz
2020-07-22 9:17 ` [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter Dr. David Alan Gilbert
3 siblings, 1 reply; 13+ messages in thread
From: Max Reitz @ 2020-07-16 13:53 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Juan Quintela,
John Snow, qemu-devel, Dr . David Alan Gilbert, Peter Krempa,
Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3590ed78a0..fb240a334c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -803,6 +803,10 @@ class VM(qtest.QEMUQtestMachine):
'Found node %s under %s (but expected %s)' % \
(node['name'], path, expected_node)
+ def wait_for_runstate(self, runstate: str) -> None:
+ while self.qmp('query-status')['return']['status'] != runstate:
+ pass
+
index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
class QMPTestCase(unittest.TestCase):
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration
2020-07-16 13:53 [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter Max Reitz
2020-07-16 13:53 ` [PATCH v2 1/3] " Max Reitz
2020-07-16 13:53 ` [PATCH v2 2/3] iotests.py: Add wait_for_runstate() Max Reitz
@ 2020-07-16 13:53 ` Max Reitz
2020-07-20 18:02 ` Vladimir Sementsov-Ogievskiy
2020-07-22 9:17 ` [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter Dr. David Alan Gilbert
3 siblings, 1 reply; 13+ messages in thread
From: Max Reitz @ 2020-07-16 13:53 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Juan Quintela,
John Snow, qemu-devel, Dr . David Alan Gilbert, Peter Krempa,
Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/300 | 511 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/300.out | 5 +
tests/qemu-iotests/group | 1 +
3 files changed, 517 insertions(+)
create mode 100755 tests/qemu-iotests/300
create mode 100644 tests/qemu-iotests/300.out
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
new file mode 100755
index 0000000000..68714b7167
--- /dev/null
+++ b/tests/qemu-iotests/300
@@ -0,0 +1,511 @@
+#!/usr/bin/env python3
+#
+# Tests for dirty bitmaps migration with node aliases
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import random
+from typing import Dict, List, Optional, Union
+import iotests
+import qemu
+
+BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
+
+assert iotests.sock_dir is not None
+mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
+
+class TestDirtyBitmapMigration(iotests.QMPTestCase):
+ src_node_name: str = ''
+ dst_node_name: str = ''
+ src_bmap_name: str = ''
+ dst_bmap_name: str = ''
+
+ def setUp(self) -> None:
+ self.vm_a = iotests.VM(path_suffix='-a')
+ self.vm_a.add_blockdev(f'node-name={self.src_node_name},' \
+ 'driver=null-co')
+ self.vm_a.launch()
+
+ self.vm_b = iotests.VM(path_suffix='-b')
+ self.vm_b.add_blockdev(f'node-name={self.dst_node_name},' \
+ 'driver=null-co')
+ self.vm_b.add_incoming(f'unix:{mig_sock}')
+ self.vm_b.launch()
+
+ result = self.vm_a.qmp('block-dirty-bitmap-add',
+ node=self.src_node_name,
+ name=self.src_bmap_name)
+ self.assert_qmp(result, 'return', {})
+
+ # Dirty some random megabytes
+ for _ in range(9):
+ mb_ofs = random.randrange(1024)
+ self.vm_a.hmp_qemu_io(self.src_node_name, 'write %dM 1M' % mb_ofs)
+
+ result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',
+ node=self.src_node_name,
+ name=self.src_bmap_name)
+ self.bitmap_hash_reference = result['return']['sha256']
+
+ caps = [{'capability': name, 'state': True} \
+ for name in ('dirty-bitmaps', 'events')]
+
+ for vm in (self.vm_a, self.vm_b):
+ result = vm.qmp('migrate-set-capabilities', capabilities=caps)
+ self.assert_qmp(result, 'return', {})
+
+ def tearDown(self) -> None:
+ self.vm_a.shutdown()
+ self.vm_b.shutdown()
+ try:
+ os.remove(mig_sock)
+ except OSError:
+ pass
+
+ def check_bitmap(self, bitmap_name_valid: bool) -> None:
+ result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',
+ node=self.dst_node_name,
+ name=self.dst_bmap_name)
+ if bitmap_name_valid:
+ self.assert_qmp(result, 'return/sha256',
+ self.bitmap_hash_reference)
+ else:
+ self.assert_qmp(result, 'error/desc',
+ f"Dirty bitmap '{self.dst_bmap_name}' not found")
+
+ def migrate(self, migration_success: bool = True,
+ bitmap_name_valid: bool = True) -> None:
+ result = self.vm_a.qmp('migrate', uri=f'unix:{mig_sock}')
+ self.assert_qmp(result, 'return', {})
+
+ status = None
+ while status not in ('completed', 'failed'):
+ status = self.vm_a.event_wait('MIGRATION')['data']['status']
+
+ self.assertEqual(status == 'completed', migration_success)
+ if status == 'failed':
+ # Wait until the migration has been cleaned up
+ # (Otherwise, bdrv_close_all() will abort because the
+ # dirty bitmap migration code still holds a reference to
+ # the BDS)
+ # (Unfortunately, there does not appear to be a nicer way
+ # of waiting until a failed migration has been cleaned up)
+ timeout_msg = 'Timeout waiting for migration to be cleaned up'
+ with iotests.Timeout(30, timeout_msg):
+ while os.path.exists(mig_sock):
+ pass
+
+ # Dropping src_node_name will only work once the
+ # bitmap migration code has released it
+ while 'error' in self.vm_a.qmp('blockdev-del',
+ node_name=self.src_node_name):
+ pass
+
+ return
+
+ self.vm_a.wait_for_runstate('postmigrate')
+ self.vm_b.wait_for_runstate('running')
+
+ self.check_bitmap(bitmap_name_valid)
+
+ @staticmethod
+ def mapping(node_name: str, node_alias: str,
+ bitmap_name: str, bitmap_alias: str) \
+ -> BlockBitmapMapping:
+ return [{
+ 'node-name': node_name,
+ 'alias': node_alias,
+ 'bitmaps': [{
+ 'name': bitmap_name,
+ 'alias': bitmap_alias
+ }]
+ }]
+
+ def set_mapping(self, vm: iotests.VM, mapping: BlockBitmapMapping,
+ error: Optional[str] = None) \
+ -> None:
+ """
+ Invoke migrate-set-parameters on @vm to set the given @mapping.
+ Check for success if @error is None, or verify the error message
+ if it is not.
+ On success, verify that "info migrate_parameters" on HMP returns
+ our mapping. (Just to check its formatting code.)
+ """
+ result = vm.qmp('migrate-set-parameters',
+ block_bitmap_mapping=mapping)
+
+ if error is None:
+ self.assert_qmp(result, 'return', {})
+
+ result = vm.qmp('human-monitor-command',
+ command_line='info migrate_parameters')
+
+ hmp_mapping: List[str] = []
+ for line in result['return'].split('\n'):
+ line = line.rstrip()
+
+ if hmp_mapping == []:
+ if line == 'block-bitmap-mapping:':
+ hmp_mapping.append(line)
+ continue
+
+ if line.startswith(' '):
+ hmp_mapping.append(line)
+ else:
+ break
+
+ self.assertEqual('\n'.join(hmp_mapping) + '\n',
+ self.to_hmp_mapping(mapping))
+ else:
+ self.assert_qmp(result, 'error/desc', error)
+
+ @staticmethod
+ def to_hmp_mapping(mapping: BlockBitmapMapping) -> str:
+ result = 'block-bitmap-mapping:\n'
+
+ for node in mapping:
+ result += f" '{node['node-name']}' -> '{node['alias']}'\n"
+
+ assert isinstance(node['bitmaps'], list)
+ for bitmap in node['bitmaps']:
+ result += f" '{bitmap['name']}' -> '{bitmap['alias']}'\n"
+
+ return result
+
+
+class TestAliasMigration(TestDirtyBitmapMigration):
+ src_node_name = 'node0'
+ dst_node_name = 'node0'
+ src_bmap_name = 'bmap0'
+ dst_bmap_name = 'bmap0'
+
+ def test_migration_without_alias(self) -> None:
+ self.migrate(self.src_node_name == self.dst_node_name,
+ self.src_bmap_name == self.dst_bmap_name)
+
+ # Expect abnormal shutdown of the destination VM on migration failure
+ if self.src_node_name != self.dst_node_name:
+ try:
+ self.vm_b.shutdown()
+ except qemu.machine.AbnormalShutdown:
+ pass
+
+ def test_alias_on_src_migration(self) -> None:
+ mapping = self.mapping(self.src_node_name, self.dst_node_name,
+ self.src_bmap_name, self.dst_bmap_name)
+
+ self.set_mapping(self.vm_a, mapping)
+ self.migrate()
+
+ def test_alias_on_dst_migration(self) -> None:
+ mapping = self.mapping(self.dst_node_name, self.src_node_name,
+ self.dst_bmap_name, self.src_bmap_name)
+
+ self.set_mapping(self.vm_b, mapping)
+ self.migrate()
+
+ def test_alias_on_both_migration(self) -> None:
+ src_map = self.mapping(self.src_node_name, 'node-alias',
+ self.src_bmap_name, 'bmap-alias')
+
+ dst_map = self.mapping(self.dst_node_name, 'node-alias',
+ self.dst_bmap_name, 'bmap-alias')
+
+ self.set_mapping(self.vm_a, src_map)
+ self.set_mapping(self.vm_b, dst_map)
+ self.migrate()
+
+
+class TestNodeAliasMigration(TestAliasMigration):
+ src_node_name = 'node-src'
+ dst_node_name = 'node-dst'
+
+
+class TestBitmapAliasMigration(TestAliasMigration):
+ src_bmap_name = 'bmap-src'
+ dst_bmap_name = 'bmap-dst'
+
+
+class TestFullAliasMigration(TestAliasMigration):
+ src_node_name = 'node-src'
+ dst_node_name = 'node-dst'
+ src_bmap_name = 'bmap-src'
+ dst_bmap_name = 'bmap-dst'
+
+
+class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
+ src_node_name = 'node0'
+ dst_node_name = 'node0'
+ src_bmap_name = 'bmap0'
+ dst_bmap_name = 'bmap0'
+
+ """
+ Note that mapping nodes or bitmaps that do not exist is not an error.
+ """
+
+ def test_non_injective_node_mapping(self) -> None:
+ mapping: BlockBitmapMapping = [
+ {
+ 'node-name': 'node0',
+ 'alias': 'common-alias',
+ 'bitmaps': [{
+ 'name': 'bmap0',
+ 'alias': 'bmap-alias0'
+ }]
+ },
+ {
+ 'node-name': 'node1',
+ 'alias': 'common-alias',
+ 'bitmaps': [{
+ 'name': 'bmap1',
+ 'alias': 'bmap-alias1'
+ }]
+ }
+ ]
+
+ self.set_mapping(self.vm_a, mapping,
+ 'Invalid mapping given for block-bitmap-mapping: ' \
+ 'The node alias common-alias is used twice')
+
+ def test_non_injective_bitmap_mapping(self) -> None:
+ mapping: BlockBitmapMapping = [{
+ 'node-name': 'node0',
+ 'alias': 'node-alias0',
+ 'bitmaps': [
+ {
+ 'name': 'bmap0',
+ 'alias': 'common-alias'
+ },
+ {
+ 'name': 'bmap1',
+ 'alias': 'common-alias'
+ }
+ ]
+ }]
+
+ self.set_mapping(self.vm_a, mapping,
+ 'Invalid mapping given for block-bitmap-mapping: ' \
+ 'The bitmap alias node-alias0/common-alias is used ' \
+ 'twice')
+
+ def test_ambiguous_node_mapping(self) -> None:
+ mapping: BlockBitmapMapping = [
+ {
+ 'node-name': 'node0',
+ 'alias': 'node-alias0',
+ 'bitmaps': [{
+ 'name': 'bmap0',
+ 'alias': 'bmap-alias0'
+ }]
+ },
+ {
+ 'node-name': 'node0',
+ 'alias': 'node-alias1',
+ 'bitmaps': [{
+ 'name': 'bmap0',
+ 'alias': 'bmap-alias0'
+ }]
+ }
+ ]
+
+ self.set_mapping(self.vm_a, mapping,
+ 'Invalid mapping given for block-bitmap-mapping: ' \
+ 'The node name node0 is mapped twice')
+
+ def test_ambiguous_bitmap_mapping(self) -> None:
+ mapping: BlockBitmapMapping = [{
+ 'node-name': 'node0',
+ 'alias': 'node-alias0',
+ 'bitmaps': [
+ {
+ 'name': 'bmap0',
+ 'alias': 'bmap-alias0'
+ },
+ {
+ 'name': 'bmap0',
+ 'alias': 'bmap-alias1'
+ }
+ ]
+ }]
+
+ self.set_mapping(self.vm_a, mapping,
+ 'Invalid mapping given for block-bitmap-mapping: ' \
+ 'The bitmap node0/bmap0 is mapped twice')
+
+ def test_migratee_node_is_not_mapped_on_src(self) -> None:
+ self.set_mapping(self.vm_a, [])
+ # Should just ignore all bitmaps on unmapped nodes
+ self.migrate(True, False)
+
+ def test_migratee_node_is_not_mapped_on_dst(self) -> None:
+ self.set_mapping(self.vm_b, [])
+ self.migrate(False, False)
+
+ # Expect abnormal shutdown of the destination VM on migration failure
+ try:
+ self.vm_b.shutdown()
+ except qemu.machine.AbnormalShutdown:
+ pass
+
+ self.assertIn(f"Unknown node alias '{self.src_node_name}'",
+ self.vm_b.get_log())
+
+ def test_migratee_bitmap_is_not_mapped_on_src(self) -> None:
+ mapping: BlockBitmapMapping = [{
+ 'node-name': self.src_node_name,
+ 'alias': self.dst_node_name,
+ 'bitmaps': []
+ }]
+
+ self.set_mapping(self.vm_a, mapping)
+ # Should just ignore all unmapped bitmaps
+ self.migrate(True, False)
+
+ def test_migratee_bitmap_is_not_mapped_on_dst(self) -> None:
+ mapping: BlockBitmapMapping = [{
+ 'node-name': self.dst_node_name,
+ 'alias': self.src_node_name,
+ 'bitmaps': []
+ }]
+
+ self.set_mapping(self.vm_b, mapping)
+ self.migrate(False, False)
+
+ # Expect abnormal shutdown of the destination VM on migration failure
+ try:
+ self.vm_b.shutdown()
+ except qemu.machine.AbnormalShutdown:
+ pass
+
+ self.assertIn(f"Unknown bitmap alias '{self.src_bmap_name}' on node " \
+ f"'{self.dst_node_name}' (alias '{self.src_node_name}')",
+ self.vm_b.get_log())
+
+ def test_non_wellformed_node_alias(self) -> None:
+ mapping: BlockBitmapMapping = [{
+ 'node-name': self.src_bmap_name,
+ 'alias': '123-foo',
+ 'bitmaps': []
+ }]
+
+ self.set_mapping(self.vm_a, mapping,
+ 'Invalid mapping given for block-bitmap-mapping: ' \
+ 'The node alias 123-foo is not well-formed')
+
+
+class TestCrossAliasMigration(TestDirtyBitmapMigration):
+ """
+ Swap aliases, both to see that qemu does not get confused, and
+ that we can migrate multiple things at once.
+
+ So we migrate this:
+ node-a.bmap-a -> node-b.bmap-b
+ node-a.bmap-b -> node-b.bmap-a
+ node-b.bmap-a -> node-a.bmap-b
+ node-b.bmap-b -> node-a.bmap-a
+ """
+
+ src_node_name = 'node-a'
+ dst_node_name = 'node-b'
+ src_bmap_name = 'bmap-a'
+ dst_bmap_name = 'bmap-b'
+
+ def setUp(self) -> None:
+ TestDirtyBitmapMigration.setUp(self)
+
+ # Now create another block device and let both have two bitmaps each
+ result = self.vm_a.qmp('blockdev-add',
+ node_name='node-b', driver='null-co')
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm_b.qmp('blockdev-add',
+ node_name='node-a', driver='null-co')
+ self.assert_qmp(result, 'return', {})
+
+ bmaps_to_add = (('node-a', 'bmap-b'),
+ ('node-b', 'bmap-a'),
+ ('node-b', 'bmap-b'))
+
+ for (node, bmap) in bmaps_to_add:
+ result = self.vm_a.qmp('block-dirty-bitmap-add',
+ node=node, name=bmap)
+ self.assert_qmp(result, 'return', {})
+
+ @staticmethod
+ def cross_mapping() -> BlockBitmapMapping:
+ return [
+ {
+ 'node-name': 'node-a',
+ 'alias': 'node-b',
+ 'bitmaps': [
+ {
+ 'name': 'bmap-a',
+ 'alias': 'bmap-b'
+ },
+ {
+ 'name': 'bmap-b',
+ 'alias': 'bmap-a'
+ }
+ ]
+ },
+ {
+ 'node-name': 'node-b',
+ 'alias': 'node-a',
+ 'bitmaps': [
+ {
+ 'name': 'bmap-b',
+ 'alias': 'bmap-a'
+ },
+ {
+ 'name': 'bmap-a',
+ 'alias': 'bmap-b'
+ }
+ ]
+ }
+ ]
+
+ def verify_dest_has_all_bitmaps(self) -> None:
+ bitmaps = self.vm_a.query_bitmaps()
+
+ # Extract and sort bitmap names
+ for node in bitmaps:
+ bitmaps[node] = sorted((bmap['name'] for bmap in bitmaps[node]))
+
+ self.assertEqual(bitmaps,
+ {'node-a': ['bmap-a', 'bmap-b'],
+ 'node-b': ['bmap-a', 'bmap-b']})
+
+ def test_alias_on_src(self) -> None:
+ self.set_mapping(self.vm_a, self.cross_mapping())
+
+ # Checks that node-a.bmap-a was migrated to node-b.bmap-b, and
+ # that is enough
+ self.migrate()
+
+ self.verify_dest_has_all_bitmaps()
+
+ def test_alias_on_dst(self) -> None:
+ self.set_mapping(self.vm_b, self.cross_mapping())
+
+ # Checks that node-a.bmap-a was migrated to node-b.bmap-b, and
+ # that is enough
+ self.migrate()
+
+ self.verify_dest_has_all_bitmaps()
+
+
+if __name__ == '__main__':
+ iotests.main(supported_protocols=['file'])
diff --git a/tests/qemu-iotests/300.out b/tests/qemu-iotests/300.out
new file mode 100644
index 0000000000..6d9bee1a4b
--- /dev/null
+++ b/tests/qemu-iotests/300.out
@@ -0,0 +1,5 @@
+...........................
+----------------------------------------------------------------------
+Ran 27 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index c0710cd99e..6b8910a767 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -307,4 +307,5 @@
295 rw
296 rw
297 meta
+300 migration
301 backing quick
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] migration: Add block-bitmap-mapping parameter
2020-07-16 13:53 ` [PATCH v2 1/3] " Max Reitz
@ 2020-07-20 16:31 ` Vladimir Sementsov-Ogievskiy
2020-07-21 8:02 ` Max Reitz
2020-07-22 9:06 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 13+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-20 16:31 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: Kevin Wolf, Peter Krempa, Juan Quintela, John Snow, qemu-devel,
Dr . David Alan Gilbert
16.07.2020 16:53, Max Reitz wrote:
> This migration parameter allows mapping block node names and bitmap
> names to aliases for the purpose of block dirty bitmap migration.
>
> This way, management tools can use different node and bitmap names on
> the source and destination and pass the mapping of how bitmaps are to be
> transferred to qemu (on the source, the destination, or even both with
> arbitrary aliases in the migration stream).
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> Vladimir noted in v1 that it would be better to ignore bitmaps whose
> names aren't mapped, or that are on nodes whose names aren't mapped.
> One of the reasons he gave was that bitmap migration is inherently a
> form of postcopy migration, and we should not break the target when it
> is already running because of a bitmap issue.
>
> So in this version, I changed the behavior to just ignore bitmaps
> without a mapping on the source side. On the destination, however, I
> kept it an error when an incoming bitmap or node alias is unknown.
>
> This is in violation of Vladimir's (rightful) reasoning that such
> errors should never break the already running target, but I decided to
> do so anyway for a couple of reasons:
>
> - Ignoring unmapped bitmaps on the source is trivial: We just have to
> not put them into the migration stream.
> On the destination, it isn't so easy: We (I think) would have to
> modify the code to actively skip the bitmap in the stream.
> (On the other hand, erroring out is always easy.)
Agree. Actually, my series "[PATCH v2 00/22] Fix error handling during bitmap postcopy"
do something like this. But no sense in mixing such logic into your series :)
>
> - Incoming bitmaps with unknown names are already an error before this
> series. So this isn't introducing new breakage.
>
> - I think it makes sense to drop all bitmaps you don't want to migrate
> (or implicitly drop them by just not specifying them if you don't care
> about them) on the source. I can't imagine a scenario where it would
> be really useful if the destination could silently drop unknown
> bitmaps. Unknown bitmaps should just be dropped on the source.
>
> - Choosing to make it an error now doesn't prevent us from relaxing that
> restriction in the future.
Agree, that's all OK. Still we can at least ignore, if we don't get some
bitmap on destination, for which mapping is set (I think you do exactly
this, will see below).
> ---
> qapi/migration.json | 92 +++++++-
> migration/migration.h | 3 +
> migration/block-dirty-bitmap.c | 373 ++++++++++++++++++++++++++++-----
> migration/migration.c | 30 +++
> monitor/hmp-cmds.c | 30 +++
> 5 files changed, 473 insertions(+), 55 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d5000558c6..1b0fbcef96 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -507,6 +507,44 @@
[..]
> #
> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
> +# aliases for the purpose of dirty bitmap migration. Such
> +# aliases may for example be the corresponding names on the
> +# opposite site.
> +# The mapping must be one-to-one, and on the destination also
> +# complete: On the source, bitmaps on nodes where either the
> +# bitmap or the node is not mapped will be ignored. In
> +# contrast, on the destination, receiving a bitmap (by alias)
> +# from a node (by alias) when either alias is not mapped will
> +# result in an error.
Still, not receiving some bitmap is not an error. It's good. I think, should
be mentioned here too (does it violate "compelete" term?).
> +# By default (when this parameter has never been set), bitmap
> +# names are mapped to themselves. Nodes are mapped to their
> +# block device name if there is one, and to their node name
> +# otherwise. (Since 5.2)
> +#
> # Since: 2.4
[..]
> -static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
> +static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s,
> + GHashTable *alias_map)
> {
> + GHashTable *bitmap_alias_map = NULL;
> Error *local_err = NULL;
> bool nothing;
> s->flags = qemu_get_bitmap_flags(f);
> @@ -676,25 +890,68 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
> nothing = s->flags == (s->flags & DIRTY_BITMAP_MIG_FLAG_EOS);
>
> if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
> - if (!qemu_get_counted_string(f, s->node_name)) {
> - error_report("Unable to read node name string");
> + if (!qemu_get_counted_string(f, s->node_alias)) {
> + error_report("Unable to read node alias string");
> return -EINVAL;
> }
> - s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
> +
> + if (alias_map) {
> + const AliasMapInnerNode *amin;
> +
> + amin = g_hash_table_lookup(alias_map, s->node_alias);
> + if (!amin) {
> + error_report("Error: Unknown node alias '%s'", s->node_alias);
> + return -EINVAL;
> + }
> +
> + bitmap_alias_map = amin->subtree;
> + s->bs = bdrv_lookup_bs(NULL, amin->string, &local_err);
> + } else {
> + s->bs = bdrv_lookup_bs(s->node_alias, s->node_alias, &local_err);
> + }
> if (!s->bs) {
> error_report_err(local_err);
> return -EINVAL;
> }
> - } else if (!s->bs && !nothing) {
> + } else if (s->bs) {
> + if (alias_map) {
> + const AliasMapInnerNode *amin;
> +
> + /* Must be present in the map, or s->bs would not be set */
> + amin = g_hash_table_lookup(alias_map, s->node_alias);
> + assert(amin != NULL);
> +
> + bitmap_alias_map = amin->subtree;
> + }
> + } else if (!nothing) {
> error_report("Error: block device name is not set");
> return -EINVAL;
> }
>
> + assert(!!alias_map == !!bitmap_alias_map);
Actually one '!' is enough. But '!!' looks good too (usual convertion to bool, why not).
But what's more serious: this assertion will fail in case of "nothing"
(sorry my architecture :(.
I assume, by protocol, chunk with EOS flag may contain bitmap and/or node information or may not.
So, it most probably should be: assert(nothing || (!alias_map == !bitmap_alias_map))
(You can cover "nothing" case in test, if enable bitmap migrations when no bitmaps to migrate)
> +
> if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> - if (!qemu_get_counted_string(f, s->bitmap_name)) {
> + const char *bitmap_name;
> +
> + if (!qemu_get_counted_string(f, s->bitmap_alias)) {
> error_report("Unable to read bitmap name string");
> return -EINVAL;
> }
> +
> + if (bitmap_alias_map) {
> + bitmap_name = g_hash_table_lookup(bitmap_alias_map,
> + s->bitmap_alias);
> + if (!bitmap_name) {
> + error_report("Error: Unknown bitmap alias '%s' on node '%s' "
> + "(alias '%s')", s->bitmap_alias, s->bs->node_name,
> + s->node_alias);
> + return -EINVAL;
> + }
> + } else {
> + bitmap_name = s->bitmap_alias;
> + }
> +
> + g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
Hmm. Actually, we should check that in alias map target bitmap_name is short enough. It may be done later.
> s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
>
> /* bitmap may be NULL here, it wouldn't be an error if it is the
> @@ -702,7 +959,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
> if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
OK, with assertion fixed;
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
It's a bit weak, because:
- I don't have good understanding all these migration parameters logics with this triple duplication. So if it works in test, it should be good enough.
- The whole logic of bitmap migration is a bit hard to follow (I know, that it's my code :). I'll revisit it when rebasing my big series "[PATCH v2 00/22] Fix error handling during bitmap postcopy".
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] iotests.py: Add wait_for_runstate()
2020-07-16 13:53 ` [PATCH v2 2/3] iotests.py: Add wait_for_runstate() Max Reitz
@ 2020-07-20 16:46 ` Vladimir Sementsov-Ogievskiy
2020-07-21 8:05 ` Max Reitz
0 siblings, 1 reply; 13+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-20 16:46 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: Kevin Wolf, Peter Krempa, Juan Quintela, John Snow, qemu-devel,
Dr . David Alan Gilbert
16.07.2020 16:53, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 3590ed78a0..fb240a334c 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -803,6 +803,10 @@ class VM(qtest.QEMUQtestMachine):
> 'Found node %s under %s (but expected %s)' % \
> (node['name'], path, expected_node)
>
> + def wait_for_runstate(self, runstate: str) -> None:
> + while self.qmp('query-status')['return']['status'] != runstate:
> + pass
don't we need something like time.sleep(0.5) instead of just pass?
Anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> +
> index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>
> class QMPTestCase(unittest.TestCase):
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration
2020-07-16 13:53 ` [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration Max Reitz
@ 2020-07-20 18:02 ` Vladimir Sementsov-Ogievskiy
2020-07-21 8:33 ` Max Reitz
0 siblings, 1 reply; 13+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-20 18:02 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: Kevin Wolf, Peter Krempa, Juan Quintela, John Snow, qemu-devel,
Dr . David Alan Gilbert
16.07.2020 16:53, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/300 | 511 +++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/300.out | 5 +
> tests/qemu-iotests/group | 1 +
> 3 files changed, 517 insertions(+)
> create mode 100755 tests/qemu-iotests/300
> create mode 100644 tests/qemu-iotests/300.out
>
> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> new file mode 100755
> index 0000000000..68714b7167
> --- /dev/null
> +++ b/tests/qemu-iotests/300
> @@ -0,0 +1,511 @@
> +#!/usr/bin/env python3
> +#
> +# Tests for dirty bitmaps migration with node aliases
copyright?
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import random
> +from typing import Dict, List, Optional, Union
> +import iotests
> +import qemu
> +
> +BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
> +
> +assert iotests.sock_dir is not None
> +mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
> +
> +class TestDirtyBitmapMigration(iotests.QMPTestCase):
> + src_node_name: str = ''
> + dst_node_name: str = ''
> + src_bmap_name: str = ''
> + dst_bmap_name: str = ''
Hmm, I hope, typing actually not needed with such an obvious initialization
> +
> + def setUp(self) -> None:
> + self.vm_a = iotests.VM(path_suffix='-a')
> + self.vm_a.add_blockdev(f'node-name={self.src_node_name},' \
> + 'driver=null-co')
Hmm, you don't specify disk size.. How can it work? Aha, it's 1G by default. OK.
> + self.vm_a.launch()
> +
> + self.vm_b = iotests.VM(path_suffix='-b')
> + self.vm_b.add_blockdev(f'node-name={self.dst_node_name},' \
> + 'driver=null-co')
> + self.vm_b.add_incoming(f'unix:{mig_sock}')
> + self.vm_b.launch()
> +
> + result = self.vm_a.qmp('block-dirty-bitmap-add',
> + node=self.src_node_name,
> + name=self.src_bmap_name)
> + self.assert_qmp(result, 'return', {})
> +
> + # Dirty some random megabytes
> + for _ in range(9):
> + mb_ofs = random.randrange(1024)
> + self.vm_a.hmp_qemu_io(self.src_node_name, 'write %dM 1M' % mb_ofs)
May be, use f-string for consistency
> +
> + result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',
> + node=self.src_node_name,
> + name=self.src_bmap_name)
> + self.bitmap_hash_reference = result['return']['sha256']
> +
> + caps = [{'capability': name, 'state': True} \
> + for name in ('dirty-bitmaps', 'events')]
> +
> + for vm in (self.vm_a, self.vm_b):
> + result = vm.qmp('migrate-set-capabilities', capabilities=caps)
> + self.assert_qmp(result, 'return', {})
> +
> + def tearDown(self) -> None:
> + self.vm_a.shutdown()
> + self.vm_b.shutdown()
> + try:
> + os.remove(mig_sock)
> + except OSError:
> + pass
> +
> + def check_bitmap(self, bitmap_name_valid: bool) -> None:
> + result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',
> + node=self.dst_node_name,
> + name=self.dst_bmap_name)
> + if bitmap_name_valid:
> + self.assert_qmp(result, 'return/sha256',
> + self.bitmap_hash_reference)
> + else:
> + self.assert_qmp(result, 'error/desc',
> + f"Dirty bitmap '{self.dst_bmap_name}' not found")
> +
> + def migrate(self, migration_success: bool = True,
> + bitmap_name_valid: bool = True) -> None:
> + result = self.vm_a.qmp('migrate', uri=f'unix:{mig_sock}')
> + self.assert_qmp(result, 'return', {})
> +
> + status = None
> + while status not in ('completed', 'failed'):
> + status = self.vm_a.event_wait('MIGRATION')['data']['status']
> +
> + self.assertEqual(status == 'completed', migration_success)
> + if status == 'failed':
> + # Wait until the migration has been cleaned up
> + # (Otherwise, bdrv_close_all() will abort because the
> + # dirty bitmap migration code still holds a reference to
> + # the BDS)
> + # (Unfortunately, there does not appear to be a nicer way
> + # of waiting until a failed migration has been cleaned up)
> + timeout_msg = 'Timeout waiting for migration to be cleaned up'
> + with iotests.Timeout(30, timeout_msg):
> + while os.path.exists(mig_sock):
> + pass
> +
> + # Dropping src_node_name will only work once the
> + # bitmap migration code has released it
> + while 'error' in self.vm_a.qmp('blockdev-del',
> + node_name=self.src_node_name):
> + pass
Somehow I would feel calmer with s/pass/time.sleep(0.5)/ in such loops.
> +
> + return
> +
> + self.vm_a.wait_for_runstate('postmigrate')
> + self.vm_b.wait_for_runstate('running')
Actually, bitmaps migration may continue in postcopy, so more correct would be to wait for completed status for migration on target. Still, shouldn't be a big difference when migrate small bitmap data.
> +
> + self.check_bitmap(bitmap_name_valid)
> +
> + @staticmethod
> + def mapping(node_name: str, node_alias: str,
> + bitmap_name: str, bitmap_alias: str) \
> + -> BlockBitmapMapping:
> + return [{
> + 'node-name': node_name,
> + 'alias': node_alias,
> + 'bitmaps': [{
> + 'name': bitmap_name,
> + 'alias': bitmap_alias
> + }]
> + }]
> +
> + def set_mapping(self, vm: iotests.VM, mapping: BlockBitmapMapping,
> + error: Optional[str] = None) \
> + -> None:
> + """
> + Invoke migrate-set-parameters on @vm to set the given @mapping.
> + Check for success if @error is None, or verify the error message
> + if it is not.
> + On success, verify that "info migrate_parameters" on HMP returns
> + our mapping. (Just to check its formatting code.)
> + """
> + result = vm.qmp('migrate-set-parameters',
> + block_bitmap_mapping=mapping)
> +
> + if error is None:
> + self.assert_qmp(result, 'return', {})
> +
> + result = vm.qmp('human-monitor-command',
> + command_line='info migrate_parameters')
> +
> + hmp_mapping: List[str] = []
> + for line in result['return'].split('\n'):
> + line = line.rstrip()
> +
> + if hmp_mapping == []:
> + if line == 'block-bitmap-mapping:':
> + hmp_mapping.append(line)
> + continue
> +
> + if line.startswith(' '):
> + hmp_mapping.append(line)
> + else:
> + break
Let me try:
hmp_mapping = re.search(r'^block-bitmap-mapping:.*(\n .*)*', result['return'], flags=re.MULTILINE)
> +
> + self.assertEqual('\n'.join(hmp_mapping) + '\n',
> + self.to_hmp_mapping(mapping))
> + else:
> + self.assert_qmp(result, 'error/desc', error)
> +
> + @staticmethod
> + def to_hmp_mapping(mapping: BlockBitmapMapping) -> str:
> + result = 'block-bitmap-mapping:\n'
> +
> + for node in mapping:
> + result += f" '{node['node-name']}' -> '{node['alias']}'\n"
> +
> + assert isinstance(node['bitmaps'], list)
> + for bitmap in node['bitmaps']:
> + result += f" '{bitmap['name']}' -> '{bitmap['alias']}'\n"
> +
> + return result
> +
> +
> +class TestAliasMigration(TestDirtyBitmapMigration):
> + src_node_name = 'node0'
> + dst_node_name = 'node0'
> + src_bmap_name = 'bmap0'
> + dst_bmap_name = 'bmap0'
> +
> + def test_migration_without_alias(self) -> None:
> + self.migrate(self.src_node_name == self.dst_node_name,
> + self.src_bmap_name == self.dst_bmap_name)
> +
> + # Expect abnormal shutdown of the destination VM on migration failure
> + if self.src_node_name != self.dst_node_name:
> + try:
> + self.vm_b.shutdown()
> + except qemu.machine.AbnormalShutdown:
> + pass
> +
> + def test_alias_on_src_migration(self) -> None:
> + mapping = self.mapping(self.src_node_name, self.dst_node_name,
> + self.src_bmap_name, self.dst_bmap_name)
> +
> + self.set_mapping(self.vm_a, mapping)
> + self.migrate()
> +
> + def test_alias_on_dst_migration(self) -> None:
> + mapping = self.mapping(self.dst_node_name, self.src_node_name,
> + self.dst_bmap_name, self.src_bmap_name)
> +
> + self.set_mapping(self.vm_b, mapping)
> + self.migrate()
> +
> + def test_alias_on_both_migration(self) -> None:
> + src_map = self.mapping(self.src_node_name, 'node-alias',
> + self.src_bmap_name, 'bmap-alias')
> +
> + dst_map = self.mapping(self.dst_node_name, 'node-alias',
> + self.dst_bmap_name, 'bmap-alias')
> +
> + self.set_mapping(self.vm_a, src_map)
> + self.set_mapping(self.vm_b, dst_map)
> + self.migrate()
> +
> +
> +class TestNodeAliasMigration(TestAliasMigration):
> + src_node_name = 'node-src'
> + dst_node_name = 'node-dst'
> +
> +
> +class TestBitmapAliasMigration(TestAliasMigration):
> + src_bmap_name = 'bmap-src'
> + dst_bmap_name = 'bmap-dst'
> +
> +
> +class TestFullAliasMigration(TestAliasMigration):
> + src_node_name = 'node-src'
> + dst_node_name = 'node-dst'
> + src_bmap_name = 'bmap-src'
> + dst_bmap_name = 'bmap-dst'
> +
> +
> +class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
> + src_node_name = 'node0'
> + dst_node_name = 'node0'
> + src_bmap_name = 'bmap0'
> + dst_bmap_name = 'bmap0'
> +
> + """
> + Note that mapping nodes or bitmaps that do not exist is not an error.
> + """
> +
> + def test_non_injective_node_mapping(self) -> None:
> + mapping: BlockBitmapMapping = [
> + {
> + 'node-name': 'node0',
> + 'alias': 'common-alias',
> + 'bitmaps': [{
> + 'name': 'bmap0',
> + 'alias': 'bmap-alias0'
> + }]
> + },
> + {
> + 'node-name': 'node1',
> + 'alias': 'common-alias',
> + 'bitmaps': [{
> + 'name': 'bmap1',
> + 'alias': 'bmap-alias1'
> + }]
> + }
> + ]
> +
> + self.set_mapping(self.vm_a, mapping,
> + 'Invalid mapping given for block-bitmap-mapping: ' \
> + 'The node alias common-alias is used twice')
> +
> + def test_non_injective_bitmap_mapping(self) -> None:
> + mapping: BlockBitmapMapping = [{
> + 'node-name': 'node0',
> + 'alias': 'node-alias0',
> + 'bitmaps': [
> + {
> + 'name': 'bmap0',
> + 'alias': 'common-alias'
> + },
> + {
> + 'name': 'bmap1',
> + 'alias': 'common-alias'
> + }
> + ]
> + }]
> +
> + self.set_mapping(self.vm_a, mapping,
> + 'Invalid mapping given for block-bitmap-mapping: ' \
> + 'The bitmap alias node-alias0/common-alias is used ' \
> + 'twice')
> +
> + def test_ambiguous_node_mapping(self) -> None:
> + mapping: BlockBitmapMapping = [
> + {
> + 'node-name': 'node0',
> + 'alias': 'node-alias0',
> + 'bitmaps': [{
> + 'name': 'bmap0',
> + 'alias': 'bmap-alias0'
> + }]
> + },
> + {
> + 'node-name': 'node0',
> + 'alias': 'node-alias1',
> + 'bitmaps': [{
> + 'name': 'bmap0',
> + 'alias': 'bmap-alias0'
> + }]
> + }
> + ]
> +
> + self.set_mapping(self.vm_a, mapping,
> + 'Invalid mapping given for block-bitmap-mapping: ' \
> + 'The node name node0 is mapped twice')
> +
> + def test_ambiguous_bitmap_mapping(self) -> None:
> + mapping: BlockBitmapMapping = [{
> + 'node-name': 'node0',
> + 'alias': 'node-alias0',
> + 'bitmaps': [
> + {
> + 'name': 'bmap0',
> + 'alias': 'bmap-alias0'
> + },
> + {
> + 'name': 'bmap0',
> + 'alias': 'bmap-alias1'
> + }
> + ]
> + }]
> +
> + self.set_mapping(self.vm_a, mapping,
> + 'Invalid mapping given for block-bitmap-mapping: ' \
> + 'The bitmap node0/bmap0 is mapped twice')
> +
> + def test_migratee_node_is_not_mapped_on_src(self) -> None:
is migratee a mistake or an abbreviation for migrate-error ? :)
> + self.set_mapping(self.vm_a, [])
> + # Should just ignore all bitmaps on unmapped nodes
> + self.migrate(True, False)
> +
> + def test_migratee_node_is_not_mapped_on_dst(self) -> None:
> + self.set_mapping(self.vm_b, [])
> + self.migrate(False, False)
> +
> + # Expect abnormal shutdown of the destination VM on migration failure
> + try:
> + self.vm_b.shutdown()
> + except qemu.machine.AbnormalShutdown:
> + pass
> +
> + self.assertIn(f"Unknown node alias '{self.src_node_name}'",
> + self.vm_b.get_log())
> +
> + def test_migratee_bitmap_is_not_mapped_on_src(self) -> None:
> + mapping: BlockBitmapMapping = [{
> + 'node-name': self.src_node_name,
> + 'alias': self.dst_node_name,
> + 'bitmaps': []
> + }]
> +
> + self.set_mapping(self.vm_a, mapping)
> + # Should just ignore all unmapped bitmaps
> + self.migrate(True, False)
> +
> + def test_migratee_bitmap_is_not_mapped_on_dst(self) -> None:
> + mapping: BlockBitmapMapping = [{
> + 'node-name': self.dst_node_name,
> + 'alias': self.src_node_name,
> + 'bitmaps': []
> + }]
> +
> + self.set_mapping(self.vm_b, mapping)
> + self.migrate(False, False)
> +
> + # Expect abnormal shutdown of the destination VM on migration failure
> + try:
> + self.vm_b.shutdown()
> + except qemu.machine.AbnormalShutdown:
> + pass
> +
> + self.assertIn(f"Unknown bitmap alias '{self.src_bmap_name}' on node " \
> + f"'{self.dst_node_name}' (alias '{self.src_node_name}')",
> + self.vm_b.get_log())
> +
> + def test_non_wellformed_node_alias(self) -> None:
> + mapping: BlockBitmapMapping = [{
> + 'node-name': self.src_bmap_name,
> + 'alias': '123-foo',
> + 'bitmaps': []
> + }]
> +
> + self.set_mapping(self.vm_a, mapping,
> + 'Invalid mapping given for block-bitmap-mapping: ' \
> + 'The node alias 123-foo is not well-formed')
> +
> +
> +class TestCrossAliasMigration(TestDirtyBitmapMigration):
> + """
> + Swap aliases, both to see that qemu does not get confused, and
> + that we can migrate multiple things at once.
> +
> + So we migrate this:
> + node-a.bmap-a -> node-b.bmap-b
> + node-a.bmap-b -> node-b.bmap-a
> + node-b.bmap-a -> node-a.bmap-b
> + node-b.bmap-b -> node-a.bmap-a
> + """
> +
> + src_node_name = 'node-a'
> + dst_node_name = 'node-b'
> + src_bmap_name = 'bmap-a'
> + dst_bmap_name = 'bmap-b'
> +
> + def setUp(self) -> None:
> + TestDirtyBitmapMigration.setUp(self)
> +
> + # Now create another block device and let both have two bitmaps each
> + result = self.vm_a.qmp('blockdev-add',
> + node_name='node-b', driver='null-co')
> + self.assert_qmp(result, 'return', {})
> +
> + result = self.vm_b.qmp('blockdev-add',
> + node_name='node-a', driver='null-co')
> + self.assert_qmp(result, 'return', {})
> +
> + bmaps_to_add = (('node-a', 'bmap-b'),
> + ('node-b', 'bmap-a'),
> + ('node-b', 'bmap-b'))
> +
> + for (node, bmap) in bmaps_to_add:
> + result = self.vm_a.qmp('block-dirty-bitmap-add',
> + node=node, name=bmap)
> + self.assert_qmp(result, 'return', {})
> +
> + @staticmethod
> + def cross_mapping() -> BlockBitmapMapping:
> + return [
> + {
> + 'node-name': 'node-a',
> + 'alias': 'node-b',
> + 'bitmaps': [
> + {
> + 'name': 'bmap-a',
> + 'alias': 'bmap-b'
> + },
> + {
> + 'name': 'bmap-b',
> + 'alias': 'bmap-a'
> + }
> + ]
> + },
> + {
> + 'node-name': 'node-b',
> + 'alias': 'node-a',
> + 'bitmaps': [
> + {
> + 'name': 'bmap-b',
> + 'alias': 'bmap-a'
> + },
> + {
> + 'name': 'bmap-a',
> + 'alias': 'bmap-b'
> + }
> + ]
> + }
> + ]
> +
> + def verify_dest_has_all_bitmaps(self) -> None:
> + bitmaps = self.vm_a.query_bitmaps()
s/vm_a/vm_b/
Ha! I've already thought, that I'll not find any mistake :)
With it fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
(other my notes up to you)
> +
> + # Extract and sort bitmap names
> + for node in bitmaps:
> + bitmaps[node] = sorted((bmap['name'] for bmap in bitmaps[node]))
> +
> + self.assertEqual(bitmaps,
> + {'node-a': ['bmap-a', 'bmap-b'],
> + 'node-b': ['bmap-a', 'bmap-b']})
> +
> + def test_alias_on_src(self) -> None:
> + self.set_mapping(self.vm_a, self.cross_mapping())
> +
> + # Checks that node-a.bmap-a was migrated to node-b.bmap-b, and
> + # that is enough
> + self.migrate()
> +
> + self.verify_dest_has_all_bitmaps()
> +
> + def test_alias_on_dst(self) -> None:
> + self.set_mapping(self.vm_b, self.cross_mapping())
> +
> + # Checks that node-a.bmap-a was migrated to node-b.bmap-b, and
> + # that is enough
> + self.migrate()
> +
> + self.verify_dest_has_all_bitmaps()
> +
> +
> +if __name__ == '__main__':
> + iotests.main(supported_protocols=['file'])
> diff --git a/tests/qemu-iotests/300.out b/tests/qemu-iotests/300.out
> new file mode 100644
> index 0000000000..6d9bee1a4b
> --- /dev/null
> +++ b/tests/qemu-iotests/300.out
> @@ -0,0 +1,5 @@
> +...........................
> +----------------------------------------------------------------------
> +Ran 27 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index c0710cd99e..6b8910a767 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -307,4 +307,5 @@
> 295 rw
> 296 rw
> 297 meta
> +300 migration
> 301 backing quick
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] migration: Add block-bitmap-mapping parameter
2020-07-20 16:31 ` Vladimir Sementsov-Ogievskiy
@ 2020-07-21 8:02 ` Max Reitz
2020-07-22 11:07 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 13+ messages in thread
From: Max Reitz @ 2020-07-21 8:02 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: Kevin Wolf, Peter Krempa, Juan Quintela, John Snow, qemu-devel,
Dr . David Alan Gilbert
[-- Attachment #1.1: Type: text/plain, Size: 10908 bytes --]
On 20.07.20 18:31, Vladimir Sementsov-Ogievskiy wrote:
> 16.07.2020 16:53, Max Reitz wrote:
>> This migration parameter allows mapping block node names and bitmap
>> names to aliases for the purpose of block dirty bitmap migration.
>>
>> This way, management tools can use different node and bitmap names on
>> the source and destination and pass the mapping of how bitmaps are to be
>> transferred to qemu (on the source, the destination, or even both with
>> arbitrary aliases in the migration stream).
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> Vladimir noted in v1 that it would be better to ignore bitmaps whose
>> names aren't mapped, or that are on nodes whose names aren't mapped.
>> One of the reasons he gave was that bitmap migration is inherently a
>> form of postcopy migration, and we should not break the target when it
>> is already running because of a bitmap issue.
>>
>> So in this version, I changed the behavior to just ignore bitmaps
>> without a mapping on the source side. On the destination, however, I
>> kept it an error when an incoming bitmap or node alias is unknown.
>>
>> This is in violation of Vladimir's (rightful) reasoning that such
>> errors should never break the already running target, but I decided to
>> do so anyway for a couple of reasons:
>>
>> - Ignoring unmapped bitmaps on the source is trivial: We just have to
>> not put them into the migration stream.
>> On the destination, it isn't so easy: We (I think) would have to
>> modify the code to actively skip the bitmap in the stream.
>> (On the other hand, erroring out is always easy.)
>
> Agree. Actually, my series "[PATCH v2 00/22] Fix error handling during
> bitmap postcopy"
> do something like this. But no sense in mixing such logic into your
> series :)
>
>>
>> - Incoming bitmaps with unknown names are already an error before this
>> series. So this isn't introducing new breakage.
>>
>> - I think it makes sense to drop all bitmaps you don't want to migrate
>> (or implicitly drop them by just not specifying them if you don't care
>> about them) on the source. I can't imagine a scenario where it would
>> be really useful if the destination could silently drop unknown
>> bitmaps. Unknown bitmaps should just be dropped on the source.
>>
>> - Choosing to make it an error now doesn't prevent us from relaxing that
>> restriction in the future.
>
> Agree, that's all OK. Still we can at least ignore, if we don't get some
> bitmap on destination, for which mapping is set (I think you do exactly
> this, will see below).
>
>
>> ---
>> qapi/migration.json | 92 +++++++-
>> migration/migration.h | 3 +
>> migration/block-dirty-bitmap.c | 373 ++++++++++++++++++++++++++++-----
>> migration/migration.c | 30 +++
>> monitor/hmp-cmds.c | 30 +++
>> 5 files changed, 473 insertions(+), 55 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index d5000558c6..1b0fbcef96 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -507,6 +507,44 @@
>
> [..]
>
>> #
>> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>> +# aliases for the purpose of dirty bitmap migration. Such
>> +# aliases may for example be the corresponding names on the
>> +# opposite site.
>> +# The mapping must be one-to-one, and on the destination also
>> +# complete: On the source, bitmaps on nodes where either the
>> +# bitmap or the node is not mapped will be ignored. In
>> +# contrast, on the destination, receiving a bitmap (by alias)
>> +# from a node (by alias) when either alias is not mapped will
>> +# result in an error.
>
> Still, not receiving some bitmap is not an error. It's good. I think,
> should
> be mentioned here too (does it violate "compelete" term?).
Hm. Well, from the implementation’s perspective, it obviously isn’t an
error, because there’s no list of bitmaps that’s transferred outside of
the bitmaps themselves; so if some bitmap isn’t transferred, the
destination of course never knows about it. (And “complete” just means
that all received bitmaps must be mapped.)
So I never thought about mentioning that detail here.
How would we even go about documenting that? “Note that the destination
does not know about bitmaps it does not receive, so there is no
limitation or requirement about the number of bitmaps received, or how
they are named, or on which nodes they are placed.”
>> +# By default (when this parameter has never been set), bitmap
>> +# names are mapped to themselves. Nodes are mapped to their
>> +# block device name if there is one, and to their node name
>> +# otherwise. (Since 5.2)
>> +#
>> # Since: 2.4
>
> [..]
>
>> -static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState
>> *s)
>> +static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState
>> *s,
>> + GHashTable *alias_map)
>> {
>> + GHashTable *bitmap_alias_map = NULL;
>> Error *local_err = NULL;
>> bool nothing;
>> s->flags = qemu_get_bitmap_flags(f);
>> @@ -676,25 +890,68 @@ static int dirty_bitmap_load_header(QEMUFile *f,
>> DirtyBitmapLoadState *s)
>> nothing = s->flags == (s->flags & DIRTY_BITMAP_MIG_FLAG_EOS);
>> if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
>> - if (!qemu_get_counted_string(f, s->node_name)) {
>> - error_report("Unable to read node name string");
>> + if (!qemu_get_counted_string(f, s->node_alias)) {
>> + error_report("Unable to read node alias string");
>> return -EINVAL;
>> }
>> - s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
>> +
>> + if (alias_map) {
>> + const AliasMapInnerNode *amin;
>> +
>> + amin = g_hash_table_lookup(alias_map, s->node_alias);
>> + if (!amin) {
>> + error_report("Error: Unknown node alias '%s'",
>> s->node_alias);
>> + return -EINVAL;
>> + }
>> +
>> + bitmap_alias_map = amin->subtree;
>> + s->bs = bdrv_lookup_bs(NULL, amin->string, &local_err);
>> + } else {
>> + s->bs = bdrv_lookup_bs(s->node_alias, s->node_alias,
>> &local_err);
>> + }
>> if (!s->bs) {
>> error_report_err(local_err);
>> return -EINVAL;
>> }
>> - } else if (!s->bs && !nothing) {
>> + } else if (s->bs) {
>> + if (alias_map) {
>> + const AliasMapInnerNode *amin;
>> +
>> + /* Must be present in the map, or s->bs would not be set */
>> + amin = g_hash_table_lookup(alias_map, s->node_alias);
>> + assert(amin != NULL);
>> +
>> + bitmap_alias_map = amin->subtree;
>> + }
>> + } else if (!nothing) {
>> error_report("Error: block device name is not set");
>> return -EINVAL;
>> }
>> + assert(!!alias_map == !!bitmap_alias_map);
>
> Actually one '!' is enough. But '!!' looks good too (usual convertion to
> bool, why not).
>
> But what's more serious: this assertion will fail in case of "nothing"
> (sorry my architecture :(.
>
> I assume, by protocol, chunk with EOS flag may contain bitmap and/or
> node information or may not.
>
> So, it most probably should be: assert(nothing || (!alias_map ==
> !bitmap_alias_map))
Right, sure.
> (You can cover "nothing" case in test, if enable bitmap migrations when
> no bitmaps to migrate)
Thanks, will do.
>> +
>> if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
>> - if (!qemu_get_counted_string(f, s->bitmap_name)) {
>> + const char *bitmap_name;
>> +
>> + if (!qemu_get_counted_string(f, s->bitmap_alias)) {
>> error_report("Unable to read bitmap name string");
>> return -EINVAL;
>> }
>> +
>> + if (bitmap_alias_map) {
>> + bitmap_name = g_hash_table_lookup(bitmap_alias_map,
>> + s->bitmap_alias);
>> + if (!bitmap_name) {
>> + error_report("Error: Unknown bitmap alias '%s' on
>> node '%s' "
>> + "(alias '%s')", s->bitmap_alias,
>> s->bs->node_name,
>> + s->node_alias);
>> + return -EINVAL;
>> + }
>> + } else {
>> + bitmap_name = s->bitmap_alias;
>> + }
>> +
>> + g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
>
> Hmm. Actually, we should check that in alias map target bitmap_name is
> short enough. It may be done later.
OK.
>> s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
>> /* bitmap may be NULL here, it wouldn't be an error if it
>> is the
>> @@ -702,7 +959,7 @@ static int dirty_bitmap_load_header(QEMUFile *f,
>> DirtyBitmapLoadState *s)
>> if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
>
>
> OK, with assertion fixed;
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Thanks again!
> It's a bit weak, because:
>
> - I don't have good understanding all these migration parameters logics
> with this triple duplication. So if it works in test, it should be good
> enough.
I hope so, yes.
> - The whole logic of bitmap migration is a bit hard to follow (I know,
> that it's my code :).
O:)
> I'll revisit it when rebasing my big series
> "[PATCH v2 00/22] Fix error handling during bitmap postcopy".
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] iotests.py: Add wait_for_runstate()
2020-07-20 16:46 ` Vladimir Sementsov-Ogievskiy
@ 2020-07-21 8:05 ` Max Reitz
0 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2020-07-21 8:05 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: Kevin Wolf, Peter Krempa, Juan Quintela, John Snow, qemu-devel,
Dr . David Alan Gilbert
[-- Attachment #1.1: Type: text/plain, Size: 1354 bytes --]
On 20.07.20 18:46, Vladimir Sementsov-Ogievskiy wrote:
> 16.07.2020 16:53, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/iotests.py | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/iotests.py
>> b/tests/qemu-iotests/iotests.py
>> index 3590ed78a0..fb240a334c 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -803,6 +803,10 @@ class VM(qtest.QEMUQtestMachine):
>> 'Found node %s under %s (but expected %s)' % \
>> (node['name'], path, expected_node)
>> + def wait_for_runstate(self, runstate: str) -> None:
>> + while self.qmp('query-status')['return']['status'] != runstate:
>> + pass
>
> don't we need something like time.sleep(0.5) instead of just pass?
Hm. Maybe not “need”? I thought this polling would last a short time
anyway, so it wouldn’t matter if there was high CPU usage during that time.
OTOH, I don’t think a short sleep would hurt.
> Anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>> +
>> index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>> class QMPTestCase(unittest.TestCase):
>>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration
2020-07-20 18:02 ` Vladimir Sementsov-Ogievskiy
@ 2020-07-21 8:33 ` Max Reitz
0 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2020-07-21 8:33 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: Kevin Wolf, Peter Krempa, Juan Quintela, John Snow, qemu-devel,
Dr . David Alan Gilbert
[-- Attachment #1.1: Type: text/plain, Size: 7538 bytes --]
On 20.07.20 20:02, Vladimir Sementsov-Ogievskiy wrote:
> 16.07.2020 16:53, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/300 | 511 +++++++++++++++++++++++++++++++++++++
>> tests/qemu-iotests/300.out | 5 +
>> tests/qemu-iotests/group | 1 +
>> 3 files changed, 517 insertions(+)
>> create mode 100755 tests/qemu-iotests/300
>> create mode 100644 tests/qemu-iotests/300.out
>>
>> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
>> new file mode 100755
>> index 0000000000..68714b7167
>> --- /dev/null
>> +++ b/tests/qemu-iotests/300
>> @@ -0,0 +1,511 @@
>> +#!/usr/bin/env python3
>> +#
>> +# Tests for dirty bitmaps migration with node aliases
>
> copyright?
Who needs that.
Will fix. O:)
[...]
>> +class TestDirtyBitmapMigration(iotests.QMPTestCase):
>> + src_node_name: str = ''
>> + dst_node_name: str = ''
>> + src_bmap_name: str = ''
>> + dst_bmap_name: str = ''
>
> Hmm, I hope, typing actually not needed with such an obvious initialization
I think mypy complained for some other variables, so I decided to just
type everything. *shrug*
[...]
>> + # Dirty some random megabytes
>> + for _ in range(9):
>> + mb_ofs = random.randrange(1024)
>> + self.vm_a.hmp_qemu_io(self.src_node_name, 'write %dM 1M'
>> % mb_ofs)
>
> May be, use f-string for consistency
Ah, sure.
[...]
>> + # Wait until the migration has been cleaned up
>> + # (Otherwise, bdrv_close_all() will abort because the
>> + # dirty bitmap migration code still holds a reference to
>> + # the BDS)
>> + # (Unfortunately, there does not appear to be a nicer way
>> + # of waiting until a failed migration has been cleaned up)
>> + timeout_msg = 'Timeout waiting for migration to be
>> cleaned up'
>> + with iotests.Timeout(30, timeout_msg):
>> + while os.path.exists(mig_sock):
>> + pass
>> +
>> + # Dropping src_node_name will only work once the
>> + # bitmap migration code has released it
>> + while 'error' in self.vm_a.qmp('blockdev-del',
>> +
>> node_name=self.src_node_name):
>> + pass
>
> Somehow I would feel calmer with s/pass/time.sleep(0.5)/ in such loops.
Why, if you don’t mind me asking?
I’m always afraid of needlessly adding to the runtime of the test. But
it’s not like I couldn’t go for e.g. a sleep(0.1).
>> +
>> + return
>> +
>> + self.vm_a.wait_for_runstate('postmigrate')
>> + self.vm_b.wait_for_runstate('running')
>
> Actually, bitmaps migration may continue in postcopy, so more correct
> would be to wait for completed status for migration on target. Still,
> shouldn't be a big difference when migrate small bitmap data.
Why not, I can add a wait_for_migration_state() to patch 2 and then use
that here.
>> +
>> + self.check_bitmap(bitmap_name_valid)
[...]
>> + hmp_mapping: List[str] = []
>> + for line in result['return'].split('\n'):
>> + line = line.rstrip()
>> +
>> + if hmp_mapping == []:
>> + if line == 'block-bitmap-mapping:':
>> + hmp_mapping.append(line)
>> + continue
>> +
>> + if line.startswith(' '):
>> + hmp_mapping.append(line)
>> + else:
>> + break
>
> Let me try:
>
> hmp_mapping = re.search(r'^block-bitmap-mapping:.*(\n .*)*',
> result['return'], flags=re.MULTILINE)
Nice, thanks!
>> +
>> + self.assertEqual('\n'.join(hmp_mapping) + '\n',
>> + self.to_hmp_mapping(mapping))
>> + else:
>> + self.assert_qmp(result, 'error/desc', error)
[...]
>> + def test_migratee_node_is_not_mapped_on_src(self) -> None:
>
> is migratee a mistake or an abbreviation for migrate-error ? :)
I meant it as “the node that is being migrated”. As in “employee =
someone who’s employed” or “callee = something that’s being called”.
>> + self.set_mapping(self.vm_a, [])
>> + # Should just ignore all bitmaps on unmapped nodes
>> + self.migrate(True, False)
>> +
>> + def test_migratee_node_is_not_mapped_on_dst(self) -> None:
>> + self.set_mapping(self.vm_b, [])
>> + self.migrate(False, False)
>> +
>> + # Expect abnormal shutdown of the destination VM on migration
>> failure
>> + try:
>> + self.vm_b.shutdown()
>> + except qemu.machine.AbnormalShutdown:
>> + pass
>> +
>> + self.assertIn(f"Unknown node alias '{self.src_node_name}'",
>> + self.vm_b.get_log())
>> +
>> + def test_migratee_bitmap_is_not_mapped_on_src(self) -> None:
>> + mapping: BlockBitmapMapping = [{
>> + 'node-name': self.src_node_name,
>> + 'alias': self.dst_node_name,
>> + 'bitmaps': []
>> + }]
>> +
>> + self.set_mapping(self.vm_a, mapping)
>> + # Should just ignore all unmapped bitmaps
>> + self.migrate(True, False)
>> +
>> + def test_migratee_bitmap_is_not_mapped_on_dst(self) -> None:
>> + mapping: BlockBitmapMapping = [{
>> + 'node-name': self.dst_node_name,
>> + 'alias': self.src_node_name,
>> + 'bitmaps': []
>> + }]
>> +
>> + self.set_mapping(self.vm_b, mapping)
>> + self.migrate(False, False)
>> +
>> + # Expect abnormal shutdown of the destination VM on migration
>> failure
>> + try:
>> + self.vm_b.shutdown()
>> + except qemu.machine.AbnormalShutdown:
>> + pass
>> +
>> + self.assertIn(f"Unknown bitmap alias '{self.src_bmap_name}'
>> on node " \
>> + f"'{self.dst_node_name}' (alias
>> '{self.src_node_name}')",
>> + self.vm_b.get_log())
>> +
[...]
>> + def verify_dest_has_all_bitmaps(self) -> None:
>> + bitmaps = self.vm_a.query_bitmaps()
>
> s/vm_a/vm_b/
>
> Ha! I've already thought, that I'll not find any mistake :)
:)
> With it fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> (other my notes up to you)
Thanks, I’ll take them to heart.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] migration: Add block-bitmap-mapping parameter
2020-07-16 13:53 ` [PATCH v2 1/3] " Max Reitz
2020-07-20 16:31 ` Vladimir Sementsov-Ogievskiy
@ 2020-07-22 9:06 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 13+ messages in thread
From: Dr. David Alan Gilbert @ 2020-07-22 9:06 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, John Snow, qemu-devel, Peter Krempa
* Max Reitz (mreitz@redhat.com) wrote:
> This migration parameter allows mapping block node names and bitmap
> names to aliases for the purpose of block dirty bitmap migration.
>
> This way, management tools can use different node and bitmap names on
> the source and destination and pass the mapping of how bitmaps are to be
> transferred to qemu (on the source, the destination, or even both with
> arbitrary aliases in the migration stream).
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
For both HMP and migration stuff:
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> Vladimir noted in v1 that it would be better to ignore bitmaps whose
> names aren't mapped, or that are on nodes whose names aren't mapped.
> One of the reasons he gave was that bitmap migration is inherently a
> form of postcopy migration, and we should not break the target when it
> is already running because of a bitmap issue.
>
> So in this version, I changed the behavior to just ignore bitmaps
> without a mapping on the source side. On the destination, however, I
> kept it an error when an incoming bitmap or node alias is unknown.
>
> This is in violation of Vladimir's (rightful) reasoning that such
> errors should never break the already running target, but I decided to
> do so anyway for a couple of reasons:
>
> - Ignoring unmapped bitmaps on the source is trivial: We just have to
> not put them into the migration stream.
> On the destination, it isn't so easy: We (I think) would have to
> modify the code to actively skip the bitmap in the stream.
> (On the other hand, erroring out is always easy.)
>
> - Incoming bitmaps with unknown names are already an error before this
> series. So this isn't introducing new breakage.
>
> - I think it makes sense to drop all bitmaps you don't want to migrate
> (or implicitly drop them by just not specifying them if you don't care
> about them) on the source. I can't imagine a scenario where it would
> be really useful if the destination could silently drop unknown
> bitmaps. Unknown bitmaps should just be dropped on the source.
>
> - Choosing to make it an error now doesn't prevent us from relaxing that
> restriction in the future.
> ---
> qapi/migration.json | 92 +++++++-
> migration/migration.h | 3 +
> migration/block-dirty-bitmap.c | 373 ++++++++++++++++++++++++++++-----
> migration/migration.c | 30 +++
> monitor/hmp-cmds.c | 30 +++
> 5 files changed, 473 insertions(+), 55 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d5000558c6..1b0fbcef96 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -507,6 +507,44 @@
> 'data': [ 'none', 'zlib',
> { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>
> +##
> +# @BitmapMigrationBitmapAlias:
> +#
> +# @name: The name of the bitmap.
> +#
> +# @alias: An alias name for migration (for example the bitmap name on
> +# the opposite site).
> +#
> +# Since: 5.1
> +##
> +{ 'struct': 'BitmapMigrationBitmapAlias',
> + 'data': {
> + 'name': 'str',
> + 'alias': 'str'
> + } }
> +
> +##
> +# @BitmapMigrationNodeAlias:
> +#
> +# Maps a block node name and the bitmaps it has to aliases for dirty
> +# bitmap migration.
> +#
> +# @node-name: A block node name.
> +#
> +# @alias: An alias block node name for migration (for example the
> +# node name on the opposite site).
> +#
> +# @bitmaps: Mappings for the bitmaps on this node.
> +#
> +# Since: 5.1
> +##
> +{ 'struct': 'BitmapMigrationNodeAlias',
> + 'data': {
> + 'node-name': 'str',
> + 'alias': 'str',
> + 'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
> + } }
> +
> ##
> # @MigrationParameter:
> #
> @@ -641,6 +679,21 @@
> # will consume more CPU.
> # Defaults to 1. (Since 5.0)
> #
> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
> +# aliases for the purpose of dirty bitmap migration. Such
> +# aliases may for example be the corresponding names on the
> +# opposite site.
> +# The mapping must be one-to-one, and on the destination also
> +# complete: On the source, bitmaps on nodes where either the
> +# bitmap or the node is not mapped will be ignored. In
> +# contrast, on the destination, receiving a bitmap (by alias)
> +# from a node (by alias) when either alias is not mapped will
> +# result in an error.
> +# By default (when this parameter has never been set), bitmap
> +# names are mapped to themselves. Nodes are mapped to their
> +# block device name if there is one, and to their node name
> +# otherwise. (Since 5.2)
> +#
> # Since: 2.4
> ##
> { 'enum': 'MigrationParameter',
> @@ -655,7 +708,8 @@
> 'multifd-channels',
> 'xbzrle-cache-size', 'max-postcopy-bandwidth',
> 'max-cpu-throttle', 'multifd-compression',
> - 'multifd-zlib-level' ,'multifd-zstd-level' ] }
> + 'multifd-zlib-level' ,'multifd-zstd-level',
> + 'block-bitmap-mapping' ] }
>
> ##
> # @MigrateSetParameters:
> @@ -781,6 +835,21 @@
> # will consume more CPU.
> # Defaults to 1. (Since 5.0)
> #
> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
> +# aliases for the purpose of dirty bitmap migration. Such
> +# aliases may for example be the corresponding names on the
> +# opposite site.
> +# The mapping must be one-to-one, and on the destination also
> +# complete: On the source, bitmaps on nodes where either the
> +# bitmap or the node is not mapped will be ignored. In
> +# contrast, on the destination, receiving a bitmap (by alias)
> +# from a node (by alias) when either alias is not mapped will
> +# result in an error.
> +# By default (when this parameter has never been set), bitmap
> +# names are mapped to themselves. Nodes are mapped to their
> +# block device name if there is one, and to their node name
> +# otherwise. (Since 5.2)
> +#
> # Since: 2.4
> ##
> # TODO either fuse back into MigrationParameters, or make
> @@ -811,7 +880,8 @@
> '*max-cpu-throttle': 'int',
> '*multifd-compression': 'MultiFDCompression',
> '*multifd-zlib-level': 'int',
> - '*multifd-zstd-level': 'int' } }
> + '*multifd-zstd-level': 'int',
> + '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>
> ##
> # @migrate-set-parameters:
> @@ -957,6 +1027,21 @@
> # will consume more CPU.
> # Defaults to 1. (Since 5.0)
> #
> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
> +# aliases for the purpose of dirty bitmap migration. Such
> +# aliases may for example be the corresponding names on the
> +# opposite site.
> +# The mapping must be one-to-one, and on the destination also
> +# complete: On the source, bitmaps on nodes where either the
> +# bitmap or the node is not mapped will be ignored. In
> +# contrast, on the destination, receiving a bitmap (by alias)
> +# from a node (by alias) when either alias is not mapped will
> +# result in an error.
> +# By default (when this parameter has never been set), bitmap
> +# names are mapped to themselves. Nodes are mapped to their
> +# block device name if there is one, and to their node name
> +# otherwise. (Since 5.2)
> +#
> # Since: 2.4
> ##
> { 'struct': 'MigrationParameters',
> @@ -985,7 +1070,8 @@
> '*max-cpu-throttle': 'uint8',
> '*multifd-compression': 'MultiFDCompression',
> '*multifd-zlib-level': 'uint8',
> - '*multifd-zstd-level': 'uint8' } }
> + '*multifd-zstd-level': 'uint8',
> + '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>
> ##
> # @query-migrate-parameters:
> diff --git a/migration/migration.h b/migration/migration.h
> index f617960522..038c318b92 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -336,6 +336,9 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
>
> void dirty_bitmap_mig_before_vm_start(void);
> void init_dirty_bitmap_incoming_migration(void);
> +bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
> + Error **errp);
> +
> void migrate_add_address(SocketAddress *address);
>
> int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index b0dbf9eeed..c7945c631b 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -29,10 +29,10 @@
> *
> * # Header (shared for different chunk types)
> * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
> - * [ 1 byte: node name size ] \ flags & DEVICE_NAME
> - * [ n bytes: node name ] /
> - * [ 1 byte: bitmap name size ] \ flags & BITMAP_NAME
> - * [ n bytes: bitmap name ] /
> + * [ 1 byte: node alias size ] \ flags & DEVICE_NAME
> + * [ n bytes: node alias ] /
> + * [ 1 byte: bitmap alias size ] \ flags & BITMAP_NAME
> + * [ n bytes: bitmap alias ] /
> *
> * # Start of bitmap migration (flags & START)
> * header
> @@ -72,7 +72,9 @@
> #include "migration/register.h"
> #include "qemu/hbitmap.h"
> #include "qemu/cutils.h"
> +#include "qemu/id.h"
> #include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
> #include "trace.h"
>
> #define CHUNK_SIZE (1 << 10)
> @@ -103,7 +105,8 @@
> typedef struct DirtyBitmapMigBitmapState {
> /* Written during setup phase. */
> BlockDriverState *bs;
> - const char *node_name;
> + char *node_alias;
> + char *bitmap_alias;
> BdrvDirtyBitmap *bitmap;
> uint64_t total_sectors;
> uint64_t sectors_per_chunk;
> @@ -128,7 +131,8 @@ typedef struct DirtyBitmapMigState {
>
> typedef struct DirtyBitmapLoadState {
> uint32_t flags;
> - char node_name[256];
> + char node_alias[256];
> + char bitmap_alias[256];
> char bitmap_name[256];
> BlockDriverState *bs;
> BdrvDirtyBitmap *bitmap;
> @@ -144,6 +148,162 @@ typedef struct DirtyBitmapLoadBitmapState {
> static GSList *enabled_bitmaps;
> QemuMutex finish_lock;
>
> +/* For hash tables that map node/bitmap names to aliases */
> +typedef struct AliasMapInnerNode {
> + char *string;
> + GHashTable *subtree;
> +} AliasMapInnerNode;
> +
> +static void free_alias_map_inner_node(void *amin_ptr)
> +{
> + AliasMapInnerNode *amin = amin_ptr;
> +
> + g_free(amin->string);
> + g_hash_table_unref(amin->subtree);
> + g_free(amin);
> +}
> +
> +/**
> + * Construct an alias map based on the given QMP structure.
> + *
> + * (Note that we cannot store such maps in the MigrationParameters
> + * object, because that struct is defined by the QAPI schema, which
> + * makes it basically impossible to have dicts with arbitrary keys.
> + * Therefore, we instead have to construct these maps when migration
> + * starts.)
> + *
> + * @bbm is the block_bitmap_mapping from the migration parameters.
> + *
> + * If @name_to_alias is true, the returned hash table will map node
> + * and bitmap names to their respective aliases (for outgoing
> + * migration).
> + *
> + * If @name_to_alias is false, the returned hash table will map node
> + * and bitmap aliases to their respective names (for incoming
> + * migration).
> + *
> + * The hash table maps node names/aliases to AliasMapInnerNode
> + * objects, whose .string is the respective node alias/name, and whose
> + * .subtree table maps bitmap names/aliases to the respective bitmap
> + * alias/name.
> + */
> +static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
> + bool name_to_alias,
> + Error **errp)
> +{
> + GHashTable *alias_map;
> +
> + alias_map = g_hash_table_new_full(g_str_hash, g_str_equal,
> + g_free, free_alias_map_inner_node);
> +
> + for (; bbm; bbm = bbm->next) {
> + const BitmapMigrationNodeAlias *bmna = bbm->value;
> + const BitmapMigrationBitmapAliasList *bmbal;
> + AliasMapInnerNode *amin;
> + GHashTable *bitmaps_map;
> + const char *node_map_from, *node_map_to;
> +
> + if (!id_wellformed(bmna->alias)) {
> + error_setg(errp, "The node alias %s is not well-formed",
> + bmna->alias);
> + goto fail;
> + }
> +
> + if (name_to_alias) {
> + if (g_hash_table_contains(alias_map, bmna->node_name)) {
> + error_setg(errp, "The node name %s is mapped twice",
> + bmna->node_name);
> + goto fail;
> + }
> +
> + node_map_from = bmna->node_name;
> + node_map_to = bmna->alias;
> + } else {
> + if (g_hash_table_contains(alias_map, bmna->alias)) {
> + error_setg(errp, "The node alias %s is used twice",
> + bmna->alias);
> + goto fail;
> + }
> +
> + node_map_from = bmna->alias;
> + node_map_to = bmna->node_name;
> + }
> +
> + bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
> + g_free, g_free);
> +
> + amin = g_new(AliasMapInnerNode, 1);
> + *amin = (AliasMapInnerNode){
> + .string = g_strdup(node_map_to),
> + .subtree = bitmaps_map,
> + };
> +
> + g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
> +
> + for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
> + const BitmapMigrationBitmapAlias *bmba = bmbal->value;
> + const char *bmap_map_from, *bmap_map_to;
> +
> + if (name_to_alias) {
> + bmap_map_from = bmba->name;
> + bmap_map_to = bmba->alias;
> +
> + if (g_hash_table_contains(bitmaps_map, bmba->name)) {
> + error_setg(errp, "The bitmap %s/%s is mapped twice",
> + bmna->node_name, bmba->name);
> + goto fail;
> + }
> + } else {
> + bmap_map_from = bmba->alias;
> + bmap_map_to = bmba->name;
> +
> + if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
> + error_setg(errp, "The bitmap alias %s/%s is used twice",
> + bmna->alias, bmba->alias);
> + goto fail;
> + }
> + }
> +
> + g_hash_table_insert(bitmaps_map,
> + g_strdup(bmap_map_from), g_strdup(bmap_map_to));
> + }
> + }
> +
> + return alias_map;
> +
> +fail:
> + g_hash_table_destroy(alias_map);
> + return NULL;
> +}
> +
> +/**
> + * Run construct_alias_map() in both directions to check whether @bbm
> + * is valid.
> + * (This function is to be used by migration/migration.c to validate
> + * the user-specified block-bitmap-mapping migration parameter.)
> + *
> + * Returns true if and only if the mapping is valid.
> + */
> +bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
> + Error **errp)
> +{
> + GHashTable *alias_map;
> +
> + alias_map = construct_alias_map(bbm, true, errp);
> + if (!alias_map) {
> + return false;
> + }
> + g_hash_table_destroy(alias_map);
> +
> + alias_map = construct_alias_map(bbm, false, errp);
> + if (!alias_map) {
> + return false;
> + }
> + g_hash_table_destroy(alias_map);
> +
> + return true;
> +}
> +
> void init_dirty_bitmap_incoming_migration(void)
> {
> qemu_mutex_init(&finish_lock);
> @@ -191,11 +351,11 @@ static void send_bitmap_header(QEMUFile *f, DirtyBitmapMigBitmapState *dbms,
> qemu_put_bitmap_flags(f, flags);
>
> if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
> - qemu_put_counted_string(f, dbms->node_name);
> + qemu_put_counted_string(f, dbms->node_alias);
> }
>
> if (flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> - qemu_put_counted_string(f, bdrv_dirty_bitmap_name(bitmap));
> + qemu_put_counted_string(f, dbms->bitmap_alias);
> }
> }
>
> @@ -263,17 +423,25 @@ static void dirty_bitmap_mig_cleanup(void)
> QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
> bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
> bdrv_unref(dbms->bs);
> + g_free(dbms->node_alias);
> + g_free(dbms->bitmap_alias);
> g_free(dbms);
> }
> }
>
> /* Called with iothread lock taken. */
> -static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
> +static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name,
> + GHashTable *alias_map)
> {
> BdrvDirtyBitmap *bitmap;
> DirtyBitmapMigBitmapState *dbms;
> + GHashTable *bitmap_aliases;
> + const char *node_alias, *bitmap_name, *bitmap_alias;
> Error *local_err = NULL;
>
> + /* When an alias map is given, @bs_name must be @bs's node name */
> + assert(!alias_map || !strcmp(bs_name, bdrv_get_node_name(bs)));
> +
> FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
> if (bdrv_dirty_bitmap_name(bitmap)) {
> break;
> @@ -283,21 +451,39 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
> return 0;
> }
>
> + bitmap_name = bdrv_dirty_bitmap_name(bitmap);
> +
> if (!bs_name || strcmp(bs_name, "") == 0) {
> error_report("Bitmap '%s' in unnamed node can't be migrated",
> - bdrv_dirty_bitmap_name(bitmap));
> + bitmap_name);
> return -1;
> }
>
> - if (bs_name[0] == '#') {
> + if (alias_map) {
> + const AliasMapInnerNode *amin = g_hash_table_lookup(alias_map, bs_name);
> +
> + if (!amin) {
> + /* Skip bitmaps on nodes with no alias */
> + return 0;
> + }
> +
> + node_alias = amin->string;
> + bitmap_aliases = amin->subtree;
> + } else {
> + node_alias = bs_name;
> + bitmap_aliases = NULL;
> + }
> +
> + if (node_alias[0] == '#') {
> error_report("Bitmap '%s' in a node with auto-generated "
> "name '%s' can't be migrated",
> - bdrv_dirty_bitmap_name(bitmap), bs_name);
> + bitmap_name, node_alias);
> return -1;
> }
>
> FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
> - if (!bdrv_dirty_bitmap_name(bitmap)) {
> + bitmap_name = bdrv_dirty_bitmap_name(bitmap);
> + if (!bitmap_name) {
> continue;
> }
>
> @@ -306,12 +492,23 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
> return -1;
> }
>
> + if (bitmap_aliases) {
> + bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
> + if (!bitmap_alias) {
> + /* Skip bitmaps with no alias */
> + continue;
> + }
> + } else {
> + bitmap_alias = bitmap_name;
> + }
> +
> bdrv_ref(bs);
> bdrv_dirty_bitmap_set_busy(bitmap, true);
>
> dbms = g_new0(DirtyBitmapMigBitmapState, 1);
> dbms->bs = bs;
> - dbms->node_name = bs_name;
> + dbms->node_alias = g_strdup(node_alias);
> + dbms->bitmap_alias = g_strdup(bitmap_alias);
> dbms->bitmap = bitmap;
> dbms->total_sectors = bdrv_nb_sectors(bs);
> dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
> @@ -337,43 +534,52 @@ static int init_dirty_bitmap_migration(void)
> DirtyBitmapMigBitmapState *dbms;
> GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
> BlockBackend *blk;
> + const MigrationParameters *mig_params = &migrate_get_current()->parameters;
> + GHashTable *alias_map = NULL;
> +
> + if (mig_params->has_block_bitmap_mapping) {
> + alias_map = construct_alias_map(mig_params->block_bitmap_mapping, true,
> + &error_abort);
> + }
>
> dirty_bitmap_mig_state.bulk_completed = false;
> dirty_bitmap_mig_state.prev_bs = NULL;
> dirty_bitmap_mig_state.prev_bitmap = NULL;
> dirty_bitmap_mig_state.no_bitmaps = false;
>
> - /*
> - * Use blockdevice name for direct (or filtered) children of named block
> - * backends.
> - */
> - for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> - const char *name = blk_name(blk);
> -
> - if (!name || strcmp(name, "") == 0) {
> - continue;
> - }
> + if (!alias_map) {
> + /*
> + * Use blockdevice name for direct (or filtered) children of named block
> + * backends.
> + */
> + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> + const char *name = blk_name(blk);
>
> - bs = blk_bs(blk);
> + if (!name || strcmp(name, "") == 0) {
> + continue;
> + }
>
> - /* Skip filters without bitmaps */
> - while (bs && bs->drv && bs->drv->is_filter &&
> - !bdrv_has_named_bitmaps(bs))
> - {
> - if (bs->backing) {
> - bs = bs->backing->bs;
> - } else if (bs->file) {
> - bs = bs->file->bs;
> - } else {
> - bs = NULL;
> + bs = blk_bs(blk);
> +
> + /* Skip filters without bitmaps */
> + while (bs && bs->drv && bs->drv->is_filter &&
> + !bdrv_has_named_bitmaps(bs))
> + {
> + if (bs->backing) {
> + bs = bs->backing->bs;
> + } else if (bs->file) {
> + bs = bs->file->bs;
> + } else {
> + bs = NULL;
> + }
> }
> - }
>
> - if (bs && bs->drv && !bs->drv->is_filter) {
> - if (add_bitmaps_to_list(bs, name)) {
> - goto fail;
> + if (bs && bs->drv && !bs->drv->is_filter) {
> + if (add_bitmaps_to_list(bs, name, NULL)) {
> + goto fail;
> + }
> + g_hash_table_add(handled_by_blk, bs);
> }
> - g_hash_table_add(handled_by_blk, bs);
> }
> }
>
> @@ -382,7 +588,7 @@ static int init_dirty_bitmap_migration(void)
> continue;
> }
>
> - if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs))) {
> + if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs), alias_map)) {
> goto fail;
> }
> }
> @@ -397,11 +603,17 @@ static int init_dirty_bitmap_migration(void)
> }
>
> g_hash_table_destroy(handled_by_blk);
> + if (alias_map) {
> + g_hash_table_destroy(alias_map);
> + }
>
> return 0;
>
> fail:
> g_hash_table_destroy(handled_by_blk);
> + if (alias_map) {
> + g_hash_table_destroy(alias_map);
> + }
> dirty_bitmap_mig_cleanup();
>
> return -1;
> @@ -666,8 +878,10 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DirtyBitmapLoadState *s)
> return 0;
> }
>
> -static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
> +static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s,
> + GHashTable *alias_map)
> {
> + GHashTable *bitmap_alias_map = NULL;
> Error *local_err = NULL;
> bool nothing;
> s->flags = qemu_get_bitmap_flags(f);
> @@ -676,25 +890,68 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
> nothing = s->flags == (s->flags & DIRTY_BITMAP_MIG_FLAG_EOS);
>
> if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
> - if (!qemu_get_counted_string(f, s->node_name)) {
> - error_report("Unable to read node name string");
> + if (!qemu_get_counted_string(f, s->node_alias)) {
> + error_report("Unable to read node alias string");
> return -EINVAL;
> }
> - s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
> +
> + if (alias_map) {
> + const AliasMapInnerNode *amin;
> +
> + amin = g_hash_table_lookup(alias_map, s->node_alias);
> + if (!amin) {
> + error_report("Error: Unknown node alias '%s'", s->node_alias);
> + return -EINVAL;
> + }
> +
> + bitmap_alias_map = amin->subtree;
> + s->bs = bdrv_lookup_bs(NULL, amin->string, &local_err);
> + } else {
> + s->bs = bdrv_lookup_bs(s->node_alias, s->node_alias, &local_err);
> + }
> if (!s->bs) {
> error_report_err(local_err);
> return -EINVAL;
> }
> - } else if (!s->bs && !nothing) {
> + } else if (s->bs) {
> + if (alias_map) {
> + const AliasMapInnerNode *amin;
> +
> + /* Must be present in the map, or s->bs would not be set */
> + amin = g_hash_table_lookup(alias_map, s->node_alias);
> + assert(amin != NULL);
> +
> + bitmap_alias_map = amin->subtree;
> + }
> + } else if (!nothing) {
> error_report("Error: block device name is not set");
> return -EINVAL;
> }
>
> + assert(!!alias_map == !!bitmap_alias_map);
> +
> if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> - if (!qemu_get_counted_string(f, s->bitmap_name)) {
> + const char *bitmap_name;
> +
> + if (!qemu_get_counted_string(f, s->bitmap_alias)) {
> error_report("Unable to read bitmap name string");
> return -EINVAL;
> }
> +
> + if (bitmap_alias_map) {
> + bitmap_name = g_hash_table_lookup(bitmap_alias_map,
> + s->bitmap_alias);
> + if (!bitmap_name) {
> + error_report("Error: Unknown bitmap alias '%s' on node '%s' "
> + "(alias '%s')", s->bitmap_alias, s->bs->node_name,
> + s->node_alias);
> + return -EINVAL;
> + }
> + } else {
> + bitmap_name = s->bitmap_alias;
> + }
> +
> + g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
> s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
>
> /* bitmap may be NULL here, it wouldn't be an error if it is the
> @@ -702,7 +959,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
> if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
> error_report("Error: unknown dirty bitmap "
> "'%s' for block device '%s'",
> - s->bitmap_name, s->node_name);
> + s->bitmap_name, s->bs->node_name);
> return -EINVAL;
> }
> } else if (!s->bitmap && !nothing) {
> @@ -715,6 +972,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
>
> static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
> {
> + GHashTable *alias_map = NULL;
> + const MigrationParameters *mig_params = &migrate_get_current()->parameters;
> static DirtyBitmapLoadState s;
> int ret = 0;
>
> @@ -724,10 +983,15 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
> return -EINVAL;
> }
>
> + if (mig_params->has_block_bitmap_mapping) {
> + alias_map = construct_alias_map(mig_params->block_bitmap_mapping,
> + false, &error_abort);
> + }
> +
> do {
> - ret = dirty_bitmap_load_header(f, &s);
> + ret = dirty_bitmap_load_header(f, &s, alias_map);
> if (ret < 0) {
> - return ret;
> + goto fail;
> }
>
> if (s.flags & DIRTY_BITMAP_MIG_FLAG_START) {
> @@ -743,12 +1007,17 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
> }
>
> if (ret) {
> - return ret;
> + goto fail;
> }
> } while (!(s.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
>
> trace_dirty_bitmap_load_success();
> - return 0;
> + ret = 0;
> +fail:
> + if (alias_map) {
> + g_hash_table_destroy(alias_map);
> + }
> + return ret;
> }
>
> static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
> diff --git a/migration/migration.c b/migration/migration.c
> index 2ed9923227..bcf7c3bf76 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -36,6 +36,7 @@
> #include "block/block.h"
> #include "qapi/error.h"
> #include "qapi/clone-visitor.h"
> +#include "qapi/qapi-visit-migration.h"
> #include "qapi/qapi-visit-sockets.h"
> #include "qapi/qapi-commands-migration.h"
> #include "qapi/qapi-events-migration.h"
> @@ -832,6 +833,13 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
> params->has_announce_step = true;
> params->announce_step = s->parameters.announce_step;
>
> + if (s->parameters.has_block_bitmap_mapping) {
> + params->has_block_bitmap_mapping = true;
> + params->block_bitmap_mapping =
> + QAPI_CLONE(BitmapMigrationNodeAliasList,
> + s->parameters.block_bitmap_mapping);
> + }
> +
> return params;
> }
>
> @@ -1297,6 +1305,13 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
> "is invalid, it must be in the range of 1 to 10000 ms");
> return false;
> }
> +
> + if (params->has_block_bitmap_mapping &&
> + !check_dirty_bitmap_mig_alias_map(params->block_bitmap_mapping, errp)) {
> + error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
> + return false;
> + }
> +
> return true;
> }
>
> @@ -1391,6 +1406,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
> if (params->has_announce_step) {
> dest->announce_step = params->announce_step;
> }
> +
> + if (params->has_block_bitmap_mapping) {
> + dest->has_block_bitmap_mapping = true;
> + dest->block_bitmap_mapping = params->block_bitmap_mapping;
> + }
> }
>
> static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
> @@ -1503,6 +1523,16 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
> if (params->has_announce_step) {
> s->parameters.announce_step = params->announce_step;
> }
> +
> + if (params->has_block_bitmap_mapping) {
> + qapi_free_BitmapMigrationNodeAliasList(
> + s->parameters.block_bitmap_mapping);
> +
> + s->parameters.has_block_bitmap_mapping = true;
> + s->parameters.block_bitmap_mapping =
> + QAPI_CLONE(BitmapMigrationNodeAliasList,
> + params->block_bitmap_mapping);
> + }
> }
>
> void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index ae4b6a4246..7711726fd2 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -469,6 +469,32 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
> monitor_printf(mon, "%s: '%s'\n",
> MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
> params->tls_authz);
> +
> + if (params->has_block_bitmap_mapping) {
> + const BitmapMigrationNodeAliasList *bmnal;
> +
> + monitor_printf(mon, "%s:\n",
> + MigrationParameter_str(
> + MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING));
> +
> + for (bmnal = params->block_bitmap_mapping;
> + bmnal;
> + bmnal = bmnal->next)
> + {
> + const BitmapMigrationNodeAlias *bmna = bmnal->value;
> + const BitmapMigrationBitmapAliasList *bmbal;
> +
> + monitor_printf(mon, " '%s' -> '%s'\n",
> + bmna->node_name, bmna->alias);
> +
> + for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
> + const BitmapMigrationBitmapAlias *bmba = bmbal->value;
> +
> + monitor_printf(mon, " '%s' -> '%s'\n",
> + bmba->name, bmba->alias);
> + }
> + }
> + }
> }
>
> qapi_free_MigrationParameters(params);
> @@ -1384,6 +1410,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
> p->has_announce_step = true;
> visit_type_size(v, param, &p->announce_step, &err);
> break;
> + case MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING:
> + error_setg(&err, "The block-bitmap-mapping parameter can only be set "
> + "through QMP");
> + break;
> default:
> assert(0);
> }
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter
2020-07-16 13:53 [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter Max Reitz
` (2 preceding siblings ...)
2020-07-16 13:53 ` [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration Max Reitz
@ 2020-07-22 9:17 ` Dr. David Alan Gilbert
3 siblings, 0 replies; 13+ messages in thread
From: Dr. David Alan Gilbert @ 2020-07-22 9:17 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, John Snow, qemu-devel, Peter Krempa
* Max Reitz (mreitz@redhat.com) wrote:
> RFC v1: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00912.html
> RFC v2: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00915.html
> v1: https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg09792.html
>
> Branch: https://github.com/XanClic/qemu.git migration-bitmap-mapping-v2
> Branch: https://git.xanclic.moe/XanClic/qemu.git migration-bitmap-mapping-v2
>
> Based-on: <20200626130658.76498-1-vsementsov@virtuozzo.com>
> (“migration/block-dirty-bitmap: fix add_bitmaps_to_list”)
>
>
> Hi,
>
> This new migration parameter allows mapping block node names and bitmap
> names to aliases for the purpose of block dirty bitmap migration.
One random thought is that you might find these block name aliases turn
out to be useful in other places, and an alias list may well turn out to
be generically useful.
Dave
> This way, management tools can use different node names on the source
> and destination and pass the mapping of how bitmaps are to be
> transferred to qemu (on the source, the destination, or even both with
> arbitrary aliases in the migration stream).
>
>
> v2:
> - Dropped what used to be patch 1 (the memleak fix), I see the exact
> same fix has been sent concurrently and has been merged as
> 9728ebfb77f0159f4
>
> - Patch 1:
> - Changed documentation to clarify the default behavior
> - s/5.1/5.2/
> - Dropped dead assignment
> - Fixed bitmaps_map memleak
> - Assert that the bs_name given to add_bitmaps_to_list() must be the
> BDS’s node name if an alias_map is given
> - On the source side, unmapped bitmaps are simply dropped without
> error
> - On the destination side, unmapped aliases still result in errors
> (see patch 1 for a short explanation on my reasoning)
> - Fixed a bug in qmp_query_migrate_parameters(): We have to clone
> s->parameters.block_bitmap_mapping, not
> params->block_bitmap_mapping, or the latter will just stay NULL (and
> so qmp_query_migrate_parameters() won’t return a result for the
> block-bitmap-mapping)
> - Emit the mapping through HMP’s “info migrate_parameters”
> - Return an error when trying to set the mapping through HMP (instead
> of just crashing because of an “assert(0)” signalling an unhandled
> migration parameter)
>
> - Patch 3:
> - Type alias for BlockBitmapMapping
> - Check the result of “info migrate_parameters” whenever setting the
> block-bitmap-mapping (just to test the new formatting code)
> - Catch the qemu.machine.AbnormalShutdown exception on the destination
> VM whenever the migration is expected to fail
> (necessary since commit ef5d474472426eda6abf81)
> - Cases where we don’t set up a mapping for some bitmap on the source
> are now expected to succeed (without the bitmap being migrated)
>
>
> git-backport-diff against v1:
>
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
>
> 001/3:[0117] [FC] 'migration: Add block-bitmap-mapping parameter'
> 002/3:[----] [--] 'iotests.py: Add wait_for_runstate()'
> 003/3:[0202] [FC] 'iotests: Test node/bitmap aliases during migration'
>
>
> Max Reitz (3):
> migration: Add block-bitmap-mapping parameter
> iotests.py: Add wait_for_runstate()
> iotests: Test node/bitmap aliases during migration
>
> qapi/migration.json | 92 +++++-
> migration/migration.h | 3 +
> migration/block-dirty-bitmap.c | 373 ++++++++++++++++++++----
> migration/migration.c | 30 ++
> monitor/hmp-cmds.c | 30 ++
> tests/qemu-iotests/300 | 511 +++++++++++++++++++++++++++++++++
> tests/qemu-iotests/300.out | 5 +
> tests/qemu-iotests/group | 1 +
> tests/qemu-iotests/iotests.py | 4 +
> 9 files changed, 994 insertions(+), 55 deletions(-)
> create mode 100755 tests/qemu-iotests/300
> create mode 100644 tests/qemu-iotests/300.out
>
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] migration: Add block-bitmap-mapping parameter
2020-07-21 8:02 ` Max Reitz
@ 2020-07-22 11:07 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 13+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-22 11:07 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: Kevin Wolf, Peter Krempa, Juan Quintela, John Snow, qemu-devel,
Dr . David Alan Gilbert
21.07.2020 11:02, Max Reitz wrote:
> On 20.07.20 18:31, Vladimir Sementsov-Ogievskiy wrote:
>> 16.07.2020 16:53, Max Reitz wrote:
>>> This migration parameter allows mapping block node names and bitmap
>>> names to aliases for the purpose of block dirty bitmap migration.
>>>
>>> This way, management tools can use different node and bitmap names on
>>> the source and destination and pass the mapping of how bitmaps are to be
>>> transferred to qemu (on the source, the destination, or even both with
>>> arbitrary aliases in the migration stream).
>>>
>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>> Vladimir noted in v1 that it would be better to ignore bitmaps whose
>>> names aren't mapped, or that are on nodes whose names aren't mapped.
>>> One of the reasons he gave was that bitmap migration is inherently a
>>> form of postcopy migration, and we should not break the target when it
>>> is already running because of a bitmap issue.
>>>
>>> So in this version, I changed the behavior to just ignore bitmaps
>>> without a mapping on the source side. On the destination, however, I
>>> kept it an error when an incoming bitmap or node alias is unknown.
>>>
>>> This is in violation of Vladimir's (rightful) reasoning that such
>>> errors should never break the already running target, but I decided to
>>> do so anyway for a couple of reasons:
>>>
>>> - Ignoring unmapped bitmaps on the source is trivial: We just have to
>>> not put them into the migration stream.
>>> On the destination, it isn't so easy: We (I think) would have to
>>> modify the code to actively skip the bitmap in the stream.
>>> (On the other hand, erroring out is always easy.)
>>
>> Agree. Actually, my series "[PATCH v2 00/22] Fix error handling during
>> bitmap postcopy"
>> do something like this. But no sense in mixing such logic into your
>> series :)
>>
>>>
>>> - Incoming bitmaps with unknown names are already an error before this
>>> series. So this isn't introducing new breakage.
>>>
>>> - I think it makes sense to drop all bitmaps you don't want to migrate
>>> (or implicitly drop them by just not specifying them if you don't care
>>> about them) on the source. I can't imagine a scenario where it would
>>> be really useful if the destination could silently drop unknown
>>> bitmaps. Unknown bitmaps should just be dropped on the source.
>>>
>>> - Choosing to make it an error now doesn't prevent us from relaxing that
>>> restriction in the future.
>>
>> Agree, that's all OK. Still we can at least ignore, if we don't get some
>> bitmap on destination, for which mapping is set (I think you do exactly
>> this, will see below).
>>
>>
>>> ---
>>> qapi/migration.json | 92 +++++++-
>>> migration/migration.h | 3 +
>>> migration/block-dirty-bitmap.c | 373 ++++++++++++++++++++++++++++-----
>>> migration/migration.c | 30 +++
>>> monitor/hmp-cmds.c | 30 +++
>>> 5 files changed, 473 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index d5000558c6..1b0fbcef96 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -507,6 +507,44 @@
>>
>> [..]
>>
>>> #
>>> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>>> +# aliases for the purpose of dirty bitmap migration. Such
>>> +# aliases may for example be the corresponding names on the
>>> +# opposite site.
>>> +# The mapping must be one-to-one, and on the destination also
>>> +# complete: On the source, bitmaps on nodes where either the
>>> +# bitmap or the node is not mapped will be ignored. In
>>> +# contrast, on the destination, receiving a bitmap (by alias)
>>> +# from a node (by alias) when either alias is not mapped will
>>> +# result in an error.
>>
>> Still, not receiving some bitmap is not an error. It's good. I think,
>> should
>> be mentioned here too (does it violate "compelete" term?).
>
> Hm. Well, from the implementation’s perspective, it obviously isn’t an
> error, because there’s no list of bitmaps that’s transferred outside of
> the bitmaps themselves; so if some bitmap isn’t transferred, the
> destination of course never knows about it. (And “complete” just means
> that all received bitmaps must be mapped.)
>
> So I never thought about mentioning that detail here.
>
> How would we even go about documenting that? “Note that the destination
> does not know about bitmaps it does not receive, so there is no
> limitation or requirement about the number of bitmaps received, or how
> they are named, or on which nodes they are placed.”
Destination "knows" about bitmaps to receive, if block-bitmap-mapping set
on destination.. Hm, but yes, mapping is not a list of bitmaps to receive,
it's just mapping. May be, "Note that it's not an error if source qemu doesn't find
all bitmaps specified in mapping or destination doesn't receive all such
bitmaps"? But I'm OK without any additional note as well.
>
>>> +# By default (when this parameter has never been set), bitmap
>>> +# names are mapped to themselves. Nodes are mapped to their
>>> +# block device name if there is one, and to their node name
>>> +# otherwise. (Since 5.2)
>>> +#
>>> # Since: 2.4
>>
>> [..]
>>
>>> -static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState
>>> *s)
>>> +static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState
>>> *s,
>>> + GHashTable *alias_map)
>>> {
>>> + GHashTable *bitmap_alias_map = NULL;
>>> Error *local_err = NULL;
>>> bool nothing;
>>> s->flags = qemu_get_bitmap_flags(f);
>>> @@ -676,25 +890,68 @@ static int dirty_bitmap_load_header(QEMUFile *f,
>>> DirtyBitmapLoadState *s)
>>> nothing = s->flags == (s->flags & DIRTY_BITMAP_MIG_FLAG_EOS);
>>> if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
>>> - if (!qemu_get_counted_string(f, s->node_name)) {
>>> - error_report("Unable to read node name string");
>>> + if (!qemu_get_counted_string(f, s->node_alias)) {
>>> + error_report("Unable to read node alias string");
>>> return -EINVAL;
>>> }
>>> - s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
>>> +
>>> + if (alias_map) {
>>> + const AliasMapInnerNode *amin;
>>> +
>>> + amin = g_hash_table_lookup(alias_map, s->node_alias);
>>> + if (!amin) {
>>> + error_report("Error: Unknown node alias '%s'",
>>> s->node_alias);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + bitmap_alias_map = amin->subtree;
>>> + s->bs = bdrv_lookup_bs(NULL, amin->string, &local_err);
>>> + } else {
>>> + s->bs = bdrv_lookup_bs(s->node_alias, s->node_alias,
>>> &local_err);
>>> + }
>>> if (!s->bs) {
>>> error_report_err(local_err);
>>> return -EINVAL;
>>> }
>>> - } else if (!s->bs && !nothing) {
>>> + } else if (s->bs) {
>>> + if (alias_map) {
>>> + const AliasMapInnerNode *amin;
>>> +
>>> + /* Must be present in the map, or s->bs would not be set */
>>> + amin = g_hash_table_lookup(alias_map, s->node_alias);
>>> + assert(amin != NULL);
>>> +
>>> + bitmap_alias_map = amin->subtree;
>>> + }
>>> + } else if (!nothing) {
>>> error_report("Error: block device name is not set");
>>> return -EINVAL;
>>> }
>>> + assert(!!alias_map == !!bitmap_alias_map);
>>
>> Actually one '!' is enough. But '!!' looks good too (usual convertion to
>> bool, why not).
>>
>> But what's more serious: this assertion will fail in case of "nothing"
>> (sorry my architecture :(.
>>
>> I assume, by protocol, chunk with EOS flag may contain bitmap and/or
>> node information or may not.
>>
>> So, it most probably should be: assert(nothing || (!alias_map ==
>> !bitmap_alias_map))
>
> Right, sure.
>
>> (You can cover "nothing" case in test, if enable bitmap migrations when
>> no bitmaps to migrate)
>
> Thanks, will do.
>
>>> +
>>> if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
>>> - if (!qemu_get_counted_string(f, s->bitmap_name)) {
>>> + const char *bitmap_name;
>>> +
>>> + if (!qemu_get_counted_string(f, s->bitmap_alias)) {
>>> error_report("Unable to read bitmap name string");
>>> return -EINVAL;
>>> }
>>> +
>>> + if (bitmap_alias_map) {
>>> + bitmap_name = g_hash_table_lookup(bitmap_alias_map,
>>> + s->bitmap_alias);
>>> + if (!bitmap_name) {
>>> + error_report("Error: Unknown bitmap alias '%s' on
>>> node '%s' "
>>> + "(alias '%s')", s->bitmap_alias,
>>> s->bs->node_name,
>>> + s->node_alias);
>>> + return -EINVAL;
>>> + }
>>> + } else {
>>> + bitmap_name = s->bitmap_alias;
>>> + }
>>> +
>>> + g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
>>
>> Hmm. Actually, we should check that in alias map target bitmap_name is
>> short enough. It may be done later.
>
> OK.
>
>>> s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
>>> /* bitmap may be NULL here, it wouldn't be an error if it
>>> is the
>>> @@ -702,7 +959,7 @@ static int dirty_bitmap_load_header(QEMUFile *f,
>>> DirtyBitmapLoadState *s)
>>> if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
>>
>>
>> OK, with assertion fixed;
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Thanks again!
>
>> It's a bit weak, because:
>>
>> - I don't have good understanding all these migration parameters logics
>> with this triple duplication. So if it works in test, it should be good
>> enough.
>
> I hope so, yes.
>
>> - The whole logic of bitmap migration is a bit hard to follow (I know,
>> that it's my code :).
>
> O:)
>
>> I'll revisit it when rebasing my big series
>> "[PATCH v2 00/22] Fix error handling during bitmap postcopy".
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-07-22 11:08 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 13:53 [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter Max Reitz
2020-07-16 13:53 ` [PATCH v2 1/3] " Max Reitz
2020-07-20 16:31 ` Vladimir Sementsov-Ogievskiy
2020-07-21 8:02 ` Max Reitz
2020-07-22 11:07 ` Vladimir Sementsov-Ogievskiy
2020-07-22 9:06 ` Dr. David Alan Gilbert
2020-07-16 13:53 ` [PATCH v2 2/3] iotests.py: Add wait_for_runstate() Max Reitz
2020-07-20 16:46 ` Vladimir Sementsov-Ogievskiy
2020-07-21 8:05 ` Max Reitz
2020-07-16 13:53 ` [PATCH v2 3/3] iotests: Test node/bitmap aliases during migration Max Reitz
2020-07-20 18:02 ` Vladimir Sementsov-Ogievskiy
2020-07-21 8:33 ` Max Reitz
2020-07-22 9:17 ` [PATCH v2 0/3] migration: Add block-bitmap-mapping parameter Dr. David Alan Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.