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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Mon,  1 Apr 2019 11:08:24 +0200
> > Markus Armbruster <armbru@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 <armbru@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 <dgilbert@redhat.com>

> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> Thanks!
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK