[Qemu-devel] [PATCH v2 0/3] Remove old MigrationParams

[Qemu-devel] [PATCH v2 0/3] Remove old MigrationParams

[Juan Quintela]

Hi

Changes from v1:

- make migrate_block_set_* take a boolean
- disable block migration in colo to maintain semantics.

Please review, Juan.

[v1]
Upon a time there were MigrationParms (only used for block migration)
and then MigrationParams used for everything else.  This series:

- create migration capabilities for block parameters
- make the migrate command line parameters to use capabilities
- remove MigrationParams completely

Please, review.


*** BLURB HERE ***

Juan Quintela (3):
  migration: Create block capabilities for shared and enable
  migration: Remove use of old MigrationParams
  migration: Remove old MigrationParams

 include/migration/block.h     |  3 +++
 include/migration/migration.h | 14 +++++-------
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h       |  1 -
 include/sysemu/sysemu.h       |  3 +--
 migration/block.c             | 17 ++------------
 migration/colo.c              |  7 +++---
 migration/migration.c         | 52 ++++++++++++++++++++++++++++++++++++-------
 migration/savevm.c            | 18 +++------------
 qapi-schema.json              |  7 +++++-
 10 files changed, 68 insertions(+), 55 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Those two capabilities were added through the command line.  Notice that
we just created them.  This is just the boilerplate.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

--

Make migrate_set_block_* take a boolean argument.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/block.h     |  3 +++
 include/migration/migration.h |  3 +++
 migration/migration.c         | 36 ++++++++++++++++++++++++++++++++++++
 qapi-schema.json              |  7 ++++++-
 4 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/include/migration/block.h b/include/migration/block.h
index 41a1ac8..5b3f1a5 100644
--- a/include/migration/block.h
+++ b/include/migration/block.h
@@ -20,4 +20,7 @@ uint64_t blk_mig_bytes_transferred(void);
 uint64_t blk_mig_bytes_remaining(void);
 uint64_t blk_mig_bytes_total(void);
 
+void migrate_set_block_shared(MigrationState *s, bool value);
+void migrate_set_block_enabled(MigrationState *s, bool value);
+
 #endif /* MIGRATION_BLOCK_H */
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 0c9b6af..30c2913 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -305,6 +305,9 @@ bool migrate_colo_enabled(void);
 
 int64_t xbzrle_cache_resize(int64_t new_size);
 
+bool migrate_use_block_enabled(void);
+bool migrate_use_block_shared(void);
+
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
diff --git a/migration/migration.c b/migration/migration.c
index fe62f15..2f981aa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1195,6 +1195,16 @@ bool migration_is_blocked(Error **errp)
     return false;
 }
 
+void migrate_set_block_shared(MigrationState *s, bool value)
+{
+    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
+}
+
+void migrate_set_block_enabled(MigrationState *s, bool value)
+{
+    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = value;
+}
+
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  Error **errp)
@@ -1224,6 +1234,14 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 
     s = migrate_init(&params);
 
+    if (has_blk && blk) {
+        migrate_set_block_enabled(s, true);
+    }
+
+    if (has_inc && inc) {
+        migrate_set_block_shared(s, true);
+    }
+
     if (strstart(uri, "tcp:", &p)) {
         tcp_start_outgoing_migration(s, p, &local_err);
 #ifdef CONFIG_RDMA
@@ -1419,6 +1437,24 @@ int64_t migrate_xbzrle_cache_size(void)
     return s->xbzrle_cache_size;
 }
 
+bool migrate_use_block_enabled(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED];
+}
+
+bool migrate_use_block_shared(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED];
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/qapi-schema.json b/qapi-schema.json
index 5728b7f..109852e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -894,11 +894,16 @@
 # @release-ram: if enabled, qemu will free the migrated ram pages on the source
 #        during postcopy-ram migration. (since 2.9)
 #
+# @block-enabled: enable block migration (Since 2.10)
+#
+# @block-shared: enable block shared migration (Since 2.10)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
+           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
+           'block-enabled', 'block-shared' ] }
 
 ##
 # @MigrationCapabilityStatus:
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

On 05/11/2017 11:32 AM, Juan Quintela wrote:
> Those two capabilities were added through the command line.  Notice that
> we just created them.  This is just the boilerplate.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> --
> 
> Make migrate_set_block_* take a boolean argument.

Question - do we support the orthogonal selection of all 4 combinations
under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
together), or are there only 3 actual states? If the latter, should we
represent this as a single enum-valued property, rather than as two
independent boolean properties?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>> Those two capabilities were added through the command line.  Notice that
>> we just created them.  This is just the boilerplate.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> 
>> --
>> 
>> Make migrate_set_block_* take a boolean argument.
>
> Question - do we support the orthogonal selection of all 4 combinations
> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
> together), or are there only 3 actual states? If the latter, should we
> represent this as a single enum-valued property, rather than as two
> independent boolean properties?

{ 'enum': 'MigrationCapability',
  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }


My understanding is that we can only have boolean capabilities here.
Or, how could we put a non-boolean capability?

There are three states as far as I can see.

