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

[Pradeep Jagadeesh]

> On Mon 07 Nov 2016 04:45:55 PM CET, Pradeep Jagadeesh wrote:
> > --- a/fsdev/qemu-fsdev-opts.c
> > +++ b/fsdev/qemu-fsdev-opts.c
> > @@ -37,8 +37,83 @@ 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",
> > +        },{
>
> Not very important, but since this version also needs to be corrected
> (as you'll see below) you can update this part to keep the sytle
> consisent: all other entries in this array have a space between the
> command and the opening brace ("}, {"). Most of your entries don't have
> that space ("},{")
OK
> > +void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error *err)
> > +{
> > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> > +        qemu_opt_get_number(opts, "throttling.bps-total", 0);
>   /* ... */
> > +    fst->cfg.op_size =
> > +        qemu_opt_get_number(opts, "throttling.iops-size", 0);
> > +
> > +    if (!throttle_is_valid(&fst->cfg, &err)) {
> > +        return;
> > +    }
> > +}
>
> That last 'if' is a bit odd :) but I won't object if you want to keep
> it. You can simply leave the throttle_is_valid() call and a comment that
> explains what it does ("Check that the config is valid and update errp
> otherwise").
I will better remove that. :)
> > +void fsdev_throttle_init(FsThrottle *fst)
> > +{
> > +        if (throttle_enabled(&fst->cfg)) {
> > +            throttle_init(&fst->ts);
>
> This block is indented with 8 whitespaces, not with 4 as it should be.
Hmm,I will fix it
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -1209,6 +1209,7 @@ 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");
> > +    Error *err = NULL;
> >
> >      if (!sec_model) {
> >          error_report("Security model not specified, local fs needs security 
> > model");
> > @@ -1237,6 +1238,13 @@ static int local_parse_opts(QemuOpts *opts, struct 
> > FsDriverEntry *fse)
> >          error_report("fsdev: No path specified");
> >          return -1;
> >      }
> > +
> > +    fsdev_throttle_parse_opts(opts, &fse->fst, err);
> > +    if (err) {
> > +        error_reportf_err(err, "Throttle configuration is not valid: ");
> > +        return -1;
> > +    }
> > +
>
> This is the important part that needs to be changed: 'err' here is NULL,
> so you're passing a NULL pointer to fsdev_throttle_parse_opts() and
> invalid throttling configurations are not being reported.
>
> You should pass &err instead, and fsdev_throttle_parse_opts() must
> receive Error **errp.

OK

-Pradeep
> Berto