Re: [PATCH QEMU HCI-8.1 v5 36/36] VZ: migration: don't wait for stop-copy-only device state #VSTOR-150595

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] vfio: vfio_state_pending_exact() ignores vfio_query_precopy_size() failure
- [Medium] migration: irreducible-state switchover bound exceeds downtime-limit and commit message
- [Medium] migration: irreducible_pending only learned after RAM estimate drops below plain threshold
- [Medium] migration: irreducible-data switchover overrides a pending migrate-start-postcopy request
- [Low] migration: irreducible size passed via file-static global in savevm.c
--

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

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

This patch lets state_pending_exact handlers report pending data that only
moves once the guest is stopped (the VFIO stop-copy size). When that data
alone exceeds the threshold, migration_iteration_run() switches over once
the rest is within the threshold plus as much again.

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -569,18 +569,25 @@ 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.
>       */
> -    vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
> +    ret = vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
>      *must_precopy += stop_copy_size;
>  
>      if (vfio_device_state_is_precopy(vbasedev)) {
>          vfio_query_precopy_size(migration);
>      }
>  
> +    /* Once there's nothing left to pre-copy, iterating can't shrink it */
> +    if (!ret && !migration->precopy_init_size &&
> +        !migration->precopy_dirty_size) {
> +        qemu_savevm_state_pending_add_irreducible(stop_copy_size);
> +    }

[Severity: Medium]
Can a failed precopy query be treated as "nothing left to pre-copy" here?

The return value of vfio_query_precopy_size() is dropped.  That function
zeroes both sizes before the ioctl and returns -errno on failure:

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

    if (ioctl(migration->data_fd, VFIO_MIG_GET_PRECOPY_INFO, &precopy)) {
        return -errno;
    }

So if VFIO_MIG_GET_PRECOPY_INFO fails while the device is in PRE_COPY or
PRE_COPY_P2P, the new test "!ret && !migration->precopy_init_size &&
!migration->precopy_dirty_size" still passes, since ret only comes from
vfio_query_stop_copy_size().  The whole stop-copy size is then reported
through qemu_savevm_state_pending_add_irreducible(), even though pre-copy
data may remain that iterating would drain.

The existing code fails closed when the stop-copy size query fails, but this
path fails open.  Should the return value of vfio_query_precopy_size() be
captured, and irreducible data reported only when it succeeded (or when the
device is not in a pre-copy state)?

The same logic also seems to treat a transient dirty_bytes == 0 on a
precopy-capable device as irreducible.

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -3576,8 +3577,10 @@ static MigIterateState migration_iteration_run(MigrationState *s)
>           * postcopy started, so ESTIMATE should always match with EXACT
>           * during postcopy phase.
>           */
> -        if (pending_size < s->threshold_size) {
> -            qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
> +        /* Allow for what the estimate lacks, see the irreducible rule below */
> +        if (pending_size < s->threshold_size + 2 * s->irreducible_pending) {
> +            s->irreducible_pending =
> +                qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);

[Severity: Medium]
Is irreducible_pending only learned after the RAM estimate has dropped
below the plain threshold_size?

irreducible_pending is set to 0 in migrate_init(), so the gate above is
the old "pending_size < s->threshold_size" until an exact query has run.
The pending_size here comes from qemu_savevm_state_pending_estimate().
vfio_state_pending_estimate() returns early when the device is not in a
precopy state, and otherwise reports only:

    *must_precopy +=
        migration->precopy_init_size + migration->precopy_dirty_size;

It never includes the stop-copy size.  So the estimate is roughly the RAM
dirty estimate from ram_state_pending_estimate():

    uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;

If RAM dirtying keeps that at or above threshold_size, the exact query is
never made and irreducible_pending stays 0.  The relaxed completion check
is guarded by "s->irreducible_pending > s->threshold_size", so it is never
evaluated.

Would a guest whose RAM remainder sits between threshold and threshold plus
the irreducible size, which is the case the commit message targets, then
keep iterating as before until the estimate happens to dip below the
threshold?

>              pending_size = must_precopy + can_postcopy;
>              trace_migrate_pending_exact(pending_size, must_precopy,
>                                          can_postcopy);

[ ... ]

>          /* Should we switch to postcopy now? */
>          if (must_precopy <= s->threshold_size &&
>              can_switchover && qatomic_read(&s->start_postcopy)) {

[Severity: Medium]
Can the new override below discard a pending migrate-start-postcopy
request?

must_precopy includes the VFIO stop-copy size, so when that size exceeds
threshold_size, the postcopy gate above is never satisfied and
postcopy_start() is skipped.

> @@ -3601,6 +3604,16 @@ static MigIterateState migration_iteration_run(MigrationState *s)
>           *     (which was calculated from expected downtime)
>           */
>          complete_ready = can_switchover && (pending_size <= s->threshold_size);
> +
> +        /*
> +         * Data which only moves once the guest is stopped can't shrink by
> +         * iterating: if it alone exceeds the threshold, switch over once
> +         * the rest is within the threshold plus as much again.
> +         */
> +        if (s->irreducible_pending > s->threshold_size) {
> +            complete_ready = can_switchover &&
> +                pending_size <= s->threshold_size + 2 * s->irreducible_pending;
> +        }

Before the patch, pending_size <= threshold_size implied must_precopy <=
threshold_size, so a requested postcopy always took precedence over precopy
completion.  The new override does not look at s->start_postcopy.  Once it
becomes true, migration_completion() runs with s->state == ACTIVE and takes
the migration_completion_precopy() branch, stopping the VM and sending the
whole device state during downtime.

Should this override be skipped when start_postcopy is set (or postcopy is
enabled)?

[Severity: Medium]
Does this bound match the commit message and the comment?

The commit message says "switch over once the rest is within the threshold
plus as much again", and the comment says the same.  But pending_size
already includes the irreducible bytes, since vfio_state_pending_exact()
does "*must_precopy += stop_copy_size" and then also calls
qemu_savevm_state_pending_add_irreducible(stop_copy_size).  So the
condition

    pending_size <= s->threshold_size + 2 * s->irreducible_pending

is equivalent to "rest <= threshold + irreducible", not roughly twice the
threshold.  For a ~2 GiB vGPU, around 2 GiB plus the threshold of RAM and
other state may still be pending at switchover, so about 6 GiB in total.

This replaces the bandwidth x downtime-limit criterion from
migration_update_counters():

    s->threshold_size = expected_bw_per_ms * migrate_downtime_limit();

with one that scales with the device-reported size.  s->expected_downtime
is not updated, there is no capability, parameter or compat property, and
the downtime-limit documentation does not mention it.  Is that intended,
and can the wording be made to match the actual bound?

> diff --git a/migration/savevm.c b/migration/savevm.c
> --- a/migration/savevm.c
> +++ b/migration/savevm.c

[ ... ]

> +/* Exact pending data which only moves once the guest is stopped */
> +static uint64_t savevm_pending_irreducible;
> +
> +void qemu_savevm_state_pending_add_irreducible(uint64_t size)
> +{
> +    savevm_pending_irreducible += size;
> +}

[Severity: Low]
This isn't a bug, but the irreducible amount is passed from device handlers
to the migration core through a file-static global.

It is reset only inside qemu_savevm_state_pending_exact().  Nothing checks
that an exact query is in progress, so a call from any other context would
be wiped by the next query or leak into it.  The state_pending_exact
documentation in include/migration/register.h was not updated to describe
the new contract either, and the global is not tied to MigrationState.

Would it be cleaner to extend the handler interface, or at least document
that the function may only be called from within a state_pending_exact
handler?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=36