[v2] 9pfs: add support for IO limits to 9p-local driver

Uses throttling APIs to limit I/O bandwidth and number of operations on the 
devices which use 9p-local driver.

Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
---
 fsdev/file-op-9p.h      |   3 +
 fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
 hw/9pfs/9p-local.c      |  18 ++++-
 hw/9pfs/9p-throttle.c   | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/9p-throttle.h   |  46 +++++++++++
 hw/9pfs/9p.c            |   7 ++
 hw/9pfs/Makefile.objs   |   1 +
 7 files changed, 326 insertions(+), 2 deletions(-)
 create mode 100644 hw/9pfs/9p-throttle.c
 create mode 100644 hw/9pfs/9p-throttle.h

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.

v1 -> v2:

-Fixed FsContext redeclaration issue
-Removed couple of function declarations from 9p-throttle.h
-Fixed some of the .help messages

Hi Pradeep,

two comment below:

On 09.09.2016 11:10, Pradeep Jagadeesh wrote:
> Uses throttling APIs to limit I/O bandwidth and number of operations on the 
> devices which use 9p-local driver.
> 
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---
>  fsdev/file-op-9p.h      |   3 +
>  fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
>  hw/9pfs/9p-local.c      |  18 ++++-
>  hw/9pfs/9p-throttle.c   | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/9pfs/9p-throttle.h   |  46 +++++++++++
>  hw/9pfs/9p.c            |   7 ++
>  hw/9pfs/Makefile.objs   |   1 +
>  7 files changed, 326 insertions(+), 2 deletions(-)
>  create mode 100644 hw/9pfs/9p-throttle.c
>  create mode 100644 hw/9pfs/9p-throttle.h
> 
> 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.
> 
> v1 -> v2:
> 
> -Fixed FsContext redeclaration issue
> -Removed couple of function declarations from 9p-throttle.h
> -Fixed some of the .help messages
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 6db9fea..e86b91a 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -17,6 +17,7 @@
>  #include <dirent.h>
>  #include <utime.h>
>  #include <sys/vfs.h>
> +#include "hw/9pfs/9p-throttle.h"
>  
>  #define SM_LOCAL_MODE_BITS    0600
>  #define SM_LOCAL_DIR_MODE_BITS    0700
> @@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
>      char *path;
>      int export_flags;
>      FileOperations *ops;
> +    FsThrottle fst;
>  } FsDriverEntry;
>  
>  typedef struct FsContext
> @@ -83,6 +85,7 @@ typedef struct FsContext
>      int export_flags;
>      struct xattr_operations **xops;
>      struct extended_ops exops;
> +    FsThrottle *fst;
>      /* fs driver specific data */
>      void *private;
>  } FsContext;
> diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
> index 1dd8c7a..2774855 100644
> --- a/fsdev/qemu-fsdev-opts.c
> +++ b/fsdev/qemu-fsdev-opts.c
> @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
>          }, {
>              .name = "sock_fd",
>              .type = QEMU_OPT_NUMBER,
> +        }, {
> +            .name = "bps",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit total bytes per second",
> +        }, {
> +            .name = "bps_rd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit read bytes per second",
> +        }, {
> +            .name = "bps_wr",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit write bytes per second",
> +        }, {
> +            .name = "iops",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit total io operations per second",
> +        }, {
> +            .name = "iops_rd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit read operations per second ",
> +        }, {
> +            .name = "iops_wr",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit write operations per second",
> +        }, {
> +            .name = "bps_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes burst",
> +        }, {
> +            .name = "bps_rd_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes read burst",
> +        }, {
> +            .name = "bps_wr_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes write burst",
> +        }, {
> +            .name = "iops_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations burst",
> +        }, {
> +            .name = "iops_rd_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations read burst",
> +        }, {
> +            .name = "iops_wr_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations write burst",
> +        }, {
> +            .name = "iops_size",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "io iops-size",
>          },
>  
>          { /*End of list */ }
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 3f271fc..49c2819 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
>                              const struct iovec *iov,
>                              int iovcnt, off_t offset)
>  {
> +    throttle9p_request(ctx->fst, false, iov->iov_len);
> +
>  #ifdef CONFIG_PREADV
>      return preadv(fs->fd, iov, iovcnt, offset);
>  #else
> @@ -436,8 +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
>                               const struct iovec *iov,
>                               int iovcnt, off_t offset)
>  {
> -    ssize_t ret
> -;
> +    ssize_t ret;
> +
> +    throttle9p_request(ctx->fst, true, iov->iov_len);
> +
>  #ifdef CONFIG_PREADV
>      ret = pwritev(fs->fd, iov, iovcnt, offset);
>  #else
> @@ -1213,6 +1217,9 @@ 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");
>  
> +    /* get the throttle structure */
> +    FsThrottle *fst = &fse->fst;
> +
>      if (!sec_model) {
>          error_report("Security model not specified, local fs needs security model");
>          error_printf("valid options are:"
> @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
>          error_report("fsdev: No path specified");
>          return -1;
>      }
> +
> +    throttle9p_enable_io_limits(opts, fst);
> +
> +    if (throttle9p_get_io_limits_state(fst)) {
> +        throttle9p_configure_iolimits(opts, fst);
> +    }
> +
>      fse->path = g_strdup(path);
>  
>      return 0;
> diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c
> new file mode 100644
> index 0000000..f2a7ba5
> --- /dev/null
> +++ b/hw/9pfs/9p-throttle.c
> @@ -0,0 +1,201 @@
> +/*
> + * 9P Throttle
> + *
> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.
> + *
> + * See the COPYING file in the top-level directory for details.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> +#include "qemu/cutils.h"
> +#include "qemu/error-report.h"
> +#include <libgen.h>
> +#include <linux/fs.h>
> +#include <sys/ioctl.h>
> +#include <string.h>
> +#include "fsdev/file-op-9p.h"
> +#include "9p-throttle.h"
> +
> +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst)
> +{
> +    const float bps = qemu_opt_get_number(opts, "bps", 0);
> +    const float iops = qemu_opt_get_number(opts, "iops", 0);
> +    const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0);
> +    const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0);
> +    const float rdops = qemu_opt_get_number(opts, "iops_rd", 0);
> +    const float wrops = qemu_opt_get_number(opts, "iops_wr", 0);
> +
> +    if (bps > 0 || iops > 0 || rdbps > 0 ||
> +        wrbps > 0 || rdops > 0 || wrops > 0) {
> +        fst->io_limits_enabled = true;
> +    } else {
> +        fst->io_limits_enabled = false;
> +    }
> +}
> +
> +static bool throttle9p_check_for_wait(FsThrottle *fst, bool is_write)
> +{
> +    if (fst->any_timer_armed[is_write]) {
> +        return true;
> +    } else {
> +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> +    }
> +}
> +
> +static void throttle9p_schedule_next_request(FsThrottle *fst, bool is_write)
> +{
> +    bool must_wait = throttle9p_check_for_wait(fst, is_write);
> +    if (!fst->pending_reqs[is_write]) {
> +        return;
> +    }
> +    if (!must_wait) {
> +        if (qemu_in_coroutine() &&
> +            qemu_co_queue_next(&fst->throttled_reqs[is_write])) {
> +            ;
> +       } else {
> +           int64_t now = qemu_clock_get_ns(fst->tt.clock_type);
> +           timer_mod(fst->tt.timers[is_write], now + 1);
> +           fst->any_timer_armed[is_write] = true;
> +       }
> +   }
> +}
> +
> +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write)
> +{
> +    bool empty_queue;
> +    qemu_mutex_lock(&fst->lock);
> +    fst->any_timer_armed[is_write] = false;
> +    qemu_mutex_unlock(&fst->lock);
> +    empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]);
> +    if (empty_queue) {
> +        qemu_mutex_lock(&fst->lock);
> +        throttle9p_schedule_next_request(fst, is_write);
> +        qemu_mutex_unlock(&fst->lock);
> +    }
> +}
> +
> +
> +bool throttle9p_get_io_limits_state(FsThrottle *fst)
> +{
> +
> +    return fst->io_limits_enabled;
> +}
> +
> +static void throttle9p_read_timer_cb(void *opaque)
> +{
> +    throttle9p_timer_cb(opaque, false);
> +}
> +
> +static void throttle9p_write_timer_cb(void *opaque)
> +{
> +    throttle9p_timer_cb(opaque, true);
> +}
> +
> +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle *fst)
> +{
> +    memset(&fst->ts, 1, sizeof(fst->ts));
> +    memset(&fst->tt, 1, sizeof(fst->tt));
> +    memset(&fst->cfg, 0, sizeof(fst->cfg));

why do you memset these structs to those values?
->ts and ->tt to all 01h bytes and cfg to 00h?

There are already initialization functions for those which you call below,
throttle_init, throttle_timers_init and throttle_config_init,
so I think these memsets should be removed.


> +    fst->aioctx = qemu_get_aio_context();
> +
> +    if (!fst->aioctx) {
> +        error_report("Failed to create AIO Context");
> +        exit(1);
> +    }
> +    throttle_init(&fst->ts);
> +    throttle_timers_init(&fst->tt,
> +                         fst->aioctx,
> +                         QEMU_CLOCK_REALTIME,
> +                         throttle9p_read_timer_cb,
> +                         throttle9p_write_timer_cb,
> +                         fst);
> +    throttle_config_init(&fst->cfg);
> +    g_assert(throttle_is_valid(&fst->cfg, NULL));
> +
> +    qemu_co_queue_init(&fst->throttled_reqs[0]);
> +    qemu_co_queue_init(&fst->throttled_reqs[1]);
> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> +          qemu_opt_get_number(opts, "bps", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
> +          qemu_opt_get_number(opts, "bps_rd", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> +          qemu_opt_get_number(opts, "bps_wr", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> +          qemu_opt_get_number(opts, "iops", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> +          qemu_opt_get_number(opts, "iops_rd", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> +          qemu_opt_get_number(opts, "iops_wr", 0);
> +
> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> +          qemu_opt_get_number(opts, "bps_max", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
> +          qemu_opt_get_number(opts, "bps_rd_max", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> +          qemu_opt_get_number(opts, "bps_wr_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> +          qemu_opt_get_number(opts, "iops_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].max =
> +          qemu_opt_get_number(opts, "iops_rd_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> +          qemu_opt_get_number(opts, "iops_wr_max", 0);
> +
> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
> +          qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].burst_length  =
> +          qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
> +          qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
> +          qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
> +          qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
> +          qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
> +    fst->cfg.op_size =
> +          qemu_opt_get_number(opts, "iops_size", 0);
> +
> +    throttle_config(&fst->ts, &fst->tt, &fst->cfg);
> +    if (!throttle_is_valid(&fst->cfg, NULL)) {
> +        return;

What is the expected state if we bail at this point?
Is QEMU expected to work anyway in this case? Ignoring the limits?
 
The data structures initialized below will instead not be, including the fst->lock.

Ciao,

Claudio

> +    }
> +
> +    g_assert(fst->tt.timers[0]);
> +    g_assert(fst->tt.timers[1]);
> +    fst->pending_reqs[0] = 0;
> +    fst->pending_reqs[1] = 0;
> +    fst->any_timer_armed[0] = false;
> +    fst->any_timer_armed[1] = false;
> +    qemu_mutex_init(&fst->lock);
> +}
> +
> +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t bytes)
> +{
> +    if (fst->io_limits_enabled) {
> +        qemu_mutex_lock(&fst->lock);
> +        bool must_wait = throttle9p_check_for_wait(fst, is_write);
> +        if (must_wait || fst->pending_reqs[is_write]) {
> +            fst->pending_reqs[is_write]++;
> +            qemu_mutex_unlock(&fst->lock);
> +            qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> +            qemu_mutex_lock(&fst->lock);
> +            fst->pending_reqs[is_write]--;
> +       }
> +       throttle_account(&fst->ts, is_write, bytes);
> +       throttle9p_schedule_next_request(fst, is_write);
> +       qemu_mutex_unlock(&fst->lock);
> +    }
> +}
> +
> +void throttle9p_cleanup(FsThrottle *fst)
> +{
> +    throttle_timers_destroy(&fst->tt);
> +    qemu_mutex_destroy(&fst->lock);
> +}
> diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h
> new file mode 100644
> index 0000000..0f7551d
> --- /dev/null
> +++ b/hw/9pfs/9p-throttle.h
> @@ -0,0 +1,46 @@
> +/*
> + * 9P Throttle
> + *
> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.
> + *
> + * See the COPYING file in the top-level directory for details.
> + *
> + */
> +
> +#ifndef _9P_THROTTLE_H
> +#define _9P_THROTTLE_H
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include "block/aio.h"
> +#include "qemu/main-loop.h"
> +#include "qemu/coroutine.h"
> +#include "qemu/throttle.h"
> +
> +typedef struct FsThrottle {
> +    ThrottleState ts;
> +    ThrottleTimers tt;
> +    AioContext   *aioctx;
> +    ThrottleConfig cfg;
> +    bool io_limits_enabled;
> +    CoQueue      throttled_reqs[2];
> +    unsigned     pending_reqs[2];
> +    bool any_timer_armed[2];
> +    QemuMutex lock;
> +} FsThrottle;
> +
> +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *);
> +
> +bool throttle9p_get_io_limits_state(FsThrottle *);
> +
> +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *);
> +
> +void throttle9p_request(FsThrottle *, bool , ssize_t);
> +
> +void throttle9p_cleanup(FsThrottle *);
> +#endif /* _9P_THROTTLE_H */
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index dfe293d..7be926a 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
>          error_setg(errp, "share path %s is not a directory", fse->path);
>          goto out;
>      }
> +
> +    /* Throttle structure initialization */
> +    s->ctx.fst = &fse->fst;
> +
>      v9fs_path_free(&path);
>  
>      rc = 0;
> @@ -3504,6 +3508,9 @@ out:
>  
>  void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
>  {
> +    if (throttle9p_get_io_limits_state(s->ctx.fst)) {
> +        throttle9p_cleanup(s->ctx.fst);
> +    }
>      g_free(s->ctx.fs_root);
>      g_free(s->tag);
>  }
> diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
> index da0ae0c..07523f1 100644
> --- a/hw/9pfs/Makefile.objs
> +++ b/hw/9pfs/Makefile.objs
> @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o
>  common-obj-y += coxattr.o 9p-synth.o
>  common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o
>  common-obj-y += 9p-proxy.o
> +common-obj-y += 9p-throttle.o
>  
>  obj-y += virtio-9p-device.o
>

On Fri,  9 Sep 2016 05:10:27 -0400
Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:

> Uses throttling APIs to limit I/O bandwidth and number of operations on the 
> devices which use 9p-local driver.
> 
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---

Hi Pradeep,

Please find some remarks below. I haven't dived deep enough to see if this
actually works. Maybe Berto can provide some feedback ?

Cheers.

--
Greg

>  fsdev/file-op-9p.h      |   3 +
>  fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
>  hw/9pfs/9p-local.c      |  18 ++++-
>  hw/9pfs/9p-throttle.c   | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/9pfs/9p-throttle.h   |  46 +++++++++++
>  hw/9pfs/9p.c            |   7 ++
>  hw/9pfs/Makefile.objs   |   1 +
>  7 files changed, 326 insertions(+), 2 deletions(-)
>  create mode 100644 hw/9pfs/9p-throttle.c
>  create mode 100644 hw/9pfs/9p-throttle.h
> 
> 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.
> 
> v1 -> v2:
> 
> -Fixed FsContext redeclaration issue
> -Removed couple of function declarations from 9p-throttle.h
> -Fixed some of the .help messages
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 6db9fea..e86b91a 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -17,6 +17,7 @@
>  #include <dirent.h>
>  #include <utime.h>
>  #include <sys/vfs.h>
> +#include "hw/9pfs/9p-throttle.h"
>  
>  #define SM_LOCAL_MODE_BITS    0600
>  #define SM_LOCAL_DIR_MODE_BITS    0700
> @@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
>      char *path;
>      int export_flags;
>      FileOperations *ops;
> +    FsThrottle fst;
>  } FsDriverEntry;
>  
>  typedef struct FsContext
> @@ -83,6 +85,7 @@ typedef struct FsContext
>      int export_flags;
>      struct xattr_operations **xops;
>      struct extended_ops exops;
> +    FsThrottle *fst;
>      /* fs driver specific data */
>      void *private;
>  } FsContext;
> diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
> index 1dd8c7a..2774855 100644
> --- a/fsdev/qemu-fsdev-opts.c
> +++ b/fsdev/qemu-fsdev-opts.c
> @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
>          }, {
>              .name = "sock_fd",
>              .type = QEMU_OPT_NUMBER,
> +        }, {
> +            .name = "",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit total bytes per second",
> +        }, {
> +            .name = "bps_rd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit read bytes per second",
> +        }, {
> +            .name = "bps_wr",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit write bytes per second",
> +        }, {
> +            .name = "iops",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit total io operations per second",
> +        }, {
> +            .name = "iops_rd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit read operations per second ",
> +        }, {
> +            .name = "iops_wr",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit write operations per second",
> +        }, {
> +            .name = "bps_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes burst",
> +        }, {
> +            .name = "bps_rd_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes read burst",
> +        }, {
> +            .name = "bps_wr_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes write burst",
> +        }, {
> +            .name = "iops_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations burst",
> +        }, {
> +            .name = "iops_rd_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations read burst",
> +        }, {
> +            .name = "iops_wr_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations write burst",
> +        }, {
> +            .name = "iops_size",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "io iops-size",
>          },
>  

In case we end up sharing code with blockdev as suggested by Eric, maybe you
can use the same QMP friendly naming scheme ("throttling.*") and the same
help strings as well.

>          { /*End of list */ }
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 3f271fc..49c2819 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
>                              const struct iovec *iov,
>                              int iovcnt, off_t offset)
>  {
> +    throttle9p_request(ctx->fst, false, iov->iov_len);
> +
>  #ifdef CONFIG_PREADV
>      return preadv(fs->fd, iov, iovcnt, offset);
>  #else
> @@ -436,8 +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
>                               const struct iovec *iov,
>                               int iovcnt, off_t offset)
>  {
> -    ssize_t ret
> -;
> +    ssize_t ret;
> +
> +    throttle9p_request(ctx->fst, true, iov->iov_len);
> +
>  #ifdef CONFIG_PREADV
>      ret = pwritev(fs->fd, iov, iovcnt, offset);
>  #else
> @@ -1213,6 +1217,9 @@ 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");
>  
> +    /* get the throttle structure */

Not sure this comment is helpful.

> +    FsThrottle *fst = &fse->fst;
> +
>      if (!sec_model) {
>          error_report("Security model not specified, local fs needs security model");
>          error_printf("valid options are:"
> @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
>          error_report("fsdev: No path specified");
>          return -1;
>      }
> +
> +    throttle9p_enable_io_limits(opts, fst);
> +
> +    if (throttle9p_get_io_limits_state(fst)) {
> +        throttle9p_configure_iolimits(opts, fst);
> +    }
> +
>      fse->path = g_strdup(path);
>  
>      return 0;
> diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c
> new file mode 100644
> index 0000000..f2a7ba5
> --- /dev/null
> +++ b/hw/9pfs/9p-throttle.c
> @@ -0,0 +1,201 @@
> +/*
> + * 9P Throttle
> + *
> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.
> + *
> + * See the COPYING file in the top-level directory for details.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> +#include "qemu/cutils.h"
> +#include "qemu/error-report.h"
> +#include <libgen.h>
> +#include <linux/fs.h>
> +#include <sys/ioctl.h>
> +#include <string.h>
> +#include "fsdev/file-op-9p.h"
> +#include "9p-throttle.h"
> +

Only "qemu/osdep.h", "qemu/error-report.h" and "9p-throttle.h" are actually
needed.

> +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst)
> +{
> +    const float bps = qemu_opt_get_number(opts, "bps", 0);
> +    const float iops = qemu_opt_get_number(opts, "iops", 0);
> +    const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0);
> +    const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0);
> +    const float rdops = qemu_opt_get_number(opts, "iops_rd", 0);
> +    const float wrops = qemu_opt_get_number(opts, "iops_wr", 0);
> +
> +    if (bps > 0 || iops > 0 || rdbps > 0 ||
> +        wrbps > 0 || rdops > 0 || wrops > 0) {
> +        fst->io_limits_enabled = true;
> +    } else {
> +        fst->io_limits_enabled = false;
> +    }
> +}
> +

This function should be named *_parse_* but I'm not even sure it is
actually needed (see below).

> +static bool throttle9p_check_for_wait(FsThrottle *fst, bool is_write)
> +{
> +    if (fst->any_timer_armed[is_write]) {
> +        return true;
> +    } else {
> +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> +    }
> +}
> +
> +static void throttle9p_schedule_next_request(FsThrottle *fst, bool is_write)
> +{
> +    bool must_wait = throttle9p_check_for_wait(fst, is_write);
> +    if (!fst->pending_reqs[is_write]) {
> +        return;
> +    }
> +    if (!must_wait) {
> +        if (qemu_in_coroutine() &&
> +            qemu_co_queue_next(&fst->throttled_reqs[is_write])) {
> +            ;
> +       } else {
> +           int64_t now = qemu_clock_get_ns(fst->tt.clock_type);
> +           timer_mod(fst->tt.timers[is_write], now + 1);
> +           fst->any_timer_armed[is_write] = true;
> +       }
> +   }
> +}
> +
> +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write)
> +{
> +    bool empty_queue;
> +    qemu_mutex_lock(&fst->lock);
> +    fst->any_timer_armed[is_write] = false;
> +    qemu_mutex_unlock(&fst->lock);
> +    empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]);
> +    if (empty_queue) {
> +        qemu_mutex_lock(&fst->lock);
> +        throttle9p_schedule_next_request(fst, is_write);
> +        qemu_mutex_unlock(&fst->lock);
> +    }
> +}
> +
> +
> +bool throttle9p_get_io_limits_state(FsThrottle *fst)

The name looks a bit strange, since this helper simply returns a boolean flag.
I guess throttle9p_enabled() is enough.

> +{
> +
> +    return fst->io_limits_enabled;
> +}
> +
> +static void throttle9p_read_timer_cb(void *opaque)
> +{
> +    throttle9p_timer_cb(opaque, false);
> +}
> +
> +static void throttle9p_write_timer_cb(void *opaque)
> +{
> +    throttle9p_timer_cb(opaque, true);
> +}
> +
> +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle *fst)
> +{

This function can fail, it should have a return value (0 or -1).

> +    memset(&fst->ts, 1, sizeof(fst->ts));
> +    memset(&fst->tt, 1, sizeof(fst->tt));
> +    memset(&fst->cfg, 0, sizeof(fst->cfg));

Same remark as Claudio.

> +    fst->aioctx = qemu_get_aio_context();
> +
> +    if (!fst->aioctx) {
> +        error_report("Failed to create AIO Context");
> +        exit(1);
> +    }
> +    throttle_init(&fst->ts);
> +    throttle_timers_init(&fst->tt,
> +                         fst->aioctx,
> +                         QEMU_CLOCK_REALTIME,
> +                         throttle9p_read_timer_cb,
> +                         throttle9p_write_timer_cb,
> +                         fst);

Shouldn't all this be done later when we know the config is valid ?

> +    throttle_config_init(&fst->cfg);
> +    g_assert(throttle_is_valid(&fst->cfg, NULL));
> +

What's the point in calling throttle_is_valid() on a freshly initialized
config ?

> +    qemu_co_queue_init(&fst->throttled_reqs[0]);
> +    qemu_co_queue_init(&fst->throttled_reqs[1]);

Later, when the config is valid ?

> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> +          qemu_opt_get_number(opts, "bps", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
> +          qemu_opt_get_number(opts, "bps_rd", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> +          qemu_opt_get_number(opts, "bps_wr", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> +          qemu_opt_get_number(opts, "iops", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> +          qemu_opt_get_number(opts, "iops_rd", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> +          qemu_opt_get_number(opts, "iops_wr", 0);
> +
> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> +          qemu_opt_get_number(opts, "bps_max", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
> +          qemu_opt_get_number(opts, "bps_rd_max", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> +          qemu_opt_get_number(opts, "bps_wr_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> +          qemu_opt_get_number(opts, "iops_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].max =
> +          qemu_opt_get_number(opts, "iops_rd_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> +          qemu_opt_get_number(opts, "iops_wr_max", 0);
> +
> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
> +          qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].burst_length  =
> +          qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
> +          qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
> +          qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
> +          qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
> +          qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
> +    fst->cfg.op_size =
> +          qemu_opt_get_number(opts, "iops_size", 0);
> +
> +    throttle_config(&fst->ts, &fst->tt, &fst->cfg);

Let's set the config later, when we we it is valid.

> +    if (!throttle_is_valid(&fst->cfg, NULL)) {

You should pass an Error * to throttle_is_valid() to be able to report the
misconfiguration to the user. I guess it is okay to print it here using
error_repport_err() (see include/qapi/error.h) and return -1.

> +        return;
> +    }
> +
> +    g_assert(fst->tt.timers[0]);
> +    g_assert(fst->tt.timers[1]);

These are really not needed since, timers are set with:

    QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer));

and g_malloc0() never returns NULL when passed a non-nul size. It calls
g_assert() internally instead.

> +    fst->pending_reqs[0] = 0;
> +    fst->pending_reqs[1] = 0;
> +    fst->any_timer_armed[0] = false;
> +    fst->any_timter_armed[1] = false;
> +    qemu_mutex_init(&fst->lock);

And there you may set the enabled flag.

> +}
> +
> +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t bytes)
> +{
> +    if (fst->io_limits_enabled) {

throttle9p_enabled(fst)

> +        qemu_mutex_lock(&fst->lock);
> +        bool must_wait = throttle9p_check_for_wait(fst, is_write);
> +        if (must_wait || fst->pending_reqs[is_write]) {
> +            fst->pending_reqs[is_write]++;
> +            qemu_mutex_unlock(&fst->lock);
> +            qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> +            qemu_mutex_lock(&fst->lock);
> +            fst->pending_reqs[is_write]--;
> +       }
> +       throttle_account(&fst->ts, is_write, bytes);
> +       throttle9p_schedule_next_request(fst, is_write);
> +       qemu_mutex_unlock(&fst->lock);
> +    }
> +}
> +
> +void throttle9p_cleanup(FsThrottle *fst)
> +{
> +    throttle_timers_destroy(&fst->tt);
> +    qemu_mutex_destroy(&fst->lock);
> +}
> diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h
> new file mode 100644
> index 0000000..0f7551d
> --- /dev/null
> +++ b/hw/9pfs/9p-throttle.h
> @@ -0,0 +1,46 @@
> +/*
> + * 9P Throttle
> + *
> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.
> + *
> + * See the COPYING file in the top-level directory for details.
> + *
> + */
> +
> +#ifndef _9P_THROTTLE_H
> +#define _9P_THROTTLE_H
> +
> +#include <stdbool.h>
> +#include <stdint.h>

These includes are forbidden. They are already handled by "qemu/osdep.h" which
is supposed to be included by all .c files.

> +#include "block/aio.h"
> +#include "qemu/main-loop.h"
> +#include "qemu/coroutine.h"
> +#include "qemu/throttle.h"
> +
> +typedef struct FsThrottle {
> +    ThrottleState ts;
> +    ThrottleTimers tt;
> +    AioContext   *aioctx;
> +    ThrottleConfig cfg;
> +    bool io_limits_enabled;

Let's simply call this enabled.

> +    CoQueue      throttled_reqs[2];
> +    unsigned     pending_reqs[2];
> +    bool any_timer_armed[2];
> +    QemuMutex lock;
> +} FsThrottle;
> +
> +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *);
> +
> +bool throttle9p_get_io_limits_state(FsThrottle *);
> +
> +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *);
> +
> +void throttle9p_request(FsThrottle *, bool , ssize_t);
> +
> +void throttle9p_cleanup(FsThrottle *);
> +#endif /* _9P_THROTTLE_H */
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index dfe293d..7be926a 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
>          error_setg(errp, "share path %s is not a directory", fse->path);
>          goto out;
>      }
> +
> +    /* Throttle structure initialization */

Not sure this comment is helpful.

> +    s->ctx.fst = &fse->fst;
> +
>      v9fs_path_free(&path);
>  
>      rc = 0;
> @@ -3504,6 +3508,9 @@ out:
>  
>  void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
>  {
> +    if (throttle9p_get_io_limits_state(s->ctx.fst)) {
> +        throttle9p_cleanup(s->ctx.fst);
> +    }
>      g_free(s->ctx.fs_root);
>      g_free(s->tag);
>  }
> diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
> index da0ae0c..07523f1 100644
> --- a/hw/9pfs/Makefile.objs
> +++ b/hw/9pfs/Makefile.objs
> @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o
>  common-obj-y += coxattr.o 9p-synth.o
>  common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o
>  common-obj-y += 9p-proxy.o
> +common-obj-y += 9p-throttle.o
>  
>  obj-y += virtio-9p-device.o

On Fri, 9 Sep 2016 17:29:16 +0200
Greg Kurz <groug@kaod.org> wrote:
> > +bool throttle9p_get_io_limits_state(FsThrottle *fst)  
> 
> The name looks a bit strange, since this helper simply returns a boolean flag.
> I guess throttle9p_enabled() is enough.
> 
> > +{
> > +
> > +    return fst->io_limits_enabled;
> > +}
> > +

And this could be an inline function in 9p-throttle.h.

> [...]
> > +    if (!throttle_is_valid(&fst->cfg, NULL)) {  
> 
> You should pass an Error * to throttle_is_valid() to be able to report the

Read Error ** :)

> misconfiguration to the user. I guess it is okay to print it here using
> error_repport_err() (see include/qapi/error.h) and return -1.
> 

Cheers.

--
Greg

On Fri 09 Sep 2016 05:29:16 PM CEST, Greg Kurz wrote:
> On Fri,  9 Sep 2016 05:10:27 -0400
> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:
>
>> Uses throttling APIs to limit I/O bandwidth and number of operations on the 
>> devices which use 9p-local driver.
>> 
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> ---
>
> Hi Pradeep,
>
> Please find some remarks below. I haven't dived deep enough to see if this
> actually works. Maybe Berto can provide some feedback ?

I'm planning to, I've just been quite busy lately, but you'll get my
feedback.

Thanks,

Berto

Hi Greg,

Thanks for looking into the patch. Please look at the replies inline.

Regards,
Pradeep

-----Original Message-----
From: Greg Kurz [mailto:groug@kaod.org] 
Sent: Friday, September 09, 2016 5:29 PM
To: Pradeep Jagadeesh
Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver

On Fri,  9 Sep 2016 05:10:27 -0400
Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:

> Uses throttling APIs to limit I/O bandwidth and number of operations 
> on the devices which use 9p-local driver.
> 
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---

Hi Pradeep,

Please find some remarks below. I haven't dived deep enough to see if this actually works. Maybe Berto can provide some feedback ?

Cheers.

--
Greg

>  fsdev/file-op-9p.h      |   3 +
>  fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
>  hw/9pfs/9p-local.c      |  18 ++++-
>  hw/9pfs/9p-throttle.c   | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/9pfs/9p-throttle.h   |  46 +++++++++++
>  hw/9pfs/9p.c            |   7 ++
>  hw/9pfs/Makefile.objs   |   1 +
>  7 files changed, 326 insertions(+), 2 deletions(-)  create mode 
> 100644 hw/9pfs/9p-throttle.c  create mode 100644 hw/9pfs/9p-throttle.h
> 
> 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.
> 
> v1 -> v2:
> 
> -Fixed FsContext redeclaration issue
> -Removed couple of function declarations from 9p-throttle.h -Fixed 
> some of the .help messages
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index 
> 6db9fea..e86b91a 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -17,6 +17,7 @@
>  #include <dirent.h>
>  #include <utime.h>
>  #include <sys/vfs.h>
> +#include "hw/9pfs/9p-throttle.h"
>  
>  #define SM_LOCAL_MODE_BITS    0600
>  #define SM_LOCAL_DIR_MODE_BITS    0700
> @@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
>      char *path;
>      int export_flags;
>      FileOperations *ops;
> +    FsThrottle fst;
>  } FsDriverEntry;
>  
>  typedef struct FsContext
> @@ -83,6 +85,7 @@ typedef struct FsContext
>      int export_flags;
>      struct xattr_operations **xops;
>      struct extended_ops exops;
> +    FsThrottle *fst;
>      /* fs driver specific data */
>      void *private;
>  } FsContext;
> diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c index 
> 1dd8c7a..2774855 100644
> --- a/fsdev/qemu-fsdev-opts.c
> +++ b/fsdev/qemu-fsdev-opts.c
> @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
>          }, {
>              .name = "sock_fd",
>              .type = QEMU_OPT_NUMBER,
> +        }, {
> +            .name = "",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit total bytes per second",
> +        }, {
> +            .name = "bps_rd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit read bytes per second",
> +        }, {
> +            .name = "bps_wr",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit write bytes per second",
> +        }, {
> +            .name = "iops",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit total io operations per second",
> +        }, {
> +            .name = "iops_rd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit read operations per second ",
> +        }, {
> +            .name = "iops_wr",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit write operations per second",
> +        }, {
> +            .name = "bps_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes burst",
> +        }, {
> +            .name = "bps_rd_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes read burst",
> +        }, {
> +            .name = "bps_wr_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes write burst",
> +        }, {
> +            .name = "iops_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations burst",
> +        }, {
> +            .name = "iops_rd_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations read burst",
> +        }, {
> +            .name = "iops_wr_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations write burst",
> +        }, {
> +            .name = "iops_size",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "io iops-size",
>          },
>  

In case we end up sharing code with blockdev as suggested by Eric, maybe you can use the same QMP friendly naming scheme ("throttling.*") and the same help strings as well.

[Pradeep Jagadeesh] 
[Pradeep Jagadeesh] You mean to say I should re-name the options how they are done for block devices? Or mentioning
the cli options itself as throttling.*?
                                 
>          { /*End of list */ }
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 
> 3f271fc..49c2819 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
>                              const struct iovec *iov,
>                              int iovcnt, off_t offset)  {
> +    throttle9p_request(ctx->fst, false, iov->iov_len);
> +
>  #ifdef CONFIG_PREADV
>      return preadv(fs->fd, iov, iovcnt, offset);  #else @@ -436,8 
> +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
>                               const struct iovec *iov,
>                               int iovcnt, off_t offset)  {
> -    ssize_t ret
> -;
> +    ssize_t ret;
> +
> +    throttle9p_request(ctx->fst, true, iov->iov_len);
> +
>  #ifdef CONFIG_PREADV
>      ret = pwritev(fs->fd, iov, iovcnt, offset);  #else @@ -1213,6 
> +1217,9 @@ 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");
>  
> +    /* get the throttle structure */

Not sure this comment is helpful.
[Pradeep Jagadeesh] OK

> +    FsThrottle *fst = &fse->fst;
> +
>      if (!sec_model) {
>          error_report("Security model not specified, local fs needs security model");
>          error_printf("valid options are:"
> @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
>          error_report("fsdev: No path specified");
>          return -1;
>      }
> +
> +    throttle9p_enable_io_limits(opts, fst);
> +
> +    if (throttle9p_get_io_limits_state(fst)) {
> +        throttle9p_configure_iolimits(opts, fst);
> +    }
> +
>      fse->path = g_strdup(path);
>  
>      return 0;
> diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c new file 
> mode 100644 index 0000000..f2a7ba5
> --- /dev/null
> +++ b/hw/9pfs/9p-throttle.c
> @@ -0,0 +1,201 @@
> +/*
> + * 9P Throttle
> + *
> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.
> + *
> + * See the COPYING file in the top-level directory for details.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> +#include "qemu/cutils.h"
> +#include "qemu/error-report.h"
> +#include <libgen.h>
> +#include <linux/fs.h>
> +#include <sys/ioctl.h>
> +#include <string.h>
> +#include "fsdev/file-op-9p.h"
> +#include "9p-throttle.h"
> +

Only "qemu/osdep.h", "qemu/error-report.h" and "9p-throttle.h" are actually needed.

> +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst) {
> +    const float bps = qemu_opt_get_number(opts, "bps", 0);
> +    const float iops = qemu_opt_get_number(opts, "iops", 0);
> +    const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0);
> +    const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0);
> +    const float rdops = qemu_opt_get_number(opts, "iops_rd", 0);
> +    const float wrops = qemu_opt_get_number(opts, "iops_wr", 0);
> +
> +    if (bps > 0 || iops > 0 || rdbps > 0 ||
> +        wrbps > 0 || rdops > 0 || wrops > 0) {
> +        fst->io_limits_enabled = true;
> +    } else {
> +        fst->io_limits_enabled = false;
> +    }
> +}
> +

This function should be named *_parse_* but I'm not even sure it is actually needed (see below).
[Pradeep Jagadeesh] This function is used to avoid the parsing of the options if not enabled.
From my understanding of code block devices throttling code, irrespective of the throttle options
are enabled or not the parse functions are called for all the devices. I am trying to avoid that 
by checking these variables at the before I get into parsing and setting up the throttle structures. 

> +static bool throttle9p_check_for_wait(FsThrottle *fst, bool is_write) 
> +{
> +    if (fst->any_timer_armed[is_write]) {
> +        return true;
> +    } else {
> +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> +    }
> +}
> +
> +static void throttle9p_schedule_next_request(FsThrottle *fst, bool 
> +is_write) {
> +    bool must_wait = throttle9p_check_for_wait(fst, is_write);
> +    if (!fst->pending_reqs[is_write]) {
> +        return;
> +    }
> +    if (!must_wait) {
> +        if (qemu_in_coroutine() &&
> +            qemu_co_queue_next(&fst->throttled_reqs[is_write])) {
> +            ;
> +       } else {
> +           int64_t now = qemu_clock_get_ns(fst->tt.clock_type);
> +           timer_mod(fst->tt.timers[is_write], now + 1);
> +           fst->any_timer_armed[is_write] = true;
> +       }
> +   }
> +}
> +
> +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write) {
> +    bool empty_queue;
> +    qemu_mutex_lock(&fst->lock);
> +    fst->any_timer_armed[is_write] = false;
> +    qemu_mutex_unlock(&fst->lock);
> +    empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]);
> +    if (empty_queue) {
> +        qemu_mutex_lock(&fst->lock);
> +        throttle9p_schedule_next_request(fst, is_write);
> +        qemu_mutex_unlock(&fst->lock);
> +    }
> +}
> +
> +
> +bool throttle9p_get_io_limits_state(FsThrottle *fst)

The name looks a bit strange, since this helper simply returns a boolean flag.
I guess throttle9p_enabled() is enough.
[Pradeep Jagadeesh] OK

> +{
> +
> +    return fst->io_limits_enabled;
> +}
> +
> +static void throttle9p_read_timer_cb(void *opaque) {
> +    throttle9p_timer_cb(opaque, false); }
> +
> +static void throttle9p_write_timer_cb(void *opaque) {
> +    throttle9p_timer_cb(opaque, true); }
> +
> +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle *fst) {

This function can fail, it should have a return value (0 or -1).
[Pradeep Jagadeesh] OK

> +    memset(&fst->ts, 1, sizeof(fst->ts));
> +    memset(&fst->tt, 1, sizeof(fst->tt));
> +    memset(&fst->cfg, 0, sizeof(fst->cfg));

Same remark as Claudio.
[Pradeep Jagadeesh] OK, will address.

> +    fst->aioctx = qemu_get_aio_context();
> +
> +    if (!fst->aioctx) {
> +        error_report("Failed to create AIO Context");
> +        exit(1);
> +    }
> +    throttle_init(&fst->ts);
> +    throttle_timers_init(&fst->tt,
> +                         fst->aioctx,
> +                         QEMU_CLOCK_REALTIME,
> +                         throttle9p_read_timer_cb,
> +                         throttle9p_write_timer_cb,
> +                         fst);

Shouldn't all this be done later when we know the config is valid ?
[Pradeep Jagadeesh] Yes, fixed.

> +    throttle_config_init(&fst->cfg);
> +    g_assert(throttle_is_valid(&fst->cfg, NULL));
> +

What's the point in calling throttle_is_valid() on a freshly initialized config ?
[Pradeep Jagadeesh] Removed

> +    qemu_co_queue_init(&fst->throttled_reqs[0]);
> +    qemu_co_queue_init(&fst->throttled_reqs[1]);

Later, when the config is valid ?
[Pradeep Jagadeesh] Done

> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> +          qemu_opt_get_number(opts, "bps", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
> +          qemu_opt_get_number(opts, "bps_rd", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> +          qemu_opt_get_number(opts, "bps_wr", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> +          qemu_opt_get_number(opts, "iops", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> +          qemu_opt_get_number(opts, "iops_rd", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> +          qemu_opt_get_number(opts, "iops_wr", 0);
> +
> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> +          qemu_opt_get_number(opts, "bps_max", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
> +          qemu_opt_get_number(opts, "bps_rd_max", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> +          qemu_opt_get_number(opts, "bps_wr_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> +          qemu_opt_get_number(opts, "iops_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].max =
> +          qemu_opt_get_number(opts, "iops_rd_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> +          qemu_opt_get_number(opts, "iops_wr_max", 0);
> +
> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
> +          qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].burst_length  =
> +          qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
> +          qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
> +          qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
> +          qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
> +          qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
> +    fst->cfg.op_size =
> +          qemu_opt_get_number(opts, "iops_size", 0);
> +
> +    throttle_config(&fst->ts, &fst->tt, &fst->cfg);

Let's set the config later, when we we it is valid.
[Pradeep Jagadeesh] OK

> +    if (!throttle_is_valid(&fst->cfg, NULL)) {

You should pass an Error * to throttle_is_valid() to be able to report the misconfiguration to the user. I guess it is okay to print it here using
error_repport_err() (see include/qapi/error.h) and return -1.
[Pradeep Jagadeesh] OK

> +        return;
> +    }
> +
> +    g_assert(fst->tt.timers[0]);
> +    g_assert(fst->tt.timers[1]);

These are really not needed since, timers are set with:

    QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer));

and g_malloc0() never returns NULL when passed a non-nul size. It calls
g_assert() internally instead.
[Pradeep Jagadeesh] OK

> +    fst->pending_reqs[0] = 0;
> +    fst->pending_reqs[1] = 0;
> +    fst->any_timer_armed[0] = false;
> +    fst->any_timter_armed[1] = false;
> +    qemu_mutex_init(&fst->lock);

And there you may set the enabled flag.
[Pradeep Jagadeesh] Explained before.

> +}
> +
> +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t 
> +bytes) {
> +    if (fst->io_limits_enabled) {

throttle9p_enabled(fst)
[Pradeep Jagadeesh] OK

> +        qemu_mutex_lock(&fst->lock);
> +        bool must_wait = throttle9p_check_for_wait(fst, is_write);
> +        if (must_wait || fst->pending_reqs[is_write]) {
> +            fst->pending_reqs[is_write]++;
> +            qemu_mutex_unlock(&fst->lock);
> +            qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> +            qemu_mutex_lock(&fst->lock);
> +            fst->pending_reqs[is_write]--;
> +       }
> +       throttle_account(&fst->ts, is_write, bytes);
> +       throttle9p_schedule_next_request(fst, is_write);
> +       qemu_mutex_unlock(&fst->lock);
> +    }
> +}
> +
> +void throttle9p_cleanup(FsThrottle *fst) {
> +    throttle_timers_destroy(&fst->tt);
> +    qemu_mutex_destroy(&fst->lock);
> +}
> diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h new file 
> mode 100644 index 0000000..0f7551d
> --- /dev/null
> +++ b/hw/9pfs/9p-throttle.h
> @@ -0,0 +1,46 @@
> +/*
> + * 9P Throttle
> + *
> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.
> + *
> + * See the COPYING file in the top-level directory for details.
> + *
> + */
> +
> +#ifndef _9P_THROTTLE_H
> +#define _9P_THROTTLE_H
> +
> +#include <stdbool.h>
> +#include <stdint.h>

These includes are forbidden. They are already handled by "qemu/osdep.h" which is supposed to be included by all .c files.
[Pradeep Jagadeesh] OK

> +#include "block/aio.h"
> +#include "qemu/main-loop.h"
> +#include "qemu/coroutine.h"
> +#include "qemu/throttle.h"
> +
> +typedef struct FsThrottle {
> +    ThrottleState ts;
> +    ThrottleTimers tt;
> +    AioContext   *aioctx;
> +    ThrottleConfig cfg;
> +    bool io_limits_enabled;

Let's simply call this enabled.
[Pradeep Jagadeesh] OK

> +    CoQueue      throttled_reqs[2];
> +    unsigned     pending_reqs[2];
> +    bool any_timer_armed[2];
> +    QemuMutex lock;
> +} FsThrottle;
> +
> +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *);
> +
> +bool throttle9p_get_io_limits_state(FsThrottle *);
> +
> +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *);
> +
> +void throttle9p_request(FsThrottle *, bool , ssize_t);
> +
> +void throttle9p_cleanup(FsThrottle *); #endif /* _9P_THROTTLE_H */
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index dfe293d..7be926a 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
>          error_setg(errp, "share path %s is not a directory", fse->path);
>          goto out;
>      }
> +
> +    /* Throttle structure initialization */

Not sure this comment is helpful.
[Pradeep Jagadeesh] Just to give a hint, where the throttle structures are getting
Populated. May be some other comment?

> +    s->ctx.fst = &fse->fst;
> +
>      v9fs_path_free(&path);
>  
>      rc = 0;
> @@ -3504,6 +3508,9 @@ out:
>  
>  void v9fs_device_unrealize_common(V9fsState *s, Error **errp)  {
> +    if (throttle9p_get_io_limits_state(s->ctx.fst)) {
> +        throttle9p_cleanup(s->ctx.fst);
> +    }
>      g_free(s->ctx.fs_root);
>      g_free(s->tag);
>  }
> diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs index 
> da0ae0c..07523f1 100644
> --- a/hw/9pfs/Makefile.objs
> +++ b/hw/9pfs/Makefile.objs
> @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o  
> common-obj-y += coxattr.o 9p-synth.o
>  common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o  common-obj-y += 
> 9p-proxy.o
> +common-obj-y += 9p-throttle.o
>  
>  obj-y += virtio-9p-device.o

On Mon, 12 Sep 2016 12:52:55 +0000
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:

> Hi Greg,
> 
> Thanks for looking into the patch. Please look at the replies inline.
> 
> Regards,
> Pradeep
> 

Hi Pradeep,

Remarks and answers below.

Cheers.

--
Greg

> -----Original Message-----
> From: Greg Kurz [mailto:groug@kaod.org] 
> Sent: Friday, September 09, 2016 5:29 PM
> To: Pradeep Jagadeesh
> Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver
> 
> On Fri,  9 Sep 2016 05:10:27 -0400
> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:
> 
> > Uses throttling APIs to limit I/O bandwidth and number of operations 
> > on the devices which use 9p-local driver.
> > 
> > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > ---  
> 
> Hi Pradeep,
> 
> Please find some remarks below. I haven't dived deep enough to see if this actually works. Maybe Berto can provide some feedback ?
> 
> Cheers.
> 
> --
> Greg
> 
> >  fsdev/file-op-9p.h      |   3 +
> >  fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
> >  hw/9pfs/9p-local.c      |  18 ++++-
> >  hw/9pfs/9p-throttle.c   | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/9pfs/9p-throttle.h   |  46 +++++++++++

I forgot to mention it, but this throttling implementation isn't related to
the 9P device but to the fsdev device: 9p-throttle.[ch] should move to the
fsdev directory and be renamed to fsdev-throttle.[ch].

> >  hw/9pfs/9p.c            |   7 ++
> >  hw/9pfs/Makefile.objs   |   1 +
> >  7 files changed, 326 insertions(+), 2 deletions(-)  create mode 
> > 100644 hw/9pfs/9p-throttle.c  create mode 100644 hw/9pfs/9p-throttle.h
> > 
> > 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.
> > 
> > v1 -> v2:
> > 
> > -Fixed FsContext redeclaration issue
> > -Removed couple of function declarations from 9p-throttle.h -Fixed 
> > some of the .help messages
> > 
> > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index 
> > 6db9fea..e86b91a 100644
> > --- a/fsdev/file-op-9p.h
> > +++ b/fsdev/file-op-9p.h
> > @@ -17,6 +17,7 @@
> >  #include <dirent.h>
> >  #include <utime.h>
> >  #include <sys/vfs.h>
> > +#include "hw/9pfs/9p-throttle.h"
> >  
> >  #define SM_LOCAL_MODE_BITS    0600
> >  #define SM_LOCAL_DIR_MODE_BITS    0700
> > @@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
> >      char *path;
> >      int export_flags;
> >      FileOperations *ops;
> > +    FsThrottle fst;
> >  } FsDriverEntry;
> >  
> >  typedef struct FsContext
> > @@ -83,6 +85,7 @@ typedef struct FsContext
> >      int export_flags;
> >      struct xattr_operations **xops;
> >      struct extended_ops exops;
> > +    FsThrottle *fst;
> >      /* fs driver specific data */
> >      void *private;
> >  } FsContext;
> > diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c index 
> > 1dd8c7a..2774855 100644
> > --- a/fsdev/qemu-fsdev-opts.c
> > +++ b/fsdev/qemu-fsdev-opts.c
> > @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
> >          }, {
> >              .name = "sock_fd",
> >              .type = QEMU_OPT_NUMBER,
> > +        }, {
> > +            .name = "",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit total bytes per second",
> > +        }, {
> > +            .name = "bps_rd",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit read bytes per second",
> > +        }, {
> > +            .name = "bps_wr",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit write bytes per second",
> > +        }, {
> > +            .name = "iops",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit total io operations per second",
> > +        }, {
> > +            .name = "iops_rd",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit read operations per second ",
> > +        }, {
> > +            .name = "iops_wr",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit write operations per second",
> > +        }, {
> > +            .name = "bps_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum bytes burst",
> > +        }, {
> > +            .name = "bps_rd_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum bytes read burst",
> > +        }, {
> > +            .name = "bps_wr_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum bytes write burst",
> > +        }, {
> > +            .name = "iops_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum io operations burst",
> > +        }, {
> > +            .name = "iops_rd_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum io operations read burst",
> > +        }, {
> > +            .name = "iops_wr_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum io operations write burst",
> > +        }, {
> > +            .name = "iops_size",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "io iops-size",
> >          },
> >    
> 
> In case we end up sharing code with blockdev as suggested by Eric, maybe you can use the same QMP friendly naming scheme ("throttling.*") and the same help strings as well.
> 
> [Pradeep Jagadeesh] 
> [Pradeep Jagadeesh] You mean to say I should re-name the options how they are done for block devices? Or mentioning
> the cli options itself as throttling.*?
>                                  

I meant the latter. IIUC, the renaming done in blockdev is for compatibility
only, as stated in the changelog of commit 57975222b6.

> >          { /*End of list */ }
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 
> > 3f271fc..49c2819 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
> >                              const struct iovec *iov,
> >                              int iovcnt, off_t offset)  {
> > +    throttle9p_request(ctx->fst, false, iov->iov_len);
> > +
> >  #ifdef CONFIG_PREADV
> >      return preadv(fs->fd, iov, iovcnt, offset);  #else @@ -436,8 
> > +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
> >                               const struct iovec *iov,
> >                               int iovcnt, off_t offset)  {
> > -    ssize_t ret
> > -;
> > +    ssize_t ret;
> > +
> > +    throttle9p_request(ctx->fst, true, iov->iov_len);
> > +
> >  #ifdef CONFIG_PREADV
> >      ret = pwritev(fs->fd, iov, iovcnt, offset);  #else @@ -1213,6 
> > +1217,9 @@ 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");
> >  
> > +    /* get the throttle structure */  
> 
> Not sure this comment is helpful.
> [Pradeep Jagadeesh] OK
> 
> > +    FsThrottle *fst = &fse->fst;
> > +
> >      if (!sec_model) {
> >          error_report("Security model not specified, local fs needs security model");
> >          error_printf("valid options are:"
> > @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
> >          error_report("fsdev: No path specified");
> >          return -1;
> >      }
> > +
> > +    throttle9p_enable_io_limits(opts, fst);
> > +
> > +    if (throttle9p_get_io_limits_state(fst)) {
> > +        throttle9p_configure_iolimits(opts, fst);
> > +    }
> > +
> >      fse->path = g_strdup(path);
> >  
> >      return 0;
> > diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c new file 
> > mode 100644 index 0000000..f2a7ba5
> > --- /dev/null
> > +++ b/hw/9pfs/9p-throttle.c
> > @@ -0,0 +1,201 @@
> > +/*
> > + * 9P Throttle
> > + *
> > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > + * (at your option) any later version.
> > + *
> > + * See the COPYING file in the top-level directory for details.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> > +#include "qemu/cutils.h"
> > +#include "qemu/error-report.h"
> > +#include <libgen.h>
> > +#include <linux/fs.h>
> > +#include <sys/ioctl.h>
> > +#include <string.h>
> > +#include "fsdev/file-op-9p.h"
> > +#include "9p-throttle.h"
> > +  
> 
> Only "qemu/osdep.h", "qemu/error-report.h" and "9p-throttle.h" are actually needed.
> 
> > +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst) {
> > +    const float bps = qemu_opt_get_number(opts, "bps", 0);
> > +    const float iops = qemu_opt_get_number(opts, "iops", 0);
> > +    const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0);
> > +    const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0);
> > +    const float rdops = qemu_opt_get_number(opts, "iops_rd", 0);
> > +    const float wrops = qemu_opt_get_number(opts, "iops_wr", 0);
> > +
> > +    if (bps > 0 || iops > 0 || rdbps > 0 ||
> > +        wrbps > 0 || rdops > 0 || wrops > 0) {
> > +        fst->io_limits_enabled = true;
> > +    } else {
> > +        fst->io_limits_enabled = false;
> > +    }
> > +}
> > +  
> 
> This function should be named *_parse_* but I'm not even sure it is actually needed (see below).
> [Pradeep Jagadeesh] This function is used to avoid the parsing of the options if not enabled.
> From my understanding of code block devices throttling code, irrespective of the throttle options
> are enabled or not the parse functions are called for all the devices. I am trying to avoid that 
> by checking these variables at the before I get into parsing and setting up the throttle structures. 
> 

And so, we would do a pre-parsing here before doing a full fledged parsing later ?
This still doesn't look very useful, but it is certainly error prone IMHO.

However, conditionally setting up the throttle structures and letting the
backend know that throttling is enabled, are needed indeed.

Looking again at how block devices use the throttle API, I see that there's:

/* Does any throttling must be done
 *
 * @cfg: the throttling configuration to inspect
 * @ret: true if throttling must be done else false
 */
bool throttle_enabled(ThrottleConfig *cfg)
{
    int i;

    for (i = 0; i < BUCKETS_COUNT; i++) {
        if (cfg->buckets[i].avg > 0) {
            return true;
        }
    }

    return false;
}

... which does the same checks, and is used to actually enable the throttling.

You could use the above helper to set/unset @io_limits_enabled after the
real parsing is done.

> > +static bool throttle9p_check_for_wait(FsThrottle *fst, bool is_write) 
> > +{
> > +    if (fst->any_timer_armed[is_write]) {
> > +        return true;
> > +    } else {
> > +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> > +    }
> > +}
> > +
> > +static void throttle9p_schedule_next_request(FsThrottle *fst, bool 
> > +is_write) {
> > +    bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > +    if (!fst->pending_reqs[is_write]) {
> > +        return;
> > +    }
> > +    if (!must_wait) {
> > +        if (qemu_in_coroutine() &&
> > +            qemu_co_queue_next(&fst->throttled_reqs[is_write])) {
> > +            ;
> > +       } else {
> > +           int64_t now = qemu_clock_get_ns(fst->tt.clock_type);
> > +           timer_mod(fst->tt.timers[is_write], now + 1);
> > +           fst->any_timer_armed[is_write] = true;
> > +       }
> > +   }
> > +}
> > +
> > +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write) {
> > +    bool empty_queue;
> > +    qemu_mutex_lock(&fst->lock);
> > +    fst->any_timer_armed[is_write] = false;
> > +    qemu_mutex_unlock(&fst->lock);
> > +    empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]);
> > +    if (empty_queue) {
> > +        qemu_mutex_lock(&fst->lock);
> > +        throttle9p_schedule_next_request(fst, is_write);
> > +        qemu_mutex_unlock(&fst->lock);
> > +    }
> > +}
> > +
> > +
> > +bool throttle9p_get_io_limits_state(FsThrottle *fst)  
> 
> The name looks a bit strange, since this helper simply returns a boolean flag.
> I guess throttle9p_enabled() is enough.
> [Pradeep Jagadeesh] OK
> 
> > +{
> > +
> > +    return fst->io_limits_enabled;
> > +}
> > +
> > +static void throttle9p_read_timer_cb(void *opaque) {
> > +    throttle9p_timer_cb(opaque, false); }
> > +
> > +static void throttle9p_write_timer_cb(void *opaque) {
> > +    throttle9p_timer_cb(opaque, true); }
> > +
> > +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle *fst) {  
> 
> This function can fail, it should have a return value (0 or -1).
> [Pradeep Jagadeesh] OK
> 
> > +    memset(&fst->ts, 1, sizeof(fst->ts));
> > +    memset(&fst->tt, 1, sizeof(fst->tt));
> > +    memset(&fst->cfg, 0, sizeof(fst->cfg));  
> 
> Same remark as Claudio.
> [Pradeep Jagadeesh] OK, will address.
> 
> > +    fst->aioctx = qemu_get_aio_context();
> > +
> > +    if (!fst->aioctx) {
> > +        error_report("Failed to create AIO Context");
> > +        exit(1);
> > +    }
> > +    throttle_init(&fst->ts);
> > +    throttle_timers_init(&fst->tt,
> > +                         fst->aioctx,
> > +                         QEMU_CLOCK_REALTIME,
> > +                         throttle9p_read_timer_cb,
> > +                         throttle9p_write_timer_cb,
> > +                         fst);  
> 
> Shouldn't all this be done later when we know the config is valid ?
> [Pradeep Jagadeesh] Yes, fixed.
> 
> > +    throttle_config_init(&fst->cfg);
> > +    g_assert(throttle_is_valid(&fst->cfg, NULL));
> > +  
> 
> What's the point in calling throttle_is_valid() on a freshly initialized config ?
> [Pradeep Jagadeesh] Removed
> 
> > +    qemu_co_queue_init(&fst->throttled_reqs[0]);
> > +    qemu_co_queue_init(&fst->throttled_reqs[1]);  
> 
> Later, when the config is valid ?
> [Pradeep Jagadeesh] Done
> 
> > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> > +          qemu_opt_get_number(opts, "bps", 0);
> > +    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
> > +          qemu_opt_get_number(opts, "bps_rd", 0);
> > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> > +          qemu_opt_get_number(opts, "bps_wr", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> > +          qemu_opt_get_number(opts, "iops", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> > +          qemu_opt_get_number(opts, "iops_rd", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> > +          qemu_opt_get_number(opts, "iops_wr", 0);
> > +
> > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> > +          qemu_opt_get_number(opts, "bps_max", 0);
> > +    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
> > +          qemu_opt_get_number(opts, "bps_rd_max", 0);
> > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> > +          qemu_opt_get_number(opts, "bps_wr_max", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> > +          qemu_opt_get_number(opts, "iops_max", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_READ].max =
> > +          qemu_opt_get_number(opts, "iops_rd_max", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> > +          qemu_opt_get_number(opts, "iops_wr_max", 0);
> > +
> > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
> > +          qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
> > +    fst->cfg.buckets[THROTTLE_BPS_READ].burst_length  =
> > +          qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
> > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
> > +          qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
> > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
> > +          qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
> > +    fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
> > +          qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
> > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
> > +          qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
> > +    fst->cfg.op_size =
> > +          qemu_opt_get_number(opts, "iops_size", 0);
> > +
> > +    throttle_config(&fst->ts, &fst->tt, &fst->cfg);  
> 
> Let's set the config later, when we we it is valid.
> [Pradeep Jagadeesh] OK
> 
> > +    if (!throttle_is_valid(&fst->cfg, NULL)) {  
> 
> You should pass an Error * to throttle_is_valid() to be able to report the misconfiguration to the user. I guess it is okay to print it here using
> error_repport_err() (see include/qapi/error.h) and return -1.
> [Pradeep Jagadeesh] OK
> 
> > +        return;
> > +    }
> > +
> > +    g_assert(fst->tt.timers[0]);
> > +    g_assert(fst->tt.timers[1]);  
> 
> These are really not needed since, timers are set with:
> 
>     QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer));
> 
> and g_malloc0() never returns NULL when passed a non-nul size. It calls
> g_assert() internally instead.
> [Pradeep Jagadeesh] OK
> 
> > +    fst->pending_reqs[0] = 0;
> > +    fst->pending_reqs[1] = 0;
> > +    fst->any_timer_armed[0] = false;
> > +    fst->any_timter_armed[1] = false;
> > +    qemu_mutex_init(&fst->lock);  
> 
> And there you may set the enabled flag.
> [Pradeep Jagadeesh] Explained before.
> 
> > +}
> > +
> > +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t 
> > +bytes) {
> > +    if (fst->io_limits_enabled) {  
> 
> throttle9p_enabled(fst)
> [Pradeep Jagadeesh] OK
> 
> > +        qemu_mutex_lock(&fst->lock);
> > +        bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > +        if (must_wait || fst->pending_reqs[is_write]) {
> > +            fst->pending_reqs[is_write]++;
> > +            qemu_mutex_unlock(&fst->lock);
> > +            qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> > +            qemu_mutex_lock(&fst->lock);
> > +            fst->pending_reqs[is_write]--;
> > +       }
> > +       throttle_account(&fst->ts, is_write, bytes);
> > +       throttle9p_schedule_next_request(fst, is_write);
> > +       qemu_mutex_unlock(&fst->lock);
> > +    }
> > +}
> > +
> > +void throttle9p_cleanup(FsThrottle *fst) {
> > +    throttle_timers_destroy(&fst->tt);
> > +    qemu_mutex_destroy(&fst->lock);
> > +}
> > diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h new file 
> > mode 100644 index 0000000..0f7551d
> > --- /dev/null
> > +++ b/hw/9pfs/9p-throttle.h
> > @@ -0,0 +1,46 @@
> > +/*
> > + * 9P Throttle
> > + *
> > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > + * (at your option) any later version.
> > + *
> > + * See the COPYING file in the top-level directory for details.
> > + *
> > + */
> > +
> > +#ifndef _9P_THROTTLE_H
> > +#define _9P_THROTTLE_H
> > +
> > +#include <stdbool.h>
> > +#include <stdint.h>  
> 
> These includes are forbidden. They are already handled by "qemu/osdep.h" which is supposed to be included by all .c files.
> [Pradeep Jagadeesh] OK
> 
> > +#include "block/aio.h"
> > +#include "qemu/main-loop.h"
> > +#include "qemu/coroutine.h"
> > +#include "qemu/throttle.h"
> > +
> > +typedef struct FsThrottle {
> > +    ThrottleState ts;
> > +    ThrottleTimers tt;
> > +    AioContext   *aioctx;
> > +    ThrottleConfig cfg;
> > +    bool io_limits_enabled;  
> 
> Let's simply call this enabled.
> [Pradeep Jagadeesh] OK
> 
> > +    CoQueue      throttled_reqs[2];
> > +    unsigned     pending_reqs[2];
> > +    bool any_timer_armed[2];
> > +    QemuMutex lock;
> > +} FsThrottle;
> > +
> > +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *);
> > +
> > +bool throttle9p_get_io_limits_state(FsThrottle *);
> > +
> > +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *);
> > +
> > +void throttle9p_request(FsThrottle *, bool , ssize_t);
> > +
> > +void throttle9p_cleanup(FsThrottle *); #endif /* _9P_THROTTLE_H */
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index dfe293d..7be926a 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
> >          error_setg(errp, "share path %s is not a directory", fse->path);
> >          goto out;
> >      }
> > +
> > +    /* Throttle structure initialization */  
> 
> Not sure this comment is helpful.
> [Pradeep Jagadeesh] Just to give a hint, where the throttle structures are getting
> Populated. May be some other comment?
> 

Unless I'm missing something, the throttle structures aren't getting populated
here...

> > +    s->ctx.fst = &fse->fst;
> > +
> >      v9fs_path_free(&path);
> >  
> >      rc = 0;
> > @@ -3504,6 +3508,9 @@ out:
> >  
> >  void v9fs_device_unrealize_common(V9fsState *s, Error **errp)  {
> > +    if (throttle9p_get_io_limits_state(s->ctx.fst)) {
> > +        throttle9p_cleanup(s->ctx.fst);
> > +    }
> >      g_free(s->ctx.fs_root);
> >      g_free(s->tag);
> >  }
> > diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs index 
> > da0ae0c..07523f1 100644
> > --- a/hw/9pfs/Makefile.objs
> > +++ b/hw/9pfs/Makefile.objs
> > @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o  
> > common-obj-y += coxattr.o 9p-synth.o
> >  common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o  common-obj-y += 
> > 9p-proxy.o
> > +common-obj-y += 9p-throttle.o
> >  
> >  obj-y += virtio-9p-device.o  
>

Replies inline Greg.

Thanks & Regards,
Pradeep

-----Original Message-----
From: Greg Kurz [mailto:groug@kaod.org] 
Sent: Monday, September 12, 2016 4:19 PM
To: Pradeep Jagadeesh
Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver

On Mon, 12 Sep 2016 12:52:55 +0000
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:

> Hi Greg,
> 
> Thanks for looking into the patch. Please look at the replies inline.
> 
> Regards,
> Pradeep
> 

Hi Pradeep,

Remarks and answers below.

Cheers.

--
Greg

> -----Original Message-----
> From: Greg Kurz [mailto:groug@kaod.org]
> Sent: Friday, September 09, 2016 5:29 PM
> To: Pradeep Jagadeesh
> Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; 
> qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local 
> driver
> 
> On Fri,  9 Sep 2016 05:10:27 -0400
> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:
> 
> > Uses throttling APIs to limit I/O bandwidth and number of operations 
> > on the devices which use 9p-local driver.
> > 
> > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > ---
> 
> Hi Pradeep,
> 
> Please find some remarks below. I haven't dived deep enough to see if this actually works. Maybe Berto can provide some feedback ?
> 
> Cheers.
> 
> --
> Greg
> 
> >  fsdev/file-op-9p.h      |   3 +
> >  fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
> >  hw/9pfs/9p-local.c      |  18 ++++-
> >  hw/9pfs/9p-throttle.c   | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/9pfs/9p-throttle.h   |  46 +++++++++++

I forgot to mention it, but this throttling implementation isn't related to the 9P device but to the fsdev device: 9p-throttle.[ch] should move to the fsdev directory and be renamed to fsdev-throttle.[ch].

[Pradeep Jagadeesh] Absolutely right and make sense, I will move the throttle code to fsdev directory.

> >  hw/9pfs/9p.c            |   7 ++
> >  hw/9pfs/Makefile.objs   |   1 +
> >  7 files changed, 326 insertions(+), 2 deletions(-)  create mode
> > 100644 hw/9pfs/9p-throttle.c  create mode 100644 
> > hw/9pfs/9p-throttle.h
> > 
> > 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.
> > 
> > v1 -> v2:
> > 
> > -Fixed FsContext redeclaration issue -Removed couple of function 
> > declarations from 9p-throttle.h -Fixed some of the .help messages
> > 
> > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index 
> > 6db9fea..e86b91a 100644
> > --- a/fsdev/file-op-9p.h
> > +++ b/fsdev/file-op-9p.h
> > @@ -17,6 +17,7 @@
> >  #include <dirent.h>
> >  #include <utime.h>
> >  #include <sys/vfs.h>
> > +#include "hw/9pfs/9p-throttle.h"
> >  
> >  #define SM_LOCAL_MODE_BITS    0600
> >  #define SM_LOCAL_DIR_MODE_BITS    0700
> > @@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
> >      char *path;
> >      int export_flags;
> >      FileOperations *ops;
> > +    FsThrottle fst;
> >  } FsDriverEntry;
> >  
> >  typedef struct FsContext
> > @@ -83,6 +85,7 @@ typedef struct FsContext
> >      int export_flags;
> >      struct xattr_operations **xops;
> >      struct extended_ops exops;
> > +    FsThrottle *fst;
> >      /* fs driver specific data */
> >      void *private;
> >  } FsContext;
> > diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c index
> > 1dd8c7a..2774855 100644
> > --- a/fsdev/qemu-fsdev-opts.c
> > +++ b/fsdev/qemu-fsdev-opts.c
> > @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
> >          }, {
> >              .name = "sock_fd",
> >              .type = QEMU_OPT_NUMBER,
> > +        }, {
> > +            .name = "",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit total bytes per second",
> > +        }, {
> > +            .name = "bps_rd",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit read bytes per second",
> > +        }, {
> > +            .name = "bps_wr",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit write bytes per second",
> > +        }, {
> > +            .name = "iops",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit total io operations per second",
> > +        }, {
> > +            .name = "iops_rd",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit read operations per second ",
> > +        }, {
> > +            .name = "iops_wr",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "limit write operations per second",
> > +        }, {
> > +            .name = "bps_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum bytes burst",
> > +        }, {
> > +            .name = "bps_rd_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum bytes read burst",
> > +        }, {
> > +            .name = "bps_wr_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum bytes write burst",
> > +        }, {
> > +            .name = "iops_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum io operations burst",
> > +        }, {
> > +            .name = "iops_rd_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum io operations read burst",
> > +        }, {
> > +            .name = "iops_wr_max",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "maximum io operations write burst",
> > +        }, {
> > +            .name = "iops_size",
> > +            .type = QEMU_OPT_NUMBER,
> > +            .help = "io iops-size",
> >          },
> >    
> 
> In case we end up sharing code with blockdev as suggested by Eric, maybe you can use the same QMP friendly naming scheme ("throttling.*") and the same help strings as well.
> 
> [Pradeep Jagadeesh]
> [Pradeep Jagadeesh] You mean to say I should re-name the options how 
> they are done for block devices? Or mentioning the cli options itself as throttling.*?
>                                  

I meant the latter. IIUC, the renaming done in blockdev is for compatibility only, as stated in the changelog of commit 57975222b6.
[Pradeep Jagadeesh] OK, I will rename them as well. But there will be more code duplication.
May be we have to merge the code later. We have to come up with a structure to fit all different
kind of backend devices. (those who wants to use throttling apis).

> >          { /*End of list */ }
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index
> > 3f271fc..49c2819 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
> >                              const struct iovec *iov,
> >                              int iovcnt, off_t offset)  {
> > +    throttle9p_request(ctx->fst, false, iov->iov_len);
> > +
> >  #ifdef CONFIG_PREADV
> >      return preadv(fs->fd, iov, iovcnt, offset);  #else @@ -436,8
> > +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, 
> > +V9fsFidOpenState *fs,
> >                               const struct iovec *iov,
> >                               int iovcnt, off_t offset)  {
> > -    ssize_t ret
> > -;
> > +    ssize_t ret;
> > +
> > +    throttle9p_request(ctx->fst, true, iov->iov_len);
> > +
> >  #ifdef CONFIG_PREADV
> >      ret = pwritev(fs->fd, iov, iovcnt, offset);  #else @@ -1213,6
> > +1217,9 @@ 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");
> >  
> > +    /* get the throttle structure */
> 
> Not sure this comment is helpful.
> [Pradeep Jagadeesh] OK
> 
> > +    FsThrottle *fst = &fse->fst;
> > +
> >      if (!sec_model) {
> >          error_report("Security model not specified, local fs needs security model");
> >          error_printf("valid options are:"
> > @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
> >          error_report("fsdev: No path specified");
> >          return -1;
> >      }
> > +
> > +    throttle9p_enable_io_limits(opts, fst);
> > +
> > +    if (throttle9p_get_io_limits_state(fst)) {
> > +        throttle9p_configure_iolimits(opts, fst);
> > +    }
> > +
> >      fse->path = g_strdup(path);
> >  
> >      return 0;
> > diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c new file 
> > mode 100644 index 0000000..f2a7ba5
> > --- /dev/null
> > +++ b/hw/9pfs/9p-throttle.c
> > @@ -0,0 +1,201 @@
> > +/*
> > + * 9P Throttle
> > + *
> > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 
> > +or
> > + * (at your option) any later version.
> > + *
> > + * See the COPYING file in the top-level directory for details.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> > +#include "qemu/cutils.h"
> > +#include "qemu/error-report.h"
> > +#include <libgen.h>
> > +#include <linux/fs.h>
> > +#include <sys/ioctl.h>
> > +#include <string.h>
> > +#include "fsdev/file-op-9p.h"
> > +#include "9p-throttle.h"
> > +  
> 
> Only "qemu/osdep.h", "qemu/error-report.h" and "9p-throttle.h" are actually needed.
> 
> > +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst) {
> > +    const float bps = qemu_opt_get_number(opts, "bps", 0);
> > +    const float iops = qemu_opt_get_number(opts, "iops", 0);
> > +    const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0);
> > +    const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0);
> > +    const float rdops = qemu_opt_get_number(opts, "iops_rd", 0);
> > +    const float wrops = qemu_opt_get_number(opts, "iops_wr", 0);
> > +
> > +    if (bps > 0 || iops > 0 || rdbps > 0 ||
> > +        wrbps > 0 || rdops > 0 || wrops > 0) {
> > +        fst->io_limits_enabled = true;
> > +    } else {
> > +        fst->io_limits_enabled = false;
> > +    }
> > +}
> > +  
> 
> This function should be named *_parse_* but I'm not even sure it is actually needed (see below).
> [Pradeep Jagadeesh] This function is used to avoid the parsing of the options if not enabled.
> From my understanding of code block devices throttling code, 
> irrespective of the throttle options are enabled or not the parse 
> functions are called for all the devices. I am trying to avoid that by checking these variables at the before I get into parsing and setting up the throttle structures.
> 

And so, we would do a pre-parsing here before doing a full fledged parsing later ?
This still doesn't look very useful, but it is certainly error prone IMHO.
[Pradeep Jagadeesh] Yes, before populating the structures, I wanted to check is 
throttle enabled to this particular fsdev device or not?. If enabled
then I go-head with the full populate and initialize the structures.
So, shall I set without pre-checking them for now?

However, conditionally setting up the throttle structures and letting the backend know that throttling is enabled, are needed indeed.

Looking again at how block devices use the throttle API, I see that there's:

/* Does any throttling must be done
 *
 * @cfg: the throttling configuration to inspect
 * @ret: true if throttling must be done else false  */ bool throttle_enabled(ThrottleConfig *cfg) {
    int i;

    for (i = 0; i < BUCKETS_COUNT; i++) {
        if (cfg->buckets[i].avg > 0) {
            return true;
        }
    }

    return false;
}
... which does the same checks, and is used to actually enable the throttling.

You could use the above helper to set/unset @io_limits_enabled after the real parsing is done.
[Pradeep Jagadeesh]OK.

> > +static bool throttle9p_check_for_wait(FsThrottle *fst, bool 
> > +is_write) {
> > +    if (fst->any_timer_armed[is_write]) {
> > +        return true;
> > +    } else {
> > +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> > +    }
> > +}
> > +
> > +static void throttle9p_schedule_next_request(FsThrottle *fst, bool
> > +is_write) {
> > +    bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > +    if (!fst->pending_reqs[is_write]) {
> > +        return;
> > +    }
> > +    if (!must_wait) {
> > +        if (qemu_in_coroutine() &&
> > +            qemu_co_queue_next(&fst->throttled_reqs[is_write])) {
> > +            ;
> > +       } else {
> > +           int64_t now = qemu_clock_get_ns(fst->tt.clock_type);
> > +           timer_mod(fst->tt.timers[is_write], now + 1);
> > +           fst->any_timer_armed[is_write] = true;
> > +       }
> > +   }
> > +}
> > +
> > +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write) {
> > +    bool empty_queue;
> > +    qemu_mutex_lock(&fst->lock);
> > +    fst->any_timer_armed[is_write] = false;
> > +    qemu_mutex_unlock(&fst->lock);
> > +    empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]);
> > +    if (empty_queue) {
> > +        qemu_mutex_lock(&fst->lock);
> > +        throttle9p_schedule_next_request(fst, is_write);
> > +        qemu_mutex_unlock(&fst->lock);
> > +    }
> > +}
> > +
> > +
> > +bool throttle9p_get_io_limits_state(FsThrottle *fst)
> 
> The name looks a bit strange, since this helper simply returns a boolean flag.
> I guess throttle9p_enabled() is enough.
> [Pradeep Jagadeesh] OK
> 
> > +{
> > +
> > +    return fst->io_limits_enabled;
> > +}
> > +
> > +static void throttle9p_read_timer_cb(void *opaque) {
> > +    throttle9p_timer_cb(opaque, false); }
> > +
> > +static void throttle9p_write_timer_cb(void *opaque) {
> > +    throttle9p_timer_cb(opaque, true); }
> > +
> > +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle *fst) 
> > +{
> 
> This function can fail, it should have a return value (0 or -1).
> [Pradeep Jagadeesh] OK
> 
> > +    memset(&fst->ts, 1, sizeof(fst->ts));
> > +    memset(&fst->tt, 1, sizeof(fst->tt));
> > +    memset(&fst->cfg, 0, sizeof(fst->cfg));
> 
> Same remark as Claudio.
> [Pradeep Jagadeesh] OK, will address.
> 
> > +    fst->aioctx = qemu_get_aio_context();
> > +
> > +    if (!fst->aioctx) {
> > +        error_report("Failed to create AIO Context");
> > +        exit(1);
> > +    }
> > +    throttle_init(&fst->ts);
> > +    throttle_timers_init(&fst->tt,
> > +                         fst->aioctx,
> > +                         QEMU_CLOCK_REALTIME,
> > +                         throttle9p_read_timer_cb,
> > +                         throttle9p_write_timer_cb,
> > +                         fst);
> 
> Shouldn't all this be done later when we know the config is valid ?
> [Pradeep Jagadeesh] Yes, fixed.
> 
> > +    throttle_config_init(&fst->cfg);
> > +    g_assert(throttle_is_valid(&fst->cfg, NULL));
> > +  
> 
> What's the point in calling throttle_is_valid() on a freshly initialized config ?
> [Pradeep Jagadeesh] Removed
> 
> > +    qemu_co_queue_init(&fst->throttled_reqs[0]);
> > +    qemu_co_queue_init(&fst->throttled_reqs[1]);
> 
> Later, when the config is valid ?
> [Pradeep Jagadeesh] Done
> 
> > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> > +          qemu_opt_get_number(opts, "bps", 0);
> > +    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
> > +          qemu_opt_get_number(opts, "bps_rd", 0);
> > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> > +          qemu_opt_get_number(opts, "bps_wr", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> > +          qemu_opt_get_number(opts, "iops", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> > +          qemu_opt_get_number(opts, "iops_rd", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> > +          qemu_opt_get_number(opts, "iops_wr", 0);
> > +
> > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> > +          qemu_opt_get_number(opts, "bps_max", 0);
> > +    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
> > +          qemu_opt_get_number(opts, "bps_rd_max", 0);
> > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> > +          qemu_opt_get_number(opts, "bps_wr_max", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> > +          qemu_opt_get_number(opts, "iops_max", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_READ].max =
> > +          qemu_opt_get_number(opts, "iops_rd_max", 0);
> > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> > +          qemu_opt_get_number(opts, "iops_wr_max", 0);
> > +
> > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
> > +          qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
> > +    fst->cfg.buckets[THROTTLE_BPS_READ].burst_length  =
> > +          qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
> > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
> > +          qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
> > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
> > +          qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
> > +    fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
> > +          qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
> > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
> > +          qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
> > +    fst->cfg.op_size =
> > +          qemu_opt_get_number(opts, "iops_size", 0);
> > +
> > +    throttle_config(&fst->ts, &fst->tt, &fst->cfg);
> 
> Let's set the config later, when we we it is valid.
> [Pradeep Jagadeesh] OK
> 
> > +    if (!throttle_is_valid(&fst->cfg, NULL)) {
> 
> You should pass an Error * to throttle_is_valid() to be able to report 
> the misconfiguration to the user. I guess it is okay to print it here 
> using
> error_repport_err() (see include/qapi/error.h) and return -1.
> [Pradeep Jagadeesh] OK
> 
> > +        return;
> > +    }
> > +
> > +    g_assert(fst->tt.timers[0]);
> > +    g_assert(fst->tt.timers[1]);
> 
> These are really not needed since, timers are set with:
> 
>     QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer));
> 
> and g_malloc0() never returns NULL when passed a non-nul size. It 
> calls
> g_assert() internally instead.
> [Pradeep Jagadeesh] OK
> 
> > +    fst->pending_reqs[0] = 0;
> > +    fst->pending_reqs[1] = 0;
> > +    fst->any_timer_armed[0] = false;
> > +    fst->any_timter_armed[1] = false;
> > +    qemu_mutex_init(&fst->lock);
> 
> And there you may set the enabled flag.
> [Pradeep Jagadeesh] Explained before.
> 
> > +}
> > +
> > +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t
> > +bytes) {
> > +    if (fst->io_limits_enabled) {
> 
> throttle9p_enabled(fst)
> [Pradeep Jagadeesh] OK
> 
> > +        qemu_mutex_lock(&fst->lock);
> > +        bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > +        if (must_wait || fst->pending_reqs[is_write]) {
> > +            fst->pending_reqs[is_write]++;
> > +            qemu_mutex_unlock(&fst->lock);
> > +            qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> > +            qemu_mutex_lock(&fst->lock);
> > +            fst->pending_reqs[is_write]--;
> > +       }
> > +       throttle_account(&fst->ts, is_write, bytes);
> > +       throttle9p_schedule_next_request(fst, is_write);
> > +       qemu_mutex_unlock(&fst->lock);
> > +    }
> > +}
> > +
> > +void throttle9p_cleanup(FsThrottle *fst) {
> > +    throttle_timers_destroy(&fst->tt);
> > +    qemu_mutex_destroy(&fst->lock); }
> > diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h new file 
> > mode 100644 index 0000000..0f7551d
> > --- /dev/null
> > +++ b/hw/9pfs/9p-throttle.h
> > @@ -0,0 +1,46 @@
> > +/*
> > + * 9P Throttle
> > + *
> > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 
> > +or
> > + * (at your option) any later version.
> > + *
> > + * See the COPYING file in the top-level directory for details.
> > + *
> > + */
> > +
> > +#ifndef _9P_THROTTLE_H
> > +#define _9P_THROTTLE_H
> > +
> > +#include <stdbool.h>
> > +#include <stdint.h>
> 
> These includes are forbidden. They are already handled by "qemu/osdep.h" which is supposed to be included by all .c files.
> [Pradeep Jagadeesh] OK
> 
> > +#include "block/aio.h"
> > +#include "qemu/main-loop.h"
> > +#include "qemu/coroutine.h"
> > +#include "qemu/throttle.h"
> > +
> > +typedef struct FsThrottle {
> > +    ThrottleState ts;
> > +    ThrottleTimers tt;
> > +    AioContext   *aioctx;
> > +    ThrottleConfig cfg;
> > +    bool io_limits_enabled;
> 
> Let's simply call this enabled.
> [Pradeep Jagadeesh] OK
> 
> > +    CoQueue      throttled_reqs[2];
> > +    unsigned     pending_reqs[2];
> > +    bool any_timer_armed[2];
> > +    QemuMutex lock;
> > +} FsThrottle;
> > +
> > +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *);
> > +
> > +bool throttle9p_get_io_limits_state(FsThrottle *);
> > +
> > +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *);
> > +
> > +void throttle9p_request(FsThrottle *, bool , ssize_t);
> > +
> > +void throttle9p_cleanup(FsThrottle *); #endif /* _9P_THROTTLE_H */
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index dfe293d..7be926a 
> > 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
> >          error_setg(errp, "share path %s is not a directory", fse->path);
> >          goto out;
> >      }
> > +
> > +    /* Throttle structure initialization */
> 
> Not sure this comment is helpful.
> [Pradeep Jagadeesh] Just to give a hint, where the throttle structures 
> are getting Populated. May be some other comment?
> 

Unless I'm missing something, the throttle structures aren't getting populated here...
[Pradeep Jagadeesh] Yes, not getting populated here. Initializing the throttle 
structure inside the 9pfs_device structure, while realizing a 9pfs_device.

> > +    s->ctx.fst = &fse->fst;
> > +
> >      v9fs_path_free(&path);
> >  
> >      rc = 0;
> > @@ -3504,6 +3508,9 @@ out:
> >  
> >  void v9fs_device_unrealize_common(V9fsState *s, Error **errp)  {
> > +    if (throttle9p_get_io_limits_state(s->ctx.fst)) {
> > +        throttle9p_cleanup(s->ctx.fst);
> > +    }
> >      g_free(s->ctx.fs_root);
> >      g_free(s->tag);
> >  }
> > diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs index
> > da0ae0c..07523f1 100644
> > --- a/hw/9pfs/Makefile.objs
> > +++ b/hw/9pfs/Makefile.objs
> > @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o 
> > common-obj-y += coxattr.o 9p-synth.o
> >  common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o  common-obj-y 
> > += 9p-proxy.o
> > +common-obj-y += 9p-throttle.o
> >  
> >  obj-y += virtio-9p-device.o
>

On Mon, 12 Sep 2016 16:08:43 +0000
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:

> Replies inline Greg.
> 
> Thanks & Regards,
> Pradeep
> 

Hi Pradeep,

> -----Original Message-----
> From: Greg Kurz [mailto:groug@kaod.org] 
> Sent: Monday, September 12, 2016 4:19 PM
> To: Pradeep Jagadeesh
> Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver
> 
> On Mon, 12 Sep 2016 12:52:55 +0000
> Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:
> 
> > Hi Greg,
> > 
> > Thanks for looking into the patch. Please look at the replies inline.
> > 
> > Regards,
> > Pradeep
> >   
> 
> Hi Pradeep,
> 
> Remarks and answers below.
> 
> Cheers.
> 
> --
> Greg
> 
> > -----Original Message-----
> > From: Greg Kurz [mailto:groug@kaod.org]
> > Sent: Friday, September 09, 2016 5:29 PM
> > To: Pradeep Jagadeesh
> > Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; 
> > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local 
> > driver
> > 
> > On Fri,  9 Sep 2016 05:10:27 -0400
> > Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:
> >   
> > > Uses throttling APIs to limit I/O bandwidth and number of operations 
> > > on the devices which use 9p-local driver.
> > > 
> > > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > ---  
> > 
> > Hi Pradeep,
> > 
> > Please find some remarks below. I haven't dived deep enough to see if this actually works. Maybe Berto can provide some feedback ?
> > 
> > Cheers.
> > 
> > --
> > Greg
> >   
> > >  fsdev/file-op-9p.h      |   3 +
> > >  fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
> > >  hw/9pfs/9p-local.c      |  18 ++++-
> > >  hw/9pfs/9p-throttle.c   | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  hw/9pfs/9p-throttle.h   |  46 +++++++++++  
> 
> I forgot to mention it, but this throttling implementation isn't related to the 9P device but to the fsdev device: 9p-throttle.[ch] should move to the fsdev directory and be renamed to fsdev-throttle.[ch].
> 
> [Pradeep Jagadeesh] Absolutely right and make sense, I will move the throttle code to fsdev directory.
> 
> > >  hw/9pfs/9p.c            |   7 ++
> > >  hw/9pfs/Makefile.objs   |   1 +
> > >  7 files changed, 326 insertions(+), 2 deletions(-)  create mode
> > > 100644 hw/9pfs/9p-throttle.c  create mode 100644 
> > > hw/9pfs/9p-throttle.h
> > > 
> > > 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.
> > > 
> > > v1 -> v2:
> > > 
> > > -Fixed FsContext redeclaration issue -Removed couple of function 
> > > declarations from 9p-throttle.h -Fixed some of the .help messages
> > > 
> > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index 
> > > 6db9fea..e86b91a 100644
> > > --- a/fsdev/file-op-9p.h
> > > +++ b/fsdev/file-op-9p.h
> > > @@ -17,6 +17,7 @@
> > >  #include <dirent.h>
> > >  #include <utime.h>
> > >  #include <sys/vfs.h>
> > > +#include "hw/9pfs/9p-throttle.h"
> > >  
> > >  #define SM_LOCAL_MODE_BITS    0600
> > >  #define SM_LOCAL_DIR_MODE_BITS    0700
> > > @@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
> > >      char *path;
> > >      int export_flags;
> > >      FileOperations *ops;
> > > +    FsThrottle fst;
> > >  } FsDriverEntry;
> > >  
> > >  typedef struct FsContext
> > > @@ -83,6 +85,7 @@ typedef struct FsContext
> > >      int export_flags;
> > >      struct xattr_operations **xops;
> > >      struct extended_ops exops;
> > > +    FsThrottle *fst;
> > >      /* fs driver specific data */
> > >      void *private;
> > >  } FsContext;
> > > diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c index
> > > 1dd8c7a..2774855 100644
> > > --- a/fsdev/qemu-fsdev-opts.c
> > > +++ b/fsdev/qemu-fsdev-opts.c
> > > @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
> > >          }, {
> > >              .name = "sock_fd",
> > >              .type = QEMU_OPT_NUMBER,
> > > +        }, {
> > > +            .name = "",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit total bytes per second",
> > > +        }, {
> > > +            .name = "bps_rd",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit read bytes per second",
> > > +        }, {
> > > +            .name = "bps_wr",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit write bytes per second",
> > > +        }, {
> > > +            .name = "iops",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit total io operations per second",
> > > +        }, {
> > > +            .name = "iops_rd",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit read operations per second ",
> > > +        }, {
> > > +            .name = "iops_wr",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit write operations per second",
> > > +        }, {
> > > +            .name = "bps_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum bytes burst",
> > > +        }, {
> > > +            .name = "bps_rd_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum bytes read burst",
> > > +        }, {
> > > +            .name = "bps_wr_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum bytes write burst",
> > > +        }, {
> > > +            .name = "iops_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum io operations burst",
> > > +        }, {
> > > +            .name = "iops_rd_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum io operations read burst",
> > > +        }, {
> > > +            .name = "iops_wr_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum io operations write burst",
> > > +        }, {
> > > +            .name = "iops_size",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "io iops-size",
> > >          },
> > >      
> > 
> > In case we end up sharing code with blockdev as suggested by Eric, maybe you can use the same QMP friendly naming scheme ("throttling.*") and the same help strings as well.
> > 
> > [Pradeep Jagadeesh]
> > [Pradeep Jagadeesh] You mean to say I should re-name the options how 
> > they are done for block devices? Or mentioning the cli options itself as throttling.*?
> >                                    
> 
> I meant the latter. IIUC, the renaming done in blockdev is for compatibility only, as stated in the changelog of commit 57975222b6.
> [Pradeep Jagadeesh] OK, I will rename them as well. But there will be more code duplication.
> May be we have to merge the code later. We have to come up with a structure to fit all different
> kind of backend devices. (those who wants to use throttling apis).
> 

That's the point. Let's start with the qemu_fsdev_opts list, so that it
contains the same throttling options as qemu_common_drive_opts. Later,
we'll probably even move the descriptions of these common throttling
options to an include/qemu/throttle-options.h header file.

> > >          { /*End of list */ }
> > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index
> > > 3f271fc..49c2819 100644
> > > --- a/hw/9pfs/9p-local.c
> > > +++ b/hw/9pfs/9p-local.c
> > > @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
> > >                              const struct iovec *iov,
> > >                              int iovcnt, off_t offset)  {
> > > +    throttle9p_request(ctx->fst, false, iov->iov_len);
> > > +

Maybe this could move directly to v9fs_co_preadv(), so that other backends
just need to be enabled to use throttling.

> > >  #ifdef CONFIG_PREADV
> > >      return preadv(fs->fd, iov, iovcnt, offset);  #else @@ -436,8
> > > +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, 
> > > +V9fsFidOpenState *fs,
> > >                               const struct iovec *iov,
> > >                               int iovcnt, off_t offset)  {
> > > -    ssize_t ret
> > > -;
> > > +    ssize_t ret;
> > > +
> > > +    throttle9p_request(ctx->fst, true, iov->iov_len);
> > > +

And this may move to v9fs_co_pwritev().

> > >  #ifdef CONFIG_PREADV
> > >      ret = pwritev(fs->fd, iov, iovcnt, offset);  #else @@ -1213,6
> > > +1217,9 @@ 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");
> > >  
> > > +    /* get the throttle structure */  
> > 
> > Not sure this comment is helpful.
> > [Pradeep Jagadeesh] OK
> >   
> > > +    FsThrottle *fst = &fse->fst;
> > > +
> > >      if (!sec_model) {
> > >          error_report("Security model not specified, local fs needs security model");
> > >          error_printf("valid options are:"
> > > @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
> > >          error_report("fsdev: No path specified");
> > >          return -1;
> > >      }
> > > +
> > > +    throttle9p_enable_io_limits(opts, fst);
> > > +
> > > +    if (throttle9p_get_io_limits_state(fst)) {
> > > +        throttle9p_configure_iolimits(opts, fst);
> > > +    }
> > > +
> > >      fse->path = g_strdup(path);
> > >  
> > >      return 0;
> > > diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c new file 
> > > mode 100644 index 0000000..f2a7ba5
> > > --- /dev/null
> > > +++ b/hw/9pfs/9p-throttle.c
> > > @@ -0,0 +1,201 @@
> > > +/*
> > > + * 9P Throttle
> > > + *
> > > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > > + *
> > > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 
> > > +or
> > > + * (at your option) any later version.
> > > + *
> > > + * See the COPYING file in the top-level directory for details.
> > > + *
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> > > +#include "qemu/cutils.h"
> > > +#include "qemu/error-report.h"
> > > +#include <libgen.h>
> > > +#include <linux/fs.h>
> > > +#include <sys/ioctl.h>
> > > +#include <string.h>
> > > +#include "fsdev/file-op-9p.h"
> > > +#include "9p-throttle.h"
> > > +    
> > 
> > Only "qemu/osdep.h", "qemu/error-report.h" and "9p-throttle.h" are actually needed.
> >   
> > > +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst) {
> > > +    const float bps = qemu_opt_get_number(opts, "bps", 0);
> > > +    const float iops = qemu_opt_get_number(opts, "iops", 0);
> > > +    const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0);
> > > +    const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0);
> > > +    const float rdops = qemu_opt_get_number(opts, "iops_rd", 0);
> > > +    const float wrops = qemu_opt_get_number(opts, "iops_wr", 0);
> > > +
> > > +    if (bps > 0 || iops > 0 || rdbps > 0 ||
> > > +        wrbps > 0 || rdops > 0 || wrops > 0) {
> > > +        fst->io_limits_enabled = true;
> > > +    } else {
> > > +        fst->io_limits_enabled = false;
> > > +    }
> > > +}
> > > +    
> > 
> > This function should be named *_parse_* but I'm not even sure it is actually needed (see below).
> > [Pradeep Jagadeesh] This function is used to avoid the parsing of the options if not enabled.
> > From my understanding of code block devices throttling code, 
> > irrespective of the throttle options are enabled or not the parse 
> > functions are called for all the devices. I am trying to avoid that by checking these variables at the before I get into parsing and setting up the throttle structures.
> >   
> 
> And so, we would do a pre-parsing here before doing a full fledged parsing later ?
> This still doesn't look very useful, but it is certainly error prone IMHO.
> [Pradeep Jagadeesh] Yes, before populating the structures, I wanted to check is 
> throttle enabled to this particular fsdev device or not?. If enabled
> then I go-head with the full populate and initialize the structures.
> So, shall I set without pre-checking them for now?
> 

Yes.

> However, conditionally setting up the throttle structures and letting the backend know that throttling is enabled, are needed indeed.
> 
> Looking again at how block devices use the throttle API, I see that there's:
> 
> /* Does any throttling must be done
>  *
>  * @cfg: the throttling configuration to inspect
>  * @ret: true if throttling must be done else false  */ bool throttle_enabled(ThrottleConfig *cfg) {
>     int i;
> 
>     for (i = 0; i < BUCKETS_COUNT; i++) {
>         if (cfg->buckets[i].avg > 0) {
>             return true;
>         }
>     }
> 
>     return false;
> }
> ... which does the same checks, and is used to actually enable the throttling.
> 
> You could use the above helper to set/unset @io_limits_enabled after the real parsing is done.
> [Pradeep Jagadeesh]OK.
> 
> > > +static bool throttle9p_check_for_wait(FsThrottle *fst, bool 
> > > +is_write) {
> > > +    if (fst->any_timer_armed[is_write]) {
> > > +        return true;
> > > +    } else {
> > > +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> > > +    }
> > > +}
> > > +
> > > +static void throttle9p_schedule_next_request(FsThrottle *fst, bool
> > > +is_write) {
> > > +    bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > > +    if (!fst->pending_reqs[is_write]) {
> > > +        return;
> > > +    }
> > > +    if (!must_wait) {
> > > +        if (qemu_in_coroutine() &&
> > > +            qemu_co_queue_next(&fst->throttled_reqs[is_write])) {
> > > +            ;
> > > +       } else {
> > > +           int64_t now = qemu_clock_get_ns(fst->tt.clock_type);
> > > +           timer_mod(fst->tt.timers[is_write], now + 1);
> > > +           fst->any_timer_armed[is_write] = true;
> > > +       }
> > > +   }
> > > +}
> > > +
> > > +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write) {
> > > +    bool empty_queue;
> > > +    qemu_mutex_lock(&fst->lock);
> > > +    fst->any_timer_armed[is_write] = false;
> > > +    qemu_mutex_unlock(&fst->lock);
> > > +    empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]);
> > > +    if (empty_queue) {
> > > +        qemu_mutex_lock(&fst->lock);
> > > +        throttle9p_schedule_next_request(fst, is_write);
> > > +        qemu_mutex_unlock(&fst->lock);
> > > +    }
> > > +}
> > > +
> > > +
> > > +bool throttle9p_get_io_limits_state(FsThrottle *fst)  
> > 
> > The name looks a bit strange, since this helper simply returns a boolean flag.
> > I guess throttle9p_enabled() is enough.
> > [Pradeep Jagadeesh] OK
> >   
> > > +{
> > > +
> > > +    return fst->io_limits_enabled;
> > > +}
> > > +
> > > +static void throttle9p_read_timer_cb(void *opaque) {
> > > +    throttle9p_timer_cb(opaque, false); }
> > > +
> > > +static void throttle9p_write_timer_cb(void *opaque) {
> > > +    throttle9p_timer_cb(opaque, true); }
> > > +
> > > +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle *fst) 
> > > +{  
> > 
> > This function can fail, it should have a return value (0 or -1).
> > [Pradeep Jagadeesh] OK
> >   
> > > +    memset(&fst->ts, 1, sizeof(fst->ts));
> > > +    memset(&fst->tt, 1, sizeof(fst->tt));
> > > +    memset(&fst->cfg, 0, sizeof(fst->cfg));  
> > 
> > Same remark as Claudio.
> > [Pradeep Jagadeesh] OK, will address.
> >   
> > > +    fst->aioctx = qemu_get_aio_context();
> > > +
> > > +    if (!fst->aioctx) {
> > > +        error_report("Failed to create AIO Context");
> > > +        exit(1);
> > > +    }
> > > +    throttle_init(&fst->ts);
> > > +    throttle_timers_init(&fst->tt,
> > > +                         fst->aioctx,
> > > +                         QEMU_CLOCK_REALTIME,
> > > +                         throttle9p_read_timer_cb,
> > > +                         throttle9p_write_timer_cb,
> > > +                         fst);  
> > 
> > Shouldn't all this be done later when we know the config is valid ?
> > [Pradeep Jagadeesh] Yes, fixed.
> >   
> > > +    throttle_config_init(&fst->cfg);
> > > +    g_assert(throttle_is_valid(&fst->cfg, NULL));
> > > +    
> > 
> > What's the point in calling throttle_is_valid() on a freshly initialized config ?
> > [Pradeep Jagadeesh] Removed
> >   
> > > +    qemu_co_queue_init(&fst->throttled_reqs[0]);
> > > +    qemu_co_queue_init(&fst->throttled_reqs[1]);  
> > 
> > Later, when the config is valid ?
> > [Pradeep Jagadeesh] Done
> >   
> > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> > > +          qemu_opt_get_number(opts, "bps", 0);
> > > +    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
> > > +          qemu_opt_get_number(opts, "bps_rd", 0);
> > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> > > +          qemu_opt_get_number(opts, "bps_wr", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> > > +          qemu_opt_get_number(opts, "iops", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> > > +          qemu_opt_get_number(opts, "iops_rd", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> > > +          qemu_opt_get_number(opts, "iops_wr", 0);
> > > +
> > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> > > +          qemu_opt_get_number(opts, "bps_max", 0);
> > > +    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
> > > +          qemu_opt_get_number(opts, "bps_rd_max", 0);
> > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> > > +          qemu_opt_get_number(opts, "bps_wr_max", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> > > +          qemu_opt_get_number(opts, "iops_max", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_READ].max =
> > > +          qemu_opt_get_number(opts, "iops_rd_max", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> > > +          qemu_opt_get_number(opts, "iops_wr_max", 0);
> > > +
> > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
> > > +          qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
> > > +    fst->cfg.buckets[THROTTLE_BPS_READ].burst_length  =
> > > +          qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
> > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
> > > +          qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
> > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
> > > +          qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
> > > +    fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
> > > +          qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
> > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
> > > +          qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
> > > +    fst->cfg.op_size =
> > > +          qemu_opt_get_number(opts, "iops_size", 0);
> > > +
> > > +    throttle_config(&fst->ts, &fst->tt, &fst->cfg);  
> > 
> > Let's set the config later, when we we it is valid.
> > [Pradeep Jagadeesh] OK
> >   
> > > +    if (!throttle_is_valid(&fst->cfg, NULL)) {  
> > 
> > You should pass an Error * to throttle_is_valid() to be able to report 
> > the misconfiguration to the user. I guess it is okay to print it here 
> > using
> > error_repport_err() (see include/qapi/error.h) and return -1.
> > [Pradeep Jagadeesh] OK
> >   
> > > +        return;
> > > +    }
> > > +
> > > +    g_assert(fst->tt.timers[0]);
> > > +    g_assert(fst->tt.timers[1]);  
> > 
> > These are really not needed since, timers are set with:
> > 
> >     QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer));
> > 
> > and g_malloc0() never returns NULL when passed a non-nul size. It 
> > calls
> > g_assert() internally instead.
> > [Pradeep Jagadeesh] OK
> >   
> > > +    fst->pending_reqs[0] = 0;
> > > +    fst->pending_reqs[1] = 0;
> > > +    fst->any_timer_armed[0] = false;
> > > +    fst->any_timter_armed[1] = false;
> > > +    qemu_mutex_init(&fst->lock);  
> > 
> > And there you may set the enabled flag.
> > [Pradeep Jagadeesh] Explained before.
> >   
> > > +}
> > > +
> > > +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t
> > > +bytes) {
> > > +    if (fst->io_limits_enabled) {  
> > 
> > throttle9p_enabled(fst)
> > [Pradeep Jagadeesh] OK
> >   
> > > +        qemu_mutex_lock(&fst->lock);
> > > +        bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > > +        if (must_wait || fst->pending_reqs[is_write]) {
> > > +            fst->pending_reqs[is_write]++;
> > > +            qemu_mutex_unlock(&fst->lock);
> > > +            qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> > > +            qemu_mutex_lock(&fst->lock);
> > > +            fst->pending_reqs[is_write]--;
> > > +       }
> > > +       throttle_account(&fst->ts, is_write, bytes);
> > > +       throttle9p_schedule_next_request(fst, is_write);
> > > +       qemu_mutex_unlock(&fst->lock);
> > > +    }
> > > +}
> > > +
> > > +void throttle9p_cleanup(FsThrottle *fst) {
> > > +    throttle_timers_destroy(&fst->tt);
> > > +    qemu_mutex_destroy(&fst->lock); }
> > > diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h new file 
> > > mode 100644 index 0000000..0f7551d
> > > --- /dev/null
> > > +++ b/hw/9pfs/9p-throttle.h
> > > @@ -0,0 +1,46 @@
> > > +/*
> > > + * 9P Throttle
> > > + *
> > > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > > + *
> > > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 
> > > +or
> > > + * (at your option) any later version.
> > > + *
> > > + * See the COPYING file in the top-level directory for details.
> > > + *
> > > + */
> > > +
> > > +#ifndef _9P_THROTTLE_H
> > > +#define _9P_THROTTLE_H
> > > +
> > > +#include <stdbool.h>
> > > +#include <stdint.h>  
> > 
> > These includes are forbidden. They are already handled by "qemu/osdep.h" which is supposed to be included by all .c files.
> > [Pradeep Jagadeesh] OK
> >   
> > > +#include "block/aio.h"
> > > +#include "qemu/main-loop.h"
> > > +#include "qemu/coroutine.h"
> > > +#include "qemu/throttle.h"
> > > +
> > > +typedef struct FsThrottle {
> > > +    ThrottleState ts;
> > > +    ThrottleTimers tt;
> > > +    AioContext   *aioctx;
> > > +    ThrottleConfig cfg;
> > > +    bool io_limits_enabled;  
> > 
> > Let's simply call this enabled.
> > [Pradeep Jagadeesh] OK
> >   
> > > +    CoQueue      throttled_reqs[2];
> > > +    unsigned     pending_reqs[2];
> > > +    bool any_timer_armed[2];
> > > +    QemuMutex lock;
> > > +} FsThrottle;
> > > +
> > > +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *);
> > > +
> > > +bool throttle9p_get_io_limits_state(FsThrottle *);
> > > +
> > > +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *);
> > > +
> > > +void throttle9p_request(FsThrottle *, bool , ssize_t);
> > > +
> > > +void throttle9p_cleanup(FsThrottle *); #endif /* _9P_THROTTLE_H */
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index dfe293d..7be926a 
> > > 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
> > >          error_setg(errp, "share path %s is not a directory", fse->path);
> > >          goto out;
> > >      }
> > > +
> > > +    /* Throttle structure initialization */  
> > 
> > Not sure this comment is helpful.
> > [Pradeep Jagadeesh] Just to give a hint, where the throttle structures 
> > are getting Populated. May be some other comment?
> >   
> 
> Unless I'm missing something, the throttle structures aren't getting populated here...
> [Pradeep Jagadeesh] Yes, not getting populated here. Initializing the throttle 
> structure inside the 9pfs_device structure, while realizing a 9pfs_device.
> 
> > > +    s->ctx.fst = &fse->fst;

The code is self-explanatory. No need to paraphrase.

Cheers.

--
Greg

> > > +
> > >      v9fs_path_free(&path);
> > >  
> > >      rc = 0;
> > > @@ -3504,6 +3508,9 @@ out:
> > >  
> > >  void v9fs_device_unrealize_common(V9fsState *s, Error **errp)  {
> > > +    if (throttle9p_get_io_limits_state(s->ctx.fst)) {
> > > +        throttle9p_cleanup(s->ctx.fst);
> > > +    }
> > >      g_free(s->ctx.fs_root);
> > >      g_free(s->tag);
> > >  }
> > > diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs index
> > > da0ae0c..07523f1 100644
> > > --- a/hw/9pfs/Makefile.objs
> > > +++ b/hw/9pfs/Makefile.objs
> > > @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o 
> > > common-obj-y += coxattr.o 9p-synth.o
> > >  common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o  common-obj-y 
> > > += 9p-proxy.o
> > > +common-obj-y += 9p-throttle.o
> > >  
> > >  obj-y += virtio-9p-device.o  
> >   
>

Hi Greg,

Replies inline

Cheers,
Pradeep
-----Original Message-----
From: Greg Kurz [mailto:groug@kaod.org] 
Sent: Tuesday, September 13, 2016 10:52 AM
To: Pradeep Jagadeesh
Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver

On Mon, 12 Sep 2016 16:08:43 +0000
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:

> Replies inline Greg.
> 
> Thanks & Regards,
> Pradeep
> 

Hi Pradeep,

> -----Original Message-----
> From: Greg Kurz [mailto:groug@kaod.org]
> Sent: Monday, September 12, 2016 4:19 PM
> To: Pradeep Jagadeesh
> Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; 
> qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local 
> driver
> 
> On Mon, 12 Sep 2016 12:52:55 +0000
> Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:
> 
> > Hi Greg,
> > 
> > Thanks for looking into the patch. Please look at the replies inline.
> > 
> > Regards,
> > Pradeep
> >   
> 
> Hi Pradeep,
> 
> Remarks and answers below.
> 
> Cheers.
> 
> --
> Greg
> 
> > -----Original Message-----
> > From: Greg Kurz [mailto:groug@kaod.org]
> > Sent: Friday, September 09, 2016 5:29 PM
> > To: Pradeep Jagadeesh
> > Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; 
> > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local 
> > driver
> > 
> > On Fri,  9 Sep 2016 05:10:27 -0400
> > Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:
> >   
> > > Uses throttling APIs to limit I/O bandwidth and number of 
> > > operations on the devices which use 9p-local driver.
> > > 
> > > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > ---
> > 
> > Hi Pradeep,
> > 
> > Please find some remarks below. I haven't dived deep enough to see if this actually works. Maybe Berto can provide some feedback ?
> > 
> > Cheers.
> > 
> > --
> > Greg
> >   
> > >  fsdev/file-op-9p.h      |   3 +
> > >  fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
> > >  hw/9pfs/9p-local.c      |  18 ++++-
> > >  hw/9pfs/9p-throttle.c   | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  hw/9pfs/9p-throttle.h   |  46 +++++++++++  
> 
> I forgot to mention it, but this throttling implementation isn't related to the 9P device but to the fsdev device: 9p-throttle.[ch] should move to the fsdev directory and be renamed to fsdev-throttle.[ch].
> 
> [Pradeep Jagadeesh] Absolutely right and make sense, I will move the throttle code to fsdev directory.
> 
> > >  hw/9pfs/9p.c            |   7 ++
> > >  hw/9pfs/Makefile.objs   |   1 +
> > >  7 files changed, 326 insertions(+), 2 deletions(-)  create mode
> > > 100644 hw/9pfs/9p-throttle.c  create mode 100644 
> > > hw/9pfs/9p-throttle.h
> > > 
> > > 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.
> > > 
> > > v1 -> v2:
> > > 
> > > -Fixed FsContext redeclaration issue -Removed couple of function 
> > > declarations from 9p-throttle.h -Fixed some of the .help messages
> > > 
> > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index 
> > > 6db9fea..e86b91a 100644
> > > --- a/fsdev/file-op-9p.h
> > > +++ b/fsdev/file-op-9p.h
> > > @@ -17,6 +17,7 @@
> > >  #include <dirent.h>
> > >  #include <utime.h>
> > >  #include <sys/vfs.h>
> > > +#include "hw/9pfs/9p-throttle.h"
> > >  
> > >  #define SM_LOCAL_MODE_BITS    0600
> > >  #define SM_LOCAL_DIR_MODE_BITS    0700
> > > @@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
> > >      char *path;
> > >      int export_flags;
> > >      FileOperations *ops;
> > > +    FsThrottle fst;
> > >  } FsDriverEntry;
> > >  
> > >  typedef struct FsContext
> > > @@ -83,6 +85,7 @@ typedef struct FsContext
> > >      int export_flags;
> > >      struct xattr_operations **xops;
> > >      struct extended_ops exops;
> > > +    FsThrottle *fst;
> > >      /* fs driver specific data */
> > >      void *private;
> > >  } FsContext;
> > > diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c 
> > > index
> > > 1dd8c7a..2774855 100644
> > > --- a/fsdev/qemu-fsdev-opts.c
> > > +++ b/fsdev/qemu-fsdev-opts.c
> > > @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
> > >          }, {
> > >              .name = "sock_fd",
> > >              .type = QEMU_OPT_NUMBER,
> > > +        }, {
> > > +            .name = "",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit total bytes per second",
> > > +        }, {
> > > +            .name = "bps_rd",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit read bytes per second",
> > > +        }, {
> > > +            .name = "bps_wr",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit write bytes per second",
> > > +        }, {
> > > +            .name = "iops",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit total io operations per second",
> > > +        }, {
> > > +            .name = "iops_rd",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit read operations per second ",
> > > +        }, {
> > > +            .name = "iops_wr",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "limit write operations per second",
> > > +        }, {
> > > +            .name = "bps_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum bytes burst",
> > > +        }, {
> > > +            .name = "bps_rd_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum bytes read burst",
> > > +        }, {
> > > +            .name = "bps_wr_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum bytes write burst",
> > > +        }, {
> > > +            .name = "iops_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum io operations burst",
> > > +        }, {
> > > +            .name = "iops_rd_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum io operations read burst",
> > > +        }, {
> > > +            .name = "iops_wr_max",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "maximum io operations write burst",
> > > +        }, {
> > > +            .name = "iops_size",
> > > +            .type = QEMU_OPT_NUMBER,
> > > +            .help = "io iops-size",
> > >          },
> > >      
> > 
> > In case we end up sharing code with blockdev as suggested by Eric, maybe you can use the same QMP friendly naming scheme ("throttling.*") and the same help strings as well.
> > 
> > [Pradeep Jagadeesh]
> > [Pradeep Jagadeesh] You mean to say I should re-name the options how 
> > they are done for block devices? Or mentioning the cli options itself as throttling.*?
> >                                    
> 
> I meant the latter. IIUC, the renaming done in blockdev is for compatibility only, as stated in the changelog of commit 57975222b6.
> [Pradeep Jagadeesh] OK, I will rename them as well. But there will be more code duplication.
> May be we have to merge the code later. We have to come up with a 
> structure to fit all different kind of backend devices. (those who wants to use throttling apis).
> 

That's the point. Let's start with the qemu_fsdev_opts list, so that it contains the same throttling options as qemu_common_drive_opts. Later, we'll probably even move the descriptions of these common throttling options to an include/qemu/throttle-options.h header file.
[Pradeep Jagadeesh] OK, I wanted one more thing to get clarified.The Qemu CLI options look like
throttling.bps-read or just bps_rd?, Because there is a renaming function inside the blockdev
that renames options for example from bps_rd to throttling.bps-read. 

> > >          { /*End of list */ }
> > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index
> > > 3f271fc..49c2819 100644
> > > --- a/hw/9pfs/9p-local.c
> > > +++ b/hw/9pfs/9p-local.c
> > > @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
> > >                              const struct iovec *iov,
> > >                              int iovcnt, off_t offset)  {
> > > +    throttle9p_request(ctx->fst, false, iov->iov_len);
> > > +

Maybe this could move directly to v9fs_co_preadv(), so that other backends just need to be enabled to use throttling.
[Pradeep Jagadeesh] OK

> > >  #ifdef CONFIG_PREADV
> > >      return preadv(fs->fd, iov, iovcnt, offset);  #else @@ -436,8
> > > +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, 
> > > +V9fsFidOpenState *fs,
> > >                               const struct iovec *iov,
> > >                               int iovcnt, off_t offset)  {
> > > -    ssize_t ret
> > > -;
> > > +    ssize_t ret;
> > > +
> > > +    throttle9p_request(ctx->fst, true, iov->iov_len);
> > > +

And this may move to v9fs_co_pwritev().
[Pradeep Jagadeesh] OK

> > >  #ifdef CONFIG_PREADV
> > >      ret = pwritev(fs->fd, iov, iovcnt, offset);  #else @@ -1213,6
> > > +1217,9 @@ 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");
> > >  
> > > +    /* get the throttle structure */
> > 
> > Not sure this comment is helpful.
> > [Pradeep Jagadeesh] OK
> >   
> > > +    FsThrottle *fst = &fse->fst;
> > > +
> > >      if (!sec_model) {
> > >          error_report("Security model not specified, local fs needs security model");
> > >          error_printf("valid options are:"
> > > @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
> > >          error_report("fsdev: No path specified");
> > >          return -1;
> > >      }
> > > +
> > > +    throttle9p_enable_io_limits(opts, fst);
> > > +
> > > +    if (throttle9p_get_io_limits_state(fst)) {
> > > +        throttle9p_configure_iolimits(opts, fst);
> > > +    }
> > > +
> > >      fse->path = g_strdup(path);
> > >  
> > >      return 0;
> > > diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c new 
> > > file mode 100644 index 0000000..f2a7ba5
> > > --- /dev/null
> > > +++ b/hw/9pfs/9p-throttle.c
> > > @@ -0,0 +1,201 @@
> > > +/*
> > > + * 9P Throttle
> > > + *
> > > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > > + *
> > > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 
> > > +2 or
> > > + * (at your option) any later version.
> > > + *
> > > + * See the COPYING file in the top-level directory for details.
> > > + *
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> > > +#include "qemu/cutils.h"
> > > +#include "qemu/error-report.h"
> > > +#include <libgen.h>
> > > +#include <linux/fs.h>
> > > +#include <sys/ioctl.h>
> > > +#include <string.h>
> > > +#include "fsdev/file-op-9p.h"
> > > +#include "9p-throttle.h"
> > > +    
> > 
> > Only "qemu/osdep.h", "qemu/error-report.h" and "9p-throttle.h" are actually needed.
> >   
> > > +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst) {
> > > +    const float bps = qemu_opt_get_number(opts, "bps", 0);
> > > +    const float iops = qemu_opt_get_number(opts, "iops", 0);
> > > +    const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0);
> > > +    const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0);
> > > +    const float rdops = qemu_opt_get_number(opts, "iops_rd", 0);
> > > +    const float wrops = qemu_opt_get_number(opts, "iops_wr", 0);
> > > +
> > > +    if (bps > 0 || iops > 0 || rdbps > 0 ||
> > > +        wrbps > 0 || rdops > 0 || wrops > 0) {
> > > +        fst->io_limits_enabled = true;
> > > +    } else {
> > > +        fst->io_limits_enabled = false;
> > > +    }
> > > +}
> > > +    
> > 
> > This function should be named *_parse_* but I'm not even sure it is actually needed (see below).
> > [Pradeep Jagadeesh] This function is used to avoid the parsing of the options if not enabled.
> > From my understanding of code block devices throttling code, 
> > irrespective of the throttle options are enabled or not the parse 
> > functions are called for all the devices. I am trying to avoid that by checking these variables at the before I get into parsing and setting up the throttle structures.
> >   
> 
> And so, we would do a pre-parsing here before doing a full fledged parsing later ?
> This still doesn't look very useful, but it is certainly error prone IMHO.
> [Pradeep Jagadeesh] Yes, before populating the structures, I wanted to 
> check is throttle enabled to this particular fsdev device or not?. If 
> enabled then I go-head with the full populate and initialize the structures.
> So, shall I set without pre-checking them for now?
> 

Yes.
[Pradeep Jagadeesh] OK

> However, conditionally setting up the throttle structures and letting the backend know that throttling is enabled, are needed indeed.
> 
> Looking again at how block devices use the throttle API, I see that there's:
> 
> /* Does any throttling must be done
>  *
>  * @cfg: the throttling configuration to inspect
>  * @ret: true if throttling must be done else false  */ bool throttle_enabled(ThrottleConfig *cfg) {
>     int i;
> 
>     for (i = 0; i < BUCKETS_COUNT; i++) {
>         if (cfg->buckets[i].avg > 0) {
>             return true;
>         }
>     }
> 
>     return false;
> }
> ... which does the same checks, and is used to actually enable the throttling.
> 
> You could use the above helper to set/unset @io_limits_enabled after the real parsing is done.
> [Pradeep Jagadeesh]OK.
> 
> > > +static bool throttle9p_check_for_wait(FsThrottle *fst, bool
> > > +is_write) {
> > > +    if (fst->any_timer_armed[is_write]) {
> > > +        return true;
> > > +    } else {
> > > +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> > > +    }
> > > +}
> > > +
> > > +static void throttle9p_schedule_next_request(FsThrottle *fst, 
> > > +bool
> > > +is_write) {
> > > +    bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > > +    if (!fst->pending_reqs[is_write]) {
> > > +        return;
> > > +    }
> > > +    if (!must_wait) {
> > > +        if (qemu_in_coroutine() &&
> > > +            qemu_co_queue_next(&fst->throttled_reqs[is_write])) {
> > > +            ;
> > > +       } else {
> > > +           int64_t now = qemu_clock_get_ns(fst->tt.clock_type);
> > > +           timer_mod(fst->tt.timers[is_write], now + 1);
> > > +           fst->any_timer_armed[is_write] = true;
> > > +       }
> > > +   }
> > > +}
> > > +
> > > +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write) {
> > > +    bool empty_queue;
> > > +    qemu_mutex_lock(&fst->lock);
> > > +    fst->any_timer_armed[is_write] = false;
> > > +    qemu_mutex_unlock(&fst->lock);
> > > +    empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]);
> > > +    if (empty_queue) {
> > > +        qemu_mutex_lock(&fst->lock);
> > > +        throttle9p_schedule_next_request(fst, is_write);
> > > +        qemu_mutex_unlock(&fst->lock);
> > > +    }
> > > +}
> > > +
> > > +
> > > +bool throttle9p_get_io_limits_state(FsThrottle *fst)
> > 
> > The name looks a bit strange, since this helper simply returns a boolean flag.
> > I guess throttle9p_enabled() is enough.
> > [Pradeep Jagadeesh] OK
> >   
> > > +{
> > > +
> > > +    return fst->io_limits_enabled; }
> > > +
> > > +static void throttle9p_read_timer_cb(void *opaque) {
> > > +    throttle9p_timer_cb(opaque, false); }
> > > +
> > > +static void throttle9p_write_timer_cb(void *opaque) {
> > > +    throttle9p_timer_cb(opaque, true); }
> > > +
> > > +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle 
> > > +*fst) {
> > 
> > This function can fail, it should have a return value (0 or -1).
> > [Pradeep Jagadeesh] OK
> >   
> > > +    memset(&fst->ts, 1, sizeof(fst->ts));
> > > +    memset(&fst->tt, 1, sizeof(fst->tt));
> > > +    memset(&fst->cfg, 0, sizeof(fst->cfg));
> > 
> > Same remark as Claudio.
> > [Pradeep Jagadeesh] OK, will address.
> >   
> > > +    fst->aioctx = qemu_get_aio_context();
> > > +
> > > +    if (!fst->aioctx) {
> > > +        error_report("Failed to create AIO Context");
> > > +        exit(1);
> > > +    }
> > > +    throttle_init(&fst->ts);
> > > +    throttle_timers_init(&fst->tt,
> > > +                         fst->aioctx,
> > > +                         QEMU_CLOCK_REALTIME,
> > > +                         throttle9p_read_timer_cb,
> > > +                         throttle9p_write_timer_cb,
> > > +                         fst);
> > 
> > Shouldn't all this be done later when we know the config is valid ?
> > [Pradeep Jagadeesh] Yes, fixed.
> >   
> > > +    throttle_config_init(&fst->cfg);
> > > +    g_assert(throttle_is_valid(&fst->cfg, NULL));
> > > +    
> > 
> > What's the point in calling throttle_is_valid() on a freshly initialized config ?
> > [Pradeep Jagadeesh] Removed
> >   
> > > +    qemu_co_queue_init(&fst->throttled_reqs[0]);
> > > +    qemu_co_queue_init(&fst->throttled_reqs[1]);
> > 
> > Later, when the config is valid ?
> > [Pradeep Jagadeesh] Done
> >   
> > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> > > +          qemu_opt_get_number(opts, "bps", 0);
> > > +    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
> > > +          qemu_opt_get_number(opts, "bps_rd", 0);
> > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> > > +          qemu_opt_get_number(opts, "bps_wr", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> > > +          qemu_opt_get_number(opts, "iops", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> > > +          qemu_opt_get_number(opts, "iops_rd", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> > > +          qemu_opt_get_number(opts, "iops_wr", 0);
> > > +
> > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> > > +          qemu_opt_get_number(opts, "bps_max", 0);
> > > +    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
> > > +          qemu_opt_get_number(opts, "bps_rd_max", 0);
> > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> > > +          qemu_opt_get_number(opts, "bps_wr_max", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> > > +          qemu_opt_get_number(opts, "iops_max", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_READ].max =
> > > +          qemu_opt_get_number(opts, "iops_rd_max", 0);
> > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> > > +          qemu_opt_get_number(opts, "iops_wr_max", 0);
> > > +
> > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
> > > +          qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
> > > +    fst->cfg.buckets[THROTTLE_BPS_READ].burst_length  =
> > > +          qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
> > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
> > > +          qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
> > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
> > > +          qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
> > > +    fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
> > > +          qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
> > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
> > > +          qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
> > > +    fst->cfg.op_size =
> > > +          qemu_opt_get_number(opts, "iops_size", 0);
> > > +
> > > +    throttle_config(&fst->ts, &fst->tt, &fst->cfg);
> > 
> > Let's set the config later, when we we it is valid.
> > [Pradeep Jagadeesh] OK
> >   
> > > +    if (!throttle_is_valid(&fst->cfg, NULL)) {
> > 
> > You should pass an Error * to throttle_is_valid() to be able to 
> > report the misconfiguration to the user. I guess it is okay to print 
> > it here using
> > error_repport_err() (see include/qapi/error.h) and return -1.
> > [Pradeep Jagadeesh] OK
> >   
> > > +        return;
> > > +    }
> > > +
> > > +    g_assert(fst->tt.timers[0]);
> > > +    g_assert(fst->tt.timers[1]);
> > 
> > These are really not needed since, timers are set with:
> > 
> >     QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer));
> > 
> > and g_malloc0() never returns NULL when passed a non-nul size. It 
> > calls
> > g_assert() internally instead.
> > [Pradeep Jagadeesh] OK
> >   
> > > +    fst->pending_reqs[0] = 0;
> > > +    fst->pending_reqs[1] = 0;
> > > +    fst->any_timer_armed[0] = false;
> > > +    fst->any_timter_armed[1] = false;
> > > +    qemu_mutex_init(&fst->lock);
> > 
> > And there you may set the enabled flag.
> > [Pradeep Jagadeesh] Explained before.
> >   
> > > +}
> > > +
> > > +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t
> > > +bytes) {
> > > +    if (fst->io_limits_enabled) {
> > 
> > throttle9p_enabled(fst)
> > [Pradeep Jagadeesh] OK
> >   
> > > +        qemu_mutex_lock(&fst->lock);
> > > +        bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > > +        if (must_wait || fst->pending_reqs[is_write]) {
> > > +            fst->pending_reqs[is_write]++;
> > > +            qemu_mutex_unlock(&fst->lock);
> > > +            qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> > > +            qemu_mutex_lock(&fst->lock);
> > > +            fst->pending_reqs[is_write]--;
> > > +       }
> > > +       throttle_account(&fst->ts, is_write, bytes);
> > > +       throttle9p_schedule_next_request(fst, is_write);
> > > +       qemu_mutex_unlock(&fst->lock);
> > > +    }
> > > +}
> > > +
> > > +void throttle9p_cleanup(FsThrottle *fst) {
> > > +    throttle_timers_destroy(&fst->tt);
> > > +    qemu_mutex_destroy(&fst->lock); }
> > > diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h new 
> > > file mode 100644 index 0000000..0f7551d
> > > --- /dev/null
> > > +++ b/hw/9pfs/9p-throttle.h
> > > @@ -0,0 +1,46 @@
> > > +/*
> > > + * 9P Throttle
> > > + *
> > > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > > + *
> > > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 
> > > +2 or
> > > + * (at your option) any later version.
> > > + *
> > > + * See the COPYING file in the top-level directory for details.
> > > + *
> > > + */
> > > +
> > > +#ifndef _9P_THROTTLE_H
> > > +#define _9P_THROTTLE_H
> > > +
> > > +#include <stdbool.h>
> > > +#include <stdint.h>
> > 
> > These includes are forbidden. They are already handled by "qemu/osdep.h" which is supposed to be included by all .c files.
> > [Pradeep Jagadeesh] OK
> >   
> > > +#include "block/aio.h"
> > > +#include "qemu/main-loop.h"
> > > +#include "qemu/coroutine.h"
> > > +#include "qemu/throttle.h"
> > > +
> > > +typedef struct FsThrottle {
> > > +    ThrottleState ts;
> > > +    ThrottleTimers tt;
> > > +    AioContext   *aioctx;
> > > +    ThrottleConfig cfg;
> > > +    bool io_limits_enabled;
> > 
> > Let's simply call this enabled.
> > [Pradeep Jagadeesh] OK
> >   
> > > +    CoQueue      throttled_reqs[2];
> > > +    unsigned     pending_reqs[2];
> > > +    bool any_timer_armed[2];
> > > +    QemuMutex lock;
> > > +} FsThrottle;
> > > +
> > > +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *);
> > > +
> > > +bool throttle9p_get_io_limits_state(FsThrottle *);
> > > +
> > > +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *);
> > > +
> > > +void throttle9p_request(FsThrottle *, bool , ssize_t);
> > > +
> > > +void throttle9p_cleanup(FsThrottle *); #endif /* _9P_THROTTLE_H 
> > > +*/
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index dfe293d..7be926a
> > > 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
> > >          error_setg(errp, "share path %s is not a directory", fse->path);
> > >          goto out;
> > >      }
> > > +
> > > +    /* Throttle structure initialization */
> > 
> > Not sure this comment is helpful.
> > [Pradeep Jagadeesh] Just to give a hint, where the throttle 
> > structures are getting Populated. May be some other comment?
> >   
> 
> Unless I'm missing something, the throttle structures aren't getting populated here...
> [Pradeep Jagadeesh] Yes, not getting populated here. Initializing the 
> throttle structure inside the 9pfs_device structure, while realizing a 9pfs_device.
> 
> > > +    s->ctx.fst = &fse->fst;

The code is self-explanatory. No need to paraphrase.
[Pradeep Jagadeesh] OK

Cheers.

--
Greg

> > > +
> > >      v9fs_path_free(&path);
> > >  
> > >      rc = 0;
> > > @@ -3504,6 +3508,9 @@ out:
> > >  
> > >  void v9fs_device_unrealize_common(V9fsState *s, Error **errp)  {
> > > +    if (throttle9p_get_io_limits_state(s->ctx.fst)) {
> > > +        throttle9p_cleanup(s->ctx.fst);
> > > +    }
> > >      g_free(s->ctx.fs_root);
> > >      g_free(s->tag);
> > >  }
> > > diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs index
> > > da0ae0c..07523f1 100644
> > > --- a/hw/9pfs/Makefile.objs
> > > +++ b/hw/9pfs/Makefile.objs
> > > @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o 
> > > common-obj-y += coxattr.o 9p-synth.o
> > >  common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o  common-obj-y
> > > += 9p-proxy.o
> > > +common-obj-y += 9p-throttle.o
> > >  
> > >  obj-y += virtio-9p-device.o
> >   
>

On Tue, 13 Sep 2016 09:17:49 +0000
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:

> Hi Greg,
> 
> Replies inline
> 
> Cheers,
> Pradeep
> -----Original Message-----
> From: Greg Kurz [mailto:groug@kaod.org] 
> Sent: Tuesday, September 13, 2016 10:52 AM
> To: Pradeep Jagadeesh
> Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver
> 
> On Mon, 12 Sep 2016 16:08:43 +0000
> Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:
> 
> > Replies inline Greg.
> > 
> > Thanks & Regards,
> > Pradeep
> >   
> 
> Hi Pradeep,
> 
> > -----Original Message-----
> > From: Greg Kurz [mailto:groug@kaod.org]
> > Sent: Monday, September 12, 2016 4:19 PM
> > To: Pradeep Jagadeesh
> > Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; 
> > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local 
> > driver
> > 
> > On Mon, 12 Sep 2016 12:52:55 +0000
> > Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:
> >   
> > > Hi Greg,
> > > 
> > > Thanks for looking into the patch. Please look at the replies inline.
> > > 
> > > Regards,
> > > Pradeep
> > >     
> > 
> > Hi Pradeep,
> > 
> > Remarks and answers below.
> > 
> > Cheers.
> > 
> > --
> > Greg
> >   
> > > -----Original Message-----
> > > From: Greg Kurz [mailto:groug@kaod.org]
> > > Sent: Friday, September 09, 2016 5:29 PM
> > > To: Pradeep Jagadeesh
> > > Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; 
> > > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> > > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local 
> > > driver
> > > 
> > > On Fri,  9 Sep 2016 05:10:27 -0400
> > > Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:
> > >     
> > > > Uses throttling APIs to limit I/O bandwidth and number of 
> > > > operations on the devices which use 9p-local driver.
> > > > 
> > > > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > > ---  
> > > 
> > > Hi Pradeep,
> > > 
> > > Please find some remarks below. I haven't dived deep enough to see if this actually works. Maybe Berto can provide some feedback ?
> > > 
> > > Cheers.
> > > 
> > > --
> > > Greg
> > >     
> > > >  fsdev/file-op-9p.h      |   3 +
> > > >  fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
> > > >  hw/9pfs/9p-local.c      |  18 ++++-
> > > >  hw/9pfs/9p-throttle.c   | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  hw/9pfs/9p-throttle.h   |  46 +++++++++++    
> > 
> > I forgot to mention it, but this throttling implementation isn't related to the 9P device but to the fsdev device: 9p-throttle.[ch] should move to the fsdev directory and be renamed to fsdev-throttle.[ch].
> > 
> > [Pradeep Jagadeesh] Absolutely right and make sense, I will move the throttle code to fsdev directory.
> >   
> > > >  hw/9pfs/9p.c            |   7 ++
> > > >  hw/9pfs/Makefile.objs   |   1 +
> > > >  7 files changed, 326 insertions(+), 2 deletions(-)  create mode
> > > > 100644 hw/9pfs/9p-throttle.c  create mode 100644 
> > > > hw/9pfs/9p-throttle.h
> > > > 
> > > > 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.
> > > > 
> > > > v1 -> v2:
> > > > 
> > > > -Fixed FsContext redeclaration issue -Removed couple of function 
> > > > declarations from 9p-throttle.h -Fixed some of the .help messages
> > > > 
> > > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index 
> > > > 6db9fea..e86b91a 100644
> > > > --- a/fsdev/file-op-9p.h
> > > > +++ b/fsdev/file-op-9p.h
> > > > @@ -17,6 +17,7 @@
> > > >  #include <dirent.h>
> > > >  #include <utime.h>
> > > >  #include <sys/vfs.h>
> > > > +#include "hw/9pfs/9p-throttle.h"
> > > >  
> > > >  #define SM_LOCAL_MODE_BITS    0600
> > > >  #define SM_LOCAL_DIR_MODE_BITS    0700
> > > > @@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
> > > >      char *path;
> > > >      int export_flags;
> > > >      FileOperations *ops;
> > > > +    FsThrottle fst;
> > > >  } FsDriverEntry;
> > > >  
> > > >  typedef struct FsContext
> > > > @@ -83,6 +85,7 @@ typedef struct FsContext
> > > >      int export_flags;
> > > >      struct xattr_operations **xops;
> > > >      struct extended_ops exops;
> > > > +    FsThrottle *fst;
> > > >      /* fs driver specific data */
> > > >      void *private;
> > > >  } FsContext;
> > > > diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c 
> > > > index
> > > > 1dd8c7a..2774855 100644
> > > > --- a/fsdev/qemu-fsdev-opts.c
> > > > +++ b/fsdev/qemu-fsdev-opts.c
> > > > @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
> > > >          }, {
> > > >              .name = "sock_fd",
> > > >              .type = QEMU_OPT_NUMBER,
> > > > +        }, {
> > > > +            .name = "",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit total bytes per second",
> > > > +        }, {
> > > > +            .name = "bps_rd",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit read bytes per second",
> > > > +        }, {
> > > > +            .name = "bps_wr",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit write bytes per second",
> > > > +        }, {
> > > > +            .name = "iops",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit total io operations per second",
> > > > +        }, {
> > > > +            .name = "iops_rd",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit read operations per second ",
> > > > +        }, {
> > > > +            .name = "iops_wr",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit write operations per second",
> > > > +        }, {
> > > > +            .name = "bps_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum bytes burst",
> > > > +        }, {
> > > > +            .name = "bps_rd_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum bytes read burst",
> > > > +        }, {
> > > > +            .name = "bps_wr_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum bytes write burst",
> > > > +        }, {
> > > > +            .name = "iops_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum io operations burst",
> > > > +        }, {
> > > > +            .name = "iops_rd_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum io operations read burst",
> > > > +        }, {
> > > > +            .name = "iops_wr_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum io operations write burst",
> > > > +        }, {
> > > > +            .name = "iops_size",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "io iops-size",
> > > >          },
> > > >        
> > > 
> > > In case we end up sharing code with blockdev as suggested by Eric, maybe you can use the same QMP friendly naming scheme ("throttling.*") and the same help strings as well.
> > > 
> > > [Pradeep Jagadeesh]
> > > [Pradeep Jagadeesh] You mean to say I should re-name the options how 
> > > they are done for block devices? Or mentioning the cli options itself as throttling.*?
> > >                                      
> > 
> > I meant the latter. IIUC, the renaming done in blockdev is for compatibility only, as stated in the changelog of commit 57975222b6.
> > [Pradeep Jagadeesh] OK, I will rename them as well. But there will be more code duplication.
> > May be we have to merge the code later. We have to come up with a 
> > structure to fit all different kind of backend devices. (those who wants to use throttling apis).
> >   
> 
> That's the point. Let's start with the qemu_fsdev_opts list, so that it contains the same throttling options as qemu_common_drive_opts. Later, we'll probably even move the descriptions of these common throttling options to an include/qemu/throttle-options.h header file.
> [Pradeep Jagadeesh] OK, I wanted one more thing to get clarified.The Qemu CLI options look like
> throttling.bps-read or just bps_rd?, Because there is a renaming function inside the blockdev
> that renames options for example from bps_rd to throttling.bps-read. 
> 

As said before, bps_rd and friends are old names, and the drive_new() function
renames these old names to the new ones for compatibility.

commit 57975222b6a928dd4a4a8a7b37093cc8ecba5045
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Wed Jul 17 14:41:54 2013 +0200

    blockdev: Rename I/O throttling options for QMP
    
    In QMP, we want to use dashes instead of underscores in QMP argument
    names, and use nested options for throttling.
    
    The new option names affect the command line as well, but for
    compatibility drive_init() will convert the old option names before
    calling into the code that will be shared between -drive and
    blockdev-add.

Let's start the fsdev throttling implementation with the new names
directly.

> > > >          { /*End of list */ }
> > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index
> > > > 3f271fc..49c2819 100644
> > > > --- a/hw/9pfs/9p-local.c
> > > > +++ b/hw/9pfs/9p-local.c
> > > > @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
> > > >                              const struct iovec *iov,
> > > >                              int iovcnt, off_t offset)  {
> > > > +    throttle9p_request(ctx->fst, false, iov->iov_len);
> > > > +  
> 
> Maybe this could move directly to v9fs_co_preadv(), so that other backends just need to be enabled to use throttling.
> [Pradeep Jagadeesh] OK
> 
> > > >  #ifdef CONFIG_PREADV
> > > >      return preadv(fs->fd, iov, iovcnt, offset);  #else @@ -436,8
> > > > +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, 
> > > > +V9fsFidOpenState *fs,
> > > >                               const struct iovec *iov,
> > > >                               int iovcnt, off_t offset)  {
> > > > -    ssize_t ret
> > > > -;
> > > > +    ssize_t ret;
> > > > +
> > > > +    throttle9p_request(ctx->fst, true, iov->iov_len);
> > > > +  
> 
> And this may move to v9fs_co_pwritev().
> [Pradeep Jagadeesh] OK
> 
> > > >  #ifdef CONFIG_PREADV
> > > >      ret = pwritev(fs->fd, iov, iovcnt, offset);  #else @@ -1213,6
> > > > +1217,9 @@ 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");
> > > >  
> > > > +    /* get the throttle structure */  
> > > 
> > > Not sure this comment is helpful.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +    FsThrottle *fst = &fse->fst;
> > > > +
> > > >      if (!sec_model) {
> > > >          error_report("Security model not specified, local fs needs security model");
> > > >          error_printf("valid options are:"
> > > > @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
> > > >          error_report("fsdev: No path specified");
> > > >          return -1;
> > > >      }
> > > > +
> > > > +    throttle9p_enable_io_limits(opts, fst);
> > > > +
> > > > +    if (throttle9p_get_io_limits_state(fst)) {
> > > > +        throttle9p_configure_iolimits(opts, fst);
> > > > +    }
> > > > +
> > > >      fse->path = g_strdup(path);
> > > >  
> > > >      return 0;
> > > > diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c new 
> > > > file mode 100644 index 0000000..f2a7ba5
> > > > --- /dev/null
> > > > +++ b/hw/9pfs/9p-throttle.c
> > > > @@ -0,0 +1,201 @@
> > > > +/*
> > > > + * 9P Throttle
> > > > + *
> > > > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > > > + *
> > > > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 
> > > > +2 or
> > > > + * (at your option) any later version.
> > > > + *
> > > > + * See the COPYING file in the top-level directory for details.
> > > > + *
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> > > > +#include "qemu/cutils.h"
> > > > +#include "qemu/error-report.h"
> > > > +#include <libgen.h>
> > > > +#include <linux/fs.h>
> > > > +#include <sys/ioctl.h>
> > > > +#include <string.h>
> > > > +#include "fsdev/file-op-9p.h"
> > > > +#include "9p-throttle.h"
> > > > +      
> > > 
> > > Only "qemu/osdep.h", "qemu/error-report.h" and "9p-throttle.h" are actually needed.
> > >     
> > > > +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst) {
> > > > +    const float bps = qemu_opt_get_number(opts, "bps", 0);
> > > > +    const float iops = qemu_opt_get_number(opts, "iops", 0);
> > > > +    const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0);
> > > > +    const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0);
> > > > +    const float rdops = qemu_opt_get_number(opts, "iops_rd", 0);
> > > > +    const float wrops = qemu_opt_get_number(opts, "iops_wr", 0);
> > > > +
> > > > +    if (bps > 0 || iops > 0 || rdbps > 0 ||
> > > > +        wrbps > 0 || rdops > 0 || wrops > 0) {
> > > > +        fst->io_limits_enabled = true;
> > > > +    } else {
> > > > +        fst->io_limits_enabled = false;
> > > > +    }
> > > > +}
> > > > +      
> > > 
> > > This function should be named *_parse_* but I'm not even sure it is actually needed (see below).
> > > [Pradeep Jagadeesh] This function is used to avoid the parsing of the options if not enabled.
> > > From my understanding of code block devices throttling code, 
> > > irrespective of the throttle options are enabled or not the parse 
> > > functions are called for all the devices. I am trying to avoid that by checking these variables at the before I get into parsing and setting up the throttle structures.
> > >     
> > 
> > And so, we would do a pre-parsing here before doing a full fledged parsing later ?
> > This still doesn't look very useful, but it is certainly error prone IMHO.
> > [Pradeep Jagadeesh] Yes, before populating the structures, I wanted to 
> > check is throttle enabled to this particular fsdev device or not?. If 
> > enabled then I go-head with the full populate and initialize the structures.
> > So, shall I set without pre-checking them for now?
> >   
> 
> Yes.
> [Pradeep Jagadeesh] OK
> 
> > However, conditionally setting up the throttle structures and letting the backend know that throttling is enabled, are needed indeed.
> > 
> > Looking again at how block devices use the throttle API, I see that there's:
> > 
> > /* Does any throttling must be done
> >  *
> >  * @cfg: the throttling configuration to inspect
> >  * @ret: true if throttling must be done else false  */ bool throttle_enabled(ThrottleConfig *cfg) {
> >     int i;
> > 
> >     for (i = 0; i < BUCKETS_COUNT; i++) {
> >         if (cfg->buckets[i].avg > 0) {
> >             return true;
> >         }
> >     }
> > 
> >     return false;
> > }
> > ... which does the same checks, and is used to actually enable the throttling.
> > 
> > You could use the above helper to set/unset @io_limits_enabled after the real parsing is done.
> > [Pradeep Jagadeesh]OK.
> >   
> > > > +static bool throttle9p_check_for_wait(FsThrottle *fst, bool
> > > > +is_write) {
> > > > +    if (fst->any_timer_armed[is_write]) {
> > > > +        return true;
> > > > +    } else {
> > > > +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void throttle9p_schedule_next_request(FsThrottle *fst, 
> > > > +bool
> > > > +is_write) {
> > > > +    bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > > > +    if (!fst->pending_reqs[is_write]) {
> > > > +        return;
> > > > +    }
> > > > +    if (!must_wait) {
> > > > +        if (qemu_in_coroutine() &&
> > > > +            qemu_co_queue_next(&fst->throttled_reqs[is_write])) {
> > > > +            ;
> > > > +       } else {
> > > > +           int64_t now = qemu_clock_get_ns(fst->tt.clock_type);
> > > > +           timer_mod(fst->tt.timers[is_write], now + 1);
> > > > +           fst->any_timer_armed[is_write] = true;
> > > > +       }
> > > > +   }
> > > > +}
> > > > +
> > > > +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write) {
> > > > +    bool empty_queue;
> > > > +    qemu_mutex_lock(&fst->lock);
> > > > +    fst->any_timer_armed[is_write] = false;
> > > > +    qemu_mutex_unlock(&fst->lock);
> > > > +    empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]);
> > > > +    if (empty_queue) {
> > > > +        qemu_mutex_lock(&fst->lock);
> > > > +        throttle9p_schedule_next_request(fst, is_write);
> > > > +        qemu_mutex_unlock(&fst->lock);
> > > > +    }
> > > > +}
> > > > +
> > > > +
> > > > +bool throttle9p_get_io_limits_state(FsThrottle *fst)  
> > > 
> > > The name looks a bit strange, since this helper simply returns a boolean flag.
> > > I guess throttle9p_enabled() is enough.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +{
> > > > +
> > > > +    return fst->io_limits_enabled; }
> > > > +
> > > > +static void throttle9p_read_timer_cb(void *opaque) {
> > > > +    throttle9p_timer_cb(opaque, false); }
> > > > +
> > > > +static void throttle9p_write_timer_cb(void *opaque) {
> > > > +    throttle9p_timer_cb(opaque, true); }
> > > > +
> > > > +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle 
> > > > +*fst) {  
> > > 
> > > This function can fail, it should have a return value (0 or -1).
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +    memset(&fst->ts, 1, sizeof(fst->ts));
> > > > +    memset(&fst->tt, 1, sizeof(fst->tt));
> > > > +    memset(&fst->cfg, 0, sizeof(fst->cfg));  
> > > 
> > > Same remark as Claudio.
> > > [Pradeep Jagadeesh] OK, will address.
> > >     
> > > > +    fst->aioctx = qemu_get_aio_context();
> > > > +
> > > > +    if (!fst->aioctx) {
> > > > +        error_report("Failed to create AIO Context");
> > > > +        exit(1);
> > > > +    }
> > > > +    throttle_init(&fst->ts);
> > > > +    throttle_timers_init(&fst->tt,
> > > > +                         fst->aioctx,
> > > > +                         QEMU_CLOCK_REALTIME,
> > > > +                         throttle9p_read_timer_cb,
> > > > +                         throttle9p_write_timer_cb,
> > > > +                         fst);  
> > > 
> > > Shouldn't all this be done later when we know the config is valid ?
> > > [Pradeep Jagadeesh] Yes, fixed.
> > >     
> > > > +    throttle_config_init(&fst->cfg);
> > > > +    g_assert(throttle_is_valid(&fst->cfg, NULL));
> > > > +      
> > > 
> > > What's the point in calling throttle_is_valid() on a freshly initialized config ?
> > > [Pradeep Jagadeesh] Removed
> > >     
> > > > +    qemu_co_queue_init(&fst->throttled_reqs[0]);
> > > > +    qemu_co_queue_init(&fst->throttled_reqs[1]);  
> > > 
> > > Later, when the config is valid ?
> > > [Pradeep Jagadeesh] Done
> > >     
> > > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> > > > +          qemu_opt_get_number(opts, "bps", 0);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
> > > > +          qemu_opt_get_number(opts, "bps_rd", 0);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> > > > +          qemu_opt_get_number(opts, "bps_wr", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> > > > +          qemu_opt_get_number(opts, "iops", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> > > > +          qemu_opt_get_number(opts, "iops_rd", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> > > > +          qemu_opt_get_number(opts, "iops_wr", 0);
> > > > +
> > > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> > > > +          qemu_opt_get_number(opts, "bps_max", 0);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
> > > > +          qemu_opt_get_number(opts, "bps_rd_max", 0);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> > > > +          qemu_opt_get_number(opts, "bps_wr_max", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> > > > +          qemu_opt_get_number(opts, "iops_max", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_READ].max =
> > > > +          qemu_opt_get_number(opts, "iops_rd_max", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> > > > +          qemu_opt_get_number(opts, "iops_wr_max", 0);
> > > > +
> > > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
> > > > +          qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_READ].burst_length  =
> > > > +          qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
> > > > +          qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
> > > > +          qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
> > > > +          qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
> > > > +          qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
> > > > +    fst->cfg.op_size =
> > > > +          qemu_opt_get_number(opts, "iops_size", 0);
> > > > +
> > > > +    throttle_config(&fst->ts, &fst->tt, &fst->cfg);  
> > > 
> > > Let's set the config later, when we we it is valid.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +    if (!throttle_is_valid(&fst->cfg, NULL)) {  
> > > 
> > > You should pass an Error * to throttle_is_valid() to be able to 
> > > report the misconfiguration to the user. I guess it is okay to print 
> > > it here using
> > > error_repport_err() (see include/qapi/error.h) and return -1.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    g_assert(fst->tt.timers[0]);
> > > > +    g_assert(fst->tt.timers[1]);  
> > > 
> > > These are really not needed since, timers are set with:
> > > 
> > >     QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer));
> > > 
> > > and g_malloc0() never returns NULL when passed a non-nul size. It 
> > > calls
> > > g_assert() internally instead.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +    fst->pending_reqs[0] = 0;
> > > > +    fst->pending_reqs[1] = 0;
> > > > +    fst->any_timer_armed[0] = false;
> > > > +    fst->any_timter_armed[1] = false;
> > > > +    qemu_mutex_init(&fst->lock);  
> > > 
> > > And there you may set the enabled flag.
> > > [Pradeep Jagadeesh] Explained before.
> > >     
> > > > +}
> > > > +
> > > > +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t
> > > > +bytes) {
> > > > +    if (fst->io_limits_enabled) {  
> > > 
> > > throttle9p_enabled(fst)
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +        qemu_mutex_lock(&fst->lock);
> > > > +        bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > > > +        if (must_wait || fst->pending_reqs[is_write]) {
> > > > +            fst->pending_reqs[is_write]++;
> > > > +            qemu_mutex_unlock(&fst->lock);
> > > > +            qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> > > > +            qemu_mutex_lock(&fst->lock);
> > > > +            fst->pending_reqs[is_write]--;
> > > > +       }
> > > > +       throttle_account(&fst->ts, is_write, bytes);
> > > > +       throttle9p_schedule_next_request(fst, is_write);
> > > > +       qemu_mutex_unlock(&fst->lock);
> > > > +    }
> > > > +}
> > > > +
> > > > +void throttle9p_cleanup(FsThrottle *fst) {
> > > > +    throttle_timers_destroy(&fst->tt);
> > > > +    qemu_mutex_destroy(&fst->lock); }
> > > > diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h new 
> > > > file mode 100644 index 0000000..0f7551d
> > > > --- /dev/null
> > > > +++ b/hw/9pfs/9p-throttle.h
> > > > @@ -0,0 +1,46 @@
> > > > +/*
> > > > + * 9P Throttle
> > > > + *
> > > > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > > > + *
> > > > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 
> > > > +2 or
> > > > + * (at your option) any later version.
> > > > + *
> > > > + * See the COPYING file in the top-level directory for details.
> > > > + *
> > > > + */
> > > > +
> > > > +#ifndef _9P_THROTTLE_H
> > > > +#define _9P_THROTTLE_H
> > > > +
> > > > +#include <stdbool.h>
> > > > +#include <stdint.h>  
> > > 
> > > These includes are forbidden. They are already handled by "qemu/osdep.h" which is supposed to be included by all .c files.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +#include "block/aio.h"
> > > > +#include "qemu/main-loop.h"
> > > > +#include "qemu/coroutine.h"
> > > > +#include "qemu/throttle.h"
> > > > +
> > > > +typedef struct FsThrottle {
> > > > +    ThrottleState ts;
> > > > +    ThrottleTimers tt;
> > > > +    AioContext   *aioctx;
> > > > +    ThrottleConfig cfg;
> > > > +    bool io_limits_enabled;  
> > > 
> > > Let's simply call this enabled.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +    CoQueue      throttled_reqs[2];
> > > > +    unsigned     pending_reqs[2];
> > > > +    bool any_timer_armed[2];
> > > > +    QemuMutex lock;
> > > > +} FsThrottle;
> > > > +
> > > > +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *);
> > > > +
> > > > +bool throttle9p_get_io_limits_state(FsThrottle *);
> > > > +
> > > > +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *);
> > > > +
> > > > +void throttle9p_request(FsThrottle *, bool , ssize_t);
> > > > +
> > > > +void throttle9p_cleanup(FsThrottle *); #endif /* _9P_THROTTLE_H 
> > > > +*/
> > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index dfe293d..7be926a
> > > > 100644
> > > > --- a/hw/9pfs/9p.c
> > > > +++ b/hw/9pfs/9p.c
> > > > @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
> > > >          error_setg(errp, "share path %s is not a directory", fse->path);
> > > >          goto out;
> > > >      }
> > > > +
> > > > +    /* Throttle structure initialization */  
> > > 
> > > Not sure this comment is helpful.
> > > [Pradeep Jagadeesh] Just to give a hint, where the throttle 
> > > structures are getting Populated. May be some other comment?
> > >     
> > 
> > Unless I'm missing something, the throttle structures aren't getting populated here...
> > [Pradeep Jagadeesh] Yes, not getting populated here. Initializing the 
> > throttle structure inside the 9pfs_device structure, while realizing a 9pfs_device.
> >   
> > > > +    s->ctx.fst = &fse->fst;  
> 
> The code is self-explanatory. No need to paraphrase.
> [Pradeep Jagadeesh] OK
> 
> Cheers.
> 
> --
> Greg
> 
> > > > +
> > > >      v9fs_path_free(&path);
> > > >  
> > > >      rc = 0;
> > > > @@ -3504,6 +3508,9 @@ out:
> > > >  
> > > >  void v9fs_device_unrealize_common(V9fsState *s, Error **errp)  {
> > > > +    if (throttle9p_get_io_limits_state(s->ctx.fst)) {
> > > > +        throttle9p_cleanup(s->ctx.fst);
> > > > +    }
> > > >      g_free(s->ctx.fs_root);
> > > >      g_free(s->tag);
> > > >  }
> > > > diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs index
> > > > da0ae0c..07523f1 100644
> > > > --- a/hw/9pfs/Makefile.objs
> > > > +++ b/hw/9pfs/Makefile.objs
> > > > @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o 
> > > > common-obj-y += coxattr.o 9p-synth.o
> > > >  common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o  common-obj-y
> > > > += 9p-proxy.o
> > > > +common-obj-y += 9p-throttle.o
> > > >  
> > > >  obj-y += virtio-9p-device.o  
> > >     
> >   
>

-----Original Message-----
From: Greg Kurz [mailto:groug@kaod.org] 
Sent: Tuesday, September 13, 2016 2:30 PM
To: Pradeep Jagadeesh
Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver

On Tue, 13 Sep 2016 09:17:49 +0000
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:

> Hi Greg,
> 
> Replies inline
> 
> Cheers,
> Pradeep
> -----Original Message-----
> From: Greg Kurz [mailto:groug@kaod.org]
> Sent: Tuesday, September 13, 2016 10:52 AM
> To: Pradeep Jagadeesh
> Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; 
> qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local 
> driver
> 
> On Mon, 12 Sep 2016 16:08:43 +0000
> Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:
> 
> > Replies inline Greg.
> > 
> > Thanks & Regards,
> > Pradeep
> >   
> 
> Hi Pradeep,
> 
> > -----Original Message-----
> > From: Greg Kurz [mailto:groug@kaod.org]
> > Sent: Monday, September 12, 2016 4:19 PM
> > To: Pradeep Jagadeesh
> > Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; 
> > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local 
> > driver
> > 
> > On Mon, 12 Sep 2016 12:52:55 +0000
> > Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:
> >   
> > > Hi Greg,
> > > 
> > > Thanks for looking into the patch. Please look at the replies inline.
> > > 
> > > Regards,
> > > Pradeep
> > >     
> > 
> > Hi Pradeep,
> > 
> > Remarks and answers below.
> > 
> > Cheers.
> > 
> > --
> > Greg
> >   
> > > -----Original Message-----
> > > From: Greg Kurz [mailto:groug@kaod.org]
> > > Sent: Friday, September 09, 2016 5:29 PM
> > > To: Pradeep Jagadeesh
> > > Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; 
> > > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake
> > > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 
> > > 9p-local driver
> > > 
> > > On Fri,  9 Sep 2016 05:10:27 -0400 Pradeep Jagadeesh 
> > > <pradeepkiruvale@gmail.com> wrote:
> > >     
> > > > Uses throttling APIs to limit I/O bandwidth and number of 
> > > > operations on the devices which use 9p-local driver.
> > > > 
> > > > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > > ---
> > > 
> > > Hi Pradeep,
> > > 
> > > Please find some remarks below. I haven't dived deep enough to see if this actually works. Maybe Berto can provide some feedback ?
> > > 
> > > Cheers.
> > > 
> > > --
> > > Greg
> > >     
> > > >  fsdev/file-op-9p.h      |   3 +
> > > >  fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
> > > >  hw/9pfs/9p-local.c      |  18 ++++-
> > > >  hw/9pfs/9p-throttle.c   | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  hw/9pfs/9p-throttle.h   |  46 +++++++++++    
> > 
> > I forgot to mention it, but this throttling implementation isn't related to the 9P device but to the fsdev device: 9p-throttle.[ch] should move to the fsdev directory and be renamed to fsdev-throttle.[ch].
> > 
> > [Pradeep Jagadeesh] Absolutely right and make sense, I will move the throttle code to fsdev directory.
> >   
> > > >  hw/9pfs/9p.c            |   7 ++
> > > >  hw/9pfs/Makefile.objs   |   1 +
> > > >  7 files changed, 326 insertions(+), 2 deletions(-)  create mode
> > > > 100644 hw/9pfs/9p-throttle.c  create mode 100644 
> > > > hw/9pfs/9p-throttle.h
> > > > 
> > > > 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.
> > > > 
> > > > v1 -> v2:
> > > > 
> > > > -Fixed FsContext redeclaration issue -Removed couple of function 
> > > > declarations from 9p-throttle.h -Fixed some of the .help 
> > > > messages
> > > > 
> > > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index 
> > > > 6db9fea..e86b91a 100644
> > > > --- a/fsdev/file-op-9p.h
> > > > +++ b/fsdev/file-op-9p.h
> > > > @@ -17,6 +17,7 @@
> > > >  #include <dirent.h>
> > > >  #include <utime.h>
> > > >  #include <sys/vfs.h>
> > > > +#include "hw/9pfs/9p-throttle.h"
> > > >  
> > > >  #define SM_LOCAL_MODE_BITS    0600
> > > >  #define SM_LOCAL_DIR_MODE_BITS    0700
> > > > @@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
> > > >      char *path;
> > > >      int export_flags;
> > > >      FileOperations *ops;
> > > > +    FsThrottle fst;
> > > >  } FsDriverEntry;
> > > >  
> > > >  typedef struct FsContext
> > > > @@ -83,6 +85,7 @@ typedef struct FsContext
> > > >      int export_flags;
> > > >      struct xattr_operations **xops;
> > > >      struct extended_ops exops;
> > > > +    FsThrottle *fst;
> > > >      /* fs driver specific data */
> > > >      void *private;
> > > >  } FsContext;
> > > > diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c 
> > > > index
> > > > 1dd8c7a..2774855 100644
> > > > --- a/fsdev/qemu-fsdev-opts.c
> > > > +++ b/fsdev/qemu-fsdev-opts.c
> > > > @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
> > > >          }, {
> > > >              .name = "sock_fd",
> > > >              .type = QEMU_OPT_NUMBER,
> > > > +        }, {
> > > > +            .name = "",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit total bytes per second",
> > > > +        }, {
> > > > +            .name = "bps_rd",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit read bytes per second",
> > > > +        }, {
> > > > +            .name = "bps_wr",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit write bytes per second",
> > > > +        }, {
> > > > +            .name = "iops",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit total io operations per second",
> > > > +        }, {
> > > > +            .name = "iops_rd",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit read operations per second ",
> > > > +        }, {
> > > > +            .name = "iops_wr",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "limit write operations per second",
> > > > +        }, {
> > > > +            .name = "bps_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum bytes burst",
> > > > +        }, {
> > > > +            .name = "bps_rd_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum bytes read burst",
> > > > +        }, {
> > > > +            .name = "bps_wr_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum bytes write burst",
> > > > +        }, {
> > > > +            .name = "iops_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum io operations burst",
> > > > +        }, {
> > > > +            .name = "iops_rd_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum io operations read burst",
> > > > +        }, {
> > > > +            .name = "iops_wr_max",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "maximum io operations write burst",
> > > > +        }, {
> > > > +            .name = "iops_size",
> > > > +            .type = QEMU_OPT_NUMBER,
> > > > +            .help = "io iops-size",
> > > >          },
> > > >        
> > > 
> > > In case we end up sharing code with blockdev as suggested by Eric, maybe you can use the same QMP friendly naming scheme ("throttling.*") and the same help strings as well.
> > > 
> > > [Pradeep Jagadeesh]
> > > [Pradeep Jagadeesh] You mean to say I should re-name the options 
> > > how they are done for block devices? Or mentioning the cli options itself as throttling.*?
> > >                                      
> > 
> > I meant the latter. IIUC, the renaming done in blockdev is for compatibility only, as stated in the changelog of commit 57975222b6.
> > [Pradeep Jagadeesh] OK, I will rename them as well. But there will be more code duplication.
> > May be we have to merge the code later. We have to come up with a 
> > structure to fit all different kind of backend devices. (those who wants to use throttling apis).
> >   
> 
> That's the point. Let's start with the qemu_fsdev_opts list, so that it contains the same throttling options as qemu_common_drive_opts. Later, we'll probably even move the descriptions of these common throttling options to an include/qemu/throttle-options.h header file.
> [Pradeep Jagadeesh] OK, I wanted one more thing to get clarified.The 
> Qemu CLI options look like throttling.bps-read or just bps_rd?, 
> Because there is a renaming function inside the blockdev that renames options for example from bps_rd to throttling.bps-read.
> 

As said before, bps_rd and friends are old names, and the drive_new() function renames these old names to the new ones for compatibility.

commit 57975222b6a928dd4a4a8a7b37093cc8ecba5045
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Wed Jul 17 14:41:54 2013 +0200

    blockdev: Rename I/O throttling options for QMP
    
    In QMP, we want to use dashes instead of underscores in QMP argument
    names, and use nested options for throttling.
    
    The new option names affect the command line as well, but for
    compatibility drive_init() will convert the old option names before
    calling into the code that will be shared between -drive and
    blockdev-add.

Let's start the fsdev throttling implementation with the new names directly.
[Pradeep Jagadeesh] Ok, thanks for the clarification. I will go ahead with the 
as we discussed here.

> > > >          { /*End of list */ }
> > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index
> > > > 3f271fc..49c2819 100644
> > > > --- a/hw/9pfs/9p-local.c
> > > > +++ b/hw/9pfs/9p-local.c
> > > > @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
> > > >                              const struct iovec *iov,
> > > >                              int iovcnt, off_t offset)  {
> > > > +    throttle9p_request(ctx->fst, false, iov->iov_len);
> > > > +  
> 
> Maybe this could move directly to v9fs_co_preadv(), so that other backends just need to be enabled to use throttling.
> [Pradeep Jagadeesh] OK
> 
> > > >  #ifdef CONFIG_PREADV
> > > >      return preadv(fs->fd, iov, iovcnt, offset);  #else @@ 
> > > > -436,8
> > > > +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, 
> > > > +V9fsFidOpenState *fs,
> > > >                               const struct iovec *iov,
> > > >                               int iovcnt, off_t offset)  {
> > > > -    ssize_t ret
> > > > -;
> > > > +    ssize_t ret;
> > > > +
> > > > +    throttle9p_request(ctx->fst, true, iov->iov_len);
> > > > +  
> 
> And this may move to v9fs_co_pwritev().
> [Pradeep Jagadeesh] OK
> 
> > > >  #ifdef CONFIG_PREADV
> > > >      ret = pwritev(fs->fd, iov, iovcnt, offset);  #else @@ 
> > > > -1213,6
> > > > +1217,9 @@ 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");
> > > >  
> > > > +    /* get the throttle structure */
> > > 
> > > Not sure this comment is helpful.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +    FsThrottle *fst = &fse->fst;
> > > > +
> > > >      if (!sec_model) {
> > > >          error_report("Security model not specified, local fs needs security model");
> > > >          error_printf("valid options are:"
> > > > @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
> > > >          error_report("fsdev: No path specified");
> > > >          return -1;
> > > >      }
> > > > +
> > > > +    throttle9p_enable_io_limits(opts, fst);
> > > > +
> > > > +    if (throttle9p_get_io_limits_state(fst)) {
> > > > +        throttle9p_configure_iolimits(opts, fst);
> > > > +    }
> > > > +
> > > >      fse->path = g_strdup(path);
> > > >  
> > > >      return 0;
> > > > diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c new 
> > > > file mode 100644 index 0000000..f2a7ba5
> > > > --- /dev/null
> > > > +++ b/hw/9pfs/9p-throttle.c
> > > > @@ -0,0 +1,201 @@
> > > > +/*
> > > > + * 9P Throttle
> > > > + *
> > > > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > > > + *
> > > > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, 
> > > > +version
> > > > +2 or
> > > > + * (at your option) any later version.
> > > > + *
> > > > + * See the COPYING file in the top-level directory for details.
> > > > + *
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> > > > +#include "qemu/cutils.h"
> > > > +#include "qemu/error-report.h"
> > > > +#include <libgen.h>
> > > > +#include <linux/fs.h>
> > > > +#include <sys/ioctl.h>
> > > > +#include <string.h>
> > > > +#include "fsdev/file-op-9p.h"
> > > > +#include "9p-throttle.h"
> > > > +      
> > > 
> > > Only "qemu/osdep.h", "qemu/error-report.h" and "9p-throttle.h" are actually needed.
> > >     
> > > > +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst) {
> > > > +    const float bps = qemu_opt_get_number(opts, "bps", 0);
> > > > +    const float iops = qemu_opt_get_number(opts, "iops", 0);
> > > > +    const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0);
> > > > +    const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0);
> > > > +    const float rdops = qemu_opt_get_number(opts, "iops_rd", 0);
> > > > +    const float wrops = qemu_opt_get_number(opts, "iops_wr", 
> > > > +0);
> > > > +
> > > > +    if (bps > 0 || iops > 0 || rdbps > 0 ||
> > > > +        wrbps > 0 || rdops > 0 || wrops > 0) {
> > > > +        fst->io_limits_enabled = true;
> > > > +    } else {
> > > > +        fst->io_limits_enabled = false;
> > > > +    }
> > > > +}
> > > > +      
> > > 
> > > This function should be named *_parse_* but I'm not even sure it is actually needed (see below).
> > > [Pradeep Jagadeesh] This function is used to avoid the parsing of the options if not enabled.
> > > From my understanding of code block devices throttling code, 
> > > irrespective of the throttle options are enabled or not the parse 
> > > functions are called for all the devices. I am trying to avoid that by checking these variables at the before I get into parsing and setting up the throttle structures.
> > >     
> > 
> > And so, we would do a pre-parsing here before doing a full fledged parsing later ?
> > This still doesn't look very useful, but it is certainly error prone IMHO.
> > [Pradeep Jagadeesh] Yes, before populating the structures, I wanted 
> > to check is throttle enabled to this particular fsdev device or 
> > not?. If enabled then I go-head with the full populate and initialize the structures.
> > So, shall I set without pre-checking them for now?
> >   
> 
> Yes.
> [Pradeep Jagadeesh] OK
> 
> > However, conditionally setting up the throttle structures and letting the backend know that throttling is enabled, are needed indeed.
> > 
> > Looking again at how block devices use the throttle API, I see that there's:
> > 
> > /* Does any throttling must be done
> >  *
> >  * @cfg: the throttling configuration to inspect
> >  * @ret: true if throttling must be done else false  */ bool throttle_enabled(ThrottleConfig *cfg) {
> >     int i;
> > 
> >     for (i = 0; i < BUCKETS_COUNT; i++) {
> >         if (cfg->buckets[i].avg > 0) {
> >             return true;
> >         }
> >     }
> > 
> >     return false;
> > }
> > ... which does the same checks, and is used to actually enable the throttling.
> > 
> > You could use the above helper to set/unset @io_limits_enabled after the real parsing is done.
> > [Pradeep Jagadeesh]OK.
> >   
> > > > +static bool throttle9p_check_for_wait(FsThrottle *fst, bool
> > > > +is_write) {
> > > > +    if (fst->any_timer_armed[is_write]) {
> > > > +        return true;
> > > > +    } else {
> > > > +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void throttle9p_schedule_next_request(FsThrottle *fst, 
> > > > +bool
> > > > +is_write) {
> > > > +    bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > > > +    if (!fst->pending_reqs[is_write]) {
> > > > +        return;
> > > > +    }
> > > > +    if (!must_wait) {
> > > > +        if (qemu_in_coroutine() &&
> > > > +            qemu_co_queue_next(&fst->throttled_reqs[is_write])) {
> > > > +            ;
> > > > +       } else {
> > > > +           int64_t now = qemu_clock_get_ns(fst->tt.clock_type);
> > > > +           timer_mod(fst->tt.timers[is_write], now + 1);
> > > > +           fst->any_timer_armed[is_write] = true;
> > > > +       }
> > > > +   }
> > > > +}
> > > > +
> > > > +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write) {
> > > > +    bool empty_queue;
> > > > +    qemu_mutex_lock(&fst->lock);
> > > > +    fst->any_timer_armed[is_write] = false;
> > > > +    qemu_mutex_unlock(&fst->lock);
> > > > +    empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]);
> > > > +    if (empty_queue) {
> > > > +        qemu_mutex_lock(&fst->lock);
> > > > +        throttle9p_schedule_next_request(fst, is_write);
> > > > +        qemu_mutex_unlock(&fst->lock);
> > > > +    }
> > > > +}
> > > > +
> > > > +
> > > > +bool throttle9p_get_io_limits_state(FsThrottle *fst)
> > > 
> > > The name looks a bit strange, since this helper simply returns a boolean flag.
> > > I guess throttle9p_enabled() is enough.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +{
> > > > +
> > > > +    return fst->io_limits_enabled; }
> > > > +
> > > > +static void throttle9p_read_timer_cb(void *opaque) {
> > > > +    throttle9p_timer_cb(opaque, false); }
> > > > +
> > > > +static void throttle9p_write_timer_cb(void *opaque) {
> > > > +    throttle9p_timer_cb(opaque, true); }
> > > > +
> > > > +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle
> > > > +*fst) {
> > > 
> > > This function can fail, it should have a return value (0 or -1).
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +    memset(&fst->ts, 1, sizeof(fst->ts));
> > > > +    memset(&fst->tt, 1, sizeof(fst->tt));
> > > > +    memset(&fst->cfg, 0, sizeof(fst->cfg));
> > > 
> > > Same remark as Claudio.
> > > [Pradeep Jagadeesh] OK, will address.
> > >     
> > > > +    fst->aioctx = qemu_get_aio_context();
> > > > +
> > > > +    if (!fst->aioctx) {
> > > > +        error_report("Failed to create AIO Context");
> > > > +        exit(1);
> > > > +    }
> > > > +    throttle_init(&fst->ts);
> > > > +    throttle_timers_init(&fst->tt,
> > > > +                         fst->aioctx,
> > > > +                         QEMU_CLOCK_REALTIME,
> > > > +                         throttle9p_read_timer_cb,
> > > > +                         throttle9p_write_timer_cb,
> > > > +                         fst);
> > > 
> > > Shouldn't all this be done later when we know the config is valid ?
> > > [Pradeep Jagadeesh] Yes, fixed.
> > >     
> > > > +    throttle_config_init(&fst->cfg);
> > > > +    g_assert(throttle_is_valid(&fst->cfg, NULL));
> > > > +      
> > > 
> > > What's the point in calling throttle_is_valid() on a freshly initialized config ?
> > > [Pradeep Jagadeesh] Removed
> > >     
> > > > +    qemu_co_queue_init(&fst->throttled_reqs[0]);
> > > > +    qemu_co_queue_init(&fst->throttled_reqs[1]);
> > > 
> > > Later, when the config is valid ?
> > > [Pradeep Jagadeesh] Done
> > >     
> > > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> > > > +          qemu_opt_get_number(opts, "bps", 0);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
> > > > +          qemu_opt_get_number(opts, "bps_rd", 0);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> > > > +          qemu_opt_get_number(opts, "bps_wr", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> > > > +          qemu_opt_get_number(opts, "iops", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> > > > +          qemu_opt_get_number(opts, "iops_rd", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> > > > +          qemu_opt_get_number(opts, "iops_wr", 0);
> > > > +
> > > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> > > > +          qemu_opt_get_number(opts, "bps_max", 0);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
> > > > +          qemu_opt_get_number(opts, "bps_rd_max", 0);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> > > > +          qemu_opt_get_number(opts, "bps_wr_max", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> > > > +          qemu_opt_get_number(opts, "iops_max", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_READ].max =
> > > > +          qemu_opt_get_number(opts, "iops_rd_max", 0);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> > > > +          qemu_opt_get_number(opts, "iops_wr_max", 0);
> > > > +
> > > > +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
> > > > +          qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_READ].burst_length  =
> > > > +          qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
> > > > +    fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
> > > > +          qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
> > > > +          qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
> > > > +          qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
> > > > +    fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
> > > > +          qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
> > > > +    fst->cfg.op_size =
> > > > +          qemu_opt_get_number(opts, "iops_size", 0);
> > > > +
> > > > +    throttle_config(&fst->ts, &fst->tt, &fst->cfg);
> > > 
> > > Let's set the config later, when we we it is valid.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +    if (!throttle_is_valid(&fst->cfg, NULL)) {
> > > 
> > > You should pass an Error * to throttle_is_valid() to be able to 
> > > report the misconfiguration to the user. I guess it is okay to 
> > > print it here using
> > > error_repport_err() (see include/qapi/error.h) and return -1.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    g_assert(fst->tt.timers[0]);
> > > > +    g_assert(fst->tt.timers[1]);
> > > 
> > > These are really not needed since, timers are set with:
> > > 
> > >     QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer));
> > > 
> > > and g_malloc0() never returns NULL when passed a non-nul size. It 
> > > calls
> > > g_assert() internally instead.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +    fst->pending_reqs[0] = 0;
> > > > +    fst->pending_reqs[1] = 0;
> > > > +    fst->any_timer_armed[0] = false;
> > > > +    fst->any_timter_armed[1] = false;
> > > > +    qemu_mutex_init(&fst->lock);
> > > 
> > > And there you may set the enabled flag.
> > > [Pradeep Jagadeesh] Explained before.
> > >     
> > > > +}
> > > > +
> > > > +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t
> > > > +bytes) {
> > > > +    if (fst->io_limits_enabled) {
> > > 
> > > throttle9p_enabled(fst)
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +        qemu_mutex_lock(&fst->lock);
> > > > +        bool must_wait = throttle9p_check_for_wait(fst, is_write);
> > > > +        if (must_wait || fst->pending_reqs[is_write]) {
> > > > +            fst->pending_reqs[is_write]++;
> > > > +            qemu_mutex_unlock(&fst->lock);
> > > > +            qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> > > > +            qemu_mutex_lock(&fst->lock);
> > > > +            fst->pending_reqs[is_write]--;
> > > > +       }
> > > > +       throttle_account(&fst->ts, is_write, bytes);
> > > > +       throttle9p_schedule_next_request(fst, is_write);
> > > > +       qemu_mutex_unlock(&fst->lock);
> > > > +    }
> > > > +}
> > > > +
> > > > +void throttle9p_cleanup(FsThrottle *fst) {
> > > > +    throttle_timers_destroy(&fst->tt);
> > > > +    qemu_mutex_destroy(&fst->lock); }
> > > > diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h new 
> > > > file mode 100644 index 0000000..0f7551d
> > > > --- /dev/null
> > > > +++ b/hw/9pfs/9p-throttle.h
> > > > @@ -0,0 +1,46 @@
> > > > +/*
> > > > + * 9P Throttle
> > > > + *
> > > > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> > > > + *
> > > > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, 
> > > > +version
> > > > +2 or
> > > > + * (at your option) any later version.
> > > > + *
> > > > + * See the COPYING file in the top-level directory for details.
> > > > + *
> > > > + */
> > > > +
> > > > +#ifndef _9P_THROTTLE_H
> > > > +#define _9P_THROTTLE_H
> > > > +
> > > > +#include <stdbool.h>
> > > > +#include <stdint.h>
> > > 
> > > These includes are forbidden. They are already handled by "qemu/osdep.h" which is supposed to be included by all .c files.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +#include "block/aio.h"
> > > > +#include "qemu/main-loop.h"
> > > > +#include "qemu/coroutine.h"
> > > > +#include "qemu/throttle.h"
> > > > +
> > > > +typedef struct FsThrottle {
> > > > +    ThrottleState ts;
> > > > +    ThrottleTimers tt;
> > > > +    AioContext   *aioctx;
> > > > +    ThrottleConfig cfg;
> > > > +    bool io_limits_enabled;
> > > 
> > > Let's simply call this enabled.
> > > [Pradeep Jagadeesh] OK
> > >     
> > > > +    CoQueue      throttled_reqs[2];
> > > > +    unsigned     pending_reqs[2];
> > > > +    bool any_timer_armed[2];
> > > > +    QemuMutex lock;
> > > > +} FsThrottle;
> > > > +
> > > > +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *);
> > > > +
> > > > +bool throttle9p_get_io_limits_state(FsThrottle *);
> > > > +
> > > > +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *);
> > > > +
> > > > +void throttle9p_request(FsThrottle *, bool , ssize_t);
> > > > +
> > > > +void throttle9p_cleanup(FsThrottle *); #endif /* _9P_THROTTLE_H 
> > > > +*/
> > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index dfe293d..7be926a
> > > > 100644
> > > > --- a/hw/9pfs/9p.c
> > > > +++ b/hw/9pfs/9p.c
> > > > @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
> > > >          error_setg(errp, "share path %s is not a directory", fse->path);
> > > >          goto out;
> > > >      }
> > > > +
> > > > +    /* Throttle structure initialization */
> > > 
> > > Not sure this comment is helpful.
> > > [Pradeep Jagadeesh] Just to give a hint, where the throttle 
> > > structures are getting Populated. May be some other comment?
> > >     
> > 
> > Unless I'm missing something, the throttle structures aren't getting populated here...
> > [Pradeep Jagadeesh] Yes, not getting populated here. Initializing 
> > the throttle structure inside the 9pfs_device structure, while realizing a 9pfs_device.
> >   
> > > > +    s->ctx.fst = &fse->fst;
> 
> The code is self-explanatory. No need to paraphrase.
> [Pradeep Jagadeesh] OK
> 
> Cheers.
> 
> --
> Greg
> 
> > > > +
> > > >      v9fs_path_free(&path);
> > > >  
> > > >      rc = 0;
> > > > @@ -3504,6 +3508,9 @@ out:
> > > >  
> > > >  void v9fs_device_unrealize_common(V9fsState *s, Error **errp)  
> > > > {
> > > > +    if (throttle9p_get_io_limits_state(s->ctx.fst)) {
> > > > +        throttle9p_cleanup(s->ctx.fst);
> > > > +    }
> > > >      g_free(s->ctx.fs_root);
> > > >      g_free(s->tag);
> > > >  }
> > > > diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs index
> > > > da0ae0c..07523f1 100644
> > > > --- a/hw/9pfs/Makefile.objs
> > > > +++ b/hw/9pfs/Makefile.objs
> > > > @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o 
> > > > common-obj-y += coxattr.o 9p-synth.o
> > > >  common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o  
> > > > common-obj-y
> > > > += 9p-proxy.o
> > > > +common-obj-y += 9p-throttle.o
> > > >  
> > > >  obj-y += virtio-9p-device.o
> > >     
> >   
>