[PULL 0/6] Pull migration patches

[PULL 0/6] Pull migration patches

[Juan Quintela]

The following changes since commit e18e5501d8ac692d32657a3e1ef545b14e72b730:

  Merge remote-tracking branch 'remotes/dgilbert-gitlab/tags/pull-virtiofs-20200210' into staging (2020-02-10 18:09:14 +0000)

are available in the Git repository at:

  https://github.com/juanquintela/qemu.git tags/pull-migration-pull-request

for you to fetch changes up to 1a920d2b633e13df8961328b3b3e128989a34570:

  git: Make submodule check only needed modules (2020-02-13 11:31:58 +0100)

----------------------------------------------------------------
Migration pull request

- don't pause when migration has been cancelled (Zhimin)
- fix memleaks in tests (pan)(
- optimize wait-unplug (keqian)
- improve rdma error handling/messages (dave)
- add some flexibility in autoconverge test (dave)
- git-submodule: allow compiliation from same tree with different
  number of git-submodules (juan)

Please, Apply.

----------------------------------------------------------------

Dr. David Alan Gilbert (2):
  migration/rdma: rdma_accept_incoming_migration fix error handling
  tests/migration: Add some slack to auto converge

Juan Quintela (1):
  git: Make submodule check only needed modules

Keqian Zhu (1):
  migration: Optimization about wait-unplug migration state

Pan Nengyuan (1):
  migration-test: fix some memleaks in migration-test

Zhimin Feng (1):
  migration: Maybe VM is paused when migration is cancelled

 migration/migration.c        | 26 +++++++++++++++++---------
 migration/rdma.c             | 11 +++++++----
 migration/savevm.c           | 24 +++---------------------
 migration/savevm.h           |  1 -
 scripts/git-submodule.sh     | 12 ++++++++----
 tests/qtest/migration-test.c | 17 ++++++++++++++---
 6 files changed, 49 insertions(+), 42 deletions(-)

-- 
2.24.1

[PULL 1/6] migration: Maybe VM is paused when migration is cancelled

[Juan Quintela]

From: Zhimin Feng <fengzhimin1@huawei.com>

If the migration is cancelled when it is in the completion phase,
the migration state is set to MIGRATION_STATUS_CANCELLING.
The VM maybe wait for the 'pause_sem' semaphore in migration_maybe_pause
function, so that VM always is paused.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Zhimin Feng <fengzhimin1@huawei.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3a21a4686c..1ca6be2323 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2797,14 +2797,22 @@ static int migration_maybe_pause(MigrationState *s,
         /* This block intentionally left blank */
     }
 
-    qemu_mutex_unlock_iothread();
-    migrate_set_state(&s->state, *current_active_state,
-                      MIGRATION_STATUS_PRE_SWITCHOVER);
-    qemu_sem_wait(&s->pause_sem);
-    migrate_set_state(&s->state, MIGRATION_STATUS_PRE_SWITCHOVER,
-                      new_state);
-    *current_active_state = new_state;
-    qemu_mutex_lock_iothread();
+    /*
+     * If the migration is cancelled when it is in the completion phase,
+     * the migration state is set to MIGRATION_STATUS_CANCELLING.
+     * So we don't need to wait a semaphore, otherwise we would always
+     * wait for the 'pause_sem' semaphore.
+     */
+    if (s->state != MIGRATION_STATUS_CANCELLING) {
+        qemu_mutex_unlock_iothread();
+        migrate_set_state(&s->state, *current_active_state,
+                          MIGRATION_STATUS_PRE_SWITCHOVER);
+        qemu_sem_wait(&s->pause_sem);
+        migrate_set_state(&s->state, MIGRATION_STATUS_PRE_SWITCHOVER,
+                          new_state);
+        *current_active_state = new_state;
+        qemu_mutex_lock_iothread();
+    }
 
     return s->state == new_state ? 0 : -EINVAL;
 }
-- 
2.24.1

[PULL 2/6] migration: Optimization about wait-unplug migration state

[Juan Quintela]

From: Keqian Zhu <zhukeqian1@huawei.com>

qemu_savevm_nr_failover_devices() is originally designed to
get the number of failover devices, but it actually returns
the number of "unplug-pending" failover devices now. Moreover,
what drives migration state to wait-unplug should be the number
of "unplug-pending" failover devices, not all failover devices.

We can also notice that qemu_savevm_state_guest_unplug_pending()
and qemu_savevm_nr_failover_devices() is equivalent almost (from
the code view). So the latter is incorrect semantically and
useless, just delete it.

In the qemu_savevm_state_guest_unplug_pending(), once hit a
unplug-pending failover device, then it can return true right
now to save cpu time.

Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Tested-by: Jens Freimann <jfreimann@redhat.com>
Reviewed-by: Jens Freimann <jfreimann@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c |  2 +-
 migration/savevm.c    | 24 +++---------------------
 migration/savevm.h    |  1 -
 3 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 1ca6be2323..8fb68795dc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3341,7 +3341,7 @@ static void *migration_thread(void *opaque)
 
     qemu_savevm_state_setup(s->to_dst_file);
 
