* Markus Armbruster (arm...@redhat.com) wrote: > 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.
Yes, that's fine: Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > > Reviewed-by: Igor Mammedov <imamm...@redhat.com> > > Thanks! -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
