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

[Pradeep Jagadeesh]

Replies inline Greg.

Thanks & Regards,
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).

> >          { /*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 <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?

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