-    if (qemu_savevm_nr_failover_devices()) {
+    if (qemu_savevm_state_guest_unplug_pending()) {
         migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                           MIGRATION_STATUS_WAIT_UNPLUG);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index f19cb9ec7a..1d4220ece8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1140,36 +1140,18 @@ void qemu_savevm_state_header(QEMUFile *f)
     }
 }
 
-int qemu_savevm_nr_failover_devices(void)
+bool qemu_savevm_state_guest_unplug_pending(void)
 {
     SaveStateEntry *se;
-    int n = 0;
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (se->vmsd && se->vmsd->dev_unplug_pending &&
             se->vmsd->dev_unplug_pending(se->opaque)) {
-            n++;
-        }
-    }
-
-    return n;
-}
-
-bool qemu_savevm_state_guest_unplug_pending(void)
-{
-    SaveStateEntry *se;
-    int n = 0;
-
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
-        if (!se->vmsd || !se->vmsd->dev_unplug_pending) {
-            continue;
-        }
-        if (se->vmsd->dev_unplug_pending(se->opaque)) {
-            n++;
+            return true;
         }
     }
 
-    return n > 0;
+    return false;
 }
 
 void qemu_savevm_state_setup(QEMUFile *f)
diff --git a/migration/savevm.h b/migration/savevm.h
index c42b9c80ee..ba64a7e271 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -31,7 +31,6 @@
 
 bool qemu_savevm_state_blocked(Error **errp);
 void qemu_savevm_state_setup(QEMUFile *f);
-int qemu_savevm_nr_failover_devices(void);
 bool qemu_savevm_state_guest_unplug_pending(void);
 int qemu_savevm_state_resume_prepare(MigrationState *s);
 void qemu_savevm_state_header(QEMUFile *f);
-- 
2.24.1

[PULL 3/6] migration/rdma: rdma_accept_incoming_migration fix error handling

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

rdma_accept_incoming_migration is called from an fd handler and
can't return an Error * anywhere.
Currently it's leaking Error's in errp/local_err - there's
no point putting them in there unless we can report them.

Turn most into fprintf's, and the last into an error_reportf_err
where it's coming up from another function.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/rdma.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 2379b8345b..f61587891b 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3980,13 +3980,13 @@ static void rdma_accept_incoming_migration(void *opaque)
     RDMAContext *rdma = opaque;
     int ret;
     QEMUFile *f;
-    Error *local_err = NULL, **errp = &local_err;
+    Error *local_err = NULL;
 
     trace_qemu_rdma_accept_incoming_migration();
     ret = qemu_rdma_accept(rdma);
 
     if (ret) {
-        ERROR(errp, "RDMA Migration initialization failed!");
+        fprintf(stderr, "RDMA ERROR: Migration initialization failed\n");
         return;
     }
 
@@ -3998,13 +3998,16 @@ static void rdma_accept_incoming_migration(void *opaque)
 
     f = qemu_fopen_rdma(rdma, "rb");
     if (f == NULL) {
-        ERROR(errp, "could not qemu_fopen_rdma!");
+        fprintf(stderr, "RDMA ERROR: could not qemu_fopen_rdma\n");
         qemu_rdma_cleanup(rdma);
         return;
     }
 
     rdma->migration_started_on_destination = 1;
-    migration_fd_process_incoming(f, errp);
+    migration_fd_process_incoming(f, &local_err);
+    if (local_err) {
+        error_reportf_err(local_err, "RDMA ERROR:");
+    }
 }
 
 void rdma_start_incoming_migration(const char *host_port, Error **errp)
-- 
2.24.1

[PULL 4/6] tests/migration: Add some slack to auto converge

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

There's an assert in autoconverge that checks that we quit the
iteration when we go below the expected threshold.  Philippe
saw a case where this assert fired with the measured value
slightly over the threshold. (about 3k out of a few million).

I can think of two reasons:
  a) Rounding errors
  b) That after we make the decision to quit iteration we do one
    more sync and that sees a few more dirty pages.

So add 1% slack to the assertion, that should cover a and
most cases of b, probably all we'll see for the test.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 tests/qtest/migration-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index cf27ebbc9d..a78ac0c7da 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1237,7 +1237,8 @@ static void test_migrate_auto_converge(void)
     g_assert_cmpint(percentage, <=, max_pct);
 
     remaining = read_ram_property_int(from, "remaining");
-    g_assert_cmpint(remaining, <, expected_threshold);
+    g_assert_cmpint(remaining, <,
+                    (expected_threshold + expected_threshold / 100));
 
     migrate_continue(from, "pre-switchover");
 
-- 
2.24.1

[PULL 5/6] migration-test: fix some memleaks in migration-test

[Juan Quintela]

From: Pan Nengyuan <pannengyuan@huawei.com>

