Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-02 Thread Vladimir Sementsov-Ogievskiy

02.07.2020 12:41, Max Reitz wrote:

As I've said before, it may be reasonable to ignore bitmaps not
referenced in the hash-table.

No problem with that.  We just decided on this behavior when we
discussed the RFC.

Sorry for that. The reason for my changed opinion is a recent bug from
customers about bitmap migration.

No problem.  (My original proposal was different still, where
non-specified mappings would default to the identity mapping.)



And, remembering, what my series "[PATCH v2 00/22] Fix error handling during bitmap 
postcopy" is for, I have one more reason for non-strict behavior around bitmap 
migration in general:

Bitmaps are migrating postcopy. We can't rollback in general, so we should 
ignore any error during bitmaps migration, as it's non critical data, and we 
must not break already running target due to some bitmap issue. Failed bitmap 
migration should never be a problem, the worst consequence is necessity to 
create a full backup instead of incremental.


--
Best regards,
Vladimir



Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-02 Thread Dr. David Alan Gilbert
* Max Reitz (mre...@redhat.com) wrote:
> On 30.06.20 12:51, Dr. David Alan Gilbert wrote:
> > * Max Reitz (mre...@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 
> >> Signed-off-by: Max Reitz 
> >> ---
> >>  qapi/migration.json|  83 +++-
> >>  migration/migration.h  |   3 +
> >>  migration/block-dirty-bitmap.c | 372 -
> >>  migration/migration.c  |  29 +++
> >>  4 files changed, 432 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/qapi/migration.json b/qapi/migration.json
> >> index d5000558c6..5aeae9bea8 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,18 @@
> >>  #  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 complete: On the source,
> >> +#  migrating a bitmap from a node when either is not mapped
> >> +#  will result in an error.  On the destination, similarly,
> >> +#  receiving a bitmap (by alias) from a node (by alias) when
> >> +#  either alias is not mapped will result in an error.
> >> +#  By default, all node names and bitmap names are mapped to
> >> +#  themselves. (Since 5.1)
> >> +#
> >>  # Since: 2.4
> >>  ##
> >>  { 'enum': 'MigrationParameter',
> >> @@ -655,7 +705,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 +832,18 @@
> >>  #  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 complete: On the source,
> >> +#  migrating a bitmap from a node when either is not mapped
> >> +#  will result in an error.  On the destination, similarly,
> >> +#  receiving a bitmap (by alias) from a node (by alias) when
> >> +#  either alias is not mapped will result in an error.
> >> +#  By default, all node names and bitmap names are mapped to
> >> +#  themselves. (Since 5.1)
> >> +#
> >>  # Since: 2.4
> >>  ##
> >>  # TODO either fuse back into MigrationParameters, or make
> >> @@ -811,7 +874,8 @@
> >>  '*max-cpu-throttle': 'int',
> >>  '*multifd-compression': 'MultiFDCompression',
> >>  '*multifd-zlib-level': 'int',
> >> -'*multifd-zstd-level': 'int' } }
> >> +'*multifd-zstd-level': 'int',
> >> +'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> > 
> > That's a hairy type for a migration 

Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-02 Thread Vladimir Sementsov-Ogievskiy

02.07.2020 12:41, Max Reitz wrote:

I don’t know if doing it differently would actually be beneficial for
anyone, but OTOH naively it seems like a more invasive code change.


I don't see real benefits, we can go either way, so, not worth rewriting
the patch.

===

I feel like a stupid reviewer:)

Huh?  If anything, a stupid review on a design-changing patch would be a
plain “R-b” without having actually considered the impact.  You do
consider the impact and question it in all places.

I don’t think I need to mention this, but that’s a very good and
important thing to do, because it forces me to reason why we’d want this
or that design.  Without being questioned, I wouldn’t have to reason
about that.  (Which may be a problem in our patch workflow – authors
don’t need to reason unless questioned.[1])

Sorry if I gave the impression of dismissing your comments.  It should
be my burden to reason why I took certain design decisions.


No problem. I should better describe reasons for my suggestions as well, or
if I have no one, mark it as "thinking-out-loud" instead of recommended change.

