Re: [PATCH v2 3/3] dump-guest-memory: Block live migration

[Peter Xu <peterx@redhat.com>]

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