Igor Mammedov <imamm...@redhat.com> writes:
> On Mon, 1 Apr 2019 11:08:24 +0200
> Markus Armbruster <arm...@redhat.com> 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 <arm...@redhat.com>
>
> 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 <imamm...@redhat.com>
Thanks!