--
Best regards,
Vladimir



Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-02 Thread Vladimir Sementsov-Ogievskiy

02.07.2020 12:41, Max Reitz wrote:

On 02.07.20 11:19, Vladimir Sementsov-Ogievskiy wrote:

02.07.2020 11:09, Max Reitz wrote:

On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote:

30.06.2020 11:45, 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 
Signed-off-by: Max Reitz 
---
    qapi/migration.json    |  83 +++-
    migration/migration.h  |   3 +
    migration/block-dirty-bitmap.c | 372
-
    migration/migration.c  |  29 +++
    4 files changed, 432 insertions(+), 55 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index d5000558c6..5aeae9bea8 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' ]


So, we still can't migrate bitmaps from one node to different nodes,
but we
also don't know a usecase for it, so it seems OK. But with such
scheme we
can select and rename bitmaps in-flight, and Peter said about
corresponding
use-case.

I'm OK with this, still, just an idea in my mind:

we could instead just have a list of

BitmapMigrationAlias: {
   node-name
   bitmap-name
   node-alias
   bitmap-alias
}

so, mapping is set for each bitmap in separate.


Well, OK, but why?


But why not :) Just thinking out loud. May be someone will imaging good
reasons for it.


The reason for “Why not” is that this code now exists. ;)


Exactly :) But another arguments may appear, who knows. If not - great.

Actual reason is more flexible interface: you can migrate any bitmap to any 
node with any name (except for conflicts of course). But do we need it, I don't 
know. I'd like to hear Peter's opinion, if he don't have preference, then I 
don't care too.

--
Best regards,
Vladimir



Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-02 Thread Max Reitz
On 02.07.20 11:19, Vladimir Sementsov-Ogievskiy wrote:
> 02.07.2020 11:09, Max Reitz wrote:
>> On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote:
>>> 30.06.2020 11:45, 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 
 Signed-off-by: Max Reitz 
 ---
    qapi/migration.json    |  83 +++-
    migration/migration.h  |   3 +
    migration/block-dirty-bitmap.c | 372
 -
    migration/migration.c  |  29 +++
    4 files changed, 432 insertions(+), 55 deletions(-)

 diff --git a/qapi/migration.json b/qapi/migration.json
 index d5000558c6..5aeae9bea8 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' ]
>>>
>>> So, we still can't migrate bitmaps from one node to different nodes,
>>> but we
>>> also don't know a usecase for it, so it seems OK. But with such
>>> scheme we
>>> can select and rename bitmaps in-flight, and Peter said about
>>> corresponding
>>> use-case.
>>>
>>> I'm OK with this, still, just an idea in my mind:
>>>
>>> we could instead just have a list of
>>>
>>> BitmapMigrationAlias: {
>>>   node-name
>>>   bitmap-name
>>>   node-alias
>>>   bitmap-alias
>>> }
>>>
>>> so, mapping is set for each bitmap in separate.
>>
>> Well, OK, but why?
> 
> But why not :) Just thinking out loud. May be someone will imaging good
> reasons for it.

The reason for “Why not” is that this code now exists. ;)

 +    }
 +    }
 +
 +    g_hash_table_insert(bitmaps_map,
 +    g_strdup(bmap_map_from),
 g_strdup(bmap_map_to));
 +    }
 +
 +    amin = g_new(AliasMapInnerNode, 1);
 +    *amin = (AliasMapInnerNode){
>>>
>>> style: space before '{'
>>
>> Is that required?
> 
> If checkpatch doesn't complain, than not..

O:)

 +    .string = g_strdup(node_map_to),
 +    .subtree = bitmaps_map,
 +    };
 +
 +    g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
 +    }
 +
 +    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(_lock);
 @@ -191,11 +351,11 @@ static void send_bitmap_header(QEMUFile *f,
 DirtyBitmapMigBitmapState *dbms,
    qemu_put_bitmap_flags(f, flags);
      if (flags & 

Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-02 Thread Vladimir Sementsov-Ogievskiy

02.07.2020 11:09, Max Reitz wrote:

On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote:

30.06.2020 11:45, 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 
Signed-off-by: Max Reitz 
---
   qapi/migration.json    |  83 +++-
   migration/migration.h  |   3 +
   migration/block-dirty-bitmap.c | 372 -
   migration/migration.c  |  29 +++
   4 files changed, 432 insertions(+), 55 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index d5000558c6..5aeae9bea8 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' ]


So, we still can't migrate bitmaps from one node to different nodes, but we
also don't know a usecase for it, so it seems OK. But with such scheme we
can select and rename bitmaps in-flight, and Peter said about corresponding
use-case.

I'm OK with this, still, just an idea in my mind:

we could instead just have a list of

BitmapMigrationAlias: {
  node-name
  bitmap-name
  node-alias
  bitmap-alias
}

so, mapping is set for each bitmap in separate.


Well, OK, but why?


But why not :) Just thinking out loud. May be someone will imaging good reasons 
for it.




+  } }
+
   ##
   # @MigrationParameter:
   #
@@ -641,6 +679,18 @@
   #  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 complete: On the source,


I've recently faced the case where incomplete mapping make sence: shared
migration of read-only bitmaps in backing files: it seems better to not
migrate them through migration channel, they are migrating through
shared storage automatically (yes, we'll pay the time to load them on
destination, but I'm going to improve it by implementing lazy load of
read-only and disabled bitmaps, so this will not be a problem).

So, now I think that it would be good just ignore all the bitmap not
described by mapping


OK.


+#  migrating a bitmap from a node when either is not mapped
+#  will result in an error.  On the destination, similarly,
+#  receiving a bitmap (by alias) from a node (by alias) when
+#  either alias is not mapped will result in an error.
+#  By default, all node names and bitmap names are mapped to
+#  themselves. (Since 5.1)


This sentense is unclear, want means "by default" - if the mapping is
not specified at all or if some nodes/bitmaps are not covered.


It is clear.


Still,
tha latter will conflict with previous sentencies, so "by default" is
about when mapping is not set at all.


Precisely.


I suggest to word it directly:
"If mapping is not set (the command never called, or mapping was
removed".


The mapping cannot be removed.

It’s also technically clear because mentioning a default for some
parameter always means that that particular parameter will have that
default.  So in this case “by default” refers to block-bitmap-mapping,
not anything nested in it.  (Defaults for stuff nested in its value
would be described in the respective structs’ definitions.)


Reasonable.


> I’d be OK with “By default (when this parameter has never been set)…”.


Yes, this is absolutely clear.




And, actual behavior without mapping is not as simple: it actually tries
to use blk names if possible and node-names if not, and this veries
from version to version. So, I think not worth to document it in detail,
just note, that without mapping 

Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-02 Thread Max Reitz
On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote:
> 30.06.2020 11:45, 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 
>> Signed-off-by: Max Reitz 
>> ---
>>   qapi/migration.json    |  83 +++-
>>   migration/migration.h  |   3 +
>>   migration/block-dirty-bitmap.c | 372 -
>>   migration/migration.c  |  29 +++
>>   4 files changed, 432 insertions(+), 55 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index d5000558c6..5aeae9bea8 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' ]
> 
> So, we still can't migrate bitmaps from one node to different nodes, but we
> also don't know a usecase for it, so it seems OK. But with such scheme we
> can select and rename bitmaps in-flight, and Peter said about corresponding
> use-case.
> 
> I'm OK with this, still, just an idea in my mind:
> 
> we could instead just have a list of
> 
> BitmapMigrationAlias: {
>  node-name
>  bitmap-name
>  node-alias
>  bitmap-alias
> }
> 
> so, mapping is set for each bitmap in separate.

Well, OK, but why?

>> +  } }
>> +
>>   ##
>>   # @MigrationParameter:
>>   #
>> @@ -641,6 +679,18 @@
>>   #  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 complete: On the source,
> 
> I've recently faced the case where incomplete mapping make sence: shared
> migration of read-only bitmaps in backing files: it seems better to not
> migrate them through migration channel, they are migrating through
> shared storage automatically (yes, we'll pay the time to load them on
> destination, but I'm going to improve it by implementing lazy load of
> read-only and disabled bitmaps, so this will not be a problem).
> 
> So, now I think that it would be good just ignore all the bitmap not
> described by mapping

