[PATCH v4 00/10] COLO: improve build options

[PATCH v4 00/10] COLO: improve build options

[Vladimir Sementsov-Ogievskiy]

v4:
01: add r-b by Lukas
02: new
03: - keep x-colo capability enum value unconditional
    - drop ifdefs in options.c and keep capability check instead
    - update stubs
    - add missed a-b by Dr. David
04: keep filter-mirror untouched, add r-b by Juan

others: new. Some further improvements of COLO module API. May be merged
separately.

Hi all!

COLO substem seems to be useless when CONFIG_REPLICATION is unset, as we
simply don't allow to set x-colo capability in this case. So, let's not
compile in unreachable code and interface we cannot use when
CONFIG_REPLICATION is unset.

Also, provide personal configure option for COLO Proxy subsystem.

Vladimir Sementsov-Ogievskiy (10):
  block/meson.build: prefer positive condition for replication
  colo: make colo_checkpoint_notify static and provide simpler API
  build: move COLO under CONFIG_REPLICATION
  configure: add --disable-colo-proxy option
  migration: drop colo_incoming_thread from MigrationIncomingState
  migration: process_incoming_migration_co: simplify code flow around
    ret
  migration: split migration_incoming_co
  migration: process_incoming_migration_co(): move colo part to colo
  migration: disallow change capabilities in COLO state
  migration: block incoming colo when capability is disabled

 block/meson.build              |   2 +-
 hmp-commands.hx                |   2 +
 include/migration/colo.h       |  18 +++++-
 meson_options.txt              |   2 +
 migration/colo.c               | 100 +++++++++++++++++++--------------
 migration/meson.build          |   6 +-
 migration/migration-hmp-cmds.c |   2 +
 migration/migration.c          |  51 +++++++----------
 migration/migration.h          |  11 +++-
 migration/options.c            |   6 +-
 net/meson.build                |  13 ++++-
 qapi/migration.json            |   9 ++-
 scripts/meson-buildoptions.sh  |   3 +
 stubs/colo-compare.c           |   7 +++
 stubs/colo.c                   |  37 ++++++++++++
 stubs/meson.build              |   2 +
 16 files changed, 181 insertions(+), 90 deletions(-)
 create mode 100644 stubs/colo-compare.c
 create mode 100644 stubs/colo.c

-- 
2.34.1

[PATCH v4 01/10] block/meson.build: prefer positive condition for replication

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Lukas Straub <lukasstraub2@web.de>
---
 block/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/meson.build b/block/meson.build
index 382bec0e7d..b9a72e219b 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -84,7 +84,7 @@ block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-win32.c', 'win32-aio.c')
 block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit])
 block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c'))
 block_ss.add(when: 'CONFIG_LINUX', if_true: files('nvme.c'))
-if not get_option('replication').disabled()
+if get_option('replication').allowed()
   block_ss.add(files('replication.c'))
 endif
 block_ss.add(when: libaio, if_true: files('linux-aio.c'))
-- 
2.34.1

[PATCH v4 02/10] colo: make colo_checkpoint_notify static and provide simpler API

[Vladimir Sementsov-Ogievskiy]

colo_checkpoint_notify() is mostly used in colo.c. Outside we use it
once when x-checkpoint-delay migration parameter is set. So, let's
simplify the external API to only that function - notify COLO that
parameter was set. This make external API more robust and hides
implementation details from external callers. Also this helps us to
make COLO module optional in further patch (i.e. we are going to add
possibility not build the COLO module).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 include/migration/colo.h |  9 ++++++++-
 migration/colo.c         | 29 ++++++++++++++++++-----------
 migration/options.c      |  4 +---
 3 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/include/migration/colo.h b/include/migration/colo.h
index 5fbe1a6d5d..7ef315473e 100644
--- a/include/migration/colo.h
+++ b/include/migration/colo.h
@@ -36,6 +36,13 @@ COLOMode get_colo_mode(void);
 /* failover */
 void colo_do_failover(void);
 
-void colo_checkpoint_notify(void *opaque);
+/*
+ * colo_checkpoint_delay_set
+ *
+ * Handles change of x-checkpoint-delay migration parameter, called from
+ * migrate_params_apply() to notify COLO module about the change.
+ */
+void colo_checkpoint_delay_set(void);
+
 void colo_shutdown(void);
 #endif
diff --git a/migration/colo.c b/migration/colo.c
index 07bfa21fea..c9e0b909b9 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -65,6 +65,24 @@ static bool colo_runstate_is_stopped(void)
     return runstate_check(RUN_STATE_COLO) || !runstate_is_running();
 }
 
+static void colo_checkpoint_notify(void *opaque)
+{
+    MigrationState *s = opaque;
+    int64_t next_notify_time;
+
+    qemu_event_set(&s->colo_checkpoint_event);
+    s->colo_checkpoint_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
+    next_notify_time = s->colo_checkpoint_time + migrate_checkpoint_delay();
+    timer_mod(s->colo_delay_timer, next_notify_time);
+}
+
+void colo_checkpoint_delay_set(void)
+{
+    if (migration_in_colo_state()) {
+        colo_checkpoint_notify(migrate_get_current());
+    }
+}
+
 static void secondary_vm_do_failover(void)
 {
 /* COLO needs enable block-replication */
@@ -644,17 +662,6 @@ out:
     }
 }
 
-void colo_checkpoint_notify(void *opaque)
-{
-    MigrationState *s = opaque;
-    int64_t next_notify_time;
-
-    qemu_event_set(&s->colo_checkpoint_event);
-    s->colo_checkpoint_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
-    next_notify_time = s->colo_checkpoint_time + migrate_checkpoint_delay();
-    timer_mod(s->colo_delay_timer, next_notify_time);
-}
-
 void migrate_start_colo_process(MigrationState *s)
 {
     qemu_mutex_unlock_iothread();
diff --git a/migration/options.c b/migration/options.c
index 53b7fc5d5d..865a0214d8 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1246,9 +1246,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
 
     if (params->has_x_checkpoint_delay) {
         s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
-        if (migration_in_colo_state()) {
-            colo_checkpoint_notify(s);
-        }
+        colo_checkpoint_delay_set();
     }
 
     if (params->has_block_incremental) {
-- 
2.34.1

[PATCH v4 03/10] build: move COLO under CONFIG_REPLICATION

[Vladimir Sementsov-Ogievskiy]

We don't allow to use x-colo capability when replication is not
configured. So, no reason to build COLO when replication is disabled,
it's unusable in this case.

Note also that the check in migrate_caps_check() is not the only
restriction: some functions in migration/colo.c will just abort if
called with not defined CONFIG_REPLICATION, for example:

    migration_iteration_finish()
       case MIGRATION_STATUS_COLO:
           migrate_start_colo_process()
               colo_process_checkpoint()
                   abort()

It could probably make sense to have possibility to enable COLO without
REPLICATION, but this requires deeper audit of colo & replication code,
which may be done later if needed.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Acked-by: Dr. David Alan Gilbert <dave@treblig.org>
---
 hmp-commands.hx                |  2 ++
 migration/colo.c               | 28 ------------------------
 migration/meson.build          |  6 ++++--
 migration/migration-hmp-cmds.c |  2 ++
 migration/migration.c          |  6 ++++++
 qapi/migration.json            |  9 +++++---
 stubs/colo.c                   | 39 ++++++++++++++++++++++++++++++++++
 stubs/meson.build              |  1 +
 8 files changed, 60 insertions(+), 33 deletions(-)
 create mode 100644 stubs/colo.c

diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb85ee1d26..fbd0932232 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1035,6 +1035,7 @@ SRST
   migration (or once already in postcopy).
 ERST
 
+#ifdef CONFIG_REPLICATION
     {
         .name       = "x_colo_lost_heartbeat",
         .args_type  = "",
@@ -1043,6 +1044,7 @@ ERST
                       "a failover or takeover is needed.",
         .cmd = hmp_x_colo_lost_heartbeat,
     },
+#endif
 
 SRST
 ``x_colo_lost_heartbeat``
diff --git a/migration/colo.c b/migration/colo.c
index c9e0b909b9..6c7c313956 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -26,9 +26,7 @@
 #include "qemu/rcu.h"
 #include "migration/failover.h"
 #include "migration/ram.h"
-#ifdef CONFIG_REPLICATION
 #include "block/replication.h"
-#endif
 #include "net/colo-compare.h"
 #include "net/colo.h"
 #include "block/block.h"
@@ -86,7 +84,6 @@ void colo_checkpoint_delay_set(void)
 static void secondary_vm_do_failover(void)
 {
 /* COLO needs enable block-replication */
-#ifdef CONFIG_REPLICATION
     int old_state;
     MigrationIncomingState *mis = migration_incoming_get_current();
     Error *local_err = NULL;
@@ -151,14 +148,10 @@ static void secondary_vm_do_failover(void)
     if (mis->migration_incoming_co) {
         qemu_coroutine_enter(mis->migration_incoming_co);
     }
-#else
-    abort();
-#endif
 }
 
 static void primary_vm_do_failover(void)
 {
-#ifdef CONFIG_REPLICATION
     MigrationState *s = migrate_get_current();
     int old_state;
     Error *local_err = NULL;
@@ -199,9 +192,6 @@ static void primary_vm_do_failover(void)
 
     /* Notify COLO thread that failover work is finished */
     qemu_sem_post(&s->colo_exit_sem);
-#else
-    abort();
-#endif
 }
 
 COLOMode get_colo_mode(void)
@@ -235,7 +225,6 @@ void colo_do_failover(void)
     }
 }
 
-#ifdef CONFIG_REPLICATION
 void qmp_xen_set_replication(bool enable, bool primary,
                              bool has_failover, bool failover,
                              Error **errp)
@@ -289,7 +278,6 @@ void qmp_xen_colo_do_checkpoint(Error **errp)
     /* Notify all filters of all NIC to do checkpoint */
     colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
 }
-#endif
 
 COLOStatus *qmp_query_colo_status(Error **errp)
 {
@@ -453,15 +441,11 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
     qemu_mutex_lock_iothread();
 
-#ifdef CONFIG_REPLICATION
     replication_do_checkpoint_all(&local_err);
     if (local_err) {
         qemu_mutex_unlock_iothread();
         goto out;
     }
-#else
-        abort();
-#endif
 
     colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND, &local_err);
     if (local_err) {
@@ -579,15 +563,11 @@ static void colo_process_checkpoint(MigrationState *s)
     object_unref(OBJECT(bioc));
 
     qemu_mutex_lock_iothread();
-#ifdef CONFIG_REPLICATION
     replication_start_all(REPLICATION_MODE_PRIMARY, &local_err);
     if (local_err) {
         qemu_mutex_unlock_iothread();
         goto out;
     }
-#else
-        abort();
-#endif
 
     vm_start();
     qemu_mutex_unlock_iothread();
@@ -755,7 +735,6 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
         return;
     }
 
-#ifdef CONFIG_REPLICATION
     replication_get_error_all(&local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -772,9 +751,6 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
         qemu_mutex_unlock_iothread();
         return;
     }
-#else
-    abort();
-#endif
     /* Notify all filters of all NIC to do checkpoint */
     colo_notify_filters_event(COLO_EVENT_CHECKPOINT, &local_err);
 
@@ -881,15 +857,11 @@ void *colo_process_incoming_thread(void *opaque)
     object_unref(OBJECT(bioc));
 
     qemu_mutex_lock_iothread();
-#ifdef CONFIG_REPLICATION
     replication_start_all(REPLICATION_MODE_SECONDARY, &local_err);
     if (local_err) {
         qemu_mutex_unlock_iothread();
         goto out;
     }
-#else
-        abort();
-#endif
     vm_start();
     qemu_mutex_unlock_iothread();
     trace_colo_vm_state_change("stop", "run");
diff --git a/migration/meson.build b/migration/meson.build
index 480ff6854a..9c6a8e65d3 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -13,8 +13,6 @@ softmmu_ss.add(files(
   'block-dirty-bitmap.c',
   'channel.c',
   'channel-block.c',
-  'colo-failover.c',
-  'colo.c',
   'exec.c',
   'fd.c',
   'global_state.c',
@@ -30,6 +28,10 @@ softmmu_ss.add(files(
   'threadinfo.c',
 ), gnutls)
 
+if get_option('replication').allowed()
+  softmmu_ss.add(files('colo-failover.c', 'colo.c'))
+endif
+
 softmmu_ss.add(when: rdma, if_true: files('rdma.c'))
 if get_option('live_block_migration').allowed()
   softmmu_ss.add(files('block.c'))
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 4e9f00e7dc..9885d7c9f7 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -643,6 +643,7 @@ void hmp_migrate_start_postcopy(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, err);
 }
 
+#ifdef CONFIG_REPLICATION
 void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
@@ -650,6 +651,7 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
     qmp_x_colo_lost_heartbeat(&err);
     hmp_handle_error(mon, err);
 }
+#endif
 
 typedef struct HMPMigrationStatus {
     QEMUTimer *timer;
diff --git a/migration/migration.c b/migration/migration.c
index abcadbb619..0c14837cd3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -389,6 +389,12 @@ void migration_incoming_disable_colo(void)
 
 int migration_incoming_enable_colo(void)
 {
+#ifndef CONFIG_REPLICATION
+    error_report("ENABLE_COLO command come in migration stream, but COLO "
+                 "module is not built in");
+    return -ENOTSUP;
+#endif
+
     if (ram_block_discard_disable(true)) {
         error_report("COLO: cannot disable RAM discard");
         return -EBUSY;
diff --git a/qapi/migration.json b/qapi/migration.json
index 2c35b7b9cf..422a6b2613 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1381,7 +1381,8 @@
 #
 ##
 { 'command': 'x-colo-lost-heartbeat',
-  'features': [ 'unstable' ] }
+  'features': [ 'unstable' ],
+  'if': 'CONFIG_REPLICATION' }
 
 ##
 # @migrate_cancel:
@@ -1657,7 +1658,8 @@
 ##
 { 'struct': 'COLOStatus',
   'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
-            'reason': 'COLOExitReason' } }
+            'reason': 'COLOExitReason' },
+  'if': 'CONFIG_REPLICATION' }
 
 ##
 # @query-colo-status:
@@ -1674,7 +1676,8 @@
 # Since: 3.1
 ##
 { 'command': 'query-colo-status',
-  'returns': 'COLOStatus' }
+  'returns': 'COLOStatus',
+  'if': 'CONFIG_REPLICATION' }
 
 ##
 # @migrate-recover:
diff --git a/stubs/colo.c b/stubs/colo.c
new file mode 100644
index 0000000000..cf9816d368
--- /dev/null
+++ b/stubs/colo.c
@@ -0,0 +1,39 @@
+#include "qemu/osdep.h"
+#include "qemu/notify.h"
+#include "net/colo-compare.h"
+#include "migration/colo.h"
+#include "migration/migration.h"
+#include "qemu/error-report.h"
+#include "qapi/qapi-commands-migration.h"
+
+void colo_shutdown(void)
+{
+}
+
+void *colo_process_incoming_thread(void *opaque)
+{
+    error_report("Impossible happend: trying to start COLO thread when COLO "
+                 "module is not built in");
+    abort();
+}
+
+void colo_checkpoint_delay_set(void)
+{
+}
+
+void migrate_start_colo_process(MigrationState *s)
+{
+    error_report("Impossible happend: trying to start COLO when COLO "
+                 "module is not built in");
+    abort();
+}
+
+bool migration_in_colo_state(void)
+{
+    return false;
+}
+
+bool migration_incoming_in_colo_state(void)
+{
+    return false;
+}
diff --git a/stubs/meson.build b/stubs/meson.build
index b2b5956d97..8412cad15f 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -45,6 +45,7 @@ stub_ss.add(files('target-get-monitor-def.c'))
 stub_ss.add(files('target-monitor-defs.c'))
 stub_ss.add(files('trace-control.c'))
 stub_ss.add(files('uuid.c'))
+stub_ss.add(files('colo.c'))
 stub_ss.add(files('vmstate.c'))
 stub_ss.add(files('vm-stop.c'))
 stub_ss.add(files('win32-kbd-hook.c'))
-- 
2.34.1

[PATCH v4 04/10] configure: add --disable-colo-proxy option

[Vladimir Sementsov-Ogievskiy]

Add option to not build filter-mirror, filter-rewriter and
colo-compare when they are not needed.

There could be more agile configuration, for example add separate
options for each filter, but that may be done in future on demand. The
aim of this patch is to make possible to disable the whole COLO Proxy
subsystem.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 meson_options.txt             |  2 ++
 net/meson.build               | 13 ++++++++++---
 scripts/meson-buildoptions.sh |  3 +++
 stubs/colo-compare.c          |  7 +++++++
 stubs/meson.build             |  1 +
 5 files changed, 23 insertions(+), 3 deletions(-)
 create mode 100644 stubs/colo-compare.c

diff --git a/meson_options.txt b/meson_options.txt
index 2471dd02da..b59e7ae342 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -289,6 +289,8 @@ option('live_block_migration', type: 'feature', value: 'auto',
        description: 'block migration in the main migration stream')
 option('replication', type: 'feature', value: 'auto',
        description: 'replication support')
+option('colo_proxy', type: 'feature', value: 'auto',
+       description: 'colo-proxy support')
 option('bochs', type: 'feature', value: 'auto',
        description: 'bochs image format support')
 option('cloop', type: 'feature', value: 'auto',
diff --git a/net/meson.build b/net/meson.build
index 87afca3e93..6f4ecde57f 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -1,13 +1,10 @@
 softmmu_ss.add(files(
   'announce.c',
   'checksum.c',
-  'colo-compare.c',
-  'colo.c',
   'dump.c',
   'eth.c',
   'filter-buffer.c',
   'filter-mirror.c',
-  'filter-rewriter.c',
   'filter.c',
   'hub.c',
   'net-hmp-cmds.c',
@@ -19,6 +16,16 @@ softmmu_ss.add(files(
   'util.c',
 ))
 
+if get_option('replication').allowed() or \
+    get_option('colo_proxy').allowed()
+  softmmu_ss.add(files('colo-compare.c'))
+  softmmu_ss.add(files('colo.c'))
+endif
+
+if get_option('colo_proxy').allowed()
+  softmmu_ss.add(files('filter-rewriter.c'))
+endif
+
 softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
 
 if have_l2tpv3
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index d4369a3ad8..036047ce6f 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -83,6 +83,7 @@ meson_options_help() {
   printf "%s\n" '  capstone        Whether and how to find the capstone library'
   printf "%s\n" '  cloop           cloop image format support'
   printf "%s\n" '  cocoa           Cocoa user interface (macOS only)'
+  printf "%s\n" '  colo-proxy      colo-proxy support'
   printf "%s\n" '  coreaudio       CoreAudio sound support'
   printf "%s\n" '  crypto-afalg    Linux AF_ALG crypto backend driver'
   printf "%s\n" '  curl            CURL block device driver'
@@ -236,6 +237,8 @@ _meson_option_parse() {
     --disable-cloop) printf "%s" -Dcloop=disabled ;;
     --enable-cocoa) printf "%s" -Dcocoa=enabled ;;
     --disable-cocoa) printf "%s" -Dcocoa=disabled ;;
+    --enable-colo-proxy) printf "%s" -Dcolo_proxy=enabled ;;
+    --disable-colo-proxy) printf "%s" -Dcolo_proxy=disabled ;;
     --enable-coreaudio) printf "%s" -Dcoreaudio=enabled ;;
     --disable-coreaudio) printf "%s" -Dcoreaudio=disabled ;;
     --enable-coroutine-pool) printf "%s" -Dcoroutine_pool=true ;;
diff --git a/stubs/colo-compare.c b/stubs/colo-compare.c
new file mode 100644
index 0000000000..ec726665be
--- /dev/null
+++ b/stubs/colo-compare.c
@@ -0,0 +1,7 @@
+#include "qemu/osdep.h"
+#include "qemu/notify.h"
+#include "net/colo-compare.h"
+
+void colo_compare_cleanup(void)
+{
+}
diff --git a/stubs/meson.build b/stubs/meson.build
index 8412cad15f..a56645e2f7 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -46,6 +46,7 @@ stub_ss.add(files('target-monitor-defs.c'))
 stub_ss.add(files('trace-control.c'))
 stub_ss.add(files('uuid.c'))
 stub_ss.add(files('colo.c'))
+stub_ss.add(files('colo-compare.c'))
 stub_ss.add(files('vmstate.c'))
 stub_ss.add(files('vm-stop.c'))
 stub_ss.add(files('win32-kbd-hook.c'))
-- 
2.34.1

[PATCH v4 05/10] migration: drop colo_incoming_thread from MigrationIncomingState

[Vladimir Sementsov-Ogievskiy]

have_colo_incoming_thread variable is unused. colo_incoming_thread can
be local.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 migration/migration.c | 7 ++++---
 migration/migration.h | 2 --
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0c14837cd3..d4fa1a853c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -535,6 +535,8 @@ process_incoming_migration_co(void *opaque)
 
     /* we get COLO info, and know if we are in COLO mode */
     if (!ret && migration_incoming_colo_enabled()) {
+        QemuThread colo_incoming_thread;
+
         /* Make sure all file formats throw away their mutable metadata */
         bdrv_activate_all(&local_err);
         if (local_err) {
@@ -542,14 +544,13 @@ process_incoming_migration_co(void *opaque)
             goto fail;
         }
 
-        qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
+        qemu_thread_create(&colo_incoming_thread, "COLO incoming",
              colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE);
-        mis->have_colo_incoming_thread = true;
         qemu_coroutine_yield();
 
         qemu_mutex_unlock_iothread();
         /* Wait checkpoint incoming thread exit before free resource */
-        qemu_thread_join(&mis->colo_incoming_thread);
+        qemu_thread_join(&colo_incoming_thread);
         qemu_mutex_lock_iothread();
         /* We hold the global iothread lock, so it is safe here */
         colo_release_ram_cache();
diff --git a/migration/migration.h b/migration/migration.h
index 3a918514e7..7721c7658b 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -162,8 +162,6 @@ struct MigrationIncomingState {
 
     int state;
 
-    bool have_colo_incoming_thread;
-    QemuThread colo_incoming_thread;
     /* The coroutine we should enter (back) after failover */
     Coroutine *migration_incoming_co;
     QemuSemaphore colo_incoming_sem;
-- 
2.34.1

[PATCH v4 06/10] migration: process_incoming_migration_co: simplify code flow around ret

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 migration/migration.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index d4fa1a853c..8db0892317 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -533,8 +533,13 @@ process_incoming_migration_co(void *opaque)
         /* Else if something went wrong then just fall out of the normal exit */
     }
 
+    if (ret < 0) {
+        error_report("load of migration failed: %s", strerror(-ret));
+        goto fail;
+    }
+
     /* we get COLO info, and know if we are in COLO mode */
-    if (!ret && migration_incoming_colo_enabled()) {
+    if (migration_incoming_colo_enabled()) {
         QemuThread colo_incoming_thread;
 
         /* Make sure all file formats throw away their mutable metadata */
@@ -556,10 +561,6 @@ process_incoming_migration_co(void *opaque)
         colo_release_ram_cache();
     }
 
-    if (ret < 0) {
-        error_report("load of migration failed: %s", strerror(-ret));
-        goto fail;
-    }
     mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
     qemu_bh_schedule(mis->bh);
     mis->migration_incoming_co = NULL;
-- 
2.34.1

[PATCH v4 07/10] migration: split migration_incoming_co

[Vladimir Sementsov-Ogievskiy]

Originally, migration_incoming_co was introduced by
25d0c16f625feb3b6
   "migration: Switch to COLO process after finishing loadvm"
to be able to enter from COLO code to one specific yield point, added
by 25d0c16f625feb3b6.

Later in 923709896b1b0
 "migration: poll the cm event for destination qemu"
we reused this variable to wake the migration incoming coroutine from
RDMA code.

That was doubtful idea. Entering coroutines is a very fragile thing:
you should be absolutely sure which yield point you are going to enter.

I don't know how much is it safe to enter during qemu_loadvm_state()
which I think what RDMA want to do. But for sure RDMA shouldn't enter
the special COLO-related yield-point. As well, COLO code doesn't want
to enter during qemu_loadvm_state(), it want to enter it's own specific
yield-point.

As well, when in 8e48ac95865ac97d
 "COLO: Add block replication into colo process" we added
bdrv_invalidate_cache_all() call (now it's called activate_all())
it became possible to enter the migration incoming coroutine during
that call which is wrong too.

So, let't make these things separate and disjoint: loadvm_co for RDMA,
non-NULL during qemu_loadvm_state(), and colo_incoming_co for COLO,
non-NULL only around specific yield.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 migration/colo.c      | 4 ++--
 migration/migration.c | 8 ++++++--
 migration/migration.h | 9 ++++++++-
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index 6c7c313956..a688ac553a 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -145,8 +145,8 @@ static void secondary_vm_do_failover(void)
     qemu_sem_post(&mis->colo_incoming_sem);
 
     /* For Secondary VM, jump to incoming co */
-    if (mis->migration_incoming_co) {
-        qemu_coroutine_enter(mis->migration_incoming_co);
+    if (mis->colo_incoming_co) {
+        qemu_coroutine_enter(mis->colo_incoming_co);
     }
 }
 
diff --git a/migration/migration.c b/migration/migration.c
index 8db0892317..23b2d187de 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -505,12 +505,14 @@ process_incoming_migration_co(void *opaque)
     Error *local_err = NULL;
 
     assert(mis->from_src_file);
-    mis->migration_incoming_co = qemu_coroutine_self();
     mis->largest_page_size = qemu_ram_pagesize_largest();
     postcopy_state_set(POSTCOPY_INCOMING_NONE);
     migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
                       MIGRATION_STATUS_ACTIVE);
+
+    mis->loadvm_co = qemu_coroutine_self();
     ret = qemu_loadvm_state(mis->from_src_file);
+    mis->loadvm_co = NULL;
 
     ps = postcopy_state_get();
     trace_process_incoming_migration_co_end(ret, ps);
@@ -551,7 +553,10 @@ process_incoming_migration_co(void *opaque)
 
         qemu_thread_create(&colo_incoming_thread, "COLO incoming",
              colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE);
+
+        mis->colo_incoming_co = qemu_coroutine_self();
         qemu_coroutine_yield();
+        mis->colo_incoming_co = NULL;
 
         qemu_mutex_unlock_iothread();
         /* Wait checkpoint incoming thread exit before free resource */
@@ -563,7 +568,6 @@ process_incoming_migration_co(void *opaque)
 
     mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
     qemu_bh_schedule(mis->bh);
-    mis->migration_incoming_co = NULL;
     return;
 fail:
     local_err = NULL;
diff --git a/migration/migration.h b/migration/migration.h
index 7721c7658b..48a46123a0 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -162,8 +162,15 @@ struct MigrationIncomingState {
 
     int state;
 
+    /*
+     * The incoming migration coroutine, non-NULL during qemu_loadvm_state().
+     * Used to wake the migration incoming coroutine from rdma code. How much is
+     * it safe - it's a question.
+     */
+    Coroutine *loadvm_co;
+
     /* The coroutine we should enter (back) after failover */
-    Coroutine *migration_incoming_co;
+    Coroutine *colo_incoming_co;
     QemuSemaphore colo_incoming_sem;
 
     /*
-- 
2.34.1

[PATCH v4 08/10] migration: process_incoming_migration_co(): move colo part to colo

[Vladimir Sementsov-Ogievskiy]

Let's make better public interface for COLO: instead of
colo_process_incoming_thread and not trivial logic around creating the
thread let's make simple colo_incoming_co(), hiding implementation from
generic code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 include/migration/colo.h |  9 ++++++++-
 migration/colo.c         | 39 ++++++++++++++++++++++++++++++++++++++-
 migration/migration.c    | 28 ++--------------------------
 stubs/colo.c             |  6 ++----
 4 files changed, 50 insertions(+), 32 deletions(-)

diff --git a/include/migration/colo.h b/include/migration/colo.h
index 7ef315473e..eaac07f26d 100644
--- a/include/migration/colo.h
+++ b/include/migration/colo.h
@@ -28,7 +28,6 @@ bool migration_in_colo_state(void);
 int migration_incoming_enable_colo(void);
 void migration_incoming_disable_colo(void);
 bool migration_incoming_colo_enabled(void);
-void *colo_process_incoming_thread(void *opaque);
 bool migration_incoming_in_colo_state(void);
 
 COLOMode get_colo_mode(void);
@@ -44,5 +43,13 @@ void colo_do_failover(void);
  */
 void colo_checkpoint_delay_set(void);
 
+/*
+ * Starts COLO incoming process. Called from process_incoming_migration_co()
+ * after loading the state.
+ *
+ * Called with BQL locked, may temporary release BQL.
+ */
+int coroutine_fn colo_incoming_co(void);
+
 void colo_shutdown(void);
 #endif
diff --git a/migration/colo.c b/migration/colo.c
index a688ac553a..d775b442d8 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -817,7 +817,7 @@ void colo_shutdown(void)
     }
 }
 
-void *colo_process_incoming_thread(void *opaque)
+static void *colo_process_incoming_thread(void *opaque)
 {
     MigrationIncomingState *mis = opaque;
     QEMUFile *fb = NULL;
@@ -918,3 +918,40 @@ out:
     rcu_unregister_thread();
     return NULL;
 }
+
+int coroutine_fn colo_incoming_co(void)
+{
+    MigrationIncomingState *mis = migration_incoming_get_current();
+    Error *local_err = NULL;
+    QemuThread th;
+
+    assert(!qemu_mutex_iothread_locked());
+
+    if (!migration_incoming_colo_enabled()) {
+        return 0;
+    }
+
+    /* Make sure all file formats throw away their mutable metadata */
+    bdrv_activate_all(&local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        return -EINVAL;
+    }
+
+    qemu_thread_create(&th, "COLO incoming", colo_process_incoming_thread,
+                       mis, QEMU_THREAD_JOINABLE);
+
+    mis->colo_incoming_co = qemu_coroutine_self();
+    qemu_coroutine_yield();
+    mis->colo_incoming_co = NULL;
+
+    qemu_mutex_unlock_iothread();
+    /* Wait checkpoint incoming thread exit before free resource */
+    qemu_thread_join(&th);
+    qemu_mutex_lock_iothread();
+
+    /* We hold the global iothread lock, so it is safe here */
+    colo_release_ram_cache();
+
+    return 0;
+}
diff --git a/migration/migration.c b/migration/migration.c
index 23b2d187de..0d912ee0d7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -502,7 +502,6 @@ process_incoming_migration_co(void *opaque)
     MigrationIncomingState *mis = migration_incoming_get_current();
     PostcopyState ps;
     int ret;
-    Error *local_err = NULL;
 
     assert(mis->from_src_file);
     mis->largest_page_size = qemu_ram_pagesize_largest();
@@ -540,37 +539,14 @@ process_incoming_migration_co(void *opaque)
         goto fail;
     }
 
