Re: [PATCH QEMU HCI-8.1 v7 25/35] 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 HUP closes cpr_state_file
--

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

migration/cpr: add cpr_is_in_progress()

This 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 CPR migration runs, which it detects by checking whether
cpr_state_file is open.

> diff --git a/migration/cpr.c b/migration/cpr.c
> index a149e790c1035..3ea5d86bab483 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c

[ ... ]

> @@ -549,6 +549,28 @@ bool cpr_exec_is_incoming(void)
>      return incoming_mode == MIG_MODE_CPR_EXEC;
>  }
>  
> +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]
Is cpr_state_file a reliable signal for "the outgoing migration is
running" in cpr-transfer mode? The commit message says the function is
true on the source "only while the outgoing one runs", but the comment
ties it to the lifetime of the CPR state file instead.

In cpr-transfer mode, qmp_migrate() calls cpr_state_save(), which sets
cpr_state_file, and then adds a HUP watch:

    cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr)

When the target closes its socket after loading the CPR state, the
callback runs:

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

    cpr_state_close();
}

cpr_state_close() calls qemu_fclose() and sets cpr_state_file to NULL.
qmp_migrate_finish() only starts the outgoing connection. The migration
thread, RAM transfer, vm_stop and the device state save all run after
this point.

From then on, on a cpr-transfer source, migrate_mode() is still
MIG_MODE_CPR_TRANSFER and cpr_is_incoming() is false. cpr_state_file is
NULL, so cpr_is_in_progress() returns false for most of the outgoing
migration.

Would callers that depend on this then take the normal live-migration
path? In the later VFIO changes in hw/vfio/migration.c:

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

    vfio_is_active():
        return !cpr_is_in_progress();

If the guard does not trigger at vm_stop time, the preserved VFIO device
could be put into STOP or STOP_COPY. vfio_is_active() would also return
true, so the VFIO device state would be saved and migrated.

cpr-exec appears unaffected, since it keeps the file open until
migration_cleanup(). Could the source-side check use a state that lasts
until the outgoing migration finishes, for example the migration state,
instead of cpr_state_file?

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