[Qemu-devel] [PATCH v5 0/5] Remove old MigrationParams

[Qemu-devel] [PATCH v5 0/5] Remove old MigrationParams

[Juan Quintela]

Hi

changes from v4:

- Address Markus review
  * better documentation messages (thanks)
  * reorganize error checking to have small data
  * Use g_new0
  * Improve commints messages

Please review.

[v4]

- patch 1 included again. We have a 'block' capability and a
  'block_incremental' parameter
  Make Dave happy: rename shared into incremental
  Make Eric and Markus happy:

    block: off -> no block migration, we don't care about
                  block_incremental value
    block: on block_incremental: off -> no incremental block migration
    block: on block_incremental: on  -> incremental block migration

- Use visitors.
  You ask if there is not a better way than hand coding a boolean parser.
  QAPI people (a.k.a. Markus) answer to use a visitor.
  Once there, I used it also for integers, not only booleans.

- Use -b/-i parameters.  OK, I bit the bullet: You can't use
  capabilities/parameters and -b/-i: it gives you one error If you use
  -b/-i, capability parameter is cleanup after the migration ends (Who
  would have guessed that there are cases where we call
  migration_fd_error(), but not migration_fd_cleanup()
  I think this should make Peter and Markus and Dave happy.

- Integrate migrate block compile time disabling
  Well, it conflicted left and right, so I fixed it
  Once there, I disable setting/getting block capability if it is disabled.

[v3]

- Patch 1 included in pull request
- disable block_shared when we disable block_enable
- enable block_enabled when we enable block_shared

[v2]

- make migrate_block_set_* take a boolean
- disable block migration in colo to maintain semantics.

[v1]
Upon a time there were MigrationParms (only used for block migration)
and then MigrationParams used for everything else.  This series:

- create migration capabilities for block parameters
- make the migrate command line parameters to use capabilities
- remove MigrationParams completely


Dr. David Alan Gilbert (1):
  block migration: Allow compile time disable

Juan Quintela (4):
  hmp: Use visitor api for hmp_migrate_set_parameter()
  migration: Create block capability
  migration: Remove use of old MigrationParams
  migration: Remove old MigrationParams

 configure                     | 11 +++++
 hmp.c                         | 23 ++++++++--
 include/migration/block.h     | 24 +++++++++++
 include/migration/migration.h | 17 ++++----
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h       |  1 -
 include/sysemu/sysemu.h       |  3 +-
 migration/Makefile.objs       |  2 +-
 migration/block.c             | 17 +-------
 migration/colo.c              |  6 +--
 migration/migration.c         | 97 +++++++++++++++++++++++++++++++++++++++----
 migration/savevm.c            | 24 ++++-------
 qapi-schema.json              | 28 +++++++++++--
 13 files changed, 192 insertions(+), 62 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/5] hmp: Use visitor api for hmp_migrate_set_parameter()

[Juan Quintela]

We only use it for int64 at this point, I am not able to find a way to
parse an int with MiB units.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hmp.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hmp.c b/hmp.c
index b9dd933..acbbc5c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -29,6 +29,7 @@
 #include "monitor/qdev.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/string-input-visitor.h"
 #include "qapi/string-output-visitor.h"
 #include "qapi/util.h"
 #include "qapi-visit.h"
@@ -1523,8 +1524,9 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
 {
     const char *param = qdict_get_str(qdict, "parameter");
     const char *valuestr = qdict_get_str(qdict, "value");
+    Visitor *v = string_input_visitor_new(valuestr);
     uint64_t valuebw = 0;
-    long valueint = 0;
+    int64_t valueint = 0;
     Error *err = NULL;
     bool use_int_value = false;
     int i, ret;
@@ -1582,9 +1584,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
             }
 
             if (use_int_value) {
-                if (qemu_strtol(valuestr, NULL, 10, &valueint) < 0) {
-                    error_setg(&err, "Unable to parse '%s' as an int",
-                               valuestr);
+                visit_type_int(v, param, &valueint, &err);
+                if (err) {
                     goto cleanup;
                 }
                 /* Set all integers; only one has_FOO will be set, and
@@ -1608,6 +1609,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     }
 
  cleanup:
+    visit_free(v);
     if (err) {
         error_report_err(err);
     }
-- 
2.9.3

[Qemu-devel] [PATCH 2/5] migration: Create block capability

[Juan Quintela]

Create one capability for block migration and one parameter for
incremental block migration.

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

---

- address all Markus comments
- use Markus and Eric text descriptions

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hmp.c                         | 13 ++++++++
 include/migration/block.h     |  2 ++
 include/migration/migration.h |  6 ++++
 migration/migration.c         | 73 +++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json              | 28 +++++++++++++++--
 5 files changed, 119 insertions(+), 3 deletions(-)

diff --git a/hmp.c b/hmp.c
index acbbc5c..208154c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -326,6 +326,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %" PRId64 "\n",
             MigrationParameter_lookup[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY],
             params->x_checkpoint_delay);
+        assert(params->has_block_incremental);
+        monitor_printf(mon, "%s: %s\n",
+            MigrationParameter_lookup[MIGRATION_PARAMETER_BLOCK_INCREMENTAL],
+                       params->block_incremental ? "on" : "off");
     }
 
     qapi_free_MigrationParameters(params);
@@ -1527,6 +1531,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     Visitor *v = string_input_visitor_new(valuestr);
     uint64_t valuebw = 0;
     int64_t valueint = 0;
+    bool valuebool = false;
     Error *err = NULL;
     bool use_int_value = false;
     int i, ret;
@@ -1581,6 +1586,14 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                 p.has_x_checkpoint_delay = true;
                 use_int_value = true;
                 break;
+            case MIGRATION_PARAMETER_BLOCK_INCREMENTAL:
+                p.has_block_incremental = true;
+                visit_type_bool(v, param, &valuebool, &err);
+                if (err) {
+                    goto cleanup;
+                }
+                p.block_incremental = valuebool;
+                break;
             }
 
             if (use_int_value) {
diff --git a/include/migration/block.h b/include/migration/block.h
index 41a1ac8..5225af9 100644
--- a/include/migration/block.h
+++ b/include/migration/block.h
@@ -20,4 +20,6 @@ uint64_t blk_mig_bytes_transferred(void);
 uint64_t blk_mig_bytes_remaining(void);
 uint64_t blk_mig_bytes_total(void);
 
+void migrate_set_block_enabled(bool value, Error **errp);
+
 #endif /* MIGRATION_BLOCK_H */
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 49ec501..024a048 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -153,6 +153,9 @@ struct MigrationState
 
     /* The last error that occurred */
     Error *error;
+    /* Do we have to clean up -b/-i from old migrate parameters */
+    /* This feature is deprecated and will be removed */
+    bool must_remove_block_options;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
@@ -265,6 +268,9 @@ bool migrate_colo_enabled(void);
 
 int64_t xbzrle_cache_resize(int64_t new_size);
 
+bool migrate_use_block(void);
+bool migrate_use_block_incremental(void);
+
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
diff --git a/migration/migration.c b/migration/migration.c
index 0304c01..f2bb448 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -592,6 +592,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->downtime_limit = s->parameters.downtime_limit;
     params->has_x_checkpoint_delay = true;
     params->x_checkpoint_delay = s->parameters.x_checkpoint_delay;
+    params->has_block_incremental = true;
+    params->block_incremental = s->parameters.block_incremental;
 
     return params;
 }
@@ -900,6 +902,9 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
             colo_checkpoint_notify(s);
         }
     }
+    if (params->has_block_incremental) {
+        s->parameters.block_incremental = params->block_incremental;
+    }
 }
 
 
@@ -935,6 +940,33 @@ void migrate_set_state(int *state, int old_state, int new_state)
     }
 }
 
+void migrate_set_block_enabled(bool value, Error **errp)
+{
+    MigrationCapabilityStatusList *cap;
+
+    cap = g_new0(MigrationCapabilityStatusList, 1);
+    cap->value = g_new0(MigrationCapabilityStatus, 1);
+    cap->value->capability = MIGRATION_CAPABILITY_BLOCK;
+    cap->value->state = value;
+    qmp_migrate_set_capabilities(cap, errp);
+    qapi_free_MigrationCapabilityStatusList(cap);
+}
+
+static void migrate_set_block_incremental(MigrationState *s, bool value)
+{
+    s->parameters.block_incremental = value;
+}
+
+static void block_cleanup_parameters(MigrationState *s)
+{
+    if (s->must_remove_block_options) {
+        /* setting to false can never fail */
+        migrate_set_block_enabled(false, &error_abort);
+        migrate_set_block_incremental(s, false);
+        s->must_remove_block_options = false;
+    }
+}
+
 static void migrate_fd_cleanup(void *opaque)
 {
     MigrationState *s = opaque;
@@ -967,6 +999,7 @@ static void migrate_fd_cleanup(void *opaque)
     }
 
     notifier_list_notify(&migration_state_notifiers, s);
+    block_cleanup_parameters(s);
 }
 
 void migrate_fd_error(MigrationState *s, const Error *error)
@@ -979,6 +1012,7 @@ void migrate_fd_error(MigrationState *s, const Error *error)
         s->error = error_copy(error);
     }
     notifier_list_notify(&migration_state_notifiers, s);
+    block_cleanup_parameters(s);
 }
 
 static void migrate_fd_cancel(MigrationState *s)
@@ -1020,6 +1054,7 @@ static void migrate_fd_cancel(MigrationState *s)
             s->block_inactive = false;
         }
     }
+    block_cleanup_parameters(s);
 }
 
 void add_migration_state_change_notifier(Notifier *notify)
@@ -1207,6 +1242,26 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         return;
     }
 
+    if (((has_blk && blk) || (has_inc && inc))
+        && (migrate_use_block() || migrate_use_block_incremental())) {
+        error_setg(errp, "Command options are incompatible with current "
+                   "migration capabilities");
+        return;
+    }
+
+    if ((has_blk && blk) || (has_inc & inc)) {
+        migrate_set_block_enabled(true, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+        s->must_remove_block_options = true;
+    }
+
+    if (has_inc && inc) {
+        migrate_set_block_incremental(s, true);
+    }
+
     s = migrate_init(&params);
 
     if (strstart(uri, "tcp:", &p)) {
@@ -1404,6 +1459,24 @@ int64_t migrate_xbzrle_cache_size(void)
     return s->xbzrle_cache_size;
 }
 
+bool migrate_use_block(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK];
+}
+
+bool migrate_use_block_incremental(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->parameters.block_incremental;
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/qapi-schema.json b/qapi-schema.json
index 80603cf..621126a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -894,11 +894,18 @@
 # @release-ram: if enabled, qemu will free the migrated ram pages on the source
 #        during postcopy-ram migration. (since 2.9)
 #
+# @block: If enabled, QEMU will also migrate the contents of all block
+#          devices.  Default is disabled.  A possible alternative are
+#          mirror jobs to a builtin NBD server on the destination, which
+#          are more flexible.
+#          (Since 2.10)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
+           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
+           'block' ] }
 
 ##
 # @MigrationCapabilityStatus:
@@ -1009,13 +1016,20 @@
 # @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints in
 #          periodic mode. (Since 2.8)
 #
+# @block-incremental: Affects how much storage is migrated when the
+# 	block migration capability is enabled.  When false, the entire
+# 	storage backing chain is migrated into a flattened image at
+# 	the destination; when true, only the active qcow2 layer is
+# 	migrated and the destination must already have access to the
+# 	same backing chain as was used on the source.  (since 2.10)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
   'data': ['compress-level', 'compress-threads', 'decompress-threads',
            'cpu-throttle-initial', 'cpu-throttle-increment',
            'tls-creds', 'tls-hostname', 'max-bandwidth',
-           'downtime-limit', 'x-checkpoint-delay' ] }
+           'downtime-limit', 'x-checkpoint-delay', 'block-incremental' ] }
 
 ##
 # @migrate-set-parameters:
@@ -1082,6 +1096,13 @@
 #
 # @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 2.8)
 #
+# @block-incremental: Affects how much storage is migrated when the
+# 	block migration capability is enabled.  When false, the entire
+# 	storage backing chain is migrated into a flattened image at
+# 	the destination; when true, only the active qcow2 layer is
+# 	migrated and the destination must already have access to the
+# 	same backing chain as was used on the source.  (since 2.10)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -1094,7 +1115,8 @@
             '*tls-hostname': 'str',
             '*max-bandwidth': 'int',
             '*downtime-limit': 'int',
-            '*x-checkpoint-delay': 'int'} }
+            '*x-checkpoint-delay': 'int',
+            '*block-incremental': 'bool' } }
 
 ##
 # @query-migrate-parameters:
-- 
2.9.3

[Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Juan Quintela]

We have change in the previous patch to use migration capabilities for
it.  Notice that we continue using the old command line flags from
migrate command from the time being.  Remove the set_params method as
now it is empty.

For savevm, one can't do a:

savevm -b/-i foo

but now one can do:

migrate_set_capability block on
savevm foo

And we can't use block migration. We could disable block capability
unconditionally, but it would not be much better.

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

---
- Maintain shared/enabled dependency (Xu suggestion)
- Now we maintain the dependency on the setter functions
- improve error messages

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/migration.h |  3 +--
 migration/block.c             | 17 ++---------------
 migration/colo.c              |  4 ++--
 migration/migration.c         |  3 ---
 migration/savevm.c            |  8 ++++++--
 5 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 024a048..4dedc66 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -39,8 +39,7 @@
 #define QEMU_VM_SECTION_FOOTER       0x7e
 
 struct MigrationParams {
-    bool blk;
-    bool shared;
+    bool unused; /* C doesn't allow empty structs */
 };
 
 /* Messages sent on the return path from destination to source */
diff --git a/migration/block.c b/migration/block.c
index 060087f..5d22926 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -94,9 +94,6 @@ typedef struct BlkMigBlock {
 } BlkMigBlock;
 
 typedef struct BlkMigState {
-    /* Written during setup phase.  Can be read without a lock.  */
-    int blk_enable;
-    int shared_base;
     QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
     int64_t total_sector_sum;
     bool zero_blocks;
@@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f)
         bmds->bulk_completed = 0;
         bmds->total_sectors = sectors;
         bmds->completed_sectors = 0;
-        bmds->shared_base = block_mig_state.shared_base;
+        bmds->shared_base = migrate_use_block_incremental();
 
         assert(i < num_bs);
         bmds_bs[i].bmds = bmds;
@@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static void block_set_params(const MigrationParams *params, void *opaque)
-{
-    block_mig_state.blk_enable = params->blk;
-    block_mig_state.shared_base = params->shared;
-
-    /* shared base means that blk_enable = 1 */
-    block_mig_state.blk_enable |= params->shared;
-}
-
 static bool block_is_active(void *opaque)
 {
-    return block_mig_state.blk_enable == 1;
+    return migrate_use_block();
 }
 
 static SaveVMHandlers savevm_block_handlers = {
-    .set_params = block_set_params,
     .save_live_setup = block_save_setup,
     .save_live_iterate = block_save_iterate,
     .save_live_complete_precopy = block_save_complete,
diff --git a/migration/colo.c b/migration/colo.c
index 963c802..8c86892 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -14,6 +14,7 @@
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
 #include "migration/colo.h"
+#include "migration/block.h"
 #include "io/channel-buffer.h"
 #include "trace.h"
 #include "qemu/error-report.h"
@@ -345,8 +346,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     /* Disable block migration */
-    s->params.blk = 0;
-    s->params.shared = 0;
+    migrate_set_block_enabled(false, &local_err);
     qemu_savevm_state_header(fb);
     qemu_savevm_state_begin(fb, &s->params);
     qemu_mutex_lock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index f2bb448..f4cbfe6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1224,9 +1224,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationParams params;
     const char *p;
 
-    params.blk = has_blk && blk;
-    params.shared = has_inc && inc;
-
     if (migration_is_setup_or_active(s->state) ||
         s->state == MIGRATION_STATUS_CANCELLING ||
         s->state == MIGRATION_STATUS_COLO) {
diff --git a/migration/savevm.c b/migration/savevm.c
index f5e8194..2f1f4eb 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
     MigrationParams params = {
-        .blk = 0,
-        .shared = 0
     };
     MigrationState *ms = migrate_init(&params);
     MigrationStatus status;
@@ -1245,6 +1243,12 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
         goto done;
     }
 
+    if (migrate_use_block()) {
+        error_setg(errp, "Block migration and snapshots are incompatible");
+        ret = -EINVAL;
+        goto done;
+    }
+
     qemu_mutex_unlock_iothread();
     qemu_savevm_state_header(f);
     qemu_savevm_state_begin(f, &params);
-- 
2.9.3

[Qemu-devel] [PATCH 4/5] migration: Remove old MigrationParams

[Juan Quintela]

Not used anymore after moving block migration to use capabilities.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 include/migration/migration.h | 10 ++--------
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h       |  1 -
 include/sysemu/sysemu.h       |  3 +--
 migration/colo.c              |  2 +-
 migration/migration.c         |  8 +++-----
 migration/savevm.c            | 16 +++-------------
 7 files changed, 10 insertions(+), 31 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 4dedc66..b80a6ed 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -38,10 +38,6 @@
 #define QEMU_VM_COMMAND              0x08
 #define QEMU_VM_SECTION_FOOTER       0x7e
 
-struct MigrationParams {
-    bool unused; /* C doesn't allow empty structs */
-};
-
 /* Messages sent on the return path from destination to source */
 enum mig_rp_message_type {
     MIG_RP_MSG_INVALID = 0,  /* Must be 0 */
@@ -109,12 +105,10 @@ struct MigrationState
     QEMUBH *cleanup_bh;
     QEMUFile *to_dst_file;
 
-    /* New style params from 'migrate-set-parameters' */
+    /* params from 'migrate-set-parameters' */
     MigrationParameters parameters;
 
     int state;
-    /* Old style params from 'migrate' command */
-    MigrationParams params;
 
     /* State related to return path */
     struct {
@@ -207,7 +201,7 @@ void migrate_fd_connect(MigrationState *s);
 
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
-MigrationState *migrate_init(const MigrationParams *params);
+MigrationState *migrate_init(void);
 bool migration_is_blocked(Error **errp);
 bool migration_in_setup(MigrationState *);
 bool migration_is_idle(void);
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 8489659..dacb052 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -37,7 +37,6 @@ typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
 
 typedef struct SaveVMHandlers {
     /* This runs inside the iothread lock.  */
-    void (*set_params)(const MigrationParams *params, void * opaque);
     SaveStateHandler *save_state;
 
     void (*cleanup)(void *opaque);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 7d85057..33a6aa1 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -49,7 +49,6 @@ typedef struct MemoryRegion MemoryRegion;
 typedef struct MemoryRegionCache MemoryRegionCache;
 typedef struct MemoryRegionSection MemoryRegionSection;
 typedef struct MigrationIncomingState MigrationIncomingState;
-typedef struct MigrationParams MigrationParams;
 typedef struct MigrationState MigrationState;
 typedef struct Monitor Monitor;
 typedef struct MonitorDef MonitorDef;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 83c1ceb..765358e 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -102,8 +102,7 @@ enum qemu_vm_cmd {
 #define MAX_VM_CMD_PACKAGED_SIZE (1ul << 24)
 
 bool qemu_savevm_state_blocked(Error **errp);
-void qemu_savevm_state_begin(QEMUFile *f,
-                             const MigrationParams *params);
+void qemu_savevm_state_begin(QEMUFile *f);
 void qemu_savevm_state_header(QEMUFile *f);
 int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
 void qemu_savevm_state_cleanup(void);
diff --git a/migration/colo.c b/migration/colo.c
index 8c86892..dd38fed 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -348,7 +348,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     /* Disable block migration */
     migrate_set_block_enabled(false, &local_err);
     qemu_savevm_state_header(fb);
-    qemu_savevm_state_begin(fb, &s->params);
+    qemu_savevm_state_begin(fb);
     qemu_mutex_lock_iothread();
     qemu_savevm_state_complete_precopy(fb, false);
     qemu_mutex_unlock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index f4cbfe6..168b648 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1118,7 +1118,7 @@ bool migration_is_idle(void)
     return false;
 }
 
-MigrationState *migrate_init(const MigrationParams *params)
+MigrationState *migrate_init(void)
 {
     MigrationState *s = migrate_get_current();
 
@@ -1132,7 +1132,6 @@ MigrationState *migrate_init(const MigrationParams *params)
     s->cleanup_bh = 0;
     s->to_dst_file = NULL;
     s->state = MIGRATION_STATUS_NONE;
-    s->params = *params;
     s->rp_state.from_dst_file = NULL;
     s->rp_state.error = false;
     s->mbps = 0.0;
@@ -1221,7 +1220,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    MigrationParams params;
     const char *p;
 
     if (migration_is_setup_or_active(s->state) ||
@@ -1259,7 +1257,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         migrate_set_block_incremental(s, true);
     }
 
-    s = migrate_init(&params);
+    s = migrate_init();
 
     if (strstart(uri, "tcp:", &p)) {
         tcp_start_outgoing_migration(s, p, &local_err);
@@ -1983,7 +1981,7 @@ static void *migration_thread(void *opaque)
         qemu_savevm_send_postcopy_advise(s->to_dst_file);
     }
 
-    qemu_savevm_state_begin(s->to_dst_file, &s->params);
+    qemu_savevm_state_begin(s->to_dst_file);
 
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
     migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
diff --git a/migration/savevm.c b/migration/savevm.c
index 2f1f4eb..a728414 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -966,21 +966,13 @@ void qemu_savevm_state_header(QEMUFile *f)
 
 }
 
-void qemu_savevm_state_begin(QEMUFile *f,
-                             const MigrationParams *params)
+void qemu_savevm_state_begin(QEMUFile *f)
 {
     SaveStateEntry *se;
     int ret;
 
     trace_savevm_state_begin();
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
-        if (!se->ops || !se->ops->set_params) {
-            continue;
-        }
-        se->ops->set_params(params, se->opaque);
-    }
-
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (!se->ops || !se->ops->save_live_setup) {
             continue;
         }
@@ -1232,9 +1224,7 @@ void qemu_savevm_state_cleanup(void)
 static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
-    MigrationParams params = {
-    };
-    MigrationState *ms = migrate_init(&params);
+    MigrationState *ms = migrate_init();
     MigrationStatus status;
     ms->to_dst_file = f;
 
@@ -1251,7 +1241,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 
     qemu_mutex_unlock_iothread();
     qemu_savevm_state_header(f);
-    qemu_savevm_state_begin(f, &params);
+    qemu_savevm_state_begin(f);
     qemu_mutex_lock_iothread();
 
     while (qemu_file_get_error(f) == 0) {
-- 
2.9.3

[Qemu-devel] [PATCH 5/5] block migration: Allow compile time disable

[Juan Quintela]

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

Many users now prefer to use drive_mirror over NBD as an
alternative to the older migrate -b option; drive_mirror is
more complex to setup but gives you more options (e.g. only
migrating some of the disks if some of them are shared).

Allow the large chunk of block migration code to be compiled
out for those who don't use it.

Based on a downstream-patch we've had for a while by Jeff Cody.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 configure                 | 11 +++++++++++
 include/migration/block.h | 24 +++++++++++++++++++++++-
 migration/Makefile.objs   |  2 +-
 migration/migration.c     | 13 +++++++++++++
 4 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 57b5ae6..ace1a52 100755
--- a/configure
+++ b/configure
@@ -316,6 +316,7 @@ vte=""
 virglrenderer=""
 tpm="yes"
 libssh2=""
+live_block_migration="yes"
 numa=""
 tcmalloc="no"
 jemalloc="no"
@@ -1169,6 +1170,10 @@ for opt do
   ;;
   --enable-libssh2) libssh2="yes"
   ;;
+  --disable-live-block-migration) live_block_migration="no"
+  ;;
+  --enable-live-block-migration) live_block_migration="yes"
+  ;;
   --disable-numa) numa="no"
   ;;
   --enable-numa) numa="yes"
@@ -1401,6 +1406,7 @@ disabled with --disable-FEATURE, default is enabled if available:
   libnfs          nfs support
   smartcard       smartcard support (libcacard)
   libusb          libusb (for usb passthrough)
+  live-block-migration   Block migration in the main migration stream
   usb-redir       usb network redirection support
   lzo             support of lzo compression library
   snappy          support of snappy compression library
@@ -5216,6 +5222,7 @@ echo "TPM support       $tpm"
 echo "libssh2 support   $libssh2"
 echo "TPM passthrough   $tpm_passthrough"
 echo "QOM debugging     $qom_cast_debug"
+echo "Live block migration $live_block_migration"
 echo "lzo support       $lzo"
 echo "snappy support    $snappy"
 echo "bzip2 support     $bzip2"
@@ -5782,6 +5789,10 @@ if test "$libssh2" = "yes" ; then
   echo "LIBSSH2_LIBS=$libssh2_libs" >> $config_host_mak
 fi
 
+if test "$live_block_migration" = "yes" ; then
+  echo "CONFIG_LIVE_BLOCK_MIGRATION=y" >> $config_host_mak
+fi
+
 # USB host support
 if test "$libusb" = "yes"; then
   echo "HOST_USB=libusb legacy" >> $config_host_mak
diff --git a/include/migration/block.h b/include/migration/block.h
index 5225af9..28cff53 100644
--- a/include/migration/block.h
+++ b/include/migration/block.h
@@ -14,12 +14,34 @@
 #ifndef MIGRATION_BLOCK_H
 #define MIGRATION_BLOCK_H
 
+#ifdef CONFIG_LIVE_BLOCK_MIGRATION
 void blk_mig_init(void);
 int blk_mig_active(void);
 uint64_t blk_mig_bytes_transferred(void);
 uint64_t blk_mig_bytes_remaining(void);
 uint64_t blk_mig_bytes_total(void);
 
+#else
+static inline void blk_mig_init(void) { }
+static inline int blk_mig_active(void)
+{
+    return false;
+}
+static inline uint64_t blk_mig_bytes_transferred(void)
+{
+    return 0;
+}
+
+static inline uint64_t blk_mig_bytes_remaining(void)
+{
+    return 0;
+}
+
+static inline uint64_t blk_mig_bytes_total(void)
+{
+    return 0;
+}
+#endif /* CONFIG_LIVE_BLOCK_MIGRATION */
+
 void migrate_set_block_enabled(bool value, Error **errp);
-
 #endif /* MIGRATION_BLOCK_H */
diff --git a/migration/Makefile.objs b/migration/Makefile.objs
index c1920b6..00a3f4a 100644
--- a/migration/Makefile.objs
+++ b/migration/Makefile.objs
@@ -9,5 +9,5 @@ common-obj-y += qjson.o
 
 common-obj-$(CONFIG_RDMA) += rdma.o
 
-common-obj-y += block.o
+common-obj-$(CONFIG_LIVE_BLOCK_MIGRATION) += block.o
 
diff --git a/migration/migration.c b/migration/migration.c
index 168b648..d5ebcd2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -547,6 +547,11 @@ MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
 
     caps = NULL; /* silence compiler warning */
     for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
+#ifndef CONFIG_LIVE_BLOCK_MIGRATION
+        if (i == MIGRATION_CAPABILITY_BLOCK) {
+            continue;
+        }
+#endif
         if (i == MIGRATION_CAPABILITY_X_COLO && !colo_supported()) {
             continue;
         }
@@ -763,6 +768,14 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     }
 
     for (cap = params; cap; cap = cap->next) {
+#ifndef CONFIG_LIVE_BLOCK_MIGRATION
+        if (cap->value->capability == MIGRATION_CAPABILITY_BLOCK) {
+            error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
+                       "block migration");
+            error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
+            continue;
+        }
+#endif
         if (cap->value->capability == MIGRATION_CAPABILITY_X_COLO) {
             if (!colo_supported()) {
                 error_setg(errp, "COLO is not currently supported, please"
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Eric Blake]

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

On 05/17/2017 10:38 AM, Juan Quintela wrote:
> Create one capability for block migration and one parameter for
> incremental block migration.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> ---
> 
> @@ -1207,6 +1242,26 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          return;
>      }
>  
> +    if (((has_blk && blk) || (has_inc && inc))
> +        && (migrate_use_block() || migrate_use_block_incremental())) {
> +        error_setg(errp, "Command options are incompatible with current "
> +                   "migration capabilities");
> +        return;
> +    }
> +
> +    if ((has_blk && blk) || (has_inc & inc)) {
> +        migrate_set_block_enabled(true, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +        s->must_remove_block_options = true;
> +    }

You wrote:
if (A && B) {
  error;
}
if (A) {
  stuff;
}

I might have done:

if (A) {
  if (B) {
    error;
  }
  stuff;
}

but it's the same either way.


> +++ b/qapi-schema.json
> @@ -894,11 +894,18 @@
>  # @release-ram: if enabled, qemu will free the migrated ram pages on the source
>  #        during postcopy-ram migration. (since 2.9)
>  #
> +# @block: If enabled, QEMU will also migrate the contents of all block
> +#          devices.  Default is disabled.  A possible alternative are

s/are/uses/

> +#          mirror jobs to a builtin NBD server on the destination, which
> +#          are more flexible.

s/are more flexible/offers more flexibility/

> +#          (Since 2.10)
> +#
>  # Since: 1.2
>  ##
>  { 'enum': 'MigrationCapability',
>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> -           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
> +           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
> +           'block' ] }
>  

The grammar touchups can be done in preparing the pull request, if there
is no other reason for a respin.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Eric Blake]

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

On 05/17/2017 10:38 AM, Juan Quintela wrote:
> We have change in the previous patch to use migration capabilities for
> it.  Notice that we continue using the old command line flags from
> migrate command from the time being.  Remove the set_params method as
> now it is empty.
> 
> For savevm, one can't do a:
> 
> savevm -b/-i foo
> 
> but now one can do:
> 
> migrate_set_capability block on
> savevm foo
> 
> And we can't use block migration. We could disable block capability
> unconditionally, but it would not be much better.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> ---
> - Maintain shared/enabled dependency (Xu suggestion)
> - Now we maintain the dependency on the setter functions
> - improve error messages
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

