| Message ID | 1477148842-44794-1-git-send-email-pradeep.jagadeesh@huawei.com |
|---|---|
| State | New |
| Headers | show |
On Sat 22 Oct 2016 05:07:22 PM CEST, Pradeep Jagadeesh wrote: > 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. Ok, I think this is almost ready. There's only a few corrections and style fixes left. > diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs > index 1b120a4..2c6da2d 100644 > --- a/fsdev/Makefile.objs > +++ b/fsdev/Makefile.objs > @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o > endif > common-obj-y += qemu-fsdev-opts.o > > +common-obj-y += qemu-fsdev-throttle.o (Optional) You can put these two in the same line: common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o > +++ b/fsdev/qemu-fsdev-throttle.c In this file there's many lines that are incorrectly indented. Indentation should be four spaces. The following functions have lines that need to be corrected: fsdev_throttle_check_for_wait fsdev_throttle_schedule_next_request fsdev_parse_io_limit_opts fsdev_throttle_configure_iolimits fsdev_co_throttle_request Check also the other changes in your patch in case there are more lines with wrong indentation. > +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write) "Check for wait" doesn't sound like correct English to me. How about simply fsdev_throttle_schedule_timer()? Or fsdev_throttle_must_wait() > +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) > +{ > + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { Here's an example of a line with the wrong indentation. It uses three spaces, it should use four. > + qemu_co_queue_next(&fst->throttled_reqs[is_write]); And this one uses seven, it should use eight. There are more cases like this, I only highlighted a couple of them. > +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) > +{ > + Error *err = NULL; > + > + /* Parse and set populate the cfg structure */ > + fsdev_parse_io_limit_opts(opts, fst); > + > + if (!throttle_is_valid(&fst->cfg, &err)) { > + error_reportf_err(err, "Throttle configuration is not valid: "); > + return -1; > + } > + if (throttle_enabled(&fst->cfg)) { > + g_assert((fst->aioctx = qemu_get_aio_context())); > + throttle_init(&fst->ts); > + throttle_timers_init(&fst->tt, > + fst->aioctx, > + 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]); > + } > + return 0; > +} Here you're parsing the command-line options and initializing the throtting structures in local_parse_opts(). Then you're cleaning up in v9fs_device_unrealize_common(). I have two questions: a) You could split fsdev_throttle_configure_iolimits() in two: 1) fsdev_throttle_parse_opts(), called from local_parse_opts() 2) fsdev_throttle_init(), called from v9fs_device_realize_common(). b) Why are you only doing this for the "local" fsdriver, and not for "handle" or "proxy"? Isn't it possible to do throttling with those as well? > +static int get_num_bytes(struct iovec *iov, int iovcnt) > +{ > + int i; > + unsigned int bytes = 0; You changed 'bytes' from int to unsigned as I suggested, but you still return an int, and in fsdev_co_throttle_request() you also get an int. Please use unsigned in all these cases. > +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); > + > +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, > + struct iovec *iov, int iovcnt); Please align this line with the parameters of the first line. Also, why do you include the name of the parameters in this function prototype but not in the other two? > @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts, > struct FsDriverEntry *fse) > error_report("fsdev: No path specified"); > return -1; > } > + > + if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) { > + return -1; > + } If you create fsdev_throttle_parse_opts() as I suggest, I'd rather print the error message here as the other error messages in this function. I think I haven't forgotten anything, everything else looks fine. I'd still like to have all the throtting parameters merged so we don't need to duplicate them, but that can be done in the future (I can even take care of it if I find a bit of time). Thanks for the patch! Berto
On Sat, 22 Oct 2016 11:07:22 -0400 Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote: > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> > --- Hi Pradeep, I see that Berto already did a thorough review for this patch and I agree for all the suggestions he made. I have some more to add. First: this patch doesn't apply cleanly, please rebase. More remarks below. > fsdev/Makefile.objs | 1 + > fsdev/file-op-9p.h | 3 + > fsdev/qemu-fsdev-opts.c | 76 +++++++++++++++++++++++ > fsdev/qemu-fsdev-throttle.c | 147 ++++++++++++++++++++++++++++++++++++++++++++ > fsdev/qemu-fsdev-throttle.h | 37 +++++++++++ > hw/9pfs/9p-local.c | 9 ++- > hw/9pfs/9p.c | 6 ++ > hw/9pfs/cofile.c | 5 ++ > 8 files changed, 282 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. > The above lines are the changelog for the patch. We want this to be displayed when running 'git log'. For this to happen, please move these lines above your SoB tag. Only the vN -> vN+1 changes are not relevant (we don't need to record all the intermediate reviews in git) and should stay here. > 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 > > > diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs > index 1b120a4..2c6da2d 100644 > --- a/fsdev/Makefile.objs > +++ b/fsdev/Makefile.objs > @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o > endif > common-obj-y += qemu-fsdev-opts.o > > +common-obj-y += qemu-fsdev-throttle.o > # Toplevel always builds this; targets without virtio will put it in > # common-obj-y > common-obj-$(CONFIG_ALL) += qemu-fsdev-dummy.o > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h > index 6db9fea..33fe822 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..395d497 100644 > --- a/fsdev/qemu-fsdev-opts.c > +++ b/fsdev/qemu-fsdev-opts.c > @@ -37,6 +37,82 @@ 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..d48d031 > --- /dev/null > +++ b/fsdev/qemu-fsdev-throttle.c > @@ -0,0 +1,147 @@ > +/* > + * Fsdev Throttle > + * > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH > + * > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 "qemu/error-report.h" > +#include "qapi/error.h" > +#include "qemu-fsdev-throttle.h" > + > +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write) > +{ > + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); > +} > + > +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) > +{ > + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { > + if (fsdev_throttle_check_for_wait(fst, is_write)) { > + return; > + } > + qemu_co_queue_next(&fst->throttled_reqs[is_write]); This calls for a co > + } > +} > + As Berto already suggested in v6, these functions aren't needed and your "I just would like to keep these functions" answer isn't convincing enough. Personally, I'd greatly really prefer the code to be inlined in the callers, so that we can see the whole throttling/queue logic in one place. > +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]); > +} > + > +static void fsdev_parse_io_limit_opts(QemuOpts *opts, FsThrottle *fst) > +{ > + 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); > +} > + > +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) > +{ > + Error *err = NULL; > + > + /* Parse and set populate the cfg structure */ > + fsdev_parse_io_limit_opts(opts, fst); > + > + if (!throttle_is_valid(&fst->cfg, &err)) { > + error_reportf_err(err, "Throttle configuration is not valid: "); > + return -1; > + } > + if (throttle_enabled(&fst->cfg)) { > + g_assert((fst->aioctx = qemu_get_aio_context())); > + throttle_init(&fst->ts); > + throttle_timers_init(&fst->tt, > + fst->aioctx, > + 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]); > + } > + return 0; > +} > + > +static int get_num_bytes(struct iovec *iov, int iovcnt) > +{ > + int i; > + unsigned int bytes = 0; > + > + for (i = 0; i < iovcnt; i++) { > + bytes += iov[i].iov_len; > + } > + return bytes; > +} > + > +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, > + struct iovec *iov, int iovcnt) > +{ > + if (throttle_enabled(&fst->cfg)) { > + int bytes = get_num_bytes(iov, iovcnt); > + bool must_wait = fsdev_throttle_check_for_wait(fst, is_write); > + if (must_wait || !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { > + qemu_co_queue_wait(&fst->throttled_reqs[is_write]); > + } > + throttle_account(&fst->ts, is_write, bytes); > + > + fsdev_throttle_schedule_next_request(fst, is_write); > + } > +} > + > +void fsdev_throttle_cleanup(FsThrottle *fst) > +{ > + 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..df4e608 > --- /dev/null > +++ b/fsdev/qemu-fsdev-throttle.h > @@ -0,0 +1,37 @@ > +/* > + * Fsdev Throttle > + * > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH > + * > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 _FSDEV_THROTTLE_H > +#define _FSDEV_THROTTLE_H > + > +#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; > + CoQueue throttled_reqs[2]; > +} FsThrottle; > + > +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); > + > +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, > + struct iovec *iov, int iovcnt); > + > +void fsdev_throttle_cleanup(FsThrottle *); > +#endif /* _FSDEV_THROTTLE_H */ > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index 3f271fc..54459f2 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -436,8 +436,8 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, > const struct iovec *iov, > int iovcnt, off_t offset) > { > - ssize_t ret > -; > + ssize_t ret; > + Please push this to a separate patch. > #ifdef CONFIG_PREADV > ret = pwritev(fs->fd, iov, iovcnt, offset); > #else > @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) > error_report("fsdev: No path specified"); > return -1; > } > + > + if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) { > + return -1; > + } > + > fse->path = g_strdup(path); > > return 0; > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index dfe293d..ccf6d0c 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -3490,6 +3490,9 @@ 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; > + > v9fs_path_free(&path); > > rc = 0; > @@ -3504,6 +3507,9 @@ out: > > void v9fs_device_unrealize_common(V9fsState *s, Error **errp) > { > + if (throttle_enabled(&s->ctx.fst->cfg)) { > + fsdev_throttle_cleanup(s->ctx.fst); > + } > g_free(s->ctx.fs_root); > g_free(s->tag); > } > diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c > index 10343c0..039031d 100644 > --- a/hw/9pfs/cofile.c > +++ b/hw/9pfs/cofile.c > @@ -16,6 +16,7 @@ > #include "qemu/thread.h" > #include "qemu/coroutine.h" > #include "coth.h" > +#include "fsdev/qemu-fsdev-throttle.h" > > int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode, > V9fsStatDotl *v9stat) > @@ -245,6 +246,8 @@ int 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); > @@ -264,6 +267,8 @@ int 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);
Re-post (I had hit the send button by error :) On Sat, 22 Oct 2016 11:07:22 -0400 Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote: > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> > --- Hi Pradeep, I see that Berto already did a thorough review for this patch and I agree for all the suggestions he made. I have some more to add. First: this patch doesn't apply, please rebase. More remarks below. > fsdev/Makefile.objs | 1 + > fsdev/file-op-9p.h | 3 + > fsdev/qemu-fsdev-opts.c | 76 +++++++++++++++++++++++ > fsdev/qemu-fsdev-throttle.c | 147 ++++++++++++++++++++++++++++++++++++++++++++ > fsdev/qemu-fsdev-throttle.h | 37 +++++++++++ > hw/9pfs/9p-local.c | 9 ++- > hw/9pfs/9p.c | 6 ++ > hw/9pfs/cofile.c | 5 ++ > 8 files changed, 282 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. > The above lines are the changelog for the patch. We want this to be displayed when running 'git log'. For this to happen, please move these lines above your SoB tag. Only the vN -> vN+1 changes are not relevant (we don't need to record all the intermediate reviews in git) and should stay here. > 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 > > > diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs > index 1b120a4..2c6da2d 100644 > --- a/fsdev/Makefile.objs > +++ b/fsdev/Makefile.objs > @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o > endif > common-obj-y += qemu-fsdev-opts.o > > +common-obj-y += qemu-fsdev-throttle.o > # Toplevel always builds this; targets without virtio will put it in > # common-obj-y > common-obj-$(CONFIG_ALL) += qemu-fsdev-dummy.o > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h > index 6db9fea..33fe822 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..395d497 100644 > --- a/fsdev/qemu-fsdev-opts.c > +++ b/fsdev/qemu-fsdev-opts.c > @@ -37,6 +37,82 @@ 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..d48d031 > --- /dev/null > +++ b/fsdev/qemu-fsdev-throttle.c > @@ -0,0 +1,147 @@ > +/* > + * Fsdev Throttle > + * > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH > + * > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 "qemu/error-report.h" > +#include "qapi/error.h" > +#include "qemu-fsdev-throttle.h" > + > +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write) > +{ > + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); > +} > + > +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) > +{ > + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { > + if (fsdev_throttle_check_for_wait(fst, is_write)) { > + return; > + } > + qemu_co_queue_next(&fst->throttled_reqs[is_write]); This calls for a coroutine_fn tag but... > + } > +} > + ... as Berto already suggested in v6, these functions aren't needed and your "I just would like to keep these functions" answer isn't convincing enough. Personally, I'd greatly really prefer the code to be inlined in the callers, so that we can see the whole throttling/queue logic in one place. > +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]); > +} > + > +static void fsdev_parse_io_limit_opts(QemuOpts *opts, FsThrottle *fst) > +{ > + 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); > +} > + > +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) > +{ > + Error *err = NULL; > + > + /* Parse and set populate the cfg structure */ > + fsdev_parse_io_limit_opts(opts, fst); > + > + if (!throttle_is_valid(&fst->cfg, &err)) { > + error_reportf_err(err, "Throttle configuration is not valid: "); > + return -1; > + } > + if (throttle_enabled(&fst->cfg)) { > + g_assert((fst->aioctx = qemu_get_aio_context())); > + throttle_init(&fst->ts); > + throttle_timers_init(&fst->tt, > + fst->aioctx, > + 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]); > + } > + return 0; > +} > + > +static int get_num_bytes(struct iovec *iov, int iovcnt) > +{ > + int i; > + unsigned int bytes = 0; > + > + for (i = 0; i < iovcnt; i++) { > + bytes += iov[i].iov_len; > + } > + return bytes; > +} > + > +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, > + struct iovec *iov, int iovcnt) > +{ > + if (throttle_enabled(&fst->cfg)) { > + int bytes = get_num_bytes(iov, iovcnt); > + bool must_wait = fsdev_throttle_check_for_wait(fst, is_write); > + if (must_wait || !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { > + qemu_co_queue_wait(&fst->throttled_reqs[is_write]); > + } > + throttle_account(&fst->ts, is_write, bytes); > + > + fsdev_throttle_schedule_next_request(fst, is_write); > + } > +} > + > +void fsdev_throttle_cleanup(FsThrottle *fst) > +{ > + 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..df4e608 > --- /dev/null > +++ b/fsdev/qemu-fsdev-throttle.h > @@ -0,0 +1,37 @@ > +/* > + * Fsdev Throttle > + * > + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH > + * > + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 _FSDEV_THROTTLE_H > +#define _FSDEV_THROTTLE_H > + > +#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; > + CoQueue throttled_reqs[2]; > +} FsThrottle; > + > +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); > + > +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, > + struct iovec *iov, int iovcnt); > + > +void fsdev_throttle_cleanup(FsThrottle *); > +#endif /* _FSDEV_THROTTLE_H */ > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index 3f271fc..54459f2 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -436,8 +436,8 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, > const struct iovec *iov, > int iovcnt, off_t offset) > { > - ssize_t ret > -; > + ssize_t ret; > + Please push this to a separate patch. Thanks. -- Greg > #ifdef CONFIG_PREADV > ret = pwritev(fs->fd, iov, iovcnt, offset); > #else > @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) > error_report("fsdev: No path specified"); > return -1; > } > + > + if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) { > + return -1; > + } > + > fse->path = g_strdup(path); > > return 0; > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index dfe293d..ccf6d0c 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -3490,6 +3490,9 @@ 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; > + > v9fs_path_free(&path); > > rc = 0; > @@ -3504,6 +3507,9 @@ out: > > void v9fs_device_unrealize_common(V9fsState *s, Error **errp) > { > + if (throttle_enabled(&s->ctx.fst->cfg)) { > + fsdev_throttle_cleanup(s->ctx.fst); > + } > g_free(s->ctx.fs_root); > g_free(s->tag); > } > diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c > index 10343c0..039031d 100644 > --- a/hw/9pfs/cofile.c > +++ b/hw/9pfs/cofile.c > @@ -16,6 +16,7 @@ > #include "qemu/thread.h" > #include "qemu/coroutine.h" > #include "coth.h" > +#include "fsdev/qemu-fsdev-throttle.h" > > int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode, > V9fsStatDotl *v9stat) > @@ -245,6 +246,8 @@ int 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); > @@ -264,6 +267,8 @@ int 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);
On 10/24/2016 3:00 PM, Alberto Garcia wrote: > On Sat 22 Oct 2016 05:07:22 PM CEST, Pradeep Jagadeesh wrote: > >> 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. > > Ok, I think this is almost ready. There's only a few corrections and > style fixes left. Hi Alberto, thanks for having a look at the patch. I will send the patch early next week as I am out of office. -Pradeep > >> diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs >> index 1b120a4..2c6da2d 100644 >> --- a/fsdev/Makefile.objs >> +++ b/fsdev/Makefile.objs >> @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o >> endif >> common-obj-y += qemu-fsdev-opts.o >> >> +common-obj-y += qemu-fsdev-throttle.o > > (Optional) You can put these two in the same line: > > common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o > >> +++ b/fsdev/qemu-fsdev-throttle.c > > In this file there's many lines that are incorrectly indented. > Indentation should be four spaces. The following functions have lines > that need to be corrected: > > fsdev_throttle_check_for_wait > fsdev_throttle_schedule_next_request > fsdev_parse_io_limit_opts > fsdev_throttle_configure_iolimits > fsdev_co_throttle_request > > Check also the other changes in your patch in case there are more lines > with wrong indentation. > >> +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write) > > "Check for wait" doesn't sound like correct English to me. How about > simply fsdev_throttle_schedule_timer()? Or fsdev_throttle_must_wait() > >> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) >> +{ >> + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { > > Here's an example of a line with the wrong indentation. It uses three > spaces, it should use four. > >> + qemu_co_queue_next(&fst->throttled_reqs[is_write]); > > And this one uses seven, it should use eight. There are more cases like > this, I only highlighted a couple of them. > >> +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) >> +{ >> + Error *err = NULL; >> + >> + /* Parse and set populate the cfg structure */ >> + fsdev_parse_io_limit_opts(opts, fst); >> + >> + if (!throttle_is_valid(&fst->cfg, &err)) { >> + error_reportf_err(err, "Throttle configuration is not valid: "); >> + return -1; >> + } >> + if (throttle_enabled(&fst->cfg)) { >> + g_assert((fst->aioctx = qemu_get_aio_context())); >> + throttle_init(&fst->ts); >> + throttle_timers_init(&fst->tt, >> + fst->aioctx, >> + 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]); >> + } >> + return 0; >> +} > > Here you're parsing the command-line options and initializing the > throtting structures in local_parse_opts(). Then you're cleaning up in > v9fs_device_unrealize_common(). > > I have two questions: > > a) You could split fsdev_throttle_configure_iolimits() in two: > 1) fsdev_throttle_parse_opts(), called from local_parse_opts() > 2) fsdev_throttle_init(), called from v9fs_device_realize_common(). > > b) Why are you only doing this for the "local" fsdriver, and not for > "handle" or "proxy"? Isn't it possible to do throttling with those > as well? > >> +static int get_num_bytes(struct iovec *iov, int iovcnt) >> +{ >> + int i; >> + unsigned int bytes = 0; > > You changed 'bytes' from int to unsigned as I suggested, but you still > return an int, and in fsdev_co_throttle_request() you also get an > int. Please use unsigned in all these cases. > >> +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); >> + >> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, >> + struct iovec *iov, int iovcnt); > > Please align this line with the parameters of the first line. > > Also, why do you include the name of the parameters in this function > prototype but not in the other two? > >> @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts, >> struct FsDriverEntry *fse) >> error_report("fsdev: No path specified"); >> return -1; >> } >> + >> + if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) { >> + return -1; >> + } > > If you create fsdev_throttle_parse_opts() as I suggest, I'd rather print > the error message here as the other error messages in this function. > > I think I haven't forgotten anything, everything else looks fine. I'd > still like to have all the throtting parameters merged so we don't need > to duplicate them, but that can be done in the future (I can even take > care of it if I find a bit of time). > > Thanks for the patch! > > Berto >
On 10/24/2016 11:28 PM, Greg Kurz wrote: > Re-post (I had hit the send button by error :) > > On Sat, 22 Oct 2016 11:07:22 -0400 > Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote: > >> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> >> --- > > Hi Pradeep, > > I see that Berto already did a thorough review for this patch and I agree for > all the suggestions he made. > > I have some more to add. First: this patch doesn't apply, please rebase. Hi Greg, Thanks for having a look at the patch. I will incorporate the comments and send the patch early next week as I am out of office till then. -Pradeep > > More remarks below. > >> fsdev/Makefile.objs | 1 + >> fsdev/file-op-9p.h | 3 + >> fsdev/qemu-fsdev-opts.c | 76 +++++++++++++++++++++++ >> fsdev/qemu-fsdev-throttle.c | 147 ++++++++++++++++++++++++++++++++++++++++++++ >> fsdev/qemu-fsdev-throttle.h | 37 +++++++++++ >> hw/9pfs/9p-local.c | 9 ++- >> hw/9pfs/9p.c | 6 ++ >> hw/9pfs/cofile.c | 5 ++ >> 8 files changed, 282 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. >> > > The above lines are the changelog for the patch. We want this to be displayed > when running 'git log'. For this to happen, please move these lines above your > SoB tag. > > Only the vN -> vN+1 changes are not relevant (we don't need to record all the > intermediate reviews in git) and should stay here. > >> 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 >> >> >> diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs >> index 1b120a4..2c6da2d 100644 >> --- a/fsdev/Makefile.objs >> +++ b/fsdev/Makefile.objs >> @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o >> endif >> common-obj-y += qemu-fsdev-opts.o >> >> +common-obj-y += qemu-fsdev-throttle.o >> # Toplevel always builds this; targets without virtio will put it in >> # common-obj-y >> common-obj-$(CONFIG_ALL) += qemu-fsdev-dummy.o >> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h >> index 6db9fea..33fe822 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..395d497 100644 >> --- a/fsdev/qemu-fsdev-opts.c >> +++ b/fsdev/qemu-fsdev-opts.c >> @@ -37,6 +37,82 @@ 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..d48d031 >> --- /dev/null >> +++ b/fsdev/qemu-fsdev-throttle.c >> @@ -0,0 +1,147 @@ >> +/* >> + * Fsdev Throttle >> + * >> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH >> + * >> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 "qemu/error-report.h" >> +#include "qapi/error.h" >> +#include "qemu-fsdev-throttle.h" >> + >> +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write) >> +{ >> + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); >> +} >> + >> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) >> +{ >> + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { >> + if (fsdev_throttle_check_for_wait(fst, is_write)) { >> + return; >> + } >> + qemu_co_queue_next(&fst->throttled_reqs[is_write]); > > This calls for a coroutine_fn tag but... > >> + } >> +} >> + > > ... as Berto already suggested in v6, these functions aren't needed and your > "I just would like to keep these functions" answer isn't convincing enough. > Personally, I'd greatly really prefer the code to be inlined in the callers, > so that we can see the whole throttling/queue logic in one place. > >> +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]); >> +} >> + >> +static void fsdev_parse_io_limit_opts(QemuOpts *opts, FsThrottle *fst) >> +{ >> + 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); >> +} >> + >> +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) >> +{ >> + Error *err = NULL; >> + >> + /* Parse and set populate the cfg structure */ >> + fsdev_parse_io_limit_opts(opts, fst); >> + >> + if (!throttle_is_valid(&fst->cfg, &err)) { >> + error_reportf_err(err, "Throttle configuration is not valid: "); >> + return -1; >> + } >> + if (throttle_enabled(&fst->cfg)) { >> + g_assert((fst->aioctx = qemu_get_aio_context())); >> + throttle_init(&fst->ts); >> + throttle_timers_init(&fst->tt, >> + fst->aioctx, >> + 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]); >> + } >> + return 0; >> +} >> + >> +static int get_num_bytes(struct iovec *iov, int iovcnt) >> +{ >> + int i; >> + unsigned int bytes = 0; >> + >> + for (i = 0; i < iovcnt; i++) { >> + bytes += iov[i].iov_len; >> + } >> + return bytes; >> +} >> + >> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, >> + struct iovec *iov, int iovcnt) >> +{ >> + if (throttle_enabled(&fst->cfg)) { >> + int bytes = get_num_bytes(iov, iovcnt); >> + bool must_wait = fsdev_throttle_check_for_wait(fst, is_write); >> + if (must_wait || !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { >> + qemu_co_queue_wait(&fst->throttled_reqs[is_write]); >> + } >> + throttle_account(&fst->ts, is_write, bytes); >> + >> + fsdev_throttle_schedule_next_request(fst, is_write); >> + } >> +} >> + >> +void fsdev_throttle_cleanup(FsThrottle *fst) >> +{ >> + 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..df4e608 >> --- /dev/null >> +++ b/fsdev/qemu-fsdev-throttle.h >> @@ -0,0 +1,37 @@ >> +/* >> + * Fsdev Throttle >> + * >> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH >> + * >> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 _FSDEV_THROTTLE_H >> +#define _FSDEV_THROTTLE_H >> + >> +#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; >> + CoQueue throttled_reqs[2]; >> +} FsThrottle; >> + >> +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); >> + >> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, >> + struct iovec *iov, int iovcnt); >> + >> +void fsdev_throttle_cleanup(FsThrottle *); >> +#endif /* _FSDEV_THROTTLE_H */ >> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >> index 3f271fc..54459f2 100644 >> --- a/hw/9pfs/9p-local.c >> +++ b/hw/9pfs/9p-local.c >> @@ -436,8 +436,8 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, >> const struct iovec *iov, >> int iovcnt, off_t offset) >> { >> - ssize_t ret >> -; >> + ssize_t ret; >> + > > Please push this to a separate patch. > > Thanks. > > -- > Greg > >> #ifdef CONFIG_PREADV >> ret = pwritev(fs->fd, iov, iovcnt, offset); >> #else >> @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) >> error_report("fsdev: No path specified"); >> return -1; >> } >> + >> + if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) { >> + return -1; >> + } >> + >> fse->path = g_strdup(path); >> >> return 0; >> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c >> index dfe293d..ccf6d0c 100644 >> --- a/hw/9pfs/9p.c >> +++ b/hw/9pfs/9p.c >> @@ -3490,6 +3490,9 @@ 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; >> + >> v9fs_path_free(&path); >> >> rc = 0; >> @@ -3504,6 +3507,9 @@ out: >> >> void v9fs_device_unrealize_common(V9fsState *s, Error **errp) >> { >> + if (throttle_enabled(&s->ctx.fst->cfg)) { >> + fsdev_throttle_cleanup(s->ctx.fst); >> + } >> g_free(s->ctx.fs_root); >> g_free(s->tag); >> } >> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c >> index 10343c0..039031d 100644 >> --- a/hw/9pfs/cofile.c >> +++ b/hw/9pfs/cofile.c >> @@ -16,6 +16,7 @@ >> #include "qemu/thread.h" >> #include "qemu/coroutine.h" >> #include "coth.h" >> +#include "fsdev/qemu-fsdev-throttle.h" >> >> int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode, >> V9fsStatDotl *v9stat) >> @@ -245,6 +246,8 @@ int 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); >> @@ -264,6 +267,8 @@ int 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); >
>> >>> diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs >>> index 1b120a4..2c6da2d 100644 >>> --- a/fsdev/Makefile.objs >>> +++ b/fsdev/Makefile.objs >>> @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o >>> endif >>> common-obj-y += qemu-fsdev-opts.o >>> >>> +common-obj-y += qemu-fsdev-throttle.o >> >> (Optional) You can put these two in the same line: >> >> common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o >> >>> +++ b/fsdev/qemu-fsdev-throttle.c >> >> In this file there's many lines that are incorrectly indented. >> Indentation should be four spaces. The following functions have lines >> that need to be corrected: >> >> fsdev_throttle_check_for_wait >> fsdev_throttle_schedule_next_request >> fsdev_parse_io_limit_opts >> fsdev_throttle_configure_iolimits >> fsdev_co_throttle_request >> >> Check also the other changes in your patch in case there are more lines >> with wrong indentation. Fixed. >> >>> +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool >>> is_write) >> >> "Check for wait" doesn't sound like correct English to me. How about >> simply fsdev_throttle_schedule_timer()? Or fsdev_throttle_must_wait() >> >>> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, >>> bool is_write) >>> +{ >>> + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { >> >> Here's an example of a line with the wrong indentation. It uses three >> spaces, it should use four. >> >>> + qemu_co_queue_next(&fst->throttled_reqs[is_write]); >> >> And this one uses seven, it should use eight. There are more cases like >> this, I only highlighted a couple of them. >> Done! >>> +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) >>> +{ >>> + Error *err = NULL; >>> + >>> + /* Parse and set populate the cfg structure */ >>> + fsdev_parse_io_limit_opts(opts, fst); >>> + >>> + if (!throttle_is_valid(&fst->cfg, &err)) { >>> + error_reportf_err(err, "Throttle configuration is not valid: >>> "); >>> + return -1; >>> + } >>> + if (throttle_enabled(&fst->cfg)) { >>> + g_assert((fst->aioctx = qemu_get_aio_context())); >>> + throttle_init(&fst->ts); >>> + throttle_timers_init(&fst->tt, >>> + fst->aioctx, >>> + 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]); >>> + } >>> + return 0; >>> +} >> >> Here you're parsing the command-line options and initializing the >> throtting structures in local_parse_opts(). Then you're cleaning up in >> v9fs_device_unrealize_common(). >> >> I have two questions: >> >> a) You could split fsdev_throttle_configure_iolimits() in two: >> 1) fsdev_throttle_parse_opts(), called from local_parse_opts() >> 2) fsdev_throttle_init(), called from v9fs_device_realize_common(). >> Done as you suggested. Now parsing happens in local_parse_opts() and initialization in v9fs_device_realize_common() >> b) Why are you only doing this for the "local" fsdriver, and not for >> "handle" or "proxy"? Isn't it possible to do throttling with those >> as well? >> Yes, its possible. As of now I wanted to go with just with 9p-local because, we had a use case for only that. I feel with existing throttle apis (fsdev_throttle), it is easy enable for proxy and handle drivers also. >>> +static int get_num_bytes(struct iovec *iov, int iovcnt) >>> +{ >>> + int i; >>> + unsigned int bytes = 0; >> >> You changed 'bytes' from int to unsigned as I suggested, but you still >> return an int, and in fsdev_co_throttle_request() you also get an >> int. Please use unsigned in all these cases. >> done >>> +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); >>> + >>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool >>> is_write, >>> + struct iovec *iov, int iovcnt); >> >> Please align this line with the parameters of the first line. >> >> Also, why do you include the name of the parameters in this function >> prototype but not in the other two? I removed them. >> >>> @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts, >>> struct FsDriverEntry *fse) >>> error_report("fsdev: No path specified"); >>> return -1; >>> } >>> + >>> + if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) { >>> + return -1; >>> + } >> >> If you create fsdev_throttle_parse_opts() as I suggest, I'd rather print >> the error message here as the other error messages in this function. I re-wrote as suggested by you. >> >> I think I haven't forgotten anything, everything else looks fine. I'd >> still like to have all the throtting parameters merged so we don't need >> to duplicate them, but that can be done in the future (I can even take >> care of it if I find a bit of time). >> >> Thanks for the patch! -Pradeep >> Berto >>
>> More remarks below. >> >>> fsdev/Makefile.objs | 1 + >>> fsdev/file-op-9p.h | 3 + >>> fsdev/qemu-fsdev-opts.c | 76 +++++++++++++++++++++++ >>> fsdev/qemu-fsdev-throttle.c | 147 >>> ++++++++++++++++++++++++++++++++++++++++++++ >>> fsdev/qemu-fsdev-throttle.h | 37 +++++++++++ >>> hw/9pfs/9p-local.c | 9 ++- >>> hw/9pfs/9p.c | 6 ++ >>> hw/9pfs/cofile.c | 5 ++ >>> 8 files changed, 282 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. >>> >> >> The above lines are the changelog for the patch. We want this to be >> displayed >> when running 'git log'. For this to happen, please move these lines >> above your >> SoB tag. OK >> >> Only the vN -> vN+1 changes are not relevant (we don't need to record >> all the >> intermediate reviews in git) and should stay here. >> I just put there just keep track of what comments I fixed from which reviewer in recent patches. This we can take off when we have the final patch right? Also I did not understand your comment about vN -> vN+1. >>> 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 >>> >>> >>> diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs >>> index 1b120a4..2c6da2d 100644 >>> --- a/fsdev/Makefile.objs >>> +++ b/fsdev/Makefile.objs >>> @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o >>> endif >>> common-obj-y += qemu-fsdev-opts.o >>> >>> +common-obj-y += qemu-fsdev-throttle.o >>> # Toplevel always builds this; targets without virtio will put it in >>> # common-obj-y >>> common-obj-$(CONFIG_ALL) += qemu-fsdev-dummy.o >>> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h >>> index 6db9fea..33fe822 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..395d497 100644 >>> --- a/fsdev/qemu-fsdev-opts.c >>> +++ b/fsdev/qemu-fsdev-opts.c >>> @@ -37,6 +37,82 @@ 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..d48d031 >>> --- /dev/null >>> +++ b/fsdev/qemu-fsdev-throttle.c >>> @@ -0,0 +1,147 @@ >>> +/* >>> + * Fsdev Throttle >>> + * >>> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH >>> + * >>> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 "qemu/error-report.h" >>> +#include "qapi/error.h" >>> +#include "qemu-fsdev-throttle.h" >>> + >>> +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool >>> is_write) >>> +{ >>> + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); >>> +} >>> + >>> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, >>> bool is_write) >>> +{ >>> + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { >>> + if (fsdev_throttle_check_for_wait(fst, is_write)) { >>> + return; >>> + } >>> + qemu_co_queue_next(&fst->throttled_reqs[is_write]); >> >> This calls for a coroutine_fn tag but... >> >>> + } >>> +} >>> + >> >> ... as Berto already suggested in v6, these functions aren't needed >> and your >> "I just would like to keep these functions" answer isn't convincing >> enough. >> Personally, I'd greatly really prefer the code to be inlined in the >> callers, >> so that we can see the whole throttling/queue logic in one place. In lined the code fsdev_throttle_check_for_wait(). >> >>> +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]); >>> +} >>> + >>> +static void fsdev_parse_io_limit_opts(QemuOpts *opts, FsThrottle *fst) >>> +{ >>> + 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); >>> +} >>> + >>> +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) >>> +{ >>> + Error *err = NULL; >>> + >>> + /* Parse and set populate the cfg structure */ >>> + fsdev_parse_io_limit_opts(opts, fst); >>> + >>> + if (!throttle_is_valid(&fst->cfg, &err)) { >>> + error_reportf_err(err, "Throttle configuration is not valid: >>> "); >>> + return -1; >>> + } >>> + if (throttle_enabled(&fst->cfg)) { >>> + g_assert((fst->aioctx = qemu_get_aio_context())); >>> + throttle_init(&fst->ts); >>> + throttle_timers_init(&fst->tt, >>> + fst->aioctx, >>> + 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]); >>> + } >>> + return 0; >>> +} >>> + >>> +static int get_num_bytes(struct iovec *iov, int iovcnt) >>> +{ >>> + int i; >>> + unsigned int bytes = 0; >>> + >>> + for (i = 0; i < iovcnt; i++) { >>> + bytes += iov[i].iov_len; >>> + } >>> + return bytes; >>> +} >>> + >>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool >>> is_write, >>> + struct iovec *iov, int iovcnt) >>> +{ >>> + if (throttle_enabled(&fst->cfg)) { >>> + int bytes = get_num_bytes(iov, iovcnt); >>> + bool must_wait = fsdev_throttle_check_for_wait(fst, is_write); >>> + if (must_wait || >>> !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { >>> + qemu_co_queue_wait(&fst->throttled_reqs[is_write]); >>> + } >>> + throttle_account(&fst->ts, is_write, bytes); >>> + >>> + fsdev_throttle_schedule_next_request(fst, is_write); >>> + } >>> +} >>> + >>> +void fsdev_throttle_cleanup(FsThrottle *fst) >>> +{ >>> + 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..df4e608 >>> --- /dev/null >>> +++ b/fsdev/qemu-fsdev-throttle.h >>> @@ -0,0 +1,37 @@ >>> +/* >>> + * Fsdev Throttle >>> + * >>> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH >>> + * >>> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 _FSDEV_THROTTLE_H >>> +#define _FSDEV_THROTTLE_H >>> + >>> +#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; >>> + CoQueue throttled_reqs[2]; >>> +} FsThrottle; >>> + >>> +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); >>> + >>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool >>> is_write, >>> + struct iovec *iov, int iovcnt); >>> + >>> +void fsdev_throttle_cleanup(FsThrottle *); >>> +#endif /* _FSDEV_THROTTLE_H */ >>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >>> index 3f271fc..54459f2 100644 >>> --- a/hw/9pfs/9p-local.c >>> +++ b/hw/9pfs/9p-local.c >>> @@ -436,8 +436,8 @@ static ssize_t local_pwritev(FsContext *ctx, >>> V9fsFidOpenState *fs, >>> const struct iovec *iov, >>> int iovcnt, off_t offset) >>> { >>> - ssize_t ret >>> -; >>> + ssize_t ret; >>> + >> >> Please push this to a separate patch. OK. -Pradeep >> >> Thanks. >> >> -- >> Greg >> >>> #ifdef CONFIG_PREADV >>> ret = pwritev(fs->fd, iov, iovcnt, offset); >>> #else >>> @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts, >>> struct FsDriverEntry *fse) >>> error_report("fsdev: No path specified"); >>> return -1; >>> } >>> + >>> + if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) { >>> + return -1; >>> + } >>> + >>> fse->path = g_strdup(path); >>> >>> return 0; >>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c >>> index dfe293d..ccf6d0c 100644 >>> --- a/hw/9pfs/9p.c >>> +++ b/hw/9pfs/9p.c >>> @@ -3490,6 +3490,9 @@ 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; >>> + >>> v9fs_path_free(&path); >>> >>> rc = 0; >>> @@ -3504,6 +3507,9 @@ out: >>> >>> void v9fs_device_unrealize_common(V9fsState *s, Error **errp) >>> { >>> + if (throttle_enabled(&s->ctx.fst->cfg)) { >>> + fsdev_throttle_cleanup(s->ctx.fst); >>> + } >>> g_free(s->ctx.fs_root); >>> g_free(s->tag); >>> } >>> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c >>> index 10343c0..039031d 100644 >>> --- a/hw/9pfs/cofile.c >>> +++ b/hw/9pfs/cofile.c >>> @@ -16,6 +16,7 @@ >>> #include "qemu/thread.h" >>> #include "qemu/coroutine.h" >>> #include "coth.h" >>> +#include "fsdev/qemu-fsdev-throttle.h" >>> >>> int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode, >>> V9fsStatDotl *v9stat) >>> @@ -245,6 +246,8 @@ int 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); >>> @@ -264,6 +267,8 @@ int 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); >>
On Sun, 30 Oct 2016 14:04:49 +0100 Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote: > >> More remarks below. > >> > >>> fsdev/Makefile.objs | 1 + > >>> fsdev/file-op-9p.h | 3 + > >>> fsdev/qemu-fsdev-opts.c | 76 +++++++++++++++++++++++ > >>> fsdev/qemu-fsdev-throttle.c | 147 > >>> ++++++++++++++++++++++++++++++++++++++++++++ > >>> fsdev/qemu-fsdev-throttle.h | 37 +++++++++++ > >>> hw/9pfs/9p-local.c | 9 ++- > >>> hw/9pfs/9p.c | 6 ++ > >>> hw/9pfs/cofile.c | 5 ++ > >>> 8 files changed, 282 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. > >>> > >> > >> The above lines are the changelog for the patch. We want this to be > >> displayed > >> when running 'git log'. For this to happen, please move these lines > >> above your > >> SoB tag. > OK > >> > >> Only the vN -> vN+1 changes are not relevant (we don't need to record > >> all the > >> intermediate reviews in git) and should stay here. > >> > I just put there just keep track of what comments I fixed from which > reviewer in recent patches. > This we can take off when we have the final patch right? > > Also I did not understand your comment about vN -> vN+1. Heh sorry for not being clear :) I was saying only the v1->v2, v2->v3 (i.e. vN -> vN+1) parts must stay below the --- so that git am doesn't copy them to the commit log. So, you're good with this part :) > >>> 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 > >>> > >>> > >>> diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs > >>> index 1b120a4..2c6da2d 100644 > >>> --- a/fsdev/Makefile.objs > >>> +++ b/fsdev/Makefile.objs > >>> @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o > >>> endif > >>> common-obj-y += qemu-fsdev-opts.o > >>> > >>> +common-obj-y += qemu-fsdev-throttle.o > >>> # Toplevel always builds this; targets without virtio will put it in > >>> # common-obj-y > >>> common-obj-$(CONFIG_ALL) += qemu-fsdev-dummy.o > >>> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h > >>> index 6db9fea..33fe822 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..395d497 100644 > >>> --- a/fsdev/qemu-fsdev-opts.c > >>> +++ b/fsdev/qemu-fsdev-opts.c > >>> @@ -37,6 +37,82 @@ 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..d48d031 > >>> --- /dev/null > >>> +++ b/fsdev/qemu-fsdev-throttle.c > >>> @@ -0,0 +1,147 @@ > >>> +/* > >>> + * Fsdev Throttle > >>> + * > >>> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH > >>> + * > >>> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 "qemu/error-report.h" > >>> +#include "qapi/error.h" > >>> +#include "qemu-fsdev-throttle.h" > >>> + > >>> +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool > >>> is_write) > >>> +{ > >>> + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); > >>> +} > >>> + > >>> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, > >>> bool is_write) > >>> +{ > >>> + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { > >>> + if (fsdev_throttle_check_for_wait(fst, is_write)) { > >>> + return; > >>> + } > >>> + qemu_co_queue_next(&fst->throttled_reqs[is_write]); > >> > >> This calls for a coroutine_fn tag but... > >> > >>> + } > >>> +} > >>> + > >> > >> ... as Berto already suggested in v6, these functions aren't needed > >> and your > >> "I just would like to keep these functions" answer isn't convincing > >> enough. > >> Personally, I'd greatly really prefer the code to be inlined in the > >> callers, > >> so that we can see the whole throttling/queue logic in one place. > In lined the code fsdev_throttle_check_for_wait(). Cool, thanks! I'll be AFK most of tomorrow and soft freeze starts next Tuesday... do you *really* need this to be in 2.8 ? Cheers. -- Greg > >> > >>> +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]); > >>> +} > >>> + > >>> +static void fsdev_parse_io_limit_opts(QemuOpts *opts, FsThrottle *fst) > >>> +{ > >>> + 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); > >>> +} > >>> + > >>> +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) > >>> +{ > >>> + Error *err = NULL; > >>> + > >>> + /* Parse and set populate the cfg structure */ > >>> + fsdev_parse_io_limit_opts(opts, fst); > >>> + > >>> + if (!throttle_is_valid(&fst->cfg, &err)) { > >>> + error_reportf_err(err, "Throttle configuration is not valid: > >>> "); > >>> + return -1; > >>> + } > >>> + if (throttle_enabled(&fst->cfg)) { > >>> + g_assert((fst->aioctx = qemu_get_aio_context())); > >>> + throttle_init(&fst->ts); > >>> + throttle_timers_init(&fst->tt, > >>> + fst->aioctx, > >>> + 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]); > >>> + } > >>> + return 0; > >>> +} > >>> + > >>> +static int get_num_bytes(struct iovec *iov, int iovcnt) > >>> +{ > >>> + int i; > >>> + unsigned int bytes = 0; > >>> + > >>> + for (i = 0; i < iovcnt; i++) { > >>> + bytes += iov[i].iov_len; > >>> + } > >>> + return bytes; > >>> +} > >>> + > >>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool > >>> is_write, > >>> + struct iovec *iov, int iovcnt) > >>> +{ > >>> + if (throttle_enabled(&fst->cfg)) { > >>> + int bytes = get_num_bytes(iov, iovcnt); > >>> + bool must_wait = fsdev_throttle_check_for_wait(fst, is_write); > >>> + if (must_wait || > >>> !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { > >>> + qemu_co_queue_wait(&fst->throttled_reqs[is_write]); > >>> + } > >>> + throttle_account(&fst->ts, is_write, bytes); > >>> + > >>> + fsdev_throttle_schedule_next_request(fst, is_write); > >>> + } > >>> +} > >>> + > >>> +void fsdev_throttle_cleanup(FsThrottle *fst) > >>> +{ > >>> + 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..df4e608 > >>> --- /dev/null > >>> +++ b/fsdev/qemu-fsdev-throttle.h > >>> @@ -0,0 +1,37 @@ > >>> +/* > >>> + * Fsdev Throttle > >>> + * > >>> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH > >>> + * > >>> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 _FSDEV_THROTTLE_H > >>> +#define _FSDEV_THROTTLE_H > >>> + > >>> +#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; > >>> + CoQueue throttled_reqs[2]; > >>> +} FsThrottle; > >>> + > >>> +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); > >>> + > >>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool > >>> is_write, > >>> + struct iovec *iov, int iovcnt); > >>> + > >>> +void fsdev_throttle_cleanup(FsThrottle *); > >>> +#endif /* _FSDEV_THROTTLE_H */ > >>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > >>> index 3f271fc..54459f2 100644 > >>> --- a/hw/9pfs/9p-local.c > >>> +++ b/hw/9pfs/9p-local.c > >>> @@ -436,8 +436,8 @@ static ssize_t local_pwritev(FsContext *ctx, > >>> V9fsFidOpenState *fs, > >>> const struct iovec *iov, > >>> int iovcnt, off_t offset) > >>> { > >>> - ssize_t ret > >>> -; > >>> + ssize_t ret; > >>> + > >> > >> Please push this to a separate patch. > OK. > > -Pradeep > > >> > >> Thanks. > >> > >> -- > >> Greg > >> > >>> #ifdef CONFIG_PREADV > >>> ret = pwritev(fs->fd, iov, iovcnt, offset); > >>> #else > >>> @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts, > >>> struct FsDriverEntry *fse) > >>> error_report("fsdev: No path specified"); > >>> return -1; > >>> } > >>> + > >>> + if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) { > >>> + return -1; > >>> + } > >>> + > >>> fse->path = g_strdup(path); > >>> > >>> return 0; > >>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > >>> index dfe293d..ccf6d0c 100644 > >>> --- a/hw/9pfs/9p.c > >>> +++ b/hw/9pfs/9p.c > >>> @@ -3490,6 +3490,9 @@ 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; > >>> + > >>> v9fs_path_free(&path); > >>> > >>> rc = 0; > >>> @@ -3504,6 +3507,9 @@ out: > >>> > >>> void v9fs_device_unrealize_common(V9fsState *s, Error **errp) > >>> { > >>> + if (throttle_enabled(&s->ctx.fst->cfg)) { > >>> + fsdev_throttle_cleanup(s->ctx.fst); > >>> + } > >>> g_free(s->ctx.fs_root); > >>> g_free(s->tag); > >>> } > >>> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c > >>> index 10343c0..039031d 100644 > >>> --- a/hw/9pfs/cofile.c > >>> +++ b/hw/9pfs/cofile.c > >>> @@ -16,6 +16,7 @@ > >>> #include "qemu/thread.h" > >>> #include "qemu/coroutine.h" > >>> #include "coth.h" > >>> +#include "fsdev/qemu-fsdev-throttle.h" > >>> > >>> int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode, > >>> V9fsStatDotl *v9stat) > >>> @@ -245,6 +246,8 @@ int 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); > >>> @@ -264,6 +267,8 @@ int 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); > >> >
On 10/30/2016 2:35 PM, Greg Kurz wrote: > On Sun, 30 Oct 2016 14:04:49 +0100 > Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote: > >>>> More remarks below. >>>> >>>>> fsdev/Makefile.objs | 1 + >>>>> fsdev/file-op-9p.h | 3 + >>>>> fsdev/qemu-fsdev-opts.c | 76 +++++++++++++++++++++++ >>>>> fsdev/qemu-fsdev-throttle.c | 147 >>>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>>> fsdev/qemu-fsdev-throttle.h | 37 +++++++++++ >>>>> hw/9pfs/9p-local.c | 9 ++- >>>>> hw/9pfs/9p.c | 6 ++ >>>>> hw/9pfs/cofile.c | 5 ++ >>>>> 8 files changed, 282 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. >>>>> >>>> >>>> The above lines are the changelog for the patch. We want this to be >>>> displayed >>>> when running 'git log'. For this to happen, please move these lines >>>> above your >>>> SoB tag. >> OK >>>> >>>> Only the vN -> vN+1 changes are not relevant (we don't need to record >>>> all the >>>> intermediate reviews in git) and should stay here. >>>> >> I just put there just keep track of what comments I fixed from which >> reviewer in recent patches. >> This we can take off when we have the final patch right? >> >> Also I did not understand your comment about vN -> vN+1. > > Heh sorry for not being clear :) > > I was saying only the v1->v2, v2->v3 (i.e. vN -> vN+1) parts must stay below > the --- so that git am doesn't copy them to the commit log. So, you're good > with this part :) OK, thanks. > >>>>> 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 >>>>> >>>>> >>>>> diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs >>>>> index 1b120a4..2c6da2d 100644 >>>>> --- a/fsdev/Makefile.objs >>>>> +++ b/fsdev/Makefile.objs >>>>> @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o >>>>> endif >>>>> common-obj-y += qemu-fsdev-opts.o >>>>> >>>>> +common-obj-y += qemu-fsdev-throttle.o >>>>> # Toplevel always builds this; targets without virtio will put it in >>>>> # common-obj-y >>>>> common-obj-$(CONFIG_ALL) += qemu-fsdev-dummy.o >>>>> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h >>>>> index 6db9fea..33fe822 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..395d497 100644 >>>>> --- a/fsdev/qemu-fsdev-opts.c >>>>> +++ b/fsdev/qemu-fsdev-opts.c >>>>> @@ -37,6 +37,82 @@ 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..d48d031 >>>>> --- /dev/null >>>>> +++ b/fsdev/qemu-fsdev-throttle.c >>>>> @@ -0,0 +1,147 @@ >>>>> +/* >>>>> + * Fsdev Throttle >>>>> + * >>>>> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH >>>>> + * >>>>> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 "qemu/error-report.h" >>>>> +#include "qapi/error.h" >>>>> +#include "qemu-fsdev-throttle.h" >>>>> + >>>>> +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool >>>>> is_write) >>>>> +{ >>>>> + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); >>>>> +} >>>>> + >>>>> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, >>>>> bool is_write) >>>>> +{ >>>>> + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { >>>>> + if (fsdev_throttle_check_for_wait(fst, is_write)) { >>>>> + return; >>>>> + } >>>>> + qemu_co_queue_next(&fst->throttled_reqs[is_write]); >>>> >>>> This calls for a coroutine_fn tag but... >>>> >>>>> + } >>>>> +} >>>>> + >>>> >>>> ... as Berto already suggested in v6, these functions aren't needed >>>> and your >>>> "I just would like to keep these functions" answer isn't convincing >>>> enough. >>>> Personally, I'd greatly really prefer the code to be inlined in the >>>> callers, >>>> so that we can see the whole throttling/queue logic in one place. >> In lined the code fsdev_throttle_check_for_wait(). > > Cool, thanks! > > I'll be AFK most of tomorrow and soft freeze starts next Tuesday... do you > *really* need this to be in 2.8 ? > If everything is OK, then we can push into 2.8. When is the next release?. -Pradeep > Cheers. > > -- > Greg > >>>> >>>>> +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]); >>>>> +} >>>>> + >>>>> +static void fsdev_parse_io_limit_opts(QemuOpts *opts, FsThrottle *fst) >>>>> +{ >>>>> + 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); >>>>> +} >>>>> + >>>>> +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) >>>>> +{ >>>>> + Error *err = NULL; >>>>> + >>>>> + /* Parse and set populate the cfg structure */ >>>>> + fsdev_parse_io_limit_opts(opts, fst); >>>>> + >>>>> + if (!throttle_is_valid(&fst->cfg, &err)) { >>>>> + error_reportf_err(err, "Throttle configuration is not valid: >>>>> "); >>>>> + return -1; >>>>> + } >>>>> + if (throttle_enabled(&fst->cfg)) { >>>>> + g_assert((fst->aioctx = qemu_get_aio_context())); >>>>> + throttle_init(&fst->ts); >>>>> + throttle_timers_init(&fst->tt, >>>>> + fst->aioctx, >>>>> + 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]); >>>>> + } >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int get_num_bytes(struct iovec *iov, int iovcnt) >>>>> +{ >>>>> + int i; >>>>> + unsigned int bytes = 0; >>>>> + >>>>> + for (i = 0; i < iovcnt; i++) { >>>>> + bytes += iov[i].iov_len; >>>>> + } >>>>> + return bytes; >>>>> +} >>>>> + >>>>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool >>>>> is_write, >>>>> + struct iovec *iov, int iovcnt) >>>>> +{ >>>>> + if (throttle_enabled(&fst->cfg)) { >>>>> + int bytes = get_num_bytes(iov, iovcnt); >>>>> + bool must_wait = fsdev_throttle_check_for_wait(fst, is_write); >>>>> + if (must_wait || >>>>> !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { >>>>> + qemu_co_queue_wait(&fst->throttled_reqs[is_write]); >>>>> + } >>>>> + throttle_account(&fst->ts, is_write, bytes); >>>>> + >>>>> + fsdev_throttle_schedule_next_request(fst, is_write); >>>>> + } >>>>> +} >>>>> + >>>>> +void fsdev_throttle_cleanup(FsThrottle *fst) >>>>> +{ >>>>> + 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..df4e608 >>>>> --- /dev/null >>>>> +++ b/fsdev/qemu-fsdev-throttle.h >>>>> @@ -0,0 +1,37 @@ >>>>> +/* >>>>> + * Fsdev Throttle >>>>> + * >>>>> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH >>>>> + * >>>>> + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 _FSDEV_THROTTLE_H >>>>> +#define _FSDEV_THROTTLE_H >>>>> + >>>>> +#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; >>>>> + CoQueue throttled_reqs[2]; >>>>> +} FsThrottle; >>>>> + >>>>> +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); >>>>> + >>>>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool >>>>> is_write, >>>>> + struct iovec *iov, int iovcnt); >>>>> + >>>>> +void fsdev_throttle_cleanup(FsThrottle *); >>>>> +#endif /* _FSDEV_THROTTLE_H */ >>>>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >>>>> index 3f271fc..54459f2 100644 >>>>> --- a/hw/9pfs/9p-local.c >>>>> +++ b/hw/9pfs/9p-local.c >>>>> @@ -436,8 +436,8 @@ static ssize_t local_pwritev(FsContext *ctx, >>>>> V9fsFidOpenState *fs, >>>>> const struct iovec *iov, >>>>> int iovcnt, off_t offset) >>>>> { >>>>> - ssize_t ret >>>>> -; >>>>> + ssize_t ret; >>>>> + >>>> >>>> Please push this to a separate patch. >> OK. >> >> -Pradeep >> >>>> >>>> Thanks. >>>> >>>> -- >>>> Greg >>>> >>>>> #ifdef CONFIG_PREADV >>>>> ret = pwritev(fs->fd, iov, iovcnt, offset); >>>>> #else >>>>> @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts, >>>>> struct FsDriverEntry *fse) >>>>> error_report("fsdev: No path specified"); >>>>> return -1; >>>>> } >>>>> + >>>>> + if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) { >>>>> + return -1; >>>>> + } >>>>> + >>>>> fse->path = g_strdup(path); >>>>> >>>>> return 0; >>>>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c >>>>> index dfe293d..ccf6d0c 100644 >>>>> --- a/hw/9pfs/9p.c >>>>> +++ b/hw/9pfs/9p.c >>>>> @@ -3490,6 +3490,9 @@ 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; >>>>> + >>>>> v9fs_path_free(&path); >>>>> >>>>> rc = 0; >>>>> @@ -3504,6 +3507,9 @@ out: >>>>> >>>>> void v9fs_device_unrealize_common(V9fsState *s, Error **errp) >>>>> { >>>>> + if (throttle_enabled(&s->ctx.fst->cfg)) { >>>>> + fsdev_throttle_cleanup(s->ctx.fst); >>>>> + } >>>>> g_free(s->ctx.fs_root); >>>>> g_free(s->tag); >>>>> } >>>>> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c >>>>> index 10343c0..039031d 100644 >>>>> --- a/hw/9pfs/cofile.c >>>>> +++ b/hw/9pfs/cofile.c >>>>> @@ -16,6 +16,7 @@ >>>>> #include "qemu/thread.h" >>>>> #include "qemu/coroutine.h" >>>>> #include "coth.h" >>>>> +#include "fsdev/qemu-fsdev-throttle.h" >>>>> >>>>> int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode, >>>>> V9fsStatDotl *v9stat) >>>>> @@ -245,6 +246,8 @@ int 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); >>>>> @@ -264,6 +267,8 @@ int 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); >>>> >> >
On Sun, 30 Oct 2016 15:02:17 +0100 Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote: > On 10/30/2016 2:35 PM, Greg Kurz wrote: [...] > > > > I'll be AFK most of tomorrow and soft freeze starts next Tuesday... do you > > *really* need this to be in 2.8 ? > > > If everything is OK, then we can push into 2.8. I'm ok to issue a pull request before feature soft freeze if the patch looks ok to me and it gets a R-b from Berto. > When is the next release?. > Don't know yet... very rough estimate would be end of March or April next year I guess. > -Pradeep Cheers. -- Greg
diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs index 1b120a4..2c6da2d 100644 --- a/fsdev/Makefile.objs +++ b/fsdev/Makefile.objs @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o endif common-obj-y += qemu-fsdev-opts.o +common-obj-y += qemu-fsdev-throttle.o # Toplevel always builds this; targets without virtio will put it in # common-obj-y common-obj-$(CONFIG_ALL) += qemu-fsdev-dummy.o diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index 6db9fea..33fe822 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..395d497 100644 --- a/fsdev/qemu-fsdev-opts.c +++ b/fsdev/qemu-fsdev-opts.c @@ -37,6 +37,82 @@ 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..d48d031 --- /dev/null +++ b/fsdev/qemu-fsdev-throttle.c @@ -0,0 +1,147 @@ +/* + * Fsdev Throttle + * + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH + * + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 "qemu/error-report.h" +#include "qapi/error.h" +#include "qemu-fsdev-throttle.h" + +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write) +{ + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); +} + +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) +{ + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { + if (fsdev_throttle_check_for_wait(fst, is_write)) { + return; + } + qemu_co_queue_next(&fst->throttled_reqs[is_write]); + } +} + +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]); +} + +static void fsdev_parse_io_limit_opts(QemuOpts *opts, FsThrottle *fst) +{ + 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); +} + +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) +{ + Error *err = NULL; + + /* Parse and set populate the cfg structure */ + fsdev_parse_io_limit_opts(opts, fst); + + if (!throttle_is_valid(&fst->cfg, &err)) { + error_reportf_err(err, "Throttle configuration is not valid: "); + return -1; + } + if (throttle_enabled(&fst->cfg)) { + g_assert((fst->aioctx = qemu_get_aio_context())); + throttle_init(&fst->ts); + throttle_timers_init(&fst->tt, + fst->aioctx, + 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]); + } + return 0; +} + +static int get_num_bytes(struct iovec *iov, int iovcnt) +{ + int i; + unsigned int bytes = 0; + + for (i = 0; i < iovcnt; i++) { + bytes += iov[i].iov_len; + } + return bytes; +} + +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, + struct iovec *iov, int iovcnt) +{ + if (throttle_enabled(&fst->cfg)) { + int bytes = get_num_bytes(iov, iovcnt); + bool must_wait = fsdev_throttle_check_for_wait(fst, is_write); + if (must_wait || !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { + qemu_co_queue_wait(&fst->throttled_reqs[is_write]); + } + throttle_account(&fst->ts, is_write, bytes); + + fsdev_throttle_schedule_next_request(fst, is_write); + } +} + +void fsdev_throttle_cleanup(FsThrottle *fst) +{ + 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..df4e608 --- /dev/null +++ b/fsdev/qemu-fsdev-throttle.h @@ -0,0 +1,37 @@ +/* + * Fsdev Throttle + * + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH + * + * Author: Pradeep Jagadeesh <pradeep.jagadeesh@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 _FSDEV_THROTTLE_H +#define _FSDEV_THROTTLE_H + +#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; + CoQueue throttled_reqs[2]; +} FsThrottle; + +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); + +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, + struct iovec *iov, int iovcnt); + +void fsdev_throttle_cleanup(FsThrottle *); +#endif /* _FSDEV_THROTTLE_H */ diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 3f271fc..54459f2 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -436,8 +436,8 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, const struct iovec *iov, int iovcnt, off_t offset) { - ssize_t ret -; + ssize_t ret; + #ifdef CONFIG_PREADV ret = pwritev(fs->fd, iov, iovcnt, offset); #else @@ -1240,6 +1240,11 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) error_report("fsdev: No path specified"); return -1; } + + if (fsdev_throttle_configure_iolimits(opts, &fse->fst) < 0) { + return -1; + } + fse->path = g_strdup(path); return 0; diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index dfe293d..ccf6d0c 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -3490,6 +3490,9 @@ 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; + v9fs_path_free(&path); rc = 0; @@ -3504,6 +3507,9 @@ out: void v9fs_device_unrealize_common(V9fsState *s, Error **errp) { + if (throttle_enabled(&s->ctx.fst->cfg)) { + fsdev_throttle_cleanup(s->ctx.fst); + } g_free(s->ctx.fs_root); g_free(s->tag); } diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c index 10343c0..039031d 100644 --- a/hw/9pfs/cofile.c +++ b/hw/9pfs/cofile.c @@ -16,6 +16,7 @@ #include "qemu/thread.h" #include "qemu/coroutine.h" #include "coth.h" +#include "fsdev/qemu-fsdev-throttle.h" int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode, V9fsStatDotl *v9stat) @@ -245,6 +246,8 @@ int 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); @@ -264,6 +267,8 @@ int 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);
Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> --- fsdev/Makefile.objs | 1 + fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 76 +++++++++++++++++++++++ fsdev/qemu-fsdev-throttle.c | 147 ++++++++++++++++++++++++++++++++++++++++++++ fsdev/qemu-fsdev-throttle.h | 37 +++++++++++ hw/9pfs/9p-local.c | 9 ++- hw/9pfs/9p.c | 6 ++ hw/9pfs/cofile.c | 5 ++ 8 files changed, 282 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. 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