-    /* we get COLO info, and know if we are in COLO mode */
-    if (migration_incoming_colo_enabled()) {
-        QemuThread colo_incoming_thread;
-
-        /* Make sure all file formats throw away their mutable metadata */
-        bdrv_activate_all(&local_err);
-        if (local_err) {
-            error_report_err(local_err);
-            goto fail;
-        }
-
-        qemu_thread_create(&colo_incoming_thread, "COLO incoming",
-             colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE);
-
-        mis->colo_incoming_co = qemu_coroutine_self();
-        qemu_coroutine_yield();
-        mis->colo_incoming_co = NULL;
-
-        qemu_mutex_unlock_iothread();
-        /* Wait checkpoint incoming thread exit before free resource */
-        qemu_thread_join(&colo_incoming_thread);
-        qemu_mutex_lock_iothread();
-        /* We hold the global iothread lock, so it is safe here */
-        colo_release_ram_cache();
+    if (colo_incoming_co() < 0) {
+        goto fail;
     }
 
     mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
     qemu_bh_schedule(mis->bh);
     return;
 fail:
-    local_err = NULL;
     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                       MIGRATION_STATUS_FAILED);
     qemu_fclose(mis->from_src_file);
diff --git a/stubs/colo.c b/stubs/colo.c
index cf9816d368..f33379d0fd 100644
--- a/stubs/colo.c
+++ b/stubs/colo.c
@@ -10,11 +10,9 @@ void colo_shutdown(void)
 {
 }
 
-void *colo_process_incoming_thread(void *opaque)
+int coroutine_fn colo_incoming_co(void)
 {
-    error_report("Impossible happend: trying to start COLO thread when COLO "
-                 "module is not built in");
-    abort();
+    return 0;
 }
 
 void colo_checkpoint_delay_set(void)
-- 
2.34.1

[PATCH v4 09/10] migration: disallow change capabilities in COLO state

[Vladimir Sementsov-Ogievskiy]

COLO is not listed as running state in migrate_is_running(), so, it's
theoretically possible to disable colo capability in COLO state and the
unexpected error in migration_iteration_finish() is reachable.

Let's disallow that in qmp_migrate_set_capabilities. Than the error
becomes absolutely unreachable: we can get into COLO state only with
enabled capability and can't disable it while we are in COLO state. So
substitute the error by simple assertion.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 migration/migration.c | 5 +----
 migration/options.c   | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0d912ee0d7..8c5bbf3e94 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2751,10 +2751,7 @@ static void migration_iteration_finish(MigrationState *s)
         runstate_set(RUN_STATE_POSTMIGRATE);
         break;
     case MIGRATION_STATUS_COLO:
-        if (!migrate_colo()) {
-            error_report("%s: critical error: calling COLO code without "
-                         "COLO enabled", __func__);
-        }
+        assert(migrate_colo());
         migrate_start_colo_process(s);
         s->vm_was_running = true;
         /* Fallthrough */
diff --git a/migration/options.c b/migration/options.c
index 865a0214d8..f461d02eeb 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -598,7 +598,7 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     MigrationCapabilityStatusList *cap;
     bool new_caps[MIGRATION_CAPABILITY__MAX];
 
-    if (migration_is_running(s->state)) {
+    if (migration_is_running(s->state) || migration_in_colo_state()) {
         error_setg(errp, QERR_MIGRATION_ACTIVE);
         return;
     }
-- 
2.34.1

[PATCH v4 10/10] migration: block incoming colo when capability is disabled

[Vladimir Sementsov-Ogievskiy]

We generally require same set of capabilities on source and target.
Let's require x-colo capability to use COLO on target.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 migration/migration.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 8c5bbf3e94..5e162c0622 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void)
     return -ENOTSUP;
 #endif
 
+    if (!migrate_colo()) {
+        error_report("ENABLE_COLO command come in migration stream, but c-colo "
+                     "capability is not set");
+        return -EINVAL;
+    }
+
     if (ram_block_discard_disable(true)) {
         error_report("COLO: cannot disable RAM discard");
         return -EBUSY;
-- 
2.34.1

Re: [PATCH v4 03/10] build: move COLO under CONFIG_REPLICATION

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:21PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We don't allow to use x-colo capability when replication is not
> configured. So, no reason to build COLO when replication is disabled,
> it's unusable in this case.
> 
> Note also that the check in migrate_caps_check() is not the only
> restriction: some functions in migration/colo.c will just abort if
> called with not defined CONFIG_REPLICATION, for example:
> 
>     migration_iteration_finish()
>        case MIGRATION_STATUS_COLO:
>            migrate_start_colo_process()
>                colo_process_checkpoint()
>                    abort()
> 
> It could probably make sense to have possibility to enable COLO without
> REPLICATION, but this requires deeper audit of colo & replication code,
> which may be done later if needed.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Acked-by: Dr. David Alan Gilbert <dave@treblig.org>
> ---
>  hmp-commands.hx                |  2 ++
>  migration/colo.c               | 28 ------------------------
>  migration/meson.build          |  6 ++++--
>  migration/migration-hmp-cmds.c |  2 ++
>  migration/migration.c          |  6 ++++++
>  qapi/migration.json            |  9 +++++---
>  stubs/colo.c                   | 39 ++++++++++++++++++++++++++++++++++
>  stubs/meson.build              |  1 +
>  8 files changed, 60 insertions(+), 33 deletions(-)
>  create mode 100644 stubs/colo.c
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index bb85ee1d26..fbd0932232 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1035,6 +1035,7 @@ SRST
>    migration (or once already in postcopy).
>  ERST
>  
> +#ifdef CONFIG_REPLICATION
>      {
>          .name       = "x_colo_lost_heartbeat",
>          .args_type  = "",
> @@ -1043,6 +1044,7 @@ ERST
>                        "a failover or takeover is needed.",
>          .cmd = hmp_x_colo_lost_heartbeat,
>      },
> +#endif
>  
>  SRST
>  ``x_colo_lost_heartbeat``
> diff --git a/migration/colo.c b/migration/colo.c
> index c9e0b909b9..6c7c313956 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -26,9 +26,7 @@
>  #include "qemu/rcu.h"
>  #include "migration/failover.h"
>  #include "migration/ram.h"
> -#ifdef CONFIG_REPLICATION
>  #include "block/replication.h"
> -#endif
>  #include "net/colo-compare.h"
>  #include "net/colo.h"
>  #include "block/block.h"
> @@ -86,7 +84,6 @@ void colo_checkpoint_delay_set(void)
>  static void secondary_vm_do_failover(void)
>  {
>  /* COLO needs enable block-replication */
> -#ifdef CONFIG_REPLICATION
>      int old_state;
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      Error *local_err = NULL;
> @@ -151,14 +148,10 @@ static void secondary_vm_do_failover(void)
>      if (mis->migration_incoming_co) {
>          qemu_coroutine_enter(mis->migration_incoming_co);
>      }
> -#else
> -    abort();
> -#endif
>  }
>  
>  static void primary_vm_do_failover(void)
>  {
> -#ifdef CONFIG_REPLICATION
>      MigrationState *s = migrate_get_current();
>      int old_state;
>      Error *local_err = NULL;
> @@ -199,9 +192,6 @@ static void primary_vm_do_failover(void)
>  
>      /* Notify COLO thread that failover work is finished */
>      qemu_sem_post(&s->colo_exit_sem);
> -#else
> -    abort();
> -#endif
>  }
>  
>  COLOMode get_colo_mode(void)
> @@ -235,7 +225,6 @@ void colo_do_failover(void)
>      }
>  }
>  
> -#ifdef CONFIG_REPLICATION
>  void qmp_xen_set_replication(bool enable, bool primary,
>                               bool has_failover, bool failover,
>                               Error **errp)
> @@ -289,7 +278,6 @@ void qmp_xen_colo_do_checkpoint(Error **errp)
>      /* Notify all filters of all NIC to do checkpoint */
>      colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
>  }
> -#endif
>  
>  COLOStatus *qmp_query_colo_status(Error **errp)
>  {
> @@ -453,15 +441,11 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>      }
>      qemu_mutex_lock_iothread();
>  
> -#ifdef CONFIG_REPLICATION
>      replication_do_checkpoint_all(&local_err);
>      if (local_err) {
>          qemu_mutex_unlock_iothread();
>          goto out;
>      }
> -#else
> -        abort();
> -#endif
>  
>      colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND, &local_err);
>      if (local_err) {
> @@ -579,15 +563,11 @@ static void colo_process_checkpoint(MigrationState *s)
>      object_unref(OBJECT(bioc));
>  
>      qemu_mutex_lock_iothread();
> -#ifdef CONFIG_REPLICATION
>      replication_start_all(REPLICATION_MODE_PRIMARY, &local_err);
>      if (local_err) {
>          qemu_mutex_unlock_iothread();
>          goto out;
>      }
> -#else
> -        abort();
> -#endif
>  
>      vm_start();
>      qemu_mutex_unlock_iothread();
> @@ -755,7 +735,6 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>          return;
>      }
>  
> -#ifdef CONFIG_REPLICATION
>      replication_get_error_all(&local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -772,9 +751,6 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>          qemu_mutex_unlock_iothread();
>          return;
>      }
> -#else
> -    abort();
> -#endif
>      /* Notify all filters of all NIC to do checkpoint */
>      colo_notify_filters_event(COLO_EVENT_CHECKPOINT, &local_err);
>  
> @@ -881,15 +857,11 @@ void *colo_process_incoming_thread(void *opaque)
>      object_unref(OBJECT(bioc));
>  
>      qemu_mutex_lock_iothread();
> -#ifdef CONFIG_REPLICATION
>      replication_start_all(REPLICATION_MODE_SECONDARY, &local_err);
>      if (local_err) {
>          qemu_mutex_unlock_iothread();
>          goto out;
>      }
> -#else
> -        abort();
> -#endif
>      vm_start();
>      qemu_mutex_unlock_iothread();
>      trace_colo_vm_state_change("stop", "run");
> diff --git a/migration/meson.build b/migration/meson.build
> index 480ff6854a..9c6a8e65d3 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -13,8 +13,6 @@ softmmu_ss.add(files(
>    'block-dirty-bitmap.c',
>    'channel.c',
>    'channel-block.c',
> -  'colo-failover.c',
> -  'colo.c',
>    'exec.c',
>    'fd.c',
>    'global_state.c',
> @@ -30,6 +28,10 @@ softmmu_ss.add(files(
>    'threadinfo.c',
>  ), gnutls)
>  
> +if get_option('replication').allowed()
> +  softmmu_ss.add(files('colo-failover.c', 'colo.c'))
> +endif
> +
>  softmmu_ss.add(when: rdma, if_true: files('rdma.c'))
>  if get_option('live_block_migration').allowed()
>    softmmu_ss.add(files('block.c'))
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 4e9f00e7dc..9885d7c9f7 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -643,6 +643,7 @@ void hmp_migrate_start_postcopy(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, err);
>  }
>  
> +#ifdef CONFIG_REPLICATION
>  void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> @@ -650,6 +651,7 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
>      qmp_x_colo_lost_heartbeat(&err);
>      hmp_handle_error(mon, err);
>  }
> +#endif
>  
>  typedef struct HMPMigrationStatus {
>      QEMUTimer *timer;
> diff --git a/migration/migration.c b/migration/migration.c
> index abcadbb619..0c14837cd3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -389,6 +389,12 @@ void migration_incoming_disable_colo(void)
>  
>  int migration_incoming_enable_colo(void)
>  {
> +#ifndef CONFIG_REPLICATION
> +    error_report("ENABLE_COLO command come in migration stream, but COLO "
> +                 "module is not built in");
> +    return -ENOTSUP;
> +#endif
> +
>      if (ram_block_discard_disable(true)) {
>          error_report("COLO: cannot disable RAM discard");
>          return -EBUSY;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 2c35b7b9cf..422a6b2613 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1381,7 +1381,8 @@
>  #
>  ##
>  { 'command': 'x-colo-lost-heartbeat',
> -  'features': [ 'unstable' ] }
> +  'features': [ 'unstable' ],
> +  'if': 'CONFIG_REPLICATION' }
>  
>  ##
>  # @migrate_cancel:
> @@ -1657,7 +1658,8 @@
>  ##
>  { 'struct': 'COLOStatus',
>    'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
> -            'reason': 'COLOExitReason' } }
> +            'reason': 'COLOExitReason' },
> +  'if': 'CONFIG_REPLICATION' }
>  
>  ##
>  # @query-colo-status:
> @@ -1674,7 +1676,8 @@
>  # Since: 3.1
>  ##
>  { 'command': 'query-colo-status',
> -  'returns': 'COLOStatus' }
> +  'returns': 'COLOStatus',
> +  'if': 'CONFIG_REPLICATION' }

