From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40496) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biNkE-0004Q6-LF for qemu-devel@nongnu.org; Fri, 09 Sep 2016 11:29:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1biNk9-0008O0-J5 for qemu-devel@nongnu.org; Fri, 09 Sep 2016 11:29:33 -0400 Received: from 8.mo68.mail-out.ovh.net ([46.105.74.219]:33489) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biNk9-0008Nk-7M for qemu-devel@nongnu.org; Fri, 09 Sep 2016 11:29:29 -0400 Received: from player778.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo68.mail-out.ovh.net (Postfix) with ESMTP id 69FD3FFA00D for ; Fri, 9 Sep 2016 17:29:28 +0200 (CEST) Date: Fri, 9 Sep 2016 17:29:16 +0200 From: Greg Kurz Message-ID: <20160909172916.3cefb8f2@bahia> In-Reply-To: <1473412227-23381-1-git-send-email-pradeep.jagadeesh@huawei.com> References: <1473412227-23381-1-git-send-email-pradeep.jagadeesh@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pradeep Jagadeesh Cc: "Aneesh Kumar K.V" , Pradeep Jagadeesh , Alberto Garcia , qemu-devel@nongnu.org, Claudio Fontana , Eric Blake On Fri, 9 Sep 2016 05:10:27 -0400 Pradeep Jagadeesh wrote: > Uses throttling APIs to limit I/O bandwidth and number of operations on the > devices which use 9p-local driver. > > Signed-off-by: Pradeep Jagadeesh > --- Hi Pradeep, Please find some remarks below. I haven't dived deep enough to see if this actually works. Maybe Berto can provide some feedback ? Cheers. -- Greg > fsdev/file-op-9p.h | 3 + > fsdev/qemu-fsdev-opts.c | 52 +++++++++++++ > hw/9pfs/9p-local.c | 18 ++++- > hw/9pfs/9p-throttle.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++ > hw/9pfs/9p-throttle.h | 46 +++++++++++ > hw/9pfs/9p.c | 7 ++ > hw/9pfs/Makefile.objs | 1 + > 7 files changed, 326 insertions(+), 2 deletions(-) > create mode 100644 hw/9pfs/9p-throttle.c > create mode 100644 hw/9pfs/9p-throttle.h > > This adds the support for the 9p-local driver. > For now this functionality can be enabled only through qemu cli options. > QMP interface and support to other drivers need further extensions. > To make it simple for other drivers, the throttle code has been put in > separate files. > > v1 -> v2: > > -Fixed FsContext redeclaration issue > -Removed couple of function declarations from 9p-throttle.h > -Fixed some of the .help messages > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h > index 6db9fea..e86b91a 100644 > --- a/fsdev/file-op-9p.h > +++ b/fsdev/file-op-9p.h > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include "hw/9pfs/9p-throttle.h" > > #define SM_LOCAL_MODE_BITS 0600 > #define SM_LOCAL_DIR_MODE_BITS 0700 > @@ -74,6 +75,7 @@ typedef struct FsDriverEntry { > char *path; > int export_flags; > FileOperations *ops; > + FsThrottle fst; > } FsDriverEntry; > > typedef struct FsContext > @@ -83,6 +85,7 @@ typedef struct FsContext > int export_flags; > struct xattr_operations **xops; > struct extended_ops exops; > + FsThrottle *fst; > /* fs driver specific data */ > void *private; > } FsContext; > diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c > index 1dd8c7a..2774855 100644 > --- a/fsdev/qemu-fsdev-opts.c > +++ b/fsdev/qemu-fsdev-opts.c > @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = { > }, { > .name = "sock_fd", > .type = QEMU_OPT_NUMBER, > + }, { > + .name = "", > + .type = QEMU_OPT_NUMBER, > + .help = "limit total bytes per second", > + }, { > + .name = "bps_rd", > + .type = QEMU_OPT_NUMBER, > + .help = "limit read bytes per second", > + }, { > + .name = "bps_wr", > + .type = QEMU_OPT_NUMBER, > + .help = "limit write bytes per second", > + }, { > + .name = "iops", > + .type = QEMU_OPT_NUMBER, > + .help = "limit total io operations per second", > + }, { > + .name = "iops_rd", > + .type = QEMU_OPT_NUMBER, > + .help = "limit read operations per second ", > + }, { > + .name = "iops_wr", > + .type = QEMU_OPT_NUMBER, > + .help = "limit write operations per second", > + }, { > + .name = "bps_max", > + .type = QEMU_OPT_NUMBER, > + .help = "maximum bytes burst", > + }, { > + .name = "bps_rd_max", > + .type = QEMU_OPT_NUMBER, > + .help = "maximum bytes read burst", > + }, { > + .name = "bps_wr_max", > + .type = QEMU_OPT_NUMBER, > + .help = "maximum bytes write burst", > + }, { > + .name = "iops_max", > + .type = QEMU_OPT_NUMBER, > + .help = "maximum io operations burst", > + }, { > + .name = "iops_rd_max", > + .type = QEMU_OPT_NUMBER, > + .help = "maximum io operations read burst", > + }, { > + .name = "iops_wr_max", > + .type = QEMU_OPT_NUMBER, > + .help = "maximum io operations write burst", > + }, { > + .name = "iops_size", > + .type = QEMU_OPT_NUMBER, > + .help = "io iops-size", > }, > In case we end up sharing code with blockdev as suggested by Eric, maybe you can use the same QMP friendly naming scheme ("throttling.*") and the same help strings as well. > { /*End of list */ } > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index 3f271fc..49c2819 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs, > const struct iovec *iov, > int iovcnt, off_t offset) > { > + throttle9p_request(ctx->fst, false, iov->iov_len); > + > #ifdef CONFIG_PREADV > return preadv(fs->fd, iov, iovcnt, offset); > #else > @@ -436,8 +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, > const struct iovec *iov, > int iovcnt, off_t offset) > { > - ssize_t ret > -; > + ssize_t ret; > + > + throttle9p_request(ctx->fst, true, iov->iov_len); > + > #ifdef CONFIG_PREADV > ret = pwritev(fs->fd, iov, iovcnt, offset); > #else > @@ -1213,6 +1217,9 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) > const char *sec_model = qemu_opt_get(opts, "security_model"); > const char *path = qemu_opt_get(opts, "path"); > > + /* get the throttle structure */ Not sure this comment is helpful. > + FsThrottle *fst = &fse->fst; > + > if (!sec_model) { > error_report("Security model not specified, local fs needs security model"); > error_printf("valid options are:" > @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) > error_report("fsdev: No path specified"); > return -1; > } > + > + throttle9p_enable_io_limits(opts, fst); > + > + if (throttle9p_get_io_limits_state(fst)) { > + throttle9p_configure_iolimits(opts, fst); > + } > + > fse->path = g_strdup(path); > > return 0; > diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c > new file mode 100644 > index 0000000..f2a7ba5 > --- /dev/null > +++ b/hw/9pfs/9p-throttle.c > @@ -0,0 +1,201 @@ > +/* > + * 9P Throttle > + * > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH > + * > + * Author: Pradeep Jagadeesh > + * > + * 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 > +#include > +#include > +#include > +#include "fsdev/file-op-9p.h" > +#include "9p-throttle.h" > + Only "qemu/osdep.h", "qemu/error-report.h" and "9p-throttle.h" are actually needed. > +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst) > +{ > + const float bps = qemu_opt_get_number(opts, "bps", 0); > + const float iops = qemu_opt_get_number(opts, "iops", 0); > + const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0); > + const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0); > + const float rdops = qemu_opt_get_number(opts, "iops_rd", 0); > + const float wrops = qemu_opt_get_number(opts, "iops_wr", 0); > + > + if (bps > 0 || iops > 0 || rdbps > 0 || > + wrbps > 0 || rdops > 0 || wrops > 0) { > + fst->io_limits_enabled = true; > + } else { > + fst->io_limits_enabled = false; > + } > +} > + This function should be named *_parse_* but I'm not even sure it is actually needed (see below). > +static bool throttle9p_check_for_wait(FsThrottle *fst, bool is_write) > +{ > + if (fst->any_timer_armed[is_write]) { > + return true; > + } else { > + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); > + } > +} > + > +static void throttle9p_schedule_next_request(FsThrottle *fst, bool is_write) > +{ > + bool must_wait = throttle9p_check_for_wait(fst, is_write); > + if (!fst->pending_reqs[is_write]) { > + return; > + } > + if (!must_wait) { > + if (qemu_in_coroutine() && > + qemu_co_queue_next(&fst->throttled_reqs[is_write])) { > + ; > + } else { > + int64_t now = qemu_clock_get_ns(fst->tt.clock_type); > + timer_mod(fst->tt.timers[is_write], now + 1); > + fst->any_timer_armed[is_write] = true; > + } > + } > +} > + > +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write) > +{ > + bool empty_queue; > + qemu_mutex_lock(&fst->lock); > + fst->any_timer_armed[is_write] = false; > + qemu_mutex_unlock(&fst->lock); > + empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]); > + if (empty_queue) { > + qemu_mutex_lock(&fst->lock); > + throttle9p_schedule_next_request(fst, is_write); > + qemu_mutex_unlock(&fst->lock); > + } > +} > + > + > +bool throttle9p_get_io_limits_state(FsThrottle *fst) The name looks a bit strange, since this helper simply returns a boolean flag. I guess throttle9p_enabled() is enough. > +{ > + > + return fst->io_limits_enabled; > +} > + > +static void throttle9p_read_timer_cb(void *opaque) > +{ > + throttle9p_timer_cb(opaque, false); > +} > + > +static void throttle9p_write_timer_cb(void *opaque) > +{ > + throttle9p_timer_cb(opaque, true); > +} > + > +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle *fst) > +{ This function can fail, it should have a return value (0 or -1). > + memset(&fst->ts, 1, sizeof(fst->ts)); > + memset(&fst->tt, 1, sizeof(fst->tt)); > + memset(&fst->cfg, 0, sizeof(fst->cfg)); Same remark as Claudio. > + fst->aioctx = qemu_get_aio_context(); > + > + if (!fst->aioctx) { > + error_report("Failed to create AIO Context"); > + exit(1); > + } > + throttle_init(&fst->ts); > + throttle_timers_init(&fst->tt, > + fst->aioctx, > + QEMU_CLOCK_REALTIME, > + throttle9p_read_timer_cb, > + throttle9p_write_timer_cb, > + fst); Shouldn't all this be done later when we know the config is valid ? > + throttle_config_init(&fst->cfg); > + g_assert(throttle_is_valid(&fst->cfg, NULL)); > + What's the point in calling throttle_is_valid() on a freshly initialized config ? > + qemu_co_queue_init(&fst->throttled_reqs[0]); > + qemu_co_queue_init(&fst->throttled_reqs[1]); Later, when the config is valid ? > + fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg = > + qemu_opt_get_number(opts, "bps", 0); > + fst->cfg.buckets[THROTTLE_BPS_READ].avg = > + qemu_opt_get_number(opts, "bps_rd", 0); > + fst->cfg.buckets[THROTTLE_BPS_WRITE].avg = > + qemu_opt_get_number(opts, "bps_wr", 0); > + fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg = > + qemu_opt_get_number(opts, "iops", 0); > + fst->cfg.buckets[THROTTLE_OPS_READ].avg = > + qemu_opt_get_number(opts, "iops_rd", 0); > + fst->cfg.buckets[THROTTLE_OPS_WRITE].avg = > + qemu_opt_get_number(opts, "iops_wr", 0); > + > + fst->cfg.buckets[THROTTLE_BPS_TOTAL].max = > + qemu_opt_get_number(opts, "bps_max", 0); > + fst->cfg.buckets[THROTTLE_BPS_READ].max = > + qemu_opt_get_number(opts, "bps_rd_max", 0); > + fst->cfg.buckets[THROTTLE_BPS_WRITE].max = > + qemu_opt_get_number(opts, "bps_wr_max", 0); > + fst->cfg.buckets[THROTTLE_OPS_TOTAL].max = > + qemu_opt_get_number(opts, "iops_max", 0); > + fst->cfg.buckets[THROTTLE_OPS_READ].max = > + qemu_opt_get_number(opts, "iops_rd_max", 0); > + fst->cfg.buckets[THROTTLE_OPS_WRITE].max = > + qemu_opt_get_number(opts, "iops_wr_max", 0); > + > + fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = > + qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1); > + fst->cfg.buckets[THROTTLE_BPS_READ].burst_length = > + qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1); > + fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length = > + qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1); > + fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = > + qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1); > + fst->cfg.buckets[THROTTLE_OPS_READ].burst_length = > + qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1); > + fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length = > + qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1); > + fst->cfg.op_size = > + qemu_opt_get_number(opts, "iops_size", 0); > + > + throttle_config(&fst->ts, &fst->tt, &fst->cfg); Let's set the config later, when we we it is valid. > + if (!throttle_is_valid(&fst->cfg, NULL)) { You should pass an Error * to throttle_is_valid() to be able to report the misconfiguration to the user. I guess it is okay to print it here using error_repport_err() (see include/qapi/error.h) and return -1. > + return; > + } > + > + g_assert(fst->tt.timers[0]); > + g_assert(fst->tt.timers[1]); These are really not needed since, timers are set with: QEMUTimer *ts = g_malloc0(sizeof(QEMUTimer)); and g_malloc0() never returns NULL when passed a non-nul size. It calls g_assert() internally instead. > + fst->pending_reqs[0] = 0; > + fst->pending_reqs[1] = 0; > + fst->any_timer_armed[0] = false; > + fst->any_timter_armed[1] = false; > + qemu_mutex_init(&fst->lock); And there you may set the enabled flag. > +} > + > +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t bytes) > +{ > + if (fst->io_limits_enabled) { throttle9p_enabled(fst) > + qemu_mutex_lock(&fst->lock); > + bool must_wait = throttle9p_check_for_wait(fst, is_write); > + if (must_wait || fst->pending_reqs[is_write]) { > + fst->pending_reqs[is_write]++; > + qemu_mutex_unlock(&fst->lock); > + qemu_co_queue_wait(&fst->throttled_reqs[is_write]); > + qemu_mutex_lock(&fst->lock); > + fst->pending_reqs[is_write]--; > + } > + throttle_account(&fst->ts, is_write, bytes); > + throttle9p_schedule_next_request(fst, is_write); > + qemu_mutex_unlock(&fst->lock); > + } > +} > + > +void throttle9p_cleanup(FsThrottle *fst) > +{ > + throttle_timers_destroy(&fst->tt); > + qemu_mutex_destroy(&fst->lock); > +} > diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h > new file mode 100644 > index 0000000..0f7551d > --- /dev/null > +++ b/hw/9pfs/9p-throttle.h > @@ -0,0 +1,46 @@ > +/* > + * 9P Throttle > + * > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH > + * > + * Author: Pradeep Jagadeesh > + * > + * 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 > +#include These includes are forbidden. They are already handled by "qemu/osdep.h" which is supposed to be included by all .c files. > +#include "block/aio.h" > +#include "qemu/main-loop.h" > +#include "qemu/coroutine.h" > +#include "qemu/throttle.h" > + > +typedef struct FsThrottle { > + ThrottleState ts; > + ThrottleTimers tt; > + AioContext *aioctx; > + ThrottleConfig cfg; > + bool io_limits_enabled; Let's simply call this enabled. > + CoQueue throttled_reqs[2]; > + unsigned pending_reqs[2]; > + bool any_timer_armed[2]; > + QemuMutex lock; > +} FsThrottle; > + > +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *); > + > +bool throttle9p_get_io_limits_state(FsThrottle *); > + > +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *); > + > +void throttle9p_request(FsThrottle *, bool , ssize_t); > + > +void throttle9p_cleanup(FsThrottle *); > +#endif /* _9P_THROTTLE_H */ > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index dfe293d..7be926a 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp) > error_setg(errp, "share path %s is not a directory", fse->path); > goto out; > } > + > + /* Throttle structure initialization */ Not sure this comment is helpful. > + s->ctx.fst = &fse->fst; > + > v9fs_path_free(&path); > > rc = 0; > @@ -3504,6 +3508,9 @@ out: > > void v9fs_device_unrealize_common(V9fsState *s, Error **errp) > { > + if (throttle9p_get_io_limits_state(s->ctx.fst)) { > + throttle9p_cleanup(s->ctx.fst); > + } > g_free(s->ctx.fs_root); > g_free(s->tag); > } > diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs > index da0ae0c..07523f1 100644 > --- a/hw/9pfs/Makefile.objs > +++ b/hw/9pfs/Makefile.objs > @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o > common-obj-y += coxattr.o 9p-synth.o > common-obj-$(CONFIG_OPEN_BY_HANDLE) += 9p-handle.o > common-obj-y += 9p-proxy.o > +common-obj-y += 9p-throttle.o > > obj-y += virtio-9p-device.o