Juan Quintela <quint...@redhat.com> writes: > Create one capability for block migration and one parameter for > incremental block migration. > > Signed-off-by: Juan Quintela <quint...@redhat.com> > --- > hmp.c | 13 +++++++ > include/migration/block.h | 2 + > include/migration/migration.h | 7 ++++ > migration/migration.c | 88 > +++++++++++++++++++++++++++++++++++++++++++ > qapi-schema.json | 14 +++++-- > 5 files changed, 121 insertions(+), 3 deletions(-) > > diff --git a/hmp.c b/hmp.c > index acbbc5c..208154c 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -326,6 +326,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const > QDict *qdict) > monitor_printf(mon, "%s: %" PRId64 "\n", > > MigrationParameter_lookup[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY], > params->x_checkpoint_delay); > + assert(params->has_block_incremental); > + monitor_printf(mon, "%s: %s\n", > + MigrationParameter_lookup[MIGRATION_PARAMETER_BLOCK_INCREMENTAL], > + params->block_incremental ? "on" : "off"); > } > > qapi_free_MigrationParameters(params); > @@ -1527,6 +1531,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const > QDict *qdict) > Visitor *v = string_input_visitor_new(valuestr); > uint64_t valuebw = 0; > int64_t valueint = 0; > + bool valuebool = false; > Error *err = NULL; > bool use_int_value = false; > int i, ret; > @@ -1581,6 +1586,14 @@ void hmp_migrate_set_parameter(Monitor *mon, const > QDict *qdict) > p.has_x_checkpoint_delay = true; > use_int_value = true; > break; > + case MIGRATION_PARAMETER_BLOCK_INCREMENTAL: > + p.has_block_incremental = true; > + visit_type_bool(v, param, &valuebool, &err); > + if (err) { > + goto cleanup; > + } > + p.block_incremental = valuebool; > + break; > } > > if (use_int_value) { > diff --git a/include/migration/block.h b/include/migration/block.h > index 41a1ac8..5225af9 100644 > --- a/include/migration/block.h > +++ b/include/migration/block.h > @@ -20,4 +20,6 @@ uint64_t blk_mig_bytes_transferred(void); > uint64_t blk_mig_bytes_remaining(void); > uint64_t blk_mig_bytes_total(void); > > +void migrate_set_block_enabled(bool value, Error **errp); > + > #endif /* MIGRATION_BLOCK_H */ > diff --git a/include/migration/migration.h b/include/migration/migration.h > index 47262bd..adcb8f6 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -179,6 +179,10 @@ struct MigrationState > > /* The last error that occurred */ > Error *error; > + /* Do we have to clean up -b/-i from old migrate paramteres */
parameters > + /* This feature is deprecated and will be removed */ > + bool must_remove_block; > + bool must_remove_block_incremental; > }; > > void migrate_set_state(int *state, int old_state, int new_state); > @@ -293,6 +297,9 @@ bool migrate_colo_enabled(void); > > int64_t xbzrle_cache_resize(int64_t new_size); > > +bool migrate_use_block(void); > +bool migrate_use_block_incremental(void); > + > bool migrate_use_compression(void); > int migrate_compress_level(void); > int migrate_compress_threads(void); > diff --git a/migration/migration.c b/migration/migration.c > index 5c92851..03f96df 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -599,6 +599,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error > **errp) > params->downtime_limit = s->parameters.downtime_limit; > params->has_x_checkpoint_delay = true; > params->x_checkpoint_delay = s->parameters.x_checkpoint_delay; > + params->has_block_incremental = true; > + params->block_incremental = s->parameters.block_incremental; > > return params; > } > @@ -907,6 +909,9 @@ void qmp_migrate_set_parameters(MigrationParameters > *params, Error **errp) > colo_checkpoint_notify(s); > } > } > + if (params->has_block_incremental) { > + s->parameters.block_incremental = params->block_incremental; > + } > } > > > @@ -942,6 +947,35 @@ void migrate_set_state(int *state, int old_state, int > new_state) > } > } > > +void migrate_set_block_enabled(bool value, Error **errp) > +{ > + MigrationCapabilityStatusList *cap; > + > + cap = g_malloc0(sizeof(*cap)); > + cap->value = g_malloc(sizeof(*cap->value)); I prefer g_new() for extra type checking. Your choice. > + cap->value->capability = MIGRATION_CAPABILITY_BLOCK; > + cap->value->state = value; > + qmp_migrate_set_capabilities(cap, errp); > + qapi_free_MigrationCapabilityStatusList(cap); > +} The objective is to set one bit. The means is building a MigrationCapabilityStatusList. When you have to build an elaborate data structure just so you can set one bit, your interfaces are likely deficient. Observation, not change request. > + > +static void migrate_set_block_incremental(MigrationState *s, bool value) > +{ > + s->parameters.block_incremental = value; > +} This is how setting one bit should look like. > + > +static void block_cleanup_parameters(MigrationState *s) > +{ > + if (s->must_remove_block) { > + migrate_set_block_enabled(false, NULL); NULL ignores errors. If an error happens, and we ignore it, we're screwed, aren't we? I suspect we want &error_abort here. > + s->must_remove_block = false; > + } > + if (s->must_remove_block_incremental) { > + migrate_set_block_incremental(s, false); > + s->must_remove_block_incremental = false; > + } > +} > + > static void migrate_fd_cleanup(void *opaque) > { > MigrationState *s = opaque; > @@ -974,6 +1008,7 @@ static void migrate_fd_cleanup(void *opaque) > } > > notifier_list_notify(&migration_state_notifiers, s); > + block_cleanup_parameters(s); > } > > void migrate_fd_error(MigrationState *s, const Error *error) > @@ -986,6 +1021,7 @@ void migrate_fd_error(MigrationState *s, const Error > *error) > s->error = error_copy(error); > } > notifier_list_notify(&migration_state_notifiers, s); > + block_cleanup_parameters(s); > } > > static void migrate_fd_cancel(MigrationState *s) > @@ -1027,6 +1063,7 @@ static void migrate_fd_cancel(MigrationState *s) > s->block_inactive = false; > } > } > + block_cleanup_parameters(s); > } > > void add_migration_state_change_notifier(Notifier *notify) > @@ -1209,6 +1246,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool > blk, > Error *local_err = NULL; > MigrationState *s = migrate_get_current(); > MigrationParams params; > + bool block_enabled = false; > const char *p; > > params.blk = has_blk && blk; > @@ -1229,6 +1267,38 @@ void qmp_migrate(const char *uri, bool has_blk, bool > blk, > return; > } > > + if (has_blk && blk) { > + if (migrate_use_block() || migrate_use_block_incremental()) { > + error_setg(errp, "You can't use block capability and -b/i"); Error message assumes HMP. In QMP, the thing is called 'blk', not -b. Perhaps we can sidestep the issue like this: "Command options are incompatible with current migration capabilities". > + return; > + } > + migrate_set_block_enabled(true, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + block_enabled = true; > + s->must_remove_block = true; > + } > + > + if (has_inc && inc) { > + if ((migrate_use_block() && !block_enabled) > + || migrate_use_block_incremental()) { > + error_setg(errp, "You can't use block capability and -b/i"); Likewise. The condition would be slightly simpler if you completed error checking before changing global state. Matter of taste. > + return; > + } > + if (!block_enabled) { > + migrate_set_block_enabled(true, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + s->must_remove_block = true; > + } > + migrate_set_block_incremental(s, true); > + s->must_remove_block_incremental = true; > + } > + > s = migrate_init(¶ms); > > if (strstart(uri, "tcp:", &p)) { > @@ -1426,6 +1496,24 @@ int64_t migrate_xbzrle_cache_size(void) > return s->xbzrle_cache_size; > } > > +bool migrate_use_block(void) > +{ > + MigrationState *s; > + > + s = migrate_get_current(); > + > + return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK]; > +} > + > +bool migrate_use_block_incremental(void) > +{ > + MigrationState *s; > + > + s = migrate_get_current(); > + > + return s->parameters.block_incremental; > +} > + > /* migration thread support */ > /* > * Something bad happened to the RP stream, mark an error > diff --git a/qapi-schema.json b/qapi-schema.json > index 5728b7f..62246bc 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -894,11 +894,14 @@ > # @release-ram: if enabled, qemu will free the migrated ram pages on the > source > # during postcopy-ram migration. (since 2.9) > # > +# @block: enable block migration (Since 2.10) > +# What's "block migration" and why should I care? > # Since: 1.2 > ## > { 'enum': 'MigrationCapability', > 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', > - 'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] } > + 'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram', > + 'block' ] } > > ## > # @MigrationCapabilityStatus: > @@ -1009,13 +1012,15 @@ > # @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints in > # periodic mode. (Since 2.8) > # > +# @block-incremental: enable block incremental migration (Since 2.10) > +# What's "block incremental migration" and why should I care? > # Since: 2.4 > ## > { 'enum': 'MigrationParameter', > 'data': ['compress-level', 'compress-threads', 'decompress-threads', > 'cpu-throttle-initial', 'cpu-throttle-increment', > 'tls-creds', 'tls-hostname', 'max-bandwidth', > - 'downtime-limit', 'x-checkpoint-delay' ] } > + 'downtime-limit', 'x-checkpoint-delay', 'block-incremental' ] } > > ## > # @migrate-set-parameters: > @@ -1082,6 +1087,8 @@ > # > # @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since > 2.8) > # > +# @block-incremental: enable block incremental migration (Since 2.10) > +# > # Since: 2.4 > ## > { 'struct': 'MigrationParameters', > @@ -1094,7 +1101,8 @@ > '*tls-hostname': 'str', > '*max-bandwidth': 'int', > '*downtime-limit': 'int', > - '*x-checkpoint-delay': 'int'} } > + '*x-checkpoint-delay': 'int', > + '*block-incremental': 'bool' } } > > ## > # @query-migrate-parameters: Can't say I like the split between MigrationCapability and MigrationParameters, but we got to work with what we have.