[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 3/6] block: change block-job-set-speed argume
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/6] block: change block-job-set-speed argument from 'value' to 'speed' |
Date: |
Tue, 24 Apr 2012 17:02:48 +0100 |
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
- [Qemu-devel] [PATCH v2 0/6] block: add optional 'speed' parameter to block-stream, Stefan Hajnoczi, 2012/04/24
- [Qemu-devel] [PATCH v2 4/6] block: add 'speed' optional parameter to block-stream, Stefan Hajnoczi, 2012/04/24
- [Qemu-devel] [PATCH v2 3/6] block: change block-job-set-speed argument from 'value' to 'speed', Stefan Hajnoczi, 2012/04/24
- [Qemu-devel] [PATCH v2 2/6] block: use Error mechanism instead of -errno for block_job_set_speed(), Stefan Hajnoczi, 2012/04/24
- [Qemu-devel] [PATCH v2 1/6] block: use Error mechanism instead of -errno for block_job_create(), Stefan Hajnoczi, 2012/04/24
- [Qemu-devel] [PATCH v2 5/6] qemu-iotests: add block-stream with invalid speed value test, Stefan Hajnoczi, 2012/04/24
- [Qemu-devel] [PATCH v2 6/6] qemu-iotests: fix missing 'result' variable assignment in 030, Stefan Hajnoczi, 2012/04/24