[PATCH v5 0/7] Migration deprecated parts

[PATCH v5 0/7] Migration deprecated parts

[Juan Quintela]

Based on: Message-ID: <20231017083003.15951-1-quintela@redhat.com>
          Migration 20231017 patches

On this v5:
- Rebased on top of last migration pull requesnt:

- address markus comments.  Basically we recommend always
  blockdev-mirror + NBD.  In deprecated.rst we also put the posiblity
  of using block-incremental and block, but we state that they are
  also deprecated.
  I know, I know, I deprecated them in the following patch.

- Dropped the removal of block-migration and block-incremental I am
  only interested in showing why I want to remove the -b/-i options.

Please review.

Later, Juan.

On this v4:
- addressed all markus comments.
- rebased on latest.
- improve formatting of migration.json
- print block migration status when needed.
- patches 7-10 are not mean to merge, they just show why we want to
  deprecate block migration and remove its support.
- Patch 7 just drop support for -i/-b and qmp equivalents.
- Patch 8 shows all the helpers and convolutions we need to have to
  support that -i and -d.
- patch 9 drops block-incremental migration support.
- patch 9 drops block migration support.

Please review.

Thanks, Juan.

On this v3:

- Rebase on top of upstream.
- Changed v8.1 to 8.2 (I left the reviewed by anyways)
- missing the block deprecation code, please.

Please, review.

Later, Juan.

On this v2:

- dropped -incoming <uri> deprecation
  Paolo came with a better solution using keyvalues.

- skipped field is already ready for next pull request, so dropped.

- dropped the RFC bits, nermal PATCH.

- Assessed all the review comments.

- Added indentation of migration.json.

- Used the documentation pointer to substitute block migration.

Please review.

[v1]
Hi this series describe the migration parts that have to be deprecated.

- It is an rfc because I doubt that I did the deprecation process right. Hello Markus O:-)

- skipped field: It is older than me, I have never know what it stands
  for.  As far as I know it has always been zero.

- inc/blk migrate command options.  They are only used by block
  migration (that I deprecate on the following patch).  And they are really bad.
  grep must_remove_block_options.

- block migration.  block jobs, whatever they are called this week are
  way more flexible.  Current code works, but we broke it here and
  there, and really nobody has stand up to maintain it.  It is quite
  contained and can be left there.  Is anyone really using it?

- old compression method.  It don't work.  See last try from Lukas to
  make a test that works reliabely.  I failed with the same task years
  ago.  It is really slow, and if compression is good for you, multifd
  + zlib is going to perform/compress way more.

  I don't know what to do with this code, really.

  * Remove it for this release?  It don't work, and haven't work
    reliabely in quite a few time.

  * Deprecate it and remove in another couple of releases, i.e. normal
    deprecation.

  * Ideas?

- -incoming <uri>

  if you need to set parameters (multifd cames to mind, and preempt has
  the same problem), you really needs to use defer.  So what should we do here?

  This part is not urget, because management apps have a working
  option that are already using "defer", and the code simplifacation
  if we remove it is not so big.  So we can leave it until 9.0 or
  whatever we think fit.

What do you think?

Later, Juan.

Juan Quintela (7):
  migration: Print block status when needed
  migration: migrate 'inc' command option is deprecated.
  migration: migrate 'blk' command option is deprecated.
  migration: Deprecate block migration
  migration: Deprecate old compression method
  [RFC] migration: Make -i/-b an error for hmp and qmp
  [RFC] migration: Remove helpers needed for -i/-b migrate options

 docs/about/deprecated.rst      |  36 +++++++++++
 qapi/migration.json            | 110 ++++++++++++++++++++++++---------
 migration/migration.h          |   4 --
 migration/options.h            |   6 --
 migration/block.c              |   3 +
 migration/migration-hmp-cmds.c |  18 ++++--
 migration/migration.c          |  35 ++++-------
 migration/options.c            |  63 +++++++------------
 8 files changed, 165 insertions(+), 110 deletions(-)

-- 
2.41.0

[PATCH v5 1/7] migration: Print block status when needed

[Juan Quintela]

The new line was only printed when command options were used.  When we
used migration parameters and capabilities, it wasn't.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-hmp-cmds.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index d206700a43..a82597f18e 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -30,6 +30,7 @@
 #include "sysemu/runstate.h"
 #include "ui/qemu-spice.h"
 #include "sysemu/sysemu.h"
+#include "options.h"
 #include "migration.h"
 
 static void migration_global_dump(Monitor *mon)
@@ -696,7 +697,6 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
 typedef struct HMPMigrationStatus {
     QEMUTimer *timer;
     Monitor *mon;
-    bool is_block_migration;
 } HMPMigrationStatus;
 
 static void hmp_migrate_status_cb(void *opaque)
