[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: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver |
Date: |
Fri, 9 Sep 2016 17:29:16 +0200 |
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 +++++++++++
> 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.
> { /*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 <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).
> +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 <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.
> +#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
- [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver, Pradeep Jagadeesh, 2016/09/09
- 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 <=
- 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, 2016/09/13