Patchwork [3/4] block: add 'speed' optional parameter to block-stream

login
register
mail settings
Submitter Stefan Hajnoczi
Date April 23, 2012, 3:39 p.m.
Message ID <1335195589-13285-4-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/154473/
State New
Headers show

Comments

Stefan Hajnoczi - April 23, 2012, 3:39 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(-)
Paolo Bonzini - April 23, 2012, 3:51 p.m.
Il 23/04/2012 17:39, Stefan Hajnoczi ha scritto:
> +    /* 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;
> +        }
> +    }

Similarly, here I would instead modify the QAPI entry point, and add a
call to qmp_block_job_set_speed there.

In patch 2, qmp_block_job_set_speed can parse the return code to
distinguish ENOTSUP from EINVAL.

Paolo
Luiz Capitulino - April 24, 2012, 7:31 p.m.
On Mon, 23 Apr 2012 16:39:48 +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 7056d8c..e3c1483 100644
> --- a/block.c
> +++ b/block.c
> @@ -4072,8 +4072,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;
>  
> @@ -4089,6 +4089,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) {

Missed this small detail. Ideally, you should test against has_speed, but
I think that there are only two possibilities for a false negativehere:
1. the client/user expects speed=0 to "work" 2. 'speed' is (probably
incorrectly) initialized to some value != 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 f0486a3..dc15fb6 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; /* bs must already be in use */
>      }
> diff --git a/block_int.h b/block_int.h
> index 6015c27..bffca35 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -343,6 +343,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.
> @@ -357,8 +358,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:
> @@ -417,6 +418,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.
> @@ -428,7 +430,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 c484259..f18af16 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;
> @@ -1110,7 +1111,8 @@ void qmp_block_stream(const char *device, bool has_base,
>          }
>      }
>  
> -    stream_start(bs, base_bs, base, block_stream_cb, bs, errp);
> +    stream_start(bs, base_bs, base, has_speed ? speed : 0,
> +                 block_stream_cb, bs, errp);
>      if (error_is_set(errp)) {
>          return;
>      }
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index a6f5a84..66b70e1 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 d0d4f693..9950451 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 f972332..a5ed6d0 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:56 a.m.
On Tue, Apr 24, 2012 at 8:31 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Mon, 23 Apr 2012 16:39:48 +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 7056d8c..e3c1483 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4072,8 +4072,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;
>>
>> @@ -4089,6 +4089,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) {
>
> Missed this small detail. Ideally, you should test against has_speed, but
> I think that there are only two possibilities for a false negativehere:
> 1. the client/user expects speed=0 to "work" 2. 'speed' is (probably
> incorrectly) initialized to some value != 0.

By the time we get here we've already checked has_speed and set
speed=0 when has_speed=false.  (The qmp marshaller generated code does
indeed leave the int64_t uninitialized.)

Stefan
Luiz Capitulino - April 25, 2012, 1:19 p.m.
On Wed, 25 Apr 2012 11:56:51 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Tue, Apr 24, 2012 at 8:31 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > On Mon, 23 Apr 2012 16:39:48 +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 7056d8c..e3c1483 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -4072,8 +4072,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;
> >>
> >> @@ -4089,6 +4089,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) {
> >
> > Missed this small detail. Ideally, you should test against has_speed, but
> > I think that there are only two possibilities for a false negativehere:
> > 1. the client/user expects speed=0 to "work" 2. 'speed' is (probably
> > incorrectly) initialized to some value != 0.
> 
> By the time we get here we've already checked has_speed and set
> speed=0 when has_speed=false. 

Ah, true, but speed=0 is just ignored. In practice doesn't matter, in
theory would be good to be consistent.

> (The qmp marshaller generated code does
> indeed leave the int64_t uninitialized.)

I didn't know that. We could change that, but maybe it's a good idea to
force people to rely on the has_ bool.

Patch

diff --git a/block.c b/block.c
index 7056d8c..e3c1483 100644
--- a/block.c
+++ b/block.c
@@ -4072,8 +4072,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;
 
@@ -4089,6 +4089,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 f0486a3..dc15fb6 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; /* bs must already be in use */
     }
diff --git a/block_int.h b/block_int.h
index 6015c27..bffca35 100644
--- a/block_int.h
+++ b/block_int.h
@@ -343,6 +343,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.
@@ -357,8 +358,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:
@@ -417,6 +418,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.
@@ -428,7 +430,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 c484259..f18af16 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;
@@ -1110,7 +1111,8 @@  void qmp_block_stream(const char *device, bool has_base,
         }
     }
 
-    stream_start(bs, base_bs, base, block_stream_cb, bs, errp);
+    stream_start(bs, base_bs, base, has_speed ? speed : 0,
+                 block_stream_cb, bs, errp);
     if (error_is_set(errp)) {
         return;
     }
diff --git a/hmp-commands.hx b/hmp-commands.hx
index a6f5a84..66b70e1 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 d0d4f693..9950451 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 f972332..a5ed6d0 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,
     },