Re: [RFC PATCH v2 6/7] migration: add support for fault thread to load pages from disk

[Peter Xu <peterx@redhat.com>]

On Tue, Jun 30, 2026 at 01:49:39PM +0530, Aadeshveer Singh wrote:
> In fast snapshot load, we would like to serve faults as soon as possible
> hence loading pages directly instead of requesting a source
> 
> Add postcopy_mapped_ram_load_page() function which serves single page
> fault by reading the snapshot file. It uses bitmap_test_and_clear_atomic
> on pending_bmap to coordinate between threads so each page is loaded
> exactly once. Non-zero pages are read using qemu_get_buffer_at into a
> temporary page (for loading page atomically), which is then placed using
> postcopy_place_page. Zero pages are placed directly using
> postcopy_place_page_zero.
> 
> Update postcopy_ram_fault_thread to call postcopy_mapped_ram_load_page
> instead of requesting source in case of fast snapshot load. to_src_file
> check is bypassed in fast snapshot load case as there is no source.
> 
> Add postcopy-blocktime support by calling mark_postcopy_blocktime_begin
> on every fault. We need to aqcuire the page_request_mutex lock and check
> the recv bitmap to make sure we prevent a race condition when a fault
> occurs but before the fault thread can mark the page for tracking the
> eager thread loads in the page making us fail the assert for recv bitmap
> being clear while marking in mark_postcopy_blocktime_begin().
> 
> Allocate another channel in postcopy_temp_pages_setup(like the preempt
> case), for both the fault thread and eager thread to load pages
> independently.
> 
> Signed-off-by: Aadeshveer Singh <aadeshveer07@gmail.com>
> ---
>  migration/postcopy-ram.c | 121 +++++++++++++++++++++++++++++++++------
>  1 file changed, 104 insertions(+), 17 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 96a65aa976..be9edac572 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -949,6 +949,68 @@ int postcopy_wake_shared(struct PostCopyFD *pcfd,
>                         pagesize);
>  }
>  
> +/**
> + * postcopy_mapped_ram_load_page() - Load a page to given host address.
> + * @mis: Migration Incoming State.
> + * @rb: RAMBlock from where page is loaded.
> + * @rb_offset: Offset of page in RAMBlock.
> + * @haddr: Base of page where to load in page.
> + * @channel: Used to identify between threads and use corresponding temp.

Missing @errp.

> + *
> + * Load a page from RAMBlock at offset at given host address. Used by postcopy
> + * ram fault thread and eager thread in fast snapshot load case.
> + *
> + * Return: True on success.
> + */
> +static bool postcopy_mapped_ram_load_page(MigrationIncomingState *mis,
> +                                          RAMBlock *rb, ram_addr_t rb_offset,
> +                                          uint64_t haddr, int channel,
> +                                          Error **errp)
> +{
> +    void *place_source = mis->postcopy_tmp_pages[channel].tmp_huge_page;
> +    size_t page;
> +    size_t read;
> +
> +    page = rb_offset / qemu_ram_pagesize(rb);

Yes, here it is correct to use qemu_ram_pagesize().

> +
> +    if (bitmap_test_and_clear_atomic(rb->pending_bmap, page, 1)) {
> +        if (test_bit(page, rb->file_bmap)) {

We'll need to be careful here when you add huge page support, because
file_bmap so far is guest-psize based.  This line will start to break for
huge pages, I am not sure if this is the bug you hit when developing huge
page support, maybe yes?

Not sure how you resolved it there if this is the case, but one idea is
when postcopy-ram is enabled, you can convert the per-guest-psize bitmap
into per-host-psize bitmap (when any bit set within the huge page range,
set the bit in the new bitmap), then here it will be correct.

> +            /*
> +             * This can happen concurrently, but it's thread-safe because
> +             * qemu_get_buffer_at() is thread-safe, and the caller will be using
> +             * different temporary buffers.
> +             */
> +            read = qemu_get_buffer_at(mis->from_src_file, place_source,
> +                                      qemu_ram_pagesize(rb),
> +                                      rb->pages_offset + rb_offset);
> +
> +            if (read != qemu_ram_pagesize(rb)) {
> +                error_setg(errp, "Could not read page %zu from RAM Block %s",
> +                           page, rb->idstr);
> +                return false;
> +            }
> +
> +            if (postcopy_place_page(mis, (void *)haddr, place_source, rb)) {
> +                error_setg(errp,
> +                           "Failed to place page %zu from RAM Block %s at "
> +                           "address %" PRIu64,
> +                           page, rb->idstr, haddr);
> +                return false;
> +            }
> +

Nit: unnecessary newline.

> +        } else {
> +            if (postcopy_place_page_zero(mis, (void *)haddr, rb)) {
> +                error_setg(errp,
> +                           "Failed to place zero page %zu from RAM Block %s at "
> +                           "address %" PRIu64,
> +                           page, rb->idstr, haddr);
> +                return false;
> +            }
> +        }
> +    }
> +    return true;
> +}
> +
>  /*
>   * NOTE: @tid is only used when postcopy-blocktime feature is enabled, and
>   * also optional: when zero is provided, the fault accounting will be ignored.
> @@ -1279,6 +1341,7 @@ static void *postcopy_ram_fault_thread(void *opaque)
>      int ret;
>      size_t index;
>      RAMBlock *rb = NULL;
> +    Error *local_err = NULL;
>  
>      trace_postcopy_ram_fault_thread_entry();
>      rcu_register_thread();
> @@ -1320,11 +1383,13 @@ static void *postcopy_ram_fault_thread(void *opaque)
>              break;
>          }
>  
> -        if (!mis->to_src_file) {
> +        if (!migrate_mapped_ram() && !mis->to_src_file) {
>              /*
> -             * Possibly someone tells us that the return path is
> -             * broken already using the event. We should hold until
> -             * the channel is rebuilt.
> +             * Possibly someone tells us that the return path is broken already
> +             * using the event. We should hold until the channel is rebuilt.
> +             * Fast snapshot load doesn't support pause and recover, because
> +             * it's not necessary: we can fail right away when QEMU just booted
> +             * with nothing to lose.
>               */
>              postcopy_pause_fault_thread(mis);
>          }
> @@ -1387,18 +1452,37 @@ static void *postcopy_ram_fault_thread(void *opaque)
>                                                  qemu_ram_get_idstr(rb),
>                                                  rb_offset,
>                                                  msg.arg.pagefault.feat.ptid);
> +
> +            if (migrate_mapped_ram()) {
> +                /* Load page directly in case of fast snapshot load */
> +                WITH_QEMU_LOCK_GUARD(&mis->page_request_mutex) {
> +                    if (!ramblock_recv_bitmap_test(
> +                            rb, (void *)msg.arg.pagefault.address)) {
> +                        mark_postcopy_blocktime_begin(
> +                            msg.arg.pagefault.address,
> +                            msg.arg.pagefault.feat.ptid, rb);
> +                    }
> +                }

This part needs some more thought.

First of all, if the page is set already in receivedmap, it means it's
already installed, we shouldn't bother postcopy_mapped_ram_load_page().

But the problem is not only about that.

So far looks like the blocktime feature relies on the receivedmap, then it
means the easy way for this series is to support receivedmap too.  Then
with receivedmap, we should unify the receivedmap detection rather than
testing the bit at multiple places.

Say, you can provide a small helper like this:

diff --git a/migration/migration.c b/migration/migration.c
index 278cad502a..79f737bc69 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -573,12 +573,10 @@ int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
     return migrate_send_rp_message(mis, msg_type, msglen, bufc);
 }

