Re: [PATCH QEMU HCI-8.1 v5 24/36] migration/cpr: add cpr_is_in_progress()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] migration/cpr: cpr_is_in_progress() false on cpr-transfer source after early cpr_state_close()
--

Commit 453b7d2ec8b4fa272ccd4231894792f7b5e92c8a
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

migration/cpr: add cpr_is_in_progress()

This commit adds cpr_is_in_progress(). It returns true on the target until the incoming migration is done. On the source it returns true only while the outgoing migration runs, which it detects by checking whether cpr_state_file is open.

> diff --git a/migration/cpr.c b/migration/cpr.c
> index a149e790c103..3ea5d86bab48 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c

[ ... ]

> +bool cpr_is_in_progress(void)
> +{
> +    /* The incoming mode on the target, the 'mode' parameter on the source */
> +    MigMode mode = migrate_mode();
> +
> +    if (mode != MIG_MODE_CPR_TRANSFER && mode != MIG_MODE_CPR_EXEC) {
> +        return false;
> +    }
> +
> +    /* Target: the incoming mode is set until the incoming migration is done */
> +    if (cpr_is_incoming()) {
> +        return true;
> +    }
> +
> +    /*
> +     * Source: the parameter alone means nothing, it may just have been set.
> +     * An outgoing CPR migration has the CPR state open, from
> +     * cpr_state_save() to cpr_state_close().
> +     */
> +    return cpr_state_file != NULL;

[Severity: High]
Can this return false for most of an outgoing cpr-transfer migration?
The commit message says the function is true "on the source only while
the outgoing one runs", but cpr_state_file does not appear to stay open
that long in cpr-transfer mode.

In qmp_migrate(), cpr-transfer defers the real migration to a HUP
callback:

    if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
        cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr);

When the target closes the CPR socket, qmp_migrate_finish_cb() starts the
outgoing migration and then closes the CPR state right away:

    qmp_migrate_finish(addr, NULL);

    cpr_state_close();

cpr_state_close() does qemu_fclose(cpr_state_file) and sets
cpr_state_file to NULL.

From then on, for the migration thread, precopy, the vm_stop state change
handlers and save_complete, the predicate evaluates as follows:

    migrate_mode()      == MIG_MODE_CPR_TRANSFER
    cpr_is_incoming()   == false  (source)
    cpr_state_file      == NULL
    => cpr_is_in_progress() returns false

In cpr-exec mode there is no HUP callback. The file stays open until
migration_cleanup() calls cpr_state_close(), so the result is correct
there. The two CPR modes therefore give different answers.

There is a second window where it returns false. cpr_state_file is
assigned only at the end of cpr_state_save():

    cpr_state_file = f;
    return true;

So the predicate is also false while the CPR state is still being
written.

Later patches in this series call cpr_is_in_progress() from
vfio_vmstate_change(), vfio_is_active(), vfio_save_state() and
vfio_listener_log_global_start(). For example, vfio_vmstate_change() has:

    /* CPR source only: the device keeps running across the exec */
    if (cpr_is_in_progress()) {
        return;
    }

On a cpr-transfer source this check sees false. The VM state change
handler would then stop the device, save and send its state, and start
dirty page tracking. The series is meant to avoid exactly that.

No later patch in the series changes cpr_is_in_progress(),
cpr_state_close() or the cpr_state_file lifetime. The series also removes
the "CPR is prohibited: passthrough (VFIO) devices present" blocker, so
this path becomes reachable with cpr-transfer.

Should the source-side check use something that lasts for the whole
outgoing migration, rather than cpr_state_file? Alternatively, could the
cpr_state_close() call in qmp_migrate_finish_cb() be moved so that it
matches the lifetime the commit message describes?

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