Re: [Qemu-devel] [RFC PATCH] migration/postcopy: skip compression when postcopy is active

[Wei Yang]

On Fri, Aug 23, 2019 at 05:27:01PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (address@hidden) wrote:
>> Now postcopy is not compatible with compression. And we disable setting
>> these two capability at the same time. While we can still leverage
>> compress before postcopy is active, for example at the bulk stage.
>> 
>> This patch skips compression when postcopy is active instead of
>> forbidding setting these capability at the same time.
>> 
>> Signed-off-by: Wei Yang <address@hidden>
>
>So I think this is OK, because ram_save_complete has a call to
>flush_compressed_data in it.
>
>However I think we should hold it until you figure out the other series
>that really enables compress; the problem is that as soon as you
>allow the capability, then there's nothing to distinguish this version
>and the version that fully enables it.  So how would you stop this
>version being used as the source to the version which fully enables it?
>
>So I think if we do the other series, then you should start off like
>this and then remove the capability check right at the end.
>

Agree, it is not proper to leave this middle state.

>Dave
>
>> ---
>>  migration/migration.c | 11 -----------
>>  migration/ram.c       | 10 ++++++++++
>>  2 files changed, 10 insertions(+), 11 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 5a496addbd..33c373033d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -995,17 +995,6 @@ static bool migrate_caps_check(bool *cap_list,
>>  #endif
>>  
>>      if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
>> -        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
>> -            /* The decompression threads asynchronously write into RAM
>> -             * rather than use the atomic copies needed to avoid
>> -             * userfaulting.  It should be possible to fix the decompression
>> -             * threads for compatibility in future.
>> -             */
>> -            error_setg(errp, "Postcopy is not currently compatible "
>> -                       "with compression");
>> -            return false;
>> -        }
>> -
>>          /* This check is reasonably expensive, so only when it's being
>>           * set the first time, also it's only the destination that needs
>>           * special support.
>> diff --git a/migration/ram.c b/migration/ram.c
>> index da12774a24..a0d3bc60b2 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2384,6 +2384,16 @@ static bool save_page_use_compression(RAMState *rs)
>>          return false;
>>      }
>>  
>> +    /*
>> +     * The decompression threads asynchronously write into RAM
>> +     * rather than use the atomic copies needed to avoid
>> +     * userfaulting.  It should be possible to fix the decompression
>> +     * threads for compatibility in future.
>> +     */
>> +    if (migration_in_postcopy()) {
>> +        return false;
>> +    }
>> +
>>      /*
>>       * If xbzrle is on, stop using the data compression after first
>>       * round of migration even if compression is enabled. In theory,
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / address@hidden / Manchester, UK

-- 
Wei Yang
Help you, Help me