This second S-o-b gets stripped, but the one that counts is in place
(I've made the same mistake before, as well - doing 'git commit --amend
-s' on a commit where I already had a --- separator)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 5/5] block migration: Allow compile time disable

[Eric Blake]

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

On 05/17/2017 10:38 AM, Juan Quintela wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Many users now prefer to use drive_mirror over NBD as an
> alternative to the older migrate -b option; drive_mirror is
> more complex to setup but gives you more options (e.g. only
> migrating some of the disks if some of them are shared).
> 
> Allow the large chunk of block migration code to be compiled
> out for those who don't use it.
> 
> Based on a downstream-patch we've had for a while by Jeff Cody.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  configure                 | 11 +++++++++++
>  include/migration/block.h | 24 +++++++++++++++++++++++-
>  migration/Makefile.objs   |  2 +-
>  migration/migration.c     | 13 +++++++++++++
>  4 files changed, 48 insertions(+), 2 deletions(-)

> @@ -763,6 +768,14 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>      }
>  
>      for (cap = params; cap; cap = cap->next) {
> +#ifndef CONFIG_LIVE_BLOCK_MIGRATION
> +        if (cap->value->capability == MIGRATION_CAPABILITY_BLOCK) {
> +            error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
> +                       "block migration");
> +            error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
> +            continue;
> +        }
> +#endif

If I'm not mistaken, this will cause an unconditional error even if I do
the QMP { "command":"migrate", "arguments":{"uri":"...", "blk":false} }

Wouldn't it be better to make the conditional:

if (cap->value->capability == MIGRATION_CAPABILITY_BLOCK &&
cap->value->state) {

so that I can pass an explicit false, and the error only happens when
trying to set the optional parameter to true?  I ask, because libvirt
(currently) ALWAYS passes the "inc" and "blk" QMP parameters, but
generally to an explicit false when it uses NBD+mirror.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/17/2017 10:38 AM, Juan Quintela wrote:
>> We have change in the previous patch to use migration capabilities for
>> it.  Notice that we continue using the old command line flags from
>> migrate command from the time being.  Remove the set_params method as
>> now it is empty.
>> 
>> For savevm, one can't do a:
>> 
>> savevm -b/-i foo
>> 
>> but now one can do:
>> 
>> migrate_set_capability block on
>> savevm foo
>> 
>> And we can't use block migration. We could disable block capability
>> unconditionally, but it would not be much better.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> 
>> ---
>> - Maintain shared/enabled dependency (Xu suggestion)
>> - Now we maintain the dependency on the setter functions
>> - improve error messages
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> This second S-o-b gets stripped, but the one that counts is in place
> (I've made the same mistake before, as well - doing 'git commit --amend
> -s' on a commit where I already had a --- separator)

And this time I *really* remove the -s from my git-format-patch alias.
I dropped that last week on the terminal, rebooted the machine and
..... magic ... got the old alias O:-)

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/17/2017 10:38 AM, Juan Quintela wrote:
>> Create one capability for block migration and one parameter for
>> incremental block migration.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> 
>> ---
>> 
>> @@ -1207,6 +1242,26 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>          return;
>>      }
>>  
>> +    if (((has_blk && blk) || (has_inc && inc))
>> +        && (migrate_use_block() || migrate_use_block_incremental())) {
>> +        error_setg(errp, "Command options are incompatible with current "
>> +                   "migration capabilities");
>> +        return;
>> +    }
>> +
>> +    if ((has_blk && blk) || (has_inc & inc)) {
>> +        migrate_set_block_enabled(true, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +        s->must_remove_block_options = true;
>> +    }
>
> You wrote:
> if (A && B) {
>   error;
> }
> if (A) {
>   stuff;
> }
>
> I might have done:
>
> if (A) {
>   if (B) {
>     error;
>   }
>   stuff;
> }
>
> but it's the same either way.

One less line, and as I have to respin due to the last patch.

>> +++ b/qapi-schema.json
>> @@ -894,11 +894,18 @@
>>  # @release-ram: if enabled, qemu will free the migrated ram pages on the source
>>  #        during postcopy-ram migration. (since 2.9)
>>  #
>> +# @block: If enabled, QEMU will also migrate the contents of all block
>> +#          devices.  Default is disabled.  A possible alternative are
>
> s/are/uses/

done

>> +#          mirror jobs to a builtin NBD server on the destination, which
>> +#          are more flexible.
>
> s/are more flexible/offers more flexibility/

done

>> +#          (Since 2.10)
>> +#
>>  # Since: 1.2
>>  ##
>>  { 'enum': 'MigrationCapability',
>>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>> -           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>> +           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
>> +           'block' ] }
>>  
>
> The grammar touchups can be done in preparing the pull request, if there
> is no other reason for a respin.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks, Juan.

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> We have change in the previous patch to use migration capabilities for
> it.  Notice that we continue using the old command line flags from
> migrate command from the time being.  Remove the set_params method as

for the time being

> now it is empty.
>
> For savevm, one can't do a:
>
> savevm -b/-i foo
>
> but now one can do:
>
> migrate_set_capability block on
> savevm foo
>
> And we can't use block migration. We could disable block capability
> unconditionally, but it would not be much better.

I think I get what you're trying to say, but only because I have plenty
of context right now.  Let me try to rephrase:

  migration: Use new configuration instead of old MigrationParams

  The previous commit introduced a MigrationCapability and a
  MigrationParameter for block migration.  Use them instead of the old
  MigrationParams.

  Take care to reject attempts to combine block migration with
  snapshots, e.g. like this:

      migrate_set_capability block on
      savevm foo

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

Preferably with a commit message I can still understand three weeks from
now:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Markus Armbruster]

I got confused and replied to an old version.  Please ignore.

Markus Armbruster <armbru@redhat.com> writes:

> Juan Quintela <quintela@redhat.com> writes:
>
>> We have change in the previous patch to use migration capabilities for
>> it.  Notice that we continue using the old command line flags from
>> migrate command from the time being.  Remove the set_params method as
>
> for the time being
>
>> now it is empty.
>>
>> For savevm, one can't do a:
>>
>> savevm -b/-i foo
>>
>> but now one can do:
>>
>> migrate_set_capability block on
>> savevm foo
>>
>> And we can't use block migration. We could disable block capability
>> unconditionally, but it would not be much better.
>
> I think I get what you're trying to say, but only because I have plenty
> of context right now.  Let me try to rephrase:
>
>   migration: Use new configuration instead of old MigrationParams
>
>   The previous commit introduced a MigrationCapability and a
>   MigrationParameter for block migration.  Use them instead of the old
>   MigrationParams.
>
>   Take care to reject attempts to combine block migration with
>   snapshots, e.g. like this:
>
>       migrate_set_capability block on
>       savevm foo
>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> Preferably with a commit message I can still understand three weeks from
> now:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Create one capability for block migration and one parameter for
> incremental block migration.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
[...]
> diff --git a/include/migration/block.h b/include/migration/block.h
> index 41a1ac8..5225af9 100644
> --- a/include/migration/block.h
> +++ b/include/migration/block.h
> @@ -20,4 +20,6 @@ uint64_t blk_mig_bytes_transferred(void);
>  uint64_t blk_mig_bytes_remaining(void);
>  uint64_t blk_mig_bytes_total(void);
>  
> +void migrate_set_block_enabled(bool value, Error **errp);
> +
>  #endif /* MIGRATION_BLOCK_H */
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 49ec501..024a048 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -153,6 +153,9 @@ struct MigrationState
>  
>      /* The last error that occurred */
>      Error *error;
> +    /* Do we have to clean up -b/-i from old migrate parameters */

The sentence is a question, so it should end with a '?'.

> +    /* This feature is deprecated and will be removed */
> +    bool must_remove_block_options;
>  };
>  
>  void migrate_set_state(int *state, int old_state, int new_state);
> @@ -265,6 +268,9 @@ bool migrate_colo_enabled(void);
>  
>  int64_t xbzrle_cache_resize(int64_t new_size);
>  
> +bool migrate_use_block(void);
> +bool migrate_use_block_incremental(void);
> +
>  bool migrate_use_compression(void);
>  int migrate_compress_level(void);
>  int migrate_compress_threads(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index 0304c01..c13c0a2 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
[...]
> @@ -1207,6 +1242,24 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          return;
>      }
>  
> +    if ((has_blk && blk) || (has_inc && inc)) {
> +        if (migrate_use_block() || migrate_use_block_incremental()) {
> +            error_setg(errp, "Command options are incompatible with "
> +                       "current migration capabilities");
> +            return;
> +        }
> +        migrate_set_block_enabled(true, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +        s->must_remove_block_options = true;
> +    }
> +
> +    if (has_inc && inc) {
> +        migrate_set_block_incremental(s, true);
> +    }
> +

Putting this within the previous conditional might be clearer.  Your
choice.

>      s = migrate_init(&params);
>  
>      if (strstart(uri, "tcp:", &p)) {
[...]

Only nitpicks, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Eric Blake]

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

On 05/18/2017 06:18 AM, Juan Quintela wrote:
> Create one capability for block migration and one parameter for
> incremental block migration.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> ---
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 2/5] migration: Create block capability

[Juan Quintela]

Create one capability for block migration and one parameter for
incremental block migration.

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

---

- address all Markus comments
- use Markus and Eric text descriptions
- change logic another time
- improve text messages
---
 hmp.c                         | 13 ++++++++
 include/migration/block.h     |  2 ++
 include/migration/migration.h |  6 ++++
 migration/migration.c         | 71 +++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json              | 28 +++++++++++++++--
 5 files changed, 117 insertions(+), 3 deletions(-)

diff --git a/hmp.c b/hmp.c
index acbbc5c..208154c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -326,6 +326,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %" PRId64 "\n",
             MigrationParameter_lookup[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY],
             params->x_checkpoint_delay);
+        assert(params->has_block_incremental);
+        monitor_printf(mon, "%s: %s\n",
+            MigrationParameter_lookup[MIGRATION_PARAMETER_BLOCK_INCREMENTAL],
+                       params->block_incremental ? "on" : "off");
     }
 
     qapi_free_MigrationParameters(params);
@@ -1527,6 +1531,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     Visitor *v = string_input_visitor_new(valuestr);
     uint64_t valuebw = 0;
     int64_t valueint = 0;
+    bool valuebool = false;
     Error *err = NULL;
     bool use_int_value = false;
     int i, ret;
@@ -1581,6 +1586,14 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                 p.has_x_checkpoint_delay = true;
                 use_int_value = true;
                 break;
