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

[Pradeep Jagadeesh]

Hi Greg,

Replies inline

Cheers,
Pradeep
-----Original Message-----
From: Greg Kurz [mailto:address@hidden 
Sent: Tuesday, September 13, 2016 10:52 AM
To: Pradeep Jagadeesh
Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; address@hidden; 
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 <address@hidden> wrote:

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

Hi Pradeep,

> -----Original Message-----
> From: Greg Kurz [mailto:address@hidden
> Sent: Monday, September 12, 2016 4:19 PM
> To: Pradeep Jagadeesh
> Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; 
> address@hidden; 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 <address@hidden> 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:address@hidden
> > Sent: Friday, September 09, 2016 5:29 PM
> > To: Pradeep Jagadeesh
> > Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; 
> > address@hidden; 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 <address@hidden> 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 <address@hidden>
> > > ---
> > 
> > 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 <address@hidden>
> > > + *
> > > + * 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 <address@hidden>
> > > + *
> > > + * 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
> >   
>