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

[Markus Armbruster]

Juan Quintela <address@hidden> writes:

> Eric Blake <address@hidden> 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.