|
| From: | Vladimir Sementsov-Ogievskiy |
| Subject: | Re: [PATCH v2 1/3] migration: Add block-bitmap-mapping parameter |
| Date: | Wed, 22 Jul 2020 14:07:58 +0300 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
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
| [Prev in Thread] | Current Thread | [Next in Thread] |