I still see a bunch of other colo related definitions around in the qapi
schema, e.g. COLOExitReason.  Is it intended to leave them as is?

>  
>  ##
>  # @migrate-recover:
> diff --git a/stubs/colo.c b/stubs/colo.c
> new file mode 100644
> index 0000000000..cf9816d368
> --- /dev/null
> +++ b/stubs/colo.c
> @@ -0,0 +1,39 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +#include "migration/colo.h"
> +#include "migration/migration.h"
> +#include "qemu/error-report.h"
> +#include "qapi/qapi-commands-migration.h"
> +
> +void colo_shutdown(void)
> +{
> +}
> +
> +void *colo_process_incoming_thread(void *opaque)
> +{
> +    error_report("Impossible happend: trying to start COLO thread when COLO "
> +                 "module is not built in");
> +    abort();
> +}
> +
> +void colo_checkpoint_delay_set(void)
> +{
> +}
> +
> +void migrate_start_colo_process(MigrationState *s)
> +{
> +    error_report("Impossible happend: trying to start COLO when COLO "
> +                 "module is not built in");
> +    abort();
> +}
> +
> +bool migration_in_colo_state(void)
> +{
> +    return false;
> +}
> +
> +bool migration_incoming_in_colo_state(void)
> +{
> +    return false;
> +}
> diff --git a/stubs/meson.build b/stubs/meson.build
> index b2b5956d97..8412cad15f 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -45,6 +45,7 @@ stub_ss.add(files('target-get-monitor-def.c'))
>  stub_ss.add(files('target-monitor-defs.c'))
>  stub_ss.add(files('trace-control.c'))
>  stub_ss.add(files('uuid.c'))
> +stub_ss.add(files('colo.c'))
>  stub_ss.add(files('vmstate.c'))
>  stub_ss.add(files('vm-stop.c'))
>  stub_ss.add(files('win32-kbd-hook.c'))
> -- 
> 2.34.1
> 

-- 
Peter Xu

Re: [PATCH v4 05/10] migration: drop colo_incoming_thread from MigrationIncomingState

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:23PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> have_colo_incoming_thread variable is unused. colo_incoming_thread can
> be local.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v4 06/10] migration: process_incoming_migration_co: simplify code flow around ret

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:24PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v4 05/10] migration: drop colo_incoming_thread from MigrationIncomingState

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> have_colo_incoming_thread variable is unused. colo_incoming_thread can
> be local.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

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

Re: [PATCH v4 06/10] migration: process_incoming_migration_co: simplify code flow around ret

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

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

Re: [PATCH v4 02/10] colo: make colo_checkpoint_notify static and provide simpler API

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> colo_checkpoint_notify() is mostly used in colo.c. Outside we use it
> once when x-checkpoint-delay migration parameter is set. So, let's
> simplify the external API to only that function - notify COLO that
> parameter was set. This make external API more robust and hides
> implementation details from external callers. Also this helps us to
> make COLO module optional in further patch (i.e. we are going to add
> possibility not build the COLO module).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

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

Re: [PATCH v4 07/10] migration: split migration_incoming_co

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:25PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Originally, migration_incoming_co was introduced by
> 25d0c16f625feb3b6
>    "migration: Switch to COLO process after finishing loadvm"
> to be able to enter from COLO code to one specific yield point, added
> by 25d0c16f625feb3b6.
> 
> Later in 923709896b1b0
>  "migration: poll the cm event for destination qemu"
> we reused this variable to wake the migration incoming coroutine from
> RDMA code.
> 
> That was doubtful idea. Entering coroutines is a very fragile thing:
> you should be absolutely sure which yield point you are going to enter.
> 
> I don't know how much is it safe to enter during qemu_loadvm_state()
> which I think what RDMA want to do. But for sure RDMA shouldn't enter
> the special COLO-related yield-point. As well, COLO code doesn't want
> to enter during qemu_loadvm_state(), it want to enter it's own specific
> yield-point.
> 
> As well, when in 8e48ac95865ac97d
>  "COLO: Add block replication into colo process" we added
> bdrv_invalidate_cache_all() call (now it's called activate_all())
> it became possible to enter the migration incoming coroutine during
> that call which is wrong too.
> 
> So, let't make these things separate and disjoint: loadvm_co for RDMA,
> non-NULL during qemu_loadvm_state(), and colo_incoming_co for COLO,
> non-NULL only around specific yield.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  migration/colo.c      | 4 ++--
>  migration/migration.c | 8 ++++++--
>  migration/migration.h | 9 ++++++++-
>  3 files changed, 16 insertions(+), 5 deletions(-)

The idea looks right to me, but I really know mostly nothing on coroutines
and also rdma+colo..

Is the other ref in rdma.c (rdma_cm_poll_handler()) still missing?

-- 
Peter Xu

Re: [PATCH v4 08/10] migration: process_incoming_migration_co(): move colo part to colo

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> +int coroutine_fn colo_incoming_co(void)
> +{
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> +    Error *local_err = NULL;
> +    QemuThread th;
> +
> +    assert(!qemu_mutex_iothread_locked());

Is this assert reverted?  I assume it wants to guarantee BQL held rather
than not..

> +
> +    if (!migration_incoming_colo_enabled()) {
> +        return 0;
> +    }
> +
> +    /* Make sure all file formats throw away their mutable metadata */
> +    bdrv_activate_all(&local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        return -EINVAL;
> +    }
> +
> +    qemu_thread_create(&th, "COLO incoming", colo_process_incoming_thread,
> +                       mis, QEMU_THREAD_JOINABLE);
> +
> +    mis->colo_incoming_co = qemu_coroutine_self();
> +    qemu_coroutine_yield();
> +    mis->colo_incoming_co = NULL;
> +
> +    qemu_mutex_unlock_iothread();
> +    /* Wait checkpoint incoming thread exit before free resource */
> +    qemu_thread_join(&th);
> +    qemu_mutex_lock_iothread();
> +
> +    /* We hold the global iothread lock, so it is safe here */
> +    colo_release_ram_cache();
> +
> +    return 0;
> +}

-- 
Peter Xu

Re: [PATCH v4 09/10] migration: disallow change capabilities in COLO state

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:27PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> COLO is not listed as running state in migrate_is_running(), so, it's
> theoretically possible to disable colo capability in COLO state and the
> unexpected error in migration_iteration_finish() is reachable.
> 
> Let's disallow that in qmp_migrate_set_capabilities. Than the error
> becomes absolutely unreachable: we can get into COLO state only with
> enabled capability and can't disable it while we are in COLO state. So
> substitute the error by simple assertion.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v4 10/10] migration: block incoming colo when capability is disabled

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:28PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We generally require same set of capabilities on source and target.
> Let's require x-colo capability to use COLO on target.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v4 02/10] colo: make colo_checkpoint_notify static and provide simpler API

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:20PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> colo_checkpoint_notify() is mostly used in colo.c. Outside we use it
> once when x-checkpoint-delay migration parameter is set. So, let's
> simplify the external API to only that function - notify COLO that
> parameter was set. This make external API more robust and hides
> implementation details from external callers. Also this helps us to
> make COLO module optional in further patch (i.e. we are going to add
> possibility not build the COLO module).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v4 08/10] migration: process_incoming_migration_co(): move colo part to colo

[Vladimir Sementsov-Ogievskiy]

On 02.05.23 23:54, Peter Xu wrote:
> On Fri, Apr 28, 2023 at 10:49:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> +int coroutine_fn colo_incoming_co(void)
>> +{
>> +    MigrationIncomingState *mis = migration_incoming_get_current();
>> +    Error *local_err = NULL;
>> +    QemuThread th;
>> +
>> +    assert(!qemu_mutex_iothread_locked());
> Is this assert reverted?  I assume it wants to guarantee BQL held rather
> than not..
> 

Oops, right.

-- 
Best regards,
Vladimir

Re: [PATCH v4 03/10] build: move COLO under CONFIG_REPLICATION

[Vladimir Sementsov-Ogievskiy]

On 02.05.23 19:41, Peter Xu wrote:
>>   ##
>>   # @query-colo-status:
>> @@ -1674,7 +1676,8 @@
>>   # Since: 3.1
>>   ##
>>   { 'command': 'query-colo-status',
>> -  'returns': 'COLOStatus' }
>> +  'returns': 'COLOStatus',
>> +  'if': 'CONFIG_REPLICATION' }
> I still see a bunch of other colo related definitions around in the qapi
> schema, e.g. COLOExitReason.  Is it intended to leave them as is?
> 

Removing them will make us to add more and more ifdefs in the code.

We decided to keep x-colo capability.
One more enum field is colo migration status - not worse than x-colo enum field, and is not possible with replication disabled

The others (like COLOExitReason) are just structure definitions - not a public API, so no reason to care about.

The exclustion is COLOStatus, we have to handle it. If not compilation fails:

qapi/qapi-commands-migration.c:821:13: error: ‘qmp_marshal_output_COLOStatus’ defined but not used [-Werror=unused-function]
   821 | static void qmp_marshal_output_COLOStatus(COLOStatus *ret_in,
       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


- COLOStatus becomes unused with replication disabled, as it is used only in migration/colo.c

-- 
Best regards,
Vladimir

Re: [PATCH v4 07/10] migration: split migration_incoming_co

[Vladimir Sementsov-Ogievskiy]

On 02.05.23 23:48, Peter Xu wrote:
> On Fri, Apr 28, 2023 at 10:49:25PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Originally, migration_incoming_co was introduced by
>> 25d0c16f625feb3b6
>>     "migration: Switch to COLO process after finishing loadvm"
>> to be able to enter from COLO code to one specific yield point, added
>> by 25d0c16f625feb3b6.
>>
>> Later in 923709896b1b0
>>   "migration: poll the cm event for destination qemu"
>> we reused this variable to wake the migration incoming coroutine from
>> RDMA code.
>>
>> That was doubtful idea. Entering coroutines is a very fragile thing:
>> you should be absolutely sure which yield point you are going to enter.
>>
>> I don't know how much is it safe to enter during qemu_loadvm_state()
>> which I think what RDMA want to do. But for sure RDMA shouldn't enter
>> the special COLO-related yield-point. As well, COLO code doesn't want
>> to enter during qemu_loadvm_state(), it want to enter it's own specific
>> yield-point.
>>
>> As well, when in 8e48ac95865ac97d
>>   "COLO: Add block replication into colo process" we added
>> bdrv_invalidate_cache_all() call (now it's called activate_all())
>> it became possible to enter the migration incoming coroutine during
>> that call which is wrong too.
>>
>> So, let't make these things separate and disjoint: loadvm_co for RDMA,
>> non-NULL during qemu_loadvm_state(), and colo_incoming_co for COLO,
>> non-NULL only around specific yield.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   migration/colo.c      | 4 ++--
>>   migration/migration.c | 8 ++++++--
>>   migration/migration.h | 9 ++++++++-
>>   3 files changed, 16 insertions(+), 5 deletions(-)
> 
> The idea looks right to me, but I really know mostly nothing on coroutines
> and also rdma+colo..
> 
> Is the other ref in rdma.c (rdma_cm_poll_handler()) still missing?
> 

Oops right.. I was building with rdma disabled. Will fix.

Thanks a lot for reviewing!

-- 
Best regards,
Vladimir

RE: [PATCH v4 01/10] block/meson.build: prefer positive condition for replication

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Saturday, April 29, 2023 3:49 AM
> To: qemu-devel@nongnu.org
> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Philippe Mathieu-
> Daudé <philmd@linaro.org>; Kevin Wolf <kwolf@redhat.com>; Hanna Reitz
> <hreitz@redhat.com>; open list:Block layer core <qemu-block@nongnu.org>
> Subject: [PATCH v4 01/10] block/meson.build: prefer positive condition for
> replication
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Lukas Straub <lukasstraub2@web.de>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

> ---
>  block/meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/meson.build b/block/meson.build index
> 382bec0e7d..b9a72e219b 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -84,7 +84,7 @@ block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-
> win32.c', 'win32-aio.c')
>  block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit])
>  block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c'))
>  block_ss.add(when: 'CONFIG_LINUX', if_true: files('nvme.c')) -if not
> get_option('replication').disabled()
> +if get_option('replication').allowed()
>    block_ss.add(files('replication.c'))
>  endif
>  block_ss.add(when: libaio, if_true: files('linux-aio.c'))
> --
> 2.34.1

RE: [PATCH v4 02/10] colo: make colo_checkpoint_notify static and provide simpler API

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Saturday, April 29, 2023 3:49 AM
> To: qemu-devel@nongnu.org
> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Zhang, Hailiang
> <zhanghailiang@xfusion.com>; Peter Xu <peterx@redhat.com>; Leonardo
> Bras <leobras@redhat.com>
> Subject: [PATCH v4 02/10] colo: make colo_checkpoint_notify static and
> provide simpler API
> 
> colo_checkpoint_notify() is mostly used in colo.c. Outside we use it once
> when x-checkpoint-delay migration parameter is set. So, let's simplify the
> external API to only that function - notify COLO that parameter was set. This
> make external API more robust and hides implementation details from
> external callers. Also this helps us to make COLO module optional in further
> patch (i.e. we are going to add possibility not build the COLO module).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

