Re: [PATCH QEMU HCI-8.1 v6 07/34] migration: don't refuse a blocker over a migration of another mode

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: is_busy() infers running migration mode from mutable migrate_mode()
--

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

migration: don't refuse a blocker over a migration of another mode

This patch changes is_busy() in migration/migration.c so that a running
migration only causes a migration blocker to be refused if the blocker
applies to the running migration's mode. Snapshots (RUN_STATE_SAVE_VM)
are still checked as before.

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

[ ... ]

> -static bool is_busy(Error **reasonp, Error **errp)
> +static bool is_busy(Error **reasonp, Error **errp, int modes)
>  {
>      ERRP_GUARD();
>  
> -    /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
> -    if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
> +    /*
> +     * Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too.
> +     * A running migration only matters if the blocker applies to its mode.
> +     */
> +    if (runstate_check(RUN_STATE_SAVE_VM) ||
> +        (migration_is_running() && (modes & BIT(migrate_mode())))) {

[Severity: Low]
Can this test be wrong while a migration is in flight? is_busy() infers
the mode of the running migration from migrate_mode(), which is not a
value recorded when the migration started:

migration/options.c:migrate_mode() {
    MigMode mode = cpr_get_incoming_mode();

    if (mode == MIG_MODE_NONE) {
        mode = migrate_get_current()->parameters.mode;
    }
    ...
}

The parameters.mode value can be changed at any time through
qmp_migrate_set_parameters()->migrate_params_apply():

    if (params->has_mode) {
        s->parameters.mode = params->mode;
    }

Unlike qmp_migrate_set_capabilities(), which rejects changes with
"There's a migration process in progress" when migration_is_running() is
true, I could not find a similar guard on the parameters path.

Suppose a normal migration is running and mode is then set to
cpr-transfer. A blocker for MIG_MODE_NORMAL only would compute
BIT(MIG_MODE_NORMAL) & BIT(MIG_MODE_CPR_TRANSFER) == 0 and be accepted
while the normal migration is still running. The reverse case would
wrongly refuse a blocker for the newly set mode.

Also, migrate_mode() returns cpr_get_incoming_mode() when it is set. That
describes the incoming migration, whereas migration_is_running() checks the
outgoing current_migration state. Can the two describe different
migrations here?

Would it be better to record the mode in MigrationState in migrate_init()
(or migrate_prepare()) and test that value in is_busy(), or to reject mode
changes while migration_is_running()?

>          error_propagate_prepend(errp, *reasonp,
>                                  "disallowing migration blocker "
>                                  "(migration/snapshot in progress) for: ");

[ ... ]

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