-int migrate_send_rp_req_pages(MigrationIncomingState *mis,
-                              RAMBlock *rb, ram_addr_t start, uint64_t haddr,
-                              uint32_t tid)
+bool postcopy_page_received(MigrationIncomingState *mis, RAMBlock *rb,
+                            ram_addr_t start, uint64_t haddr, uint32_t tid)
 {
-    void *aligned = (void *)(uintptr_t)ROUND_DOWN(haddr, qemu_ram_pagesize(rb));
-    bool received = false;
+    bool received;

     WITH_QEMU_LOCK_GUARD(&mis->page_request_mutex) {
         received = ramblock_recv_bitmap_test_byte_offset(rb, start);
@@ -598,11 +596,20 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis,
         }
     }

+    return received;
+}
+
+int migrate_send_rp_req_pages(MigrationIncomingState *mis,
+                              RAMBlock *rb, ram_addr_t start, uint64_t haddr,
+                              uint32_t tid)
+{
+    void *aligned = (void *)(uintptr_t)ROUND_DOWN(haddr, qemu_ram_pagesize(rb));
+
     /*
      * If the page is there, skip sending the message.  We don't even need the
      * lock because as long as the page arrived, it'll be there forever.
      */
-    if (received) {
+    if (postcopy_page_received(mis, rb, start, aligned, tid)) {
         return 0;
     }

Then I think we can use postcopy_page_received() here to avoid duplicating
the code.

It means with your feature we will also maintain the tree of page_requested
but I think it's fine; if we really want we can make that optional based on
whether mapped-ram enabled, but not yet required.

PS: I feel like there was a bug in existing code passing "haddr" (rather
than "aligned") into mark_postcopy_blocktime_begin() in the current impl of
migrate_send_rp_req_pages().  Please help to check if it's an issue, if
true we can have a separate patch fixing that first.  In general, I think
we should use a page-aligned address in both the tree and blocktime.

> +                if (!postcopy_mapped_ram_load_page(
> +                        mis, rb, rb_offset, msg.arg.pagefault.address,
> +                        RAM_CHANNEL_POSTCOPY, &local_err)) {
> +                    error_report_err(local_err);
> +                    break;
> +                }
> +            } else {
>  retry:
> -            /*
> -             * Send the request to the source - we want to request one
> -             * of our host page sizes (which is >= TPS)
> -             */
> -            ret = postcopy_request_page(mis, rb, rb_offset,
> -                                        msg.arg.pagefault.address,
> -                                        msg.arg.pagefault.feat.ptid);
> -            if (ret) {
> -                /* May be network failure, try to wait for recovery */
> -                postcopy_pause_fault_thread(mis);
> -                goto retry;
> +                /*
> +                 * Send the request to the source - we want to request one
> +                 * of our host page sizes (which is >= TPS)
> +                 */
> +                ret = postcopy_request_page(mis, rb, rb_offset,
> +                                            msg.arg.pagefault.address,
> +                                            msg.arg.pagefault.feat.ptid);
> +                if (ret) {
> +                    /* May be network failure, try to wait for recovery */
> +                    postcopy_pause_fault_thread(mis);
> +                    goto retry;
> +                }
>              }
>          }
>  
> @@ -1470,8 +1554,11 @@ static int postcopy_temp_pages_setup(MigrationIncomingState *mis, Error **errp)
>      unsigned i, channels;
>      void *temp_page;
>  
> -    if (migrate_postcopy_preempt()) {
> -        /* If preemption enabled, need extra channel for urgent requests */
> +    if (migrate_postcopy_preempt() || migrate_mapped_ram()) {
> +        /*
> +         * If preemption enabled or it is fast snapshot load, need extra channel
> +         * for urgent requests/faults
> +         */
>          mis->postcopy_channels = RAM_CHANNEL_MAX;
>      } else {
>          /* Both precopy/postcopy on the same channel */
> -- 
> 2.54.0
> 

-- 
Peter Xu