Patchwork [v2,4/6] block: add 'speed' optional parameter to block-stream

login
register
mail settings
Submitter Stefan Hajnoczi
Date April 24, 2012, 1:53 p.m.
Message ID <1335275640-3349-5-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/154678/
State New
Headers show

Comments

Stefan Hajnoczi - April 24, 2012, 1:53 p.m.
Allow streaming operations to be started with an initial speed limit.
This eliminates the window of time between starting streaming and
issuing block-job-set-speed.  Users should use the new optional 'speed'
parameter instead so that speed limits are in effect immediately when
the job starts.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c          |   18 ++++++++++++++++--
 block/stream.c   |    5 +++--
 block_int.h      |    9 ++++++---
 blockdev.c       |    6 ++++--
 hmp-commands.hx  |    4 ++--
 hmp.c            |    4 +++-
 qapi-schema.json |    6 +++++-
 qmp-commands.hx  |    2 +-
 8 files changed, 40 insertions(+), 14 deletions(-)
Eric Blake - April 24, 2012, 3:17 p.m.
On 04/24/2012 07:53 AM, Stefan Hajnoczi wrote:
> Allow streaming operations to be started with an initial speed limit.
> This eliminates the window of time between starting streaming and
> issuing block-job-set-speed.  Users should use the new optional 'speed'
> parameter instead so that speed limits are in effect immediately when
> the job starts.
> 

