Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Markus Armbruster]

Juan Quintela <address@hidden> writes:

> Create one capability for block migration and one parameter for
> incremental block migration.
>
> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  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(&params);
>  
>      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.