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

[Alberto Garcia]

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 ("},{")

> +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").

> +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.

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

Berto