Markus,
On Wed, Aug 25, 2021 at 09:54:12AM +0200, Markus Armbruster wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > Both dump-guest-memory and live migration have vm state cached internally.
> > Allowing them to happen together means the vm state can be messed up.
> > Simply
> > block live migration for dump-guest-memory.
> >
> > One trivial thing to mention is we should still allow dump-guest-memory
> > even if
> > -only-migratable is specified, because that flag should majorly be used to
> > guarantee not adding devices that will block migration by accident. Dump
> > guest
> > memory is not like that - it'll only block for the seconds when it's
> > dumping.
>
> I recently ran into a similarly unusual use of migration blockers:
>
> Subject: -only-migrate and the two different uses of migration blockers
> (was: spapr_events: Sure we may ignore migrate_add_blocker() failure?)
> Date: Mon, 19 Jul 2021 13:00:20 +0200 (5 weeks, 1 day, 20 hours ago)
> Message-ID: <87sg0amuuz.fsf...@dusky.pond.sub.org>
>
> We appear to use migration blockers in two ways:
>
> (1) Prevent migration for an indefinite time, typically due to use of
> some feature that isn't compatible with migration.
>
> (2) Delay migration for a short time.
>
> Option -only-migrate is designed for (1). It interferes with (2).
>
> Example for (1): device "x-pci-proxy-dev" doesn't support migration. It
> adds a migration blocker on realize, and deletes it on unrealize. With
> -only-migrate, device realize fails. Works as designed.
>
> Example for (2): spapr_mce_req_event() makes an effort to prevent
> migration degrate the reporting of FWNMIs. It adds a migration blocker
> when it receives one, and deletes it when it's done handling it. This
> is a best effort; if migration is already in progress by the time FWNMI
> is received, we simply carry on, and that's okay. However, option
> -only-migrate sabotages the best effort entirely.
>
> While this isn't exactly terrible, it may be a weakness in our thinking
> and our infrastructure. I'm bringing it up so the people in charge are
> aware :)
>
> https://lists.nongnu.org/archive/html/qemu-devel/2021-07/msg04723.html
>
> Downthread there, Dave Gilbert opined
>
> It almost feels like they need a way to temporarily hold off
> 'completion' of migratio - i.e. the phase where we stop the CPU and
> write the device data; mind you you'd also probably want it to stop
> cold-migrates/snapshots?
Yeah, maybe spapr_mce_req_event() can be another candidate of the internal
version of migration_add_blocker().
I can add a patch to replace it if anyone likes me to.
Both cold and live snapshot should have checked migration blockers, I think.
E.g., cold snapshot has:
bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
bool has_devices, strList *devices, Error **errp)
{
[...]
if (migration_is_blocked(errp)) {
return false;
}
[...]
}
While the live snapshot shares similar code in migrate_prepare().
So looks safe that nothing wrong should happen within add/del pair of blockers.
However I do see that it's possible we'll allow the add_blocker to succeed even
if during cold snapshot, because migration_is_idle() in migration_add_blocker()
only checks migration state, not RUN_STATE_SAVE_VM. So I'm wondering whether
we'd like one more patch to cover that too, like:
---8<---
diff --git a/migration/migration.c b/migration/migration.c
index 41429680c2..9c602a4ac1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2055,15 +2055,16 @@ void migrate_init(MigrationState *s)
int migrate_add_blocker_internal(Error *reason, Error **errp)
{
- if (migration_is_idle()) {
- migration_blockers = g_slist_prepend(migration_blockers, reason);
- return 0;
+ /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
+ if (runstate_check(RUN_STATE_SAVE_VM) || !migration_is_idle()) {
+ error_propagate_prepend(errp, error_copy(reason),
+ "disallowing migration blocker "
+ "(migration in progress) for: ");
+ return -EBUSY;
}
- error_propagate_prepend(errp, error_copy(reason),
- "disallowing migration blocker "
- "(migration in progress) for: ");
- return -EBUSY;
+ migration_blockers = g_slist_prepend(migration_blockers, reason);
+ return 0;
}
int migrate_add_blocker(Error *reason, Error **errp)
---8<---
It'll by accident also cover guest dump which also sets RUN_STATE_SAVE_VM, but
I think that's ok.
Thanks,
--
Peter Xu
