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

[Pradeep Jagadeesh]

> > > 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.
Fixed.
> > > +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.
Done!
> > > +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().
Done as you suggested. Now parsing happens in local_parse_opts()
and initialization in 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?
Yes, its possible. As of now I wanted to go with just with 9p-local 
because, we had a use case for only that. I feel with existing throttle 
apis (fsdev_throttle), it is easy enable for proxy and handle drivers also.
> > > +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.
done
> > > +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?
I removed them.
> > > @@ -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 re-wrote as suggested by you.

> > 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!

-Pradeep

> > Berto