[PATCH for-6.0 0/3] migration: Fixes to the 'background-snapshot' code

[PATCH for-6.0 0/3] migration: Fixes to the 'background-snapshot' code

[Andrey Gruzdev]

This patch series contains:
 * Fix to the issue with occasionally truncated non-iterable device state
 * Solution to compatibility issues with virtio-balloon device
 * Fix to the issue when discarded or never populated pages miss UFFD
   write protection and get into migration stream in dirty state

Andrey Gruzdev (3):
  migration: Fix missing qemu_fflush() on buffer file in
    bg_migration_thread
  migration: Inhibit virtio-balloon for the duration of background
    snapshot
  migration: Pre-fault memory before starting background snasphot

 hw/virtio/virtio-balloon.c |  8 +++++--
 include/migration/misc.h   |  2 ++
 migration/migration.c      | 22 ++++++++++++++++-
 migration/ram.c            | 48 ++++++++++++++++++++++++++++++++++++++
 migration/ram.h            |  1 +
 5 files changed, 78 insertions(+), 3 deletions(-)

-- 
2.27.0

[PATCH for-6.0 1/3] migration: Fix missing qemu_fflush() on buffer file in bg_migration_thread

[Andrey Gruzdev]

Added missing qemu_fflush() on buffer file holding precopy device state.
Increased initial QIOChannelBuffer allocation to 512KB to avoid reallocs.
Typical configurations often require >200KB for device state and VMDESC.

Signed-off-by: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
---
 migration/migration.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index ca8b97baa5..00e13f9d58 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3812,7 +3812,7 @@ static void *bg_migration_thread(void *opaque)
      * with vCPUs running and, finally, write stashed non-RAM part of
      * the vmstate from the buffer to the migration stream.
      */
-    s->bioc = qio_channel_buffer_new(128 * 1024);
+    s->bioc = qio_channel_buffer_new(512 * 1024);
     qio_channel_set_name(QIO_CHANNEL(s->bioc), "vmstate-buffer");
     fb = qemu_fopen_channel_output(QIO_CHANNEL(s->bioc));
     object_unref(OBJECT(s->bioc));
@@ -3866,6 +3866,12 @@ static void *bg_migration_thread(void *opaque)
     if (qemu_savevm_state_complete_precopy_non_iterable(fb, false, false)) {
         goto fail;
     }
+    /*
+     * Since we are going to get non-iterable state data directly
+     * from s->bioc->data, explicit flush is needed here.
+     */
+    qemu_fflush(fb);
+
     /* Now initialize UFFD context and start tracking RAM writes */
     if (ram_write_tracking_start()) {
         goto fail;
-- 
2.27.0

[PATCH for-6.0 2/3] migration: Inhibit virtio-balloon for the duration of background snapshot

[Andrey Gruzdev]

The same thing as for incoming postcopy - we cannot deal with concurrent
RAM discards when using background snapshot feature in outgoing migration.

Signed-off-by: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-balloon.c | 8 ++++++--
 include/migration/misc.h   | 2 ++
 migration/migration.c      | 8 ++++++++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index e770955176..d120bf8f43 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -66,8 +66,12 @@ static bool virtio_balloon_pbp_matches(PartiallyBalloonedPage *pbp,
 
 static bool virtio_balloon_inhibited(void)
 {
-    /* Postcopy cannot deal with concurrent discards, so it's special. */
-    return ram_block_discard_is_disabled() || migration_in_incoming_postcopy();
+    /*
+     * Postcopy cannot deal with concurrent discards,
+     * so it's special, as well as background snapshots.
+     */
+    return ram_block_discard_is_disabled() || migration_in_incoming_postcopy() ||
+            migration_in_bg_snapshot();
 }
 
 static void balloon_inflate_page(VirtIOBalloon *balloon,
diff --git a/include/migration/misc.h b/include/migration/misc.h
index bccc1b6b44..738675ef52 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -70,6 +70,8 @@ bool migration_in_postcopy_after_devices(MigrationState *);
 void migration_global_dump(Monitor *mon);
 /* True if incomming migration entered POSTCOPY_INCOMING_DISCARD */
 bool migration_in_incoming_postcopy(void);
+/* True if background snapshot is active */
+bool migration_in_bg_snapshot(void);
 
 /* migration/block-dirty-bitmap.c */
 void dirty_bitmap_mig_init(void);
diff --git a/migration/migration.c b/migration/migration.c
index 00e13f9d58..be4729e7c8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1976,6 +1976,14 @@ bool migration_in_incoming_postcopy(void)
     return ps >= POSTCOPY_INCOMING_DISCARD && ps < POSTCOPY_INCOMING_END;
 }
 
+bool migration_in_bg_snapshot(void)
+{
+    MigrationState *s = migrate_get_current();
+
+    return migrate_background_snapshot() &&
+            migration_is_setup_or_active(s->state);
+}
+
 bool migration_is_idle(void)
 {
     MigrationState *s = current_migration;
-- 
2.27.0

[PATCH for-6.0 3/3] migration: Pre-fault memory before starting background snasphot

[Andrey Gruzdev]

This commit solves the issue with userfault_fd WP feature that
background snapshot is based on. For any never poluated or discarded
memory page, the UFFDIO_WRITEPROTECT ioctl() would skip updating
PTE for that page, thereby loosing WP setting for it.

So we need to pre-fault pages for each RAM block to be protected
before making a userfault_fd wr-protect ioctl().

Signed-off-by: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
---
 migration/migration.c |  6 ++++++
 migration/ram.c       | 48 +++++++++++++++++++++++++++++++++++++++++++
 migration/ram.h       |  1 +
 3 files changed, 55 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index be4729e7c8..71bce15a1b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3827,6 +3827,12 @@ static void *bg_migration_thread(void *opaque)
 
     update_iteration_initial_status(s);
 
+    /*
+     * Prepare for tracking memory writes with UFFD-WP - populate
+     * RAM pages before protecting.
+     */
+    ram_write_tracking_prepare();
+
     qemu_savevm_state_header(s->to_dst_file);
     qemu_savevm_state_setup(s->to_dst_file);
 
diff --git a/migration/ram.c b/migration/ram.c
index 40e78952ad..24c8627214 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1560,6 +1560,54 @@ out:
     return ret;
 }
 
+/*
+ * ram_block_populate_pages: populate memory in the RAM block by reading
+ *   an integer from the beginning of each page.
+ *
+ * Since it's solely used for userfault_fd WP feature, here we just
+ *   hardcode page size to qemu_real_host_page_size.
+ *
+ * @bs: RAM block to populate
+ */
+static void ram_block_populate_pages(RAMBlock *bs)
+{
+    char *ptr = (char *) bs->host;
+
+    for (ram_addr_t offset = 0; offset < bs->used_length;
+            offset += qemu_real_host_page_size) {
+        char tmp = *(ptr + offset);
+        /* Don't optimize the read out */
+        asm volatile("" : "+r" (tmp));
+    }
+}
+
+/*
+ * ram_write_tracking_prepare: prepare for UFFD-WP memory tracking
+ */
+void ram_write_tracking_prepare(void)
+{
+    RAMBlock *bs;
+
+    RCU_READ_LOCK_GUARD();
+
+    RAMBLOCK_FOREACH_NOT_IGNORED(bs) {
+        /* Nothing to do with read-only and MMIO-writable regions */
+        if (bs->mr->readonly || bs->mr->rom_device) {
+            continue;
+        }
+
+        /*
+         * Populate pages of the RAM block before enabling userfault_fd
+         * write protection.
+         *
+         * This stage is required since ioctl(UFFDIO_WRITEPROTECT) with
+         * UFFDIO_WRITEPROTECT_MODE_WP mode setting would silently skip
+         * pages with pte_none() entries in page table.
+         */
+        ram_block_populate_pages(bs);
+    }
+}
+
 /*
  * ram_write_tracking_start: start UFFD-WP memory tracking
  *
diff --git a/migration/ram.h b/migration/ram.h
index 6378bb3ebc..4833e9fd5b 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -82,6 +82,7 @@ void colo_incoming_start_dirty_log(void);
 /* Background snapshot */
 bool ram_write_tracking_available(void);
 bool ram_write_tracking_compatible(void);
+void ram_write_tracking_prepare(void);
 int ram_write_tracking_start(void);
 void ram_write_tracking_stop(void);
 
-- 
2.27.0

Re: [PATCH for-6.0 3/3] migration: Pre-fault memory before starting background snasphot

[David Hildenbrand]

On 31.03.21 19:28, Andrey Gruzdev wrote:
> This commit solves the issue with userfault_fd WP feature that
> background snapshot is based on. For any never poluated or discarded
> memory page, the UFFDIO_WRITEPROTECT ioctl() would skip updating
> PTE for that page, thereby loosing WP setting for it.
> 
> So we need to pre-fault pages for each RAM block to be protected
> before making a userfault_fd wr-protect ioctl().
> 
> Signed-off-by: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
> ---
>   migration/migration.c |  6 ++++++
>   migration/ram.c       | 48 +++++++++++++++++++++++++++++++++++++++++++
>   migration/ram.h       |  1 +
>   3 files changed, 55 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index be4729e7c8..71bce15a1b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3827,6 +3827,12 @@ static void *bg_migration_thread(void *opaque)
>   
>       update_iteration_initial_status(s);
>   
> +    /*
> +     * Prepare for tracking memory writes with UFFD-WP - populate
> +     * RAM pages before protecting.
> +     */
> +    ram_write_tracking_prepare();
> +
>       qemu_savevm_state_header(s->to_dst_file);
>       qemu_savevm_state_setup(s->to_dst_file);
>   
> diff --git a/migration/ram.c b/migration/ram.c
> index 40e78952ad..24c8627214 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1560,6 +1560,54 @@ out:
>       return ret;
>   }
>   
> +/*
> + * ram_block_populate_pages: populate memory in the RAM block by reading
> + *   an integer from the beginning of each page.
> + *
> + * Since it's solely used for userfault_fd WP feature, here we just
> + *   hardcode page size to qemu_real_host_page_size.
> + *
> + * @bs: RAM block to populate
> + */
> +static void ram_block_populate_pages(RAMBlock *bs)

Usually we use "rb" or "block"; however migration/ram.c seems to do 
things differently.

> +{
> +    char *ptr = (char *) bs->host;
> +
> +    for (ram_addr_t offset = 0; offset < bs->used_length;
> +            offset += qemu_real_host_page_size) {
> +        char tmp = *(ptr + offset);

^ missing empty line.

> +        /* Don't optimize the read out */
> +        asm volatile("" : "+r" (tmp));
> +    }


Reviewed-by: David Hildenbrand <david@redhat.com>


and might want to add

Reported-by: David Hildenbrand <david@redhat.com>

(also to patch #2)

-- 
Thanks,

David / dhildenb

Re: [PATCH for-6.0 3/3] migration: Pre-fault memory before starting background snasphot

[David Hildenbrand]

On 31.03.21 19:33, David Hildenbrand wrote:
> On 31.03.21 19:28, Andrey Gruzdev wrote:
>> This commit solves the issue with userfault_fd WP feature that
>> background snapshot is based on. For any never poluated or discarded
>> memory page, the UFFDIO_WRITEPROTECT ioctl() would skip updating
>> PTE for that page, thereby loosing WP setting for it.
>>
>> So we need to pre-fault pages for each RAM block to be protected
>> before making a userfault_fd wr-protect ioctl().
>>
>> Signed-off-by: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
>> ---
>>    migration/migration.c |  6 ++++++
>>    migration/ram.c       | 48 +++++++++++++++++++++++++++++++++++++++++++
>>    migration/ram.h       |  1 +
>>    3 files changed, 55 insertions(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index be4729e7c8..71bce15a1b 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -3827,6 +3827,12 @@ static void *bg_migration_thread(void *opaque)
>>    
>>        update_iteration_initial_status(s);
>>    
>> +    /*
>> +     * Prepare for tracking memory writes with UFFD-WP - populate
>> +     * RAM pages before protecting.
>> +     */
>> +    ram_write_tracking_prepare();
>> +
>>        qemu_savevm_state_header(s->to_dst_file);
>>        qemu_savevm_state_setup(s->to_dst_file);
>>    
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 40e78952ad..24c8627214 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1560,6 +1560,54 @@ out:
>>        return ret;
>>    }
>>    
>> +/*
>> + * ram_block_populate_pages: populate memory in the RAM block by reading
>> + *   an integer from the beginning of each page.
>> + *
>> + * Since it's solely used for userfault_fd WP feature, here we just
>> + *   hardcode page size to qemu_real_host_page_size.
>> + *
>> + * @bs: RAM block to populate
>> + */
>> +static void ram_block_populate_pages(RAMBlock *bs)
> 
> Usually we use "rb" or "block"; however migration/ram.c seems to do
> things differently.
> 
>> +{
>> +    char *ptr = (char *) bs->host;
>> +
>> +    for (ram_addr_t offset = 0; offset < bs->used_length;
>> +            offset += qemu_real_host_page_size) {
>> +        char tmp = *(ptr + offset);
> 
> ^ missing empty line.
> 
>> +        /* Don't optimize the read out */
>> +        asm volatile("" : "+r" (tmp));
>> +    }
> 
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> 
> and might want to add
> 
> Reported-by: David Hildenbrand <david@redhat.com>
> 
> (also to patch #2)
> 

Also, proper "Fixes:" tags would be handy as well.

-- 
Thanks,

David / dhildenb

Re: [PATCH for-6.0 3/3] migration: Pre-fault memory before starting background snasphot

[Andrey Gruzdev]

On 31.03.2021 20:33, David Hildenbrand wrote:
> On 31.03.21 19:28, Andrey Gruzdev wrote:
>> This commit solves the issue with userfault_fd WP feature that
>> background snapshot is based on. For any never poluated or discarded
>> memory page, the UFFDIO_WRITEPROTECT ioctl() would skip updating
>> PTE for that page, thereby loosing WP setting for it.
>>
>> So we need to pre-fault pages for each RAM block to be protected
>> before making a userfault_fd wr-protect ioctl().
>>
>> Signed-off-by: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
>> ---
>>   migration/migration.c |  6 ++++++
>>   migration/ram.c       | 48 +++++++++++++++++++++++++++++++++++++++++++
>>   migration/ram.h       |  1 +
>>   3 files changed, 55 insertions(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index be4729e7c8..71bce15a1b 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -3827,6 +3827,12 @@ static void *bg_migration_thread(void *opaque)
>>         update_iteration_initial_status(s);
>>   +    /*
>> +     * Prepare for tracking memory writes with UFFD-WP - populate
>> +     * RAM pages before protecting.
>> +     */
>> +    ram_write_tracking_prepare();
>> +
>>       qemu_savevm_state_header(s->to_dst_file);
>>       qemu_savevm_state_setup(s->to_dst_file);
>>   diff --git a/migration/ram.c b/migration/ram.c
>> index 40e78952ad..24c8627214 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1560,6 +1560,54 @@ out:
>>       return ret;
>>   }
>>   +/*
>> + * ram_block_populate_pages: populate memory in the RAM block by 
>> reading
>> + *   an integer from the beginning of each page.
>> + *
>> + * Since it's solely used for userfault_fd WP feature, here we just
>> + *   hardcode page size to qemu_real_host_page_size.
>> + *
>> + * @bs: RAM block to populate
>> + */
>> +static void ram_block_populate_pages(RAMBlock *bs)
>
> Usually we use "rb" or "block"; however migration/ram.c seems to do 
> things differently.
>
Yes, I'll rename.
>> +{
>> +    char *ptr = (char *) bs->host;
>> +
>> +    for (ram_addr_t offset = 0; offset < bs->used_length;
>> +            offset += qemu_real_host_page_size) {
>> +        char tmp = *(ptr + offset);
>
> ^ missing empty line.
>
Aha.
>> +        /* Don't optimize the read out */
>> +        asm volatile("" : "+r" (tmp));
>> +    }
>
>
> Reviewed-by: David Hildenbrand <david@redhat.com>
>
>
> and might want to add
>
> Reported-by: David Hildenbrand <david@redhat.com>
>
> (also to patch #2)
>
I'll add, thanks.


-- 
Andrey Gruzdev, Principal Engineer
Virtuozzo GmbH  +7-903-247-6397
                 virtuzzo.com

Re: [PATCH for-6.0 3/3] migration: Pre-fault memory before starting background snasphot

[Andrey Gruzdev]

On 31.03.2021 20:37, David Hildenbrand wrote:
> On 31.03.21 19:33, David Hildenbrand wrote:
>> On 31.03.21 19:28, Andrey Gruzdev wrote:
>>> This commit solves the issue with userfault_fd WP feature that
>>> background snapshot is based on. For any never poluated or discarded
>>> memory page, the UFFDIO_WRITEPROTECT ioctl() would skip updating
>>> PTE for that page, thereby loosing WP setting for it.
>>>
>>> So we need to pre-fault pages for each RAM block to be protected
>>> before making a userfault_fd wr-protect ioctl().
>>>
>>> Signed-off-by: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
>>> ---
>>>    migration/migration.c |  6 ++++++
>>>    migration/ram.c       | 48 
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>    migration/ram.h       |  1 +
>>>    3 files changed, 55 insertions(+)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index be4729e7c8..71bce15a1b 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -3827,6 +3827,12 @@ static void *bg_migration_thread(void *opaque)
>>>           update_iteration_initial_status(s);
>>>    +    /*
>>> +     * Prepare for tracking memory writes with UFFD-WP - populate
>>> +     * RAM pages before protecting.
>>> +     */
>>> +    ram_write_tracking_prepare();
>>> +
>>>        qemu_savevm_state_header(s->to_dst_file);
>>>        qemu_savevm_state_setup(s->to_dst_file);
>>>    diff --git a/migration/ram.c b/migration/ram.c
>>> index 40e78952ad..24c8627214 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -1560,6 +1560,54 @@ out:
>>>        return ret;
>>>    }
>>>    +/*
>>> + * ram_block_populate_pages: populate memory in the RAM block by 
>>> reading
>>> + *   an integer from the beginning of each page.
>>> + *
>>> + * Since it's solely used for userfault_fd WP feature, here we just
>>> + *   hardcode page size to qemu_real_host_page_size.
>>> + *
>>> + * @bs: RAM block to populate
>>> + */
>>> +static void ram_block_populate_pages(RAMBlock *bs)
>>
>> Usually we use "rb" or "block"; however migration/ram.c seems to do
>> things differently.
>>
>>> +{
>>> +    char *ptr = (char *) bs->host;
>>> +
>>> +    for (ram_addr_t offset = 0; offset < bs->used_length;
>>> +            offset += qemu_real_host_page_size) {
>>> +        char tmp = *(ptr + offset);
>>
>> ^ missing empty line.
>>
>>> +        /* Don't optimize the read out */
>>> +        asm volatile("" : "+r" (tmp));
>>> +    }
>>
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>
>>
>> and might want to add
>>
>> Reported-by: David Hildenbrand <david@redhat.com>
>>
>> (also to patch #2)
>>
>
> Also, proper "Fixes:" tags would be handy as well.
>
Ok, thanks.


-- 
Andrey Gruzdev, Principal Engineer
Virtuozzo GmbH  +7-903-247-6397
                 virtuzzo.com