[Qemu-devel] [RFC PATCH 0/2] Increase usability of external snapshots

[Qemu-devel] [RFC PATCH 0/2] Increase usability of external snapshots

[Richard Palethorpe]

Hello,

Following on from the discussion about creating savevm/loadvm QMP
equivalents. I decided to take the advice given that we should use external
snapshots. However reverting to a snapshot currently requires QEMU to be
restarted with "-incoming". Restarting QEMU requires a fair bit of book
keeping to be done by the management application and may take measurably more
time.

Also to quote the previous discussion:
>>> > Then you could just use the regular migrate QMP commands for loading
>>> > and saving snapshots.  Might need a little extra work on the incoming
>>> > side, since we need to be able to load snapshots, despite QEMU not
>>> > being started with '-incoming defer', but might still be doable ?
>>> > This would theoretically give us progress monitoring, cancellation,
>>> > etc for free.
>>> 
>>> What actually stops this working other than the sanity check in
>>> migrate_incoming ?
>>
>> No idea really - not looked closely at the code implications.
>
> It would be a plus for migration code, right now there are _two_
> implementations, and savevm/loadvm one gets less love.
>
> And we will check "much more" the way to load migration in a
> non-pristine qemu, so ....
>
> Later, Juan.

This patch just changes a few lines which shuts down the currently running VM
and puts the QEMU instance into a state where it can accept an incoming
migration. I haven't tested it yet with more complex scenarios, but it
appears to work fine so far.

I have also started work on a new migration test, which is almost identical to
the existing post-copy test, but migrates to a non-pristine QEMU. It works on
X86, but not yet on PPC64 because the serial output is different. It needs
quite a lot more work, but before I continue, I would like to know first if
this is something people fundamentally object to?

Richard Palethorpe (2):
  migrate: Allow incoming migration without defer
  migrate: Tests for migrating to an already used QEMU

 migration/migration.c  |  12 +++---
 tests/migration-test.c | 113 +++++++++++++++++++++++++++++++++++++------------
 vl.c                   |   2 +
 3 files changed, 92 insertions(+), 35 deletions(-)

-- 
2.16.2

[Qemu-devel] [RFC PATCH 1/2] migrate: Allow incoming migration without defer

[Richard Palethorpe]

Allow a QEMU instance which has been started and used without the "-incoming"
flag to accept an incoming migration with the "migrate-incoming" QMP
command. This allows the user to dump the VM state to an external file then
revert to that state at a later time without restarting QEMU.
---
 migration/migration.c | 12 +++++-------
 vl.c                  |  2 ++
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0aa596f867..05595a4cec 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1321,14 +1321,14 @@ void migrate_del_blocker(Error *reason)
 void qmp_migrate_incoming(const char *uri, Error **errp)
 {
     Error *local_err = NULL;
-    static bool once = true;
 
-    if (!deferred_incoming) {
-        error_setg(errp, "For use with '-incoming defer'");
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        error_setg(errp, "The incoming migration has already been started");
         return;
     }
-    if (!once) {
-        error_setg(errp, "The incoming migration has already been started");
+
+    if (!deferred_incoming) {
+        vm_stop(RUN_STATE_INMIGRATE);
     }
 
     qemu_start_incoming_migration(uri, &local_err);
@@ -1337,8 +1337,6 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
         error_propagate(errp, local_err);
         return;
     }
-
-    once = false;
 }
 
 bool migration_is_blocked(Error **errp)
