Hi Greg, Replies inline
Cheers, Pradeep -----Original Message----- From: Greg Kurz [mailto:gr...@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.jagade...@huawei.com> wrote: > Replies inline Greg. > > Thanks & Regards, > Pradeep > Hi Pradeep, > -----Original Message----- > From: Greg Kurz [mailto:gr...@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.jagade...@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:gr...@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 <pradeepkiruv...@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.jagade...@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.jagade...@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.jagade...@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 > > >
