Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState"

[Markus Armbruster]

Igor Mammedov <address@hidden> writes:

> On Mon,  1 Apr 2019 11:08:24 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> This reverts commit 3df663e575f1876d7f3bc684f80e72fca0703d39.
>> This reverts commit b605c47b57b58e61a901a50a0762dccf43d94783.
>> 
>> Command line option --only-migratable is for disallowing any
>> configuration that can block migration.
>> 
>> Initially, --only-migratable set global variable @only_migratable.
>> 
>> Commit 3df663e575 "migration: move only_migratable to MigrationState"
>> replaced it by MigrationState member @only_migratable.  That was a
>> mistake.
>> 
>> First, it doesn't make sense on the design level.  MigrationState
>> captures the state of an individual migration, but --only-migratable
>> isn't a property of an individual migration, it's a restriction on
>> QEMU configuration.  With fault tolerance, we could have several
>> migrations at once.  --only-migratable would certainly protect all of
>> them.  Storing it in MigrationState feels inappropriate.
>> 
>> Second, it contributes to a dependency cycle that manifests itself as
>> a bug now.
>> 
>> Putting @only_migratable into MigrationState means its available only
>> after migration_object_init().
>> 
>> We can't set it before migration_object_init(), so we delay setting it
>> with a global property (this is fixup commit b605c47b57 "migration:
>> fix handling for --only-migratable").
>> 
>> We can't get it before migration_object_init(), so anything that uses
>> it can only run afterwards.
>> 
>> Since migrate_add_blocker() needs to obey --only-migratable, any code
>> adding migration blockers can run only afterwards.  This contributes
>> to the following dependency cycle:
>> 
>> * configure_blockdev() must run before machine_set_property()
>>   so machine properties can refer to block backends
>> 
>> * machine_set_property() before configure_accelerator()
>>   so machine properties like kvm-irqchip get applied
>> 
>> * configure_accelerator() before migration_object_init()
>>   so that Xen's accelerator compat properties get applied.
>> 
>> * migration_object_init() before configure_blockdev()
>>   so configure_blockdev() can add migration blockers
>> 
>> The cycle was closed when recent commit cda4aa9a5a0 "Create block
>> backends before setting machine properties" added the first
>> dependency, and satisfied it by violating the last one.  Broke block
>> backends that add migration blockers.
>> 
>> Moving @only_migratable into MigrationState was a mistake.  Revert it.
>> 
>> This doesn't quite break the "migration_object_init() before
>> configure_blockdev() dependency, since migrate_add_blocker() still has
>> another dependency on migration_object_init().  To be addressed the
>> next commit
>> 
>> Conflicts:
>>      include/migration/misc.h
>>      migration/migration.c
>>      migration/migration.h
>>      vl.c
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>
> with commit message amended like mentioned by David
> (wrt removed "only-migratable" property)

Inserting right before the "Conflicts:" line:

    Note that the reverted commit made -only-migratable sugar for -global
    migration.only-migratable=on below the hood.  Documentation has only
    ever mentioned -only-migratable.  This commit removes the arcane &
    undocumented alternative to -only-migratable again.  Nobody should be
    using it.

> Reviewed-by: Igor Mammedov <address@hidden>

Thanks!