Re: [RFC v2] migration: Add migrate-set-bitmap-node-mapping

[Vladimir Sementsov-Ogievskiy]

13.05.2020 17:56, Max Reitz wrote:
> This command allows mapping block node 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).
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Signed-off-by: Max Reitz <address@hidden>
> ---
> Branch: https://github.com/XanClic/qemu.git migration-bitmap-mapping-rfc-v2
> Branch: https://git.xanclic.moe/XanClic/qemu.git migration-bitmap-mapping-rfc-v2
>
> (Sorry, v1 was just broken.  This one should work better.)
>
> Vladimir has proposed something like this in April:
> https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00171.html
>
> Now I’ve been asked by my manager to look at this, so I decided to just
> write a patch to see how it’d play out.

Great! Sometimes I remember about this thing, but never start implementing :)

> This is an RFC, because I’d like to tack on tests to the final version,
> but I’m not sure whether I can come up with something before the end of
> the week (and I’ll be on PTO for the next two weeks).
>
> Also, I don’t know whether migration/block-dirty-bitmap.c is the best
> place to put qmp_migrate_set_bitmap_mapping(), but it appears we already
> have some QMP handlers in migration/, so I suppose it isn’t too bad.
> ---
>   qapi/migration.json            | 36 ++++++++++++++++++++
>   migration/block-dirty-bitmap.c | 60 ++++++++++++++++++++++++++++++++--
>   2 files changed, 94 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d5000558c6..97037ea635 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1621,3 +1621,39 @@
>   ##
>   { 'event': 'UNPLUG_PRIMARY',
>     'data': { 'device-id': 'str' } }
> +
> +##
> +# @MigrationBlockNodeMapping:
> +#
> +# Maps a block node name to an alias for migration.
> +#
> +# @node-name: A block node name.
> +#
> +# @alias: An alias name for migration (for example the node name on
> +#         the opposite site).
> +#
> +# Since: 5.1
> +##
> +{ 'struct': 'MigrationBlockNodeMapping',
> +  'data': {
> +      'node-name': 'str',
> +      'alias': 'str'
> +  } }
> +
> +##
> +# @migrate-set-bitmap-node-mapping:
> +#
> +# Maps block node names to arbitrary aliases for the purpose of dirty
> +# bitmap migration.  Such aliases may for example be the corresponding
> +# node names on the opposite site.
> +#
> +# By default, every node name is mapped to itself.
> +#
> +# @mapping: The mapping; must be one-to-one, but not necessarily
> +#           complete.  Any mapping not given will be reset to the
> +#           default (i.e. the identity mapping).
> +#
> +# Since: 5.1
> +##
> +{ 'command': 'migrate-set-bitmap-node-mapping',
> +  'data': { 'mapping': ['MigrationBlockNodeMapping'] } }

Hm. I like it, it's simpler and clearer than what I was thinking about.

1. So, you decided to make only node-mapping, not bitmap-mapping, so we can't 
rename bitmaps in-flight and can't migrate bitmaps from one node to several and 
visa-versa. I think it's OK, nothing good in such possibilities, and this 
simplifies things.

2. If I understand correctly, default to node-name matching doesn't make real 
sense for libvirt.. But on the other hand, libvirt should not be considered as 
the ony user of Qemu. Still, the default seems unsafe.. Could we make it 
optional? Or add an option to disable this default for absolutely strict 
behavior?

May be, add a parameter

fallback: node-name | error | drop

where
  node-name: use node-name as an alias, if found bitmap on the node not 
mentioned in @mapping [should not be useful for libvirt, but may be for others]
  error: just error-out if such bitmap found [libvirt should use it, I propose 
it as a default value for @fallback]
  drop: just ignore such bitmap - it will be lost [just and idea, I doubt that 
it is really useful]

=======

Also, we should understand (and document here), that default behavior of this 
command and default behavior of bitmap migration (without this command) are 
different things:

if command is not called, device names may be used instead of node-names.

> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 7eafface61..73f400e7ea 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c

conflicts with my series "[PATCH v2 00/22] Fix error handling during bitmap 
postcopy"..Still, not too difficult to rebase my series if this goes first.

