qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability
Date: Tue, 16 May 2017 18:00:10 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)

Juan Quintela <address@hidden> writes:

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

Sounds like a job for &error_abort to me.

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

# @block: If enabled, QEMU will also migrate the contents of all block
#          devices.  Default is disabled.  A possible alternative are
#          mirror jobs to a builtin NBD server on the destination, which
#          are more flexible.
#          (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' ] }
>>>  
>>>  ##
>>>  # @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.

Well, to suggest something, I'd first have to figure out WTF incremental
block migration does.  Your text helps me some, but not enough.  What
exactly is being migrated, and what exactly is assumed to be shared
between source and destination?

Block migration is scandalously underdocumented.

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

Grown, not designed.  It happens.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]