Re: [PATCH v5 5/7] migration: Deprecate old compression method
Juan Quintela writes:
> Markus Armbruster wrote:
>> Juan Quintela writes:
>>
>>> Signed-off-by: Juan Quintela
>>> Acked-by: Stefan Hajnoczi
>>> Acked-by: Peter Xu
>
>
>>> # @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
>
> Thanks for your patience.
Re: [PATCH v5 5/7] migration: Deprecate old compression method
Markus Armbruster wrote:
> Juan Quintela writes:
>
>> Signed-off-by: Juan Quintela
>> Acked-by: Stefan Hajnoczi
>> Acked-by: Peter Xu
>> # @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
Thanks for your patience.
Re: [PATCH v5 5/7] migration: Deprecate old compression method
Juan Quintela writes:
> Signed-off-by: Juan Quintela
> Acked-by: Stefan Hajnoczi
> Acked-by: Peter Xu
> ---
> 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:
[PATCH v5 5/7] migration: Deprecate old compression method
Signed-off-by: Juan Quintela
Acked-by: Stefan Hajnoczi
Acked-by: Peter Xu
---
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
-#
