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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

[-- 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 --]