Re: [PATCH v0 2/4] migration: add background snapshot capability

[Peter Xu]

On Wed, Jul 22, 2020 at 11:11:31AM +0300, Denis Plotnikov wrote:
> diff --git a/migration/migration.c b/migration/migration.c
> index 2ed9923227..2ec0451abe 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1086,6 +1086,32 @@ static bool migrate_caps_check(bool *cap_list,
>              error_setg(errp, "Postcopy is not compatible with 
> ignore-shared");
>              return false;
>          }
> +
> +        if (cap_list[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
> +            error_setg(errp, "Postcopy is not compatible "
> +                        "with background snapshot");
> +            return false;
> +        }
> +    }
> +
> +    if (cap_list[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
> +        if (cap_list[MIGRATION_CAPABILITY_RELEASE_RAM]) {
> +            error_setg(errp, "Background snapshot is not compatible "
> +                        "with release ram capability");
> +            return false;
> +        }
> +
> +        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
> +            error_setg(errp, "Background snapshot is not "
> +                        "currently compatible with compression");
> +            return false;
> +        }
> +
> +        if (cap_list[MIGRATION_CAPABILITY_XBZRLE]) {
> +            error_setg(errp, "Background snapshot is not "
> +                        "currently compatible with XBZLRE");
> +            return false;
> +        }

Are these four the only ones that is not compatible with background snapshot?
I'm looking at:

typedef enum MigrationCapability {
    MIGRATION_CAPABILITY_XBZRLE,
    MIGRATION_CAPABILITY_RDMA_PIN_ALL,
    MIGRATION_CAPABILITY_AUTO_CONVERGE,
    MIGRATION_CAPABILITY_ZERO_BLOCKS,
    MIGRATION_CAPABILITY_COMPRESS,
    MIGRATION_CAPABILITY_EVENTS,
    MIGRATION_CAPABILITY_POSTCOPY_RAM,
    MIGRATION_CAPABILITY_X_COLO,
    MIGRATION_CAPABILITY_RELEASE_RAM,
    MIGRATION_CAPABILITY_BLOCK,
    MIGRATION_CAPABILITY_RETURN_PATH,
    MIGRATION_CAPABILITY_PAUSE_BEFORE_SWITCHOVER,
    MIGRATION_CAPABILITY_MULTIFD,
    MIGRATION_CAPABILITY_DIRTY_BITMAPS,
    MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME,
    MIGRATION_CAPABILITY_LATE_BLOCK_ACTIVATE,
    MIGRATION_CAPABILITY_X_IGNORE_SHARED,
    MIGRATION_CAPABILITY_VALIDATE_UUID,
    MIGRATION_CAPABILITY__MAX,
} MigrationCapability;

My gut feeling is that most of them is not compatible with it... If background
snapshot is majorly used on its own, not sure whether it's worth it to create a
new qmp command, rather than reusing the "migrate" command.  The thing is it
could be confusing when people noticed when all the parameters won't work again
with snapshots.

Btw, it does not mean we need to duplicate the code.  We should still be able
to leverage most of the codes in qmp_migrate(), maybe even call qmp_migrate()
inside a new qmp_snapshot().

Thoughts?..

-- 
Peter Xu