+            case MIGRATION_PARAMETER_BLOCK_INCREMENTAL:
+                p.has_block_incremental = true;
+                visit_type_bool(v, param, &valuebool, &err);
+                if (err) {
+                    goto cleanup;
+                }
+                p.block_incremental = valuebool;
+                break;
             }
 
             if (use_int_value) {
diff --git a/include/migration/block.h b/include/migration/block.h
index 41a1ac8..5225af9 100644
--- a/include/migration/block.h
+++ b/include/migration/block.h
@@ -20,4 +20,6 @@ uint64_t blk_mig_bytes_transferred(void);
 uint64_t blk_mig_bytes_remaining(void);
 uint64_t blk_mig_bytes_total(void);
 
+void migrate_set_block_enabled(bool value, Error **errp);
+
 #endif /* MIGRATION_BLOCK_H */
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 49ec501..024a048 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -153,6 +153,9 @@ struct MigrationState
 
     /* The last error that occurred */
     Error *error;
+    /* Do we have to clean up -b/-i from old migrate parameters */
+    /* This feature is deprecated and will be removed */
+    bool must_remove_block_options;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
@@ -265,6 +268,9 @@ bool migrate_colo_enabled(void);
 
 int64_t xbzrle_cache_resize(int64_t new_size);
 
+bool migrate_use_block(void);
+bool migrate_use_block_incremental(void);
+
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
diff --git a/migration/migration.c b/migration/migration.c
index 0304c01..c13c0a2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -592,6 +592,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->downtime_limit = s->parameters.downtime_limit;
     params->has_x_checkpoint_delay = true;
     params->x_checkpoint_delay = s->parameters.x_checkpoint_delay;
+    params->has_block_incremental = true;
+    params->block_incremental = s->parameters.block_incremental;
 
     return params;
 }
@@ -900,6 +902,9 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
             colo_checkpoint_notify(s);
         }
     }
+    if (params->has_block_incremental) {
+        s->parameters.block_incremental = params->block_incremental;
+    }
 }
 
 
@@ -935,6 +940,33 @@ void migrate_set_state(int *state, int old_state, int new_state)
     }
 }
 
+void migrate_set_block_enabled(bool value, Error **errp)
+{
+    MigrationCapabilityStatusList *cap;
+
+    cap = g_new0(MigrationCapabilityStatusList, 1);
+    cap->value = g_new0(MigrationCapabilityStatus, 1);
+    cap->value->capability = MIGRATION_CAPABILITY_BLOCK;
+    cap->value->state = value;
+    qmp_migrate_set_capabilities(cap, errp);
+    qapi_free_MigrationCapabilityStatusList(cap);
+}
+
+static void migrate_set_block_incremental(MigrationState *s, bool value)
+{
+    s->parameters.block_incremental = value;
+}
+
+static void block_cleanup_parameters(MigrationState *s)
+{
+    if (s->must_remove_block_options) {
+        /* setting to false can never fail */
+        migrate_set_block_enabled(false, &error_abort);
+        migrate_set_block_incremental(s, false);
+        s->must_remove_block_options = false;
+    }
+}
+
 static void migrate_fd_cleanup(void *opaque)
 {
     MigrationState *s = opaque;
@@ -967,6 +999,7 @@ static void migrate_fd_cleanup(void *opaque)
     }
 
     notifier_list_notify(&migration_state_notifiers, s);
+    block_cleanup_parameters(s);
 }
 
 void migrate_fd_error(MigrationState *s, const Error *error)
@@ -979,6 +1012,7 @@ void migrate_fd_error(MigrationState *s, const Error *error)
         s->error = error_copy(error);
     }
     notifier_list_notify(&migration_state_notifiers, s);
+    block_cleanup_parameters(s);
 }
 
 static void migrate_fd_cancel(MigrationState *s)
@@ -1020,6 +1054,7 @@ static void migrate_fd_cancel(MigrationState *s)
             s->block_inactive = false;
         }
     }
+    block_cleanup_parameters(s);
 }
 
 void add_migration_state_change_notifier(Notifier *notify)
@@ -1207,6 +1242,24 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         return;
     }
 
+    if ((has_blk && blk) || (has_inc && inc)) {
+        if (migrate_use_block() || migrate_use_block_incremental()) {
+            error_setg(errp, "Command options are incompatible with "
+                       "current migration capabilities");
+            return;
+        }
+        migrate_set_block_enabled(true, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+        s->must_remove_block_options = true;
+    }
+
+    if (has_inc && inc) {
+        migrate_set_block_incremental(s, true);
+    }
+
     s = migrate_init(&params);
 
     if (strstart(uri, "tcp:", &p)) {
@@ -1404,6 +1457,24 @@ int64_t migrate_xbzrle_cache_size(void)
     return s->xbzrle_cache_size;
 }
 
+bool migrate_use_block(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK];
+}
+
+bool migrate_use_block_incremental(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->parameters.block_incremental;
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/qapi-schema.json b/qapi-schema.json
index 80603cf..e38c5f0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -894,11 +894,18 @@
 # @release-ram: if enabled, qemu will free the migrated ram pages on the source
 #        during postcopy-ram migration. (since 2.9)
 #
+# @block: If enabled, QEMU will also migrate the contents of all block
+#          devices.  Default is disabled.  A possible alternative uses
+#          mirror jobs to a builtin NBD server on the destination, which
+#          offers more flexibility.
+#          (Since 2.10)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
+           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
+           'block' ] }
 
 ##
 # @MigrationCapabilityStatus:
@@ -1009,13 +1016,20 @@
 # @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints in
 #          periodic mode. (Since 2.8)
 #
+# @block-incremental: Affects how much storage is migrated when the
+# 	block migration capability is enabled.  When false, the entire
+# 	storage backing chain is migrated into a flattened image at
+# 	the destination; when true, only the active qcow2 layer is
+# 	migrated and the destination must already have access to the
+# 	same backing chain as was used on the source.  (since 2.10)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
   'data': ['compress-level', 'compress-threads', 'decompress-threads',
            'cpu-throttle-initial', 'cpu-throttle-increment',
            'tls-creds', 'tls-hostname', 'max-bandwidth',
-           'downtime-limit', 'x-checkpoint-delay' ] }
+           'downtime-limit', 'x-checkpoint-delay', 'block-incremental' ] }
 
 ##
 # @migrate-set-parameters:
@@ -1082,6 +1096,13 @@
 #
 # @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 2.8)
 #
+# @block-incremental: Affects how much storage is migrated when the
+# 	block migration capability is enabled.  When false, the entire
+# 	storage backing chain is migrated into a flattened image at
+# 	the destination; when true, only the active qcow2 layer is
+# 	migrated and the destination must already have access to the
+# 	same backing chain as was used on the source.  (since 2.10)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -1094,7 +1115,8 @@
             '*tls-hostname': 'str',
             '*max-bandwidth': 'int',
             '*downtime-limit': 'int',
-            '*x-checkpoint-delay': 'int'} }
+            '*x-checkpoint-delay': 'int',
+            '*block-incremental': 'bool' } }
 
 ##
 # @query-migrate-parameters:
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/16/2017 11:42 AM, Markus Armbruster wrote:
>
>>>> Well, to suggest something, I'd first have to figure out WTF incremental
>>>> block migration does.  Your text helps me some, but not enough.  What
>>>> exactly is being migrated, and what exactly is assumed to be shared
>>>> between source and destination?
>>>>
>>>> Block migration is scandalously underdocumented.
>>>
>
>> Can you draft a documentation comment for @block-incremental?
>
> How about:
>
> @block-incremental: Affects how much storage is migrated when the block
> migration capability is enabled.  When false, the entire storage backing
> chain is migrated into a flattened image at the destination; when true,
> only the active qcow2 layer is migrated and the destination must already
> have access to the same backing chain as was used on the source.
> (since 2.10)

Changed.  Thanks.

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/16/2017 11:42 AM, Markus Armbruster wrote:
>
>>>> Well, to suggest something, I'd first have to figure out WTF incremental
>>>> block migration does.  Your text helps me some, but not enough.  What
>>>> exactly is being migrated, and what exactly is assumed to be shared
>>>> between source and destination?
>>>>
>>>> Block migration is scandalously underdocumented.
>>>
>
>> Can you draft a documentation comment for @block-incremental?
>
> How about:
>
> @block-incremental: Affects how much storage is migrated when the block
> migration capability is enabled.  When false, the entire storage backing
> chain is migrated into a flattened image at the destination; when true,
> only the active qcow2 layer is migrated and the destination must already
> have access to the same backing chain as was used on the source.
> (since 2.10)

Works for me, thanks!

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Eric Blake]

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

On 05/16/2017 11:42 AM, Markus Armbruster wrote:

>>> Well, to suggest something, I'd first have to figure out WTF incremental
>>> block migration does.  Your text helps me some, but not enough.  What
>>> exactly is being migrated, and what exactly is assumed to be shared
>>> between source and destination?
>>>
>>> Block migration is scandalously underdocumented.
>>

> Can you draft a documentation comment for @block-incremental?

How about:

@block-incremental: Affects how much storage is migrated when the block
migration capability is enabled.  When false, the entire storage backing
chain is migrated into a flattened image at the destination; when true,
only the active qcow2 layer is migrated and the destination must already
have access to the same backing chain as was used on the source.
(since 2.10)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/16/2017 11:00 AM, Markus Armbruster wrote:
>
>>>>>  #          periodic mode. (Since 2.8)
>>>>>  #
>>>>> +# @block-incremental: enable block incremental migration (Since 2.10)
>>>>> +#
>>>>
>>>> What's "block incremental migration" and why should I care?
>>>
>>> This is good, I will try.
>>>
>>> "block incremental migration assumes that we have a base image in both
>>> sides, and then we continue writting in one of the sides.  This way we
>>> need to only migrate the changes since the previous state where it was
>>> the same in both sides".
>>>
>>> I am not sure what to put there, really.
>> 
>> Well, to suggest something, I'd first have to figure out WTF incremental
>> block migration does.  Your text helps me some, but not enough.  What
>> exactly is being migrated, and what exactly is assumed to be shared
>> between source and destination?
>> 
>> Block migration is scandalously underdocumented.
>
> If I have:
>
> base <- active
>
> on the source, then:
>
> block migration without incremental creates:
>
> active
>
> on the destination (the entire disk contents are migrated).  Conversely,
> block migration WITH incremental assumes that I have pre-created 'base'
> on the destination (easy to do, since base is read-only so it can be
> copied prior to starting the migration), and the migration results in:
>
> base <- active
>
> on the destination, where only the contents of active were transferred
> by qemu.

Can you draft a documentation comment for @block-incremental?

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Eric Blake]

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

On 05/16/2017 11:00 AM, Markus Armbruster wrote:

>>>>  #          periodic mode. (Since 2.8)
>>>>  #
>>>> +# @block-incremental: enable block incremental migration (Since 2.10)
>>>> +#
>>>
>>> What's "block incremental migration" and why should I care?
>>
>> This is good, I will try.
>>
>> "block incremental migration assumes that we have a base image in both
>> sides, and then we continue writting in one of the sides.  This way we
>> need to only migrate the changes since the previous state where it was
>> the same in both sides".
>>
>> I am not sure what to put there, really.
> 
> Well, to suggest something, I'd first have to figure out WTF incremental
> block migration does.  Your text helps me some, but not enough.  What
> exactly is being migrated, and what exactly is assumed to be shared
> between source and destination?
> 
> Block migration is scandalously underdocumented.

If I have:

base <- active

on the source, then:

block migration without incremental creates:

active

on the destination (the entire disk contents are migrated).  Conversely,
block migration WITH incremental assumes that I have pre-created 'base'
on the destination (easy to do, since base is read-only so it can be
copied prior to starting the migration), and the migration results in:

base <- active

on the destination, where only the contents of active were transferred
by qemu.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Juan Quintela <quintela@redhat.com> writes:
>>
>>> Create one capability for block migration and one parameter for
>>> incremental block migration.
>>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
>>>  
>>>      /* The last error that occurred */
>>>      Error *error;
>>> +    /* Do we have to clean up -b/-i from old migrate paramteres */
>>
>> parameters
>
> ok.
>
>
>>> +void migrate_set_block_enabled(bool value, Error **errp)
>>> +{
>>> +    MigrationCapabilityStatusList *cap;
>>> +
>>> +    cap = g_malloc0(sizeof(*cap));
>>> +    cap->value = g_malloc(sizeof(*cap->value));
>>
>> I prefer g_new() for extra type checking.  Your choice.
>
> ok.
>
>
>>> +    cap->value->capability = MIGRATION_CAPABILITY_BLOCK;
>>> +    cap->value->state = value;
>>> +    qmp_migrate_set_capabilities(cap, errp);
>>> +    qapi_free_MigrationCapabilityStatusList(cap);
>>> +}
>>
>> The objective is to set one bit.  The means is building a
>> MigrationCapabilityStatusList.  When you have to build an elaborate data
>> structure just so you can set one bit, your interfaces are likely
>> deficient.  Observation, not change request.
>
> This was Dave suggestion.  The reason for doing this is in the following
> patch when we enable compile disable of block migration to put the ifdef
> in a single place.  Otherwise, I have to put it in two places.
>
>>> +static void migrate_set_block_incremental(MigrationState *s, bool value)
>>> +{
>>> +    s->parameters.block_incremental = value;
>>> +}
>>
>> This is how setting one bit should look like.
>
> See previous comment.
>
>>
>>> +
>>> +static void block_cleanup_parameters(MigrationState *s)
>>> +{
>>> +    if (s->must_remove_block) {
>>> +        migrate_set_block_enabled(false, NULL);
>>
>> NULL ignores errors.  If an error happens, and we ignore it, we're
>> screwed, aren't we?  I suspect we want &error_abort here.
>
> setting to false can't never fail.  It is when we set it to true that it
> can fail because it is compiled out (that will be in next patch).

Sounds like a job for &error_abort to me.

>>> @@ -1229,6 +1267,38 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>          return;
>>>      }
>>>  
>>> +    if (has_blk && blk) {
>>> +        if (migrate_use_block() || migrate_use_block_incremental()) {
>>> +            error_setg(errp, "You can't use block capability and -b/i");
>>
>> Error message assumes HMP.  In QMP, the thing is called 'blk', not -b.
>> Perhaps we can sidestep the issue like this: "Command options are
>> incompatible with current migration capabilities".
>
> ok.
>
>>
>>> +            return;
>>> +        }
>>> +        migrate_set_block_enabled(true, &local_err);
>>> +        if (local_err) {
>>> +            error_propagate(errp, local_err);
>>> +            return;
>>> +        }
>>> +        block_enabled = true;
>>> +        s->must_remove_block = true;
>>> +    }
>>> +
>>> +    if (has_inc && inc) {
>>> +        if ((migrate_use_block() && !block_enabled)
>>> +            || migrate_use_block_incremental()) {
>>> +            error_setg(errp, "You can't use block capability and -b/i");
>>
>> Likewise.
>>
>> The condition would be slightly simpler if you completed error checking
>> before changing global state.  Matter of taste.
>
> Being bug compatible has this drawbacks.  I also hate that -i implies -b.
>
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -894,11 +894,14 @@
>>>  # @release-ram: if enabled, qemu will free the migrated ram pages on the source
>>>  #        during postcopy-ram migration. (since 2.9)
>>>  #
>>> +# @block: enable block migration (Since 2.10)
>>> +#
>>
>> What's "block migration" and why should I care?
>
> "enable migration of block devices.  The granularity is all devices or
> no devices, nothing in the middle."
>
> I will be happy with suggestions.

# @block: If enabled, QEMU will also migrate the contents of all block
#          devices.  Default is disabled.  A possible alternative are
#          mirror jobs to a builtin NBD server on the destination, which
#          are more flexible.
#          (Since 2.10)

>>>  # Since: 1.2
>>>  ##
>>>  { 'enum': 'MigrationCapability',
>>>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>> -           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>> +           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
>>> +           'block' ] }
>>>  
>>>  ##
>>>  # @MigrationCapabilityStatus:
>>> @@ -1009,13 +1012,15 @@
>>>  # @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints in
>>>  #          periodic mode. (Since 2.8)
>>>  #
>>> +# @block-incremental: enable block incremental migration (Since 2.10)
>>> +#
>>
>> What's "block incremental migration" and why should I care?
>
> This is good, I will try.
>
> "block incremental migration assumes that we have a base image in both
> sides, and then we continue writting in one of the sides.  This way we
> need to only migrate the changes since the previous state where it was
> the same in both sides".
>
> I am not sure what to put there, really.

Well, to suggest something, I'd first have to figure out WTF incremental
block migration does.  Your text helps me some, but not enough.  What
exactly is being migrated, and what exactly is assumed to be shared
between source and destination?

Block migration is scandalously underdocumented.

>>>  # Since: 2.4
>>>  ##
>>>  { 'enum': 'MigrationParameter',
>>>    'data': ['compress-level', 'compress-threads', 'decompress-threads',
>>>             'cpu-throttle-initial', 'cpu-throttle-increment',
>>>             'tls-creds', 'tls-hostname', 'max-bandwidth',
>>> -           'downtime-limit', 'x-checkpoint-delay' ] }
>>> +           'downtime-limit', 'x-checkpoint-delay', 'block-incremental' ] }
>>>  
>>>  ##
>>>  # @migrate-set-parameters:
>>> @@ -1082,6 +1087,8 @@
>>>  #
>>>  # @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 2.8)
>>>  #
>>> +# @block-incremental: enable block incremental migration (Since 2.10)
>>> +#
>>>  # Since: 2.4
>>>  ##
>>>  { 'struct': 'MigrationParameters',
>>> @@ -1094,7 +1101,8 @@
>>>              '*tls-hostname': 'str',
>>>              '*max-bandwidth': 'int',
>>>              '*downtime-limit': 'int',
>>> -            '*x-checkpoint-delay': 'int'} }
>>> +            '*x-checkpoint-delay': 'int',
>>> +            '*block-incremental': 'bool' } }
>>>  
>>>  ##
>>>  # @query-migrate-parameters:
>>
>> Can't say I like the split between MigrationCapability and
>> MigrationParameters, but we got to work with what we have.
>
> :-(
>
> And it is still not good enough.  There are parameters that make sense
> to change in middle migration (max-bandwidth for example) and others
> that you shouldn't, like tls-hostname or compression-threads.
>
> Go figure.

Grown, not designed.  It happens.

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Create one capability for block migration and one parameter for
>> incremental block migration.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>

>>  
>>      /* The last error that occurred */
>>      Error *error;
>> +    /* Do we have to clean up -b/-i from old migrate paramteres */
>
> parameters

ok.


>> +void migrate_set_block_enabled(bool value, Error **errp)
>> +{
>> +    MigrationCapabilityStatusList *cap;
>> +
>> +    cap = g_malloc0(sizeof(*cap));
>> +    cap->value = g_malloc(sizeof(*cap->value));
>
> I prefer g_new() for extra type checking.  Your choice.

ok.


>> +    cap->value->capability = MIGRATION_CAPABILITY_BLOCK;
>> +    cap->value->state = value;
>> +    qmp_migrate_set_capabilities(cap, errp);
>> +    qapi_free_MigrationCapabilityStatusList(cap);
>> +}
>
> The objective is to set one bit.  The means is building a
> MigrationCapabilityStatusList.  When you have to build an elaborate data
> structure just so you can set one bit, your interfaces are likely
> deficient.  Observation, not change request.

This was Dave suggestion.  The reason for doing this is in the following
patch when we enable compile disable of block migration to put the ifdef
in a single place.  Otherwise, I have to put it in two places.

>> +static void migrate_set_block_incremental(MigrationState *s, bool value)
>> +{
>> +    s->parameters.block_incremental = value;
>> +}
>
> This is how setting one bit should look like.

See previous comment.

>
>> +
>> +static void block_cleanup_parameters(MigrationState *s)
>> +{
>> +    if (s->must_remove_block) {
>> +        migrate_set_block_enabled(false, NULL);
>
> NULL ignores errors.  If an error happens, and we ignore it, we're
> screwed, aren't we?  I suspect we want &error_abort here.

setting to false can't never fail.  It is when we set it to true that it
can fail because it is compiled out (that will be in next patch).


>> @@ -1229,6 +1267,38 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>          return;
>>      }
>>  
>> +    if (has_blk && blk) {
>> +        if (migrate_use_block() || migrate_use_block_incremental()) {
>> +            error_setg(errp, "You can't use block capability and -b/i");
>
> Error message assumes HMP.  In QMP, the thing is called 'blk', not -b.
> Perhaps we can sidestep the issue like this: "Command options are
> incompatible with current migration capabilities".

ok.

>
>> +            return;
>> +        }
>> +        migrate_set_block_enabled(true, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +        block_enabled = true;
>> +        s->must_remove_block = true;
>> +    }
>> +
>> +    if (has_inc && inc) {
>> +        if ((migrate_use_block() && !block_enabled)
>> +            || migrate_use_block_incremental()) {
>> +            error_setg(errp, "You can't use block capability and -b/i");
>
> Likewise.
>
> The condition would be slightly simpler if you completed error checking
> before changing global state.  Matter of taste.

Being bug compatible has this drawbacks.  I also hate that -i implies -b.

>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -894,11 +894,14 @@
>>  # @release-ram: if enabled, qemu will free the migrated ram pages on the source
>>  #        during postcopy-ram migration. (since 2.9)
>>  #
>> +# @block: enable block migration (Since 2.10)
>> +#
>
> What's "block migration" and why should I care?

"enable migration of block devices.  The granularity is all devices or
no devices, nothing in the middle."

I will be happy with suggestions.

>
>>  # Since: 1.2
>>  ##
>>  { 'enum': 'MigrationCapability',
>>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>> -           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>> +           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
>> +           'block' ] }
>>  
>>  ##
>>  # @MigrationCapabilityStatus:
>> @@ -1009,13 +1012,15 @@
>>  # @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints in
>>  #          periodic mode. (Since 2.8)
>>  #
>> +# @block-incremental: enable block incremental migration (Since 2.10)
>> +#
>
> What's "block incremental migration" and why should I care?

This is good, I will try.

"block incremental migration assumes that we have a base image in both
sides, and then we continue writting in one of the sides.  This way we
need to only migrate the changes since the previous state where it was
the same in both sides".

I am not sure what to put there, really.


>
>>  # Since: 2.4
>>  ##
>>  { 'enum': 'MigrationParameter',
>>    'data': ['compress-level', 'compress-threads', 'decompress-threads',
>>             'cpu-throttle-initial', 'cpu-throttle-increment',
>>             'tls-creds', 'tls-hostname', 'max-bandwidth',
>> -           'downtime-limit', 'x-checkpoint-delay' ] }
>> +           'downtime-limit', 'x-checkpoint-delay', 'block-incremental' ] }
>>  
>>  ##
>>  # @migrate-set-parameters:
>> @@ -1082,6 +1087,8 @@
>>  #
>>  # @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 2.8)
>>  #
>> +# @block-incremental: enable block incremental migration (Since 2.10)
>> +#
>>  # Since: 2.4
>>  ##
>>  { 'struct': 'MigrationParameters',
>> @@ -1094,7 +1101,8 @@
>>              '*tls-hostname': 'str',
>>              '*max-bandwidth': 'int',
>>              '*downtime-limit': 'int',
>> -            '*x-checkpoint-delay': 'int'} }
>> +            '*x-checkpoint-delay': 'int',
>> +            '*block-incremental': 'bool' } }
>>  
>>  ##
>>  # @query-migrate-parameters:
>
> Can't say I like the split between MigrationCapability and
> MigrationParameters, but we got to work with what we have.