@@ -722,7 +722,7 @@ static void hmp_migrate_status_cb(void *opaque)
 
         timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 1000);
     } else {
-        if (status->is_block_migration) {
+        if (migrate_block()) {
             monitor_printf(status->mon, "\n");
         }
         if (info->error_desc) {
@@ -762,7 +762,6 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 
         status = g_malloc0(sizeof(*status));
         status->mon = mon;
-        status->is_block_migration = blk || inc;
         status->timer = timer_new_ms(QEMU_CLOCK_REALTIME, hmp_migrate_status_cb,
                                           status);
         timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
-- 
2.41.0

[PATCH v5 2/7] migration: migrate 'inc' command option is deprecated.

[Juan Quintela]

Use blockdev-mirror with NBD instead.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>

---

Improve documentation and style (thanks Markus)
---
 docs/about/deprecated.rst      | 8 ++++++++
 qapi/migration.json            | 8 +++++++-
 migration/migration-hmp-cmds.c | 5 +++++
 migration/migration.c          | 5 +++++
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 2febd2d12f..fc6adf1dea 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -461,3 +461,11 @@ Migration
 ``skipped`` field in Migration stats has been deprecated.  It hasn't
 been used for more than 10 years.
 
+``inc`` migrate command option (since 8.2)
+''''''''''''''''''''''''''''''''''''''''''
+
+Use blockdev-mirror with NBD instead.
+
+As an intermediate step the ``inc`` functionality can be achieved by
+setting the ``block-incremental`` migration parameter to ``true``.
+But this parameter is also deprecated.
diff --git a/qapi/migration.json b/qapi/migration.json
index db3df12d6c..fa7f4f2575 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1524,6 +1524,11 @@
 #
 # @resume: resume one paused migration, default "off". (since 3.0)
 #
+# Features:
+#
+# @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
+#     NBD instead.
+#
 # Returns: nothing on success
 #
 # Since: 0.14
@@ -1545,7 +1550,8 @@
 # <- { "return": {} }
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
+  'data': {'uri': 'str', '*blk': 'bool',
+           '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
            '*detach': 'bool', '*resume': 'bool' } }
 
 ##
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index a82597f18e..fee7079afa 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -745,6 +745,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     const char *uri = qdict_get_str(qdict, "uri");
     Error *err = NULL;
 
+    if (inc) {
+        warn_report("option '-i' is deprecated.  Use 'blockdev-mirror + NBD'"
+                    " instead.");
+    }
+
     qmp_migrate(uri, !!blk, blk, !!inc, inc,
                 false, false, true, resume, &err);
     if (hmp_handle_error(mon, err)) {
diff --git a/migration/migration.c b/migration/migration.c
index 6ba5e145ac..b8b3ba58df 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1603,6 +1603,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
 {
     Error *local_err = NULL;
 
+    if (blk_inc) {
+        warn_report("parameter 'inc' is deprecated.  Use blockdev-mirror with"
+                    " NBD instead");
+    }
+
     if (resume) {
         if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
             error_setg(errp, "Cannot resume if there is no "
-- 
2.41.0

[PATCH v5 3/7] migration: migrate 'blk' command option is deprecated.

[Juan Quintela]

Use blocked-mirror with NBD instead.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>

---

Improve documentation and style (markus)
---
 docs/about/deprecated.rst      | 10 ++++++++++
 qapi/migration.json            |  6 ++++--
 migration/migration-hmp-cmds.c |  5 +++++
 migration/migration.c          |  5 +++++
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index fc6adf1dea..0149f040b6 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -469,3 +469,13 @@ Use blockdev-mirror with NBD instead.
 As an intermediate step the ``inc`` functionality can be achieved by
 setting the ``block-incremental`` migration parameter to ``true``.
 But this parameter is also deprecated.
+
+``blk`` migrate command option (since 8.2)
+''''''''''''''''''''''''''''''''''''''''''
+
+Use blockdev-mirror with NBD instead.
+
+As an intermediate step the ``blk`` functionality can be achieved by
+setting the ``block`` migration capability to ``true``.
+But this capability is also deprecated.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index fa7f4f2575..59a07b50f0 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1527,7 +1527,8 @@
 # Features:
 #
 # @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
-#     NBD instead.
+#     NBD instead.  Member @blk is deprecated.  Use blockdev-mirror
+#     with NBD instead.
 #
 # Returns: nothing on success
 #
@@ -1550,7 +1551,8 @@
 # <- { "return": {} }
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool',
+  'data': {'uri': 'str',
+           '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
            '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
            '*detach': 'bool', '*resume': 'bool' } }
 
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index fee7079afa..bfeb1a476a 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -750,6 +750,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
                     " instead.");
     }
 
+    if (blk) {
+        warn_report("option '-b' is deprecated.  Use 'blockdev-mirror + NBD'"
+                    " instead.");
+    }
+
     qmp_migrate(uri, !!blk, blk, !!inc, inc,
                 false, false, true, resume, &err);
     if (hmp_handle_error(mon, err)) {
diff --git a/migration/migration.c b/migration/migration.c
index b8b3ba58df..4da7fcfe0f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1608,6 +1608,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
                     " NBD instead");
     }
 
+    if (blk) {
+        warn_report("capability 'blk is deprecated.  Use blockdev-mirror with"
+                    " NBD instead");
+    }
+
     if (resume) {
         if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
             error_setg(errp, "Cannot resume if there is no "
-- 
2.41.0

[PATCH v5 4/7] migration: Deprecate block migration

[Juan Quintela]

It is obsolete.  It is better to use driver-mirror with NBD instead.

CC: Kevin Wolf <kwolf@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Hanna Czenczek <hreitz@redhat.com>

Signed-off-by: Juan Quintela <quintela@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 docs/about/deprecated.rst | 10 ++++++++++
 qapi/migration.json       | 29 ++++++++++++++++++++++++-----
 migration/block.c         |  3 +++
 migration/options.c       |  9 ++++++++-
 4 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 0149f040b6..5eaf096040 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -479,3 +479,13 @@ As an intermediate step the ``blk`` functionality can be achieved by
 setting the ``block`` migration capability to ``true``.
 But this capability is also deprecated.
 
+block migration (since 8.2)
+'''''''''''''''''''''''''''
+
+Block migration is too inflexible.  It needs to migrate all block
+devices or none.
+
+Please see "QMP invocation for live storage migration with
+``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst
+for a detailed explanation.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index 59a07b50f0..c7633b22c0 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -269,11 +269,15 @@
 #     average memory load of the virtual CPU indirectly.  Note that
 #     zero means guest doesn't dirty memory.  (Since 8.1)
 #
+# Features:
+#
+# @deprecated: Member @disk is deprecated because block migration is.
+#
 # Since: 0.14
 ##
 { 'struct': 'MigrationInfo',
   'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
-           '*disk': 'MigrationStats',
+           '*disk': { 'type': 'MigrationStats', 'features': [ 'deprecated' ] },
            '*vfio': 'VfioStats',
            '*xbzrle-cache': 'XBZRLECacheStats',
            '*total-time': 'int',
@@ -525,6 +529,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block is deprecated.  Use blockdev-mirror with
+#     NBD instead.
+#
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
 #
 # Since: 1.2
@@ -534,7 +541,8 @@
            'compress', 'events', 'postcopy-ram',
            { 'name': 'x-colo', 'features': [ 'unstable' ] },
            'release-ram',
-           'block', 'return-path', 'pause-before-switchover', 'multifd',
+           { 'name': 'block', 'features': [ 'deprecated' ] },
+           'return-path', 'pause-before-switchover', 'multifd',
            'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
            { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
            'validate-uuid', 'background-snapshot',
@@ -835,6 +843,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block-incremental is deprecated. Use
+#     blockdev-mirror with NBD instead.
+#
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 #     are experimental.
 #
@@ -850,7 +861,7 @@
            'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
            'avail-switchover-bandwidth', 'downtime-limit',
            { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
-           'block-incremental',
+           { 'name': 'block-incremental', 'features': [ 'deprecated' ] },
            'multifd-channels',
            'xbzrle-cache-size', 'max-postcopy-bandwidth',
            'max-cpu-throttle', 'multifd-compression',
@@ -1011,6 +1022,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block-incremental is deprecated. Use
+#     blockdev-mirror with NBD instead.
+#
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 #     are experimental.
 #
@@ -1040,7 +1054,8 @@
             '*downtime-limit': 'uint64',
             '*x-checkpoint-delay': { 'type': 'uint32',
                                      'features': [ 'unstable' ] },
-            '*block-incremental': 'bool',
+            '*block-incremental': { 'type': 'bool',
+                                    'features': [ 'deprecated' ] },
             '*multifd-channels': 'uint8',
             '*xbzrle-cache-size': 'size',
             '*max-postcopy-bandwidth': 'size',
@@ -1225,6 +1240,9 @@
 #
 # Features:
 #
+# @deprecated: Member @block-incremental is deprecated. Use
+#     blockdev-mirror with NBD instead.
+#
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 #     are experimental.
 #
@@ -1251,7 +1269,8 @@
             '*downtime-limit': 'uint64',
             '*x-checkpoint-delay': { 'type': 'uint32',
                                      'features': [ 'unstable' ] },
-            '*block-incremental': 'bool',
+            '*block-incremental': { 'type': 'bool',
+                                    'features': [ 'deprecated' ] },
             '*multifd-channels': 'uint8',
             '*xbzrle-cache-size': 'size',
             '*max-postcopy-bandwidth': 'size',
diff --git a/migration/block.c b/migration/block.c
index b60698d6e2..7682f4fbd2 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -731,6 +731,9 @@ static int block_save_setup(QEMUFile *f, void *opaque)
     trace_migration_block_save("setup", block_mig_state.submitted,
                                block_mig_state.transferred);
 
+    warn_report("block migration is deprecated.  Use blockdev-mirror with"
+                "NBD instead.");
+
     ret = init_blk_migration(f);
     if (ret < 0) {
         return ret;
diff --git a/migration/options.c b/migration/options.c
index 42fb818956..0d0a3f8edb 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "exec/target_page.h"
 #include "qapi/clone-visitor.h"
 #include "qapi/error.h"
@@ -473,10 +474,14 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
     if (new_caps[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");
+        error_append_hint(errp, "Use blockdev-mirror with NBD instead.\n");
         return false;
     }
 #endif
+    if (new_caps[MIGRATION_CAPABILITY_BLOCK]) {
+        warn_report("Block migration is deprecated. "
+                    "Use blockdev-mirror with NBD instead.");
+    }
 
 #ifndef CONFIG_REPLICATION
     if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
@@ -1386,6 +1391,8 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
     }
 
     if (params->has_block_incremental) {
+        warn_report("Block migration is deprecated. "
+                    "Use blockdev-mirror with NBD instead.");
         s->parameters.block_incremental = params->block_incremental;
     }
     if (params->has_multifd_channels) {
-- 
2.41.0

[PATCH v5 5/7] migration: Deprecate old compression method

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Peter Xu <peterx@redhat.com>
---
 docs/about/deprecated.rst |  8 ++++
 qapi/migration.json       | 79 +++++++++++++++++++++++++--------------
 migration/options.c       | 13 +++++++
 3 files changed, 72 insertions(+), 28 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 5eaf096040..f46baf9ee9 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -489,3 +489,11 @@ Please see "QMP invocation for live storage migration with
 ``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst
 for a detailed explanation.
 
+old compression method (since 8.2)
+''''''''''''''''''''''''''''''''''
+
+Compression method fails too much.  Too many races.  We are going to
+remove it if nobody fixes it.  For starters, migration-test
+compression tests are disabled becase they fail randomly.  If you need
+compression, use multifd compression methods.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index c7633b22c0..834506a02b 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -272,6 +272,10 @@
 # Features:
 #
 # @deprecated: Member @disk is deprecated because block migration is.
+#     Member @compression is deprecated because it is unreliable and
+#     untested. It is recommended to use multifd migration, which
+#     offers an alternative compression implementation that is
+#     reliable and tested.
 #
 # Since: 0.14
 ##
@@ -289,7 +293,7 @@
            '*blocked-reasons': ['str'],
            '*postcopy-blocktime': 'uint32',
            '*postcopy-vcpu-blocktime': ['uint32'],
-           '*compression': 'CompressionStats',
+           '*compression': { 'type': 'CompressionStats', 'features': [ 'deprecated' ] },
            '*socket-address': ['SocketAddress'],
            '*dirty-limit-throttle-time-per-round': 'uint64',
            '*dirty-limit-ring-full-time': 'uint64'} }
@@ -530,7 +534,10 @@
 # Features:
 #
 # @deprecated: Member @block is deprecated.  Use blockdev-mirror with
-#     NBD instead.
+#     NBD instead.  Member @compression is deprecated because it is
+#     unreliable and untested. It is recommended to use multifd
+#     migration, which offers an alternative compression
+#     implementation that is reliable and tested.
 #
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
 #
@@ -538,7 +545,8 @@
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-           'compress', 'events', 'postcopy-ram',
+           { 'name': 'compress', 'features': [ 'deprecated' ] },
+           'events', 'postcopy-ram',
            { 'name': 'x-colo', 'features': [ 'unstable' ] },
            'release-ram',
            { 'name': 'block', 'features': [ 'deprecated' ] },
@@ -844,7 +852,9 @@
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated. Use
-#     blockdev-mirror with NBD instead.
+#     blockdev-mirror with NBD instead. Members @compress-level,
+#     @compress-threads, @decompress-threads and @compress-wait-thread
+#     are deprecated because @compression is deprecated.
 #
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 #     are experimental.
@@ -854,8 +864,11 @@
 { 'enum': 'MigrationParameter',
   'data': ['announce-initial', 'announce-max',
            'announce-rounds', 'announce-step',
-           'compress-level', 'compress-threads', 'decompress-threads',
-           'compress-wait-thread', 'throttle-trigger-threshold',
+           { 'name': 'compress-level', 'features': [ 'deprecated' ] },
+           { 'name': 'compress-threads', 'features': [ 'deprecated' ] },
+           { 'name': 'decompress-threads', 'features': [ 'deprecated' ] },
+           { 'name': 'compress-wait-thread', 'features': [ 'deprecated' ] },
+           'throttle-trigger-threshold',
            'cpu-throttle-initial', 'cpu-throttle-increment',
            'cpu-throttle-tailslow',
            'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
@@ -885,16 +898,16 @@
 # @announce-step: Increase in delay (in milliseconds) between
 #     subsequent packets in the announcement (Since 4.0)
 #
-# @compress-level: compression level
+# @compress-level: compression level.
 #
-# @compress-threads: compression thread count
+# @compress-threads: compression thread count.
 #
 # @compress-wait-thread: Controls behavior when all compression
 #     threads are currently busy.  If true (default), wait for a free
 #     compression thread to become available; otherwise, send the page
-#     uncompressed.  (Since 3.1)
+#     uncompressed. (Since 3.1)
 #
-# @decompress-threads: decompression thread count
+# @decompress-threads: decompression thread count.
 #
 # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
 #     bytes_xfer_period to trigger throttling.  It is expressed as
@@ -1023,7 +1036,9 @@
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated. Use
-#     blockdev-mirror with NBD instead.
+#     blockdev-mirror with NBD instead. Members @compress-level,
+#     @compress-threads, @decompress-threads and @compress-wait-thread
+#     are deprecated because @compression is deprecated.
 #
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 #     are experimental.
@@ -1038,10 +1053,14 @@
             '*announce-max': 'size',
             '*announce-rounds': 'size',
             '*announce-step': 'size',
-            '*compress-level': 'uint8',
-            '*compress-threads': 'uint8',
-            '*compress-wait-thread': 'bool',
-            '*decompress-threads': 'uint8',
+            '*compress-level': { 'type': 'uint8',
+                                 'features': [ 'deprecated' ] },
+            '*compress-threads':  { 'type': 'uint8',
+                                    'features': [ 'deprecated' ] },
+            '*compress-wait-thread':  { 'type': 'bool',
+                                        'features': [ 'deprecated' ] },
+            '*decompress-threads':  { 'type': 'uint8',
+                                      'features': [ 'deprecated' ] },
             '*throttle-trigger-threshold': 'uint8',
             '*cpu-throttle-initial': 'uint8',
             '*cpu-throttle-increment': 'uint8',
@@ -1078,7 +1097,7 @@
 # Example:
 #
 # -> { "execute": "migrate-set-parameters" ,
-#      "arguments": { "compress-level": 1 } }
+#      "arguments": { "multifd-channels": 5 } }
 # <- { "return": {} }
 ##
 { 'command': 'migrate-set-parameters', 'boxed': true,
@@ -1101,16 +1120,16 @@
 # @announce-step: Increase in delay (in milliseconds) between
 #     subsequent packets in the announcement (Since 4.0)
 #
-# @compress-level: compression level
+# @compress-level: compression level.
 #
-# @compress-threads: compression thread count
+# @compress-threads: compression thread count.
 #
 # @compress-wait-thread: Controls behavior when all compression
 #     threads are currently busy.  If true (default), wait for a free
 #     compression thread to become available; otherwise, send the page
-#     uncompressed.  (Since 3.1)
+#     uncompressed. (Since 3.1)
 #
-# @decompress-threads: decompression thread count
+# @decompress-threads: decompression thread count.
 #
 # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
 #     bytes_xfer_period to trigger throttling.  It is expressed as
@@ -1241,7 +1260,9 @@
 # Features:
 #
 # @deprecated: Member @block-incremental is deprecated. Use
-#     blockdev-mirror with NBD instead.
+#     blockdev-mirror with NBD instead. Members @compress-level,
+#     @compress-threads, @decompress-threads and @compress-wait-thread
+#     are deprecated because @compression is deprecated.
 #
 # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
 #     are experimental.
@@ -1253,10 +1274,14 @@
             '*announce-max': 'size',
             '*announce-rounds': 'size',
             '*announce-step': 'size',
-            '*compress-level': 'uint8',
-            '*compress-threads': 'uint8',
-            '*compress-wait-thread': 'bool',
-            '*decompress-threads': 'uint8',
+            '*compress-level': { 'type': 'uint8',
+                                 'features': [ 'deprecated' ] },
+            '*compress-threads': { 'type': 'uint8',
+                                   'features': [ 'deprecated' ] },
+            '*compress-wait-thread': { 'type': 'bool',
+                                       'features': [ 'deprecated' ] },
+            '*decompress-threads': { 'type': 'uint8',
+                                     'features': [ 'deprecated' ] },
             '*throttle-trigger-threshold': 'uint8',
             '*cpu-throttle-initial': 'uint8',
             '*cpu-throttle-increment': 'uint8',
@@ -1296,10 +1321,8 @@
 #
 # -> { "execute": "query-migrate-parameters" }
 # <- { "return": {
-#          "decompress-threads": 2,
+#          "multifd-channels": 2,
 #          "cpu-throttle-increment": 10,
-#          "compress-threads": 8,
-#          "compress-level": 1,
 #          "cpu-throttle-initial": 20,
 #          "max-bandwidth": 33554432,
 #          "downtime-limit": 300
diff --git a/migration/options.c b/migration/options.c
index 0d0a3f8edb..7cb99a82a5 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -483,6 +483,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
                     "Use blockdev-mirror with NBD instead.");
     }
 
+    if (new_caps[MIGRATION_CAPABILITY_COMPRESS]) {
+        warn_report("Old compression method is deprecated. "
+                    "Use multifd compression methods instead.");
+    }
+
 #ifndef CONFIG_REPLICATION
     if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
         error_setg(errp, "QEMU compiled without replication module"
@@ -1321,18 +1326,26 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
     /* TODO use QAPI_CLONE() instead of duplicating it inline */
 
     if (params->has_compress_level) {
+        warn_report("Old compression is deprecated. "
+                    "Use multifd compression methods instead.");
         s->parameters.compress_level = params->compress_level;
     }
 
     if (params->has_compress_threads) {
+        warn_report("Old compression is deprecated. "
+                    "Use multifd compression methods instead.");
         s->parameters.compress_threads = params->compress_threads;
     }
 
     if (params->has_compress_wait_thread) {
+        warn_report("Old compression is deprecated. "
+                    "Use multifd compression methods instead.");
         s->parameters.compress_wait_thread = params->compress_wait_thread;
     }
 
     if (params->has_decompress_threads) {
+        warn_report("Old compression is deprecated. "
+                    "Use multifd compression methods instead.");
         s->parameters.decompress_threads = params->decompress_threads;
     }
 
-- 
2.41.0

[PATCH v5 6/7] [RFC] migration: Make -i/-b an error for hmp and qmp

[Juan Quintela]

[DON'T MERGE]

Block migration is obsolete, users should use blockdev-mirror
instead.
Make it one error to set them.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-hmp-cmds.c | 13 +++++++------
 migration/migration.c          | 33 ++++++---------------------------
 2 files changed, 13 insertions(+), 33 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index bfeb1a476a..0acc866c79 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -746,16 +746,17 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     if (inc) {
-        warn_report("option '-i' is deprecated.  Use 'blockdev-mirror + NBD'"
-                    " instead.");
+        monitor_printf(mon, "option '-i' is deprecated.  Use"
+                       " 'blockdev-mirror + NBD' instead.");
+        return;
     }
 
     if (blk) {
-        warn_report("option '-b' is deprecated.  Use 'blockdev-mirror + NBD'"
-                    " instead.");
+        monitor_printf(mon, "option '-b' is deprecated.  Use"
+                       " 'blockdev-mirror + NBD' instead.");
+        return;
     }
-
-    qmp_migrate(uri, !!blk, blk, !!inc, inc,
+    qmp_migrate(uri, false, false, false, false,
                 false, false, true, resume, &err);
     if (hmp_handle_error(mon, err)) {
         return;
diff --git a/migration/migration.c b/migration/migration.c
index 4da7fcfe0f..9a695299ba 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1601,16 +1601,16 @@ bool migration_is_blocked(Error **errp)
 static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
                             bool resume, Error **errp)
 {
-    Error *local_err = NULL;
-
     if (blk_inc) {
-        warn_report("parameter 'inc' is deprecated.  Use blockdev-mirror with"
-                    " NBD instead");
+        error_setg(errp, "parameter 'inc' is deprecated.  Use blockdev-mirror"
+                   " with NBD instead");
+        return false;
     }
 
     if (blk) {
-        warn_report("capability 'blk is deprecated.  Use blockdev-mirror with"
-                    " NBD instead");
+        error_setg(errp, "capability 'blk is deprecated.  Use blockdev-mirror"
+                   " with NBD instead");
+        return false;
     }
 
     if (resume) {
@@ -1662,27 +1662,6 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
         return false;
     }
 
-    if (blk || blk_inc) {
-        if (migrate_colo()) {
-            error_setg(errp, "No disk migration is required in COLO mode");
-            return false;
-        }
-        if (migrate_block() || migrate_block_incremental()) {
-            error_setg(errp, "Command options are incompatible with "
-                       "current migration capabilities");
-            return false;
-        }
-        if (!migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, true, &local_err)) {
-            error_propagate(errp, local_err);
-            return false;
-        }
-        s->must_remove_block_options = true;
-    }
-
-    if (blk_inc) {
-        migrate_set_block_incremental(true);
-    }
-
     if (migrate_init(s, errp)) {
         return false;
     }
-- 
2.41.0

[PATCH v5 7/7] [RFC] migration: Remove helpers needed for -i/-b migrate options

[Juan Quintela]

[DON'T MERGE]

We were abusing capabilities and parameters to implement -i/-b.
Previous patch convert that options into one error.  Remove all the
helpers needed to implement them.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.h |  4 ----
 migration/options.h   |  6 ------
 migration/migration.c |  2 --
 migration/options.c   | 41 -----------------------------------------
 4 files changed, 53 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index ae82004892..937a3534c5 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -382,10 +382,6 @@ struct MigrationState {
     /* mutex to protect errp */
     QemuMutex error_mutex;
 
-    /* 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;
-
     /*
      * Global switch on whether we need to store the global state
      * during migration.
diff --git a/migration/options.h b/migration/options.h
index 237f2d6b4a..e41ea38322 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -62,7 +62,6 @@ bool migrate_tls(void);
 /* capabilities helpers */
 
 bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp);
-bool migrate_cap_set(int cap, bool value, Error **errp);
 
 /* parameters */
 
@@ -93,14 +92,9 @@ const char *migrate_tls_creds(void);
 const char *migrate_tls_hostname(void);
 uint64_t migrate_xbzrle_cache_size(void);
 
-/* parameters setters */
-
-void migrate_set_block_incremental(bool value);
-
 /* parameters helpers */
 
 bool migrate_params_check(MigrationParameters *params, Error **errp);
 void migrate_params_init(MigrationParameters *params);
-void block_cleanup_parameters(void);
 
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 9a695299ba..9792bd98ca 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1205,7 +1205,6 @@ static void migrate_fd_cleanup(MigrationState *s)
         error_report_err(error_copy(s->error));
     }
     notifier_list_notify(&migration_state_notifiers, s);
-    block_cleanup_parameters();
     yank_unregister_instance(MIGRATION_YANK_INSTANCE);
 }
 
@@ -1715,7 +1714,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
                    "a valid migration protocol");
         migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                           MIGRATION_STATUS_FAILED);
-        block_cleanup_parameters();
     }
 
     if (local_err) {
diff --git a/migration/options.c b/migration/options.c
index 7cb99a82a5..3be7e35f40 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -631,26 +631,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
     return true;
 }
 
-bool migrate_cap_set(int cap, bool value, Error **errp)
-{
-    MigrationState *s = migrate_get_current();
-    bool new_caps[MIGRATION_CAPABILITY__MAX];
-
-    if (migration_is_running(s->state)) {
-        error_setg(errp, QERR_MIGRATION_ACTIVE);
-        return false;
-    }
-
-    memcpy(new_caps, s->capabilities, sizeof(new_caps));
-    new_caps[cap] = value;
-
-    if (!migrate_caps_check(s->capabilities, new_caps, errp)) {
-        return false;
-    }
-    s->capabilities[cap] = value;
-    return true;
-}
-
 MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
 {
     MigrationCapabilityStatusList *head = NULL, **tail = &head;
@@ -877,29 +857,8 @@ uint64_t migrate_xbzrle_cache_size(void)
     return s->parameters.xbzrle_cache_size;
 }
 
-/* parameter setters */
-
-void migrate_set_block_incremental(bool value)
-{
-    MigrationState *s = migrate_get_current();
-
-    s->parameters.block_incremental = value;
-}
-
 /* parameters helpers */
 
-void block_cleanup_parameters(void)
-{
-    MigrationState *s = migrate_get_current();
-
-    if (s->must_remove_block_options) {
-        /* setting to false can never fail */
-        migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, false, &error_abort);
-        migrate_set_block_incremental(false);
-        s->must_remove_block_options = false;
-    }
-}
-
 AnnounceParameters *migrate_announce_params(void)
 {
     static AnnounceParameters ap;
-- 
2.41.0

Re: [PATCH v5 2/7] migration: migrate 'inc' command option is deprecated.

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Use blockdev-mirror with NBD instead.
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> ---
>
> Improve documentation and style (thanks Markus)
> ---
>  docs/about/deprecated.rst      | 8 ++++++++
>  qapi/migration.json            | 8 +++++++-
>  migration/migration-hmp-cmds.c | 5 +++++
>  migration/migration.c          | 5 +++++
>  4 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 2febd2d12f..fc6adf1dea 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -461,3 +461,11 @@ Migration
>  ``skipped`` field in Migration stats has been deprecated.  It hasn't
>  been used for more than 10 years.
>  
> +``inc`` migrate command option (since 8.2)
> +''''''''''''''''''''''''''''''''''''''''''
> +
> +Use blockdev-mirror with NBD instead.
> +
> +As an intermediate step the ``inc`` functionality can be achieved by
> +setting the ``block-incremental`` migration parameter to ``true``.
> +But this parameter is also deprecated.
> diff --git a/qapi/migration.json b/qapi/migration.json
> index db3df12d6c..fa7f4f2575 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1524,6 +1524,11 @@
>  #
>  # @resume: resume one paused migration, default "off". (since 3.0)
>  #
> +# Features:
> +#
> +# @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
> +#     NBD instead.
> +#
>  # Returns: nothing on success
>  #
>  # Since: 0.14
> @@ -1545,7 +1550,8 @@
>  # <- { "return": {} }
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> +  'data': {'uri': 'str', '*blk': 'bool',
> +           '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
>             '*detach': 'bool', '*resume': 'bool' } }
>  
>  ##
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index a82597f18e..fee7079afa 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -745,6 +745,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>      const char *uri = qdict_get_str(qdict, "uri");
>      Error *err = NULL;
>  
> +    if (inc) {
> +        warn_report("option '-i' is deprecated.  Use 'blockdev-mirror + NBD'"
> +                    " instead.");

Convention: an error or warning message is a single phrase, with no
newline or trailing punctuation.  The simplest way to conform to it is
something like

           warn_report("option '-i' is deprecated;"
                       " use blockdev-mirror with NBD instead.");

> +    }
> +
>      qmp_migrate(uri, !!blk, blk, !!inc, inc,
>                  false, false, true, resume, &err);
>      if (hmp_handle_error(mon, err)) {
> diff --git a/migration/migration.c b/migration/migration.c
> index 6ba5e145ac..b8b3ba58df 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1603,6 +1603,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>  {
>      Error *local_err = NULL;
>  
> +    if (blk_inc) {
> +        warn_report("parameter 'inc' is deprecated.  Use blockdev-mirror with"
> +                    " NBD instead");

Likewise.

> +    }
> +
>      if (resume) {
>          if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>              error_setg(errp, "Cannot resume if there is no "

Other than that
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v5 3/7] migration: migrate 'blk' command option is deprecated.

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Use blocked-mirror with NBD instead.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
> ---
>
> Improve documentation and style (markus)
> ---
>  docs/about/deprecated.rst      | 10 ++++++++++
>  qapi/migration.json            |  6 ++++--
>  migration/migration-hmp-cmds.c |  5 +++++
>  migration/migration.c          |  5 +++++
>  4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index fc6adf1dea..0149f040b6 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -469,3 +469,13 @@ Use blockdev-mirror with NBD instead.
>  As an intermediate step the ``inc`` functionality can be achieved by
>  setting the ``block-incremental`` migration parameter to ``true``.
>  But this parameter is also deprecated.
> +
> +``blk`` migrate command option (since 8.2)
> +''''''''''''''''''''''''''''''''''''''''''
> +
> +Use blockdev-mirror with NBD instead.
> +
> +As an intermediate step the ``blk`` functionality can be achieved by
> +setting the ``block`` migration capability to ``true``.
> +But this capability is also deprecated.
> +
> diff --git a/qapi/migration.json b/qapi/migration.json
> index fa7f4f2575..59a07b50f0 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1527,7 +1527,8 @@
>  # Features:
>  #
>  # @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
> -#     NBD instead.
> +#     NBD instead.  Member @blk is deprecated.  Use blockdev-mirror
> +#     with NBD instead.

Better:

   # @deprecated: Members @inc and @blk are deprecated.  Use
   #     blockdev-mirror with NBD instead.

>  #
>  # Returns: nothing on success
>  #
> @@ -1550,7 +1551,8 @@
>  # <- { "return": {} }
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool',
> +  'data': {'uri': 'str',
> +           '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
>             '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
>             '*detach': 'bool', '*resume': 'bool' } }
>  
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index fee7079afa..bfeb1a476a 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -750,6 +750,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>                      " instead.");
>      }
>  
> +    if (blk) {
> +        warn_report("option '-b' is deprecated.  Use 'blockdev-mirror + NBD'"
> +                    " instead.");

           warn_report("option '-b' is deprecated;"
                       " use blockdev-mirror with NBD instead.");

> +    }
> +
>      qmp_migrate(uri, !!blk, blk, !!inc, inc,
>                  false, false, true, resume, &err);
>      if (hmp_handle_error(mon, err)) {
> diff --git a/migration/migration.c b/migration/migration.c
> index b8b3ba58df..4da7fcfe0f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1608,6 +1608,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>                      " NBD instead");
>      }
>  
> +    if (blk) {
> +        warn_report("capability 'blk is deprecated.  Use blockdev-mirror with"
> +                    " NBD instead");
> +    }

