Re: [PATCH 1/6] migration: Add multi-thread compress method

[Zeyu Jin <jinzeyu@huawei.com>]

On 2020/11/27 17:48, Markus Armbruster wrote:
> Kevin, Max, suggest to skip right to Qcow2CompressionType.
> 
> Zeyu Jin <jinzeyu@huawei.com> writes:
> 
>> A multi-thread compress method parameter is added to hold the method we
>> are going to use. By default the 'zlib' method is used to maintain the
>> compatibility as before.
>>
>> Signed-off-by: Zeyu Jin <jinzeyu@huawei.com>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
> [...]
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 3c75820527..2ed6a55b92 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -525,6 +525,19 @@
>>    'data': [ 'none', 'zlib',
>>              { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>>  
>> +##
>> +# @CompressMethod:
>> +#
>> +# An enumeration of multi-thread compression methods.
>> +#
>> +# @zlib: use zlib compression method.
>> +#
>> +# Since: 6.0
>> +#
>> +##
>> +{ 'enum': 'CompressMethod',
>> +  'data': [ 'zlib' ] }
>> +
>>  ##
>>  # @BitmapMigrationBitmapAlias:
>>  #
>> @@ -599,6 +612,9 @@
>>  #                      compression, so set the decompress-threads to the number about 1/4
>>  #                      of compress-threads is adequate.
>>  #
>> +# @compress-method: Set compression method to use in multi-thread compression.
>> +#                   Defaults to zlib. (Since 6.0)
> 
> We already have @multifd-compression.  Why do we need to control the two
> separately?  Can you give a use case for different settings?
> 

Generally, mulit-thread compression deals with the situation
where network bandwith is limited but cpu resource is adequate. Multifd
instead aims to situation where single fd cannot take full advantage of
network bandwith. So compression based on multifd cannot fully cover the
cases for multi-thread compression.

For example, for migration with a bandwith limitation of 10M
bytes/second, single fd is enough for data delivery. This is the case
for multi-thread compression.

> If we do want two parameters: the names @compress-method and
> @multifd-compression are inconsistent.  According to your comment,
> @compress-method applies only to multi-thread compression.  That leads
> me to suggest renaming it to @multi-thread-compression.
>

For the names, my original idea is to make 'CompressMethod' consistent
with other multi-thread compression parameters, like 'compress-threads'
and 'compress-level'. There is truly some inconsistency here, between
multifd-compression params and old multi-thread compression params.

For now, I agree with you that 'multi-thread-compression' is better. It
is more specific and clear. I will rename the params in next version.
Thanks for the suggestion.

> After PATCH 4, CompressMethod is almost the same as MultiFDCompression:
> 
>    { 'enum': 'CompressMethod',
>      'data': [ 'zlib' ] }
>      'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
> 
>    { 'enum': 'MultiFDCompression',
>      'data': [ 'none', 'zlib',
>                { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
> 
> The difference is member 'none'.  Why does compression 'none' make sense
> for multi-fd, but not for multi-thread?
> 

When you set 'none'in multi-fd compression, you would not use the
compression capability in multi-fd.

In comparison, once you turn on multi-thread compression capability, you
have already admitted to use compression. In this case, member 'none' is
meaningless.

> If the difference is wanted: the names are inconsistent.  Less of an
> issue than member names, because type names are not externally visible.
> Let's make them consistent anyway.
> 
> Hmm, there's also Qcow2CompressionType.  That's another conversation;
> I'll start a new thread for it.
> 
> [...]
> 
> .
>