Re: [PATCH QEMU HCI-8.1 v6 24/34] 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:
- [Medium] migration/cpr: cpr_is_in_progress() false on cpr-transfer source mid-migration
--

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

migration/cpr: add cpr_is_in_progress()

This adds cpr_is_in_progress(), a helper meant to report whether a CPR
migration is actually going on. It returns true on the target until the
incoming migration is done. On the source it returns true when the 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();

[ ... ]

> +    /*
> +     * 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: Medium]
The commit message says the helper is true "on the source only while the
outgoing one runs", and the comment says the state is open "from
cpr_state_save() to cpr_state_close()". Does this hold for cpr-transfer?

On a cpr-transfer source, qmp_migrate() only registers a HUP watch after
cpr_state_save():

migration/migration.c:qmp_migrate() {
    ...
    if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
        cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr);
    } else {
        qmp_migrate_finish(addr, errp);
    ...
}

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

migration/migration.c:qmp_migrate_finish_cb() {
    ...
    qmp_migrate_finish(addr, NULL);

    cpr_state_close();
    ...
}

cpr_state_close() sets cpr_state_file to NULL. From then until
migration_cleanup(), the source has cpr_is_incoming() false and
cpr_state_file == NULL. Can cpr_is_in_progress() then return false for the
whole of precopy, the vm_stop at completion, the VM state change handlers
and the device save?

The helper would be true on a cpr-transfer source only between the end of
cpr_state_save() and the target's HUP. Nothing has been migrated and the
VM is still running during that window.

Callers such as VM state change handlers and savevm .is_active hooks would
take the non-CPR path on a cpr-transfer source. That includes the VFIO
callbacks later in the series: vfio_is_active(), vfio_vmstate_change(),
vfio_vmstate_change_prepare(), vfio_switchover_ack_needed() and
vfio_save_state().

Only cpr-exec keeps cpr_state_file open until migration_cleanup(), so the
two CPR modes behave differently. I did not find a later patch in the
series that changes this.

Also, cpr_state_file is assigned only at the end of cpr_state_save(), so
the helper returns false while cpr_state_save() is still running. Is that
intended?

Could the source-side check use a state that lasts for the whole outgoing
migration, rather than cpr_state_file?

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