:-(

And it is still not good enough.  There are parameters that make sense
to change in middle migration (max-bandwidth for example) and others
that you shouldn't, like tls-hostname or compression-threads.

Go figure.

Thanks, Juan.

Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Create one capability for block migration and one parameter for
> incremental block migration.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hmp.c                         | 13 +++++++
>  include/migration/block.h     |  2 +
>  include/migration/migration.h |  7 ++++
>  migration/migration.c         | 88 +++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json              | 14 +++++--
>  5 files changed, 121 insertions(+), 3 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index acbbc5c..208154c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -326,6 +326,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "%s: %" PRId64 "\n",
>              MigrationParameter_lookup[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY],
>              params->x_checkpoint_delay);
> +        assert(params->has_block_incremental);
> +        monitor_printf(mon, "%s: %s\n",
> +            MigrationParameter_lookup[MIGRATION_PARAMETER_BLOCK_INCREMENTAL],
> +                       params->block_incremental ? "on" : "off");
>      }
>  
>      qapi_free_MigrationParameters(params);
> @@ -1527,6 +1531,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>      Visitor *v = string_input_visitor_new(valuestr);
>      uint64_t valuebw = 0;
>      int64_t valueint = 0;
> +    bool valuebool = false;
>      Error *err = NULL;
>      bool use_int_value = false;
>      int i, ret;
> @@ -1581,6 +1586,14 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>                  p.has_x_checkpoint_delay = true;
>                  use_int_value = true;
>                  break;
> +            case MIGRATION_PARAMETER_BLOCK_INCREMENTAL:
> +                p.has_block_incremental = true;
> +                visit_type_bool(v, param, &valuebool, &err);
> +                if (err) {
> +                    goto cleanup;
> +                }
> +                p.block_incremental = valuebool;
> +                break;
>              }
>  
>              if (use_int_value) {
> diff --git a/include/migration/block.h b/include/migration/block.h
> index 41a1ac8..5225af9 100644
> --- a/include/migration/block.h
> +++ b/include/migration/block.h
> @@ -20,4 +20,6 @@ uint64_t blk_mig_bytes_transferred(void);
>  uint64_t blk_mig_bytes_remaining(void);
>  uint64_t blk_mig_bytes_total(void);
>  
> +void migrate_set_block_enabled(bool value, Error **errp);
> +
>  #endif /* MIGRATION_BLOCK_H */
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 47262bd..adcb8f6 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -179,6 +179,10 @@ struct MigrationState
>  
>      /* The last error that occurred */
>      Error *error;
> +    /* Do we have to clean up -b/-i from old migrate paramteres */

parameters

> +    /* This feature is deprecated and will be removed */
> +    bool must_remove_block;
> +    bool must_remove_block_incremental;
>  };
>  
>  void migrate_set_state(int *state, int old_state, int new_state);
> @@ -293,6 +297,9 @@ bool migrate_colo_enabled(void);
>  
>  int64_t xbzrle_cache_resize(int64_t new_size);
>  
> +bool migrate_use_block(void);
> +bool migrate_use_block_incremental(void);
> +
>  bool migrate_use_compression(void);
>  int migrate_compress_level(void);
>  int migrate_compress_threads(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index 5c92851..03f96df 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -599,6 +599,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->downtime_limit = s->parameters.downtime_limit;
>      params->has_x_checkpoint_delay = true;
>      params->x_checkpoint_delay = s->parameters.x_checkpoint_delay;
> +    params->has_block_incremental = true;
> +    params->block_incremental = s->parameters.block_incremental;
>  
>      return params;
>  }
> @@ -907,6 +909,9 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>              colo_checkpoint_notify(s);
>          }
>      }
> +    if (params->has_block_incremental) {
> +        s->parameters.block_incremental = params->block_incremental;
> +    }
>  }
>  
>  
> @@ -942,6 +947,35 @@ void migrate_set_state(int *state, int old_state, int new_state)
>      }
>  }
>  
> +void migrate_set_block_enabled(bool value, Error **errp)
> +{
> +    MigrationCapabilityStatusList *cap;
> +
> +    cap = g_malloc0(sizeof(*cap));
> +    cap->value = g_malloc(sizeof(*cap->value));

I prefer g_new() for extra type checking.  Your choice.

> +    cap->value->capability = MIGRATION_CAPABILITY_BLOCK;
> +    cap->value->state = value;
> +    qmp_migrate_set_capabilities(cap, errp);
> +    qapi_free_MigrationCapabilityStatusList(cap);
> +}

The objective is to set one bit.  The means is building a
MigrationCapabilityStatusList.  When you have to build an elaborate data
structure just so you can set one bit, your interfaces are likely
deficient.  Observation, not change request.

> +
> +static void migrate_set_block_incremental(MigrationState *s, bool value)
> +{
> +    s->parameters.block_incremental = value;
> +}

This is how setting one bit should look like.

