Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Juan Quintela]

Markus Armbruster <address@hidden> wrote:
> Juan Quintela <address@hidden> writes:
>
>> Create one capability for block migration and one parameter for
>> incremental block migration.
>>
>> Signed-off-by: Juan Quintela <address@hidden>

>>  
>>      /* The last error that occurred */
>>      Error *error;
>> +    /* Do we have to clean up -b/-i from old migrate paramteres */
>
> parameters

ok.


>> +void migrate_set_block_enabled(bool value, Error **errp)
>> +{
>> +    MigrationCapabilityStatusList *cap;
>> +
>> +    cap = g_malloc0(sizeof(*cap));
>> +    cap->value = g_malloc(sizeof(*cap->value));
>
> I prefer g_new() for extra type checking.  Your choice.

ok.


>> +    cap->value->capability = MIGRATION_CAPABILITY_BLOCK;
>> +    cap->value->state = value;
>> +    qmp_migrate_set_capabilities(cap, errp);
>> +    qapi_free_MigrationCapabilityStatusList(cap);
>> +}
>
> The objective is to set one bit.  The means is building a
> MigrationCapabilityStatusList.  When you have to build an elaborate data
> structure just so you can set one bit, your interfaces are likely
> deficient.  Observation, not change request.

This was Dave suggestion.  The reason for doing this is in the following
patch when we enable compile disable of block migration to put the ifdef
in a single place.  Otherwise, I have to put it in two places.

>> +static void migrate_set_block_incremental(MigrationState *s, bool value)
>> +{
>> +    s->parameters.block_incremental = value;
>> +}
>
> This is how setting one bit should look like.

See previous comment.

>
>> +
>> +static void block_cleanup_parameters(MigrationState *s)
>> +{
>> +    if (s->must_remove_block) {
>> +        migrate_set_block_enabled(false, NULL);
>
> NULL ignores errors.  If an error happens, and we ignore it, we're
> screwed, aren't we?  I suspect we want &error_abort here.

setting to false can't never fail.  It is when we set it to true that it
can fail because it is compiled out (that will be in next patch).


>> @@ -1229,6 +1267,38 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
>> blk,
>>          return;
>>      }
>>  
>> +    if (has_blk && blk) {
>> +        if (migrate_use_block() || migrate_use_block_incremental()) {
>> +            error_setg(errp, "You can't use block capability and -b/i");
>
> Error message assumes HMP.  In QMP, the thing is called 'blk', not -b.
> Perhaps we can sidestep the issue like this: "Command options are
> incompatible with current migration capabilities".

ok.

>
>> +            return;
>> +        }
>> +        migrate_set_block_enabled(true, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +        block_enabled = true;
>> +        s->must_remove_block = true;
>> +    }
>> +
>> +    if (has_inc && inc) {
>> +        if ((migrate_use_block() && !block_enabled)
>> +            || migrate_use_block_incremental()) {
>> +            error_setg(errp, "You can't use block capability and -b/i");
>
> Likewise.
>
> The condition would be slightly simpler if you completed error checking
> before changing global state.  Matter of taste.

Being bug compatible has this drawbacks.  I also hate that -i implies -b.

>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -894,11 +894,14 @@
>>  # @release-ram: if enabled, qemu will free the migrated ram pages on the 
>> source
>>  #        during postcopy-ram migration. (since 2.9)
>>  #
>> +# @block: enable block migration (Since 2.10)
>> +#
>
> What's "block migration" and why should I care?

"enable migration of block devices.  The granularity is all devices or
no devices, nothing in the middle."

I will be happy with suggestions.

>
>>  # 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' ] }
>>  
>>  ##
>>  # @MigrationCapabilityStatus:
>> @@ -1009,13 +1012,15 @@
>>  # @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints 
>> in
>>  #          periodic mode. (Since 2.8)
>>  #
>> +# @block-incremental: enable block incremental migration (Since 2.10)
>> +#
>
> What's "block incremental migration" and why should I care?

This is good, I will try.

"block incremental migration assumes that we have a base image in both
sides, and then we continue writting in one of the sides.  This way we
need to only migrate the changes since the previous state where it was
the same in both sides".

I am not sure what to put there, really.


>
>>  # Since: 2.4
>>  ##
>>  { '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' ] }
>> +           'downtime-limit', 'x-checkpoint-delay', 'block-incremental' ] }
>>  
>>  ##
>>  # @migrate-set-parameters:
>> @@ -1082,6 +1087,8 @@
>>  #
>>  # @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 
>> 2.8)
>>  #
>> +# @block-incremental: enable block incremental migration (Since 2.10)
>> +#
>>  # Since: 2.4
>>  ##
>>  { 'struct': 'MigrationParameters',
>> @@ -1094,7 +1101,8 @@
>>              '*tls-hostname': 'str',
>>              '*max-bandwidth': 'int',
>>              '*downtime-limit': 'int',
>> -            '*x-checkpoint-delay': 'int'} }
>> +            '*x-checkpoint-delay': 'int',
>> +            '*block-incremental': 'bool' } }
>>  
>>  ##
>>  # @query-migrate-parameters:
>
> Can't say I like the split between MigrationCapability and
> MigrationParameters, but we got to work with what we have.

:-(

And it is still not good enough.  There are parameters that make sense
to change in middle migration (max-bandwidth for example) and others
that you shouldn't, like tls-hostname or compression-threads.

Go figure.

Thanks, Juan.