Patchwork [v2,3/6] block: change block-job-set-speed argument from 'value' to 'speed'

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

Comments

Stefan Hajnoczi - April 24, 2012, 1:53 p.m.
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c          |    6 +++---
 block/stream.c   |    8 ++++----
 block_int.h      |    4 ++--
 blockdev.c       |    4 ++--
 hmp-commands.hx  |    4 ++--
 qapi-schema.json |    4 ++--
 qmp-commands.hx  |    2 +-
 7 files changed, 16 insertions(+), 16 deletions(-)
Eric Blake - April 24, 2012, 3:03 p.m.
On 04/24/2012 07:53 AM, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.c          |    6 +++---
>  block/stream.c   |    8 ++++----
>  block_int.h      |    4 ++--
>  blockdev.c       |    4 ++--
>  hmp-commands.hx  |    4 ++--
>  qapi-schema.json |    4 ++--
>  qmp-commands.hx  |    2 +-
>  7 files changed, 16 insertions(+), 16 deletions(-)
> 

>  
> -static void stream_set_speed(BlockJob *job, int64_t value, Error **errp)
> +static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp)
>  {
>      StreamBlockJob *s = container_of(job, StreamBlockJob, common);
>  
> -    if (value < 0) {
> -        error_set(errp, QERR_INVALID_PARAMETER, "value");
> +    if (speed < 0) {
> +        error_set(errp, QERR_INVALID_PARAMETER, "speed");
>          return;
>      }
> -    ratelimit_set_speed(&s->limit, value / BDRV_SECTOR_SIZE);
> +    ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE);

Unrelated to this patch, but as long as I'm noticing it:

What happens if speed is < BDRV_SECTOR_SIZE?  Should we be rounding this
division up, rather than truncating downwards?  On the other hand,
libvirt always passes speed in 1MiB multiples, so unless you have a 2
megabyte sector size, libvirt won't trigger the original question.

> @@ -79,7 +79,7 @@ typedef struct BlockJobType {
>      const char *job_type;
>  
>      /** Optional callback for job types that support setting a speed limit */
> -    void (*set_speed)(BlockJob *job, int64_t value, Error **errp);
> +    void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);

As long as we are touching the interface to get it right...

Would it be any simpler to convert speed to uint64_t and not have to
deal with invalid negative speed values?

> +++ b/hmp-commands.hx
> @@ -85,8 +85,8 @@ ETEXI
>  
>      {
>          .name       = "block_job_set_speed",
> -        .args_type  = "device:B,value:o",
> -        .params     = "device value",
> +        .args_type  = "device:B,speed:o",
> +        .params     = "device speed",

For that matter, can the :o type _ever_ usefully allow negative values,
or is it always an unsigned calculation?
Stefan Hajnoczi - April 24, 2012, 4:02 p.m.
On Tue, Apr 24, 2012 at 4:03 PM, Eric Blake <eblake@redhat.com> wrote:
> On 04/24/2012 07:53 AM, Stefan Hajnoczi wrote:
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> ---
>>  block.c          |    6 +++---
>>  block/stream.c   |    8 ++++----
>>  block_int.h      |    4 ++--
>>  blockdev.c       |    4 ++--
>>  hmp-commands.hx  |    4 ++--
>>  qapi-schema.json |    4 ++--
>>  qmp-commands.hx  |    2 +-
>>  7 files changed, 16 insertions(+), 16 deletions(-)
>>
>
>>
>> -static void stream_set_speed(BlockJob *job, int64_t value, Error **errp)
>> +static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp)
>>  {
>>      StreamBlockJob *s = container_of(job, StreamBlockJob, common);
>>
>> -    if (value < 0) {
>> -        error_set(errp, QERR_INVALID_PARAMETER, "value");
>> +    if (speed < 0) {
>> +        error_set(errp, QERR_INVALID_PARAMETER, "speed");
>>          return;
>>      }
>> -    ratelimit_set_speed(&s->limit, value / BDRV_SECTOR_SIZE);
>> +    ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE);
>
> Unrelated to this patch, but as long as I'm noticing it:
>
> What happens if speed is < BDRV_SECTOR_SIZE?  Should we be rounding this
> division up, rather than truncating downwards?  On the other hand,
> libvirt always passes speed in 1MiB multiples, so unless you have a 2
> megabyte sector size, libvirt won't trigger the original question.

I'm not sure if this matters.  The real problem with throttling is
that users may set a limit so low that no work can ever be done -
thereby pausing the job completely (you could call that a feature).

>> @@ -79,7 +79,7 @@ typedef struct BlockJobType {
>>      const char *job_type;
>>
>>      /** Optional callback for job types that support setting a speed limit */
>> -    void (*set_speed)(BlockJob *job, int64_t value, Error **errp);
>> +    void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
>
> As long as we are touching the interface to get it right...
>
> Would it be any simpler to convert speed to uint64_t and not have to
> deal with invalid negative speed values?
>
>> +++ b/hmp-commands.hx
>> @@ -85,8 +85,8 @@ ETEXI
>>
>>      {
>>          .name       = "block_job_set_speed",
>> -        .args_type  = "device:B,value:o",
>> -        .params     = "device value",
>> +        .args_type  = "device:B,speed:o",
>> +        .params     = "device speed",
>
> For that matter, can the :o type _ever_ usefully allow negative values,
> or is it always an unsigned calculation?

These are both QEMU block layer quirks - we use int64_t instead of
uint64_t for byte offsets.  Using uint64_t in one place tends to
collide with int64_t values from another place (signed/unsigned
comparison, for example) so I'm sticking to int64_t.

Stefan
Luiz Capitulino - April 24, 2012, 7:02 p.m.
On Tue, 24 Apr 2012 09:03:06 -0600
Eric Blake <eblake@redhat.com> wrote:

> > +++ b/hmp-commands.hx
> > @@ -85,8 +85,8 @@ ETEXI
> >  
> >      {
> >          .name       = "block_job_set_speed",
> > -        .args_type  = "device:B,value:o",
> > -        .params     = "device value",
> > +        .args_type  = "device:B,speed:o",
> > +        .params     = "device speed",
> 
> For that matter, can the :o type _ever_ usefully allow negative values,
> or is it always an unsigned calculation?

It does allow for negative values. As an example, migrate_set_speed rounds
the value to zero if it's negative.

Patch

diff --git a/block.c b/block.c
index dc02736..1ab6e52 100644
--- a/block.c
+++ b/block.c
@@ -4114,7 +4114,7 @@  void block_job_complete(BlockJob *job, int ret)
     bdrv_set_in_use(bs, 0);
 }
 
-void block_job_set_speed(BlockJob *job, int64_t value, Error **errp)
+void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 {
     Error *local_err = NULL;
 
@@ -4122,13 +4122,13 @@  void block_job_set_speed(BlockJob *job, int64_t value, Error **errp)
         error_set(errp, QERR_NOT_SUPPORTED);
         return;
     }
-    job->job_type->set_speed(job, value, &local_err);
+    job->job_type->set_speed(job, speed, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
         return;
     }
 
-    job->speed = value;
+    job->speed = speed;
 }
 
 void block_job_cancel(BlockJob *job)
diff --git a/block/stream.c b/block/stream.c
index 06bc70a..b66242a 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -263,15 +263,15 @@  retry:
     block_job_complete(&s->common, ret);
 }
 
-static void stream_set_speed(BlockJob *job, int64_t value, Error **errp)
+static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common);
 
-    if (value < 0) {
-        error_set(errp, QERR_INVALID_PARAMETER, "value");
+    if (speed < 0) {
+        error_set(errp, QERR_INVALID_PARAMETER, "speed");
         return;
     }
-    ratelimit_set_speed(&s->limit, value / BDRV_SECTOR_SIZE);
+    ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE);
 }
 
 static BlockJobType stream_job_type = {
diff --git a/block_int.h b/block_int.h
index f8e6592..9d8bebf 100644
--- a/block_int.h
+++ b/block_int.h
@@ -79,7 +79,7 @@  typedef struct BlockJobType {
     const char *job_type;
 
     /** Optional callback for job types that support setting a speed limit */
-    void (*set_speed)(BlockJob *job, int64_t value, Error **errp);
+    void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
 } BlockJobType;
 
 /**
@@ -380,7 +380,7 @@  void block_job_complete(BlockJob *job, int ret);
  * Set a rate-limiting parameter for the job; the actual meaning may
  * vary depending on the job type.
  */
-void block_job_set_speed(BlockJob *job, int64_t value, Error **errp);
+void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
 
 /**
  * block_job_cancel:
diff --git a/blockdev.c b/blockdev.c
index 7073330..80b62c3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1136,7 +1136,7 @@  static BlockJob *find_block_job(const char *device)
     return bs->job;
 }
 
-void qmp_block_job_set_speed(const char *device, int64_t value, Error **errp)
+void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
 {
     BlockJob *job = find_block_job(device);
 
@@ -1145,7 +1145,7 @@  void qmp_block_job_set_speed(const char *device, int64_t value, Error **errp)
         return;
     }
 
-    block_job_set_speed(job, value, errp);
+    block_job_set_speed(job, speed, errp);
 }
 
 void qmp_block_job_cancel(const char *device, Error **errp)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 461fa59..8a929f0 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -85,8 +85,8 @@  ETEXI
 
     {
         .name       = "block_job_set_speed",
-        .args_type  = "device:B,value:o",
-        .params     = "device value",
+        .args_type  = "device:B,speed:o",
+        .params     = "device speed",
         .help       = "set maximum speed for a background block operation",
         .mhandler.cmd = hmp_block_job_set_speed,
     },
diff --git a/qapi-schema.json b/qapi-schema.json
index 49f1e16..d56fcb6 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1592,7 +1592,7 @@ 
 #
 # @device: the device name
 #
-# @value:  the maximum speed, in bytes per second
+# @speed:  the maximum speed, in bytes per second
 #
 # Returns: Nothing on success
 #          If the job type does not support throttling, NotSupported
@@ -1602,7 +1602,7 @@ 
 # Since: 1.1
 ##
 { 'command': 'block-job-set-speed',
-  'data': { 'device': 'str', 'value': 'int' } }
+  'data': { 'device': 'str', 'speed': 'int' } }
 
 ##
 # @block-job-cancel:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index f972332..b07ed59 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -694,7 +694,7 @@  EQMP
 
     {
         .name       = "block-job-set-speed",
-        .args_type  = "device:B,value:o",
+        .args_type  = "device:B,speed:o",
         .mhandler.cmd_new = qmp_marshal_input_block_job_set_speed,
     },