Re: [PATCH QEMU HCI-8.1 v6 34/34] VZ: vfio/migration: don't wait for stop-copy-only device state #VSTOR-141821

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfio: stop-copy size dropped from pending data, bypassing the downtime limit
- [Medium] vfio/migration: stop-copy size dropped for devices that never enter PRE_COPY
--

commit e935c30e55885d702d35a8da4365fcb9090511cd
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

VZ: vfio/migration: don't wait for stop-copy-only device state #VSTOR-141821

This patch changes vfio_state_pending_exact() so that the device's stop-copy
size is added to must_precopy only when a query failed or pre-copy data
(initial or dirty) is still pending. Once pre-copy has drained, the
stop-copy size no longer holds up the switchover decision.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 04d0de7cef0a..eff1172569d3 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> @@ -569,16 +569,24 @@ static void vfio_state_pending_exact(void *opaque, uint64_t *must_precopy,
>      VFIODevice *vbasedev = opaque;
>      VFIOMigration *migration = vbasedev->migration;
>      uint64_t stop_copy_size = VFIO_MIG_STOP_COPY_SIZE;
> +    int ret;
>  
>      /*
>       * If getting pending migration size fails, VFIO_MIG_STOP_COPY_SIZE is
>       * reported so downtime limit won't be violated.
>       */

[Severity: Medium]
Is this comment still accurate? On the success path the stop-copy size is
now deliberately left out of must_precopy, so the downtime limit is no
longer protected for it. Only the error fallback still reports
VFIO_MIG_STOP_COPY_SIZE.

> -    vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
> -    *must_precopy += stop_copy_size;
> +    ret = vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
>  
>      if (vfio_device_state_is_precopy(vbasedev)) {
> -        vfio_query_precopy_size(migration);
> +        ret |= vfio_query_precopy_size(migration);
> +    }
> +
> +    /*
> +     * Once there's nothing left to pre-copy, the stop-copy state can't
> +     * shrink by iterating: don't let it hold up the switchover.
> +     */
> +    if (ret || migration->precopy_init_size || migration->precopy_dirty_size) {
> +        *must_precopy += stop_copy_size;
>      }

[Severity: Medium]
The commit message says the stop-copy size is reported "only while the
device still has something to pre-copy". Can this condition tell a drained
PRE_COPY device apart from a device that never pre-copies?

If the device lacks VFIO_MIGRATION_PRE_COPY, vfio_save_setup() never moves
it to PRE_COPY. In that case vfio_device_state_is_precopy() is false and
vfio_query_precopy_size() is never called:

    if (vfio_device_state_is_precopy(vbasedev)) {
        ret |= vfio_query_precopy_size(migration);
    }

precopy_init_size and precopy_dirty_size then stay at 0. They are zeroed
in vfio_save_cleanup() and are only decremented elsewhere. If
vfio_query_stop_copy_size() succeeds, ret is 0, so this is the result:

    if (ret || migration->precopy_init_size || migration->precopy_dirty_size)

The condition is false and stop_copy_size is not added to *must_precopy.
vfio_state_pending_estimate() also returns early when
!vfio_device_state_is_precopy(), so nothing else accounts for the device's
stop-copy data.

Can migration_iteration_run() then reach this check with the device blob
excluded?

    complete_ready = can_switchover && (pending_size <= s->threshold_size);

If so, the full stop-copy state would be sent after the guest is stopped,
outside the configured downtime limit. Before the patch, the downtime limit
applied to such devices. Should the omission be gated on
vfio_device_state_is_precopy() (or vfio_precopy_supported()) together with
successful queries?

[Severity: Medium]
Even for a PRE_COPY device, can the stop-copy size be dropped too early?
vfio_update_estimated_pending_data() zeroes both counters whenever
vfio_save_block() returns 0 bytes:

    migration->precopy_init_size = 0;
    migration->precopy_dirty_size = 0;

vfio_query_precopy_size() can also report 0/0 transiently. Either case
satisfies the "drained" condition, and nothing re-checks the stop-copy size
afterwards.

The change is also not opt-in. There is no property or capability gate, so
every VFIO migration user now gets this behaviour. Was Nova's stepwise
downtime-limit raise meant to be bypassed for all devices, and not only for
the vGPU case described in the commit message?

>  
>      trace_vfio_state_pending_exact(vbasedev->name, *must_precopy, *can_postcopy,
>                                     stop_copy_size, migration->precopy_init_size,
>                                     migration->precopy_dirty_size);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=34