> +++ b/hmp-commands.hx
> @@ -71,8 +71,8 @@ ETEXI
>  
>      {
>          .name       = "block_stream",
> -        .args_type  = "device:B,base:s?",
> -        .params     = "device [base]",
> +        .args_type  = "device:B,speed:o?,base:s?",

Am I remembering correctly that the :o type lets me pass in suffixes,
including 'b' to force bytes, but defaults to 'M' if I don't pass a
suffix?  Should we be documenting the default unit and the ability to
scale as part of the command help (see migrate_set_speed for a similar
command that documents the scaling)?

> +        .params     = "device [speed] [base]",

By having two optional arguments in HMP, can I write 'block_stream
device base' and have things work, or am I now forced to provide a speed
if I want a base?  If the latter, wouldn't this look better as:

.params = "device [speed [base]]",

and if so, how many other HMP commands suffer from the same
documentation flaw?

Thankfully, libvirt doesn't care about HMP for this command (it will
always be using the QMP interface for any qemu new enough to support
block streaming), so I'm not really impacted by the answer to these
questions, other than satisfying my curiosity.  After all, one of the
benefits of QMP is that arguments are in name:value pairs, in part so
that I can provide just the parameters I want in the order I want,
without worrying about providing dummy arguments for any earlier parameters.
Stefan Hajnoczi - April 24, 2012, 3:44 p.m.
On Tue, Apr 24, 2012 at 4:17 PM, Eric Blake <eblake@redhat.com> wrote:
> On 04/24/2012 07:53 AM, Stefan Hajnoczi wrote:
>> Allow streaming operations to be started with an initial speed limit.
>> This eliminates the window of time between starting streaming and
>> issuing block-job-set-speed.  Users should use the new optional 'speed'
>> parameter instead so that speed limits are in effect immediately when
>> the job starts.
>>
>
>> +++ b/hmp-commands.hx
>> @@ -71,8 +71,8 @@ ETEXI
>>
>>      {
>>          .name       = "block_stream",
>> -        .args_type  = "device:B,base:s?",
>> -        .params     = "device [base]",
>> +        .args_type  = "device:B,speed:o?,base:s?",
>
> Am I remembering correctly that the :o type lets me pass in suffixes,
> including 'b' to force bytes, but defaults to 'M' if I don't pass a
> suffix?  Should we be documenting the default unit and the ability to
> scale as part of the command help (see migrate_set_speed for a similar
> command that documents the scaling)?

Yes, it supports suffixes.  The HMP documentation for these commands
does not include details about the parameters, so I didn't document it
- management tools should be using QMP and HMP is mainly for
debugging/ad-hoc testing.

>> +        .params     = "device [speed] [base]",
>
> By having two optional arguments in HMP, can I write 'block_stream
> device base' and have things work, or am I now forced to provide a speed
> if I want a base?  If the latter, wouldn't this look better as:
>
> .params = "device [speed [base]]",
>
> and if so, how many other HMP commands suffer from the same
> documentation flaw?

You are right.  The .params I gave are incorrect since you must give
'speed' if you want to give 'base'.

Stefan
Luiz Capitulino - April 24, 2012, 7:14 p.m.
On Tue, 24 Apr 2012 14:53:58 +0100
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:

> Allow streaming operations to be started with an initial speed limit.
> This eliminates the window of time between starting streaming and
> issuing block-job-set-speed.  Users should use the new optional 'speed'
> parameter instead so that speed limits are in effect immediately when
> the job starts.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.c          |   18 ++++++++++++++++--
>  block/stream.c   |    5 +++--
>  block_int.h      |    9 ++++++---
>  blockdev.c       |    6 ++++--
>  hmp-commands.hx  |    4 ++--
>  hmp.c            |    4 +++-
>  qapi-schema.json |    6 +++++-
>  qmp-commands.hx  |    2 +-
>  8 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1ab6e52..43c794c 100644
> --- a/block.c
> +++ b/block.c
> @@ -4083,8 +4083,8 @@ out:
>  }
>  
>  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
> -                       BlockDriverCompletionFunc *cb, void *opaque,
> -                       Error **errp)
> +                       int64_t speed, BlockDriverCompletionFunc *cb,
> +                       void *opaque, Error **errp)
>  {
>      BlockJob *job;
>  
> @@ -4100,6 +4100,20 @@ void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
>      job->cb            = cb;
>      job->opaque        = opaque;
>      bs->job = job;
> +
> +    /* Only set speed when necessary to avoid NotSupported error */
> +    if (speed != 0) {
> +        Error *local_err = NULL;
> +
> +        block_job_set_speed(job, speed, &local_err);
> +        if (error_is_set(&local_err)) {
> +            bs->job = NULL;
> +            g_free(job);
> +            bdrv_set_in_use(bs, 0);
> +            error_propagate(errp, local_err);
> +            return NULL;
> +        }
> +    }
>      return job;
>  }
>  
> diff --git a/block/stream.c b/block/stream.c
> index b66242a..6724af2 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -281,13 +281,14 @@ static BlockJobType stream_job_type = {
>  };
>  
>  void stream_start(BlockDriverState *bs, BlockDriverState *base,
> -                  const char *base_id, BlockDriverCompletionFunc *cb,
> +                  const char *base_id, int64_t speed,
> +                  BlockDriverCompletionFunc *cb,
>                    void *opaque, Error **errp)
>  {
>      StreamBlockJob *s;
>      Coroutine *co;
>  
> -    s = block_job_create(&stream_job_type, bs, cb, opaque, errp);
> +    s = block_job_create(&stream_job_type, bs, speed, cb, opaque, errp);
>      if (!s) {
>          return;
>      }
> diff --git a/block_int.h b/block_int.h
> index 9d8bebf..3824e54 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -344,6 +344,7 @@ int is_windows_drive(const char *filename);
>   * block_job_create:
>   * @job_type: The class object for the newly-created job.
>   * @bs: The block
> + * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
>   * @errp: A location to return DeviceInUse.
> @@ -358,8 +359,8 @@ int is_windows_drive(const char *filename);
>   * called from a wrapper that is specific to the job type.
>   */
>  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
> -                       BlockDriverCompletionFunc *cb, void *opaque,
> -                       Error **errp);
> +                       int64_t speed, BlockDriverCompletionFunc *cb,
> +                       void *opaque, Error **errp);
>  
>  /**
>   * block_job_complete:
> @@ -418,6 +419,7 @@ void block_job_cancel_sync(BlockJob *job);
>   * flatten the whole backing file chain onto @bs.
>   * @base_id: The file name that will be written to @bs as the new
>   * backing file if the job completes.  Ignored if @base is %NULL.
> + * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
>   * @errp: A location to return DeviceInUse.
> @@ -429,7 +431,8 @@ void block_job_cancel_sync(BlockJob *job);
>   * @base_id in the written image and to @base in the live BlockDriverState.
>   */
>  void stream_start(BlockDriverState *bs, BlockDriverState *base,
> -                  const char *base_id, BlockDriverCompletionFunc *cb,
> +                  const char *base_id, int64_t speed,
> +                  BlockDriverCompletionFunc *cb,
>                    void *opaque, Error **errp);
>  
>  #endif /* BLOCK_INT_H */
> diff --git a/blockdev.c b/blockdev.c
> index 80b62c3..d25ffea 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1091,7 +1091,8 @@ static void block_stream_cb(void *opaque, int ret)
>  }
>  
>  void qmp_block_stream(const char *device, bool has_base,
> -                      const char *base, Error **errp)
> +                      const char *base, bool has_speed,
> +                      int64_t speed, Error **errp)
>  {
>      BlockDriverState *bs;
>      BlockDriverState *base_bs = NULL;
> @@ -1111,7 +1112,8 @@ void qmp_block_stream(const char *device, bool has_base,
>          }
>      }
>  
> -    stream_start(bs, base_bs, base, block_stream_cb, bs, &local_err);
> +    stream_start(bs, base_bs, base, has_speed ? speed : 0,
> +                 block_stream_cb, bs, &local_err);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
>          return;
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8a929f0..6dfda45 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -71,8 +71,8 @@ ETEXI
>  
>      {
>          .name       = "block_stream",
> -        .args_type  = "device:B,base:s?",
> -        .params     = "device [base]",
> +        .args_type  = "device:B,speed:o?,base:s?",
> +        .params     = "device [speed] [base]",
>          .help       = "copy data from a backing file into a block device",
>          .mhandler.cmd = hmp_block_stream,
>      },
> diff --git a/hmp.c b/hmp.c
> index f3e5163..eb96618 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -835,8 +835,10 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
>      Error *error = NULL;
>      const char *device = qdict_get_str(qdict, "device");
>      const char *base = qdict_get_try_str(qdict, "base");
> +    int64_t speed = qdict_get_try_int(qdict, "speed", 0);
>  
> -    qmp_block_stream(device, base != NULL, base, &error);
> +    qmp_block_stream(device, base != NULL, base,
> +                     qdict_haskey(qdict, "speed"), speed, &error);
>  
>      hmp_handle_error(mon, &error);
>  }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d56fcb6..b1e349f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1571,15 +1571,19 @@
>  #
>  # @base:   #optional the common backing file name
>  #
> +# @speed:  #optional the maximum speed, in bytes per second
> +#
>  # Returns: Nothing on success
>  #          If streaming is already active on this device, DeviceInUse
>  #          If @device does not exist, DeviceNotFound
>  #          If image streaming is not supported by this device, NotSupported
>  #          If @base does not exist, BaseNotFound
> +#          If @speed is invalid, BlockJobSpeedInvalid