spotted by asan, 'check-qtest-aarch64' runs fail if sanitizers is enabled.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 tests/qtest/migration-test.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index a78ac0c7da..ccf313f288 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -498,11 +498,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     const char *arch = qtest_get_arch();
     const char *machine_opts = NULL;
     const char *memory_size;
+    int ret = 0;
 
     if (args->use_shmem) {
         if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
             g_test_skip("/dev/shm is not supported");
-            return -1;
+            ret = -1;
+            goto out;
         }
     }
 
@@ -611,8 +613,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         g_free(shmem_path);
     }
 
+out:
     migrate_start_destroy(args);
-    return 0;
+    return ret;
 }
 
 static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
@@ -1134,6 +1137,8 @@ static void test_validate_uuid(void)
 {
     MigrateStart *args = migrate_start_new();
 
+    g_free(args->opts_source);
+    g_free(args->opts_target);
     args->opts_source = g_strdup("-uuid 11111111-1111-1111-1111-111111111111");
     args->opts_target = g_strdup("-uuid 11111111-1111-1111-1111-111111111111");
     do_test_validate_uuid(args, false);
@@ -1143,6 +1148,8 @@ static void test_validate_uuid_error(void)
 {
     MigrateStart *args = migrate_start_new();
 
+    g_free(args->opts_source);
+    g_free(args->opts_target);
     args->opts_source = g_strdup("-uuid 11111111-1111-1111-1111-111111111111");
     args->opts_target = g_strdup("-uuid 22222222-2222-2222-2222-222222222222");
     args->hide_stderr = true;
@@ -1153,6 +1160,7 @@ static void test_validate_uuid_src_not_set(void)
 {
     MigrateStart *args = migrate_start_new();
 
+    g_free(args->opts_target);
     args->opts_target = g_strdup("-uuid 22222222-2222-2222-2222-222222222222");
     args->hide_stderr = true;
     do_test_validate_uuid(args, false);
@@ -1162,6 +1170,7 @@ static void test_validate_uuid_dst_not_set(void)
 {
     MigrateStart *args = migrate_start_new();
 
+    g_free(args->opts_source);
     args->opts_source = g_strdup("-uuid 11111111-1111-1111-1111-111111111111");
     args->hide_stderr = true;
     do_test_validate_uuid(args, false);
@@ -1380,6 +1389,7 @@ static void test_multifd_tcp_cancel(void)
                             "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
     qobject_unref(rsp);
 
+    g_free(uri);
     uri = migrate_get_socket_address(to2, "socket-address");
 
     wait_for_migration_status(from, "cancelled", NULL);
-- 
2.24.1

[PULL 6/6] git: Make submodule check only needed modules

[Juan Quintela]

If one is compiling more than one tree from the same source, it is
possible that they need different submodules.  Change the check to see
that all modules that we are interested in are updated, discarding the
ones that we don't care about.

Signed-off-by: Juan Quintela <quintela@redhat.com>

---

v1->v2:
patchw insists in not using modules
---
 scripts/git-submodule.sh | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/scripts/git-submodule.sh b/scripts/git-submodule.sh
index 98ca0f2737..65ed877aef 100755
--- a/scripts/git-submodule.sh
+++ b/scripts/git-submodule.sh
@@ -59,10 +59,14 @@ status)
     fi
 
     test -f "$substat" || exit 1
-    CURSTATUS=$($GIT submodule status $modules)
-    OLDSTATUS=$(cat $substat)
-    test "$CURSTATUS" = "$OLDSTATUS"
-    exit $?
+    for module in $modules; do
+        CURSTATUS=$($GIT submodule status $module)
+        OLDSTATUS=$(cat $substat | grep $module)
+        if test "$CURSTATUS" != "$OLDSTATUS"; then
+            exit 1
+        fi
+    done
+    exit 0
     ;;
 update)
     if test -z "$maybe_modules"
-- 
2.24.1

Re: [PULL 0/6] Pull migration patches

[Peter Maydell]

On Thu, 13 Feb 2020 at 13:21, Juan Quintela <quintela@redhat.com> wrote:
>
> The following changes since commit e18e5501d8ac692d32657a3e1ef545b14e72b730:
>
>   Merge remote-tracking branch 'remotes/dgilbert-gitlab/tags/pull-virtiofs-20200210' into staging (2020-02-10 18:09:14 +0000)
>
> are available in the Git repository at:
>
>   https://github.com/juanquintela/qemu.git tags/pull-migration-pull-request
>
> for you to fetch changes up to 1a920d2b633e13df8961328b3b3e128989a34570:
>
>   git: Make submodule check only needed modules (2020-02-13 11:31:58 +0100)
>
> ----------------------------------------------------------------
> Migration pull request
>
> - don't pause when migration has been cancelled (Zhimin)
> - fix memleaks in tests (pan)(
> - optimize wait-unplug (keqian)
> - improve rdma error handling/messages (dave)
> - add some flexibility in autoconverge test (dave)
> - git-submodule: allow compiliation from same tree with different
>   number of git-submodules (juan)
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM