Re: [PATCH v4 07/11] migration: add support for fault thread to load pages from disk

On Wed, Aug 12, 2026 at 10:08:38PM +0530, Aadeshveer Singh wrote:
> On Mon, Aug 10, 2026 at 9:10 PM Peter Xu <[email protected]> wrote:
> >
> > Aadeshveer,
> >
> > Thanks for looking into this problem.  Below solution should work in
> > general, but there's still one problem..
> >
> > On Sat, Aug 08, 2026 at 10:04:48AM +0530, Aadeshveer Singh wrote:
> > > A subtle concurrency bug exists in the current patch; this small patch
> > > should fix it.
> > >
> > > Current patch uses bitmap_test_and_clear_atomic assuming it performs
> > > an atomic operation over a range, which as pointer out by Peter is not
> > > the case. It performs atomic word by word operations making the
> > > overall operation unsafe.
> > > Therefore, keeping the same bitmap would require protection by a mutex
> > > lock which might not be as efficient. Hence this small patch resizes
> > > the pending_bmap to have exactly one bit per load operation(loading
> > > the larger of host page and guest page).
> > >
> > > Thank you,
> > > Aadeshveer Singh
> > >
> > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > > index e01ac628f5..0bf0f837ad 100644
> > > --- a/migration/postcopy-ram.c
> > > +++ b/migration/postcopy-ram.c
> > > @@ -1031,9 +1031,13 @@ static bool
> > > postcopy_mapped_ram_load_page(MigrationIncomingState *mis,
> > >      host_page = rb_offset / qemu_ram_pagesize(rb);
> > >      page = rb_offset >> qemu_target_page_bits();
> > >
> > > -    if (bitmap_test_and_clear_atomic(
> > > -            rb->pending_bmap, host_page,
> > > -            MAX(1, qemu_target_page_size() / qemu_ram_pagesize(rb)))) {
> > > +    /*
> > > +     * pending_bmap needs the index of host or guest page based on which is
> > > +     * larger. As page index is inversely proportional to page size we use the
> > > +     * minimum of both.
> > > +     */
> > > +    if (bitmap_test_and_clear_atomic(rb->pending_bmap, MIN(host_page, page),
> > > +                                     1)) {
> > >          if (find_next_bit(rb->file_bmap, page + guest_pages_to_load, page) ==
> > >              page + guest_pages_to_load) {
> > >              /* It is efficient to use UFFDIO_ZERO if all pages are zero */
> >
> > Consider the case where guest psize > host psize, here the current code
> > will invoke postcopy_place_page_zero() or postcopy_place_page() only once
> > for each guest page. But IIUC that's not enough: we'll need to loop over
> > the few host pages that is covered by the same guest page.
> I believe I missed it, confusing the page sizes. One of the solutions
> can be a loop or a cleaner solution might be to give larger length to
> ioctl command to handle multiple pages at once. This should be
> efficient but might require some changes to basic postcopy code as
> most of it hardcoded length as page size to most userfaultfd_ioctl
> calls.

Yes the helpers were for one host page only.

Since you mentioned, I found that maybe the generic postcopy code has issue
with such special psize setup too.. see qemu_ufd_copy_ioctl() where it has:

        ramblock_recv_bitmap_set_range(rb, host_addr,
                                       pagesize / qemu_target_page_size());

Here pagesize should be host psize, which means if guest psize is larger
then it'll have nr==0, so QEMU is not properly updating the receivedmap in
postcopy phase.  It's highly likely nobody ran postcopy with such setup
before.

> >
> > I confess this is really a rare corner case..  so you can decide how to
> > "fix" it.  There's always the option to disable this feature for now when
> > guest psize is larger than host psize (OTOH, host psize > guest psize is
> > much more common, because that's normally how huge page backed VMs work on
> > linux systems).
> >
> > Or just provide the loops over host pages, I think it will start working
> > and IIUC it's indeed the most efficient.  But then, to make it slightly
> > easier to future readers (I still think the bitmap definition will be
> > slightly hard to grasp for future readers.. your comments all over
> > hopefully will help), maybe we can also rename "page"; it implies guest
> > page index but it's not obvious.  We can make it "guest_page" to match
> > "host_page".  The "haddr" seems fine.
> I believe it should be simple to implement using direct syscalls,
> however for a cleaner solution current functions need to be made more
> modular. I will try the latter and if the changes go out of hand might
> resort to direct syscalls or loops.

Let's avoid direct syscalls; it's not good for long term maintenability.

Please try to see if it's easy to fix this problem.  If it's time
consuming, let's fallback to fail the enablement of postcopy when guest
psize is larger, and leave it for later; such breakage should apply to
remote postcopy too.

Thanks,

-- 
Peter Xu