Re: [PATCH QEMU HCI-8.1 v7 35/35] 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/migration: stop-copy size dropped from pending, so downtime-limit is bypassed
- [Low] vfio: comment in vfio_state_pending_exact() misdescribes error fallback
--

commit 4741edc5da3eca96f6669a6d5a3ce9d95caac4ae
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 VFIO stop-copy size is added to *must_precopy only when a query failed or the device still has pre-copy data. Devices without pre-copy support, and pre-copy devices that have drained, no longer have their stop-copy state counted toward the switchover decision.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- 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.
> +     * If getting the pending sizes fails, VFIO_MIG_STOP_COPY_SIZE is
> +     * reported, unconditionally, so the switchover isn't taken blindly.
>       */
> -    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);
> +    }

[Severity: Low]
This isn't a bug, but is the new comment accurate?

It says that if getting the pending sizes fails, VFIO_MIG_STOP_COPY_SIZE is reported unconditionally. That only holds when vfio_query_stop_copy_size() itself fails, because only then does stop_copy_size keep its initial value.

If vfio_query_stop_copy_size() succeeds and vfio_query_precopy_size() then fails, ret is non-zero. The real stop_copy_length value is added to *must_precopy, not VFIO_MIG_STOP_COPY_SIZE.

vfio_query_precopy_size() also zeroes precopy_init_size and precopy_dirty_size in that case, so the trace event shows 0 for both. Could the comment be reworded to match this?

> +
> +    /*
> +     * 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]
Does this gate let the switchover happen while a large stop-copy state still has to be sent with the guest paused?

The switchover decision in migration_iteration_run() depends only on the reported pending size:

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

With stop_copy_size left out of *must_precopy, this can become true even when the device state is much larger than bandwidth x downtime-limit. The commit message acknowledges that "the downtime limit no longer bounds the time that state takes to send".

Two cases make the gate false:

1) A device without VFIO_MIGRATION_PRE_COPY never reaches vfio_query_precopy_size(), because vfio_device_state_is_precopy() is false. precopy_init_size and precopy_dirty_size stay 0, so its stop-copy state is never counted, however large it is. vfio_state_pending_estimate() also returns early for such a device:

    if (!vfio_device_state_is_precopy(vbasedev)) {
        return;
    }

so no other handler accounts for that state.

2) A pre-copy device whose query returns 0/0 at a single sampling instant is treated as drained, although the guest is still running. Can stop_copy_length grow after that sample? The comment "can't shrink by iterating" says nothing about whether the size is final once 0/0 is read.

There is no per-device or per-driver opt-in here, so every VFIO device is affected, not only the vGPU case described in the commit message. Would it make sense to limit this behaviour to devices known to need it, or to make it configurable?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-vfio_v1-v7-0-4467fdccbbe6@virtuozzo.com?part=35