[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState" |
Date: |
Tue, 2 Apr 2019 13:03:51 +0100 |
User-agent: |
Mutt/1.11.3 (2019-02-01) |
* Markus Armbruster (address@hidden) wrote:
> 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.
Yes, that's fine:
Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> > Reviewed-by: Igor Mammedov <address@hidden>
>
> Thanks!
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
- Re: [Qemu-devel] [PATCH v2 4/5] vl: Document dependencies hiding in global and compat props, (continued)
Re: [Qemu-devel] [PATCH v2 2/5] Revert "migration: move only_migratable to MigrationState", Igor Mammedov, 2019/04/02
[Qemu-devel] [PATCH v2 5/5] accel: Unbreak accelerator fallback, Markus Armbruster, 2019/04/01