Capability?  Isn't this a parameter?

"'blk" lacks a closing single quote.

I figure we want

           warn_report("parameter 'blk' is deprecated;"
                       " use blockdev-mirror with NBD instead.");

> +
>      if (resume) {
>          if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>              error_setg(errp, "Cannot resume if there is no "

Other than that
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v5 4/7] migration: Deprecate block migration

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> It is obsolete.  It is better to use driver-mirror with NBD instead.
>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Hanna Czenczek <hreitz@redhat.com>
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/about/deprecated.rst | 10 ++++++++++
>  qapi/migration.json       | 29 ++++++++++++++++++++++++-----
>  migration/block.c         |  3 +++
>  migration/options.c       |  9 ++++++++-
>  4 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 0149f040b6..5eaf096040 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -479,3 +479,13 @@ As an intermediate step the ``blk`` functionality can be achieved by
>  setting the ``block`` migration capability to ``true``.
>  But this capability is also deprecated.
>  
> +block migration (since 8.2)
> +'''''''''''''''''''''''''''
> +
> +Block migration is too inflexible.  It needs to migrate all block
> +devices or none.
> +
> +Please see "QMP invocation for live storage migration with
> +``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst
> +for a detailed explanation.
> +
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 59a07b50f0..c7633b22c0 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -269,11 +269,15 @@
>  #     average memory load of the virtual CPU indirectly.  Note that
>  #     zero means guest doesn't dirty memory.  (Since 8.1)
>  #
> +# Features:
> +#
> +# @deprecated: Member @disk is deprecated because block migration is.
> +#
>  # Since: 0.14
>  ##
>  { 'struct': 'MigrationInfo',
>    'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
> -           '*disk': 'MigrationStats',
> +           '*disk': { 'type': 'MigrationStats', 'features': [ 'deprecated' ] },
>             '*vfio': 'VfioStats',
>             '*xbzrle-cache': 'XBZRLECacheStats',
>             '*total-time': 'int',
> @@ -525,6 +529,9 @@
>  #
>  # Features:
>  #
> +# @deprecated: Member @block is deprecated.  Use blockdev-mirror with
> +#     NBD instead.
> +#
>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>  #
>  # Since: 1.2
> @@ -534,7 +541,8 @@
>             'compress', 'events', 'postcopy-ram',
>             { 'name': 'x-colo', 'features': [ 'unstable' ] },
>             'release-ram',
> -           'block', 'return-path', 'pause-before-switchover', 'multifd',
> +           { 'name': 'block', 'features': [ 'deprecated' ] },
> +           'return-path', 'pause-before-switchover', 'multifd',
>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>             { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
>             'validate-uuid', 'background-snapshot',
> @@ -835,6 +843,9 @@
>  #
>  # Features:
>  #
> +# @deprecated: Member @block-incremental is deprecated. Use

Two spaces between sentences for consistency, please.

> +#     blockdev-mirror with NBD instead.
> +#
>  # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
>  #     are experimental.
>  #
> @@ -850,7 +861,7 @@
>             'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
>             'avail-switchover-bandwidth', 'downtime-limit',
>             { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
> -           'block-incremental',
> +           { 'name': 'block-incremental', 'features': [ 'deprecated' ] },
>             'multifd-channels',
>             'xbzrle-cache-size', 'max-postcopy-bandwidth',
>             'max-cpu-throttle', 'multifd-compression',
> @@ -1011,6 +1022,9 @@
>  #
>  # Features:
>  #
> +# @deprecated: Member @block-incremental is deprecated. Use

Likewise.

> +#     blockdev-mirror with NBD instead.
> +#
>  # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
>  #     are experimental.
>  #
> @@ -1040,7 +1054,8 @@
>              '*downtime-limit': 'uint64',
>              '*x-checkpoint-delay': { 'type': 'uint32',
>                                       'features': [ 'unstable' ] },
> -            '*block-incremental': 'bool',
> +            '*block-incremental': { 'type': 'bool',
> +                                    'features': [ 'deprecated' ] },
>              '*multifd-channels': 'uint8',
>              '*xbzrle-cache-size': 'size',
>              '*max-postcopy-bandwidth': 'size',
> @@ -1225,6 +1240,9 @@
>  #
>  # Features:
>  #
> +# @deprecated: Member @block-incremental is deprecated. Use

Likewise.

> +#     blockdev-mirror with NBD instead.
> +#
>  # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
>  #     are experimental.
>  #
> @@ -1251,7 +1269,8 @@
>              '*downtime-limit': 'uint64',
>              '*x-checkpoint-delay': { 'type': 'uint32',
>                                       'features': [ 'unstable' ] },
> -            '*block-incremental': 'bool',
> +            '*block-incremental': { 'type': 'bool',
> +                                    'features': [ 'deprecated' ] },
>              '*multifd-channels': 'uint8',
>              '*xbzrle-cache-size': 'size',
>              '*max-postcopy-bandwidth': 'size',
> diff --git a/migration/block.c b/migration/block.c
> index b60698d6e2..7682f4fbd2 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -731,6 +731,9 @@ static int block_save_setup(QEMUFile *f, void *opaque)
>      trace_migration_block_save("setup", block_mig_state.submitted,
>                                 block_mig_state.transferred);
>  
> +    warn_report("block migration is deprecated.  Use blockdev-mirror with"
> +                "NBD instead.");

       warn_report("block migration is deprecated;"
                   " use blockdev-mirror with NBD instead.");


> +
>      ret = init_blk_migration(f);
>      if (ret < 0) {
>          return ret;
> diff --git a/migration/options.c b/migration/options.c
> index 42fb818956..0d0a3f8edb 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "exec/target_page.h"
>  #include "qapi/clone-visitor.h"
>  #include "qapi/error.h"
> @@ -473,10 +474,14 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>      if (new_caps[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");
> +        error_append_hint(errp, "Use blockdev-mirror with NBD instead.\n");
>          return false;
>      }
>  #endif
> +    if (new_caps[MIGRATION_CAPABILITY_BLOCK]) {
> +        warn_report("Block migration is deprecated. "
> +                    "Use blockdev-mirror with NBD instead.");

Likewise.

> +    }
>  
>  #ifndef CONFIG_REPLICATION
>      if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
> @@ -1386,6 +1391,8 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>      }
>  
>      if (params->has_block_incremental) {
> +        warn_report("Block migration is deprecated. "
> +                    "Use blockdev-mirror with NBD instead.");

Likewise.

>          s->parameters.block_incremental = params->block_incremental;
>      }
>      if (params->has_multifd_channels) {

Other than that
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v5 5/7] migration: Deprecate old compression method

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> Acked-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/about/deprecated.rst |  8 ++++
>  qapi/migration.json       | 79 +++++++++++++++++++++++++--------------
>  migration/options.c       | 13 +++++++
>  3 files changed, 72 insertions(+), 28 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 5eaf096040..f46baf9ee9 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -489,3 +489,11 @@ Please see "QMP invocation for live storage migration with
>  ``blockdev-mirror`` + NBD" in docs/interop/live-block-operations.rst
>  for a detailed explanation.
>  
> +old compression method (since 8.2)
> +''''''''''''''''''''''''''''''''''
> +
> +Compression method fails too much.  Too many races.  We are going to
> +remove it if nobody fixes it.  For starters, migration-test
> +compression tests are disabled becase they fail randomly.  If you need
> +compression, use multifd compression methods.
> +
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c7633b22c0..834506a02b 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -272,6 +272,10 @@
>  # Features:
>  #
>  # @deprecated: Member @disk is deprecated because block migration is.
> +#     Member @compression is deprecated because it is unreliable and
> +#     untested. It is recommended to use multifd migration, which
> +#     offers an alternative compression implementation that is
> +#     reliable and tested.

Two spaces between sentences for consistency, please.

>  #
>  # Since: 0.14
>  ##
> @@ -289,7 +293,7 @@
>             '*blocked-reasons': ['str'],
>             '*postcopy-blocktime': 'uint32',
>             '*postcopy-vcpu-blocktime': ['uint32'],
> -           '*compression': 'CompressionStats',
> +           '*compression': { 'type': 'CompressionStats', 'features': [ 'deprecated' ] },
>             '*socket-address': ['SocketAddress'],
>             '*dirty-limit-throttle-time-per-round': 'uint64',
>             '*dirty-limit-ring-full-time': 'uint64'} }
> @@ -530,7 +534,10 @@
>  # Features:
>  #
>  # @deprecated: Member @block is deprecated.  Use blockdev-mirror with
> -#     NBD instead.
> +#     NBD instead.  Member @compression is deprecated because it is
> +#     unreliable and untested. It is recommended to use multifd
> +#     migration, which offers an alternative compression
> +#     implementation that is reliable and tested.

Likewise.

>  #
>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>  #
> @@ -538,7 +545,8 @@
>  ##
>  { 'enum': 'MigrationCapability',
>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> -           'compress', 'events', 'postcopy-ram',
> +           { 'name': 'compress', 'features': [ 'deprecated' ] },
> +           'events', 'postcopy-ram',
>             { 'name': 'x-colo', 'features': [ 'unstable' ] },
>             'release-ram',
>             { 'name': 'block', 'features': [ 'deprecated' ] },
> @@ -844,7 +852,9 @@
>  # Features:
>  #
>  # @deprecated: Member @block-incremental is deprecated. Use
> -#     blockdev-mirror with NBD instead.
> +#     blockdev-mirror with NBD instead. Members @compress-level,
> +#     @compress-threads, @decompress-threads and @compress-wait-thread
> +#     are deprecated because @compression is deprecated.

Likewise.

>  #
>  # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
>  #     are experimental.
> @@ -854,8 +864,11 @@
>  { 'enum': 'MigrationParameter',
>    'data': ['announce-initial', 'announce-max',
>             'announce-rounds', 'announce-step',
> -           'compress-level', 'compress-threads', 'decompress-threads',
> -           'compress-wait-thread', 'throttle-trigger-threshold',
> +           { 'name': 'compress-level', 'features': [ 'deprecated' ] },
> +           { 'name': 'compress-threads', 'features': [ 'deprecated' ] },
> +           { 'name': 'decompress-threads', 'features': [ 'deprecated' ] },
> +           { 'name': 'compress-wait-thread', 'features': [ 'deprecated' ] },
> +           'throttle-trigger-threshold',
>             'cpu-throttle-initial', 'cpu-throttle-increment',
>             'cpu-throttle-tailslow',
>             'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
> @@ -885,16 +898,16 @@
>  # @announce-step: Increase in delay (in milliseconds) between
>  #     subsequent packets in the announcement (Since 4.0)
>  #
> -# @compress-level: compression level
> +# @compress-level: compression level.
>  #
> -# @compress-threads: compression thread count
> +# @compress-threads: compression thread count.
>  #
>  # @compress-wait-thread: Controls behavior when all compression
>  #     threads are currently busy.  If true (default), wait for a free
>  #     compression thread to become available; otherwise, send the page
> -#     uncompressed.  (Since 3.1)
> +#     uncompressed. (Since 3.1)
>  #
> -# @decompress-threads: decompression thread count
> +# @decompress-threads: decompression thread count.
>  #
>  # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>  #     bytes_xfer_period to trigger throttling.  It is expressed as

Unrelated.

> @@ -1023,7 +1036,9 @@
>  # Features:
>  #
>  # @deprecated: Member @block-incremental is deprecated. Use
> -#     blockdev-mirror with NBD instead.
> +#     blockdev-mirror with NBD instead. Members @compress-level,
> +#     @compress-threads, @decompress-threads and @compress-wait-thread
> +#     are deprecated because @compression is deprecated.

Two spaces between sentences for consistency, please.

>  #
>  # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
>  #     are experimental.
> @@ -1038,10 +1053,14 @@
>              '*announce-max': 'size',
>              '*announce-rounds': 'size',
>              '*announce-step': 'size',
> -            '*compress-level': 'uint8',
> -            '*compress-threads': 'uint8',
> -            '*compress-wait-thread': 'bool',
> -            '*decompress-threads': 'uint8',
> +            '*compress-level': { 'type': 'uint8',
> +                                 'features': [ 'deprecated' ] },
> +            '*compress-threads':  { 'type': 'uint8',
> +                                    'features': [ 'deprecated' ] },
> +            '*compress-wait-thread':  { 'type': 'bool',
> +                                        'features': [ 'deprecated' ] },
> +            '*decompress-threads':  { 'type': 'uint8',
> +                                      'features': [ 'deprecated' ] },
>              '*throttle-trigger-threshold': 'uint8',
>              '*cpu-throttle-initial': 'uint8',
>              '*cpu-throttle-increment': 'uint8',
> @@ -1078,7 +1097,7 @@
>  # Example:
>  #
>  # -> { "execute": "migrate-set-parameters" ,
> -#      "arguments": { "compress-level": 1 } }
> +#      "arguments": { "multifd-channels": 5 } }
>  # <- { "return": {} }
>  ##

Thanks for taking care of updating the example!

>  { 'command': 'migrate-set-parameters', 'boxed': true,
> @@ -1101,16 +1120,16 @@
>  # @announce-step: Increase in delay (in milliseconds) between
>  #     subsequent packets in the announcement (Since 4.0)
>  #
> -# @compress-level: compression level
> +# @compress-level: compression level.
>  #
> -# @compress-threads: compression thread count
> +# @compress-threads: compression thread count.
>  #
>  # @compress-wait-thread: Controls behavior when all compression
>  #     threads are currently busy.  If true (default), wait for a free
>  #     compression thread to become available; otherwise, send the page
> -#     uncompressed.  (Since 3.1)
> +#     uncompressed. (Since 3.1)
>  #
> -# @decompress-threads: decompression thread count
> +# @decompress-threads: decompression thread count.
>  #
>  # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>  #     bytes_xfer_period to trigger throttling.  It is expressed as

Unrelated.

> @@ -1241,7 +1260,9 @@
>  # Features:
>  #
>  # @deprecated: Member @block-incremental is deprecated. Use
> -#     blockdev-mirror with NBD instead.
> +#     blockdev-mirror with NBD instead. Members @compress-level,
> +#     @compress-threads, @decompress-threads and @compress-wait-thread
> +#     are deprecated because @compression is deprecated.

Two spaces between sentences for consistency, please.

>  #
>  # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period
>  #     are experimental.
> @@ -1253,10 +1274,14 @@
>              '*announce-max': 'size',
>              '*announce-rounds': 'size',
>              '*announce-step': 'size',
> -            '*compress-level': 'uint8',
> -            '*compress-threads': 'uint8',
> -            '*compress-wait-thread': 'bool',
> -            '*decompress-threads': 'uint8',
> +            '*compress-level': { 'type': 'uint8',
> +                                 'features': [ 'deprecated' ] },
> +            '*compress-threads': { 'type': 'uint8',
> +                                   'features': [ 'deprecated' ] },
> +            '*compress-wait-thread': { 'type': 'bool',
> +                                       'features': [ 'deprecated' ] },
> +            '*decompress-threads': { 'type': 'uint8',
> +                                     'features': [ 'deprecated' ] },
>              '*throttle-trigger-threshold': 'uint8',
>              '*cpu-throttle-initial': 'uint8',
>              '*cpu-throttle-increment': 'uint8',
> @@ -1296,10 +1321,8 @@
>  #
>  # -> { "execute": "query-migrate-parameters" }
>  # <- { "return": {
> -#          "decompress-threads": 2,
> +#          "multifd-channels": 2,
>  #          "cpu-throttle-increment": 10,
> -#          "compress-threads": 8,
> -#          "compress-level": 1,
>  #          "cpu-throttle-initial": 20,
>  #          "max-bandwidth": 33554432,
>  #          "downtime-limit": 300

Thanks again!

> diff --git a/migration/options.c b/migration/options.c
> index 0d0a3f8edb..7cb99a82a5 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -483,6 +483,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>                      "Use blockdev-mirror with NBD instead.");
>      }
>  
> +    if (new_caps[MIGRATION_CAPABILITY_COMPRESS]) {
> +        warn_report("Old compression method is deprecated. "
> +                    "Use multifd compression methods instead.");
> +    }
> +
>  #ifndef CONFIG_REPLICATION
>      if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
>          error_setg(errp, "QEMU compiled without replication module"
> @@ -1321,18 +1326,26 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>      /* TODO use QAPI_CLONE() instead of duplicating it inline */
>  
>      if (params->has_compress_level) {
> +        warn_report("Old compression is deprecated. "
> +                    "Use multifd compression methods instead.");
>          s->parameters.compress_level = params->compress_level;
>      }
>  
>      if (params->has_compress_threads) {
> +        warn_report("Old compression is deprecated. "
> +                    "Use multifd compression methods instead.");
>          s->parameters.compress_threads = params->compress_threads;
>      }
>  
>      if (params->has_compress_wait_thread) {
> +        warn_report("Old compression is deprecated. "
> +                    "Use multifd compression methods instead.");
>          s->parameters.compress_wait_thread = params->compress_wait_thread;
>      }
>  
>      if (params->has_decompress_threads) {
> +        warn_report("Old compression is deprecated. "
> +                    "Use multifd compression methods instead.");
>          s->parameters.decompress_threads = params->decompress_threads;
>      }

Other than that
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v5 2/7] migration: migrate 'inc' command option is deprecated.

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Use blockdev-mirror with NBD instead.
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>
>> ---
>>
>> Improve documentation and style (thanks Markus)
>> ---
>>  docs/about/deprecated.rst      | 8 ++++++++
>>  qapi/migration.json            | 8 +++++++-
>>  migration/migration-hmp-cmds.c | 5 +++++
>>  migration/migration.c          | 5 +++++
>>  4 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index 2febd2d12f..fc6adf1dea 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -461,3 +461,11 @@ Migration
>>  ``skipped`` field in Migration stats has been deprecated.  It hasn't
>>  been used for more than 10 years.
>>  
>> +``inc`` migrate command option (since 8.2)
>> +''''''''''''''''''''''''''''''''''''''''''
>> +
>> +Use blockdev-mirror with NBD instead.
>> +
>> +As an intermediate step the ``inc`` functionality can be achieved by
>> +setting the ``block-incremental`` migration parameter to ``true``.
>> +But this parameter is also deprecated.
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index db3df12d6c..fa7f4f2575 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1524,6 +1524,11 @@
>>  #
>>  # @resume: resume one paused migration, default "off". (since 3.0)
>>  #
>> +# Features:
>> +#
>> +# @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
>> +#     NBD instead.
>> +#
>>  # Returns: nothing on success
>>  #
>>  # Since: 0.14
>> @@ -1545,7 +1550,8 @@
>>  # <- { "return": {} }
>>  ##
>>  { 'command': 'migrate',
>> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>> +  'data': {'uri': 'str', '*blk': 'bool',
>> +           '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
>>             '*detach': 'bool', '*resume': 'bool' } }
>>  
>>  ##
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index a82597f18e..fee7079afa 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -745,6 +745,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>>      const char *uri = qdict_get_str(qdict, "uri");
>>      Error *err = NULL;
>>  
>> +    if (inc) {
>> +        warn_report("option '-i' is deprecated.  Use 'blockdev-mirror + NBD'"
>> +                    " instead.");
>
> Convention: an error or warning message is a single phrase, with no
> newline or trailing punctuation.  The simplest way to conform to it is
> something like
>
>            warn_report("option '-i' is deprecated;"
>                        " use blockdev-mirror with NBD instead.");

then the trailing dot is not needed, right?

>> +    }
>> +
>>      qmp_migrate(uri, !!blk, blk, !!inc, inc,
>>                  false, false, true, resume, &err);
>>      if (hmp_handle_error(mon, err)) {
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 6ba5e145ac..b8b3ba58df 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1603,6 +1603,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>>  {
>>      Error *local_err = NULL;
>>  
>> +    if (blk_inc) {
>> +        warn_report("parameter 'inc' is deprecated.  Use blockdev-mirror with"
>> +                    " NBD instead");
>
> Likewise.
>
>> +    }
>> +
>>      if (resume) {
>>          if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>>              error_setg(errp, "Cannot resume if there is no "
>
> Other than that
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

OK, fixing it.

Re: [PATCH v5 3/7] migration: migrate 'blk' command option is deprecated.

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Use blocked-mirror with NBD instead.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>> ---
>>
>> Improve documentation and style (markus)
>> ---
>>  docs/about/deprecated.rst      | 10 ++++++++++
>>  qapi/migration.json            |  6 ++++--
>>  migration/migration-hmp-cmds.c |  5 +++++
>>  migration/migration.c          |  5 +++++
>>  4 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index fc6adf1dea..0149f040b6 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -469,3 +469,13 @@ Use blockdev-mirror with NBD instead.
>>  As an intermediate step the ``inc`` functionality can be achieved by
>>  setting the ``block-incremental`` migration parameter to ``true``.
>>  But this parameter is also deprecated.
>> +
>> +``blk`` migrate command option (since 8.2)
>> +''''''''''''''''''''''''''''''''''''''''''
>> +
>> +Use blockdev-mirror with NBD instead.
>> +
>> +As an intermediate step the ``blk`` functionality can be achieved by
>> +setting the ``block`` migration capability to ``true``.
>> +But this capability is also deprecated.
>> +
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index fa7f4f2575..59a07b50f0 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1527,7 +1527,8 @@
>>  # Features:
>>  #
>>  # @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
>> -#     NBD instead.
>> +#     NBD instead.  Member @blk is deprecated.  Use blockdev-mirror
>> +#     with NBD instead.
>
> Better:
>
>    # @deprecated: Members @inc and @blk are deprecated.  Use
>    #     blockdev-mirror with NBD instead.

Fixed.

>>  #
>>  # Returns: nothing on success
>>  #
>> @@ -1550,7 +1551,8 @@
>>  # <- { "return": {} }
>>  ##
>>  { 'command': 'migrate',
>> -  'data': {'uri': 'str', '*blk': 'bool',
>> +  'data': {'uri': 'str',
>> +           '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
>>             '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
>>             '*detach': 'bool', '*resume': 'bool' } }
>>  
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index fee7079afa..bfeb1a476a 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -750,6 +750,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>>                      " instead.");
>>      }
>>  
>> +    if (blk) {
>> +        warn_report("option '-b' is deprecated.  Use 'blockdev-mirror + NBD'"
>> +                    " instead.");
>
>            warn_report("option '-b' is deprecated;"
>                        " use blockdev-mirror with NBD instead.");

Fixed (removed the trailing dot)

>> +    }
>> +
>>      qmp_migrate(uri, !!blk, blk, !!inc, inc,
>>                  false, false, true, resume, &err);
>>      if (hmp_handle_error(mon, err)) {
>> diff --git a/migration/migration.c b/migration/migration.c
>> index b8b3ba58df..4da7fcfe0f 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1608,6 +1608,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>>                      " NBD instead");
>>      }
>>  
>> +    if (blk) {
>> +        warn_report("capability 'blk is deprecated.  Use blockdev-mirror with"
>> +                    " NBD instead");
>> +    }
>
> Capability?  Isn't this a parameter?
>
> "'blk" lacks a closing single quote.

You are right.  You need to set the 'blk capability for substitution,
but this is a parameter.  My fault.

> I figure we want
>
>            warn_report("parameter 'blk' is deprecated;"
>                        " use blockdev-mirror with NBD instead.");
>
>> +
>>      if (resume) {
>>          if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>>              error_setg(errp, "Cannot resume if there is no "
>
> Other than that
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks,

Re: [PATCH v5 4/7] migration: Deprecate block migration

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>>  #
>> +# @deprecated: Member @block-incremental is deprecated. Use
>
> Two spaces between sentences for consistency, please.

Done.  At least here I did the copy and paste right.

>> diff --git a/migration/block.c b/migration/block.c
>> index b60698d6e2..7682f4fbd2 100644
>> --- a/migration/block.c
>> +++ b/migration/block.c
>> @@ -731,6 +731,9 @@ static int block_save_setup(QEMUFile *f, void *opaque)
>>      trace_migration_block_save("setup", block_mig_state.submitted,
>>                                 block_mig_state.transferred);
>>  
>> +    warn_report("block migration is deprecated.  Use blockdev-mirror with"
>> +                "NBD instead.");
>
>        warn_report("block migration is deprecated;"
>                    " use blockdev-mirror with NBD instead.");

Done.

>> +    if (new_caps[MIGRATION_CAPABILITY_BLOCK]) {
>> +        warn_report("Block migration is deprecated. "
>> +                    "Use blockdev-mirror with NBD instead.");
>
> Likewise.
>
>> +    }
>>  
>>  #ifndef CONFIG_REPLICATION
>>      if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
>> @@ -1386,6 +1391,8 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>>      }
>>  
>>      if (params->has_block_incremental) {
>> +        warn_report("Block migration is deprecated. "
>> +                    "Use blockdev-mirror with NBD instead.");
>
> Likewise.
>
>>          s->parameters.block_incremental = params->block_incremental;
>>      }
>>      if (params->has_multifd_channels) {
>
> Other than that
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks.

Re: [PATCH v5 1/7] migration: Print block status when needed

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> The new line was only printed when command options were used.  When we
> used migration parameters and capabilities, it wasn't.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v5 5/7] migration: Deprecate old compression method

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Acked-by: Peter Xu <peterx@redhat.com>


>>  # @deprecated: Member @disk is deprecated because block migration is.
>> +#     Member @compression is deprecated because it is unreliable and
>> +#     untested. It is recommended to use multifd migration, which
>> +#     offers an alternative compression implementation that is
>> +#     reliable and tested.
>
> Two spaces between sentences for consistency, please.

I have reviewed all the patches again.  Let's hope that I didn't miss
one.

>>  # @announce-step: Increase in delay (in milliseconds) between
>>  #     subsequent packets in the announcement (Since 4.0)
>>  #
>> -# @compress-level: compression level
>> +# @compress-level: compression level.
>>  #
>> -# @compress-threads: compression thread count
>> +# @compress-threads: compression thread count.
>>  #
>>  # @compress-wait-thread: Controls behavior when all compression
>>  #     threads are currently busy.  If true (default), wait for a free
>>  #     compression thread to become available; otherwise, send the page
>> -#     uncompressed.  (Since 3.1)
>> +#     uncompressed. (Since 3.1)
>>  #
>> -# @decompress-threads: decompression thread count
>> +# @decompress-threads: decompression thread count.
>>  #
>>  # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>>  #     bytes_xfer_period to trigger throttling.  It is expressed as
>
> Unrelated.

Rebases are bad for you O:-)

>> @@ -1023,7 +1036,9 @@
>>  # Features:
>>  #
>>  # @deprecated: Member @block-incremental is deprecated. Use
>> -#     blockdev-mirror with NBD instead.
>> +#     blockdev-mirror with NBD instead. Members @compress-level,
>> +#     @compress-threads, @decompress-threads and @compress-wait-thread
>> +#     are deprecated because @compression is deprecated.
>
> Two spaces between sentences for consistency, please.

Done.
>> @@ -1078,7 +1097,7 @@
>>  # Example:
>>  #
>>  # -> { "execute": "migrate-set-parameters" ,
>> -#      "arguments": { "compress-level": 1 } }
>> +#      "arguments": { "multifd-channels": 5 } }
>>  # <- { "return": {} }
>>  ##
>
> Thanks for taking care of updating the example!

You are welcome.  grep for all occurences of compress-level and friends
has its advantages.

>>  # @compress-wait-thread: Controls behavior when all compression
>>  #     threads are currently busy.  If true (default), wait for a free
>>  #     compression thread to become available; otherwise, send the page
>> -#     uncompressed.  (Since 3.1)
>> +#     uncompressed. (Since 3.1)
>>  #
>> -# @decompress-threads: decompression thread count
>> +# @decompress-threads: decompression thread count.
>>  #
>>  # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>>  #     bytes_xfer_period to trigger throttling.  It is expressed as
>
> Unrelated.

I have removed the periods.

But I have a question, why the descriptions that are less than one line
don't have period and the other have it.
>>      if (params->has_compress_level) {
>> +        warn_report("Old compression is deprecated. "
>> +                    "Use multifd compression methods instead.");
>>          s->parameters.compress_level = params->compress_level;
>>      }
>>  
>>      if (params->has_compress_threads) {
>> +        warn_report("Old compression is deprecated. "
>> +                    "Use multifd compression methods instead.");
>>          s->parameters.compress_threads = params->compress_threads;
>>      }
>>  
>>      if (params->has_compress_wait_thread) {
>> +        warn_report("Old compression is deprecated. "
>> +                    "Use multifd compression methods instead.");
>>          s->parameters.compress_wait_thread = params->compress_wait_thread;
>>      }
>>  
>>      if (params->has_decompress_threads) {
>> +        warn_report("Old compression is deprecated. "

Once here, I did s/Old/old/

as all your examples of description start with lowercase.

>> +                    "Use multifd compression methods instead.");
>>          s->parameters.decompress_threads = params->decompress_threads;
>>      }
>
> Other than that
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks for your patience.

Re: [PATCH v5 2/7] migration: migrate 'inc' command option is deprecated.

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Juan Quintela <quintela@redhat.com> writes:
>>
>>> Use blockdev-mirror with NBD instead.
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>
>>> ---
>>>
>>> Improve documentation and style (thanks Markus)
>>> ---
>>>  docs/about/deprecated.rst      | 8 ++++++++
>>>  qapi/migration.json            | 8 +++++++-
>>>  migration/migration-hmp-cmds.c | 5 +++++
>>>  migration/migration.c          | 5 +++++
>>>  4 files changed, 25 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>>> index 2febd2d12f..fc6adf1dea 100644
>>> --- a/docs/about/deprecated.rst
>>> +++ b/docs/about/deprecated.rst
>>> @@ -461,3 +461,11 @@ Migration
>>>  ``skipped`` field in Migration stats has been deprecated.  It hasn't
>>>  been used for more than 10 years.
>>>  
>>> +``inc`` migrate command option (since 8.2)
>>> +''''''''''''''''''''''''''''''''''''''''''
>>> +
>>> +Use blockdev-mirror with NBD instead.
>>> +
>>> +As an intermediate step the ``inc`` functionality can be achieved by
>>> +setting the ``block-incremental`` migration parameter to ``true``.
>>> +But this parameter is also deprecated.
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index db3df12d6c..fa7f4f2575 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1524,6 +1524,11 @@
>>>  #
>>>  # @resume: resume one paused migration, default "off". (since 3.0)
>>>  #
>>> +# Features:
>>> +#
>>> +# @deprecated: Member @inc is deprecated.  Use blockdev-mirror with
>>> +#     NBD instead.
>>> +#
>>>  # Returns: nothing on success
>>>  #
>>>  # Since: 0.14
>>> @@ -1545,7 +1550,8 @@
>>>  # <- { "return": {} }
>>>  ##
>>>  { 'command': 'migrate',
>>> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>>> +  'data': {'uri': 'str', '*blk': 'bool',
>>> +           '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
>>>             '*detach': 'bool', '*resume': 'bool' } }
>>>  
>>>  ##
>>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>>> index a82597f18e..fee7079afa 100644
>>> --- a/migration/migration-hmp-cmds.c
>>> +++ b/migration/migration-hmp-cmds.c
>>> @@ -745,6 +745,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>>>      const char *uri = qdict_get_str(qdict, "uri");
>>>      Error *err = NULL;
>>>  
>>> +    if (inc) {
>>> +        warn_report("option '-i' is deprecated.  Use 'blockdev-mirror + NBD'"
>>> +                    " instead.");
>>
>> Convention: an error or warning message is a single phrase, with no
>> newline or trailing punctuation.  The simplest way to conform to it is
>> something like
>>
>>            warn_report("option '-i' is deprecated;"
>>                        " use blockdev-mirror with NBD instead.");
>
> then the trailing dot is not needed, right?

No!  Comical screwup on my part %-}

>>> +    }
>>> +
>>>      qmp_migrate(uri, !!blk, blk, !!inc, inc,
>>>                  false, false, true, resume, &err);
>>>      if (hmp_handle_error(mon, err)) {
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 6ba5e145ac..b8b3ba58df 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -1603,6 +1603,11 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>>>  {
>>>      Error *local_err = NULL;
>>>  
>>> +    if (blk_inc) {
>>> +        warn_report("parameter 'inc' is deprecated.  Use blockdev-mirror with"
>>> +                    " NBD instead");
>>
>> Likewise.
>>
>>> +    }
>>> +
>>>      if (resume) {
>>>          if (s->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>>>              error_setg(errp, "Cannot resume if there is no "
>>
>> Other than that
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> OK, fixing it.

Thanks!

Re: [PATCH v5 5/7] migration: Deprecate old compression method

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Juan Quintela <quintela@redhat.com> writes:
>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Acked-by: Peter Xu <peterx@redhat.com>
>
>
>>>  # @deprecated: Member @disk is deprecated because block migration is.
>>> +#     Member @compression is deprecated because it is unreliable and
>>> +#     untested. It is recommended to use multifd migration, which
>>> +#     offers an alternative compression implementation that is
>>> +#     reliable and tested.
>>
>> Two spaces between sentences for consistency, please.
>
> I have reviewed all the patches again.  Let's hope that I didn't miss
> one.
>
>>>  # @announce-step: Increase in delay (in milliseconds) between
>>>  #     subsequent packets in the announcement (Since 4.0)
>>>  #
>>> -# @compress-level: compression level
>>> +# @compress-level: compression level.
>>>  #
>>> -# @compress-threads: compression thread count
>>> +# @compress-threads: compression thread count.
>>>  #
>>>  # @compress-wait-thread: Controls behavior when all compression
>>>  #     threads are currently busy.  If true (default), wait for a free
>>>  #     compression thread to become available; otherwise, send the page
>>> -#     uncompressed.  (Since 3.1)
>>> +#     uncompressed. (Since 3.1)
>>>  #
>>> -# @decompress-threads: decompression thread count
>>> +# @decompress-threads: decompression thread count.
>>>  #
>>>  # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>>>  #     bytes_xfer_period to trigger throttling.  It is expressed as
>>
>> Unrelated.
>
> Rebases are bad for you O:-)
>
>>> @@ -1023,7 +1036,9 @@
>>>  # Features:
>>>  #
>>>  # @deprecated: Member @block-incremental is deprecated. Use
>>> -#     blockdev-mirror with NBD instead.
>>> +#     blockdev-mirror with NBD instead. Members @compress-level,
>>> +#     @compress-threads, @decompress-threads and @compress-wait-thread
>>> +#     are deprecated because @compression is deprecated.
>>
>> Two spaces between sentences for consistency, please.
>
> Done.
>>> @@ -1078,7 +1097,7 @@
>>>  # Example:
>>>  #
>>>  # -> { "execute": "migrate-set-parameters" ,
>>> -#      "arguments": { "compress-level": 1 } }
>>> +#      "arguments": { "multifd-channels": 5 } }
>>>  # <- { "return": {} }
>>>  ##
>>
>> Thanks for taking care of updating the example!
>
> You are welcome.  grep for all occurences of compress-level and friends
> has its advantages.
>
>>>  # @compress-wait-thread: Controls behavior when all compression
>>>  #     threads are currently busy.  If true (default), wait for a free
>>>  #     compression thread to become available; otherwise, send the page
>>> -#     uncompressed.  (Since 3.1)
>>> +#     uncompressed. (Since 3.1)
>>>  #
>>> -# @decompress-threads: decompression thread count
>>> +# @decompress-threads: decompression thread count.
>>>  #
>>>  # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>>>  #     bytes_xfer_period to trigger throttling.  It is expressed as
>>
>> Unrelated.
>
> I have removed the periods.
>
> But I have a question, why the descriptions that are less than one line
> don't have period and the other have it.

When the description consists of multiple sentences, we obviously need
to end them with punctuation.

Sometimes the description isn't a sentence, just a phrase,
e.g. "decompression thread count".  No punctuation then.

Sometimes it's a single sentence.  Then we roll dice.

>>>      if (params->has_compress_level) {
>>> +        warn_report("Old compression is deprecated. "
>>> +                    "Use multifd compression methods instead.");
>>>          s->parameters.compress_level = params->compress_level;
>>>      }
>>>  
>>>      if (params->has_compress_threads) {
>>> +        warn_report("Old compression is deprecated. "
>>> +                    "Use multifd compression methods instead.");
>>>          s->parameters.compress_threads = params->compress_threads;
>>>      }
>>>  
>>>      if (params->has_compress_wait_thread) {
>>> +        warn_report("Old compression is deprecated. "
>>> +                    "Use multifd compression methods instead.");
>>>          s->parameters.compress_wait_thread = params->compress_wait_thread;
>>>      }
>>>  
>>>      if (params->has_decompress_threads) {
>>> +        warn_report("Old compression is deprecated. "
>
> Once here, I did s/Old/old/
>
> as all your examples of description start with lowercase.

Yes, please.

>>> +                    "Use multifd compression methods instead.");
>>>          s->parameters.decompress_threads = params->decompress_threads;
>>>      }
>>
>> Other than that
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks for your patience.