Later, Juan.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Eric Blake <eblake@redhat.com> wrote:
> > On 05/11/2017 11:32 AM, Juan Quintela wrote:
> >> Those two capabilities were added through the command line.  Notice that
> >> we just created them.  This is just the boilerplate.
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> 
> >> --
> >> 
> >> Make migrate_set_block_* take a boolean argument.
> >
> > Question - do we support the orthogonal selection of all 4 combinations
> > under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
> > together), or are there only 3 actual states? If the latter, should we
> > represent this as a single enum-valued property, rather than as two
> > independent boolean properties?
> 
> { 'enum': 'MigrationCapability',
>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
> 
> 
> My understanding is that we can only have boolean capabilities here.
> Or, how could we put a non-boolean capability?

Lets keep this simple and stick with the booleans.

Dave

> There are three states as far as I can see.
> 
> Later, Juan.
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Eric Blake <eblake@redhat.com> wrote:
>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>>> Those two capabilities were added through the command line.  Notice that
>>>> we just created them.  This is just the boilerplate.
>>>>
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>
>>>> --
>>>>
>>>> Make migrate_set_block_* take a boolean argument.
>>>
>>> Question - do we support the orthogonal selection of all 4 combinations
>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>> together), or are there only 3 actual states? If the latter, should we
>>> represent this as a single enum-valued property, rather than as two
>>> independent boolean properties?
>>
>> { 'enum': 'MigrationCapability',
>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>
>>
>> My understanding is that we can only have boolean capabilities here.
>> Or, how could we put a non-boolean capability?

If we want a non-boolean, then we make it a migration parameter rather
than a migration capability.  There may be other advantages to using
MigrationParameter instead of MigrationCapability (such as making it
easier to figure out whether the parameter settings are persistent or
apply per-migration).

> 
> Lets keep this simple and stick with the booleans.
> 
> Dave
> 
>> There are three states as far as I can see.

I'll leave it up to you as maintainers which way you prefer, I'm just
offering the potential design tradeoffs for simplicity of booleans (but
complexity in an unused state) vs. simplicity of design (but complexity
in code).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quintela@redhat.com) wrote:
>>> Eric Blake <eblake@redhat.com> wrote:
>>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>>>> Those two capabilities were added through the command line.  Notice that
>>>>> we just created them.  This is just the boilerplate.
>>>>>
>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>
>>>>> --
>>>>>
>>>>> Make migrate_set_block_* take a boolean argument.
>>>>
>>>> Question - do we support the orthogonal selection of all 4 combinations
>>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>>> together), or are there only 3 actual states? If the latter, should we
>>>> represent this as a single enum-valued property, rather than as two
>>>> independent boolean properties?
>>>
>>> { 'enum': 'MigrationCapability',
>>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>>
>>>
>>> My understanding is that we can only have boolean capabilities here.
>>> Or, how could we put a non-boolean capability?
>
> If we want a non-boolean, then we make it a migration parameter rather
> than a migration capability.  There may be other advantages to using
> MigrationParameter instead of MigrationCapability (such as making it
> easier to figure out whether the parameter settings are persistent or
> apply per-migration).

What makes a migration knob a MigrationCapability rather than a
MigrationParameter?  Type bool, or is there more to it?

>> 
>> Lets keep this simple and stick with the booleans.
>> 
>> Dave
>> 
>>> There are three states as far as I can see.

Begs the question how the fourth state behaves.  Documentation is of no
help:

    diff --git a/qapi-schema.json b/qapi-schema.json
    index 5728b7f..109852e 100644
    --- a/qapi-schema.json
    +++ b/qapi-schema.json
    @@ -894,11 +894,16 @@
     # @release-ram: if enabled, qemu will free the migrated ram pages on the source
     #        during postcopy-ram migration. (since 2.9)
     #
    +# @block-enabled: enable block migration (Since 2.10)
    +#
    +# @block-shared: enable block shared migration (Since 2.10)
    +#
     # Since: 1.2
     ##

Please explain all four states clearly there.

> I'll leave it up to you as maintainers which way you prefer, I'm just
> offering the potential design tradeoffs for simplicity of booleans (but
> complexity in an unused state) vs. simplicity of design (but complexity
> in code).

For what it's worth, I dislike entangled booleans.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>>> * Juan Quintela (quintela@redhat.com) wrote:
>>>> Eric Blake <eblake@redhat.com> wrote:
>>>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>>>>> Those two capabilities were added through the command line.  Notice that
>>>>>> we just created them.  This is just the boilerplate.
>>>>>>
>>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>>
>>>>>> --
>>>>>>
>>>>>> Make migrate_set_block_* take a boolean argument.
>>>>>
>>>>> Question - do we support the orthogonal selection of all 4 combinations
>>>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>>>> together), or are there only 3 actual states? If the latter, should we
>>>>> represent this as a single enum-valued property, rather than as two
>>>>> independent boolean properties?
>>>>
>>>> { 'enum': 'MigrationCapability',
>>>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>>>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>>>
>>>>
>>>> My understanding is that we can only have boolean capabilities here.
>>>> Or, how could we put a non-boolean capability?
>>
>> If we want a non-boolean, then we make it a migration parameter rather
>> than a migration capability.  There may be other advantages to using
>> MigrationParameter instead of MigrationCapability (such as making it
>> easier to figure out whether the parameter settings are persistent or
>> apply per-migration).
>
> What makes a migration knob a MigrationCapability rather than a
> MigrationParameter?  Type bool, or is there more to it?

I didn't started this, but *my* undersanding:

Migration capability: we have to set this up before migration starts.
It is like a property that we can't change later.  We can't add a
compression thread in the middle of migration (notice that we "could"
but current code can't).

So, block enabled migration is a capability.

Block shared on the other hand is weird.  *My* understading of the code
is that we have a qcow2 overlay on top of the base image, and:

- without shared: we migrate all the block device
- with shared: we migrate only the top overlay

So, thisk could be a parameter of block migration.  If anyone understand
better than me, please stand up.

>>> Lets keep this simple and stick with the booleans.
>>> 
>>> Dave
>>> 
>>>> There are three states as far as I can see.
>
> Begs the question how the fourth state behaves.  Documentation is of no
> help:
>
>     diff --git a/qapi-schema.json b/qapi-schema.json
>     index 5728b7f..109852e 100644
>     --- a/qapi-schema.json
>     +++ b/qapi-schema.json
>     @@ -894,11 +894,16 @@
>      # @release-ram: if enabled, qemu will free the migrated ram pages on the source
>      #        during postcopy-ram migration. (since 2.9)
>      #
>     +# @block-enabled: enable block migration (Since 2.10)
>     +#
>     +# @block-shared: enable block shared migration (Since 2.10)
>     +#
>      # Since: 1.2
>      ##
>
> Please explain all four states clearly there.

Is the previous explanation enough?

>> I'll leave it up to you as maintainers which way you prefer, I'm just
>> offering the potential design tradeoffs for simplicity of booleans (but
>> complexity in an unused state) vs. simplicity of design (but complexity
>> in code).
>
> For what it's worth, I dislike entangled booleans.

Some here, but except if we put shared as a migration parameter, I don't
know what to do here.

{ 'enum': 'MigrationParameter',
  'data': ['compress-level', 'compress-threads', 'decompress-threads',
           'cpu-throttle-initial', 'cpu-throttle-increment',
           'tls-creds', 'tls-hostname', 'max-bandwidth',
           'downtime-limit', 'x-checkpoint-delay' ] }


I don't see either how to define that block_shared will be a boolean parameter.

Later, Juan.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>>>> * Juan Quintela (quintela@redhat.com) wrote:
>>>>> Eric Blake <eblake@redhat.com> wrote:
>>>>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>>>>>> Those two capabilities were added through the command line.  Notice that
>>>>>>> we just created them.  This is just the boilerplate.
>>>>>>>
>>>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> Make migrate_set_block_* take a boolean argument.
>>>>>>
>>>>>> Question - do we support the orthogonal selection of all 4 combinations
>>>>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>>>>> together), or are there only 3 actual states? If the latter, should we
>>>>>> represent this as a single enum-valued property, rather than as two
>>>>>> independent boolean properties?
>>>>>
>>>>> { 'enum': 'MigrationCapability',
>>>>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>>>>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>>>>
>>>>>
>>>>> My understanding is that we can only have boolean capabilities here.
>>>>> Or, how could we put a non-boolean capability?
>>>
>>> If we want a non-boolean, then we make it a migration parameter rather
>>> than a migration capability.  There may be other advantages to using
>>> MigrationParameter instead of MigrationCapability (such as making it
>>> easier to figure out whether the parameter settings are persistent or
>>> apply per-migration).
>>
>> What makes a migration knob a MigrationCapability rather than a
>> MigrationParameter?  Type bool, or is there more to it?
>
> I didn't started this, but *my* undersanding:
>
> Migration capability: we have to set this up before migration starts.
> It is like a property that we can't change later.  We can't add a
> compression thread in the middle of migration (notice that we "could"
> but current code can't).
>
> So, block enabled migration is a capability.
>
> Block shared on the other hand is weird.  *My* understading of the code
> is that we have a qcow2 overlay on top of the base image, and:
>
> - without shared: we migrate all the block device
> - with shared: we migrate only the top overlay

Plausible.  The thing that's "shared" is the backing file, between
migration source and destination.

However, we can't change "shared" in the middle of migration, either.

> So, thisk could be a parameter of block migration.  If anyone understand
> better than me, please stand up.
>
>>>> Lets keep this simple and stick with the booleans.
>>>> 
>>>> Dave
>>>> 
>>>>> There are three states as far as I can see.
>>
>> Begs the question how the fourth state behaves.  Documentation is of no
>> help:
>>
>>     diff --git a/qapi-schema.json b/qapi-schema.json
>>     index 5728b7f..109852e 100644
>>     --- a/qapi-schema.json
>>     +++ b/qapi-schema.json
>>     @@ -894,11 +894,16 @@
>>      # @release-ram: if enabled, qemu will free the migrated ram pages on the source
>>      #        during postcopy-ram migration. (since 2.9)
>>      #
>>     +# @block-enabled: enable block migration (Since 2.10)
>>     +#
>>     +# @block-shared: enable block shared migration (Since 2.10)
>>     +#
>>      # Since: 1.2
>>      ##
>>
>> Please explain all four states clearly there.
>
> Is the previous explanation enough?

The new comments can serve as reminders for readers who already know
what "block migration" and "block shared migration" is.  Surely we can
do better for readers who don't, and I'd like you to try.

Note that the comments aren't just for QAPI schema readers, but also for
QMP documentation readers (generated docs/qemu-qmp-ref.*).

>>> I'll leave it up to you as maintainers which way you prefer, I'm just
>>> offering the potential design tradeoffs for simplicity of booleans (but
>>> complexity in an unused state) vs. simplicity of design (but complexity
>>> in code).
>>
>> For what it's worth, I dislike entangled booleans.
>
> Some here, but except if we put shared as a migration parameter, I don't
> know what to do here.
>
> { 'enum': 'MigrationParameter',
>   'data': ['compress-level', 'compress-threads', 'decompress-threads',
>            'cpu-throttle-initial', 'cpu-throttle-increment',
>            'tls-creds', 'tls-hostname', 'max-bandwidth',
>            'downtime-limit', 'x-checkpoint-delay' ] }
>
>
> I don't see either how to define that block_shared will be a boolean parameter.

Here:

  { 'struct': 'MigrationParameters',
    'data': { '*compress-level': 'int',
              '*compress-threads': 'int',
              '*decompress-threads': 'int',
              '*cpu-throttle-initial': 'int',
              '*cpu-throttle-increment': 'int',
              '*tls-creds': 'str',
              '*tls-hostname': 'str',
              '*max-bandwidth': 'int',
              '*downtime-limit': 'int',
              '*x-checkpoint-delay': 'int'} }

How enum MigrationParameter shadows the member names of
MigrationParameters is weird.  Wouldn't be necessary if QAPI provided
object type introspection to C (it provides it to QMP).

MigrationParameter is used only for HMP commands "migrate_set_parameter"
and "info migrate_parameters".  The way they're coded doesn't really
profit from QAPI; string literals would be simpler.  Yes, they'd have to
shadow MigrationParameters, but since they'd replace MigrationParameter,
we'd be roughly even.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quintela@redhat.com) wrote:
>>> Eric Blake <eblake@redhat.com> wrote:
>>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>>>> Those two capabilities were added through the command line.  Notice that
>>>>> we just created them.  This is just the boilerplate.
>>>>>
>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>
>>>>> --
>>>>>
>>>>> Make migrate_set_block_* take a boolean argument.
>>>>
>>>> Question - do we support the orthogonal selection of all 4 combinations
>>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>>> together), or are there only 3 actual states? If the latter, should we
>>>> represent this as a single enum-valued property, rather than as two
>>>> independent boolean properties?
>>>
>>> { 'enum': 'MigrationCapability',
>>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>>
>>>
>>> My understanding is that we can only have boolean capabilities here.
>>> Or, how could we put a non-boolean capability?
>
> If we want a non-boolean, then we make it a migration parameter rather
> than a migration capability.  There may be other advantages to using
> MigrationParameter instead of MigrationCapability (such as making it
> easier to figure out whether the parameter settings are persistent or
> apply per-migration).

Block migration is a capability as far as I can see/understand.  The
block shared bit could be a parameter, though.  Not sure if that would
be better/worse.

>
>> 
>> Lets keep this simple and stick with the booleans.
>> 
>> Dave
>> 
>>> There are three states as far as I can see.
>
> I'll leave it up to you as maintainers which way you prefer, I'm just
> offering the potential design tradeoffs for simplicity of booleans (but
> complexity in an unused state) vs. simplicity of design (but complexity
> in code).

As I expect to deprecate the old interface, I think that the best thing
to do is to use two capabilities or a capability(block enabled) and a
parameter (block shared).

What do you think?

Later, Juan.

[Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

We have change in the previous patch to use migration capabilities for
it.  Notice that we continue using the old command line flags from
migrate command from the time being.  Remove the set_params method as
now it is empty.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/migration.h |  3 +--
 migration/block.c             | 17 ++---------------
 migration/colo.c              |  5 +++--
 migration/migration.c         |  8 +++++---
 migration/savevm.c            |  2 --
 5 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 30c2913..2d5525c 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -39,8 +39,7 @@
 #define QEMU_VM_SECTION_FOOTER       0x7e
 
 struct MigrationParams {
-    bool blk;
-    bool shared;
+    bool unused; /* C doesn't allow empty structs */
 };
 
 /* Messages sent on the return path from destination to source */
diff --git a/migration/block.c b/migration/block.c
index 060087f..fcfa823 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -94,9 +94,6 @@ typedef struct BlkMigBlock {
 } BlkMigBlock;
 
 typedef struct BlkMigState {
-    /* Written during setup phase.  Can be read without a lock.  */
-    int blk_enable;
-    int shared_base;
     QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
     int64_t total_sector_sum;
     bool zero_blocks;
@@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f)
         bmds->bulk_completed = 0;
         bmds->total_sectors = sectors;
         bmds->completed_sectors = 0;
-        bmds->shared_base = block_mig_state.shared_base;
+        bmds->shared_base = migrate_use_block_shared();
 
         assert(i < num_bs);
         bmds_bs[i].bmds = bmds;
@@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static void block_set_params(const MigrationParams *params, void *opaque)
-{
-    block_mig_state.blk_enable = params->blk;
-    block_mig_state.shared_base = params->shared;
-
-    /* shared base means that blk_enable = 1 */
-    block_mig_state.blk_enable |= params->shared;
-}
-
 static bool block_is_active(void *opaque)
 {
-    return block_mig_state.blk_enable == 1;
+    return migrate_use_block_enabled();
 }
 
 static SaveVMHandlers savevm_block_handlers = {
-    .set_params = block_set_params,
     .save_live_setup = block_save_setup,
     .save_live_iterate = block_save_iterate,
     .save_live_complete_precopy = block_save_complete,
diff --git a/migration/colo.c b/migration/colo.c
index 963c802..e772384 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -14,6 +14,7 @@
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
 #include "migration/colo.h"
+#include "migration/block.h"
 #include "io/channel-buffer.h"
 #include "trace.h"
 #include "qemu/error-report.h"
@@ -345,8 +346,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     /* Disable block migration */
-    s->params.blk = 0;
-    s->params.shared = 0;
+    migrate_set_block_enabled(s, false);
+    migrate_set_block_shared(s, false);
     qemu_savevm_state_header(fb);
     qemu_savevm_state_begin(fb, &s->params);
     qemu_mutex_lock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index 2f981aa..8a3bf89 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -787,6 +787,10 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
         s->enabled_capabilities[cap->value->capability] = cap->value->state;
     }
 
+    if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
+        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
+    }
+
     if (migrate_postcopy_ram()) {
         if (migrate_use_compression()) {
             /* The decompression threads asynchronously write into RAM
@@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationParams params;
     const char *p;
 
-    params.blk = has_blk && blk;
-    params.shared = has_inc && inc;
-
     if (migration_is_setup_or_active(s->state) ||
         s->state == MIGRATION_STATUS_CANCELLING ||
         s->state == MIGRATION_STATUS_COLO) {
@@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     }
 
     if (has_inc && inc) {
+        migrate_set_block_enabled(s, true);
         migrate_set_block_shared(s, true);
     }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index f8d1e8b..221fb4b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
     MigrationParams params = {
-        .blk = 0,
-        .shared = 0
     };
     MigrationState *ms = migrate_init(&params);
     MigrationStatus status;
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Peter Xu]

On Thu, May 11, 2017 at 06:32:27PM +0200, Juan Quintela wrote:
> We have change in the previous patch to use migration capabilities for
> it.  Notice that we continue using the old command line flags from
> migrate command from the time being.  Remove the set_params method as
> now it is empty.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  include/migration/migration.h |  3 +--
>  migration/block.c             | 17 ++---------------
>  migration/colo.c              |  5 +++--
>  migration/migration.c         |  8 +++++---
>  migration/savevm.c            |  2 --
>  5 files changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 30c2913..2d5525c 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -39,8 +39,7 @@
>  #define QEMU_VM_SECTION_FOOTER       0x7e
>  
>  struct MigrationParams {
> -    bool blk;
> -    bool shared;
> +    bool unused; /* C doesn't allow empty structs */
>  };
>  
>  /* Messages sent on the return path from destination to source */
> diff --git a/migration/block.c b/migration/block.c
> index 060087f..fcfa823 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -94,9 +94,6 @@ typedef struct BlkMigBlock {
>  } BlkMigBlock;
>  
>  typedef struct BlkMigState {
> -    /* Written during setup phase.  Can be read without a lock.  */
> -    int blk_enable;
> -    int shared_base;
>      QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
>      int64_t total_sector_sum;
>      bool zero_blocks;
> @@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f)
>          bmds->bulk_completed = 0;
>          bmds->total_sectors = sectors;
>          bmds->completed_sectors = 0;
> -        bmds->shared_base = block_mig_state.shared_base;
> +        bmds->shared_base = migrate_use_block_shared();
>  
>          assert(i < num_bs);
>          bmds_bs[i].bmds = bmds;
> @@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> -static void block_set_params(const MigrationParams *params, void *opaque)
> -{
> -    block_mig_state.blk_enable = params->blk;
> -    block_mig_state.shared_base = params->shared;
> -
> -    /* shared base means that blk_enable = 1 */
> -    block_mig_state.blk_enable |= params->shared;
> -}
> -
>  static bool block_is_active(void *opaque)
>  {
> -    return block_mig_state.blk_enable == 1;
> +    return migrate_use_block_enabled();
>  }
>  
>  static SaveVMHandlers savevm_block_handlers = {
> -    .set_params = block_set_params,
>      .save_live_setup = block_save_setup,
>      .save_live_iterate = block_save_iterate,
>      .save_live_complete_precopy = block_save_complete,
> diff --git a/migration/colo.c b/migration/colo.c
> index 963c802..e772384 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -14,6 +14,7 @@
>  #include "qemu/timer.h"
>  #include "sysemu/sysemu.h"
>  #include "migration/colo.h"
> +#include "migration/block.h"
>  #include "io/channel-buffer.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
> @@ -345,8 +346,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>      }
>  
>      /* Disable block migration */
> -    s->params.blk = 0;
> -    s->params.shared = 0;
> +    migrate_set_block_enabled(s, false);
> +    migrate_set_block_shared(s, false);
>      qemu_savevm_state_header(fb);
>      qemu_savevm_state_begin(fb, &s->params);
>      qemu_mutex_lock_iothread();
> diff --git a/migration/migration.c b/migration/migration.c
> index 2f981aa..8a3bf89 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -787,6 +787,10 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>          s->enabled_capabilities[cap->value->capability] = cap->value->state;
>      }
>  
> +    if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
> +        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
> +    }
> +

[1]

>      if (migrate_postcopy_ram()) {
>          if (migrate_use_compression()) {
>              /* The decompression threads asynchronously write into RAM
> @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationParams params;
>      const char *p;
>  
> -    params.blk = has_blk && blk;
> -    params.shared = has_inc && inc;
> -
>      if (migration_is_setup_or_active(s->state) ||
>          s->state == MIGRATION_STATUS_CANCELLING ||
>          s->state == MIGRATION_STATUS_COLO) {
> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      }
>  
>      if (has_inc && inc) {
> +        migrate_set_block_enabled(s, true);
>          migrate_set_block_shared(s, true);

[2]

IIUC for [1] & [2] we are solving the same problem that "shared"
depends on "enabled" bit. Would it be good to unitfy this dependency
somewhere? E.g., by changing migrate_set_block_shared() into:

void migrate_set_block_shared(MigrationState *s, bool value)
{
    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
    if (value) {
        migrate_set_block_enabled(s, true);
    }
}

?

Another thing to mention: after switching to the capability interface,
we'll cache the "enabled" and "shared" bits now while we don't cache
it before, right? IIUC it'll affect behavior of such sequence:

- 1st migrate with enabled=1, shared=1, then
- 2nd migrate with enabled=0, shared=0

Before the series, the 2nd migrate will use enabled=shared=0, but
after the series it should be using enabled=shared=1. Not sure whether
this would be a problem (or I missed anything?).

Thanks,

>      }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f8d1e8b..221fb4b 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  {
>      int ret;
>      MigrationParams params = {
> -        .blk = 0,
> -        .shared = 0
>      };
>      MigrationState *ms = migrate_init(&params);
>      MigrationStatus status;
> -- 
> 2.9.3
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Thu, May 11, 2017 at 06:32:27PM +0200, Juan Quintela wrote:

>> @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>      MigrationParams params;
>>      const char *p;
>>  
>> -    params.blk = has_blk && blk;
>> -    params.shared = has_inc && inc;
>> -
>>      if (migration_is_setup_or_active(s->state) ||
>>          s->state == MIGRATION_STATUS_CANCELLING ||
>>          s->state == MIGRATION_STATUS_COLO) {
>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>      }
>>  
>>      if (has_inc && inc) {
>> +        migrate_set_block_enabled(s, true);
>>          migrate_set_block_shared(s, true);
>
> [2]
>
> IIUC for [1] & [2] we are solving the same problem that "shared"
> depends on "enabled" bit. Would it be good to unitfy this dependency
> somewhere? E.g., by changing migrate_set_block_shared() into:
>
> void migrate_set_block_shared(MigrationState *s, bool value)
> {
>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>     if (value) {
>         migrate_set_block_enabled(s, true);
>     }
> }

ok with this.
I will add once here that when we disable block enabled, we also disable
shared, or just let it that way?

> Another thing to mention: after switching to the capability interface,
> we'll cache the "enabled" and "shared" bits now while we don't cache
> it before, right? IIUC it'll affect behavior of such sequence:
>
> - 1st migrate with enabled=1, shared=1, then
> - 2nd migrate with enabled=0, shared=0
>
> Before the series, the 2nd migrate will use enabled=shared=0, but
> after the series it should be using enabled=shared=1. Not sure whether
> this would be a problem (or I missed anything?).

We can't be consistent with both old/new way.

Old way: we always setup the capabilities on command line (that should
have been deprecated long, long ago)

New way: Once set, they stay set.

So, alternatives are:
- If we are going to deprecate the old way, just let things as they are
  on this patch (easier for me O:-)

- Always disable this features at the end of migration: Compatible with
  old migration semantics, bet we are inconsistent with all other
  capabilities.

- Add yet more code to only disable them when we are setting them
  through the command line.  More code to maintain.

My idea would be to deprecate the migrate command line parameter, that
is the reason why I did the 1st option.  I hope that in due curse, we
would be able to remove the code.  But if anyone strongly think that any
of the other options is better, please let me know.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2562 bytes --]

On 05/12/2017 05:55 AM, Juan Quintela wrote:
>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>      }
>>>  
>>>      if (has_inc && inc) {
>>> +        migrate_set_block_enabled(s, true);
>>>          migrate_set_block_shared(s, true);
>>
>> [2]
>>
>> IIUC for [1] & [2] we are solving the same problem that "shared"
>> depends on "enabled" bit. Would it be good to unitfy this dependency
>> somewhere? E.g., by changing migrate_set_block_shared() into:
>>
>> void migrate_set_block_shared(MigrationState *s, bool value)
>> {
>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>>     if (value) {
>>         migrate_set_block_enabled(s, true);
>>     }
>> }
> 
> ok with this.

Or, as I commented on 1/3, maybe having a single property that is a
tri-state enum value, instead of 2 separate boolean properties, might be
nicer (but certainly a bit more complex to code up).

> I will add once here that when we disable block enabled, we also disable
> shared, or just let it that way?
> 
>> Another thing to mention: after switching to the capability interface,
>> we'll cache the "enabled" and "shared" bits now while we don't cache
>> it before, right? IIUC it'll affect behavior of such sequence:
>>
>> - 1st migrate with enabled=1, shared=1, then
>> - 2nd migrate with enabled=0, shared=0
>>
>> Before the series, the 2nd migrate will use enabled=shared=0, but
>> after the series it should be using enabled=shared=1. Not sure whether
>> this would be a problem (or I missed anything?).
> 
> We can't be consistent with both old/new way.
> 
> Old way: we always setup the capabilities on command line (that should
> have been deprecated long, long ago)

Well, the easy way out is to have the HMP migrate command (I assume
that's what you mean by "on command line") explicitly clear the
parameters if it is called without the -b/-i flag.  So the start of each
migration is what changes the properties, so long as you are still using
HMP to start the migration.  Or, on the QMP side, since 'migrate' has
optional 'blk' and 'inc' booleans, basically leave the settings alone if
the parameters were omitted, and explicitly update the property to the
value of those parameters if they were present.

Or is the proposal that we are also going to simplify the QMP 'migrate'
command to get rid of crufty parameters?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/12/2017 05:55 AM, Juan Quintela wrote:
>>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>      }
>>>>  
>>>>      if (has_inc && inc) {
>>>> +        migrate_set_block_enabled(s, true);
>>>>          migrate_set_block_shared(s, true);
>>>
>>> [2]
>>>
>>> IIUC for [1] & [2] we are solving the same problem that "shared"
>>> depends on "enabled" bit. Would it be good to unitfy this dependency
>>> somewhere? E.g., by changing migrate_set_block_shared() into:
>>>
>>> void migrate_set_block_shared(MigrationState *s, bool value)
>>> {
>>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>>>     if (value) {
>>>         migrate_set_block_enabled(s, true);
>>>     }
>>> }
>> 
>> ok with this.
>
> Or, as I commented on 1/3, maybe having a single property that is a
> tri-state enum value, instead of 2 separate boolean properties, might be
> nicer (but certainly a bit more complex to code up).

If you teach me how to do the qapi/qmp part, I will do the other bits.
I don't really care if we do it one way or the other.

>> I will add once here that when we disable block enabled, we also disable
>> shared, or just let it that way?
>> 
>>> Another thing to mention: after switching to the capability interface,
>>> we'll cache the "enabled" and "shared" bits now while we don't cache
>>> it before, right? IIUC it'll affect behavior of such sequence:
>>>
>>> - 1st migrate with enabled=1, shared=1, then
>>> - 2nd migrate with enabled=0, shared=0
>>>
>>> Before the series, the 2nd migrate will use enabled=shared=0, but
>>> after the series it should be using enabled=shared=1. Not sure whether
>>> this would be a problem (or I missed anything?).
>> 
>> We can't be consistent with both old/new way.
>> 
>> Old way: we always setup the capabilities on command line (that should
>> have been deprecated long, long ago)
>
> Well, the easy way out is to have the HMP migrate command (I assume
> that's what you mean by "on command line") explicitly clear the
> parameters if it is called without the -b/-i flag.  So the start of each
> migration is what changes the properties, so long as you are still using
> HMP to start the migration.  Or, on the QMP side, since 'migrate' has
> optional 'blk' and 'inc' booleans, basically leave the settings alone if
> the parameters were omitted, and explicitly update the property to the
> value of those parameters if they were present.

We are going to have trouble whatever way that we do it, or we start
doing lots of strange things.

Forget about qmp, we are going to assume that it is consistent with hmp.

migrate_set_capabilities block_enabled on
migrate -b .....

Should migrate disable the block_enabled capability?  Give one
warning/error?

And notice that this only matter if we do a migration, we cancel/get one
error, and then we migrate again.

What I tried to do is assume that -b/-i arguments don't exist.  And if
the user use them, we implement its behaviour with the minimal fuss
possibly.

Only way that I can think of being consistent and bug compatible will be
to store:
- old block_shared/enanbeld capability value
- if we set -b/-i on the command line

In migration cleanup do the right thing depending on this four
variables.  I think that it is adding lots of complexity for very few
improvement.


> Or is the proposal that we are also going to simplify the QMP 'migrate'
> command to get rid of crufty parameters?

I didn't read it that way, but I would not oppose O:-)

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Eric Blake <eblake@redhat.com> wrote:
> > On 05/12/2017 05:55 AM, Juan Quintela wrote:
> >>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >>>>      }
> >>>>  
> >>>>      if (has_inc && inc) {
> >>>> +        migrate_set_block_enabled(s, true);
> >>>>          migrate_set_block_shared(s, true);
> >>>
> >>> [2]
> >>>
> >>> IIUC for [1] & [2] we are solving the same problem that "shared"
> >>> depends on "enabled" bit. Would it be good to unitfy this dependency
> >>> somewhere? E.g., by changing migrate_set_block_shared() into:
> >>>
> >>> void migrate_set_block_shared(MigrationState *s, bool value)
> >>> {
> >>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
> >>>     if (value) {
> >>>         migrate_set_block_enabled(s, true);
> >>>     }
> >>> }
> >> 
> >> ok with this.
> >
> > Or, as I commented on 1/3, maybe having a single property that is a
> > tri-state enum value, instead of 2 separate boolean properties, might be
> > nicer (but certainly a bit more complex to code up).
> 
> If you teach me how to do the qapi/qmp part, I will do the other bits.
> I don't really care if we do it one way or the other.
> 
> >> I will add once here that when we disable block enabled, we also disable
> >> shared, or just let it that way?
> >> 
> >>> Another thing to mention: after switching to the capability interface,
> >>> we'll cache the "enabled" and "shared" bits now while we don't cache
> >>> it before, right? IIUC it'll affect behavior of such sequence:
> >>>
> >>> - 1st migrate with enabled=1, shared=1, then
> >>> - 2nd migrate with enabled=0, shared=0
> >>>
> >>> Before the series, the 2nd migrate will use enabled=shared=0, but
> >>> after the series it should be using enabled=shared=1. Not sure whether
> >>> this would be a problem (or I missed anything?).
> >> 
> >> We can't be consistent with both old/new way.
> >> 
> >> Old way: we always setup the capabilities on command line (that should
> >> have been deprecated long, long ago)
> >
> > Well, the easy way out is to have the HMP migrate command (I assume
> > that's what you mean by "on command line") explicitly clear the
> > parameters if it is called without the -b/-i flag.  So the start of each
> > migration is what changes the properties, so long as you are still using
> > HMP to start the migration.  Or, on the QMP side, since 'migrate' has
> > optional 'blk' and 'inc' booleans, basically leave the settings alone if
> > the parameters were omitted, and explicitly update the property to the
> > value of those parameters if they were present.
> 
> We are going to have trouble whatever way that we do it, or we start
> doing lots of strange things.
> 
> Forget about qmp, we are going to assume that it is consistent with hmp.
> 
> migrate_set_capabilities block_enabled on
> migrate -b .....
> 
> Should migrate disable the block_enabled capability?  Give one
> warning/error?
> 
> And notice that this only matter if we do a migration, we cancel/get one
> error, and then we migrate again.
> 
> What I tried to do is assume that -b/-i arguments don't exist.  And if
> the user use them, we implement its behaviour with the minimal fuss
> possibly.
> 
> Only way that I can think of being consistent and bug compatible will be
> to store:
> - old block_shared/enanbeld capability value
> - if we set -b/-i on the command line
> 
> In migration cleanup do the right thing depending on this four
> variables.  I think that it is adding lots of complexity for very few
> improvement.
> 
> 
> > Or is the proposal that we are also going to simplify the QMP 'migrate'
> > command to get rid of crufty parameters?
> 
> I didn't read it that way, but I would not oppose O:-)
> 

Ewww this is messy; you end up with almost as much code as the old
flags you're trying to remove.
For HMP you could gently deprecate the flags over time and give
a warning telling people to use the capabilities; but doing that
in one go is a bit nasty, and you still have to do something
cleverer for the QMP which is similar.
 
I think I agree though that migrate, cancel, migrate should work
sensibly and it's hard to get it right.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]

[adding Markus]

On 05/15/2017 04:48 AM, Juan Quintela wrote:
> Eric Blake <eblake@redhat.com> wrote:
>> On 05/12/2017 05:55 AM, Juan Quintela wrote:
>>>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>>      }
>>>>>  
>>>>>      if (has_inc && inc) {
>>>>> +        migrate_set_block_enabled(s, true);
>>>>>          migrate_set_block_shared(s, true);
>>>>
>>>> [2]
>>>>
>>>> IIUC for [1] & [2] we are solving the same problem that "shared"
>>>> depends on "enabled" bit. Would it be good to unitfy this dependency
>>>> somewhere? E.g., by changing migrate_set_block_shared() into:
>>>>
>>>> void migrate_set_block_shared(MigrationState *s, bool value)
>>>> {
>>>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>>>>     if (value) {
>>>>         migrate_set_block_enabled(s, true);
>>>>     }
>>>> }
>>>
>>> ok with this.
>>
>> Or, as I commented on 1/3, maybe having a single property that is a
>> tri-state enum value, instead of 2 separate boolean properties, might be
>> nicer (but certainly a bit more complex to code up).
> 
> If you teach me how to do the qapi/qmp part, I will do the other bits.
> I don't really care if we do it one way or the other.

Adding Markus in, as I value his opinion on matters of UI design.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> [adding Markus]
>
> On 05/15/2017 04:48 AM, Juan Quintela wrote:
>> Eric Blake <eblake@redhat.com> wrote:
>>> On 05/12/2017 05:55 AM, Juan Quintela wrote:
>>>>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>>>      }
>>>>>>  
>>>>>>      if (has_inc && inc) {
>>>>>> +        migrate_set_block_enabled(s, true);
>>>>>>          migrate_set_block_shared(s, true);
>>>>>
>>>>> [2]
>>>>>
>>>>> IIUC for [1] & [2] we are solving the same problem that "shared"
>>>>> depends on "enabled" bit. Would it be good to unitfy this dependency
>>>>> somewhere? E.g., by changing migrate_set_block_shared() into:
>>>>>
>>>>> void migrate_set_block_shared(MigrationState *s, bool value)
>>>>> {
>>>>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>>>>>     if (value) {
>>>>>         migrate_set_block_enabled(s, true);
>>>>>     }
>>>>> }
>>>>
>>>> ok with this.
>>>
>>> Or, as I commented on 1/3, maybe having a single property that is a
>>> tri-state enum value, instead of 2 separate boolean properties, might be
>>> nicer (but certainly a bit more complex to code up).
>> 
>> If you teach me how to do the qapi/qmp part, I will do the other bits.
>> I don't really care if we do it one way or the other.
>
> Adding Markus in, as I value his opinion on matters of UI design.


This is my "working" version of Dave patch on top of my block layer
changes.  Still using the two capabilities.

I am open to suggestions


commit 242852b71ec2b8f08754afa6eafce6fa3fe69139
Author: Dr. David Alan Gilbert <dgilbert@redhat.com>
Date:   Mon May 15 15:05:29 2017 +0100

    block migration: Allow compile time disable
    
    Many users now prefer to use drive_mirror over NBD as an
    alternative to the older migrate -b option; drive_mirror is
    more complex to setup but gives you more options (e.g. only
    migrating some of the disks if some of them are shared).
    
    Allow the large chunk of block migration code to be compiled
    out for those who don't use it.
    
    Based on a downstream-patch we've had for a while by Jeff Cody.
    
    Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

diff --git a/configure b/configure
index 7c020c0..7aba6c5 100755
--- a/configure
+++ b/configure
@@ -316,6 +316,7 @@ vte=""
 virglrenderer=""
 tpm="yes"
 libssh2=""
+live_block_migration="yes"
 numa=""
 tcmalloc="no"
 jemalloc="no"
@@ -1168,6 +1169,10 @@ for opt do
   ;;
   --enable-libssh2) libssh2="yes"
   ;;
+  --disable-live-block-migration) live_block_migration="no"
+  ;;
+  --enable-live-block-migration) live_block_migration="yes"
+  ;;
   --disable-numa) numa="no"
   ;;
   --enable-numa) numa="yes"
@@ -1400,6 +1405,7 @@ disabled with --disable-FEATURE, default is enabled if available:
   libnfs          nfs support
   smartcard       smartcard support (libcacard)
   libusb          libusb (for usb passthrough)
+  live-block-migration   Block migration in the main migration stream
   usb-redir       usb network redirection support
   lzo             support of lzo compression library
   snappy          support of snappy compression library
@@ -5224,6 +5230,7 @@ echo "TPM support       $tpm"
 echo "libssh2 support   $libssh2"
 echo "TPM passthrough   $tpm_passthrough"
 echo "QOM debugging     $qom_cast_debug"
+echo "Live block migration $live_block_migration"
 echo "lzo support       $lzo"
 echo "snappy support    $snappy"
 echo "bzip2 support     $bzip2"
@@ -5790,6 +5797,10 @@ if test "$libssh2" = "yes" ; then
   echo "LIBSSH2_LIBS=$libssh2_libs" >> $config_host_mak
 fi
 
+if test "$live_block_migration" = "yes" ; then
+  echo "CONFIG_LIVE_BLOCK_MIGRATION=y" >> $config_host_mak
+fi
+
 # USB host support
 if test "$libusb" = "yes"; then
   echo "HOST_USB=libusb legacy" >> $config_host_mak
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 5c62e92..99b5795 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -22,7 +22,12 @@ void ram_mig_init(void);
 
 /* migration/block.c */
 
+#ifdef CONFIG_LIVE_BLOCK_MIGRATION
 void blk_mig_init(void);
+#else
+static inline void blk_mig_init(void) { };
+#endif
+
 
 #define SELF_ANNOUNCE_ROUNDS 5
 
diff --git a/migration/Makefile.objs b/migration/Makefile.objs
index 4277c88..1c7770d 100644
--- a/migration/Makefile.objs
+++ b/migration/Makefile.objs
@@ -9,5 +9,5 @@ common-obj-y += qjson.o
 
 common-obj-$(CONFIG_RDMA) += rdma.o
 
-common-obj-y += block.o
+common-obj-$(CONFIG_LIVE_BLOCK_MIGRATION) += block.o
 
diff --git a/migration/block.h b/migration/block.h
index a4d4974..1884fd0 100644
--- a/migration/block.h
+++ b/migration/block.h
@@ -14,12 +14,33 @@
 #ifndef MIGRATION_BLOCK_H
 #define MIGRATION_BLOCK_H
 
+#ifdef CONFIG_LIVE_BLOCK_MIGRATION
 int blk_mig_active(void);
 uint64_t blk_mig_bytes_transferred(void);
 uint64_t blk_mig_bytes_remaining(void);
 uint64_t blk_mig_bytes_total(void);
 
-void migrate_set_block_shared(MigrationState *s, bool value);
-void migrate_set_block_enabled(MigrationState *s, bool value);
+#else
+static inline int blk_mig_active(void)
+{
+    return false;
+}
+static inline uint64_t blk_mig_bytes_transferred(void)
+{
+    return 0;
+}
 
+static inline uint64_t blk_mig_bytes_remaining(void)
+{
+    return 0;
+}
+
+static inline uint64_t blk_mig_bytes_total(void)
+{
+    return 0;
+}
+#endif /* CONFIG_LIVE_BLOCK_MIGRATION */
+
+bool migrate_set_block_shared(MigrationState *s, bool value, Error **errp);
+bool migrate_set_block_enabled(MigrationState *s, bool value, Error **errp);
 #endif /* MIGRATION_BLOCK_H */
diff --git a/migration/colo.c b/migration/colo.c
index 10ee97c..4c652ba 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -348,8 +348,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     /* Disable block migration */
-    migrate_set_block_enabled(s, false);
-    migrate_set_block_shared(s, false);
+    migrate_set_block_enabled(s, false, NULL);
+    migrate_set_block_shared(s, false, NULL);
     qemu_savevm_state_header(fb);
     qemu_savevm_state_begin(fb);
     qemu_mutex_lock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index fd0b11f..a525de5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -595,11 +595,20 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
         }
         s->enabled_capabilities[cap->value->capability] = cap->value->state;
     }
-
+#ifdef CONFIG_LIVE_BLOCK_MIGRATION
     if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
         s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
     }
-
+#else
+    if ((s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED])
+        ||(s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED])) {
+        error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
+                         "block migration");
+        error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
+        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = false;
+        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = false;
+    }
+#endif
     if (migrate_postcopy_ram()) {
         if (migrate_use_compression()) {
             /* The decompression threads asynchronously write into RAM
@@ -1004,20 +1013,38 @@ bool migration_is_blocked(Error **errp)
     return false;
 }
 
-void migrate_set_block_shared(MigrationState *s, bool value)
+bool migrate_set_block_shared(MigrationState *s, bool value, Error **errp)
 {
+#ifndef CONFIG_LIVE_BLOCK_MIGRATION
+    if (value) {
+        error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
+                         "block migration");
+        error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
+        return false;
+    }
+#endif
     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
     if (value) {
         s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
     }
+    return true;
 }
 
-void migrate_set_block_enabled(MigrationState *s, bool value)
+bool migrate_set_block_enabled(MigrationState *s, bool value, Error **errp)
 {
+#ifndef CONFIG_LIVE_BLOCK_MIGRATION
+    if (value) {
+        error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
+                         "block migration");
+        error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
+        return false;
+    }
+#endif
     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = value;
     if (!value) {
         s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = false;
     }
+    return true;
 }
 
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
@@ -1046,11 +1073,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     s = migrate_init();
 
     if (has_blk && blk) {
-        migrate_set_block_enabled(s, true);
+        if(!migrate_set_block_enabled(s, true, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     if (has_inc && inc) {
-        migrate_set_block_shared(s, true);
+        if(!migrate_set_block_shared(s, true, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     if (strstart(uri, "tcp:", &p)) {

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Eric Blake <eblake@redhat.com> wrote:
>> On 05/12/2017 05:55 AM, Juan Quintela wrote:
>>>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>>      }
>>>>>  
>>>>>      if (has_inc && inc) {
>>>>> +        migrate_set_block_enabled(s, true);
>>>>>          migrate_set_block_shared(s, true);
>>>>
>>>> [2]
>>>>
>>>> IIUC for [1] & [2] we are solving the same problem that "shared"
>>>> depends on "enabled" bit. Would it be good to unitfy this dependency
>>>> somewhere? E.g., by changing migrate_set_block_shared() into:
>>>>
>>>> void migrate_set_block_shared(MigrationState *s, bool value)
>>>> {
>>>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
>>>>     if (value) {
>>>>         migrate_set_block_enabled(s, true);
>>>>     }
>>>> }
>>> 
>>> ok with this.
>>
>> Or, as I commented on 1/3, maybe having a single property that is a
>> tri-state enum value, instead of 2 separate boolean properties, might be
>> nicer (but certainly a bit more complex to code up).
>
> If you teach me how to do the qapi/qmp part, I will do the other bits.
> I don't really care if we do it one way or the other.
>
>>> I will add once here that when we disable block enabled, we also disable
>>> shared, or just let it that way?
>>> 
>>>> Another thing to mention: after switching to the capability interface,
>>>> we'll cache the "enabled" and "shared" bits now while we don't cache
>>>> it before, right? IIUC it'll affect behavior of such sequence:
>>>>
>>>> - 1st migrate with enabled=1, shared=1, then
>>>> - 2nd migrate with enabled=0, shared=0
>>>>
>>>> Before the series, the 2nd migrate will use enabled=shared=0, but
>>>> after the series it should be using enabled=shared=1. Not sure whether
>>>> this would be a problem (or I missed anything?).
>>> 
>>> We can't be consistent with both old/new way.
>>> 
>>> Old way: we always setup the capabilities on command line (that should
>>> have been deprecated long, long ago)
>>
>> Well, the easy way out is to have the HMP migrate command (I assume
>> that's what you mean by "on command line") explicitly clear the
>> parameters if it is called without the -b/-i flag.  So the start of each
>> migration is what changes the properties, so long as you are still using
>> HMP to start the migration.  Or, on the QMP side, since 'migrate' has
>> optional 'blk' and 'inc' booleans, basically leave the settings alone if
>> the parameters were omitted, and explicitly update the property to the
>> value of those parameters if they were present.
>
> We are going to have trouble whatever way that we do it, or we start
> doing lots of strange things.
>
> Forget about qmp, we are going to assume that it is consistent with hmp.
>
> migrate_set_capabilities block_enabled on
> migrate -b .....
>
> Should migrate disable the block_enabled capability?  Give one
> warning/error?
>
> And notice that this only matter if we do a migration, we cancel/get one
> error, and then we migrate again.
>
> What I tried to do is assume that -b/-i arguments don't exist.  And if
> the user use them, we implement its behaviour with the minimal fuss
> possibly.
>
> Only way that I can think of being consistent and bug compatible will be
> to store:
> - old block_shared/enanbeld capability value
> - if we set -b/-i on the command line
>
> In migration cleanup do the right thing depending on this four
> variables.  I think that it is adding lots of complexity for very few
> improvement.
>
>
>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>> command to get rid of crufty parameters?
>
> I didn't read it that way, but I would not oppose O:-)
>
> Later, Juan.

I'm not too familiar with this stuff, so please correct my
misunderstandings.

"Normal" migration configuration is global state, i.e. it applies to all
future migrations.

Except the "migrate" command's flags apply to just the migration kicked
off by that command.

QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
!blk && inc makes no sense and is silently treated like !blk && !inc.

There's a third flag "detach" (HMP: -d), but it does nothing in QMP.

You'd like to deprecate these flags in favour of "normal" configuration.
However, we need to maintain QMP backward compatibility at least for a
while.  HMP backward compatibility is nice to have, but not required.

First step is to design the new interface you want.  Second step is to
figure out backward compatibility.

The new interface adds a block migration tri-state (off,
non-incremental, incremental) to global state, default off.  Whether
it's done as two bools or an enum of three values doesn't matter here.

If the new interface isn't used, the old one still needs to work.  If it
is used, the old one either has to do "the right thing", or fail
cleanly.

We approximate "new interface isn't used" by "block migration is off in
global state".  When it is off, the migration command needs to honor its
two flags for compatibility.  It must leave block migration off in
global state.  Yes, this will complicate the implementation until we
actually remove the deprecated flags.  Par for the backward compatility
course.

When block migration isn't off in global state, we can either

* let the flags take precedence over the global state (one
  interpretation of "do the right thing"), or

* reject flags that conflict with global state (another interpretation),
  or

* reject *all* flags (fail cleanly).

The last one looks perfectly servicable to me.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Eric Blake <eblake@redhat.com> wrote:


>>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>>> command to get rid of crufty parameters?
>>
>> I didn't read it that way, but I would not oppose O:-)
>>
>> Later, Juan.
>
> I'm not too familiar with this stuff, so please correct my
> misunderstandings.
>
> "Normal" migration configuration is global state, i.e. it applies to all
> future migrations.
>
> Except the "migrate" command's flags apply to just the migration kicked
> off by that command.
>
> QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> !blk && inc makes no sense and is silently treated like !blk && !inc.
>
> There's a third flag "detach" (HMP: -d), but it does nothing in QMP.

As qmp command is asynchronous, you can think that -d is *always* on in
QMP O:-)

> You'd like to deprecate these flags in favour of "normal" configuration.
> However, we need to maintain QMP backward compatibility at least for a
> while.  HMP backward compatibility is nice to have, but not required.
>
> First step is to design the new interface you want.  Second step is to
> figure out backward compatibility.
>
> The new interface adds a block migration tri-state (off,
> non-incremental, incremental) to global state, default off.  Whether
> it's done as two bools or an enum of three values doesn't matter here.

Tristates will complicate it.  I still think that:

- capability: block_migration
- parameter: block_shared

Makes more sense, no?

If block_migration is not enabled, we ignore the shared parameter.  We
already do that for other parameters.

> If the new interface isn't used, the old one still needs to work.  If it
> is used, the old one either has to do "the right thing", or fail
> cleanly.
>
> We approximate "new interface isn't used" by "block migration is off in
> global state".  When it is off, the migration command needs to honor its
> two flags for compatibility.  It must leave block migration off in
> global state.  Yes, this will complicate the implementation until we
> actually remove the deprecated flags.  Par for the backward compatility
> course.
>
> When block migration isn't off in global state, we can either
>
> * let the flags take precedence over the global state (one
>   interpretation of "do the right thing"), or
>
> * reject flags that conflict with global state (another interpretation),
>   or
>
> * reject *all* flags (fail cleanly).
>
> The last one looks perfectly servicable to me.

Yeap,  I think that makes sense.  If you use capabilities, parameters,
old interface don't work at all.

We still have a problem that is what happens if the user does:

migrate -b <foo>
migrate_cancel (or error)
migrate <bar> (without -b)

With current patches, it will still use -b.  Fixing it requires still
anding more code.  But I think that this use case is so weird what we
should not even care about it.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Markus Armbruster <armbru@redhat.com> wrote:
> > Juan Quintela <quintela@redhat.com> writes:
> >
> >> Eric Blake <eblake@redhat.com> wrote:
> 
> 
> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
> >>> command to get rid of crufty parameters?
> >>
> >> I didn't read it that way, but I would not oppose O:-)
> >>
> >> Later, Juan.
> >
> > I'm not too familiar with this stuff, so please correct my
> > misunderstandings.
> >
> > "Normal" migration configuration is global state, i.e. it applies to all
> > future migrations.
> >
> > Except the "migrate" command's flags apply to just the migration kicked
> > off by that command.
> >
> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> > !blk && inc makes no sense and is silently treated like !blk && !inc.
> >
> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
> 
> As qmp command is asynchronous, you can think that -d is *always* on in
> QMP O:-)
> 
> > You'd like to deprecate these flags in favour of "normal" configuration.
> > However, we need to maintain QMP backward compatibility at least for a
> > while.  HMP backward compatibility is nice to have, but not required.
> >
> > First step is to design the new interface you want.  Second step is to
> > figure out backward compatibility.
> >
> > The new interface adds a block migration tri-state (off,
> > non-incremental, incremental) to global state, default off.  Whether
> > it's done as two bools or an enum of three values doesn't matter here.
> 
> Tristates will complicate it.  I still think that:
> 
> - capability: block_migration
> - parameter: block_shared
> 
> Makes more sense, no?

I don't understand what making block_shared a parameter gives you as
opposed to simply having two capabilities.

(And how did we get 'shared'? We started off with block & incremental)

Dave

> If block_migration is not enabled, we ignore the shared parameter.  We
> already do that for other parameters.
> 
> > If the new interface isn't used, the old one still needs to work.  If it
> > is used, the old one either has to do "the right thing", or fail
> > cleanly.
> >
> > We approximate "new interface isn't used" by "block migration is off in
> > global state".  When it is off, the migration command needs to honor its
> > two flags for compatibility.  It must leave block migration off in
> > global state.  Yes, this will complicate the implementation until we
> > actually remove the deprecated flags.  Par for the backward compatility
> > course.
> >
> > When block migration isn't off in global state, we can either
> >
> > * let the flags take precedence over the global state (one
> >   interpretation of "do the right thing"), or
> >
> > * reject flags that conflict with global state (another interpretation),
> >   or
> >
> > * reject *all* flags (fail cleanly).
> >
> > The last one looks perfectly servicable to me.
> 
> Yeap,  I think that makes sense.  If you use capabilities, parameters,
> old interface don't work at all.
> 
> We still have a problem that is what happens if the user does:
> 
> migrate -b <foo>
> migrate_cancel (or error)
> migrate <bar> (without -b)
> 
> With current patches, it will still use -b.  Fixing it requires still
> anding more code.  But I think that this use case is so weird what we
> should not even care about it.
> 
> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Markus Armbruster <armbru@redhat.com> wrote:
>> > Juan Quintela <quintela@redhat.com> writes:
>> >
>> >> Eric Blake <eblake@redhat.com> wrote:
>> 
>> 
>> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>> >>> command to get rid of crufty parameters?
>> >>
>> >> I didn't read it that way, but I would not oppose O:-)
>> >>
>> >> Later, Juan.
>> >
>> > I'm not too familiar with this stuff, so please correct my
>> > misunderstandings.
>> >
>> > "Normal" migration configuration is global state, i.e. it applies to all
>> > future migrations.
>> >
>> > Except the "migrate" command's flags apply to just the migration kicked
>> > off by that command.
>> >
>> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
>> > !blk && inc makes no sense and is silently treated like !blk && !inc.
>> >
>> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
>> 
>> As qmp command is asynchronous, you can think that -d is *always* on in
>> QMP O:-)
>> 
>> > You'd like to deprecate these flags in favour of "normal" configuration.
>> > However, we need to maintain QMP backward compatibility at least for a
>> > while.  HMP backward compatibility is nice to have, but not required.
>> >
>> > First step is to design the new interface you want.  Second step is to
>> > figure out backward compatibility.
>> >
>> > The new interface adds a block migration tri-state (off,
>> > non-incremental, incremental) to global state, default off.  Whether
>> > it's done as two bools or an enum of three values doesn't matter here.
>> 
>> Tristates will complicate it.  I still think that:
>> 
>> - capability: block_migration
>> - parameter: block_shared
>> 
>> Makes more sense, no?
>
> I don't understand what making block_shared a parameter gives you as
> opposed to simply having two capabilities.
>
> (And how did we get 'shared'? We started off with block & incremental)

The variables on MigrationParams:

struct MigrationParams {
    bool blk;
    bool shared;
};


I can move to incremental.  I am not sure which one is clearer.

The advantage of having shared as a parameter is that we forget about
all this dependency bussiness.  Is the same than compression_threads
paramter, you setup to whichever value that you want.  But you don't get
compression_threads until you set the compress capability.

So, in this case we will have:

block capability: Are we using block migration or not
block-incremental parameter: If we are using block migration, are we
      using incremental copying of the block layer?


Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> Markus Armbruster <armbru@redhat.com> wrote:
> >> > Juan Quintela <quintela@redhat.com> writes:
> >> >
> >> >> Eric Blake <eblake@redhat.com> wrote:
> >> 
> >> 
> >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
> >> >>> command to get rid of crufty parameters?
> >> >>
> >> >> I didn't read it that way, but I would not oppose O:-)
> >> >>
> >> >> Later, Juan.
> >> >
> >> > I'm not too familiar with this stuff, so please correct my
> >> > misunderstandings.
> >> >
> >> > "Normal" migration configuration is global state, i.e. it applies to all
> >> > future migrations.
> >> >
> >> > Except the "migrate" command's flags apply to just the migration kicked
> >> > off by that command.
> >> >
> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> >> > !blk && inc makes no sense and is silently treated like !blk && !inc.
> >> >
> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
> >> 
> >> As qmp command is asynchronous, you can think that -d is *always* on in
> >> QMP O:-)
> >> 
> >> > You'd like to deprecate these flags in favour of "normal" configuration.
> >> > However, we need to maintain QMP backward compatibility at least for a
> >> > while.  HMP backward compatibility is nice to have, but not required.
> >> >
> >> > First step is to design the new interface you want.  Second step is to
> >> > figure out backward compatibility.
> >> >
> >> > The new interface adds a block migration tri-state (off,
> >> > non-incremental, incremental) to global state, default off.  Whether
> >> > it's done as two bools or an enum of three values doesn't matter here.
> >> 
> >> Tristates will complicate it.  I still think that:
> >> 
> >> - capability: block_migration
> >> - parameter: block_shared
> >> 
> >> Makes more sense, no?
> >
> > I don't understand what making block_shared a parameter gives you as
> > opposed to simply having two capabilities.
> >
> > (And how did we get 'shared'? We started off with block & incremental)
> 
> The variables on MigrationParams:
> 
> struct MigrationParams {
>     bool blk;
>     bool shared;
> };
> 
> 
> I can move to incremental.  I am not sure which one is clearer.
> 
> The advantage of having shared as a parameter is that we forget about
> all this dependency bussiness.  Is the same than compression_threads
> paramter, you setup to whichever value that you want.  But you don't get
> compression_threads until you set the compress capability.
> 
> So, in this case we will have:
> 
> block capability: Are we using block migration or not
> block-incremental parameter: If we are using block migration, are we
>       using incremental copying of the block layer?

If it's still a boolean why does having it as a parameter remove the
dependency?

Dave

> 
> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>> > * Juan Quintela (quintela@redhat.com) wrote:
>> >> Markus Armbruster <armbru@redhat.com> wrote:
>> >> > Juan Quintela <quintela@redhat.com> writes:
>> >> >
>> >> >> Eric Blake <eblake@redhat.com> wrote:
>> >> 
>> >> 
>> >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>> >> >>> command to get rid of crufty parameters?
>> >> >>
>> >> >> I didn't read it that way, but I would not oppose O:-)
>> >> >>
>> >> >> Later, Juan.
>> >> >
>> >> > I'm not too familiar with this stuff, so please correct my
>> >> > misunderstandings.
>> >> >
>> >> > "Normal" migration configuration is global state, i.e. it applies to all
>> >> > future migrations.
>> >> >
>> >> > Except the "migrate" command's flags apply to just the migration kicked
>> >> > off by that command.
>> >> >
>> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
>> >> > !blk && inc makes no sense and is silently treated like !blk && !inc.
>> >> >
>> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
>> >> 
>> >> As qmp command is asynchronous, you can think that -d is *always* on in
>> >> QMP O:-)
>> >> 
>> >> > You'd like to deprecate these flags in favour of "normal" configuration.
>> >> > However, we need to maintain QMP backward compatibility at least for a
>> >> > while.  HMP backward compatibility is nice to have, but not required.
>> >> >
>> >> > First step is to design the new interface you want.  Second step is to
>> >> > figure out backward compatibility.
>> >> >
>> >> > The new interface adds a block migration tri-state (off,
>> >> > non-incremental, incremental) to global state, default off.  Whether
>> >> > it's done as two bools or an enum of three values doesn't matter here.
>> >> 
>> >> Tristates will complicate it.  I still think that:
>> >> 
>> >> - capability: block_migration
>> >> - parameter: block_shared
>> >> 
>> >> Makes more sense, no?
>> >
>> > I don't understand what making block_shared a parameter gives you as
>> > opposed to simply having two capabilities.
>> >
>> > (And how did we get 'shared'? We started off with block & incremental)
>> 
>> The variables on MigrationParams:
>> 
>> struct MigrationParams {
>>     bool blk;
>>     bool shared;
>> };
>> 
>> 
>> I can move to incremental.  I am not sure which one is clearer.
>> 
>> The advantage of having shared as a parameter is that we forget about
>> all this dependency bussiness.  Is the same than compression_threads
>> paramter, you setup to whichever value that you want.  But you don't get
>> compression_threads until you set the compress capability.
>> 
>> So, in this case we will have:
>> 
>> block capability: Are we using block migration or not
>> block-incremental parameter: If we are using block migration, are we
>>       using incremental copying of the block layer?
>
> If it's still a boolean why does having it as a parameter remove the
> dependency?

Forget -b/-i.

migration_set_parameter compression_threads 8

migrate <foo>

We don't use compression_threads at all

migrate_set_capability compress

migrate <foo>

Now, we use compression threads.

So, compression_threads parameter is a parameter that is only used when
compress capability is enabled.

Same for block migration.  Block_incremental parameter is used only when
block migration capability is setup.  No dependency check needed at all.

Or I am losing something obvious here?

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >> > * Juan Quintela (quintela@redhat.com) wrote:
> >> >> Markus Armbruster <armbru@redhat.com> wrote:
> >> >> > Juan Quintela <quintela@redhat.com> writes:
> >> >> >
> >> >> >> Eric Blake <eblake@redhat.com> wrote:
> >> >> 
> >> >> 
> >> >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
> >> >> >>> command to get rid of crufty parameters?
> >> >> >>
> >> >> >> I didn't read it that way, but I would not oppose O:-)
> >> >> >>
> >> >> >> Later, Juan.
> >> >> >
> >> >> > I'm not too familiar with this stuff, so please correct my
> >> >> > misunderstandings.
> >> >> >
> >> >> > "Normal" migration configuration is global state, i.e. it applies to all
> >> >> > future migrations.
> >> >> >
> >> >> > Except the "migrate" command's flags apply to just the migration kicked
> >> >> > off by that command.
> >> >> >
> >> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> >> >> > !blk && inc makes no sense and is silently treated like !blk && !inc.
> >> >> >
> >> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
> >> >> 
> >> >> As qmp command is asynchronous, you can think that -d is *always* on in
> >> >> QMP O:-)
> >> >> 
> >> >> > You'd like to deprecate these flags in favour of "normal" configuration.
> >> >> > However, we need to maintain QMP backward compatibility at least for a
> >> >> > while.  HMP backward compatibility is nice to have, but not required.
> >> >> >
> >> >> > First step is to design the new interface you want.  Second step is to
> >> >> > figure out backward compatibility.
> >> >> >
> >> >> > The new interface adds a block migration tri-state (off,
> >> >> > non-incremental, incremental) to global state, default off.  Whether
> >> >> > it's done as two bools or an enum of three values doesn't matter here.
> >> >> 
> >> >> Tristates will complicate it.  I still think that:
> >> >> 
> >> >> - capability: block_migration
> >> >> - parameter: block_shared
> >> >> 
> >> >> Makes more sense, no?
> >> >
> >> > I don't understand what making block_shared a parameter gives you as
> >> > opposed to simply having two capabilities.
> >> >
> >> > (And how did we get 'shared'? We started off with block & incremental)
> >> 
> >> The variables on MigrationParams:
> >> 
> >> struct MigrationParams {
> >>     bool blk;
> >>     bool shared;
> >> };
> >> 
> >> 
> >> I can move to incremental.  I am not sure which one is clearer.
> >> 
> >> The advantage of having shared as a parameter is that we forget about
> >> all this dependency bussiness.  Is the same than compression_threads
> >> paramter, you setup to whichever value that you want.  But you don't get
> >> compression_threads until you set the compress capability.
> >> 
> >> So, in this case we will have:
> >> 
> >> block capability: Are we using block migration or not
> >> block-incremental parameter: If we are using block migration, are we
> >>       using incremental copying of the block layer?
> >
> > If it's still a boolean why does having it as a parameter remove the
> > dependency?
> 
> Forget -b/-i.
> 
> migration_set_parameter compression_threads 8
> 
> migrate <foo>
> 
> We don't use compression_threads at all
> 
> migrate_set_capability compress
> 
> migrate <foo>
> 
> Now, we use compression threads.
> 
> So, compression_threads parameter is a parameter that is only used when
> compress capability is enabled.
> 
> Same for block migration.  Block_incremental parameter is used only when
> block migration capability is setup.  No dependency check needed at all.
> 
> Or I am losing something obvious here?

Ah, you've made up a new rule - I don't think it's a bad rule
but is it true? Do we always enable a capability before we use a
parameter? I don't think so - I think the tls parameters don't have
a capability.
My previous rule was just that if it was a bool it was a capability
and you can have whatever dependencies you like there - or none.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

>> Forget -b/-i.
>> 
>> migration_set_parameter compression_threads 8
>> 
>> migrate <foo>
>> 
>> We don't use compression_threads at all
>> 
>> migrate_set_capability compress
>> 
>> migrate <foo>
>> 
>> Now, we use compression threads.
>> 
>> So, compression_threads parameter is a parameter that is only used when
>> compress capability is enabled.
>> 
>> Same for block migration.  Block_incremental parameter is used only when
>> block migration capability is setup.  No dependency check needed at all.
>> 
>> Or I am losing something obvious here?
>
> Ah, you've made up a new rule

Oops, I thought that was a rule on how things worked O:-)

>- I don't think it's a bad rule
> but is it true? Do we always enable a capability before we use a
> parameter? I don't think so - I think the tls parameters don't have
> a capability.

To use tls paramater, we need to use an url with tls, no?

> My previous rule was just that if it was a bool it was a capability
> and you can have whatever dependencies you like there - or none.

Dunno.

If you think that we can document it (one way or another), I am for it.

It is really weird that we both thought (reading) the interface that the
rules are different O:-)

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> >> Forget -b/-i.
> >> 
> >> migration_set_parameter compression_threads 8
> >> 
> >> migrate <foo>
> >> 
> >> We don't use compression_threads at all
> >> 
> >> migrate_set_capability compress
> >> 
> >> migrate <foo>
> >> 
> >> Now, we use compression threads.
> >> 
> >> So, compression_threads parameter is a parameter that is only used when
> >> compress capability is enabled.
> >> 
> >> Same for block migration.  Block_incremental parameter is used only when
> >> block migration capability is setup.  No dependency check needed at all.
> >> 
> >> Or I am losing something obvious here?
> >
> > Ah, you've made up a new rule
> 
> Oops, I thought that was a rule on how things worked O:-)
> 
> >- I don't think it's a bad rule
> > but is it true? Do we always enable a capability before we use a
> > parameter? I don't think so - I think the tls parameters don't have
> > a capability.
> 
> To use tls paramater, we need to use an url with tls, no?
> 
> > My previous rule was just that if it was a bool it was a capability
> > and you can have whatever dependencies you like there - or none.
> 
> Dunno.
> 
> If you think that we can document it (one way or another), I am for it.
> 
> It is really weird that we both thought (reading) the interface that the
> rules are different O:-)

Well I think that's because no one ever defined any rules!  We just
created parameters because we needed something that could take
non-boolean values, but I don't think there's anything more
about the structure of their use that's actually defined.
Similarly, I don't think there's anything defined about the
structure of capabilities.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Juan Quintela <quintela@redhat.com> writes:
>>
>>> Eric Blake <eblake@redhat.com> wrote:
>
>
>>>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>>>> command to get rid of crufty parameters?
>>>
>>> I didn't read it that way, but I would not oppose O:-)
>>>
>>> Later, Juan.
>>
>> I'm not too familiar with this stuff, so please correct my
>> misunderstandings.
>>
>> "Normal" migration configuration is global state, i.e. it applies to all
>> future migrations.
>>
>> Except the "migrate" command's flags apply to just the migration kicked
>> off by that command.
>>
>> QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
>> !blk && inc makes no sense and is silently treated like !blk && !inc.
>>
>> There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
>
> As qmp command is asynchronous, you can think that -d is *always* on in
> QMP O:-)

Yes.  The existence of "detach" in QMP is owed to limitations of early
QMP infrastructure.  It's flagged as "invalid" and "should not be
used" since 2010.

Perhaps we should start a section on QMP in
<http://wiki.qemu.org/Features/LegacyRemoval>.  But I'd like to first
have a way to communicate "you're using a deprecated feature" warnings
via QMP.

>> You'd like to deprecate these flags in favour of "normal" configuration.
>> However, we need to maintain QMP backward compatibility at least for a
>> while.  HMP backward compatibility is nice to have, but not required.
>>
>> First step is to design the new interface you want.  Second step is to
>> figure out backward compatibility.
>>
>> The new interface adds a block migration tri-state (off,
>> non-incremental, incremental) to global state, default off.  Whether
>> it's done as two bools or an enum of three values doesn't matter here.
>
> Tristates will complicate it.  I still think that:
>
> - capability: block_migration
> - parameter: block_shared
>
> Makes more sense, no?
>
> If block_migration is not enabled, we ignore the shared parameter.  We
> already do that for other parameters.

My impression as a superficial reader is that migration configuration is
a historically grown mess.  Perhaps we shouldn't try to interpret too
much intent into it :)

If we redo migration as an instance of the "job" abstraction once we
have it, then migration configuration & control should become more less
messy.  Of course, the old messes will stay with us for a while in the
form of backward compatibility messes.

I'm not too particular on how we do the tri-state now, as long as it
reasonably fits what we have, and is documented clearly.

>> If the new interface isn't used, the old one still needs to work.  If it
>> is used, the old one either has to do "the right thing", or fail
>> cleanly.
>>
>> We approximate "new interface isn't used" by "block migration is off in
>> global state".  When it is off, the migration command needs to honor its
>> two flags for compatibility.  It must leave block migration off in
>> global state.  Yes, this will complicate the implementation until we
>> actually remove the deprecated flags.  Par for the backward compatility
>> course.
>>
>> When block migration isn't off in global state, we can either
>>
>> * let the flags take precedence over the global state (one
>>   interpretation of "do the right thing"), or
>>
>> * reject flags that conflict with global state (another interpretation),
>>   or
>>
>> * reject *all* flags (fail cleanly).
>>
>> The last one looks perfectly servicable to me.
>
> Yeap,  I think that makes sense.  If you use capabilities, parameters,
> old interface don't work at all.
>
> We still have a problem that is what happens if the user does:
>
> migrate -b <foo>
> migrate_cancel (or error)
> migrate <bar> (without -b)
>
> With current patches, it will still use -b.  Fixing it requires still
> anding more code.  But I think that this use case is so weird what we
> should not even care about it.

It's a compatibility break.  Whether it's tolerable is a judgement call,
and not for me to make.

Compatibility breaks need documentation, including release notes.

Say you run migrate with -b by accident (say by recalling a prior
command from persistent command history, such as qmp-shell's or rlwrap's
or socat READLINE's), immediately realize what you've done and cancel
the migration.  Are you then stuck with -b forever?

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:

...

>> As qmp command is asynchronous, you can think that -d is *always* on in
>> QMP O:-)
>
> Yes.  The existence of "detach" in QMP is owed to limitations of early
> QMP infrastructure.  It's flagged as "invalid" and "should not be
> used" since 2010.
>
> Perhaps we should start a section on QMP in
> <http://wiki.qemu.org/Features/LegacyRemoval>.  But I'd like to first
> have a way to communicate "you're using a deprecated feature" warnings
> via QMP.

+1

>> Tristates will complicate it.  I still think that:
>>
>> - capability: block_migration
>> - parameter: block_shared
>>
>> Makes more sense, no?
>>
>> If block_migration is not enabled, we ignore the shared parameter.  We
>> already do that for other parameters.
>
> My impression as a superficial reader is that migration configuration is
> a historically grown mess.  Perhaps we shouldn't try to interpret too
> much intent into it :)
>
> If we redo migration as an instance of the "job" abstraction once we
> have it, then migration configuration & control should become more less
> messy.  Of course, the old messes will stay with us for a while in the
> form of backward compatibility messes.
>
> I'm not too particular on how we do the tri-state now, as long as it
> reasonably fits what we have, and is documented clearly.

>>> If the new interface isn't used, the old one still needs to work.  If it
>>> is used, the old one either has to do "the right thing", or fail
>>> cleanly.
>>>
>>> We approximate "new interface isn't used" by "block migration is off in
>>> global state".  When it is off, the migration command needs to honor its
>>> two flags for compatibility.  It must leave block migration off in
>>> global state.  Yes, this will complicate the implementation until we
>>> actually remove the deprecated flags.  Par for the backward compatility
>>> course.
>>>
>>> When block migration isn't off in global state, we can either
>>>
>>> * let the flags take precedence over the global state (one
>>>   interpretation of "do the right thing"), or
>>>
>>> * reject flags that conflict with global state (another interpretation),
>>>   or
>>>
>>> * reject *all* flags (fail cleanly).
>>>
>>> The last one looks perfectly servicable to me.
>>
>> Yeap,  I think that makes sense.  If you use capabilities, parameters,
>> old interface don't work at all.
>>
>> We still have a problem that is what happens if the user does:
>>
>> migrate -b <foo>
>> migrate_cancel (or error)
>> migrate <bar> (without -b)
>>
>> With current patches, it will still use -b.  Fixing it requires still
>> anding more code.  But I think that this use case is so weird what we
>> should not even care about it.
>
> It's a compatibility break.  Whether it's tolerable is a judgement call,
> and not for me to make.
>
> Compatibility breaks need documentation, including release notes.
>
> Say you run migrate with -b by accident (say by recalling a prior
> command from persistent command history, such as qmp-shell's or rlwrap's
> or socat READLINE's), immediately realize what you've done and cancel
> the migration.  Are you then stuck with -b forever?

migrate_set_capability block off

and you are done.


But I think that adding documentation would be longer that just adding
the code to clean it.

Later, Juan.

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Peter Xu]

On Fri, May 12, 2017 at 12:55:22PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Thu, May 11, 2017 at 06:32:27PM +0200, Juan Quintela wrote:
> 
> >> @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >>      MigrationParams params;
> >>      const char *p;
> >>  
> >> -    params.blk = has_blk && blk;
> >> -    params.shared = has_inc && inc;
> >> -
> >>      if (migration_is_setup_or_active(s->state) ||
> >>          s->state == MIGRATION_STATUS_CANCELLING ||
> >>          s->state == MIGRATION_STATUS_COLO) {
> >> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >>      }
> >>  
> >>      if (has_inc && inc) {
> >> +        migrate_set_block_enabled(s, true);
> >>          migrate_set_block_shared(s, true);
> >
> > [2]
> >
> > IIUC for [1] & [2] we are solving the same problem that "shared"
> > depends on "enabled" bit. Would it be good to unitfy this dependency
> > somewhere? E.g., by changing migrate_set_block_shared() into:
> >
> > void migrate_set_block_shared(MigrationState *s, bool value)
> > {
> >     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
> >     if (value) {
> >         migrate_set_block_enabled(s, true);
> >     }
> > }
> 
> ok with this.
> I will add once here that when we disable block enabled, we also disable
> shared, or just let it that way?

I should mark "nitpick" in my above comment. Any way works for me. :)

> 
> > Another thing to mention: after switching to the capability interface,
> > we'll cache the "enabled" and "shared" bits now while we don't cache
> > it before, right? IIUC it'll affect behavior of such sequence:
> >
> > - 1st migrate with enabled=1, shared=1, then
> > - 2nd migrate with enabled=0, shared=0
> >
> > Before the series, the 2nd migrate will use enabled=shared=0, but
> > after the series it should be using enabled=shared=1. Not sure whether
> > this would be a problem (or I missed anything?).
> 
> We can't be consistent with both old/new way.
> 
> Old way: we always setup the capabilities on command line (that should
> have been deprecated long, long ago)
> 
> New way: Once set, they stay set.
> 
> So, alternatives are:
> - If we are going to deprecate the old way, just let things as they are
>   on this patch (easier for me O:-)
> 
> - Always disable this features at the end of migration: Compatible with
>   old migration semantics, bet we are inconsistent with all other
>   capabilities.
> 
> - Add yet more code to only disable them when we are setting them
>   through the command line.  More code to maintain.
> 
> My idea would be to deprecate the migrate command line parameter, that
> is the reason why I did the 1st option.  I hope that in due curse, we
> would be able to remove the code.  But if anyone strongly think that any
> of the other options is better, please let me know.

I assume that sending continuous "migrate" is very rare, right (after
all, normally source VM is useless after that...)? I was just trying
to post this question up, and so... If you/Dave/others won't worry
about its compatibility, I won't either. ;)

Thanks!

-- 
Peter Xu

[Qemu-devel] [PATCH 3/3] migration: Remove old MigrationParams

[Juan Quintela]

Not used anymore after moving block migration to use capabilities.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/migration/migration.h | 10 ++--------
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h       |  1 -
 include/sysemu/sysemu.h       |  3 +--
 migration/colo.c              |  2 +-
 migration/migration.c         |  8 +++-----
 migration/savevm.c            | 16 +++-------------
 7 files changed, 10 insertions(+), 31 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 2d5525c..65543f6 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -38,10 +38,6 @@
 #define QEMU_VM_COMMAND              0x08
 #define QEMU_VM_SECTION_FOOTER       0x7e
 
-struct MigrationParams {
-    bool unused; /* C doesn't allow empty structs */
-};
-
 /* Messages sent on the return path from destination to source */
 enum mig_rp_message_type {
     MIG_RP_MSG_INVALID = 0,  /* Must be 0 */
@@ -131,12 +127,10 @@ struct MigrationState
     QEMUBH *cleanup_bh;
     QEMUFile *to_dst_file;
 
-    /* New style params from 'migrate-set-parameters' */
+    /* params from 'migrate-set-parameters' */
     MigrationParameters parameters;
 
     int state;
-    /* Old style params from 'migrate' command */
-    MigrationParams params;
 
     /* State related to return path */
     struct {
@@ -226,7 +220,7 @@ void migrate_fd_connect(MigrationState *s);
 
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
-MigrationState *migrate_init(const MigrationParams *params);
+MigrationState *migrate_init(void);
 bool migration_is_blocked(Error **errp);
 bool migration_in_setup(MigrationState *);
 bool migration_is_idle(void);
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a5bf9c..827fb4c 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -37,7 +37,6 @@ typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
 
 typedef struct SaveVMHandlers {
     /* This runs inside the iothread lock.  */
-    void (*set_params)(const MigrationParams *params, void * opaque);
     SaveStateHandler *save_state;
 
     void (*cleanup)(void *opaque);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index f08d327..a388243 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -49,7 +49,6 @@ typedef struct MemoryRegion MemoryRegion;
 typedef struct MemoryRegionCache MemoryRegionCache;
 typedef struct MemoryRegionSection MemoryRegionSection;
 typedef struct MigrationIncomingState MigrationIncomingState;
-typedef struct MigrationParams MigrationParams;
 typedef struct MigrationState MigrationState;
 typedef struct Monitor Monitor;
 typedef struct MonitorDef MonitorDef;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 16f740f..83ecd13 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -102,8 +102,7 @@ enum qemu_vm_cmd {
 #define MAX_VM_CMD_PACKAGED_SIZE (1ul << 24)
 
 bool qemu_savevm_state_blocked(Error **errp);
-void qemu_savevm_state_begin(QEMUFile *f,
-                             const MigrationParams *params);
+void qemu_savevm_state_begin(QEMUFile *f);
 void qemu_savevm_state_header(QEMUFile *f);
 int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
 void qemu_savevm_state_cleanup(void);
diff --git a/migration/colo.c b/migration/colo.c
index e772384..430bae5 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -349,7 +349,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     migrate_set_block_enabled(s, false);
     migrate_set_block_shared(s, false);
     qemu_savevm_state_header(fb);
-    qemu_savevm_state_begin(fb, &s->params);
+    qemu_savevm_state_begin(fb);
     qemu_mutex_lock_iothread();
     qemu_savevm_state_complete_precopy(fb, false);
     qemu_mutex_unlock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index 8a3bf89..b373d19 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1102,7 +1102,7 @@ bool migration_is_idle(void)
     return false;
 }
 
-MigrationState *migrate_init(const MigrationParams *params)
+MigrationState *migrate_init(void)
 {
     MigrationState *s = migrate_get_current();
 
@@ -1116,7 +1116,6 @@ MigrationState *migrate_init(const MigrationParams *params)
     s->cleanup_bh = 0;
     s->to_dst_file = NULL;
     s->state = MIGRATION_STATUS_NONE;
-    s->params = *params;
     s->rp_state.from_dst_file = NULL;
     s->rp_state.error = false;
     s->mbps = 0.0;
@@ -1215,7 +1214,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    MigrationParams params;
     const char *p;
 
     if (migration_is_setup_or_active(s->state) ||
@@ -1233,7 +1231,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         return;
     }
 
-    s = migrate_init(&params);
+    s = migrate_init();
 
     if (has_blk && blk) {
         migrate_set_block_enabled(s, true);
@@ -1966,7 +1964,7 @@ static void *migration_thread(void *opaque)
         qemu_savevm_send_postcopy_advise(s->to_dst_file);
     }
 
-    qemu_savevm_state_begin(s->to_dst_file, &s->params);
+    qemu_savevm_state_begin(s->to_dst_file);
 
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
     migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
diff --git a/migration/savevm.c b/migration/savevm.c
index 221fb4b..26ab3bb 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -966,21 +966,13 @@ void qemu_savevm_state_header(QEMUFile *f)
 
 }
 
-void qemu_savevm_state_begin(QEMUFile *f,
-                             const MigrationParams *params)
+void qemu_savevm_state_begin(QEMUFile *f)
 {
     SaveStateEntry *se;
     int ret;
 
     trace_savevm_state_begin();
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
-        if (!se->ops || !se->ops->set_params) {
-            continue;
-        }
-        se->ops->set_params(params, se->opaque);
-    }
-
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (!se->ops || !se->ops->save_live_setup) {
             continue;
         }
@@ -1232,9 +1224,7 @@ void qemu_savevm_state_cleanup(void)
 static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
-    MigrationParams params = {
-    };
-    MigrationState *ms = migrate_init(&params);
+    MigrationState *ms = migrate_init();
     MigrationStatus status;
     ms->to_dst_file = f;
 
@@ -1245,7 +1235,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 
     qemu_mutex_unlock_iothread();
     qemu_savevm_state_header(f);
-    qemu_savevm_state_begin(f, &params);
+    qemu_savevm_state_begin(f);
     qemu_mutex_lock_iothread();
 
     while (qemu_file_get_error(f) == 0) {
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 0/3] Remove old MigrationParams

[Hailiang Zhang]

On 2017/5/12 0:32, Juan Quintela wrote:
> Hi
>
> Changes from v1:
>
> - make migrate_block_set_* take a boolean
> - disable block migration in colo to maintain semantics.
>
> Please review, Juan.
>
> [v1]
> Upon a time there were MigrationParms (only used for block migration)
> and then MigrationParams used for everything else.  This series:
>
> - create migration capabilities for block parameters
> - make the migrate command line parameters to use capabilities
> - remove MigrationParams completely
>
> Please, review.
>

Looks good to me, this makes codes more grace.

Reviewed-by:zhanghailiang <zhang.zhanghailiang@huawei.com>


> *** BLURB HERE ***
>
> Juan Quintela (3):
>    migration: Create block capabilities for shared and enable
>    migration: Remove use of old MigrationParams
>    migration: Remove old MigrationParams
>
>   include/migration/block.h     |  3 +++
>   include/migration/migration.h | 14 +++++-------
>   include/migration/vmstate.h   |  1 -
>   include/qemu/typedefs.h       |  1 -
>   include/sysemu/sysemu.h       |  3 +--
>   migration/block.c             | 17 ++------------
>   migration/colo.c              |  7 +++---
>   migration/migration.c         | 52 ++++++++++++++++++++++++++++++++++++-------
>   migration/savevm.c            | 18 +++------------
>   qapi-schema.json              |  7 +++++-
>   10 files changed, 68 insertions(+), 55 deletions(-)
>

[Qemu-devel] [PATCH 0/3] Remove old MigrationParams

[Juan Quintela]

Hi

Upon a time there were MigrationParms (only used for block migration)
and then MigrationParams used for everything else.  This series:

- create migration capabilities for block parameters
- make the migrate command line parameters to use capabilities
- remove MigrationParams completely

Please, review.


Juan Quintela (3):
  migration: Create block capabilities for shared and enable
  migration: Remove use of old MigrationParams
  migration: Remove old MigrationParams

 include/migration/migration.h | 14 +++++-------
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h       |  1 -
 include/sysemu/sysemu.h       |  3 +--
 migration/block.c             | 17 ++------------
 migration/colo.c              |  5 +----
 migration/migration.c         | 52 ++++++++++++++++++++++++++++++++++++-------
 migration/savevm.c            | 18 +++------------
 qapi-schema.json              |  7 +++++-
 9 files changed, 62 insertions(+), 56 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Those two capabilities were added through the command line.  Notice that
we just created them.  This is just the boilerplate.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/migration/migration.h |  3 +++
 migration/migration.c         | 36 ++++++++++++++++++++++++++++++++++++
 qapi-schema.json              |  7 ++++++-
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index dfeca38..618ab0e 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -307,6 +307,9 @@ bool migrate_colo_enabled(void);
 
 int64_t xbzrle_cache_resize(int64_t new_size);
 
+bool migrate_use_block_enabled(void);
+bool migrate_use_block_shared(void);
+
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
diff --git a/migration/migration.c b/migration/migration.c
index 5447cab..775b24c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1195,6 +1195,16 @@ bool migration_is_blocked(Error **errp)
     return false;
 }
 
+static void migrate_set_block_shared(MigrationState *s)
+{
+    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = true;
+}
+
+static void migrate_set_block_enabled(MigrationState *s)
+{
+    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
+}
+
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  Error **errp)
@@ -1224,6 +1234,14 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 
     s = migrate_init(&params);
 
+    if (has_blk && blk) {
+        migrate_set_block_enabled(s);
+    }
+
+    if (has_inc && inc) {
+        migrate_set_block_shared(s);
+    }
+
     if (strstart(uri, "tcp:", &p)) {
         tcp_start_outgoing_migration(s, p, &local_err);
 #ifdef CONFIG_RDMA
@@ -1419,6 +1437,24 @@ int64_t migrate_xbzrle_cache_size(void)
     return s->xbzrle_cache_size;
 }
 
+bool migrate_use_block_enabled(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED];
+}
+
+bool migrate_use_block_shared(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED];
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/qapi-schema.json b/qapi-schema.json
index 01b087f..e963bb3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -894,11 +894,16 @@
 # @release-ram: if enabled, qemu will free the migrated ram pages on the source
 #        during postcopy-ram migration. (since 2.9)
 #
+# @block-enabled: enable block migration (Since 2.10)
+#
+# @block-shared: enable block shared migration (Since 2.10)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
+           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
+           'block-enabled', 'block-shared' ] }
 
 ##
 # @MigrationCapabilityStatus:
-- 
2.9.3