Re: [PULL 08/23] migration: Run final save_query_pending at switchover

[Avihai Horon <avihaih@nvidia.com>]

On 7/3/2026 11:44 AM, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 7/3/26 10:05, Avihai Horon wrote:
>>
>> On 6/30/2026 7:30 AM, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> From: Avihai Horon <avihaih@nvidia.com>
>>>
>>> Before switchover, the source needs one last exact pending query so
>>> modules can flush dirty state. This is currently done ad hoc in modules
>>> handlers. For example, RAM syncs its dirty bitmap in its save_complete
>>> handler.
>>>
>>> This should be a general concept relevant for any module, so extract it
>>> to migration core instead by running a final save_query_pending before
>>> switchover.
>>>
>>> The final query requires special handling by modules (e.g., it's called
>>> with BQL locked, during VM stop), so extend save_query_pending
>>> SaveVMHandlers callback and qemu_savevm_query_pending() with a "final"
>>> flag so migration modules can tell the last pending query during
>>> switchover from periodic iteration queries.
>>>
>>> Reviewed-by: Peter Xu <peterx@redhat.com>
>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>> Reviewed-by: Jason J. Herne <jjhern@linux.ibm.com>
>>> Link: 
>>> https://lore.kernel.org/qemu-devel/20260622101214.25770-4-avihaih@nvidia.com
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>   include/migration/register.h   | 41 
>>> ++++++++++++++++++----------------
>>>   migration/savevm.h             |  3 ++-
>>>   hw/s390x/s390-stattrib.c       |  2 +-
>>>   hw/vfio/migration.c            | 11 ++++++---
>>>   migration/block-dirty-bitmap.c | 11 ++++++---
>>>   migration/migration.c          | 14 ++++++++++--
>>>   migration/ram.c                | 40 +++++++++++++++++++--------------
>>>   migration/savevm.c             | 20 +++++++++++++----
>>>   hw/vfio/trace-events           |  2 +-
>>>   migration/trace-events         |  2 +-
>>>   10 files changed, 94 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/include/migration/register.h 
>>> b/include/migration/register.h
>>> index 
>>> 5e5e0ee432e537781ce78e84b2ff2d81c33e8f37..6f632123f1d0710de0556e68ba53e068a1e02640 
>>> 100644
>>> --- a/include/migration/register.h
>>> +++ b/include/migration/register.h
>>> @@ -171,6 +171,28 @@ typedef struct SaveVMHandlers {
>>>        */
>>>       bool (*is_active_iterate)(void *opaque);
>>>
>>> +    /**
>>> +     * @save_query_pending
>>> +     *
>>> +     * This estimates the remaining data to transfer on the source 
>>> side.
>>> +     *
>>> +     * When @exact is true, a module must report accurate results.  
>>> When
>>> +     * @exact is false, a module may report estimates.
>>> +     *
>>> +     * It's highly recommended that modules implement a faster 
>>> version of
>>> +     * the query path (for example, by proper caching on the 
>>> counters) if
>>> +     * an accurate query will be time-consuming.
>>> +     *
>>> +     * @opaque: data pointer passed to register_savevm_live()
>>> +     * @pending: pointer to a MigPendingData struct
>>> +     * @exact: set to true for an accurate (slow) query
>>> +     * @final: set to true for the final query during switchover. 
>>> When final is
>>> +     * true, the query is called with BQL locked. Otherwise, it's 
>>> called with
>>> +     * BQL unlocked.
>>> +     */
>>> +    void (*save_query_pending)(void *opaque, MigPendingData *pending,
>>> +                               bool exact, bool final);
>>> +
>>>       /* This runs outside the BQL in the migration case, and
>>>        * within the lock in the savevm case.  The callback had 
>>> better only
>>>        * use data that is local to the migration thread or protected
>>> @@ -210,25 +232,6 @@ typedef struct SaveVMHandlers {
>>>        */
>>>       bool (*save_postcopy_prepare)(QEMUFile *f, void *opaque, Error 
>>> **errp);
>>>
>>> -    /**
>>> -     * @save_query_pending
>>> -     *
>>> -     * This estimates the remaining data to transfer on the source 
>>> side.
>>> -     *
>>> -     * When @exact is true, a module must report accurate results.  
>>> When
>>> -     * @exact is false, a module may report estimates.
>>> -     *
>>> -     * It's highly recommended that modules implement a faster 
>>> version of
>>> -     * the query path (for example, by proper caching on the 
>>> counters) if
>>> -     * an accurate query will be time-consuming.
>>> -     *
>>> -     * @opaque: data pointer passed to register_savevm_live()
>>> -     * @pending: pointer to a MigPendingData struct
>>> -     * @exact: set to true for an accurate (slow) query
>>> -     */
>>> -    void (*save_query_pending)(void *opaque, MigPendingData *pending,
>>> -                               bool exact);
>>> -
>>>       /**
>>>        * @load_state
>>>        *
>>> diff --git a/migration/savevm.h b/migration/savevm.h
>>> index 
>>> b6bb4fa977fae84ed121b0668cdefdf49e3d9c67..81abd96dda94c6c95cc40b3c188df23d11c19463 
>>> 100644
>>> --- a/migration/savevm.h
>>> +++ b/migration/savevm.h
>>> @@ -45,7 +45,8 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool 
>>> postcopy);
>>>   void qemu_savevm_state_cleanup(void);
>>>   void qemu_savevm_state_complete_postcopy(QEMUFile *f);
>>>   int qemu_savevm_state_complete_precopy(MigrationState *s, Error 
>>> **errp);
>>> -void qemu_savevm_query_pending(MigPendingData *pending, bool exact);
>>> +void qemu_savevm_query_pending_iter(MigPendingData *pending, bool 
>>> exact);
>>> +void qemu_savevm_query_pending_final(MigPendingData *pending);
>>>   int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool 
>>> in_postcopy);
>>>   bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
>>>   void qemu_savevm_state_end(QEMUFile *f);
>>> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
>>> index 
>>> c334714b31ab4313e26c0cf57091a8cb21e55306..b0f04eb30c7f73e5dd91b1345d71a14cb404a4eb 
>>> 100644
>>> --- a/hw/s390x/s390-stattrib.c
>>> +++ b/hw/s390x/s390-stattrib.c
>>> @@ -190,7 +190,7 @@ static int cmma_save_setup(QEMUFile *f, void 
>>> *opaque, Error **errp)
>>>   }
>>>
>>>   static void cmma_state_pending(void *opaque, MigPendingData *pending,
>>> -                               bool exact)
>>> +                               bool exact, bool final)
>>>   {
>>>       S390StAttribState *sas = S390_STATTRIB(opaque);
>>>       S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 
>>> fb12b9717f773fdde657911517de9d74c1eb3931..195498845ed95d6a55cf68537a100b4e83221fb0 
>>> 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -622,13 +622,18 @@ static void vfio_state_pending_sync(VFIODevice 
>>> *vbasedev)
>>>   }
>>>
>>>   static void vfio_state_pending(void *opaque, MigPendingData *pending,
>>> -                               bool exact)
>>> +                               bool exact, bool final)
>>>   {
>>>       VFIODevice *vbasedev = opaque;
>>>       VFIOMigration *migration = vbasedev->migration;
>>>       uint64_t precopy_size, stopcopy_size;
>>>
>>> -    if (exact) {
>>> +    /*
>>> +     * The final pending query runs during switchover downtime. 
>>> VFIO does not
>>> +     * need a fresh device pending-data query then to get the 
>>> latest dirty
>>> +     * data, so avoid the extra work and report the cached counters 
>>> below.
>>> +     */
>>> +    if (exact && !final) {
>>>           vfio_state_pending_sync(vbasedev);
>>>       }
>>>
>>> @@ -646,7 +651,7 @@ static void vfio_state_pending(void *opaque, 
>>> MigPendingData *pending,
>>>
>>>       trace_vfio_state_pending(vbasedev->name, 
>>> migration->stopcopy_size,
>>> migration->precopy_init_size,
>>> - migration->precopy_dirty_size, exact);
>>> + migration->precopy_dirty_size, exact, final);
>>>   }
>>>
>>>   static bool vfio_is_active_iterate(void *opaque)
>>> diff --git a/migration/block-dirty-bitmap.c 
>>> b/migration/block-dirty-bitmap.c
>>> index 
>>> 7ef3759e5348a6907c0fb3773e2c6cbb0850632e..cba54e25cd50da924e1958bdfc23ce060784b0ac 
>>> 100644
>>> --- a/migration/block-dirty-bitmap.c
>>> +++ b/migration/block-dirty-bitmap.c
>>> @@ -767,13 +767,16 @@ static int dirty_bitmap_save_complete(QEMUFile 
>>> *f, void *opaque)
>>>   }
>>>
>>>   static void dirty_bitmap_state_pending(void *opaque, 
>>> MigPendingData *data,
>>> -                                       bool exact)
>>> +                                       bool exact, bool final)
>>>   {
>>>       DBMSaveState *s = &((DBMState *)opaque)->save;
>>>       SaveBitmapState *dbms;
>>>       uint64_t pending = 0;
>>>
>>> -    bql_lock();
>>> +    /* Final pending query is called with BQL locked */
>>> +    if (!final) {
>>> +        bql_lock();
>>> +    }
>>>
>>>       QSIMPLEQ_FOREACH(dbms, &s->dbms_list, entry) {
>>>           uint64_t gran = bdrv_dirty_bitmap_granularity(dbms->bitmap);
>>> @@ -783,7 +786,9 @@ static void dirty_bitmap_state_pending(void 
>>> *opaque, MigPendingData *data,
>>>           pending += DIV_ROUND_UP(sectors * BDRV_SECTOR_SIZE, gran);
>>>       }
>>>
>>> -    bql_unlock();
>>> +    if (!final) {
>>> +        bql_unlock();
>>> +    }
>>>
>>>       trace_dirty_bitmap_state_pending(pending);
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 
>>> 0f9e891cf85c8e5074f7f9800515e244fa413fa2..ea0b236597087cc005541e1916ceabb229afab4b 
>>> 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -2787,12 +2787,22 @@ static bool 
>>> migration_switchover_prepare(MigrationState *s)
>>>   static bool migration_switchover_start(MigrationState *s, Error 
>>> **errp)
>>>   {
>>>       ERRP_GUARD();
>>> +    MigPendingData pending = {};
>>>
>>>       if (!migration_switchover_prepare(s)) {
>>>           error_setg(errp, "Switchover is interrupted");
>>>           return false;
>>>       }
>>>
>>> +    /*
>>> +     * The final query to the whole system on dirty data to make 
>>> sure we
>>> +     * collect the latest status of the VM.  For precopy, source 
>>> QEMU will
>>> +     * dump all the dirty data during switchover.  For postcopy, 
>>> this will
>>> +     * properly update all the dirty bitmaps to finally generate the
>>> +     * correct discard bitmaps; see 
>>> ram_postcopy_send_discard_bitmap().
>>> +     */
>>> +    qemu_savevm_query_pending_final(&pending);
>>> +
>>>       /* Inactivate disks except in COLO */
>>>       if (!migrate_colo()) {
>>>           /*
>>> @@ -3285,7 +3295,7 @@ static void 
>>> migration_iteration_go_next(MigPendingData *pending)
>>>       /*
>>>        * Do a slow sync first before boosting the iteration count.
>>>        */
>>> -    qemu_savevm_query_pending(pending, true);
>>> +    qemu_savevm_query_pending_iter(pending, true);
>>>
>>>       /*
>>>        * Update the dirty information for the whole system for this
>>> @@ -3336,7 +3346,7 @@ static MigIterateState 
>>> migration_iteration_run(MigrationState *s)
>>>       bool complete_ready;
>>>
>>>       /* Fast path - get the estimated amount of pending data */
>>> -    qemu_savevm_query_pending(&pending, false);
>>> +    qemu_savevm_query_pending_iter(&pending, false);
>>>
>>>       if (in_postcopy) {
>>>           /*
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index 
>>> 55aa9b47dc8ab8463fd1aeba19726a493b81b070..8918b2f03b6fbcc98532bb45ab905f9bac59384c 
>>> 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -2686,9 +2686,6 @@ void 
>>> ram_postcopy_send_discard_bitmap(MigrationState *ms)
>>>
>>>       RCU_READ_LOCK_GUARD();
>>>
>>> -    /* This should be our last sync, the src is now paused */
>>> -    migration_bitmap_sync_precopy(true);
>>> -
>>>       /* Easiest way to make sure we don't resume in the middle of a 
>>> host-page */
>>>       rs->pss[RAM_CHANNEL_PRECOPY].last_sent_block = NULL;
>>>       rs->last_seen_block = NULL;
>>> @@ -3376,10 +3373,6 @@ static int ram_save_complete(QEMUFile *f, 
>>> void *opaque)
>>>       rs->last_stage = !migration_in_colo_state();
>>>
>>>       WITH_RCU_READ_LOCK_GUARD() {
>>> -        if (!migration_in_postcopy()) {
>>> -            migration_bitmap_sync_precopy(true);
>>> -        }
>>> -
>>
>> (Got some technical issues receiving QEMU ML mails that I'm not CCed 
>> to, so replying here for Stefan's mail [1])
>>
>> Seems like this patch overlooked the COLO case which calls 
>> ram_save_complete() on each checkpoint and expects the RAM complete 
>> handler to sync RAM dirty bitmap.
>> This patch moved this dirty sync from ram_save_complete() (the 
>> removed lines above) into a qemu_savevm_query_pending_final() called 
>> only once at migration_switchover_start().
>>
>> I believe this is the root cause for the CI failure reported by 
>> Stefan, since I was able to repro this locally [2] and the diff below 
>> [3] fixed the issue [4].
>>
>> I have zero knowledge in COLO so would really appreciate some 
>> feedback from Peter/COLO folks if that's a valid fix.
>>
>> Thanks.
>>
>> [1] 
>> https://lore.kernel.org/qemu-devel/CAJSP0QX-8L2CSX5q95Md5V6NTE_4p+K8Kr-QGbqKm2BBMtdh5Q@mail.gmail.com/
>>
>> [2]
>>
>> QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test 
>> -k --full -r /x86_64/migration/colo
>> [...]
>> # Running /x86_64/migration/colo/plain/secondary_failover
>> # Using machine type: pc-q35-11.1
>> # starting QEMU: exec ./qemu-system-x86_64 -qtest 
>> unix:/tmp/qtest-1415533.sock -qtest-log /dev/null -chardev 
>> socket,path=/tmp/qtest-1415533.qmp,id=char0 -mon 
>> chardev=char0,mode=control -display none -audio
>> none -run-with exit-with-parent=on -accel kvm -accel tcg -machine 
>> pc-q35-11.1, -name source,debug-threads=on -machine 
>> memory-backend=mig.mem -object 
>> memory-backend-ram,id=mig.mem,size=150M,share=off -serial
>>   file:/tmp/migration-test-GF36R3/src_serial -drive 
>> if=none,id=d0,file=/tmp/migration-test-GF36R3/bootsect,format=raw 
>> -device ide-hd,drive=d0,secs=1,cyls=1,heads=1 -snapshot 2>/dev/null 
>> -accel qtest
>> # starting QEMU: exec ./qemu-system-x86_64 -qtest 
>> unix:/tmp/qtest-1415533.sock -qtest-log /dev/null -chardev 
>> socket,path=/tmp/qtest-1415533.qmp,id=char0 -mon 
>> chardev=char0,mode=control -display none -audio
>> none -run-with exit-with-parent=on -accel kvm -accel tcg -machine 
>> pc-q35-11.1, -name target,debug-threads=on -machine 
>> memory-backend=mig.mem -object 
>> memory-backend-ram,id=mig.mem,size=150M,share=off -serial
>>   file:/tmp/migration-test-GF36R3/dest_serial -incoming defer -drive 
>> if=none,id=d0,file=/tmp/migration-test-GF36R3/bootsect,format=raw 
>> -device ide-hd,drive=d0,secs=1,cyls=1,heads=1 -snapshot 2>/dev/null  -a
>> ccel qtest
>> Memory content inconsistency at ea2000 first_byte = 53 last_byte = 53 
>> current = 54 hit_edge = 0
>> Memory content inconsistency at ea3000 first_byte = 53 last_byte = 53 
>> current = 54 hit_edge = 0
>> Memory content inconsistency at ea4000 first_byte = 53 last_byte = 53 
>> current = 54 hit_edge = 0
>> Memory content inconsistency at ea5000 first_byte = 53 last_byte = 53 
>> current = 54 hit_edge = 0
>> Memory content inconsistency at ea6000 first_byte = 53 last_byte = 53 
>> current = 54 hit_edge = 0
>> Memory content inconsistency at ea7000 first_byte = 53 last_byte = 53 
>> current = 54 hit_edge = 0
>> Memory content inconsistency at ea8000 first_byte = 53 last_byte = 53 
>> current = 54 hit_edge = 0
>> Memory content inconsistency at ea9000 first_byte = 53 last_byte = 53 
>> current = 54 hit_edge = 0
>> Memory content inconsistency at eaa000 first_byte = 53 last_byte = 53 
>> current = 54 hit_edge = 0
>> Memory content inconsistency at eab000 first_byte = 53 last_byte = 53 
>> current = 54 hit_edge = 0
>> and in another 13488 pages**
>> ERROR:../tests/qtest/migration/framework.c:190:check_guests_ram: 
>> assertion failed: (bad == 0)
>> not ok /x86_64/migration/colo/plain/secondary_failover - 
>> ERROR:../tests/qtest/migration/framework.c:190:check_guests_ram: 
>> assertion failed: (bad == 0)
>> Bail out!
>>
>> [3]
>>
>> diff --git a/migration/colo.c b/migration/colo.c
>> index 2d36f933cf..5c486c09ef 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -409,6 +409,7 @@ static int 
>> colo_do_checkpoint_transaction(MigrationState *s,
>>                                             QEMUFile *fb)
>>   {
>>       Error *local_err = NULL;
>> +    MigPendingData pending = {};
>>       int ret = -1;
>>
>>       colo_send_message(s->to_dst_file, COLO_MESSAGE_CHECKPOINT_REQUEST,
>> @@ -465,6 +466,18 @@ static int 
>> colo_do_checkpoint_transaction(MigrationState *s,
>>       if (migrate_auto_converge()) {
>>           mig_throttle_counter_reset();
>>       }
>> +
>> +    /*
>> +     * Run the final pending query so migration modules can flush 
>> their dirty
>> +     * state (e.g., RAM syncs its dirty bitmap) before this 
>> checkpoint's live
>> +     * state is saved. Unlike a regular switchover, COLO reaches 
>> completion
>> +     * repeatedly for every checkpoint, so this must be done on each 
>> one.
>> +     */
>> +    if (!qemu_savevm_query_pending_final(s, &pending, &local_err)) {
>> +        bql_unlock();
>> +        goto out;
>> +    }
>> +
>>       /*
>>        * Only save VM's live state, which not including device state.
>>        * TODO: We may need a timeout mechanism to prevent COLO process
>>
>> [4]
>>
>> QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test 
>> -k --full -r /x86_64/migration/colo
>> [...]
>> # Running /x86_64/migration/colo/plain/secondary_failover
>> # Using machine type: pc-q35-11.1
>> # starting QEMU: exec ./qemu-system-x86_64 -qtest 
>> unix:/tmp/qtest-1417324.sock -qtest-log /dev/null -chardev 
>> socket,path=/tmp/qtest-1417324.qmp,id=char0 -mon 
>> chardev=char0,mode=control -display none -audio
>> none -run-with exit-with-parent=on -accel kvm -accel tcg -machine 
>> pc-q35-11.1, -name source,debug-threads=on -machine 
>> memory-backend=mig.mem -object 
>> memory-backend-ram,id=mig.mem,size=150M,share=off -serial
>>   file:/tmp/migration-test-0TMNR3/src_serial -drive 
>> if=none,id=d0,file=/tmp/migration-test-0TMNR3/bootsect,format=raw 
>> -device ide-hd,drive=d0,secs=1,cyls=1,heads=1 -snapshot 2>/dev/null 
>> -accel qtest
>> # starting QEMU: exec ./qemu-system-x86_64 -qtest 
>> unix:/tmp/qtest-1417324.sock -qtest-log /dev/null -chardev 
>> socket,path=/tmp/qtest-1417324.qmp,id=char0 -mon 
>> chardev=char0,mode=control -display none -audio
>> none -run-with exit-with-parent=on -accel kvm -accel tcg -machine 
>> pc-q35-11.1, -name target,debug-threads=on -machine 
>> memory-backend=mig.mem -object 
>> memory-backend-ram,id=mig.mem,size=150M,share=off -serial
>>   file:/tmp/migration-test-0TMNR3/dest_serial -incoming defer -drive 
>> if=none,id=d0,file=/tmp/migration-test-0TMNR3/bootsect,format=raw 
>> -device ide-hd,drive=d0,secs=1,cyls=1,heads=1 -snapshot 2>/dev/null  -a
>> ccel qtest
>> ok 2 /x86_64/migration/colo/plain/secondary_failover
>
>
> The culprit appears to be patch 3:
>
>   migration: Run final save_query_pending at switchover
>
> I can reproduce the issue consistently with:
>
> while [ 1 ]; do
>     QTEST_QEMU_BINARY=./build/qemu-system-s390x \
>         ./build/tests/qtest/migration-test \
>         --full -p 
> /s390x/migration/colo/plain/secondary_failover_checkpoint || break
> done
>
> The patch you proposed seems to fix the issue.
>
> Could you send it as a follow-up patch? 

To make sure I get it straight, you suggest to merge your pull request 
and then send a separate fix patch for that with a proper Fixes tag, right?

> if Peter, Fabiano, Lukas
> would prefer it to be integrated in the appropriate place, it
> could be included in v5.
>
>
> Thanks,
>
> C.
>