From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44687) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6ZpB-0004bP-2u for qemu-devel@nongnu.org; Tue, 15 Nov 2016 04:14:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6Zp7-0008MU-Nz for qemu-devel@nongnu.org; Tue, 15 Nov 2016 04:14:41 -0500 Received: from lhrrgout.huawei.com ([194.213.3.17]:4665) by eggs.gnu.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.71) (envelope-from ) id 1c6Zp7-0008Hq-7r for qemu-devel@nongnu.org; Tue, 15 Nov 2016 04:14:37 -0500 References: <1478854467-25061-1-git-send-email-pradeep.jagadeesh@huawei.com> <20161112151328.0646d3b5@bahia> <591b301b-91a1-45b1-f10b-b94929d4d0d0@huawei.com> <20161114185726.1799a87f@bahia> From: Pradeep Jagadeesh Message-ID: <911be2d9-9dac-d761-7dd8-ed29e6e44979@huawei.com> Date: Tue, 15 Nov 2016 10:13:59 +0100 MIME-Version: 1.0 In-Reply-To: <20161114185726.1799a87f@bahia> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V11 1/1] fsdev: add IO throttle support to fsdev devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Pradeep Jagadeesh , "Aneesh Kumar K.V" , Alberto Garcia , qemu-devel@nongnu.org, Claudio Fontana On 11/14/2016 6:57 PM, Greg Kurz wrote: > On Mon, 14 Nov 2016 10:03:40 +0100 > Pradeep Jagadeesh wrote: > >> On 11/12/2016 3:13 PM, Greg Kurz wrote: >>> On Fri, 11 Nov 2016 03:54:27 -0500 >>> Pradeep Jagadeesh wrote: >>> >>>> Uses throttling APIs to limit I/O bandwidth and number of operations on the >>>> devices which use 9p-local driver. >>>> >>>> Signed-off-by: Pradeep Jagadeesh >>>> Reviewed-by: Alberto Garcia" >>>> --- >>> >>> Hi Pradeep, >>> >>> I'll have a look next week but I'm not sure this can go to 2.8 since we're >>> already in soft feature freeze (only bug fixes are accepted). >> >> Hi Greg, >> >> It is ok even if it does not make it to 2.8.I just want to complete this >> work from my side. >> > > Hi Pradeep, > > The patch looks good to me now. Since we're no more in a hurry to get this > merged, maybe you can now try to address the code duplication issue with > the command line options ? Ideally this should be done in a preparatory > patch. > OK,I will start that work in couple of weeks. As I am busy with some other work. Cheers, Pradeep > Cheers. > > -- > Greg > >> Thanks, >> Pradeep >>> >>> Cheers. >>> >>> -- >>> Greg >>> >>>> fsdev/Makefile.objs | 2 +- >>>> fsdev/file-op-9p.h | 3 ++ >>>> fsdev/qemu-fsdev-opts.c | 77 ++++++++++++++++++++++++++++- >>>> fsdev/qemu-fsdev-throttle.c | 117 ++++++++++++++++++++++++++++++++++++++++++++ >>>> fsdev/qemu-fsdev-throttle.h | 39 +++++++++++++++ >>>> hw/9pfs/9p-local.c | 8 +++ >>>> hw/9pfs/9p.c | 5 ++ >>>> hw/9pfs/cofile.c | 2 + >>>> 8 files changed, 251 insertions(+), 2 deletions(-) >>>> create mode 100644 fsdev/qemu-fsdev-throttle.c >>>> create mode 100644 fsdev/qemu-fsdev-throttle.h >>>> >>>> This adds the support for the 9p-local driver. >>>> For now this functionality can be enabled only through qemu cli options. >>>> QMP interface and support to other drivers need further extensions. >>>> To make it simple for other drivers, the throttle code has been put in >>>> separate files. >>>> >>>> v1 -> v2: >>>> >>>> -Fixed FsContext redeclaration issue >>>> -Removed couple of function declarations from 9p-throttle.h >>>> -Fixed some of the .help messages >>>> >>>> v2 -> v3: >>>> >>>> -Addressed follwing comments by Claudio Fontana >>>> -Removed redundant memset calls in fsdev_throttle_configure_iolimits function >>>> -Checking throttle structure validity before initializing other structures >>>> in fsdev_throttle_configure_iolimits >>>> >>>> -Addressed following comments by Greg Kurz >>>> -Moved the code from 9pfs directory to fsdev directory, because the throttling >>>> is for the fsdev devices.Renamed the files and functions to fsdev_ from 9pfs_ >>>> -Renamed throttling cli options to throttling.*, as in QMP cli options >>>> -Removed some of the unwanted .h files from qemu-fsdev-throttle.[ch] >>>> -Using throttle_enabled() function to set the thottle enabled flag for fsdev. >>>> >>>> v3 -> v4: >>>> >>>> -Addressed following comments by Alberto Garcia >>>> -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] >>>> >>>> -Addressed following comments by Greg Kurz >>>> -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function >>>> >>>> v4 -> V5: >>>> -Fixed the issue with the larger block size accounting. >>>> (i.e, when the 9pfs mounted using msize=xxx option) >>>> >>>> V5 -> V6: >>>> -Addressed the comments by Alberto Garcia >>>> -Removed the fsdev_throttle_timer_cb() >>>> -Simplified the fsdev_throttle_schedule_next_request() as suggested >>>> >>>> V6 -> V7: >>>> -Addressed the comments by Alberto Garcia >>>> -changed the fsdev_throttle_schedule_next_request() as suggested >>>> >>>> v7 -> v8: >>>> -Addressed comments by Alberto Garcia >>>> -Fixed some indentation issues and split the configure_io_limit function >>>> -Inlined throttle_timer_check code >>>> >>>> V8 -> v9: >>>> -Addressed the comments by Greg Kurz >>>> -Inlined the fsdev_throttle_schedule_next_request() code into >>>> fsdev_co_throttle_request () >>>> >>>> v9 -> v10: >>>> -Addressed the comments by alberto garcia >>>> -fixed the indentation issues and minor issues >>>> >>>> v10 -> v11: >>>> -Addressed the comments by alberto garcia >>>> -renamed err variable to errp issues >>>> >>>> >>>> diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs >>>> index 1b120a4..659df6e 100644 >>>> --- a/fsdev/Makefile.objs >>>> +++ b/fsdev/Makefile.objs >>>> @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o >>>> else >>>> common-obj-y = qemu-fsdev-dummy.o >>>> endif >>>> -common-obj-y += qemu-fsdev-opts.o >>>> +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o >>>> >>>> # Toplevel always builds this; targets without virtio will put it in >>>> # common-obj-y >>>> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h >>>> index 6db9fea..33fe822 100644 >>>> --- a/fsdev/file-op-9p.h >>>> +++ b/fsdev/file-op-9p.h >>>> @@ -17,6 +17,7 @@ >>>> #include >>>> #include >>>> #include >>>> +#include "qemu-fsdev-throttle.h" >>>> >>>> #define SM_LOCAL_MODE_BITS 0600 >>>> #define SM_LOCAL_DIR_MODE_BITS 0700 >>>> @@ -74,6 +75,7 @@ typedef struct FsDriverEntry { >>>> char *path; >>>> int export_flags; >>>> FileOperations *ops; >>>> + FsThrottle fst; >>>> } FsDriverEntry; >>>> >>>> typedef struct FsContext >>>> @@ -83,6 +85,7 @@ typedef struct FsContext >>>> int export_flags; >>>> struct xattr_operations **xops; >>>> struct extended_ops exops; >>>> + FsThrottle *fst; >>>> /* fs driver specific data */ >>>> void *private; >>>> } FsContext; >>>> diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c >>>> index 1dd8c7a..385423f0 100644 >>>> --- a/fsdev/qemu-fsdev-opts.c >>>> +++ b/fsdev/qemu-fsdev-opts.c >>>> @@ -37,8 +37,83 @@ static QemuOptsList qemu_fsdev_opts = { >>>> }, { >>>> .name = "sock_fd", >>>> .type = QEMU_OPT_NUMBER, >>>> + }, { >>>> + .name = "throttling.iops-total", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "limit total I/O operations per second", >>>> + }, { >>>> + .name = "throttling.iops-read", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "limit read operations per second", >>>> + }, { >>>> + .name = "throttling.iops-write", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "limit write operations per second", >>>> + }, { >>>> + .name = "throttling.bps-total", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "limit total bytes per second", >>>> + }, { >>>> + .name = "throttling.bps-read", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "limit read bytes per second", >>>> + }, { >>>> + .name = "throttling.bps-write", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "limit write bytes per second", >>>> + }, { >>>> + .name = "throttling.iops-total-max", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "I/O operations burst", >>>> + }, { >>>> + .name = "throttling.iops-read-max", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "I/O operations read burst", >>>> + }, { >>>> + .name = "throttling.iops-write-max", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "I/O operations write burst", >>>> + }, { >>>> + .name = "throttling.bps-total-max", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "total bytes burst", >>>> + }, { >>>> + .name = "throttling.bps-read-max", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "total bytes read burst", >>>> + }, { >>>> + .name = "throttling.bps-write-max", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "total bytes write burst", >>>> + }, { >>>> + .name = "throttling.iops-total-max-length", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "length of the iops-total-max burst period, in seconds", >>>> + }, { >>>> + .name = "throttling.iops-read-max-length", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "length of the iops-read-max burst period, in seconds", >>>> + }, { >>>> + .name = "throttling.iops-write-max-length", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "length of the iops-write-max burst period, in seconds", >>>> + }, { >>>> + .name = "throttling.bps-total-max-length", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "length of the bps-total-max burst period, in seconds", >>>> + }, { >>>> + .name = "throttling.bps-read-max-length", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "length of the bps-read-max burst period, in seconds", >>>> + }, { >>>> + .name = "throttling.bps-write-max-length", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "length of the bps-write-max burst period, in seconds", >>>> + }, { >>>> + .name = "throttling.iops-size", >>>> + .type = QEMU_OPT_NUMBER, >>>> + .help = "when limiting by iops max size of an I/O in bytes", >>>> }, >>>> - >>>> { /*End of list */ } >>>> }, >>>> }; >>>> diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c >>>> new file mode 100644 >>>> index 0000000..193ad26 >>>> --- /dev/null >>>> +++ b/fsdev/qemu-fsdev-throttle.c >>>> @@ -0,0 +1,117 @@ >>>> +/* >>>> + * Fsdev Throttle >>>> + * >>>> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH >>>> + * >>>> + * Author: Pradeep Jagadeesh >>>> + * >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or >>>> + * (at your option) any later version. >>>> + * >>>> + * See the COPYING file in the top-level directory for details. >>>> + * >>>> + */ >>>> + >>>> +#include "qemu/osdep.h" >>>> +#include "qemu/error-report.h" >>>> +#include "qemu-fsdev-throttle.h" >>>> +#include "qemu/iov.h" >>>> + >>>> +static void fsdev_throttle_read_timer_cb(void *opaque) >>>> +{ >>>> + FsThrottle *fst = opaque; >>>> + qemu_co_enter_next(&fst->throttled_reqs[false]); >>>> +} >>>> + >>>> +static void fsdev_throttle_write_timer_cb(void *opaque) >>>> +{ >>>> + FsThrottle *fst = opaque; >>>> + qemu_co_enter_next(&fst->throttled_reqs[true]); >>>> +} >>>> + >>>> +void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp) >>>> +{ >>>> + fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg = >>>> + qemu_opt_get_number(opts, "throttling.bps-total", 0); >>>> + fst->cfg.buckets[THROTTLE_BPS_READ].avg = >>>> + qemu_opt_get_number(opts, "throttling.bps-read", 0); >>>> + fst->cfg.buckets[THROTTLE_BPS_WRITE].avg = >>>> + qemu_opt_get_number(opts, "throttling.bps-write", 0); >>>> + fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg = >>>> + qemu_opt_get_number(opts, "throttling.iops-total", 0); >>>> + fst->cfg.buckets[THROTTLE_OPS_READ].avg = >>>> + qemu_opt_get_number(opts, "throttling.iops-read", 0); >>>> + fst->cfg.buckets[THROTTLE_OPS_WRITE].avg = >>>> + qemu_opt_get_number(opts, "throttling.iops-write", 0); >>>> + >>>> + fst->cfg.buckets[THROTTLE_BPS_TOTAL].max = >>>> + qemu_opt_get_number(opts, "throttling.bps-total-max", 0); >>>> + fst->cfg.buckets[THROTTLE_BPS_READ].max = >>>> + qemu_opt_get_number(opts, "throttling.bps-read-max", 0); >>>> + fst->cfg.buckets[THROTTLE_BPS_WRITE].max = >>>> + qemu_opt_get_number(opts, "throttling.bps-write-max", 0); >>>> + fst->cfg.buckets[THROTTLE_OPS_TOTAL].max = >>>> + qemu_opt_get_number(opts, "throttling.iops-total-max", 0); >>>> + fst->cfg.buckets[THROTTLE_OPS_READ].max = >>>> + qemu_opt_get_number(opts, "throttling.iops-read-max", 0); >>>> + fst->cfg.buckets[THROTTLE_OPS_WRITE].max = >>>> + qemu_opt_get_number(opts, "throttling.iops-write-max", 0); >>>> + >>>> + fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = >>>> + qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1); >>>> + fst->cfg.buckets[THROTTLE_BPS_READ].burst_length = >>>> + qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1); >>>> + fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length = >>>> + qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1); >>>> + fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = >>>> + qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1); >>>> + fst->cfg.buckets[THROTTLE_OPS_READ].burst_length = >>>> + qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1); >>>> + fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length = >>>> + qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1); >>>> + fst->cfg.op_size = >>>> + qemu_opt_get_number(opts, "throttling.iops-size", 0); >>>> + >>>> + throttle_is_valid(&fst->cfg, errp); >>>> +} >>>> + >>>> +void fsdev_throttle_init(FsThrottle *fst) >>>> +{ >>>> + if (throttle_enabled(&fst->cfg)) { >>>> + throttle_init(&fst->ts); >>>> + throttle_timers_init(&fst->tt, >>>> + qemu_get_aio_context(), >>>> + QEMU_CLOCK_REALTIME, >>>> + fsdev_throttle_read_timer_cb, >>>> + fsdev_throttle_write_timer_cb, >>>> + fst); >>>> + throttle_config(&fst->ts, &fst->tt, &fst->cfg); >>>> + qemu_co_queue_init(&fst->throttled_reqs[0]); >>>> + qemu_co_queue_init(&fst->throttled_reqs[1]); >>>> + } >>>> +} >>>> + >>>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, >>>> + struct iovec *iov, int iovcnt) >>>> +{ >>>> + if (throttle_enabled(&fst->cfg)) { >>>> + if (throttle_schedule_timer(&fst->ts, &fst->tt, is_write) || >>>> + !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { >>>> + qemu_co_queue_wait(&fst->throttled_reqs[is_write]); >>>> + } >>>> + >>>> + throttle_account(&fst->ts, is_write, iov_size(iov, iovcnt)); >>>> + >>>> + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write]) && >>>> + !throttle_schedule_timer(&fst->ts, &fst->tt, is_write)) { >>>> + qemu_co_queue_next(&fst->throttled_reqs[is_write]); >>>> + } >>>> + } >>>> +} >>>> + >>>> +void fsdev_throttle_cleanup(FsThrottle *fst) >>>> +{ >>>> + if (throttle_enabled(&fst->cfg)) { >>>> + throttle_timers_destroy(&fst->tt); >>>> + } >>>> +} >>>> diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h >>>> new file mode 100644 >>>> index 0000000..e418643 >>>> --- /dev/null >>>> +++ b/fsdev/qemu-fsdev-throttle.h >>>> @@ -0,0 +1,39 @@ >>>> +/* >>>> + * Fsdev Throttle >>>> + * >>>> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH >>>> + * >>>> + * Author: Pradeep Jagadeesh >>>> + * >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or >>>> + * (at your option) any later version. >>>> + * >>>> + * See the COPYING file in the top-level directory for details. >>>> + * >>>> + */ >>>> + >>>> +#ifndef _FSDEV_THROTTLE_H >>>> +#define _FSDEV_THROTTLE_H >>>> + >>>> +#include "block/aio.h" >>>> +#include "qemu/main-loop.h" >>>> +#include "qemu/coroutine.h" >>>> +#include "qapi/error.h" >>>> +#include "qemu/throttle.h" >>>> + >>>> +typedef struct FsThrottle { >>>> + ThrottleState ts; >>>> + ThrottleTimers tt; >>>> + ThrottleConfig cfg; >>>> + CoQueue throttled_reqs[2]; >>>> +} FsThrottle; >>>> + >>>> +void fsdev_throttle_parse_opts(QemuOpts *, FsThrottle *, Error **); >>>> + >>>> +void fsdev_throttle_init(FsThrottle *); >>>> + >>>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *, bool , >>>> + struct iovec *, int); >>>> + >>>> +void fsdev_throttle_cleanup(FsThrottle *); >>>> +#endif /* _FSDEV_THROTTLE_H */ >>>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >>>> index 845675e..828348d 100644 >>>> --- a/hw/9pfs/9p-local.c >>>> +++ b/hw/9pfs/9p-local.c >>>> @@ -1209,6 +1209,7 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) >>>> { >>>> const char *sec_model = qemu_opt_get(opts, "security_model"); >>>> const char *path = qemu_opt_get(opts, "path"); >>>> + Error *err = NULL; >>>> >>>> if (!sec_model) { >>>> error_report("Security model not specified, local fs needs security model"); >>>> @@ -1237,6 +1238,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) >>>> error_report("fsdev: No path specified"); >>>> return -1; >>>> } >>>> + >>>> + fsdev_throttle_parse_opts(opts, &fse->fst, &err); >>>> + if (err) { >>>> + error_reportf_err(err, "Throttle configuration is not valid: "); >>>> + return -1; >>>> + } >>>> + >>>> fse->path = g_strdup(path); >>>> >>>> return 0; >>>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c >>>> index e88cf25..8d46a91 100644 >>>> --- a/hw/9pfs/9p.c >>>> +++ b/hw/9pfs/9p.c >>>> @@ -3506,6 +3506,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp) >>>> error_setg(errp, "share path %s is not a directory", fse->path); >>>> goto out; >>>> } >>>> + >>>> + s->ctx.fst = &fse->fst; >>>> + fsdev_throttle_init(s->ctx.fst); >>>> + >>>> v9fs_path_free(&path); >>>> >>>> rc = 0; >>>> @@ -3520,6 +3524,7 @@ out: >>>> >>>> void v9fs_device_unrealize_common(V9fsState *s, Error **errp) >>>> { >>>> + 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 120e267..88791bc 100644 >>>> --- a/hw/9pfs/cofile.c >>>> +++ b/hw/9pfs/cofile.c >>>> @@ -247,6 +247,7 @@ int coroutine_fn v9fs_co_pwritev(V9fsPDU *pdu, V9fsFidState *fidp, >>>> if (v9fs_request_cancelled(pdu)) { >>>> return -EINTR; >>>> } >>>> + fsdev_co_throttle_request(s->ctx.fst, true, iov, iovcnt); >>>> v9fs_co_run_in_worker( >>>> { >>>> err = s->ops->pwritev(&s->ctx, &fidp->fs, iov, iovcnt, offset); >>>> @@ -266,6 +267,7 @@ int coroutine_fn v9fs_co_preadv(V9fsPDU *pdu, V9fsFidState *fidp, >>>> if (v9fs_request_cancelled(pdu)) { >>>> return -EINTR; >>>> } >>>> + fsdev_co_throttle_request(s->ctx.fst, false, iov, iovcnt); >>>> v9fs_co_run_in_worker( >>>> { >>>> err = s->ops->preadv(&s->ctx, &fidp->fs, iov, iovcnt, offset); >>> >> >