Re: [Qemu-devel] [V7 1/1] fsdev: add IO throttle support to fsdev devices

[Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>]

On 10/24/2016 3:00 PM, Alberto Garcia wrote:
> On Sat 22 Oct 2016 05:07:22 PM CEST, Pradeep Jagadeesh wrote:
>
>> This adds the support for the 9p-local driver.  For now this
>> functionality can be enabled only through qemu cli options.  QMP
>> interface and support to other drivers need further extensions.  To
>> make it simple for other drivers, the throttle code has been put in
>> separate files.
>
> Ok, I think this is almost ready. There's only a few corrections and
> style fixes left.

Hi Alberto, thanks for having a look at the patch.
I will send the patch early next week as I am out of office.

-Pradeep

>
>> diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs
>> index 1b120a4..2c6da2d 100644
>> --- a/fsdev/Makefile.objs
>> +++ b/fsdev/Makefile.objs
>> @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o
>>  endif
>>  common-obj-y += qemu-fsdev-opts.o
>>
>> +common-obj-y += qemu-fsdev-throttle.o
>
> (Optional) You can put these two in the same line:
>
> common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o
>
>> +++ b/fsdev/qemu-fsdev-throttle.c
>
> In this file there's many lines that are incorrectly indented.
> Indentation should be four spaces. The following functions have lines
> that need to be corrected:
>
> fsdev_throttle_check_for_wait
> fsdev_throttle_schedule_next_request
> fsdev_parse_io_limit_opts
> fsdev_throttle_configure_iolimits
> fsdev_co_throttle_request
>
> Check also the other changes in your patch in case there are more lines
> with wrong indentation.
>
>> +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write)
>
> "Check for wait" doesn't sound like correct English to me. How about
> simply fsdev_throttle_schedule_timer()? Or fsdev_throttle_must_wait()
>
>> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write)
>> +{
>> +   if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {
>
> Here's an example of a line with the wrong indentation. It uses three
> spaces, it should use four.
>
>> +       qemu_co_queue_next(&fst->throttled_reqs[is_write]);
>
> And this one uses seven, it should use eight. There are more cases like
> this, I only highlighted a couple of them.
>
>> +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst)
>> +{
>> +    Error *err = NULL;
>> +
>> +    /* Parse and set populate the cfg structure */
>> +    fsdev_parse_io_limit_opts(opts, fst);
>> +
>> +    if (!throttle_is_valid(&fst->cfg, &err)) {
>> +        error_reportf_err(err, "Throttle configuration is not valid: ");
>> +        return -1;
>> +    }
>> +    if (throttle_enabled(&fst->cfg)) {
>> +        g_assert((fst->aioctx = qemu_get_aio_context()));
>> +        throttle_init(&fst->ts);
>> +        throttle_timers_init(&fst->tt,
>> +                             fst->aioctx,
>> +                             QEMU_CLOCK_REALTIME,
>> +                             fsdev_throttle_read_timer_cb,
>> +                             fsdev_throttle_write_timer_cb,
>> +                             fst);
>> +        throttle_config(&fst->ts, &fst->tt, &fst->cfg);
>> +        qemu_co_queue_init(&fst->throttled_reqs[0]);
>> +        qemu_co_queue_init(&fst->throttled_reqs[1]);
>> +   }
>> +   return 0;
>> +}
>
> Here you're parsing the command-line options and initializing the
> throtting structures in local_parse_opts(). Then you're cleaning up in
> v9fs_device_unrealize_common().
>
> I have two questions:
>
> a) You could split fsdev_throttle_configure_iolimits() in two:
>    1) fsdev_throttle_parse_opts(), called from local_parse_opts()
>    2) fsdev_throttle_init(), called from v9fs_device_realize_common().
>
> b) Why are you only doing this for the "local" fsdriver, and not for
>    "handle" or "proxy"? Isn't it possible to do throttling with those
>    as well?
>
>> +static int get_num_bytes(struct iovec *iov, int iovcnt)
>> +{
>> +    int i;
>> +    unsigned int bytes = 0;
>
> You changed 'bytes' from int to unsigned as I suggested, but you still
> return an int, and in fsdev_co_throttle_request() you also get an
> int. Please use unsigned in all these cases.
>
>> +int  fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *);
>> +
>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write,
>> +                            struct iovec *iov, int iovcnt);
>
> Please align this line with the parameters of the first line.
>
> Also, why do you include the name of the parameters in this function
> prototype but not in the other two?
>
>> @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts,
>> struct FsDriverEntry *fse)
>>          error_report("fsdev: No path specified");
>>          return -1;
>>      }
>> +
>> +    if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) {
>> +        return -1;
>> +    }
>
> If you create fsdev_throttle_parse_opts() as I suggest, I'd rather print
> the error message here as the other error messages in this function.
>
> I think I haven't forgotten anything, everything else looks fine. I'd
> still like to have all the throtting parameters merged so we don't need
> to duplicate them, but that can be done in the future (I can even take
> care of it if I find a bit of time).
>
> Thanks for the patch!
>
> Berto
>