> @@ -73,6 +73,8 @@
>   #include "qemu/hbitmap.h"
>   #include "qemu/cutils.h"
>   #include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
> +#include "qapi/qmp/qdict.h"
>   #include "trace.h"
>   
>   #define CHUNK_SIZE     (1 << 10)
> @@ -121,6 +123,9 @@ typedef struct DirtyBitmapMigState {
>       bool bulk_completed;
>       bool no_bitmaps;
>   
> +    QDict *node_in_mapping;
> +    QDict *node_out_mapping;
> +
>       /* for send_bitmap_bits() */
>       BlockDriverState *prev_bs;
>       BdrvDirtyBitmap *prev_bitmap;
> @@ -281,8 +286,13 @@ static int init_dirty_bitmap_migration(void)
>       dirty_bitmap_mig_state.no_bitmaps = false;
>   
>       for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
> +        const QDict *map = dirty_bitmap_mig_state.node_out_mapping;
>           const char *name = bdrv_get_device_or_node_name(bs);
>   
> +        if (map) {
> +            name = qdict_get_try_str(map, name) ?: name;
> +        }
> +
>           FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>               if (!bdrv_dirty_bitmap_name(bitmap)) {
>                   continue;
> @@ -600,6 +610,8 @@ static int dirty_bitmap_load_bits(QEMUFile *f, 
> DirtyBitmapLoadState *s)
>   
>   static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
>   {
> +    const QDict *map = dirty_bitmap_mig_state.node_in_mapping;
> +    const char *mapped_node = "(none)";
>       Error *local_err = NULL;
>       bool nothing;
>       s->flags = qemu_get_bitmap_flags(f);
> @@ -612,7 +624,13 @@ static int dirty_bitmap_load_header(QEMUFile *f, 
> DirtyBitmapLoadState *s)
>               error_report("Unable to read node name string");
>               return -EINVAL;
>           }
> -        s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
> +
> +        mapped_node = s->node_name;

I think, we can rename s->node_name to alias.. as well as in other places in 
the code, including migration format specification in the comment at file top.

> +        if (map) {
> +            mapped_node = qdict_get_try_str(map, mapped_node) ?: mapped_node;
> +        }
> +
> +        s->bs = bdrv_lookup_bs(mapped_node, mapped_node, &local_err);

Should we search by device name? I think, that if command set-mapping was 
called, it means that user is node-name oriented, and we should work only with 
node-names.

>           if (!s->bs) {
>               error_report_err(local_err);
>               return -EINVAL;
> @@ -634,7 +652,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, mapped_node);
>               return -EINVAL;
>           }
>       } else if (!s->bitmap && !nothing) {
> @@ -713,6 +731,44 @@ static bool dirty_bitmap_has_postcopy(void *opaque)
>       return true;
>   }
>   
> +void qmp_migrate_set_bitmap_node_mapping(MigrationBlockNodeMappingList *mapping,
> +                                         Error **errp)
> +{

Ok, so, here we don't check existing of the nodes or bitmaps in them, and the 
command may safely called before creating referenced nodes. It's only mapping, 
nothing more.

> +    QDict *in_mapping = qdict_new();
> +    QDict *out_mapping = qdict_new();
> +
> +    for (; mapping; mapping = mapping->next) {
> +        MigrationBlockNodeMapping *entry = mapping->value;
> +
> +        if (qdict_haskey(out_mapping, entry->node_name)) {
> +            error_setg(errp, "Cannot map node name '%s' twice",
> +                       entry->node_name);
> +            goto fail;
> +        }
> +
> +        if (qdict_haskey(in_mapping, entry->alias)) {
> +            error_setg(errp, "Cannot use alias '%s' twice",
> +                       entry->alias);
> +            goto fail;
> +        }
> +
> +        qdict_put_str(in_mapping, entry->alias, entry->node_name);
> +        qdict_put_str(out_mapping, entry->node_name, entry->alias);
> +    }
> +
> +    qobject_unref(dirty_bitmap_mig_state.node_in_mapping);
> +    qobject_unref(dirty_bitmap_mig_state.node_out_mapping);
> +
> +    dirty_bitmap_mig_state.node_in_mapping = in_mapping;
> +    dirty_bitmap_mig_state.node_out_mapping = out_mapping;
> +
> +    return;
> +
> +fail:
> +    qobject_unref(in_mapping);
> +    qobject_unref(out_mapping);
> +}
> +
>   static SaveVMHandlers savevm_dirty_bitmap_handlers = {
>       .save_setup = dirty_bitmap_save_setup,
>       .save_live_complete_postcopy = dirty_bitmap_save_complete,


--
Best regards,
Vladimir