| Message ID | 20170516111123.17692-3-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
Juan Quintela <quintela@redhat.com> writes: > Create one capability for block migration and one parameter for > incremental block migration. > > Signed-off-by: Juan Quintela <quintela@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.
Markus Armbruster <armbru@redhat.com> wrote: > Juan Quintela <quintela@redhat.com> writes: > >> Create one capability for block migration and one parameter for >> incremental block migration. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> >> /* The last error that occurred */ >> Error *error; >> + /* Do we have to clean up -b/-i from old migrate paramteres */ > > parameters ok. >> +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. ok. >> + 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. This was Dave suggestion. The reason for doing this is in the following patch when we enable compile disable of block migration to put the ifdef in a single place. Otherwise, I have to put it in two places. >> +static void migrate_set_block_incremental(MigrationState *s, bool value) >> +{ >> + s->parameters.block_incremental = value; >> +} > > This is how setting one bit should look like. See previous comment. > >> + >> +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. setting to false can't never fail. It is when we set it to true that it can fail because it is compiled out (that will be in next patch). >> @@ -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". ok. > >> + 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. Being bug compatible has this drawbacks. I also hate that -i implies -b. >> --- 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? "enable migration of block devices. The granularity is all devices or no devices, nothing in the middle." I will be happy with suggestions. > >> # 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? This is good, I will try. "block incremental migration assumes that we have a base image in both sides, and then we continue writting in one of the sides. This way we need to only migrate the changes since the previous state where it was the same in both sides". I am not sure what to put there, really. > >> # 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. :-( And it is still not good enough. There are parameters that make sense to change in middle migration (max-bandwidth for example) and others that you shouldn't, like tls-hostname or compression-threads. Go figure. Thanks, Juan.
Juan Quintela <quintela@redhat.com> writes: > Markus Armbruster <armbru@redhat.com> wrote: >> Juan Quintela <quintela@redhat.com> writes: >> >>> Create one capability for block migration and one parameter for >>> incremental block migration. >>> >>> Signed-off-by: Juan Quintela <quintela@redhat.com> > >>> >>> /* The last error that occurred */ >>> Error *error; >>> + /* Do we have to clean up -b/-i from old migrate paramteres */ >> >> parameters > > ok. > > >>> +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. > > ok. > > >>> + 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. > > This was Dave suggestion. The reason for doing this is in the following > patch when we enable compile disable of block migration to put the ifdef > in a single place. Otherwise, I have to put it in two places. > >>> +static void migrate_set_block_incremental(MigrationState *s, bool value) >>> +{ >>> + s->parameters.block_incremental = value; >>> +} >> >> This is how setting one bit should look like. > > See previous comment. > >> >>> + >>> +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. > > setting to false can't never fail. It is when we set it to true that it > can fail because it is compiled out (that will be in next patch). Sounds like a job for &error_abort to me. >>> @@ -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". > > ok. > >> >>> + 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. > > Being bug compatible has this drawbacks. I also hate that -i implies -b. > >>> --- 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? > > "enable migration of block devices. The granularity is all devices or > no devices, nothing in the middle." > > I will be happy with suggestions. # @block: If enabled, QEMU will also migrate the contents of all block # devices. Default is disabled. A possible alternative are # mirror jobs to a builtin NBD server on the destination, which # are more flexible. # (Since 2.10) >>> # 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? > > This is good, I will try. > > "block incremental migration assumes that we have a base image in both > sides, and then we continue writting in one of the sides. This way we > need to only migrate the changes since the previous state where it was > the same in both sides". > > I am not sure what to put there, really. Well, to suggest something, I'd first have to figure out WTF incremental block migration does. Your text helps me some, but not enough. What exactly is being migrated, and what exactly is assumed to be shared between source and destination? Block migration is scandalously underdocumented. >>> # 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. > > :-( > > And it is still not good enough. There are parameters that make sense > to change in middle migration (max-bandwidth for example) and others > that you shouldn't, like tls-hostname or compression-threads. > > Go figure. Grown, not designed. It happens.
On 05/16/2017 11:00 AM, Markus Armbruster wrote: >>>> # periodic mode. (Since 2.8) >>>> # >>>> +# @block-incremental: enable block incremental migration (Since 2.10) >>>> +# >>> >>> What's "block incremental migration" and why should I care? >> >> This is good, I will try. >> >> "block incremental migration assumes that we have a base image in both >> sides, and then we continue writting in one of the sides. This way we >> need to only migrate the changes since the previous state where it was >> the same in both sides". >> >> I am not sure what to put there, really. > > Well, to suggest something, I'd first have to figure out WTF incremental > block migration does. Your text helps me some, but not enough. What > exactly is being migrated, and what exactly is assumed to be shared > between source and destination? > > Block migration is scandalously underdocumented. If I have: base <- active on the source, then: block migration without incremental creates: active on the destination (the entire disk contents are migrated). Conversely, block migration WITH incremental assumes that I have pre-created 'base' on the destination (easy to do, since base is read-only so it can be copied prior to starting the migration), and the migration results in: base <- active on the destination, where only the contents of active were transferred by qemu.
Eric Blake <eblake@redhat.com> writes: > On 05/16/2017 11:00 AM, Markus Armbruster wrote: > >>>>> # periodic mode. (Since 2.8) >>>>> # >>>>> +# @block-incremental: enable block incremental migration (Since 2.10) >>>>> +# >>>> >>>> What's "block incremental migration" and why should I care? >>> >>> This is good, I will try. >>> >>> "block incremental migration assumes that we have a base image in both >>> sides, and then we continue writting in one of the sides. This way we >>> need to only migrate the changes since the previous state where it was >>> the same in both sides". >>> >>> I am not sure what to put there, really. >> >> Well, to suggest something, I'd first have to figure out WTF incremental >> block migration does. Your text helps me some, but not enough. What >> exactly is being migrated, and what exactly is assumed to be shared >> between source and destination? >> >> Block migration is scandalously underdocumented. > > If I have: > > base <- active > > on the source, then: > > block migration without incremental creates: > > active > > on the destination (the entire disk contents are migrated). Conversely, > block migration WITH incremental assumes that I have pre-created 'base' > on the destination (easy to do, since base is read-only so it can be > copied prior to starting the migration), and the migration results in: > > base <- active > > on the destination, where only the contents of active were transferred > by qemu. Can you draft a documentation comment for @block-incremental?
On 05/16/2017 11:42 AM, Markus Armbruster wrote: >>> Well, to suggest something, I'd first have to figure out WTF incremental >>> block migration does. Your text helps me some, but not enough. What >>> exactly is being migrated, and what exactly is assumed to be shared >>> between source and destination? >>> >>> Block migration is scandalously underdocumented. >> > Can you draft a documentation comment for @block-incremental? How about: @block-incremental: Affects how much storage is migrated when the block migration capability is enabled. When false, the entire storage backing chain is migrated into a flattened image at the destination; when true, only the active qcow2 layer is migrated and the destination must already have access to the same backing chain as was used on the source. (since 2.10)
Eric Blake <eblake@redhat.com> writes: > On 05/16/2017 11:42 AM, Markus Armbruster wrote: > >>>> Well, to suggest something, I'd first have to figure out WTF incremental >>>> block migration does. Your text helps me some, but not enough. What >>>> exactly is being migrated, and what exactly is assumed to be shared >>>> between source and destination? >>>> >>>> Block migration is scandalously underdocumented. >>> > >> Can you draft a documentation comment for @block-incremental? > > How about: > > @block-incremental: Affects how much storage is migrated when the block > migration capability is enabled. When false, the entire storage backing > chain is migrated into a flattened image at the destination; when true, > only the active qcow2 layer is migrated and the destination must already > have access to the same backing chain as was used on the source. > (since 2.10) Works for me, thanks!
Eric Blake <eblake@redhat.com> wrote: > On 05/16/2017 11:42 AM, Markus Armbruster wrote: > >>>> Well, to suggest something, I'd first have to figure out WTF incremental >>>> block migration does. Your text helps me some, but not enough. What >>>> exactly is being migrated, and what exactly is assumed to be shared >>>> between source and destination? >>>> >>>> Block migration is scandalously underdocumented. >>> > >> Can you draft a documentation comment for @block-incremental? > > How about: > > @block-incremental: Affects how much storage is migrated when the block > migration capability is enabled. When false, the entire storage backing > chain is migrated into a flattened image at the destination; when true, > only the active qcow2 layer is migrated and the destination must already > have access to the same backing chain as was used on the source. > (since 2.10) Changed. Thanks.
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 */ + /* 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)); + cap->value->capability = MIGRATION_CAPABILITY_BLOCK; + cap->value->state = value; + qmp_migrate_set_capabilities(cap, errp); + qapi_free_MigrationCapabilityStatusList(cap); +} + +static void migrate_set_block_incremental(MigrationState *s, bool value) +{ + s->parameters.block_incremental = value; +} + +static void block_cleanup_parameters(MigrationState *s) +{ + if (s->must_remove_block) { + migrate_set_block_enabled(false, NULL); + 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"); + 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"); + 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) +# # 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) +# # 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:
Create one capability for block migration and one parameter for incremental block migration. Signed-off-by: Juan Quintela <quintela@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(-)