[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V12] fsdev: add IO throttle support to fsdev dev
|
From: |
Pradeep Kiruvale |
|
Subject: |
Re: [Qemu-devel] [PATCH V12] fsdev: add IO throttle support to fsdev devices |
|
Date: |
Mon, 23 Jan 2017 10:38:21 +0100 |
On 23 January 2017 at 10:32, Greg Kurz <address@hidden> wrote:
> On Thu, 12 Jan 2017 11:57:25 -0500
> Pradeep Jagadeesh <address@hidden> wrote:
>
> > Signed-off-by: Pradeep Jagadeesh <address@hidden>
> > ---
> > fsdev/Makefile.objs | 2 +-
> > fsdev/file-op-9p.h | 3 ++
> > fsdev/qemu-fsdev-opts.c | 77 ++++++++++++++++++++++++++++-
> > fsdev/qemu-fsdev-throttle.c | 118 ++++++++++++++++++++++++++++++
> ++++++++++++++
> > fsdev/qemu-fsdev-throttle.h | 39 +++++++++++++++
> > hw/9pfs/9p-local.c | 8 +++
> > hw/9pfs/9p.c | 4 ++
> > hw/9pfs/cofile.c | 2 +
> > 8 files changed, 251 insertions(+), 2 deletions(-)
> > create mode 100644 fsdev/qemu-fsdev-throttle.c
> > create mode 100644 fsdev/qemu-fsdev-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.
> >
>
> These lines ^^ are supposed to be the commit changelog: they should
> appear above your SoB.
>
You mean, I should put above "Signed of By" right?
-Pradeep
> > v1 -> v2:
> >
> > -Fixed FsContext redeclaration issue
> > -Removed couple of function declarations from 9p-throttle.h
> > -Fixed some of the .help messages
> >
> > v2 -> v3:
> >
> > -Addressed follwing comments by Claudio Fontana
> > -Removed redundant memset calls in fsdev_throttle_configure_iolimits
> function
> > -Checking throttle structure validity before initializing other
> structures
> > in fsdev_throttle_configure_iolimits
> >
> > -Addressed following comments by Greg Kurz
> > -Moved the code from 9pfs directory to fsdev directory, because the
> throttling
> > is for the fsdev devices.Renamed the files and functions to fsdev_
> from 9pfs_
> > -Renamed throttling cli options to throttling.*, as in QMP cli options
> > -Removed some of the unwanted .h files from qemu-fsdev-throttle.[ch]
> > -Using throttle_enabled() function to set the thottle enabled flag for
> fsdev.
> >
> > v3 -> v4:
> >
> > -Addressed following comments by Alberto Garcia
> > -Removed the unwanted locking and other data structures in
> qemu-fsdev-throttle.[ch]
> >
> > -Addressed following comments by Greg Kurz
> > -Removed fsdev_iolimitsenable/disable functions, instead using
> throttle_enabled function
> >
> > v4 -> V5:
> > -Fixed the issue with the larger block size accounting.
> > (i.e, when the 9pfs mounted using msize=xxx option)
> >
> > V5 -> V6:
> > -Addressed the comments by Alberto Garcia
> > -Removed the fsdev_throttle_timer_cb()
> > -Simplified the fsdev_throttle_schedule_next_request() as suggested
> >
> > V6 -> V7:
> > -Addressed the comments by Alberto Garcia
> > -changed the fsdev_throttle_schedule_next_request() as suggested
> >
> > v7 -> v8:
> > -Addressed comments by Alberto Garcia
> > -Fixed some indentation issues and split the configure_io_limit function
> > -Inlined throttle_timer_check code
> >
> > V8 -> v9:
> > -Addressed the comments by Greg Kurz
> > -Inlined the fsdev_throttle_schedule_next_request() code into
> > fsdev_co_throttle_request ()
> >
> > v9 -> v10:
> > -Addressed the comments by alberto garcia
> > -fixed the indentation issues and minor issues
> >
> > v10 -> v11:
> > -Addressed the comments by alberto garcia
> > -renamed err variable to errp issues
> >
> > v11 -> v12:
> > -Addressed the comments by Greg Kurz
> > -fixed the missing throttle_config_init() function call
> >
> > diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs
> > index 1b120a4..659df6e 100644
> > --- a/fsdev/Makefile.objs
> > +++ b/fsdev/Makefile.objs
> > @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
> > else
> > common-obj-y = qemu-fsdev-dummy.o
> > endif
> > -common-obj-y += qemu-fsdev-opts.o
> > +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o
> >
> > # Toplevel always builds this; targets without virtio will put it in
> > # common-obj-y
> > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> > index a56dc84..0844a40 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 "qemu-fsdev-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..385423f0 100644
> > --- a/fsdev/qemu-fsdev-opts.c
> > +++ b/fsdev/qemu-fsdev-opts.c
> > @@ -37,8 +37,83 @@ static QemuOptsList qemu_fsdev_opts = {
> > }, {
> > .name = "sock_fd",
> > .type = QEMU_OPT_NUMBER,
> > + }, {
> > + .name = "throttling.iops-total",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "limit total I/O operations per second",
> > + }, {
> > + .name = "throttling.iops-read",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "limit read operations per second",
> > + }, {
> > + .name = "throttling.iops-write",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "limit write operations per second",
> > + }, {
> > + .name = "throttling.bps-total",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "limit total bytes per second",
> > + }, {
> > + .name = "throttling.bps-read",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "limit read bytes per second",
> > + }, {
> > + .name = "throttling.bps-write",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "limit write bytes per second",
> > + }, {
> > + .name = "throttling.iops-total-max",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "I/O operations burst",
> > + }, {
> > + .name = "throttling.iops-read-max",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "I/O operations read burst",
> > + }, {
> > + .name = "throttling.iops-write-max",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "I/O operations write burst",
> > + }, {
> > + .name = "throttling.bps-total-max",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "total bytes burst",
> > + }, {
> > + .name = "throttling.bps-read-max",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "total bytes read burst",
> > + }, {
> > + .name = "throttling.bps-write-max",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "total bytes write burst",
> > + }, {
> > + .name = "throttling.iops-total-max-length",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "length of the iops-total-max burst period, in
> seconds",
> > + }, {
> > + .name = "throttling.iops-read-max-length",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "length of the iops-read-max burst period, in
> seconds",
> > + }, {
> > + .name = "throttling.iops-write-max-length",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "length of the iops-write-max burst period, in
> seconds",
> > + }, {
> > + .name = "throttling.bps-total-max-length",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "length of the bps-total-max burst period, in
> seconds",
> > + }, {
> > + .name = "throttling.bps-read-max-length",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "length of the bps-read-max burst period, in
> seconds",
> > + }, {
> > + .name = "throttling.bps-write-max-length",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "length of the bps-write-max burst period, in
> seconds",
> > + }, {
> > + .name = "throttling.iops-size",
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "when limiting by iops max size of an I/O in bytes",
> > },
> > -
> > { /*End of list */ }
> > },
> > };
> > diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
> > new file mode 100644
> > index 0000000..feb9af3
> > --- /dev/null
> > +++ b/fsdev/qemu-fsdev-throttle.c
> > @@ -0,0 +1,118 @@
> > +/*
> > + * Fsdev 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 "qemu/error-report.h"
> > +#include "qemu-fsdev-throttle.h"
> > +#include "qemu/iov.h"
> > +
> > +static void fsdev_throttle_read_timer_cb(void *opaque)
> > +{
> > + FsThrottle *fst = opaque;
> > + qemu_co_enter_next(&fst->throttled_reqs[false]);
> > +}
> > +
> > +static void fsdev_throttle_write_timer_cb(void *opaque)
> > +{
> > + FsThrottle *fst = opaque;
> > + qemu_co_enter_next(&fst->throttled_reqs[true]);
> > +}
> > +
> > +void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error
> **errp)
> > +{
> > + throttle_config_init(&fst->cfg);
> > + fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> > + qemu_opt_get_number(opts, "throttling.bps-total", 0);
> > + fst->cfg.buckets[THROTTLE_BPS_READ].avg =
> > + qemu_opt_get_number(opts, "throttling.bps-read", 0);
> > + fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> > + qemu_opt_get_number(opts, "throttling.bps-write", 0);
> > + fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> > + qemu_opt_get_number(opts, "throttling.iops-total", 0);
> > + fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> > + qemu_opt_get_number(opts, "throttling.iops-read", 0);
> > + fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> > + qemu_opt_get_number(opts, "throttling.iops-write", 0);
> > +
> > + fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> > + qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
> > + fst->cfg.buckets[THROTTLE_BPS_READ].max =
> > + qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
> > + fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> > + qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
> > + fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> > + qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
> > + fst->cfg.buckets[THROTTLE_OPS_READ].max =
> > + qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
> > + fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> > + qemu_opt_get_number(opts, "throttling.iops-write-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, "throttling.iops-size", 0);
> > +
> > + throttle_is_valid(&fst->cfg, errp);
> > +}
> > +
> > +void fsdev_throttle_init(FsThrottle *fst)
> > +{
> > + if (throttle_enabled(&fst->cfg)) {
> > + throttle_init(&fst->ts);
> > + throttle_timers_init(&fst->tt,
> > + qemu_get_aio_context(),
> > + QEMU_CLOCK_REALTIME,
> > + fsdev_throttle_read_timer_cb,
> > + fsdev_throttle_write_timer_cb,
> > + fst);
> > + throttle_config(&fst->ts, &fst->tt, &fst->cfg);
> > + qemu_co_queue_init(&fst->throttled_reqs[0]);
> > + qemu_co_queue_init(&fst->throttled_reqs[1]);
> > + }
> > +}
> > +
> > +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool
> is_write,
> > + struct iovec *iov, int
> iovcnt)
> > +{
> > + if (throttle_enabled(&fst->cfg)) {
> > + if (throttle_schedule_timer(&fst->ts, &fst->tt, is_write) ||
> > + !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {
> > + qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> > + }
> > +
> > + throttle_account(&fst->ts, is_write, iov_size(iov, iovcnt));
> > +
> > + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write]) &&
> > + !throttle_schedule_timer(&fst->ts, &fst->tt, is_write)) {
> > + qemu_co_queue_next(&fst->throttled_reqs[is_write]);
> > + }
> > + }
> > +}
> > +
> > +void fsdev_throttle_cleanup(FsThrottle *fst)
> > +{
> > + if (throttle_enabled(&fst->cfg)) {
> > + throttle_timers_destroy(&fst->tt);
> > + }
> > +}
> > diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h
> > new file mode 100644
> > index 0000000..e418643
> > --- /dev/null
> > +++ b/fsdev/qemu-fsdev-throttle.h
> > @@ -0,0 +1,39 @@
> > +/*
> > + * Fsdev 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 _FSDEV_THROTTLE_H
> > +#define _FSDEV_THROTTLE_H
> > +
> > +#include "block/aio.h"
> > +#include "qemu/main-loop.h"
> > +#include "qemu/coroutine.h"
> > +#include "qapi/error.h"
> > +#include "qemu/throttle.h"
> > +
> > +typedef struct FsThrottle {
> > + ThrottleState ts;
> > + ThrottleTimers tt;
> > + ThrottleConfig cfg;
> > + CoQueue throttled_reqs[2];
> > +} FsThrottle;
> > +
> > +void fsdev_throttle_parse_opts(QemuOpts *, FsThrottle *, Error **);
> > +
> > +void fsdev_throttle_init(FsThrottle *);
> > +
> > +void coroutine_fn fsdev_co_throttle_request(FsThrottle *, bool ,
> > + struct iovec *, int);
> > +
> > +void fsdev_throttle_cleanup(FsThrottle *);
> > +#endif /* _FSDEV_THROTTLE_H */
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index 845675e..828348d 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -1209,6 +1209,7 @@ 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");
> > + Error *err = NULL;
> >
> > if (!sec_model) {
> > error_report("Security model not specified, local fs needs
> security model");
> > @@ -1237,6 +1238,13 @@ static int local_parse_opts(QemuOpts *opts,
> struct FsDriverEntry *fse)
> > error_report("fsdev: No path specified");
> > return -1;
> > }
> > +
> > + fsdev_throttle_parse_opts(opts, &fse->fst, &err);
> > + if (err) {
> > + error_reportf_err(err, "Throttle configuration is not valid: ");
> > + return -1;
> > + }
> > +
> > fse->path = g_strdup(path);
> >
> > return 0;
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index fa58877..bdbc345 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -3520,6 +3520,10 @@ int v9fs_device_realize_common(V9fsState *s,
> Error **errp)
> > error_setg(errp, "share path %s is not a directory", fse->path);
> > goto out;
> > }
> > +
> > + s->ctx.fst = &fse->fst;
> > + fsdev_throttle_init(s->ctx.fst);
> > +
> > v9fs_path_free(&path);
> >
> > rc = 0;
> > diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
> > index 120e267..88791bc 100644
> > --- a/hw/9pfs/cofile.c
> > +++ b/hw/9pfs/cofile.c
> > @@ -247,6 +247,7 @@ int coroutine_fn v9fs_co_pwritev(V9fsPDU *pdu,
> V9fsFidState *fidp,
> > if (v9fs_request_cancelled(pdu)) {
> > return -EINTR;
> > }
> > + fsdev_co_throttle_request(s->ctx.fst, true, iov, iovcnt);
> > v9fs_co_run_in_worker(
> > {
> > err = s->ops->pwritev(&s->ctx, &fidp->fs, iov, iovcnt,
> offset);
> > @@ -266,6 +267,7 @@ int coroutine_fn v9fs_co_preadv(V9fsPDU *pdu,
> V9fsFidState *fidp,
> > if (v9fs_request_cancelled(pdu)) {
> > return -EINTR;
> > }
> > + fsdev_co_throttle_request(s->ctx.fst, false, iov, iovcnt);
> > v9fs_co_run_in_worker(
> > {
> > err = s->ops->preadv(&s->ctx, &fidp->fs, iov, iovcnt,
> offset);
>
>