> +
> +static void block_cleanup_parameters(MigrationState *s)
> +{
> +    if (s->must_remove_block) {
> +        migrate_set_block_enabled(false, NULL);

NULL ignores errors.  If an error happens, and we ignore it, we're
screwed, aren't we?  I suspect we want &error_abort here.

> +        s->must_remove_block = false;
> +    }
> +    if (s->must_remove_block_incremental) {
> +        migrate_set_block_incremental(s, false);
> +        s->must_remove_block_incremental = false;
> +    }
> +}
> +
>  static void migrate_fd_cleanup(void *opaque)
>  {
>      MigrationState *s = opaque;
> @@ -974,6 +1008,7 @@ static void migrate_fd_cleanup(void *opaque)
>      }
>  
>      notifier_list_notify(&migration_state_notifiers, s);
> +    block_cleanup_parameters(s);
>  }
>  
>  void migrate_fd_error(MigrationState *s, const Error *error)
> @@ -986,6 +1021,7 @@ void migrate_fd_error(MigrationState *s, const Error *error)
>          s->error = error_copy(error);
>      }
>      notifier_list_notify(&migration_state_notifiers, s);
> +    block_cleanup_parameters(s);
>  }
>  
>  static void migrate_fd_cancel(MigrationState *s)
> @@ -1027,6 +1063,7 @@ static void migrate_fd_cancel(MigrationState *s)
>              s->block_inactive = false;
>          }
>      }
> +    block_cleanup_parameters(s);
>  }
>  
>  void add_migration_state_change_notifier(Notifier *notify)
> @@ -1209,6 +1246,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
>      MigrationParams params;
> +    bool block_enabled = false;
>      const char *p;
>  
>      params.blk = has_blk && blk;
> @@ -1229,6 +1267,38 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          return;
>      }
>  
> +    if (has_blk && blk) {
> +        if (migrate_use_block() || migrate_use_block_incremental()) {
> +            error_setg(errp, "You can't use block capability and -b/i");

Error message assumes HMP.  In QMP, the thing is called 'blk', not -b.
Perhaps we can sidestep the issue like this: "Command options are
incompatible with current migration capabilities".

> +            return;
> +        }
> +        migrate_set_block_enabled(true, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +        block_enabled = true;
> +        s->must_remove_block = true;
> +    }
> +
> +    if (has_inc && inc) {
> +        if ((migrate_use_block() && !block_enabled)
> +            || migrate_use_block_incremental()) {
> +            error_setg(errp, "You can't use block capability and -b/i");

Likewise.

The condition would be slightly simpler if you completed error checking
before changing global state.  Matter of taste.

> +            return;
> +        }
> +        if (!block_enabled) {
> +            migrate_set_block_enabled(true, &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
> +            s->must_remove_block = true;
> +        }
> +        migrate_set_block_incremental(s, true);
> +        s->must_remove_block_incremental = true;
> +    }
> +
>      s = migrate_init(&params);
>  
>      if (strstart(uri, "tcp:", &p)) {
> @@ -1426,6 +1496,24 @@ int64_t migrate_xbzrle_cache_size(void)
>      return s->xbzrle_cache_size;
>  }
>  
> +bool migrate_use_block(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK];
> +}
> +
> +bool migrate_use_block_incremental(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    return s->parameters.block_incremental;
> +}
> +
>  /* migration thread support */
>  /*
>   * Something bad happened to the RP stream, mark an error
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5728b7f..62246bc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -894,11 +894,14 @@
>  # @release-ram: if enabled, qemu will free the migrated ram pages on the source
>  #        during postcopy-ram migration. (since 2.9)
>  #
> +# @block: enable block migration (Since 2.10)
> +#

What's "block migration" and why should I care?

>  # Since: 1.2
>  ##
>  { 'enum': 'MigrationCapability',
>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> -           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
> +           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
> +           'block' ] }
>  
>  ##
>  # @MigrationCapabilityStatus:
> @@ -1009,13 +1012,15 @@
>  # @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints in
>  #          periodic mode. (Since 2.8)
>  #
> +# @block-incremental: enable block incremental migration (Since 2.10)
> +#

What's "block incremental migration" and why should I care?

>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
>    'data': ['compress-level', 'compress-threads', 'decompress-threads',
>             'cpu-throttle-initial', 'cpu-throttle-increment',
>             'tls-creds', 'tls-hostname', 'max-bandwidth',
> -           'downtime-limit', 'x-checkpoint-delay' ] }
> +           'downtime-limit', 'x-checkpoint-delay', 'block-incremental' ] }
>  
>  ##
>  # @migrate-set-parameters:
> @@ -1082,6 +1087,8 @@
>  #
>  # @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 2.8)
>  #
> +# @block-incremental: enable block incremental migration (Since 2.10)
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -1094,7 +1101,8 @@
>              '*tls-hostname': 'str',
>              '*max-bandwidth': 'int',
>              '*downtime-limit': 'int',
> -            '*x-checkpoint-delay': 'int'} }
> +            '*x-checkpoint-delay': 'int',
> +            '*block-incremental': 'bool' } }
>  
>  ##
>  # @query-migrate-parameters:

Can't say I like the split between MigrationCapability and
MigrationParameters, but we got to work with what we have.

[Qemu-devel] [PATCH 2/5] migration: Create block capability

[Juan Quintela]

Create one capability for block migration and one parameter for
incremental block migration.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hmp.c                         | 13 +++++++
 include/migration/block.h     |  2 +
 include/migration/migration.h |  7 ++++
 migration/migration.c         | 88 +++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json              | 14 +++++--
 5 files changed, 121 insertions(+), 3 deletions(-)

diff --git a/hmp.c b/hmp.c
index acbbc5c..208154c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -326,6 +326,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %" PRId64 "\n",
             MigrationParameter_lookup[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY],
             params->x_checkpoint_delay);
+        assert(params->has_block_incremental);
+        monitor_printf(mon, "%s: %s\n",
+            MigrationParameter_lookup[MIGRATION_PARAMETER_BLOCK_INCREMENTAL],
+                       params->block_incremental ? "on" : "off");
     }
 
     qapi_free_MigrationParameters(params);
@@ -1527,6 +1531,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     Visitor *v = string_input_visitor_new(valuestr);
     uint64_t valuebw = 0;
     int64_t valueint = 0;
+    bool valuebool = false;
     Error *err = NULL;
     bool use_int_value = false;
     int i, ret;
@@ -1581,6 +1586,14 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                 p.has_x_checkpoint_delay = true;
                 use_int_value = true;
                 break;
+            case MIGRATION_PARAMETER_BLOCK_INCREMENTAL:
+                p.has_block_incremental = true;
+                visit_type_bool(v, param, &valuebool, &err);
+                if (err) {
+                    goto cleanup;
+                }
+                p.block_incremental = valuebool;
+                break;
             }
 
             if (use_int_value) {
diff --git a/include/migration/block.h b/include/migration/block.h
index 41a1ac8..5225af9 100644
--- a/include/migration/block.h
+++ b/include/migration/block.h
@@ -20,4 +20,6 @@ uint64_t blk_mig_bytes_transferred(void);
 uint64_t blk_mig_bytes_remaining(void);
 uint64_t blk_mig_bytes_total(void);
 
+void migrate_set_block_enabled(bool value, Error **errp);
+
 #endif /* MIGRATION_BLOCK_H */
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 47262bd..adcb8f6 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -179,6 +179,10 @@ struct MigrationState
 
     /* The last error that occurred */
     Error *error;
+    /* Do we have to clean up -b/-i from old migrate paramteres */
+    /* This feature is deprecated and will be removed */
+    bool must_remove_block;
+    bool must_remove_block_incremental;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
@@ -293,6 +297,9 @@ bool migrate_colo_enabled(void);
 
 int64_t xbzrle_cache_resize(int64_t new_size);
 
+bool migrate_use_block(void);
+bool migrate_use_block_incremental(void);
+
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
diff --git a/migration/migration.c b/migration/migration.c
index 5c92851..03f96df 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -599,6 +599,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->downtime_limit = s->parameters.downtime_limit;
     params->has_x_checkpoint_delay = true;
     params->x_checkpoint_delay = s->parameters.x_checkpoint_delay;
+    params->has_block_incremental = true;
+    params->block_incremental = s->parameters.block_incremental;
 
     return params;
 }
@@ -907,6 +909,9 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
             colo_checkpoint_notify(s);
         }
     }
+    if (params->has_block_incremental) {
+        s->parameters.block_incremental = params->block_incremental;
+    }
 }
 
 
@@ -942,6 +947,35 @@ void migrate_set_state(int *state, int old_state, int new_state)
     }
 }
 
+void migrate_set_block_enabled(bool value, Error **errp)
+{
+    MigrationCapabilityStatusList *cap;
+
+    cap = g_malloc0(sizeof(*cap));
+    cap->value = g_malloc(sizeof(*cap->value));
+    cap->value->capability = MIGRATION_CAPABILITY_BLOCK;
+    cap->value->state = value;
+    qmp_migrate_set_capabilities(cap, errp);
+    qapi_free_MigrationCapabilityStatusList(cap);
+}
+
+static void migrate_set_block_incremental(MigrationState *s, bool value)
+{
+    s->parameters.block_incremental = value;
+}
+
+static void block_cleanup_parameters(MigrationState *s)
+{
+    if (s->must_remove_block) {
+        migrate_set_block_enabled(false, NULL);
+        s->must_remove_block = false;
+    }
+    if (s->must_remove_block_incremental) {
+        migrate_set_block_incremental(s, false);
+        s->must_remove_block_incremental = false;
+    }
+}
+
 static void migrate_fd_cleanup(void *opaque)
 {
     MigrationState *s = opaque;
@@ -974,6 +1008,7 @@ static void migrate_fd_cleanup(void *opaque)
     }
 
     notifier_list_notify(&migration_state_notifiers, s);
+    block_cleanup_parameters(s);
 }
 
 void migrate_fd_error(MigrationState *s, const Error *error)
@@ -986,6 +1021,7 @@ void migrate_fd_error(MigrationState *s, const Error *error)
         s->error = error_copy(error);
     }
     notifier_list_notify(&migration_state_notifiers, s);
+    block_cleanup_parameters(s);
 }
 
 static void migrate_fd_cancel(MigrationState *s)
@@ -1027,6 +1063,7 @@ static void migrate_fd_cancel(MigrationState *s)
             s->block_inactive = false;
         }
     }
+    block_cleanup_parameters(s);
 }
 
 void add_migration_state_change_notifier(Notifier *notify)
@@ -1209,6 +1246,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
     MigrationParams params;
+    bool block_enabled = false;
     const char *p;
 
     params.blk = has_blk && blk;
@@ -1229,6 +1267,38 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         return;
     }
 
+    if (has_blk && blk) {
+        if (migrate_use_block() || migrate_use_block_incremental()) {
+            error_setg(errp, "You can't use block capability and -b/i");
+            return;
+        }
+        migrate_set_block_enabled(true, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+        block_enabled = true;
+        s->must_remove_block = true;
+    }
+
+    if (has_inc && inc) {
+        if ((migrate_use_block() && !block_enabled)
+            || migrate_use_block_incremental()) {
+            error_setg(errp, "You can't use block capability and -b/i");
+            return;
+        }
+        if (!block_enabled) {
+            migrate_set_block_enabled(true, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+            s->must_remove_block = true;
+        }
+        migrate_set_block_incremental(s, true);
+        s->must_remove_block_incremental = true;
+    }
+
     s = migrate_init(&params);
 
     if (strstart(uri, "tcp:", &p)) {
@@ -1426,6 +1496,24 @@ int64_t migrate_xbzrle_cache_size(void)
     return s->xbzrle_cache_size;
 }
 
+bool migrate_use_block(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK];
+}
+
+bool migrate_use_block_incremental(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->parameters.block_incremental;
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/qapi-schema.json b/qapi-schema.json
index 5728b7f..62246bc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -894,11 +894,14 @@
 # @release-ram: if enabled, qemu will free the migrated ram pages on the source
 #        during postcopy-ram migration. (since 2.9)
 #
+# @block: enable block migration (Since 2.10)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
+           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
+           'block' ] }
 
 ##
 # @MigrationCapabilityStatus:
@@ -1009,13 +1012,15 @@
 # @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints in
 #          periodic mode. (Since 2.8)
 #
+# @block-incremental: enable block incremental migration (Since 2.10)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
   'data': ['compress-level', 'compress-threads', 'decompress-threads',
            'cpu-throttle-initial', 'cpu-throttle-increment',
            'tls-creds', 'tls-hostname', 'max-bandwidth',
-           'downtime-limit', 'x-checkpoint-delay' ] }
+           'downtime-limit', 'x-checkpoint-delay', 'block-incremental' ] }
 
 ##
 # @migrate-set-parameters:
@@ -1082,6 +1087,8 @@
 #
 # @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 2.8)
 #
+# @block-incremental: enable block incremental migration (Since 2.10)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -1094,7 +1101,8 @@
             '*tls-hostname': 'str',
             '*max-bandwidth': 'int',
             '*downtime-limit': 'int',
-            '*x-checkpoint-delay': 'int'} }
+            '*x-checkpoint-delay': 'int',
+            '*block-incremental': 'bool' } }
 
 ##
 # @query-migrate-parameters:
-- 
2.9.3