[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local
|
From: |
Pradeep Jagadeesh |
|
Subject: |
Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver |
|
Date: |
Tue, 13 Sep 2016 12:36:04 +0000 |
-----Original Message-----
From: Greg Kurz [mailto:address@hidden
Sent: Tuesday, September 13, 2016 2:30 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 Tue, 13 Sep 2016 09:17:49 +0000
Pradeep Jagadeesh <address@hidden> wrote:
> 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.
>
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 <address@hidden>
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 <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
> > >
> >
>
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, (continued)
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Claudio Fontana, 2016/09/09
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Greg Kurz, 2016/09/09
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Greg Kurz, 2016/09/09
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Alberto Garcia, 2016/09/09
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Pradeep Jagadeesh, 2016/09/12
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Greg Kurz, 2016/09/12
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Pradeep Jagadeesh, 2016/09/12
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Greg Kurz, 2016/09/13
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Pradeep Jagadeesh, 2016/09/13
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Greg Kurz, 2016/09/13
- Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver,
Pradeep Jagadeesh <=