Re: [Qemu-devel] [PATCH v2 3/6] block: change block-job-set-speed argument from 'value' to 'speed'

[Stefan Hajnoczi]

On Tue, Apr 24, 2012 at 4:03 PM, Eric Blake <address@hidden> wrote:
> On 04/24/2012 07:53 AM, Stefan Hajnoczi wrote:
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> ---
>>  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