This should be InvalidParameter, right?

>  #
>  # Since: 1.1
>  ##
> -{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str' } }
> +{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str',
> +                                       '*speed': 'int' } }
>  
>  ##
>  # @block-job-set-speed:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b07ed59..c810c74 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -688,7 +688,7 @@ EQMP
>  
>      {
>          .name       = "block-stream",
> -        .args_type  = "device:B,base:s?",
> +        .args_type  = "device:B,base:s?,speed:o?",
>          .mhandler.cmd_new = qmp_marshal_input_block_stream,
>      },
>
Stefan Hajnoczi - April 25, 2012, 10:54 a.m.
On Tue, Apr 24, 2012 at 8:14 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>  # Returns: Nothing on success
>>  #          If streaming is already active on this device, DeviceInUse
>>  #          If @device does not exist, DeviceNotFound
>>  #          If image streaming is not supported by this device, NotSupported
>>  #          If @base does not exist, BaseNotFound
>> +#          If @speed is invalid, BlockJobSpeedInvalid
>
> This should be InvalidParameter, right?

You are right.  Thanks for pointing this out.

Stefan
Kevin Wolf - April 25, 2012, 11:16 a.m.
Am 24.04.2012 15:53, schrieb Stefan Hajnoczi:
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d56fcb6..b1e349f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1571,15 +1571,19 @@
>  #
>  # @base:   #optional the common backing file name
>  #
> +# @speed:  #optional the maximum speed, in bytes per second

Should mention that 0 means unlimited and is the default if the argument
is not specified.

That an explicit 0 (rather than leaving the argument out) is possible is
actually important for HMP, because there it's the only way to have a
base image, but no speed limit.

> +#
>  # Returns: Nothing on success
>  #          If streaming is already active on this device, DeviceInUse
>  #          If @device does not exist, DeviceNotFound
>  #          If image streaming is not supported by this device, NotSupported
>  #          If @base does not exist, BaseNotFound
> +#          If @speed is invalid, BlockJobSpeedInvalid
>  #
>  # Since: 1.1
>  ##
> -{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str' } }
> +{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str',
> +                                       '*speed': 'int' } }

Kevin

Patch

diff --git a/block.c b/block.c
index 1ab6e52..43c794c 100644
--- a/block.c
+++ b/block.c
@@ -4083,8 +4083,8 @@  out:
 }
 
 void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
-                       BlockDriverCompletionFunc *cb, void *opaque,
-                       Error **errp)
+                       int64_t speed, BlockDriverCompletionFunc *cb,
+                       void *opaque, Error **errp)
 {
     BlockJob *job;
 
@@ -4100,6 +4100,20 @@  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
     job->cb            = cb;
     job->opaque        = opaque;
     bs->job = job;
+
+    /* Only set speed when necessary to avoid NotSupported error */
+    if (speed != 0) {
+        Error *local_err = NULL;
+
+        block_job_set_speed(job, speed, &local_err);
+        if (error_is_set(&local_err)) {
+            bs->job = NULL;
+            g_free(job);
+            bdrv_set_in_use(bs, 0);
+            error_propagate(errp, local_err);
+            return NULL;
+        }
+    }
     return job;
 }
 
