Re: [PATCH v1 08/17] migration/colo: Use ram_block_discard_set_broken()

David Hildenbrand
2020-05-15T14:05:53Z

On 15.05.20 15:58, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> COLO will copy all memory in a RAM block, mark discarding of RAM broken.
>>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> include/migration/colo.h | 2 +-
>> migration/migration.c | 8 +++++++-
>> migration/savevm.c | 11 +++++++++--
>> 3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/migration/colo.h b/include/migration/colo.h
>> index 1636e6f907..768e1f04c3 100644
>> --- a/include/migration/colo.h
>> +++ b/include/migration/colo.h
>> @@ -25,7 +25,7 @@ void migrate_start_colo_process(MigrationState *s);
>> bool migration_in_colo_state(void);
>>
>> /* loadvm */
>> -void migration_incoming_enable_colo(void);
>> +int migration_incoming_enable_colo(void);
>> void migration_incoming_disable_colo(void);
>> bool migration_incoming_colo_enabled(void);
>> void *colo_process_incoming_thread(void *opaque);
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 177cce9e95..f6830e4620 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -338,12 +338,18 @@ bool migration_incoming_colo_enabled(void)
>>
>> void migration_incoming_disable_colo(void)
>> {
>> + ram_block_discard_set_broken(false);
>> migration_colo_enabled = false;
>> }
>>
>> -void migration_incoming_enable_colo(void)
>> +int migration_incoming_enable_colo(void)
>> {
>> + if (ram_block_discard_set_broken(true)) {
>> + error_report("COLO: cannot set discarding of RAM broken");
>
> I'd prefer 'COLO: cannot disable RAM discard'
>
> 'broken' suggests the user has to go and fix something or report a bug
> or something.

Sounds better, I'll adjust similar messages in the other patches. Thanks!

>
> Other than that:
>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> Dave

--
Thanks,

David / dhildenb