Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru> wrote:
> We don't allow to use x-colo capability when replication is not
> configured. So, no reason to build COLO when replication is disabled,
> it's unusable in this case.
>
> Note also that the check in migrate_caps_check() is not the only
> restriction: some functions in migration/colo.c will just abort if
> called with not defined CONFIG_REPLICATION, for example:
>
> migration_iteration_finish()
> case MIGRATION_STATUS_COLO:
> migrate_start_colo_process()
> colo_process_checkpoint()
> abort()
>
> It could probably make sense to have possibility to enable COLO without
> REPLICATION, but this requires deeper audit of colo & replication code,
> which may be done later if needed.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
Nice patch. Thanks.
> @@ -68,7 +66,6 @@ static bool colo_runstate_is_stopped(void)
> static void secondary_vm_do_failover(void)
> {
> /* COLO needs enable block-replication */
> -#ifdef CONFIG_REPLICATION
> int old_state;
> MigrationIncomingState *mis = migration_incoming_get_current();
> Error *local_err = NULL;
> @@ -133,14 +130,10 @@ static void secondary_vm_do_failover(void)
> if (mis->migration_incoming_co) {
> qemu_coroutine_enter(mis->migration_incoming_co);
> }
> -#else
> - abort();
> -#endif
> }
With only this chunks you have proved that your argument is right.
abort() is never a solution.
> diff --git a/migration/options.c b/migration/options.c
> index 912cbadddb..eef2bd0f16 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -171,7 +171,9 @@ Property migration_properties[] = {
> DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
> DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
> MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
> +#ifdef CONFIG_REPLICATION
> DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
> +#endif
> DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
> DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
> DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
> @@ -209,9 +211,13 @@ bool migrate_block(void)
> bool migrate_colo(void)
> {
> +#ifdef CONFIG_REPLICATION
> MigrationState *s = migrate_get_current();
>
> return s->capabilities[MIGRATION_CAPABILITY_X_COLO];
> +#else
> + return false;
> +#endif
> }
#ifdef 1
>
> bool migrate_compress(void)
> @@ -401,7 +407,9 @@
> INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
> MIGRATION_CAPABILITY_RDMA_PIN_ALL,
> MIGRATION_CAPABILITY_COMPRESS,
> MIGRATION_CAPABILITY_XBZRLE,
> +#ifdef CONFIG_REPLICATION
> MIGRATION_CAPABILITY_X_COLO,
> +#endif
> MIGRATION_CAPABILITY_VALIDATE_UUID,
> MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>
#ifdef 2
> @@ -428,15 +436,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps,
> Error **errp)
> }
> #endif
>
> -#ifndef CONFIG_REPLICATION
> - if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
> - error_setg(errp, "QEMU compiled without replication module"
> - " can't enable COLO");
> - error_append_hint(errp, "Please enable replication before COLO.\n");
> - return false;
> - }
> -#endif
> -
> if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
> /* This check is reasonably expensive, so only when it's being
> * set the first time, also it's only the destination that needs
I would preffer if you removed #ifdef 1 and 2 and let this one in. I am
trying to get all capabilities to this format.
> diff --git a/stubs/colo.c b/stubs/colo.c
> new file mode 100644
> index 0000000000..f306ab45d6
> --- /dev/null
> +++ b/stubs/colo.c
> @@ -0,0 +1,37 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +#include "migration/colo.h"
> +#include "migration/migration.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
> +
> +void colo_shutdown(void)
> +{
> + abort();
> +}
This is wrong, it should be empty.
void migration_shutdown(void)
{
/*
* When the QEMU main thread exit, the COLO thread
* may wait a semaphore. So, we should wakeup the
* COLO thread before migration shutdown.
*/
colo_shutdown();
......
}
> +void *colo_process_incoming_thread(void *opaque)
> +{
> + abort();
> +}
At least print an error message?
> +void colo_checkpoint_notify(void *opaque)
> +{
> + abort();
> +}
Another error message.
It is independently of this patch, but I am thinking about changing the
interface and doing something like this in options.c
changing
if (params->has_x_checkpoint_delay) {
s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
if (migration_in_colo_state()) {
colo_checkpoint_notify(s);
}
}
To
if (params->has_x_checkpoint_delay) {
s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
colo_checkpoint_refresh(s);
}
That way we can convert it to an empty function.
> +void migrate_start_colo_process(MigrationState *s)
> +{
> + abort();
> +}
Another case of changing the function interface?
case MIGRATION_STATUS_COLO:
if (!migrate_colo()) {
error_report("%s: critical error: calling COLO code without "
"COLO enabled", __func__);
}
migrate_start_colo_process(s);
The changes of functions interfaces are independent of this patch.
Later, Juan.
