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

[Pradeep Jagadeesh]

> 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.
Removed
> > +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.
Done!
> 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.
I just would like to keep these functions.
> > +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).
Done!
> I still have to review the init/cleanup functions (I'll try to do it on
> monday) but else the code looks fine now.
Ok, I am out of office for whole week. I can send the next patch only on 
next saturday/sunday. Please let me know all your comments by then.

-Pradeep
> Berto