> ---
>  include/migration/colo.h |  9 ++++++++-
>  migration/colo.c         | 29 ++++++++++++++++++-----------
>  migration/options.c      |  4 +---
>  3 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/include/migration/colo.h b/include/migration/colo.h index
> 5fbe1a6d5d..7ef315473e 100644
> --- a/include/migration/colo.h
> +++ b/include/migration/colo.h
> @@ -36,6 +36,13 @@ COLOMode get_colo_mode(void);
>  /* failover */
>  void colo_do_failover(void);
> 
> -void colo_checkpoint_notify(void *opaque);
> +/*
> + * colo_checkpoint_delay_set
> + *
> + * Handles change of x-checkpoint-delay migration parameter, called
> +from
> + * migrate_params_apply() to notify COLO module about the change.
> + */
> +void colo_checkpoint_delay_set(void);
> +
>  void colo_shutdown(void);
>  #endif
> diff --git a/migration/colo.c b/migration/colo.c index
> 07bfa21fea..c9e0b909b9 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -65,6 +65,24 @@ static bool colo_runstate_is_stopped(void)
>      return runstate_check(RUN_STATE_COLO) || !runstate_is_running();  }
> 
> +static void colo_checkpoint_notify(void *opaque) {
> +    MigrationState *s = opaque;
> +    int64_t next_notify_time;
> +
> +    qemu_event_set(&s->colo_checkpoint_event);
> +    s->colo_checkpoint_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> +    next_notify_time = s->colo_checkpoint_time +
> migrate_checkpoint_delay();
> +    timer_mod(s->colo_delay_timer, next_notify_time); }
> +
> +void colo_checkpoint_delay_set(void)
> +{
> +    if (migration_in_colo_state()) {
> +        colo_checkpoint_notify(migrate_get_current());
> +    }
> +}
> +
>  static void secondary_vm_do_failover(void)  {
>  /* COLO needs enable block-replication */ @@ -644,17 +662,6 @@ out:
>      }
>  }
> 
> -void colo_checkpoint_notify(void *opaque) -{
> -    MigrationState *s = opaque;
> -    int64_t next_notify_time;
> -
> -    qemu_event_set(&s->colo_checkpoint_event);
> -    s->colo_checkpoint_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> -    next_notify_time = s->colo_checkpoint_time +
> migrate_checkpoint_delay();
> -    timer_mod(s->colo_delay_timer, next_notify_time);
> -}
> -
>  void migrate_start_colo_process(MigrationState *s)  {
>      qemu_mutex_unlock_iothread();
> diff --git a/migration/options.c b/migration/options.c index
> 53b7fc5d5d..865a0214d8 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1246,9 +1246,7 @@ static void
> migrate_params_apply(MigrateSetParameters *params, Error **errp)
> 
>      if (params->has_x_checkpoint_delay) {
>          s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
> -        if (migration_in_colo_state()) {
> -            colo_checkpoint_notify(s);
> -        }
> +        colo_checkpoint_delay_set();
>      }
> 
>      if (params->has_block_incremental) {
> --
> 2.34.1

RE: [PATCH v4 04/10] configure: add --disable-colo-proxy option

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Saturday, April 29, 2023 3:49 AM
> To: qemu-devel@nongnu.org
> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Paolo Bonzini
> <pbonzini@redhat.com>; Marc-André Lureau
> <marcandre.lureau@redhat.com>; Daniel P. Berrangé
> <berrange@redhat.com>; Thomas Huth <thuth@redhat.com>; Philippe
> Mathieu-Daudé <philmd@linaro.org>; Jason Wang <jasowang@redhat.com>
> Subject: [PATCH v4 04/10] configure: add --disable-colo-proxy option
> 
> Add option to not build filter-mirror, filter-rewriter and colo-compare when
> they are not needed.

Typo: This patch still build the filter-mirror/filter-redirector in filter-mirror.c.
Please remove the "filter-mirror" here.
Other code look good to me.

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

> 
> There could be more agile configuration, for example add separate options
> for each filter, but that may be done in future on demand. The aim of this
> patch is to make possible to disable the whole COLO Proxy subsystem.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> ---
>  meson_options.txt             |  2 ++
>  net/meson.build               | 13 ++++++++++---
>  scripts/meson-buildoptions.sh |  3 +++
>  stubs/colo-compare.c          |  7 +++++++
>  stubs/meson.build             |  1 +
>  5 files changed, 23 insertions(+), 3 deletions(-)  create mode 100644
> stubs/colo-compare.c
> 
> diff --git a/meson_options.txt b/meson_options.txt index
> 2471dd02da..b59e7ae342 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -289,6 +289,8 @@ option('live_block_migration', type: 'feature', value:
> 'auto',
>         description: 'block migration in the main migration stream')
> option('replication', type: 'feature', value: 'auto',
>         description: 'replication support')
> +option('colo_proxy', type: 'feature', value: 'auto',
> +       description: 'colo-proxy support')
>  option('bochs', type: 'feature', value: 'auto',
>         description: 'bochs image format support')  option('cloop', type: 'feature',
> value: 'auto', diff --git a/net/meson.build b/net/meson.build index
> 87afca3e93..6f4ecde57f 100644
> --- a/net/meson.build
> +++ b/net/meson.build
> @@ -1,13 +1,10 @@
>  softmmu_ss.add(files(
>    'announce.c',
>    'checksum.c',
> -  'colo-compare.c',
> -  'colo.c',
>    'dump.c',
>    'eth.c',
>    'filter-buffer.c',
>    'filter-mirror.c',
> -  'filter-rewriter.c',
>    'filter.c',
>    'hub.c',
>    'net-hmp-cmds.c',
> @@ -19,6 +16,16 @@ softmmu_ss.add(files(
>    'util.c',
>  ))
> 
> +if get_option('replication').allowed() or \
> +    get_option('colo_proxy').allowed()
> +  softmmu_ss.add(files('colo-compare.c'))
> +  softmmu_ss.add(files('colo.c'))
> +endif
> +
> +if get_option('colo_proxy').allowed()
> +  softmmu_ss.add(files('filter-rewriter.c'))
> +endif
> +
>  softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
> 
>  if have_l2tpv3
> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> index d4369a3ad8..036047ce6f 100644
> --- a/scripts/meson-buildoptions.sh
> +++ b/scripts/meson-buildoptions.sh
> @@ -83,6 +83,7 @@ meson_options_help() {
>    printf "%s\n" '  capstone        Whether and how to find the capstone library'
>    printf "%s\n" '  cloop           cloop image format support'
>    printf "%s\n" '  cocoa           Cocoa user interface (macOS only)'
> +  printf "%s\n" '  colo-proxy      colo-proxy support'
>    printf "%s\n" '  coreaudio       CoreAudio sound support'
>    printf "%s\n" '  crypto-afalg    Linux AF_ALG crypto backend driver'
>    printf "%s\n" '  curl            CURL block device driver'
> @@ -236,6 +237,8 @@ _meson_option_parse() {
>      --disable-cloop) printf "%s" -Dcloop=disabled ;;
>      --enable-cocoa) printf "%s" -Dcocoa=enabled ;;
>      --disable-cocoa) printf "%s" -Dcocoa=disabled ;;
> +    --enable-colo-proxy) printf "%s" -Dcolo_proxy=enabled ;;
> +    --disable-colo-proxy) printf "%s" -Dcolo_proxy=disabled ;;
>      --enable-coreaudio) printf "%s" -Dcoreaudio=enabled ;;
>      --disable-coreaudio) printf "%s" -Dcoreaudio=disabled ;;
>      --enable-coroutine-pool) printf "%s" -Dcoroutine_pool=true ;; diff --git
> a/stubs/colo-compare.c b/stubs/colo-compare.c new file mode 100644 index
> 0000000000..ec726665be
> --- /dev/null
> +++ b/stubs/colo-compare.c
> @@ -0,0 +1,7 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +
> +void colo_compare_cleanup(void)
> +{
> +}
> diff --git a/stubs/meson.build b/stubs/meson.build index
> 8412cad15f..a56645e2f7 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -46,6 +46,7 @@ stub_ss.add(files('target-monitor-defs.c'))
>  stub_ss.add(files('trace-control.c'))
>  stub_ss.add(files('uuid.c'))
>  stub_ss.add(files('colo.c'))
> +stub_ss.add(files('colo-compare.c'))
>  stub_ss.add(files('vmstate.c'))
>  stub_ss.add(files('vm-stop.c'))
>  stub_ss.add(files('win32-kbd-hook.c'))
> --
> 2.34.1

RE: [PATCH v4 05/10] migration: drop colo_incoming_thread from MigrationIncomingState

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Saturday, April 29, 2023 3:49 AM
> To: qemu-devel@nongnu.org
> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu
> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
> Subject: [PATCH v4 05/10] migration: drop colo_incoming_thread from
> MigrationIncomingState
> 
> have_colo_incoming_thread variable is unused. colo_incoming_thread can
> be local.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

