[PATCH v2 0/3] dump-guest-memory: Add blocker for migration

[PATCH v2 0/3] dump-guest-memory: Add blocker for migration

[Peter Xu]

v2:
- One more patch: "migration: Make migration blocker work for snapshots too"
- Move register of migration blocker to be before dump_init [Marc-Andre]
- Collected r-bs

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.

Thanks,

Peter Xu (3):
  migration: Make migration blocker work for snapshots too
  migration: Add migrate_add_blocker_internal()
  dump-guest-memory: Block live migration

 dump/dump.c                 | 24 +++++++++++++++++++-----
 include/migration/blocker.h | 16 ++++++++++++++++
 migration/migration.c       | 24 +++++++++++++++---------
 3 files changed, 50 insertions(+), 14 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] migration: Make migration blocker work for snapshots too

[Peter Xu]

save_snapshot() checks migration blocker, which looks sane.  At the meantime we
should also teach the blocker add helper to fail if during a snapshot, just
like for migrations.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 041b8451a6..f476e2101e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2062,15 +2062,16 @@ int migrate_add_blocker(Error *reason, Error **errp)
         return -EACCES;
     }
 
-    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/snapshot 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;
 }
 
 void migrate_del_blocker(Error *reason)
-- 
2.31.1

[PATCH v2 2/3] migration: Add migrate_add_blocker_internal()

[Peter Xu]

An internal version that removes -only-migratable implications.  It can be used
for temporary migration blockers like dump-guest-memory.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/blocker.h | 16 ++++++++++++++++
 migration/migration.c       | 21 +++++++++++++--------
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/include/migration/blocker.h b/include/migration/blocker.h
index acd27018e9..9cebe2ba06 100644
--- a/include/migration/blocker.h
+++ b/include/migration/blocker.h
@@ -25,6 +25,22 @@
  */
 int migrate_add_blocker(Error *reason, Error **errp);
 
+/**
+ * @migrate_add_blocker_internal - prevent migration from proceeding without
+ *                                 only-migrate implications
+ *
+ * @reason - an error to be returned whenever migration is attempted
+ *
+ * @errp - [out] The reason (if any) we cannot block migration right now.
+ *
+ * @returns - 0 on success, -EBUSY on failure, with errp set.
+ *
+ * Some of the migration blockers can be temporary (e.g., for a few seconds),
+ * so it shouldn't need to conflict with "-only-migratable".  For those cases,
+ * we can call this function rather than @migrate_add_blocker().
+ */
+int migrate_add_blocker_internal(Error *reason, Error **errp);
+
 /**
  * @migrate_del_blocker - remove a blocking error from migration
  *
diff --git a/migration/migration.c b/migration/migration.c
index f476e2101e..2aaf2fd449 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2053,15 +2053,8 @@ void migrate_init(MigrationState *s)
     s->threshold_size = 0;
 }
 
-int migrate_add_blocker(Error *reason, Error **errp)
+int migrate_add_blocker_internal(Error *reason, Error **errp)
 {
-    if (only_migratable) {
-        error_propagate_prepend(errp, error_copy(reason),
-                                "disallowing migration blocker "
-                                "(--only-migratable) for: ");
-        return -EACCES;
-    }
-
     /* 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),
@@ -2074,6 +2067,18 @@ int migrate_add_blocker(Error *reason, Error **errp)
     return 0;
 }
 
+int migrate_add_blocker(Error *reason, Error **errp)
+{
+    if (only_migratable) {
+        error_propagate_prepend(errp, error_copy(reason),
+                                "disallowing migration blocker "
+                                "(--only-migratable) for: ");
+        return -EACCES;
+    }
+
+    return migrate_add_blocker_internal(reason, errp);
+}
+
 void migrate_del_blocker(Error *reason)
 {
     migration_blockers = g_slist_remove(migration_blockers, reason);
-- 
2.31.1

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

[Peter Xu]

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)) {
-        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)) {
+        /* Remember to release the fd before passing it over to dump state */
+        close(fd);
+        return;
+    }
+
     s = &dump_state_global;
     dump_state_prepare(s);
 
-- 
2.31.1

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3092 bytes --]

Hi

On Thu, Aug 26, 2021 at 10:58 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)) {
> -        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)) {
> +        /* Remember to release the fd before passing it over to dump
> state */
> +        close(fd);
> +        return;
> +    }
>

