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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 12140 bytes --]

On 13.05.20 22:09, Vladimir Sementsov-Ogievskiy wrote:
> 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 <vsementsov@virtuozzo.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> 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.

If it turns out that we’d want it, I suppose we can also still always
extend MigrationBlockNodeMapping by another mapping array for bitmaps.

> 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?

It was my understanding that libvirt (which should know about all
bitmaps on all nodes) would and could ensure itself that all nodes are
mapped according to what it needs.  (But that’s why Peter is CC’d, to
get his input.)

But your idea seems simple, so why not.

> 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]

You mean error out during migration?  Hm.  I suppose that’s OK, if some
mapping erroneously isn’t set and the node name doesn’t exist in the
destination, we’ll error out, too, so...

Shouldn’t be too difficult to implement, just put the enum in
dirty_bitmap_mig_state, and then do what it says when no entry can be
found in the mapping QDict.

>   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.

Ah, yes.  Well, that’s actually a real problem with my current
implementation then, too, because...

>> 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;

@name may be a device name, so it doesn’t match the interface
description.  We must use the node name for the map.

>> +        }
>> +
>>           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.

Why not.

>> +        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.

Yes, we shouldn’t.

>>           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.

Yes.  That seemed simpler to me than, say, look up the BDSs and take
references on them (and keep them until migration).  I’m not sure
whether that’s an interface we’d want in the block layer (because there
node names are to reference nodes at the time of invoking the command),
but this isn’t really the block layer, so I think/hope it should be fine.

Max

>> +    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,
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]