Re: [Qemu-devel] [PATCH v11 2/4] block: add I/O throttling algorithm

[Zhi Yong Wu]

On Wed, Nov 2, 2011 at 7:51 PM, Kevin Wolf <address@hidden> wrote:
> Am 02.11.2011 07:01, schrieb Zhi Yong Wu:
>> Signed-off-by: Zhi Yong Wu <address@hidden>
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> ---
>>  block.c               |  233 
>> +++++++++++++++++++++++++++++++++++++++++++++++--
>>  block.h               |    1 +
>>  block_int.h           |    1 +
>>  qemu-coroutine-lock.c |    8 ++
>>  qemu-coroutine.h      |    6 ++
>>  5 files changed, 242 insertions(+), 7 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index c70f86d..440e889 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -74,6 +74,13 @@ static BlockDriverAIOCB 
>> *bdrv_co_aio_rw_vector(BlockDriverState *bs,
>>                                                 bool is_write);
>>  static void coroutine_fn bdrv_co_do_rw(void *opaque);
>>
>> +static bool bdrv_exceed_bps_limits(BlockDriverState *bs, int nb_sectors,
>> +        bool is_write, double elapsed_time, uint64_t *wait);
>> +static bool bdrv_exceed_iops_limits(BlockDriverState *bs, bool is_write,
>> +        double elapsed_time, uint64_t *wait);
>> +static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
>> +        bool is_write, int64_t *wait);
>> +
>>  static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
>>      QTAILQ_HEAD_INITIALIZER(bdrv_states);
>>
>> @@ -107,6 +114,28 @@ int is_windows_drive(const char *filename)
>>  #endif
>>
>>  /* throttling disk I/O limits */
>> +void bdrv_io_limits_disable(BlockDriverState *bs)
>> +{
>> +    bs->io_limits_enabled = false;
>> +
>> +    if (!qemu_co_queue_empty(&bs->throttled_reqs)) {
>
> This if is unnecessary, you can just always run the while loop.
Good catch. Removed.
>
>> +        while (qemu_co_queue_next(&bs->throttled_reqs));
>> +    }
>> +
>> +    qemu_co_queue_init(&bs->throttled_reqs);
>
> Why?
Removed.
>
>> +
>> +    if (bs->block_timer) {
>> +        qemu_del_timer(bs->block_timer);
>> +        qemu_free_timer(bs->block_timer);
>> +        bs->block_timer = NULL;
>> +    }
>> +
>> +    bs->slice_start = 0;
>> +    bs->slice_end   = 0;
>> +    bs->slice_time  = 0;
>> +    memset(&bs->io_disps, 0, sizeof(bs->io_disps));
>> +}
>> +
>>  static void bdrv_block_timer(void *opaque)
>>  {
>>      BlockDriverState *bs = opaque;
>> @@ -116,14 +145,13 @@ static void bdrv_block_timer(void *opaque)
>>
>>  void bdrv_io_limits_enable(BlockDriverState *bs)
>>  {
>> -    bs->io_limits_enabled = true;
>>      qemu_co_queue_init(&bs->throttled_reqs);
>> -
>> -    bs->block_timer   = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
>> -    bs->slice_time    = 5 * BLOCK_IO_SLICE_TIME;
>> -    bs->slice_start   = qemu_get_clock_ns(vm_clock);
>> -    bs->slice_end     = bs->slice_start + bs->slice_time;
>> +    bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
>> +    bs->slice_time  = 5 * BLOCK_IO_SLICE_TIME;
>> +    bs->slice_start = qemu_get_clock_ns(vm_clock);
>> +    bs->slice_end   = bs->slice_start + bs->slice_time;
>
> You're only changing whitespace here, right? If so, can you please use
> the final version in patch 1?
OK
>
>>      memset(&bs->io_disps, 0, sizeof(bs->io_disps));
>> +    bs->io_limits_enabled = true;
>>  }
>>
>>  bool bdrv_io_limits_enabled(BlockDriverState *bs)
>> @@ -137,6 +165,30 @@ bool bdrv_io_limits_enabled(BlockDriverState *bs)
>>           || io_limits->iops[BLOCK_IO_LIMIT_TOTAL];
>>  }
>>
>> +static void bdrv_io_limits_intercept(BlockDriverState *bs,
>> +                                     bool is_write, int nb_sectors)
>> +{
>> +    int64_t wait_time = -1;
>> +
>> +    if (!qemu_co_queue_empty(&bs->throttled_reqs)) {
>> +        qemu_co_queue_wait(&bs->throttled_reqs);
>> +        goto resume;
>> +    } else if (bdrv_exceed_io_limits(bs, nb_sectors, is_write, &wait_time)) 
>> {
>> +        qemu_mod_timer(bs->block_timer,
>> +                       wait_time + qemu_get_clock_ns(vm_clock));
>> +        qemu_co_queue_wait(&bs->throttled_reqs);
>> +
>> +resume:
>
> This goto construct isn't very nice. You could have avoided it with an
> "else return;"
else return? no, it can not be returned shortly, i think.
>
> But anyway, why do you even need the else if? Can't you directly use the
> while loop? The difference would be that qemu_co_queue_next() is run
> even if a request is allowed without going through the queue, but would
> that hurt?
Great point. thanks.
>
>
>> +        while (bdrv_exceed_io_limits(bs, nb_sectors, is_write, &wait_time)) 
>> {
>> +            qemu_mod_timer(bs->block_timer,
>> +                           wait_time + qemu_get_clock_ns(vm_clock));
>> +            qemu_co_queue_wait_insert_head(&bs->throttled_reqs);
>
> Why do you want to insert at the head? Wouldn't a queue be more
In fact, we hope to keep each request's timing, in FIFO mode. The next
throttled requests will not be dequeued until the current request is
allowed to be serviced. So if the current request still exceeds the
limits, it will be inserted to the head. All requests followed it will
be still in throttled_reqs queue.

> appropriate than a stack?
>
>> +        }
>> +
>> +        qemu_co_queue_next(&bs->throttled_reqs);
>> +    }
>> +}
>> +
>>  /* check if the path starts with "<protocol>:" */
>>  static int path_has_protocol(const char *path)
>>  {
>
> Kevin
>



-- 
Regards,

Zhi Yong Wu