> ---
>  migration/migration.c | 7 ++++---
>  migration/migration.h | 2 --
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c index
> 0c14837cd3..d4fa1a853c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -535,6 +535,8 @@ process_incoming_migration_co(void *opaque)
> 
>      /* we get COLO info, and know if we are in COLO mode */
>      if (!ret && migration_incoming_colo_enabled()) {
> +        QemuThread colo_incoming_thread;
> +
>          /* Make sure all file formats throw away their mutable metadata */
>          bdrv_activate_all(&local_err);
>          if (local_err) {
> @@ -542,14 +544,13 @@ process_incoming_migration_co(void *opaque)
>              goto fail;
>          }
> 
> -        qemu_thread_create(&mis->colo_incoming_thread, "COLO incoming",
> +        qemu_thread_create(&colo_incoming_thread, "COLO incoming",
>               colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE);
> -        mis->have_colo_incoming_thread = true;
>          qemu_coroutine_yield();
> 
>          qemu_mutex_unlock_iothread();
>          /* Wait checkpoint incoming thread exit before free resource */
> -        qemu_thread_join(&mis->colo_incoming_thread);
> +        qemu_thread_join(&colo_incoming_thread);
>          qemu_mutex_lock_iothread();
>          /* We hold the global iothread lock, so it is safe here */
>          colo_release_ram_cache();
> diff --git a/migration/migration.h b/migration/migration.h index
> 3a918514e7..7721c7658b 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -162,8 +162,6 @@ struct MigrationIncomingState {
> 
>      int state;
> 
> -    bool have_colo_incoming_thread;
> -    QemuThread colo_incoming_thread;
>      /* The coroutine we should enter (back) after failover */
>      Coroutine *migration_incoming_co;
>      QemuSemaphore colo_incoming_sem;
> --
> 2.34.1

RE: [PATCH v4 06/10] migration: process_incoming_migration_co: simplify code flow around ret

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Saturday, April 29, 2023 3:49 AM
> To: qemu-devel@nongnu.org
> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu
> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
> Subject: [PATCH v4 06/10] migration: process_incoming_migration_co:
> simplify code flow around ret
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

> ---
>  migration/migration.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c index
> d4fa1a853c..8db0892317 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -533,8 +533,13 @@ process_incoming_migration_co(void *opaque)
>          /* Else if something went wrong then just fall out of the normal exit */
>      }
> 
> +    if (ret < 0) {
> +        error_report("load of migration failed: %s", strerror(-ret));
> +        goto fail;
> +    }
> +
>      /* we get COLO info, and know if we are in COLO mode */
> -    if (!ret && migration_incoming_colo_enabled()) {
> +    if (migration_incoming_colo_enabled()) {
>          QemuThread colo_incoming_thread;
> 
>          /* Make sure all file formats throw away their mutable metadata */
> @@ -556,10 +561,6 @@ process_incoming_migration_co(void *opaque)
>          colo_release_ram_cache();
>      }
> 
> -    if (ret < 0) {
> -        error_report("load of migration failed: %s", strerror(-ret));
> -        goto fail;
> -    }
>      mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
>      qemu_bh_schedule(mis->bh);
>      mis->migration_incoming_co = NULL;
> --
> 2.34.1

RE: [PATCH v4 07/10] migration: split migration_incoming_co

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Thursday, May 4, 2023 6:52 AM
> To: Peter Xu <peterx@redhat.com>
> Cc: qemu-devel@nongnu.org; lukasstraub2@web.de; quintela@redhat.com;
> Zhang, Chen <chen.zhang@intel.com>; Zhang, Hailiang
> <zhanghailiang@xfusion.com>; Leonardo Bras <leobras@redhat.com>
> Subject: Re: [PATCH v4 07/10] migration: split migration_incoming_co
> 
> On 02.05.23 23:48, Peter Xu wrote:
> > On Fri, Apr 28, 2023 at 10:49:25PM +0300, Vladimir Sementsov-Ogievskiy
> wrote:
> >> Originally, migration_incoming_co was introduced by
> >> 25d0c16f625feb3b6
> >>     "migration: Switch to COLO process after finishing loadvm"
> >> to be able to enter from COLO code to one specific yield point, added
> >> by 25d0c16f625feb3b6.
> >>
> >> Later in 923709896b1b0
> >>   "migration: poll the cm event for destination qemu"
> >> we reused this variable to wake the migration incoming coroutine from
> >> RDMA code.
> >>
> >> That was doubtful idea. Entering coroutines is a very fragile thing:
> >> you should be absolutely sure which yield point you are going to enter.
> >>
> >> I don't know how much is it safe to enter during qemu_loadvm_state()
> >> which I think what RDMA want to do. But for sure RDMA shouldn't enter
> >> the special COLO-related yield-point. As well, COLO code doesn't want
> >> to enter during qemu_loadvm_state(), it want to enter it's own
> >> specific yield-point.
> >>
> >> As well, when in 8e48ac95865ac97d
> >>   "COLO: Add block replication into colo process" we added
> >> bdrv_invalidate_cache_all() call (now it's called activate_all()) it
> >> became possible to enter the migration incoming coroutine during that
> >> call which is wrong too.
> >>
> >> So, let't make these things separate and disjoint: loadvm_co for
> >> RDMA, non-NULL during qemu_loadvm_state(), and colo_incoming_co for
> >> COLO, non-NULL only around specific yield.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >> <vsementsov@yandex-team.ru>
> >> ---
> >>   migration/colo.c      | 4 ++--
> >>   migration/migration.c | 8 ++++++--
> >>   migration/migration.h | 9 ++++++++-
> >>   3 files changed, 16 insertions(+), 5 deletions(-)
> >
> > The idea looks right to me, but I really know mostly nothing on
> > coroutines and also rdma+colo..
> >
> > Is the other ref in rdma.c (rdma_cm_poll_handler()) still missing?
> >
> 
> Oops right.. I was building with rdma disabled. Will fix.
> 
> Thanks a lot for reviewing!
> 

Yes, I know some people and company try to enable COLO with RDMA.
But in my side, I haven't tried this way yet.

Thanks
Chen

> --
> Best regards,
> Vladimir

RE: [PATCH v4 09/10] migration: disallow change capabilities in COLO state

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Saturday, April 29, 2023 3:49 AM
> To: qemu-devel@nongnu.org
> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu
> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
> Subject: [PATCH v4 09/10] migration: disallow change capabilities in COLO
> state
> 
> COLO is not listed as running state in migrate_is_running(), so, it's
> theoretically possible to disable colo capability in COLO state and the
> unexpected error in migration_iteration_finish() is reachable.
> 
> Let's disallow that in qmp_migrate_set_capabilities. Than the error becomes
> absolutely unreachable: we can get into COLO state only with enabled
> capability and can't disable it while we are in COLO state. So substitute the
> error by simple assertion.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  migration/migration.c | 5 +----
>  migration/options.c   | 2 +-
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c index
> 0d912ee0d7..8c5bbf3e94 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2751,10 +2751,7 @@ static void
> migration_iteration_finish(MigrationState *s)
>          runstate_set(RUN_STATE_POSTMIGRATE);
>          break;
>      case MIGRATION_STATUS_COLO:
> -        if (!migrate_colo()) {
> -            error_report("%s: critical error: calling COLO code without "
> -                         "COLO enabled", __func__);
> -        }
> +        assert(migrate_colo());
>          migrate_start_colo_process(s);
>          s->vm_was_running = true;
>          /* Fallthrough */
> diff --git a/migration/options.c b/migration/options.c index
> 865a0214d8..f461d02eeb 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -598,7 +598,7 @@ void
> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>      MigrationCapabilityStatusList *cap;
>      bool new_caps[MIGRATION_CAPABILITY__MAX];
> 
> -    if (migration_is_running(s->state)) {
> +    if (migration_is_running(s->state) || migration_in_colo_state()) {

Make the "MIGRATION_STATUS_COLO" into the " migration_is_running()" is a better way?
Like the "migration_is_setup_ot_active()".

Thanks
Chen

>          error_setg(errp, QERR_MIGRATION_ACTIVE);
>          return;
>      }
> --
> 2.34.1

Re: [PATCH v4 09/10] migration: disallow change capabilities in COLO state

[Vladimir Sementsov-Ogievskiy]

On 04.05.23 11:09, Zhang, Chen wrote:
> 
> 
>> -----Original Message-----
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Sent: Saturday, April 29, 2023 3:49 AM
>> To: qemu-devel@nongnu.org
>> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
>> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu
>> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
>> Subject: [PATCH v4 09/10] migration: disallow change capabilities in COLO
>> state
>>
>> COLO is not listed as running state in migrate_is_running(), so, it's
>> theoretically possible to disable colo capability in COLO state and the
>> unexpected error in migration_iteration_finish() is reachable.
>>
>> Let's disallow that in qmp_migrate_set_capabilities. Than the error becomes
>> absolutely unreachable: we can get into COLO state only with enabled
>> capability and can't disable it while we are in COLO state. So substitute the
>> error by simple assertion.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   migration/migration.c | 5 +----
>>   migration/options.c   | 2 +-
>>   2 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c index
>> 0d912ee0d7..8c5bbf3e94 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2751,10 +2751,7 @@ static void
>> migration_iteration_finish(MigrationState *s)
>>           runstate_set(RUN_STATE_POSTMIGRATE);
>>           break;
>>       case MIGRATION_STATUS_COLO:
>> -        if (!migrate_colo()) {
>> -            error_report("%s: critical error: calling COLO code without "
>> -                         "COLO enabled", __func__);
>> -        }
>> +        assert(migrate_colo());
>>           migrate_start_colo_process(s);
>>           s->vm_was_running = true;
>>           /* Fallthrough */
>> diff --git a/migration/options.c b/migration/options.c index
>> 865a0214d8..f461d02eeb 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -598,7 +598,7 @@ void
>> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>>       MigrationCapabilityStatusList *cap;
>>       bool new_caps[MIGRATION_CAPABILITY__MAX];
>>
>> -    if (migration_is_running(s->state)) {
>> +    if (migration_is_running(s->state) || migration_in_colo_state()) {
> 
> Make the "MIGRATION_STATUS_COLO" into the " migration_is_running()" is a better way?

I wasn't sure that that's correct.. migration_is_running() is used in several places, to do so, I'd have to analyze them all.

> Like the "migration_is_setup_ot_active()".
> 
> Thanks
> Chen
> 
>>           error_setg(errp, QERR_MIGRATION_ACTIVE);
>>           return;
>>       }
>> --
>> 2.34.1
> 

-- 
Best regards,
Vladimir

RE: [PATCH v4 09/10] migration: disallow change capabilities in COLO state

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Thursday, May 4, 2023 4:23 PM
> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
> Cc: lukasstraub2@web.de; quintela@redhat.com; Peter Xu
> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
> Subject: Re: [PATCH v4 09/10] migration: disallow change capabilities in COLO
> state
> 
> On 04.05.23 11:09, Zhang, Chen wrote:
> >
> >
> >> -----Original Message-----
> >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> Sent: Saturday, April 29, 2023 3:49 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
> >> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu
> >> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
> >> Subject: [PATCH v4 09/10] migration: disallow change capabilities in
> >> COLO state
> >>
> >> COLO is not listed as running state in migrate_is_running(), so, it's
> >> theoretically possible to disable colo capability in COLO state and
> >> the unexpected error in migration_iteration_finish() is reachable.
> >>
> >> Let's disallow that in qmp_migrate_set_capabilities. Than the error
> >> becomes absolutely unreachable: we can get into COLO state only with
> >> enabled capability and can't disable it while we are in COLO state.
> >> So substitute the error by simple assertion.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >> <vsementsov@yandex-team.ru>
> >> ---
> >>   migration/migration.c | 5 +----
> >>   migration/options.c   | 2 +-
> >>   2 files changed, 2 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/migration/migration.c b/migration/migration.c index
> >> 0d912ee0d7..8c5bbf3e94 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -2751,10 +2751,7 @@ static void
> >> migration_iteration_finish(MigrationState *s)
> >>           runstate_set(RUN_STATE_POSTMIGRATE);
> >>           break;
> >>       case MIGRATION_STATUS_COLO:
> >> -        if (!migrate_colo()) {
> >> -            error_report("%s: critical error: calling COLO code without "
> >> -                         "COLO enabled", __func__);
> >> -        }
> >> +        assert(migrate_colo());
> >>           migrate_start_colo_process(s);
> >>           s->vm_was_running = true;
> >>           /* Fallthrough */
> >> diff --git a/migration/options.c b/migration/options.c index
> >> 865a0214d8..f461d02eeb 100644
> >> --- a/migration/options.c
> >> +++ b/migration/options.c
> >> @@ -598,7 +598,7 @@ void
> >> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
> >>       MigrationCapabilityStatusList *cap;
> >>       bool new_caps[MIGRATION_CAPABILITY__MAX];
> >>
> >> -    if (migration_is_running(s->state)) {
> >> +    if (migration_is_running(s->state) || migration_in_colo_state())
> >> + {
> >
> > Make the "MIGRATION_STATUS_COLO" into the " migration_is_running()"
> is a better way?
> 
> I wasn't sure that that's correct.. migration_is_running() is used in several
> places, to do so, I'd have to analyze them all.

Actually, when running in the "MIGRATION_STATUS_COLO" means QEMU can not
do a normal migration at the same time.
Juan have any comments here?

Thanks
Chen

> 
> > Like the "migration_is_setup_ot_active()".
> >
> > Thanks
> > Chen
> >
> >>           error_setg(errp, QERR_MIGRATION_ACTIVE);
> >>           return;
> >>       }
> >> --
> >> 2.34.1
> >
> 
> --
> Best regards,
> Vladimir

RE: [PATCH v4 10/10] migration: block incoming colo when capability is disabled

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Saturday, April 29, 2023 3:49 AM
> To: qemu-devel@nongnu.org
> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu
> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
> Subject: [PATCH v4 10/10] migration: block incoming colo when capability is
> disabled
> 
> We generally require same set of capabilities on source and target.
> Let's require x-colo capability to use COLO on target.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

 ---
>  migration/migration.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c index
> 8c5bbf3e94..5e162c0622 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void)
>      return -ENOTSUP;
>  #endif
> 
> +    if (!migrate_colo()) {
> +        error_report("ENABLE_COLO command come in migration stream, but
> c-colo "
> +                     "capability is not set");
> +        return -EINVAL;
> +    }
> +
>      if (ram_block_discard_disable(true)) {
>          error_report("COLO: cannot disable RAM discard");
>          return -EBUSY;
> --
> 2.34.1

Re: [PATCH v4 10/10] migration: block incoming colo when capability is disabled

[Lukas Straub]

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

On Fri, 28 Apr 2023 22:49:28 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:

> We generally require same set of capabilities on source and target.
> Let's require x-colo capability to use COLO on target.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Good patch, this is needed anyway for COLO with multifd.

Also, it allows to remove a some code, like
migration_incoming_enable_colo(), qemu_savevm_send_colo_enable() etc.
I will send patches for that. Or you can do it if you like.

Please update the docs like below, then:
Reviewed-by: Lukas Straub <lukasstraub2@web.de>

diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt
index 8ec653f81c..2e760a4aee 100644
--- a/docs/COLO-FT.txt
+++ b/docs/COLO-FT.txt
@@ -210,6 +210,7 @@ children.0=childs0 \
 
 3. On Secondary VM's QEMU monitor, issue command
 {"execute":"qmp_capabilities"}
+{"execute": "migrate-set-capabilities", "arguments": {"capabilities": [ {"capability": "x-colo", "state": true } ] } }
 {"execute": "nbd-server-start", "arguments": {"addr": {"type": "inet", "data": {"host": "0.0.0.0", "port": "9999"} } } }
 {"execute": "nbd-server-add", "arguments": {"device": "parent0", "writable": true } }


> ---
>  migration/migration.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 8c5bbf3e94..5e162c0622 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void)
>      return -ENOTSUP;
>  #endif
>  
> +    if (!migrate_colo()) {
> +        error_report("ENABLE_COLO command come in migration stream, but c-colo "
> +                     "capability is not set");
> +        return -EINVAL;
> +    }
> +
>      if (ram_block_discard_disable(true)) {
>          error_report("COLO: cannot disable RAM discard");
>          return -EBUSY;



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 10/10] migration: block incoming colo when capability is disabled

[Vladimir Sementsov-Ogievskiy]

On 05.05.23 01:10, Lukas Straub wrote:
> On Fri, 28 Apr 2023 22:49:28 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> 
>> We generally require same set of capabilities on source and target.
>> Let's require x-colo capability to use COLO on target.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Good patch, this is needed anyway for COLO with multifd.
> 
> Also, it allows to remove a some code, like
> migration_incoming_enable_colo(), qemu_savevm_send_colo_enable() etc.
> I will send patches for that.

Great! But with such changes we should be careful to not break compatibility between versions.. On the other hand, x-colo - is still experimental with that x-, so there is no guarantee how it works.

> Or you can do it if you like.

To be honest, I don't :)

> 
> Please update the docs like below, then:
> Reviewed-by: Lukas Straub <lukasstraub2@web.de>
> 
> diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt
> index 8ec653f81c..2e760a4aee 100644
> --- a/docs/COLO-FT.txt
> +++ b/docs/COLO-FT.txt
> @@ -210,6 +210,7 @@ children.0=childs0 \
>   
>   3. On Secondary VM's QEMU monitor, issue command
>   {"execute":"qmp_capabilities"}
> +{"execute": "migrate-set-capabilities", "arguments": {"capabilities": [ {"capability": "x-colo", "state": true } ] } }
>   {"execute": "nbd-server-start", "arguments": {"addr": {"type": "inet", "data": {"host": "0.0.0.0", "port": "9999"} } } }
>   {"execute": "nbd-server-add", "arguments": {"device": "parent0", "writable": true } }
> 

I'll resend with this addition, thanks for reviewing!

> 
>> ---
>>   migration/migration.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 8c5bbf3e94..5e162c0622 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void)
>>       return -ENOTSUP;
>>   #endif
>>   
>> +    if (!migrate_colo()) {
>> +        error_report("ENABLE_COLO command come in migration stream, but c-colo "
>> +                     "capability is not set");
>> +        return -EINVAL;
>> +    }
>> +
>>       if (ram_block_discard_disable(true)) {
>>           error_report("COLO: cannot disable RAM discard");
>>           return -EBUSY;
> 
> 
> 

-- 
Best regards,
Vladimir

Re: [PATCH v4 10/10] migration: block incoming colo when capability is disabled

[Lukas Straub]

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

On Fri, 5 May 2023 01:30:34 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:

> On 05.05.23 01:10, Lukas Straub wrote:
> > On Fri, 28 Apr 2023 22:49:28 +0300
> > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> >   
> >> We generally require same set of capabilities on source and target.
> >> Let's require x-colo capability to use COLO on target.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>  
> > 
> > Good patch, this is needed anyway for COLO with multifd.
> > 
> > Also, it allows to remove a some code, like
> > migration_incoming_enable_colo(), qemu_savevm_send_colo_enable() etc.
> > I will send patches for that.  
> 
> Great! But with such changes we should be careful to not break compatibility between versions.. On the other hand, x-colo - is still experimental with that x-, so there is no guarantee how it works.

Given COLO's usecase, I think it should only be run with the same qemu
version on both sides anyway. Maybe we should even enforce that
somehow, while we're at it doing breaking changes.

For upgrading qemu without downtime, normal migration can still be used.

Zhang Cheng, what do you think?

> > Or you can do it if you like.  
> 
> To be honest, I don't :)
> 
> > 
> > Please update the docs like below, then:
> > Reviewed-by: Lukas Straub <lukasstraub2@web.de>
> > 
> > diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt
> > index 8ec653f81c..2e760a4aee 100644
> > --- a/docs/COLO-FT.txt
> > +++ b/docs/COLO-FT.txt
> > @@ -210,6 +210,7 @@ children.0=childs0 \
> >   
> >   3. On Secondary VM's QEMU monitor, issue command
> >   {"execute":"qmp_capabilities"}
> > +{"execute": "migrate-set-capabilities", "arguments": {"capabilities": [ {"capability": "x-colo", "state": true } ] } }
> >   {"execute": "nbd-server-start", "arguments": {"addr": {"type": "inet", "data": {"host": "0.0.0.0", "port": "9999"} } } }
> >   {"execute": "nbd-server-add", "arguments": {"device": "parent0", "writable": true } }
> >   
> 
> I'll resend with this addition, thanks for reviewing!
> 
> >   
> >> ---
> >>   migration/migration.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index 8c5bbf3e94..5e162c0622 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void)
> >>       return -ENOTSUP;
> >>   #endif
> >>   
> >> +    if (!migrate_colo()) {
> >> +        error_report("ENABLE_COLO command come in migration stream, but c-colo "
> >> +                     "capability is not set");
> >> +        return -EINVAL;
> >> +    }
> >> +
> >>       if (ram_block_discard_disable(true)) {
> >>           error_report("COLO: cannot disable RAM discard");
> >>           return -EBUSY;  
> > 
> > 
> >   
> 



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v4 10/10] migration: block incoming colo when capability is disabled

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Friday, May 5, 2023 6:46 AM
> To: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Cc: qemu-devel@nongnu.org; quintela@redhat.com; Zhang, Chen
> <chen.zhang@intel.com>; Peter Xu <peterx@redhat.com>; Leonardo Bras
> <leobras@redhat.com>
> Subject: Re: [PATCH v4 10/10] migration: block incoming colo when capability
> is disabled
> 
> On Fri, 5 May 2023 01:30:34 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> 
> > On 05.05.23 01:10, Lukas Straub wrote:
> > > On Fri, 28 Apr 2023 22:49:28 +0300
> > > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> > >
> > >> We generally require same set of capabilities on source and target.
> > >> Let's require x-colo capability to use COLO on target.
> > >>
> > >> Signed-off-by: Vladimir Sementsov-Ogievskiy
> > >> <vsementsov@yandex-team.ru>
> > >
> > > Good patch, this is needed anyway for COLO with multifd.
> > >
> > > Also, it allows to remove a some code, like
> > > migration_incoming_enable_colo(), qemu_savevm_send_colo_enable()
> etc.
> > > I will send patches for that.
> >
> > Great! But with such changes we should be careful to not break
> compatibility between versions.. On the other hand, x-colo - is still
> experimental with that x-, so there is no guarantee how it works.
> 
> Given COLO's usecase, I think it should only be run with the same qemu
> version on both sides anyway. Maybe we should even enforce that somehow,
> while we're at it doing breaking changes.
> 
> For upgrading qemu without downtime, normal migration can still be used.
> 
> Zhang Cheng, what do you think?

It's OK for me. We can add the same version requirement comments to the docs/COLO-FT.txt.

Thanks
Chen

> 
> > > Or you can do it if you like.
> >
> > To be honest, I don't :)
> >
> > >
> > > Please update the docs like below, then:
> > > Reviewed-by: Lukas Straub <lukasstraub2@web.de>
> > >
> > > diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt index
> > > 8ec653f81c..2e760a4aee 100644
> > > --- a/docs/COLO-FT.txt
> > > +++ b/docs/COLO-FT.txt
> > > @@ -210,6 +210,7 @@ children.0=childs0 \
> > >
> > >   3. On Secondary VM's QEMU monitor, issue command
> > >   {"execute":"qmp_capabilities"}
> > > +{"execute": "migrate-set-capabilities", "arguments":
> > > +{"capabilities": [ {"capability": "x-colo", "state": true } ] } }
> > >   {"execute": "nbd-server-start", "arguments": {"addr": {"type": "inet",
> "data": {"host": "0.0.0.0", "port": "9999"} } } }
> > >   {"execute": "nbd-server-add", "arguments": {"device": "parent0",
> > > "writable": true } }
> > >
> >
> > I'll resend with this addition, thanks for reviewing!
> >
> > >
> > >> ---
> > >>   migration/migration.c | 6 ++++++
> > >>   1 file changed, 6 insertions(+)
> > >>
> > >> diff --git a/migration/migration.c b/migration/migration.c index
> > >> 8c5bbf3e94..5e162c0622 100644
> > >> --- a/migration/migration.c
> > >> +++ b/migration/migration.c
> > >> @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void)
> > >>       return -ENOTSUP;
> > >>   #endif
> > >>
> > >> +    if (!migrate_colo()) {
> > >> +        error_report("ENABLE_COLO command come in migration stream,
> but c-colo "
> > >> +                     "capability is not set");
> > >> +        return -EINVAL;
> > >> +    }
> > >> +
> > >>       if (ram_block_discard_disable(true)) {
> > >>           error_report("COLO: cannot disable RAM discard");
> > >>           return -EBUSY;
> > >
> > >
> > >
> >
> 
> 
> 
> --

RE: [PATCH v4 00/10] COLO: improve build options

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Saturday, April 29, 2023 3:49 AM
> To: qemu-devel@nongnu.org
> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
> <chen.zhang@intel.com>; vsementsov@yandex-team.ru
> Subject: [PATCH v4 00/10] COLO: improve build options
> 
> v4:
> 01: add r-b by Lukas
> 02: new
> 03: - keep x-colo capability enum value unconditional
>     - drop ifdefs in options.c and keep capability check instead
>     - update stubs
>     - add missed a-b by Dr. David
> 04: keep filter-mirror untouched, add r-b by Juan
> 
> others: new. Some further improvements of COLO module API. May be
> merged separately.
> 
> Hi all!
> 
> COLO substem seems to be useless when CONFIG_REPLICATION is unset, as
> we simply don't allow to set x-colo capability in this case. So, let's not compile
> in unreachable code and interface we cannot use when
> CONFIG_REPLICATION is unset.
> 
> Also, provide personal configure option for COLO Proxy subsystem.

This series looks good to me.
Please add the new configure option related comments to docs/COLO-FT.txt, block-replication.txt, colo-proxy.txt.

Thanks
Chen

> 
> Vladimir Sementsov-Ogievskiy (10):
>   block/meson.build: prefer positive condition for replication
>   colo: make colo_checkpoint_notify static and provide simpler API
>   build: move COLO under CONFIG_REPLICATION
>   configure: add --disable-colo-proxy option
>   migration: drop colo_incoming_thread from MigrationIncomingState
>   migration: process_incoming_migration_co: simplify code flow around
>     ret
>   migration: split migration_incoming_co
>   migration: process_incoming_migration_co(): move colo part to colo
>   migration: disallow change capabilities in COLO state
>   migration: block incoming colo when capability is disabled
> 
>  block/meson.build              |   2 +-
>  hmp-commands.hx                |   2 +
>  include/migration/colo.h       |  18 +++++-
>  meson_options.txt              |   2 +
>  migration/colo.c               | 100 +++++++++++++++++++--------------
>  migration/meson.build          |   6 +-
>  migration/migration-hmp-cmds.c |   2 +
>  migration/migration.c          |  51 +++++++----------
>  migration/migration.h          |  11 +++-
>  migration/options.c            |   6 +-
>  net/meson.build                |  13 ++++-
>  qapi/migration.json            |   9 ++-
>  scripts/meson-buildoptions.sh  |   3 +
>  stubs/colo-compare.c           |   7 +++
>  stubs/colo.c                   |  37 ++++++++++++
>  stubs/meson.build              |   2 +
>  16 files changed, 181 insertions(+), 90 deletions(-)  create mode 100644
> stubs/colo-compare.c  create mode 100644 stubs/colo.c
> 
> --
> 2.34.1

Re: [PATCH v4 00/10] COLO: improve build options

[Vladimir Sementsov-Ogievskiy]

On 05.05.23 10:56, Zhang, Chen wrote:
> 
>> -----Original Message-----
>> From: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
>> Sent: Saturday, April 29, 2023 3:49 AM
>> To:qemu-devel@nongnu.org
>> Cc:lukasstraub2@web.de;quintela@redhat.com; Zhang, Chen
>> <chen.zhang@intel.com>;vsementsov@yandex-team.ru
>> Subject: [PATCH v4 00/10] COLO: improve build options
>>
>> v4:
>> 01: add r-b by Lukas
>> 02: new
>> 03: - keep x-colo capability enum value unconditional
>>      - drop ifdefs in options.c and keep capability check instead
>>      - update stubs
>>      - add missed a-b by Dr. David
>> 04: keep filter-mirror untouched, add r-b by Juan
>>
>> others: new. Some further improvements of COLO module API. May be
>> merged separately.
>>
>> Hi all!
>>
>> COLO substem seems to be useless when CONFIG_REPLICATION is unset, as
>> we simply don't allow to set x-colo capability in this case. So, let's not compile
>> in unreachable code and interface we cannot use when
>> CONFIG_REPLICATION is unset.
>>
>> Also, provide personal configure option for COLO Proxy subsystem.
> This series looks good to me.
> Please add the new configure option related comments to docs/COLO-FT.txt, block-replication.txt, colo-proxy.txt.


Thanks! Will do.

-- 
Best regards,
Vladimir

Re: [PATCH v4 03/10] build: move COLO under CONFIG_REPLICATION

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> We don't allow to use x-colo capability when replication is not
> configured. So, no reason to build COLO when replication is disabled,
> it's unusable in this case.
>
> Note also that the check in migrate_caps_check() is not the only
> restriction: some functions in migration/colo.c will just abort if
> called with not defined CONFIG_REPLICATION, for example:
>
>     migration_iteration_finish()
>        case MIGRATION_STATUS_COLO:
>            migrate_start_colo_process()
>                colo_process_checkpoint()
>                    abort()
>
> It could probably make sense to have possibility to enable COLO without
> REPLICATION, but this requires deeper audit of colo & replication code,
> which may be done later if needed.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Acked-by: Dr. David Alan Gilbert <dave@treblig.org>

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

Re: [PATCH v4 09/10] migration: disallow change capabilities in COLO state

[Juan Quintela]

"Zhang, Chen" <chen.zhang@intel.com> wrote:
>> -----Original Message-----
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Sent: Thursday, May 4, 2023 4:23 PM
>> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
>> Cc: lukasstraub2@web.de; quintela@redhat.com; Peter Xu
>> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
>> Subject: Re: [PATCH v4 09/10] migration: disallow change capabilities in COLO
>> state
>> 
>> On 04.05.23 11:09, Zhang, Chen wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> >> Sent: Saturday, April 29, 2023 3:49 AM
>> >> To: qemu-devel@nongnu.org
>> >> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
>> >> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu
>> >> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com>
>> >> Subject: [PATCH v4 09/10] migration: disallow change capabilities in
>> >> COLO state
>> >>
>> >> COLO is not listed as running state in migrate_is_running(), so, it's
>> >> theoretically possible to disable colo capability in COLO state and
>> >> the unexpected error in migration_iteration_finish() is reachable.
>> >>
>> >> Let's disallow that in qmp_migrate_set_capabilities. Than the error
>> >> becomes absolutely unreachable: we can get into COLO state only with
>> >> enabled capability and can't disable it while we are in COLO state.
>> >> So substitute the error by simple assertion.
>> >>
>> >> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> >> <vsementsov@yandex-team.ru>
>> >> ---
>> >>   migration/migration.c | 5 +----
>> >>   migration/options.c   | 2 +-
>> >>   2 files changed, 2 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/migration/migration.c b/migration/migration.c index
>> >> 0d912ee0d7..8c5bbf3e94 100644
>> >> --- a/migration/migration.c
>> >> +++ b/migration/migration.c
>> >> @@ -2751,10 +2751,7 @@ static void
>> >> migration_iteration_finish(MigrationState *s)
>> >>           runstate_set(RUN_STATE_POSTMIGRATE);
>> >>           break;
>> >>       case MIGRATION_STATUS_COLO:
>> >> -        if (!migrate_colo()) {
>> >> -            error_report("%s: critical error: calling COLO code without "
>> >> -                         "COLO enabled", __func__);
>> >> -        }
>> >> +        assert(migrate_colo());
>> >>           migrate_start_colo_process(s);
>> >>           s->vm_was_running = true;
>> >>           /* Fallthrough */
>> >> diff --git a/migration/options.c b/migration/options.c index
>> >> 865a0214d8..f461d02eeb 100644
>> >> --- a/migration/options.c
>> >> +++ b/migration/options.c
>> >> @@ -598,7 +598,7 @@ void
>> >> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>> >>       MigrationCapabilityStatusList *cap;
>> >>       bool new_caps[MIGRATION_CAPABILITY__MAX];
>> >>
>> >> -    if (migration_is_running(s->state)) {
>> >> +    if (migration_is_running(s->state) || migration_in_colo_state())
>> >> + {
>> >
>> > Make the "MIGRATION_STATUS_COLO" into the " migration_is_running()"
>> is a better way?
>> 
>> I wasn't sure that that's correct.. migration_is_running() is used in several
>> places, to do so, I'd have to analyze them all.
>
> Actually, when running in the "MIGRATION_STATUS_COLO" means QEMU can not
> do a normal migration at the same time.
> Juan have any comments here?

My understanding of colo is pretty limited, but my mind explodes just
trying to think how many things we would have to check/do to do a
migration while in colo state.

So I guess you are right that we can't be in both at the same time.

Later, Juan.

Re: [PATCH v4 10/10] migration: block incoming colo when capability is disabled

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> We generally require same set of capabilities on source and target.
> Let's require x-colo capability to use COLO on target.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

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

I will update the docs as said by lukas.

> ---
>  migration/migration.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 8c5bbf3e94..5e162c0622 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -395,6 +395,12 @@ int migration_incoming_enable_colo(void)
>      return -ENOTSUP;
>  #endif
>  
> +    if (!migrate_colo()) {
> +        error_report("ENABLE_COLO command come in migration stream, but c-colo "
> +                     "capability is not set");
> +        return -EINVAL;
> +    }
> +
>      if (ram_block_discard_disable(true)) {
>          error_report("COLO: cannot disable RAM discard");
>          return -EBUSY;

Re: [PATCH v4 04/10] configure: add --disable-colo-proxy option

[Juan Quintela]

"Zhang, Chen" <chen.zhang@intel.com> wrote:
>> -----Original Message-----
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Sent: Saturday, April 29, 2023 3:49 AM
>> To: qemu-devel@nongnu.org
>> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
>> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Paolo Bonzini
>> <pbonzini@redhat.com>; Marc-André Lureau
>> <marcandre.lureau@redhat.com>; Daniel P. Berrangé
>> <berrange@redhat.com>; Thomas Huth <thuth@redhat.com>; Philippe
>> Mathieu-Daudé <philmd@linaro.org>; Jason Wang <jasowang@redhat.com>
>> Subject: [PATCH v4 04/10] configure: add --disable-colo-proxy option
>> 
>> Add option to not build filter-mirror, filter-rewriter and colo-compare when
>> they are not needed.
>
> Typo: This patch still build the filter-mirror/filter-redirector in filter-mirror.c.
> Please remove the "filter-mirror" here.
> Other code look good to me.

Vladimir, I was doing this myself, with the bit attached.

But then I noticed that one needs to also disable
tests/qtest/test-filter-mirror and test-filter-rewriter.

Can you resend with that fixed?  Or I am missing something more
fundamental.

Thanks, Juan.

>> --- a/net/meson.build
>> +++ b/net/meson.build
>> @@ -1,13 +1,10 @@
>>  softmmu_ss.add(files(
>>    'announce.c',
>>    'checksum.c',
>> -  'colo-compare.c',
>> -  'colo.c',
>>    'dump.c',
>>    'eth.c',
>>    'filter-buffer.c',
>>    'filter-mirror.c',
>> -  'filter-rewriter.c',
>>    'filter.c',
>>    'hub.c',
>>    'net-hmp-cmds.c',
>> @@ -19,6 +16,16 @@ softmmu_ss.add(files(
>>    'util.c',
>>  ))
>> 
>> +if get_option('replication').allowed() or \
>> +    get_option('colo_proxy').allowed()
>> +  softmmu_ss.add(files('colo-compare.c'))
>> +  softmmu_ss.add(files('colo.c'))
>> +endif
>> +
>> +if get_option('colo_proxy').allowed()
>> +  softmmu_ss.add(files('filter-rewriter.c'))
>> +endif
>> +
>>  softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))

This is the change needed, right?

diff --git a/net/meson.build b/net/meson.build
index 6f4ecde57f..e623bb9acb 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -4,7 +4,6 @@ softmmu_ss.add(files(
   'dump.c',
   'eth.c',
   'filter-buffer.c',
-  'filter-mirror.c',
   'filter.c',
   'hub.c',
   'net-hmp-cmds.c',
@@ -23,7 +22,7 @@ if get_option('replication').allowed() or \
 endif
 
 if get_option('colo_proxy').allowed()
-  softmmu_ss.add(files('filter-rewriter.c'))
+  softmmu_ss.add(files('filter-rewriter.c', 'filter-mirror.c'))
 endif

Re: [PATCH v4 09/10] migration: disallow change capabilities in COLO state

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> COLO is not listed as running state in migrate_is_running(), so, it's
> theoretically possible to disable colo capability in COLO state and the
> unexpected error in migration_iteration_finish() is reachable.
>
> Let's disallow that in qmp_migrate_set_capabilities. Than the error
> becomes absolutely unreachable: we can get into COLO state only with
> enabled capability and can't disable it while we are in COLO state. So
> substitute the error by simple assertion.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

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

Re: [PATCH v4 04/10] configure: add --disable-colo-proxy option

[Vladimir Sementsov-Ogievskiy]

On 09.05.23 21:42, Juan Quintela wrote:
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
>>> -----Original Message-----
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> Sent: Saturday, April 29, 2023 3:49 AM
>>> To: qemu-devel@nongnu.org
>>> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
>>> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Paolo Bonzini
>>> <pbonzini@redhat.com>; Marc-André Lureau
>>> <marcandre.lureau@redhat.com>; Daniel P. Berrangé
>>> <berrange@redhat.com>; Thomas Huth <thuth@redhat.com>; Philippe
>>> Mathieu-Daudé <philmd@linaro.org>; Jason Wang <jasowang@redhat.com>
>>> Subject: [PATCH v4 04/10] configure: add --disable-colo-proxy option
>>>
>>> Add option to not build filter-mirror, filter-rewriter and colo-compare when
>>> they are not needed.
>>
>> Typo: This patch still build the filter-mirror/filter-redirector in filter-mirror.c.
>> Please remove the "filter-mirror" here.
>> Other code look good to me.
> 
> Vladimir, I was doing this myself, with the bit attached.
> 
> But then I noticed that one needs to also disable
> tests/qtest/test-filter-mirror and test-filter-rewriter.

Hmm, but we decided not touch filter-mirror in this patch, only filter-rewriter.

And there is no tests/qtest/test-filter-rewriter test.

> 
> Can you resend with that fixed?  Or I am missing something more
> fundamental.
> 
> Thanks, Juan.
> 
>>> --- a/net/meson.build
>>> +++ b/net/meson.build
>>> @@ -1,13 +1,10 @@
>>>   softmmu_ss.add(files(
>>>     'announce.c',
>>>     'checksum.c',
>>> -  'colo-compare.c',
>>> -  'colo.c',
>>>     'dump.c',
>>>     'eth.c',
>>>     'filter-buffer.c',
>>>     'filter-mirror.c',
>>> -  'filter-rewriter.c',
>>>     'filter.c',
>>>     'hub.c',
>>>     'net-hmp-cmds.c',
>>> @@ -19,6 +16,16 @@ softmmu_ss.add(files(
>>>     'util.c',
>>>   ))
>>>
>>> +if get_option('replication').allowed() or \
>>> +    get_option('colo_proxy').allowed()
>>> +  softmmu_ss.add(files('colo-compare.c'))
>>> +  softmmu_ss.add(files('colo.c'))
>>> +endif
>>> +
>>> +if get_option('colo_proxy').allowed()
>>> +  softmmu_ss.add(files('filter-rewriter.c'))
>>> +endif
>>> +
>>>   softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
> 
> This is the change needed, right?

No, we decided to keep filter-mirror as is.

> 
> diff --git a/net/meson.build b/net/meson.build
> index 6f4ecde57f..e623bb9acb 100644
> --- a/net/meson.build
> +++ b/net/meson.build
> @@ -4,7 +4,6 @@ softmmu_ss.add(files(
>     'dump.c',
>     'eth.c',
>     'filter-buffer.c',
> -  'filter-mirror.c',
>     'filter.c',
>     'hub.c',
>     'net-hmp-cmds.c',
> @@ -23,7 +22,7 @@ if get_option('replication').allowed() or \
>   endif
>   
>   if get_option('colo_proxy').allowed()
> -  softmmu_ss.add(files('filter-rewriter.c'))
> +  softmmu_ss.add(files('filter-rewriter.c', 'filter-mirror.c'))
>   endif
> 

-- 
Best regards,
Vladimir

Re: [PATCH v4 04/10] configure: add --disable-colo-proxy option

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> On 09.05.23 21:42, Juan Quintela wrote:
>> "Zhang, Chen" <chen.zhang@intel.com> wrote:
>>>> -----Original Message-----
>>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>> Sent: Saturday, April 29, 2023 3:49 AM
>>>> To: qemu-devel@nongnu.org
>>>> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
>>>> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Paolo Bonzini
>>>> <pbonzini@redhat.com>; Marc-André Lureau
>>>> <marcandre.lureau@redhat.com>; Daniel P. Berrangé
>>>> <berrange@redhat.com>; Thomas Huth <thuth@redhat.com>; Philippe
>>>> Mathieu-Daudé <philmd@linaro.org>; Jason Wang <jasowang@redhat.com>
>>>> Subject: [PATCH v4 04/10] configure: add --disable-colo-proxy option
>>>>
>>>> Add option to not build filter-mirror, filter-rewriter and colo-compare when
>>>> they are not needed.
>>>
>>> Typo: This patch still build the filter-mirror/filter-redirector in filter-mirror.c.
>>> Please remove the "filter-mirror" here.
>>> Other code look good to me.
>> Vladimir, I was doing this myself, with the bit attached.
>> But then I noticed that one needs to also disable
>> tests/qtest/test-filter-mirror and test-filter-rewriter.
>
> Hmm, but we decided not touch filter-mirror in this patch, only filter-rewriter.
>
> And there is no tests/qtest/test-filter-rewriter test.
>
>> Can you resend with that fixed?  Or I am missing something more
>> fundamental.
>> Thanks, Juan.
>> 
>>>> --- a/net/meson.build
>>>> +++ b/net/meson.build
>>>> @@ -1,13 +1,10 @@
>>>>   softmmu_ss.add(files(
>>>>     'announce.c',
>>>>     'checksum.c',
>>>> -  'colo-compare.c',
>>>> -  'colo.c',
>>>>     'dump.c',
>>>>     'eth.c',
>>>>     'filter-buffer.c',
>>>>     'filter-mirror.c',
>>>> -  'filter-rewriter.c',
>>>>     'filter.c',
>>>>     'hub.c',
>>>>     'net-hmp-cmds.c',
>>>> @@ -19,6 +16,16 @@ softmmu_ss.add(files(
>>>>     'util.c',
>>>>   ))
>>>>
>>>> +if get_option('replication').allowed() or \
>>>> +    get_option('colo_proxy').allowed()
>>>> +  softmmu_ss.add(files('colo-compare.c'))
>>>> +  softmmu_ss.add(files('colo.c'))
>>>> +endif
>>>> +
>>>> +if get_option('colo_proxy').allowed()
>>>> +  softmmu_ss.add(files('filter-rewriter.c'))
>>>> +endif
>>>> +
>>>>   softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
>> This is the change needed, right?
>
> No, we decided to keep filter-mirror as is.

Ok.  Anyways, this bit needs an ACK from Network Maintainer or go
through their tree.

Later, Juan.

Re: [PATCH v4 04/10] configure: add --disable-colo-proxy option

[Vladimir Sementsov-Ogievskiy]

On 10.05.23 15:18, Juan Quintela wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>> On 09.05.23 21:42, Juan Quintela wrote:
>>> "Zhang, Chen" <chen.zhang@intel.com> wrote:
>>>>> -----Original Message-----
>>>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>>> Sent: Saturday, April 29, 2023 3:49 AM
>>>>> To: qemu-devel@nongnu.org
>>>>> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
>>>>> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Paolo Bonzini
>>>>> <pbonzini@redhat.com>; Marc-André Lureau
>>>>> <marcandre.lureau@redhat.com>; Daniel P. Berrangé
>>>>> <berrange@redhat.com>; Thomas Huth <thuth@redhat.com>; Philippe
>>>>> Mathieu-Daudé <philmd@linaro.org>; Jason Wang <jasowang@redhat.com>
>>>>> Subject: [PATCH v4 04/10] configure: add --disable-colo-proxy option
>>>>>
>>>>> Add option to not build filter-mirror, filter-rewriter and colo-compare when
>>>>> they are not needed.
>>>>
>>>> Typo: This patch still build the filter-mirror/filter-redirector in filter-mirror.c.
>>>> Please remove the "filter-mirror" here.
>>>> Other code look good to me.
>>> Vladimir, I was doing this myself, with the bit attached.
>>> But then I noticed that one needs to also disable
>>> tests/qtest/test-filter-mirror and test-filter-rewriter.
>>
>> Hmm, but we decided not touch filter-mirror in this patch, only filter-rewriter.
>>
>> And there is no tests/qtest/test-filter-rewriter test.
>>
>>> Can you resend with that fixed?  Or I am missing something more
>>> fundamental.
>>> Thanks, Juan.
>>>
>>>>> --- a/net/meson.build
>>>>> +++ b/net/meson.build
>>>>> @@ -1,13 +1,10 @@
>>>>>    softmmu_ss.add(files(
>>>>>      'announce.c',
>>>>>      'checksum.c',
>>>>> -  'colo-compare.c',
>>>>> -  'colo.c',
>>>>>      'dump.c',
>>>>>      'eth.c',
>>>>>      'filter-buffer.c',
>>>>>      'filter-mirror.c',
>>>>> -  'filter-rewriter.c',
>>>>>      'filter.c',
>>>>>      'hub.c',
>>>>>      'net-hmp-cmds.c',
>>>>> @@ -19,6 +16,16 @@ softmmu_ss.add(files(
>>>>>      'util.c',
>>>>>    ))
>>>>>
>>>>> +if get_option('replication').allowed() or \
>>>>> +    get_option('colo_proxy').allowed()
>>>>> +  softmmu_ss.add(files('colo-compare.c'))
>>>>> +  softmmu_ss.add(files('colo.c'))
>>>>> +endif
>>>>> +
>>>>> +if get_option('colo_proxy').allowed()
>>>>> +  softmmu_ss.add(files('filter-rewriter.c'))
>>>>> +endif
>>>>> +
>>>>>    softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
>>> This is the change needed, right?
>>
>> No, we decided to keep filter-mirror as is.
> 
> Ok.  Anyways, this bit needs an ACK from Network Maintainer or go
> through their tree.
> 

I think r-b from Zhang is enough, he is maintainer of COLO Proxy, which includes filter-rewriter.

(anyway, I'll resend the rest of the series when you PULL request merged)

-- 
Best regards,
Vladimir

Re: [PATCH v4 04/10] configure: add --disable-colo-proxy option

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> On 10.05.23 15:18, Juan Quintela wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>>> On 09.05.23 21:42, Juan Quintela wrote:
>>>> "Zhang, Chen" <chen.zhang@intel.com> wrote:
>>>>>> -----Original Message-----
>>>>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>>>> Sent: Saturday, April 29, 2023 3:49 AM
>>>>>> To: qemu-devel@nongnu.org
>>>>>> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen
>>>>>> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Paolo Bonzini
>>>>>> <pbonzini@redhat.com>; Marc-André Lureau
>>>>>> <marcandre.lureau@redhat.com>; Daniel P. Berrangé
>>>>>> <berrange@redhat.com>; Thomas Huth <thuth@redhat.com>; Philippe
>>>>>> Mathieu-Daudé <philmd@linaro.org>; Jason Wang <jasowang@redhat.com>
>>>>>> Subject: [PATCH v4 04/10] configure: add --disable-colo-proxy option
>>>>>>
>>>>>> Add option to not build filter-mirror, filter-rewriter and colo-compare when
>>>>>> they are not needed.
>>>>>
>>>>> Typo: This patch still build the filter-mirror/filter-redirector in filter-mirror.c.
>>>>> Please remove the "filter-mirror" here.
>>>>> Other code look good to me.
>>>> Vladimir, I was doing this myself, with the bit attached.
>>>> But then I noticed that one needs to also disable
>>>> tests/qtest/test-filter-mirror and test-filter-rewriter.
>>>
>>> Hmm, but we decided not touch filter-mirror in this patch, only filter-rewriter.
>>>
>>> And there is no tests/qtest/test-filter-rewriter test.
>>>
>>>> Can you resend with that fixed?  Or I am missing something more
>>>> fundamental.
>>>> Thanks, Juan.
>>>>
>>>>>> --- a/net/meson.build
>>>>>> +++ b/net/meson.build
>>>>>> @@ -1,13 +1,10 @@
>>>>>>    softmmu_ss.add(files(
>>>>>>      'announce.c',
>>>>>>      'checksum.c',
>>>>>> -  'colo-compare.c',
>>>>>> -  'colo.c',
>>>>>>      'dump.c',
>>>>>>      'eth.c',
>>>>>>      'filter-buffer.c',
>>>>>>      'filter-mirror.c',
>>>>>> -  'filter-rewriter.c',
>>>>>>      'filter.c',
>>>>>>      'hub.c',
>>>>>>      'net-hmp-cmds.c',
>>>>>> @@ -19,6 +16,16 @@ softmmu_ss.add(files(
>>>>>>      'util.c',
>>>>>>    ))
>>>>>>
>>>>>> +if get_option('replication').allowed() or \
>>>>>> +    get_option('colo_proxy').allowed()
>>>>>> +  softmmu_ss.add(files('colo-compare.c'))
>>>>>> +  softmmu_ss.add(files('colo.c'))
>>>>>> +endif
>>>>>> +
>>>>>> +if get_option('colo_proxy').allowed()
>>>>>> +  softmmu_ss.add(files('filter-rewriter.c'))
>>>>>> +endif
>>>>>> +
>>>>>>    softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
>>>> This is the change needed, right?
>>>
>>> No, we decided to keep filter-mirror as is.
>> Ok.  Anyways, this bit needs an ACK from Network Maintainer or go
>> through their tree.
>> 
>
> I think r-b from Zhang is enough, he is maintainer of COLO Proxy, which includes filter-rewriter.
>
> (anyway, I'll resend the rest of the series when you PULL request merged)

Thanks.