OK.

>> +#  migrating a bitmap from a node when either is not mapped
>> +#  will result in an error.  On the destination, similarly,
>> +#  receiving a bitmap (by alias) from a node (by alias) when
>> +#  either alias is not mapped will result in an error.
>> +#  By default, all node names and bitmap names are mapped to
>> +#  themselves. (Since 5.1)
> 
> This sentense is unclear, want means "by default" - if the mapping is
> not specified at all or if some nodes/bitmaps are not covered.

It is clear.

> Still,
> tha latter will conflict with previous sentencies, so "by default" is
> about when mapping is not set at all.

Precisely.

> I suggest to word it directly:
> "If mapping is not set (the command never called, or mapping was
> removed".

The mapping cannot be removed.

It’s also technically clear because mentioning a default for some
parameter always means that that particular parameter will have that
default.  So in this case “by default” refers to block-bitmap-mapping,
not anything nested in it.  (Defaults for stuff nested in its value
would be described in the respective structs’ definitions.)

I’d be OK with “By default (when this parameter has never been set)…”.

> And, actual behavior without mapping is not as simple: it actually tries
> to use blk names if possible and node-names if not, and this veries
> 

Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-01 Thread Vladimir Sementsov-Ogievskiy

30.06.2020 11:45, 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 
Signed-off-by: Max Reitz 
---
  qapi/migration.json|  83 +++-
  migration/migration.h  |   3 +
  migration/block-dirty-bitmap.c | 372 -
  migration/migration.c  |  29 +++
  4 files changed, 432 insertions(+), 55 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index d5000558c6..5aeae9bea8 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' ]


So, we still can't migrate bitmaps from one node to different nodes, but we
also don't know a usecase for it, so it seems OK. But with such scheme we
can select and rename bitmaps in-flight, and Peter said about corresponding
use-case.

I'm OK with this, still, just an idea in my mind:

we could instead just have a list of

BitmapMigrationAlias: {
 node-name
 bitmap-name
 node-alias
 bitmap-alias
}

so, mapping is set for each bitmap in separate.


+  } }
+
  ##
  # @MigrationParameter:
  #
@@ -641,6 +679,18 @@
  #  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 complete: On the source,


I've recently faced the case where incomplete mapping make sence: shared
migration of read-only bitmaps in backing files: it seems better to not
migrate them through migration channel, they are migrating through
shared storage automatically (yes, we'll pay the time to load them on
destination, but I'm going to improve it by implementing lazy load of
read-only and disabled bitmaps, so this will not be a problem).

So, now I think that it would be good just ignore all the bitmap not
described by mapping


+#  migrating a bitmap from a node when either is not mapped
+#  will result in an error.  On the destination, similarly,
+#  receiving a bitmap (by alias) from a node (by alias) when
+#  either alias is not mapped will result in an error.
+#  By default, all node names and bitmap names are mapped to
+#  themselves. (Since 5.1)


This sentense is unclear, want means "by default" - if the mapping is
not specified at all or if some nodes/bitmaps are not covered. Still,
tha latter will conflict with previous sentencies, so "by default" is
about when mapping is not set at all. I suggest to word it directly:
"If mapping is not set (the command never called, or mapping was
removed".

And, actual behavior without mapping is not as simple: it actually tries
to use blk names if possible and node-names if not, and this veries
from version to version. So, I think not worth to document it in detail,
just note, that without mapping the behavior is not well defined and
tries to use block-device name if possible and node-name otherwise. And
of course direct mapping of bitmap names.


+#
  # Since: 2.4
  ##
  { 'enum': 'MigrationParameter',
@@ -655,7 +705,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 +832,18 @@
  #  will consume more CPU.
  #  Defaults to 1. (Since 5.0)
  #
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+#  aliases 

Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-01 Thread Max Reitz
On 30.06.20 12:51, Dr. David Alan Gilbert wrote:
> * Max Reitz (mre...@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 
>> Signed-off-by: Max Reitz 
>> ---
>>  qapi/migration.json|  83 +++-
>>  migration/migration.h  |   3 +
>>  migration/block-dirty-bitmap.c | 372 -
>>  migration/migration.c  |  29 +++
>>  4 files changed, 432 insertions(+), 55 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index d5000558c6..5aeae9bea8 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,18 @@
>>  #  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 complete: On the source,
>> +#  migrating a bitmap from a node when either is not mapped
>> +#  will result in an error.  On the destination, similarly,
>> +#  receiving a bitmap (by alias) from a node (by alias) when
>> +#  either alias is not mapped will result in an error.
>> +#  By default, all node names and bitmap names are mapped to
>> +#  themselves. (Since 5.1)
>> +#
>>  # Since: 2.4
>>  ##
>>  { 'enum': 'MigrationParameter',
>> @@ -655,7 +705,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 +832,18 @@
>>  #  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 complete: On the source,
>> +#  migrating a bitmap from a node when either is not mapped
>> +#  will result in an error.  On the destination, similarly,
>> +#  receiving a bitmap (by alias) from a node (by alias) when
>> +#  either alias is not mapped will result in an error.
>> +#  By default, all node names and bitmap names are mapped to
>> +#  themselves. (Since 5.1)
>> +#
>>  # Since: 2.4
>>  ##
>>  # TODO either fuse back into MigrationParameters, or make
>> @@ -811,7 +874,8 @@
>>  '*max-cpu-throttle': 'int',
>>  '*multifd-compression': 'MultiFDCompression',
>>  '*multifd-zlib-level': 'int',
>> -'*multifd-zstd-level': 'int' } }
>> +'*multifd-zstd-level': 'int',
>> +'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> 
> That's a hairy type for a migration parameter!
> I'm curious what 'info migrate_parameters' does in hmp or what happens
> if you try and set it?

Oh.  I didn’t know these were accessible via HMP.

As for setting it, that fails an assertion because I thus forgot to
handle it in hmp_migrate_set_parameter().  I think the best thing 

Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-06-30 Thread Dr. David Alan Gilbert
* Max Reitz (mre...@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 
> Signed-off-by: Max Reitz 
> ---
>  qapi/migration.json|  83 +++-
>  migration/migration.h  |   3 +
>  migration/block-dirty-bitmap.c | 372 -
>  migration/migration.c  |  29 +++
>  4 files changed, 432 insertions(+), 55 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d5000558c6..5aeae9bea8 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,18 @@
>  #  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 complete: On the source,
> +#  migrating a bitmap from a node when either is not mapped
> +#  will result in an error.  On the destination, similarly,
> +#  receiving a bitmap (by alias) from a node (by alias) when
> +#  either alias is not mapped will result in an error.
> +#  By default, all node names and bitmap names are mapped to
> +#  themselves. (Since 5.1)
> +#
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
> @@ -655,7 +705,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 +832,18 @@
>  #  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 complete: On the source,
> +#  migrating a bitmap from a node when either is not mapped
> +#  will result in an error.  On the destination, similarly,
> +#  receiving a bitmap (by alias) from a node (by alias) when
> +#  either alias is not mapped will result in an error.
> +#  By default, all node names and bitmap names are mapped to
> +#  themselves. (Since 5.1)
> +#
>  # Since: 2.4
>  ##
>  # TODO either fuse back into MigrationParameters, or make
> @@ -811,7 +874,8 @@
>  '*max-cpu-throttle': 'int',
>  '*multifd-compression': 'MultiFDCompression',
>  '*multifd-zlib-level': 'int',
> -'*multifd-zstd-level': 'int' } }
> +'*multifd-zstd-level': 'int',
> +'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }

That's a hairy type for a migration parameter!
I'm curious what 'info migrate_parameters' does in hmp or what happens
if you try and set it?

Dave

>  
>  ##
>  # @migrate-set-parameters:
> @@ -957,6 +1021,18 @@
>  #  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