I would move it earlier.  Why not leave it at the beginning of the function
as it was?

Then dump_in_progress() is probably redundant too.

+
>      s = &dump_state_global;
>      dump_state_prepare(s);
>
> --
> 2.31.1
>
>

[-- Attachment #2: Type: text/html, Size: 4318 bytes --]

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3679 bytes --]

Hi

On Fri, Aug 27, 2021 at 11:44 AM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

> Hi
>
> On Thu, Aug 26, 2021 at 10:58 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)) {
>> -        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)) {
>> +        /* Remember to release the fd before passing it over to dump
>> state */
>> +        close(fd);
>> +        return;
>> +    }
>>
>
> I would move it earlier.  Why not leave it at the beginning of the
> function as it was?
>
>
Ah I think it's because dump_cleanup() isn't called when returning earlier.
But relying on a cleanup done outside of this function is not obvious.
Either dump_cleanup() should be called from here, or the blocker code
should be moved to dump_init() imho.



> Then dump_in_progress() is probably redundant too.
>




> +
>>      s = &dump_state_global;
>>      dump_state_prepare(s);
>>
>> --
>> 2.31.1
>>
>>

[-- Attachment #2: Type: text/html, Size: 5499 bytes --]

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

[Peter Xu]

On Fri, Aug 27, 2021 at 11:51:04AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Aug 27, 2021 at 11:44 AM Marc-André Lureau <
> marcandre.lureau@redhat.com> wrote:
> 
> > Hi
> >
> > On Thu, Aug 26, 2021 at 10:58 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)) {
> >> -        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)) {
> >> +        /* Remember to release the fd before passing it over to dump
> >> state */
> >> +        close(fd);
> >> +        return;
> >> +    }
> >>
> >
> > I would move it earlier.  Why not leave it at the beginning of the
> > function as it was?
> >
> >
> Ah I think it's because dump_cleanup() isn't called when returning earlier.
> But relying on a cleanup done outside of this function is not obvious.
> Either dump_cleanup() should be called from here, or the blocker code
> should be moved to dump_init() imho.

Note that dump_cleanup() cleans s->fd, however fd has not delievered to s->fd
now.

I can move it into dump_init() but I need to put it after setting s->fd to make
dump_cleanup work.  That could look a bit weird (as checking migration blocker
shouldn't really be bound to initialize of dump states).

There's another way to move add blocker to the entry of dump_in_progress()
check, however then all the checks between dump_in_progress() and before
opening the fd will not be able to "return" directly but we'll need to del the
blocker if any of those check fails.

That's how I come up with the current patch, which looks the easiest change and
still looks clean.

Besides, I think dump_in_progress() can be removed too with patch 1 checks
SAVEVM state too, however that's a side effect of migration_is_idle() and
dump_in_progres is doing the check in different ways by looking up dump state,
so I kept it.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 0/3] dump-guest-memory: Add blocker for migration

[Peter Xu]

On Thu, Aug 26, 2021 at 02:58:10PM -0400, Peter Xu wrote:
> v2:
> - One more patch: "migration: Make migration blocker work for snapshots too"
> - Move register of migration blocker to be before dump_init [Marc-Andre]
> - Collected r-bs
> 
> 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.

Ping?

-- 
Peter Xu

Re: [PATCH v2 1/3] migration: Make migration blocker work for snapshots too

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]

On Thu, Aug 26, 2021 at 11:00 PM Peter Xu <peterx@redhat.com> wrote:

> save_snapshot() checks migration blocker, which looks sane.  At the
> meantime we
> should also teach the blocker add helper to fail if during a snapshot, just
> like for migrations.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  migration/migration.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 041b8451a6..f476e2101e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2062,15 +2062,16 @@ int migrate_add_blocker(Error *reason, Error
> **errp)
>          return -EACCES;
>      }
>
> -    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/snapshot 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;
>  }
>
>  void migrate_del_blocker(Error *reason)
> --
> 2.31.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2606 bytes --]

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]

Hi

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

+        /* Remember to release the fd before passing it over to dump state
> */
> +        close(fd);
> +        return;
> +    }
> +
>      s = &dump_state_global;
>      dump_state_prepare(s);
>
> --
> 2.31.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4600 bytes --]

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

[Peter Xu]

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