Re: [Qemu-devel] [V6] fsdev: add IO throttle support to fsdev devices

[Alberto Garcia]

On Fri 21 Oct 2016 06:21:59 PM CEST, Pradeep Jagadeesh <address@hidden> wrote:

> +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write)
> +{
> +   return  throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
             ^
There's an extra whitespace there.

> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool 
> is_write)
> +{
> +   if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {
> +        if (fsdev_throttle_check_for_wait(fst, is_write)) {
> +            return;
> +        }
> +   }
> +       qemu_co_queue_next(&fst->throttled_reqs[is_write]);
> +}

That is wrong, you're calling qemu_co_queue_next() even if the queue is
empty. That line should be inside the 'if' block.

I still think that -after the changes you made in v6- you could put the
contents of that whole function inside fsdev_co_throttle_request(), and
at the same time get rid of fsdev_throttle_check_for_wait() and call
throttle_schedule_timer() directly. The resulting code will be smaller
and probably easier to read. But those are just style decisions.

> +static int get_num_bytes(struct iovec *iov, int iovcnt)
> +{
> +    int i;
> +    int bytes = 0;
> +
> +    for (i = 0; i < iovcnt; i++) {
> +        bytes += iov[i].iov_len;
> +    }
> +    return bytes;
> +}

I overlooked that in my previous review, but 'bytes' should be unsigned
(both here and in fsdev_co_throttle_request).

I still have to review the init/cleanup functions (I'll try to do it on
monday) but else the code looks fine now.

Berto