diff --git a/vl.c b/vl.c
index 9e7235df6d..a91eda039e 100644
--- a/vl.c
+++ b/vl.c
@@ -634,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
     { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
     { RUN_STATE_PAUSED, RUN_STATE_COLO},
+    { RUN_STATE_PAUSED, RUN_STATE_INMIGRATE },
 
     { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
@@ -665,6 +666,7 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_RUNNING, RUN_STATE_WATCHDOG },
     { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
     { RUN_STATE_RUNNING, RUN_STATE_COLO},
+    { RUN_STATE_RUNNING, RUN_STATE_INMIGRATE },
 
     { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
 
-- 
2.16.2

[Qemu-devel] [RFC PATCH 2/2] migrate: Tests for migrating to an already used QEMU

[Richard Palethorpe]

Currently this appears to work for X86_64, but PPC64 fails due to some
unexpected data on the serial port after migration. This probably requires
quite a bit more work.
---
 tests/migration-test.c | 113 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 85 insertions(+), 28 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 74f9361bdd..8217d2e83e 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -125,19 +125,17 @@ static void init_bootfile_ppc(const char *bootpath)
  * we get an 'A' followed by an endless string of 'B's
  * but on the destination we won't have the A.
  */
-static void wait_for_serial(const char *side)
+static void wait_for_serial(const char *side, gboolean started)
 {
     char *serialpath = g_strdup_printf("%s/%s", tmpfs, side);
     FILE *serialfile = fopen(serialpath, "r");
     const char *arch = qtest_get_arch();
-    int started = (strcmp(side, "src_serial") == 0 &&
-                   strcmp(arch, "ppc64") == 0) ? 0 : 1;
 
     g_free(serialpath);
     do {
         int readvalue = fgetc(serialfile);
 
-        if (!started) {
+        if (!started && !strcmp(arch, "ppc64")) {
             /* SLOF prints its banner before starting test,
              * to ignore it, mark the start of the test with '_',
              * ignore all characters until this marker
@@ -358,16 +356,21 @@ static void migrate_set_capability(QTestState *who, const char *capability,
     QDECREF(rsp);
 }
 
-static void migrate(QTestState *who, const char *uri)
+static void migrate(QTestState *who, const char *uri, gboolean incoming)
 {
     QDict *rsp;
+    const gchar *cmd_name = incoming ? "migrate-incoming" : "migrate";
     gchar *cmd;
 
-    cmd = g_strdup_printf("{ 'execute': 'migrate',"
+    cmd = g_strdup_printf("{ 'execute': '%s',"
                           "'arguments': { 'uri': '%s' } }",
-                          uri);
+                          cmd_name, uri);
     rsp = qtest_qmp(who, cmd);
     g_free(cmd);
+    while (qdict_haskey(rsp, "event")) {
+        QDECREF(rsp);
+        rsp = qtest_qmp_receive(who);
+    }
     g_assert(qdict_haskey(rsp, "return"));
     QDECREF(rsp);
 }
@@ -382,15 +385,22 @@ static void migrate_start_postcopy(QTestState *who)
 }
 
 static void test_migrate_start(QTestState **from, QTestState **to,
-                               const char *uri)
+                               const char *uri, gboolean incoming)
 {
-    gchar *cmd_src, *cmd_dst;
+    gchar *cmd_src;
+    GString *cmd_dst = g_string_new(NULL);
     char *bootpath = g_strdup_printf("%s/bootsect", tmpfs);
+    char *bootpath_dst;
     const char *arch = qtest_get_arch();
     const char *accel = "kvm:tcg";
 
     got_stop = false;
 
+    if (incoming)
+        bootpath_dst = bootpath;
+    else
+        bootpath_dst = g_strdup_printf("%s/bootsect_dst", tmpfs);
+
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         init_bootfile_x86(bootpath);
         cmd_src = g_strdup_printf("-machine accel=%s -m 150M"
@@ -398,12 +408,14 @@ static void test_migrate_start(QTestState **from, QTestState **to,
                                   " -serial file:%s/src_serial"
                                   " -drive file=%s,format=raw",
                                   accel, tmpfs, bootpath);
-        cmd_dst = g_strdup_printf("-machine accel=%s -m 150M"
-                                  " -name target,debug-threads=on"
-                                  " -serial file:%s/dest_serial"
-                                  " -drive file=%s,format=raw"
-                                  " -incoming %s",
-                                  accel, tmpfs, bootpath, uri);
+        if (!incoming)
+            init_bootfile_x86(bootpath_dst);
+
+        g_string_printf(cmd_dst, "-machine accel=%s -m 150M"
+                        " -name target,debug-threads=on"
+                        " -serial file:%s/dest_serial"
+                        " -drive file=%s,format=raw",
+                        accel, tmpfs, bootpath_dst);
     } else if (strcmp(arch, "ppc64") == 0) {
 
         /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
@@ -416,22 +428,29 @@ static void test_migrate_start(QTestState **from, QTestState **to,
                                   " -serial file:%s/src_serial"
                                   " -drive file=%s,if=pflash,format=raw",
                                   accel, tmpfs, bootpath);
-        cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
-                                  " -name target,debug-threads=on"
-                                  " -serial file:%s/dest_serial"
-                                  " -incoming %s",
-                                  accel, tmpfs, uri);
+        g_string_printf(cmd_dst, "-machine accel=%s -m 256M"
+                        " -name target,debug-threads=on"
+                        " -serial file:%s/dest_serial",
+                        accel, tmpfs);
+        if (!incoming) {
+            g_string_append_printf(cmd_dst,
+                                   " -drive file=%s,if=pflash,format=raw",
+                                   bootpath);
+        }
     } else {
         g_assert_not_reached();
     }
 
+    if (incoming)
+        g_string_append_printf(cmd_dst, " -incoming %s", uri);
+
     g_free(bootpath);
 
     *from = qtest_start(cmd_src);
     g_free(cmd_src);
 
-    *to = qtest_init(cmd_dst);
-    g_free(cmd_dst);
+    *to = qtest_init(cmd_dst->str);
+    g_string_free(cmd_dst, TRUE);
 }
 
 static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
@@ -518,7 +537,7 @@ static void test_migrate(void)
     char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
     QTestState *from, *to;
 
-    test_migrate_start(&from, &to, uri);
+    test_migrate_start(&from, &to, uri, TRUE);
 
     migrate_set_capability(from, "postcopy-ram", "true");
     migrate_set_capability(to, "postcopy-ram", "true");
@@ -531,9 +550,9 @@ static void test_migrate(void)
     migrate_set_parameter(from, "downtime-limit", "1");
 
     /* Wait for the first serial output from the source */
-    wait_for_serial("src_serial");
+    wait_for_serial("src_serial", FALSE);
 
-    migrate(from, uri);
+    migrate(from, uri, FALSE);
 
     wait_for_migration_pass(from);
 
@@ -545,7 +564,7 @@ static void test_migrate(void)
 
     qtest_qmp_eventwait(to, "RESUME");
 
-    wait_for_serial("dest_serial");
+    wait_for_serial("dest_serial", TRUE);
     wait_for_migration_complete(from);
 
     g_free(uri);
@@ -560,8 +579,8 @@ static void test_baddest(void)
     const char *status;
     bool failed;
 
-    test_migrate_start(&from, &to, "tcp:0:0");
-    migrate(from, "tcp:0:0");
+    test_migrate_start(&from, &to, "tcp:0:0", TRUE);
+    migrate(from, "tcp:0:0", FALSE);
     do {
         rsp = wait_command(from, "{ 'execute': 'query-migrate' }");
         rsp_return = qdict_get_qdict(rsp, "return");
@@ -584,6 +603,43 @@ static void test_baddest(void)
     test_migrate_end(from, to, false);
 }
 
+static void test_migrate_to_used(void)
+{
+    char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+    QTestState *from, *to;
+
+    test_migrate_start(&from, &to, uri, FALSE);
+
+    migrate_set_capability(from, "postcopy-ram", "true");
+    migrate_set_capability(to, "postcopy-ram", "true");
+
+    migrate_set_parameter(from, "max-bandwidth", "100000000");
+    migrate_set_parameter(from, "downtime-limit", "1");
+
+    wait_for_serial("dest_serial", FALSE);
+    migrate(to, uri, TRUE);
+
+    wait_for_serial("src_serial", FALSE);
+    migrate(from, uri, FALSE);
+
+    wait_for_migration_pass(from);
+
+    migrate_start_postcopy(from);
+
+    if (!got_stop) {
+        qtest_qmp_eventwait(from, "STOP");
+    }
+
+    qtest_qmp_eventwait(to, "RESUME");
+
+    wait_for_serial("dest_serial", TRUE);
+    wait_for_migration_complete(from);
+
+    g_free(uri);
+
+    test_migrate_end(from, to, true);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -604,6 +660,7 @@ int main(int argc, char **argv)
     module_call_init(MODULE_INIT_QOM);
 
     qtest_add_func("/migration/postcopy/unix", test_migrate);
+    qtest_add_func("/migration/postcopy/to_used", test_migrate_to_used);
     qtest_add_func("/migration/deprecated", test_deprecated);
     qtest_add_func("/migration/bad_dest", test_baddest);
 
-- 
2.16.2

Re: [Qemu-devel] [RFC PATCH 0/2] Increase usability of external snapshots

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180227115651.30762-1-rpalethorpe@suse.com
Subject: [Qemu-devel] [RFC PATCH 0/2] Increase usability of external snapshots

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1519726932-13833-1-git-send-email-liran.alon@oracle.com -> patchew/1519726932-13833-1-git-send-email-liran.alon@oracle.com
 * [new tag]               patchew/20180227115651.30762-1-rpalethorpe@suse.com -> patchew/20180227115651.30762-1-rpalethorpe@suse.com
Switched to a new branch 'test'
cac7611aaa migrate: Tests for migrating to an already used QEMU
e1df946e98 migrate: Allow incoming migration without defer

=== OUTPUT BEGIN ===
Checking PATCH 1/2: migrate: Allow incoming migration without defer...
Checking PATCH 2/2: migrate: Tests for migrating to an already used QEMU...
ERROR: braces {} are necessary for all arms of this statement
#97: FILE: tests/migration-test.c:411:
+        if (!incoming)
[...]

ERROR: braces {} are necessary for all arms of this statement
#130: FILE: tests/migration-test.c:444:
+    if (incoming)
[...]

ERROR: Missing Signed-off-by: line(s)

total: 3 errors, 0 warnings, 212 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [RFC PATCH 1/2] migrate: Allow incoming migration without defer

[Dr. David Alan Gilbert]

* Richard Palethorpe (rpalethorpe@suse.com) wrote:
> Allow a QEMU instance which has been started and used without the "-incoming"
> flag to accept an incoming migration with the "migrate-incoming" QMP
> command. This allows the user to dump the VM state to an external file then
> revert to that state at a later time without restarting QEMU.
> ---
>  migration/migration.c | 12 +++++-------
>  vl.c                  |  2 ++
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0aa596f867..05595a4cec 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1321,14 +1321,14 @@ void migrate_del_blocker(Error *reason)
>  void qmp_migrate_incoming(const char *uri, Error **errp)
>  {
>      Error *local_err = NULL;
> -    static bool once = true;
>  
> -    if (!deferred_incoming) {
> -        error_setg(errp, "For use with '-incoming defer'");
> +    if (runstate_check(RUN_STATE_INMIGRATE)) {
> +        error_setg(errp, "The incoming migration has already been started");
>          return;
>      }
> -    if (!once) {
> -        error_setg(errp, "The incoming migration has already been started");
> +
> +    if (!deferred_incoming) {
> +        vm_stop(RUN_STATE_INMIGRATE);
>      }
>  
>      qemu_start_incoming_migration(uri, &local_err);
> @@ -1337,8 +1337,6 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    once = false;
>  }

That's...interesting.  I think it will work, but I bet there's a whole
bunch of corner cases [many of which loadvm will hit as well!].
For starters there's probably devices that are active and still looking
at RAM, even though vm_stop is in place (hopefully none of them are
writing to it, but I wouldn't actually trust them).  Also, you probably
need to inactivate the block devices?

Also, I think this means there's no protection against this happening
during an outgoing migration (which has an existing check to make sure
you can't start an outgoing migration while an incoming one is waiting).

>  bool migration_is_blocked(Error **errp)
> diff --git a/vl.c b/vl.c
> index 9e7235df6d..a91eda039e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -634,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
>      { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
>      { RUN_STATE_PAUSED, RUN_STATE_COLO},
> +    { RUN_STATE_PAUSED, RUN_STATE_INMIGRATE },
>  
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
> @@ -665,6 +666,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_RUNNING, RUN_STATE_WATCHDOG },
>      { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
>      { RUN_STATE_RUNNING, RUN_STATE_COLO},
> +    { RUN_STATE_RUNNING, RUN_STATE_INMIGRATE },

Experience suggests that you'll find out the hard way that there's other
states you'll need to allow or check for.
For example, do you want to be able to loadvm from a GUEST_PANICKED or
PRELAUNCH state?

Dave

>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>  
> -- 
> 2.16.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH 2/2] migrate: Tests for migrating to an already used QEMU

[Dr. David Alan Gilbert]

* Richard Palethorpe (rpalethorpe@suse.com) wrote:
> Currently this appears to work for X86_64, but PPC64 fails due to some
> unexpected data on the serial port after migration. This probably requires
> quite a bit more work.

I think the challenge is you need to drain the serial output to make
sure you aren't seeing anything from the first run, and then we need to
go back around to the point where on ppc you're expecting the banner
again; which I think I see you've tried to do.

Also, you don't want to misinterpret the succesful output of the 1st
boot as the reception of the migration.

> ---
>  tests/migration-test.c | 113 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 85 insertions(+), 28 deletions(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 74f9361bdd..8217d2e83e 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -125,19 +125,17 @@ static void init_bootfile_ppc(const char *bootpath)
>   * we get an 'A' followed by an endless string of 'B's
>   * but on the destination we won't have the A.
>   */
> -static void wait_for_serial(const char *side)
> +static void wait_for_serial(const char *side, gboolean started)
>  {
>      char *serialpath = g_strdup_printf("%s/%s", tmpfs, side);
>      FILE *serialfile = fopen(serialpath, "r");
>      const char *arch = qtest_get_arch();
> -    int started = (strcmp(side, "src_serial") == 0 &&
> -                   strcmp(arch, "ppc64") == 0) ? 0 : 1;
>  
>      g_free(serialpath);
>      do {
>          int readvalue = fgetc(serialfile);
>  
> -        if (!started) {
> +        if (!started && !strcmp(arch, "ppc64")) {
>              /* SLOF prints its banner before starting test,
>               * to ignore it, mark the start of the test with '_',
>               * ignore all characters until this marker
> @@ -358,16 +356,21 @@ static void migrate_set_capability(QTestState *who, const char *capability,
>      QDECREF(rsp);
>  }
>  
> -static void migrate(QTestState *who, const char *uri)
> +static void migrate(QTestState *who, const char *uri, gboolean incoming)
>  {
>      QDict *rsp;
> +    const gchar *cmd_name = incoming ? "migrate-incoming" : "migrate";
>      gchar *cmd;
>  
> -    cmd = g_strdup_printf("{ 'execute': 'migrate',"
> +    cmd = g_strdup_printf("{ 'execute': '%s',"
>                            "'arguments': { 'uri': '%s' } }",
> -                          uri);
> +                          cmd_name, uri);
>      rsp = qtest_qmp(who, cmd);
>      g_free(cmd);
> +    while (qdict_haskey(rsp, "event")) {
> +        QDECREF(rsp);
> +        rsp = qtest_qmp_receive(who);
> +    }
>      g_assert(qdict_haskey(rsp, "return"));
>      QDECREF(rsp);
>  }
> @@ -382,15 +385,22 @@ static void migrate_start_postcopy(QTestState *who)
>  }
>  
>  static void test_migrate_start(QTestState **from, QTestState **to,
> -                               const char *uri)
> +                               const char *uri, gboolean incoming)
>  {
> -    gchar *cmd_src, *cmd_dst;
> +    gchar *cmd_src;
> +    GString *cmd_dst = g_string_new(NULL);
>      char *bootpath = g_strdup_printf("%s/bootsect", tmpfs);
> +    char *bootpath_dst;
>      const char *arch = qtest_get_arch();
>      const char *accel = "kvm:tcg";
>  
>      got_stop = false;
>  
> +    if (incoming)
> +        bootpath_dst = bootpath;
> +    else
> +        bootpath_dst = g_strdup_printf("%s/bootsect_dst", tmpfs);
> +

Are you intending to run a separate bootsector file? You _could_ if you
wanted to make sure you couldn't confuse the two; but other than that
you could probably share the binary.

>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>          init_bootfile_x86(bootpath);
>          cmd_src = g_strdup_printf("-machine accel=%s -m 150M"
> @@ -398,12 +408,14 @@ static void test_migrate_start(QTestState **from, QTestState **to,
>                                    " -serial file:%s/src_serial"
>                                    " -drive file=%s,format=raw",
>                                    accel, tmpfs, bootpath);
> -        cmd_dst = g_strdup_printf("-machine accel=%s -m 150M"
> -                                  " -name target,debug-threads=on"
> -                                  " -serial file:%s/dest_serial"
> -                                  " -drive file=%s,format=raw"
> -                                  " -incoming %s",
> -                                  accel, tmpfs, bootpath, uri);
> +        if (!incoming)
> +            init_bootfile_x86(bootpath_dst);
> +
> +        g_string_printf(cmd_dst, "-machine accel=%s -m 150M"
> +                        " -name target,debug-threads=on"
> +                        " -serial file:%s/dest_serial"
> +                        " -drive file=%s,format=raw",
> +                        accel, tmpfs, bootpath_dst);
>      } else if (strcmp(arch, "ppc64") == 0) {
>  
>          /* On ppc64, the test only works with kvm-hv, but not with kvm-pr */
> @@ -416,22 +428,29 @@ static void test_migrate_start(QTestState **from, QTestState **to,
>                                    " -serial file:%s/src_serial"
>                                    " -drive file=%s,if=pflash,format=raw",
>                                    accel, tmpfs, bootpath);
> -        cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
> -                                  " -name target,debug-threads=on"
> -                                  " -serial file:%s/dest_serial"
> -                                  " -incoming %s",
> -                                  accel, tmpfs, uri);
> +        g_string_printf(cmd_dst, "-machine accel=%s -m 256M"
> +                        " -name target,debug-threads=on"
> +                        " -serial file:%s/dest_serial",
> +                        accel, tmpfs);
> +        if (!incoming) {
> +            g_string_append_printf(cmd_dst,
> +                                   " -drive file=%s,if=pflash,format=raw",
> +                                   bootpath);
> +        }
>      } else {
>          g_assert_not_reached();
>      }
>  
> +    if (incoming)
> +        g_string_append_printf(cmd_dst, " -incoming %s", uri);
> +
>      g_free(bootpath);
>  
>      *from = qtest_start(cmd_src);
>      g_free(cmd_src);
>  
> -    *to = qtest_init(cmd_dst);
> -    g_free(cmd_dst);
> +    *to = qtest_init(cmd_dst->str);
> +    g_string_free(cmd_dst, TRUE);
>  }
>  
>  static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
> @@ -518,7 +537,7 @@ static void test_migrate(void)
>      char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
>      QTestState *from, *to;
>  
> -    test_migrate_start(&from, &to, uri);
> +    test_migrate_start(&from, &to, uri, TRUE);
>  
>      migrate_set_capability(from, "postcopy-ram", "true");
>      migrate_set_capability(to, "postcopy-ram", "true");
> @@ -531,9 +550,9 @@ static void test_migrate(void)
>      migrate_set_parameter(from, "downtime-limit", "1");
>  
>      /* Wait for the first serial output from the source */
> -    wait_for_serial("src_serial");
> +    wait_for_serial("src_serial", FALSE);
>  
> -    migrate(from, uri);
> +    migrate(from, uri, FALSE);
>  
>      wait_for_migration_pass(from);
>  
> @@ -545,7 +564,7 @@ static void test_migrate(void)
>  
>      qtest_qmp_eventwait(to, "RESUME");
>  
> -    wait_for_serial("dest_serial");
> +    wait_for_serial("dest_serial", TRUE);
>      wait_for_migration_complete(from);
>  
>      g_free(uri);
> @@ -560,8 +579,8 @@ static void test_baddest(void)
>      const char *status;
>      bool failed;
>  
> -    test_migrate_start(&from, &to, "tcp:0:0");
> -    migrate(from, "tcp:0:0");
> +    test_migrate_start(&from, &to, "tcp:0:0", TRUE);
> +    migrate(from, "tcp:0:0", FALSE);
>      do {
>          rsp = wait_command(from, "{ 'execute': 'query-migrate' }");
>          rsp_return = qdict_get_qdict(rsp, "return");
> @@ -584,6 +603,43 @@ static void test_baddest(void)
>      test_migrate_end(from, to, false);
>  }
>  
> +static void test_migrate_to_used(void)
> +{
> +    char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> +    QTestState *from, *to;
> +
> +    test_migrate_start(&from, &to, uri, FALSE);
> +
> +    migrate_set_capability(from, "postcopy-ram", "true");
> +    migrate_set_capability(to, "postcopy-ram", "true");
> +
> +    migrate_set_parameter(from, "max-bandwidth", "100000000");
> +    migrate_set_parameter(from, "downtime-limit", "1");
> +
> +    wait_for_serial("dest_serial", FALSE);
> +    migrate(to, uri, TRUE);
> +
> +    wait_for_serial("src_serial", FALSE);
> +    migrate(from, uri, FALSE);
> +
> +    wait_for_migration_pass(from);
> +
> +    migrate_start_postcopy(from);

It's brave/good that you're doing this with postcopy; but with postcopy
we really really need to make sure any devices aren't reading memory
after they enter the paused state, although we've got so few devices
in this test we probably wont hit that.

Dave

> +    if (!got_stop) {
> +        qtest_qmp_eventwait(from, "STOP");
> +    }
> +
> +    qtest_qmp_eventwait(to, "RESUME");
> +
> +    wait_for_serial("dest_serial", TRUE);
> +    wait_for_migration_complete(from);
> +
> +    g_free(uri);
> +
> +    test_migrate_end(from, to, true);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      char template[] = "/tmp/migration-test-XXXXXX";
> @@ -604,6 +660,7 @@ int main(int argc, char **argv)
>      module_call_init(MODULE_INIT_QOM);
>  
>      qtest_add_func("/migration/postcopy/unix", test_migrate);
> +    qtest_add_func("/migration/postcopy/to_used", test_migrate_to_used);
>      qtest_add_func("/migration/deprecated", test_deprecated);
>      qtest_add_func("/migration/bad_dest", test_baddest);
>  
> -- 
> 2.16.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 0/2] Increase usability of external snapshots

[Roman Kagan]

On Tue, Feb 27, 2018 at 12:56:49PM +0100, Richard Palethorpe wrote:
> Following on from the discussion about creating savevm/loadvm QMP
> equivalents. I decided to take the advice given that we should use external
> snapshots. However reverting to a snapshot currently requires QEMU to be
> restarted with "-incoming".  Restarting QEMU requires a fair bit of
> book keeping to be done by the management application and may take
> measurably more time.

AFAICT "-incoming" is not the only reason for starting QEMU anew.  The
block devices will need to be pointed at different nodes in the backing
chains.  Moreover the snapshot you're reverting to may have been taken
at a point where the VM had different configuration than it has now.

So the management application will need to do a lot of bookkeeping stuff
anyway, and it'll probably have harder time applying all of the
configuration changes to a live QEMU instance.

Is the cost of killing the old QEMU process and starting a new one big
enough to be worth all the hassle?

Thanks,
Roman.

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 0/2] Increase usability of external snapshots

[Richard Palethorpe]

Hello Roman,

Roman Kagan writes:

> On Tue, Feb 27, 2018 at 12:56:49PM +0100, Richard Palethorpe wrote:
>> Following on from the discussion about creating savevm/loadvm QMP
>> equivalents. I decided to take the advice given that we should use external
>> snapshots. However reverting to a snapshot currently requires QEMU to be
>> restarted with "-incoming".  Restarting QEMU requires a fair bit of
>> book keeping to be done by the management application and may take
>> measurably more time.
>
> AFAICT "-incoming" is not the only reason for starting QEMU anew.  The
> block devices will need to be pointed at different nodes in the backing
> chains.  Moreover the snapshot you're reverting to may have been taken
> at a point where the VM had different configuration than it has now.

OK, so contrary to what I thought, it looks like there is no QMP command
to simply drop the current active layer and repoint the block device to
the previous node in the chain. We would have to recreate the block
device with the desired backing store and overlays or create a new
command. I am not sure that is a showstopper though.

>
> So the management application will need to do a lot of bookkeeping stuff
> anyway, and it'll probably have harder time applying all of the
> configuration changes to a live QEMU instance.

Well in our use case, configuration changes between snapshots would not
be possible. For other use cases the management app will have to restart
QEMU when necessary. AFAICT, with or without this patch it is possible for
someone to try accepting an incoming migration with an invalid
configuration. So I would hope that this is not introducing a new
problem?

>
> Is the cost of killing the old QEMU process and starting a new one big
> enough to be worth all the hassle?

I doubt I can justify it based on performance, atleast not for our use
case, but terminating the QEMU process will interrupt a number of socket
connections and such. I am sure this is a technically easier problem to
solve than modifying QEMU, but there is one QEMU and many management
apps so *maybe* the effort is better spent on QEMU if it avoids more work
in the management apps and is not introducing features to QEMU outside
of its scope. QEMU already supports reverting to internal snapshots in a
live instance, so I don't think I am increasing its scope.

>
> Thanks,
> Roman.

--
Thank you,
Richard.

Re: [Qemu-devel] [RFC PATCH 1/2] migrate: Allow incoming migration without defer

[Richard Palethorpe]

hello David,

Dr. David Alan Gilbert writes:

> * Richard Palethorpe (rpalethorpe@suse.com) wrote:
>> Allow a QEMU instance which has been started and used without the "-incoming"
>> flag to accept an incoming migration with the "migrate-incoming" QMP
>> command. This allows the user to dump the VM state to an external file then
>> revert to that state at a later time without restarting QEMU.
>> ---
>>  migration/migration.c | 12 +++++-------
>>  vl.c                  |  2 ++
>>  2 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 0aa596f867..05595a4cec 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1321,14 +1321,14 @@ void migrate_del_blocker(Error *reason)
>>  void qmp_migrate_incoming(const char *uri, Error **errp)
>>  {
>>      Error *local_err = NULL;
>> -    static bool once = true;
>>
>> -    if (!deferred_incoming) {
>> -        error_setg(errp, "For use with '-incoming defer'");
>> +    if (runstate_check(RUN_STATE_INMIGRATE)) {
>> +        error_setg(errp, "The incoming migration has already been started");
>>          return;
>>      }
>> -    if (!once) {
>> -        error_setg(errp, "The incoming migration has already been started");
>> +
>> +    if (!deferred_incoming) {
>> +        vm_stop(RUN_STATE_INMIGRATE);
>>      }
>>
>>      qemu_start_incoming_migration(uri, &local_err);
>> @@ -1337,8 +1337,6 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>> -
>> -    once = false;
>>  }
>
> That's...interesting.  I think it will work, but I bet there's a whole
> bunch of corner cases [many of which loadvm will hit as well!].

It wouldn't surprise me if we have hit some of those corner cases with
our extensive use of loadvm :-) Unfortunately I don't have much data to
show what went wrong though.

> For starters there's probably devices that are active and still looking
> at RAM, even though vm_stop is in place (hopefully none of them are
> writing to it, but I wouldn't actually trust them).

I suppose that might be a showstopper.

> Also, you probably
> need to inactivate the block devices?

They are at least flushed by vm_stop. I would expect the user to
recreate them, or at least recreate the active layer before starting the
migration.

>
> Also, I think this means there's no protection against this happening
> during an outgoing migration (which has an existing check to make sure
> you can't start an outgoing migration while an incoming one is waiting).
>
>>  bool migration_is_blocked(Error **errp)
>> diff --git a/vl.c b/vl.c
>> index 9e7235df6d..a91eda039e 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -634,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>>      { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
>>      { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
>>      { RUN_STATE_PAUSED, RUN_STATE_COLO},
>> +    { RUN_STATE_PAUSED, RUN_STATE_INMIGRATE },
>>
>>      { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
>>      { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
>> @@ -665,6 +666,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>>      { RUN_STATE_RUNNING, RUN_STATE_WATCHDOG },
>>      { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
>>      { RUN_STATE_RUNNING, RUN_STATE_COLO},
>> +    { RUN_STATE_RUNNING, RUN_STATE_INMIGRATE },
>
> Experience suggests that you'll find out the hard way that there's other
> states you'll need to allow or check for.
> For example, do you want to be able to loadvm from a GUEST_PANICKED or
> PRELAUNCH state?

Yes, I would add GUEST_PANICKED at least.

>
> Dave
>
>>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>>
>> --
>> 2.16.2
>>


--
Thank you,
Richard.