Re: [PATCH v2 1/3] migration: Add block-bitmap-mapping parameter

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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