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

[Pradeep Jagadeesh]

Hi Alberto,

Thanks for having look at the patch.
My replies are inline.
> On Fri 16 Sep 2016 10:33:36 AM CEST, Pradeep Jagadeesh wrote:
>
> Hi,
>
> first of all, sorry for the late reply! Here are my comments:
No problem!
> > --- a/fsdev/qemu-fsdev-opts.c
> > +++ b/fsdev/qemu-fsdev-opts.c
> > @@ -37,6 +37,82 @@ static QemuOptsList qemu_fsdev_opts = {
> >          }, {
> >              .name = "sock_fd",
> >              .type = QEMU_OPT_NUMBER,
> > +        }, {
> > +            .name = "throttling.iops-total",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit total I/O operations per second",
>   /*...*/
> > +        },{
> > +            .name = "throttling.iops-size",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "when limiting by iops max size of an I/O in bytes",
>
> It would be nice if we could factor these so we don't have to have them
> twice in the code. I think it should be doable with qemu_opts_append().
>
> It can be done later in a separate patch if you prefer.
OK,as of now I have just copied. We can rework and remove the redundant 
code later.
> > +typedef struct FsThrottle {
> > +    ThrottleState ts;
> > +    ThrottleTimers tt;
> > +    AioContext   *aioctx;
> > +    ThrottleConfig cfg;
> > +    bool enabled;
> > +    CoQueue      throttled_reqs[2];
> > +    unsigned     pending_reqs[2];
> > +    bool any_timer_armed[2];
> > +    QemuMutex lock;
> > +} FsThrottle;
>
> You based your implementation on block/throttle-groups.c. I think yours
> can be made simpler because one of the problems with mine is that it
> needs to support multiple parallel I/O operations on the same throttling
> group, and that's why the locking rules are more complex. With a single
> user per ThrottleConfig this is not necessary.
>
> Have you checked if you really need them? My impression is that you
> might be able to get rid of the 'lock', 'any_timer_armed' and
> 'pending_reqs' fields.
>
> Please check commit 76f4afb40fa076ed23fe0ab42c7a768ddb71123f to see how
> was the transition from single-drive throttling to group throttling,
> specifically the bdrv_io_limits_intercept() function. You will see that
> it was simpler.
OK,I am not sure,I will have to try. I will look into this and let you know.
> > @@ -436,8 +436,8 @@ static ssize_t local_pwritev(FsContext *ctx, 
> > V9fsFidOpenState *fs,
> >                               const struct iovec *iov,
> >                               int iovcnt, off_t offset)
> >  {
> > -    ssize_t ret
> > -;
> > +    ssize_t ret;
> > +
>
> This could go to a separate patch, but I'm fine if you include it in
> this one.
I have already included as Greg suggested.
> > @@ -1213,6 +1213,8 @@ static int local_parse_opts(QemuOpts *opts, struct 
> > FsDriverEntry *fse)
> >      const char *sec_model = qemu_opt_get(opts, "security_model");
> >      const char *path = qemu_opt_get(opts, "path");
> >
> > +    FsThrottle *fst = &fse->fst;
> > +
>
> I'd remove the empty line between the declarations of 'path' and 'fst',
> however...
OK
> >      if (!sec_model) {
> >          error_report("Security model not specified, local fs needs security 
> > model");
> >          error_printf("valid options are:"
> > @@ -1240,6 +1242,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, fst) < 0) {
> > +        return -1;
> > +    }
>
> ...you don't need to declare fst here, you could also pass &fse->fst
> directly.
OK

-Pradeep

> Berto