[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 3/3] dump-guest-memory: Block live migration
From: |
Peter Xu |
Subject: |
Re: [PATCH v2 3/3] dump-guest-memory: Block live migration |
Date: |
Wed, 22 Sep 2021 12:00:53 -0400 |
On Wed, Sep 22, 2021 at 07:18:15PM +0400, Marc-André Lureau wrote:
> Hi
Hi, Marc-André,
>
> On Thu, Aug 26, 2021 at 11:01 PM Peter Xu <peterx@redhat.com> wrote:
>
> > Both dump-guest-memory and live migration caches vm state at the beginning.
> > Either of them entering the other one will cause race on the vm state, and
> > even
> > more severe on that (please refer to the crash report in the bug link).
> >
> > Let's block live migration in dump-guest-memory, and that'll also block
> > dump-guest-memory if it detected that we're during a live migration.
> >
> > Side note: migrate_del_blocker() can be called even if the blocker is not
> > inserted yet, so it's safe to unconditionally delete that blocker in
> > dump_cleanup (g_slist_remove allows no-entry-found case).
> >
> > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1996609
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > dump/dump.c | 24 +++++++++++++++++++-----
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/dump/dump.c b/dump/dump.c
> > index ab625909f3..9c1c1fb738 100644
> > --- a/dump/dump.c
> > +++ b/dump/dump.c
> > @@ -29,6 +29,7 @@
> > #include "qemu/error-report.h"
> > #include "qemu/main-loop.h"
> > #include "hw/misc/vmcoreinfo.h"
> > +#include "migration/blocker.h"
> >
> > #ifdef TARGET_X86_64
> > #include "win_dump.h"
> > @@ -47,6 +48,8 @@
> >
> > #define MAX_GUEST_NOTE_SIZE (1 << 20) /* 1MB should be enough */
> >
> > +static Error *dump_migration_blocker;
> > +
> > #define ELF_NOTE_SIZE(hdr_size, name_size, desc_size) \
> > ((DIV_ROUND_UP((hdr_size), 4) + \
> > DIV_ROUND_UP((name_size), 4) + \
> > @@ -101,6 +104,7 @@ static int dump_cleanup(DumpState *s)
> > qemu_mutex_unlock_iothread();
> > }
> > }
> > + migrate_del_blocker(dump_migration_blocker);
> >
> > return 0;
> > }
> > @@ -1927,11 +1931,6 @@ void qmp_dump_guest_memory(bool paging, const char
> > *file,
> > Error *local_err = NULL;
> > bool detach_p = false;
> >
> > - if (runstate_check(RUN_STATE_INMIGRATE)) {
> >
>
> This INMIGRATE check,
>
> - error_setg(errp, "Dump not allowed during incoming migration.");
> > - return;
> > - }
> > -
> > /* if there is a dump in background, we should wait until the dump
> > * finished */
> > if (dump_in_progress()) {
> > @@ -2005,6 +2004,21 @@ void qmp_dump_guest_memory(bool paging, const char
> > *file,
> > return;
> > }
> >
> > + if (!dump_migration_blocker) {
> > + error_setg(&dump_migration_blocker,
> > + "Live migration disabled: dump-guest-memory in
> > progress");
> > + }
> > +
> > + /*
> > + * Allows even for -only-migratable, but forbid migration during the
> > + * process of dump guest memory.
> > + */
> > + if (migrate_add_blocker_internal(dump_migration_blocker, errp)) {
> >
>
> is now handled here with migration_is_idle() ?
>
> I am not familiar enough with the run & migration states intricacies here
Hmm, I thought it covers both src/dst, but after I double checked it seems
not..
On the destination side, we seem to have used MigrationState somewhere like in
migration_channel_process_incoming for reading parameters and capabilities, but
OTOH we used MigrationIncomingState for most of the rest operations, e.g., to
maintain dst migration state changes. Then migration_is_idle() won't work on
dst indeed..
I think we should rename migration_is_idle() to migration_src_is_idle() at some
point.
For now, I'll respin and just keep the RUN_STATE_INMIGRATE check above.
Thanks!
--
Peter Xu