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

[Markus Armbruster]

Juan Quintela <address@hidden> writes:

> Markus Armbruster <address@hidden> wrote:
>> Eric Blake <address@hidden> writes:
>>
>>> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>>>> * Juan Quintela (address@hidden) wrote:
>>>>> Eric Blake <address@hidden> 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 <address@hidden>
>>>>>>> Reviewed-by: Eric Blake <address@hidden>
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> 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.