diff --git a/block/stream.c b/block/stream.c
index b66242a..6724af2 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -281,13 +281,14 @@  static BlockJobType stream_job_type = {
 };
 
 void stream_start(BlockDriverState *bs, BlockDriverState *base,
-                  const char *base_id, BlockDriverCompletionFunc *cb,
+                  const char *base_id, int64_t speed,
+                  BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp)
 {
     StreamBlockJob *s;
     Coroutine *co;
 
-    s = block_job_create(&stream_job_type, bs, cb, opaque, errp);
+    s = block_job_create(&stream_job_type, bs, speed, cb, opaque, errp);
     if (!s) {
         return;
     }
diff --git a/block_int.h b/block_int.h
index 9d8bebf..3824e54 100644
--- a/block_int.h
+++ b/block_int.h
@@ -344,6 +344,7 @@  int is_windows_drive(const char *filename);
  * block_job_create:
  * @job_type: The class object for the newly-created job.
  * @bs: The block
+ * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
  * @errp: A location to return DeviceInUse.
@@ -358,8 +359,8 @@  int is_windows_drive(const char *filename);
  * called from a wrapper that is specific to the job type.
  */
 void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
-                       BlockDriverCompletionFunc *cb, void *opaque,
-                       Error **errp);
+                       int64_t speed, BlockDriverCompletionFunc *cb,
+                       void *opaque, Error **errp);
 
 /**
  * block_job_complete:
@@ -418,6 +419,7 @@  void block_job_cancel_sync(BlockJob *job);
  * flatten the whole backing file chain onto @bs.
  * @base_id: The file name that will be written to @bs as the new
  * backing file if the job completes.  Ignored if @base is %NULL.
+ * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
  * @errp: A location to return DeviceInUse.
@@ -429,7 +431,8 @@  void block_job_cancel_sync(BlockJob *job);
  * @base_id in the written image and to @base in the live BlockDriverState.
  */
 void stream_start(BlockDriverState *bs, BlockDriverState *base,
-                  const char *base_id, BlockDriverCompletionFunc *cb,
+                  const char *base_id, int64_t speed,
+                  BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp);
 
 #endif /* BLOCK_INT_H */
diff --git a/blockdev.c b/blockdev.c
index 80b62c3..d25ffea 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1091,7 +1091,8 @@  static void block_stream_cb(void *opaque, int ret)
 }
 
 void qmp_block_stream(const char *device, bool has_base,
-                      const char *base, Error **errp)
+                      const char *base, bool has_speed,
+                      int64_t speed, Error **errp)
 {
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
@@ -1111,7 +1112,8 @@  void qmp_block_stream(const char *device, bool has_base,
         }
     }
 
-    stream_start(bs, base_bs, base, block_stream_cb, bs, &local_err);
+    stream_start(bs, base_bs, base, has_speed ? speed : 0,
+                 block_stream_cb, bs, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
         return;
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8a929f0..6dfda45 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -71,8 +71,8 @@  ETEXI
 
     {
         .name       = "block_stream",
-        .args_type  = "device:B,base:s?",
-        .params     = "device [base]",
+        .args_type  = "device:B,speed:o?,base:s?",
+        .params     = "device [speed] [base]",
         .help       = "copy data from a backing file into a block device",
         .mhandler.cmd = hmp_block_stream,
     },
diff --git a/hmp.c b/hmp.c
index f3e5163..eb96618 100644
--- a/hmp.c
+++ b/hmp.c
@@ -835,8 +835,10 @@  void hmp_block_stream(Monitor *mon, const QDict *qdict)
     Error *error = NULL;
     const char *device = qdict_get_str(qdict, "device");
     const char *base = qdict_get_try_str(qdict, "base");
+    int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
-    qmp_block_stream(device, base != NULL, base, &error);
+    qmp_block_stream(device, base != NULL, base,
+                     qdict_haskey(qdict, "speed"), speed, &error);
 
     hmp_handle_error(mon, &error);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index d56fcb6..b1e349f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1571,15 +1571,19 @@ 
 #
 # @base:   #optional the common backing file name
 #
+# @speed:  #optional the maximum speed, in bytes per second
+#
 # Returns: Nothing on success
 #          If streaming is already active on this device, DeviceInUse
 #          If @device does not exist, DeviceNotFound
 #          If image streaming is not supported by this device, NotSupported
 #          If @base does not exist, BaseNotFound
+#          If @speed is invalid, BlockJobSpeedInvalid
 #
 # Since: 1.1
 ##
-{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str' } }
+{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str',
+                                       '*speed': 'int' } }
 
 ##
 # @block-job-set-speed:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b07ed59..c810c74 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -688,7 +688,7 @@  EQMP
 
     {
         .name       = "block-stream",
-        .args_type  = "device:B,base:s?",
+        .args_type  = "device:B,base:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_stream,
     },