[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V11 1/1] fsdev: add IO throttle support to fsdev
|
From: |
Greg Kurz |
|
Subject: |
Re: [Qemu-devel] [PATCH V11 1/1] fsdev: add IO throttle support to fsdev devices |
|
Date: |
Tue, 15 Nov 2016 10:58:23 +0100 |
On Tue, 15 Nov 2016 10:13:59 +0100
Pradeep Jagadeesh <address@hidden> wrote:
> On 11/14/2016 6:57 PM, Greg Kurz wrote:
> > On Mon, 14 Nov 2016 10:03:40 +0100
> > Pradeep Jagadeesh <address@hidden> wrote:
> >
> >> On 11/12/2016 3:13 PM, Greg Kurz wrote:
> >>> On Fri, 11 Nov 2016 03:54:27 -0500
> >>> Pradeep Jagadeesh <address@hidden> wrote:
> >>>
> >>>> Uses throttling APIs to limit I/O bandwidth and number of operations on
> >>>> the
> >>>> devices which use 9p-local driver.
> >>>>
> >>>> Signed-off-by: Pradeep Jagadeesh <address@hidden>
> >>>> Reviewed-by: Alberto Garcia" <address@hidden>
> >>>> ---
> >>>
> >>> 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.
>
Ok. It looks like we can factor two things:
- the set of common throttle options in the QemuOptsList (I already have patch
for that I can send to be part of your patchset)
- the initialization of the throttle config
BTW, looking at extract_common_blockdev_options(), I have the impression
that a call to throttle_config_init() is missing...
> Cheers,
> Pradeep
[...]
> >>>> +void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error
> >>>> **errp)
> >>>> +{
... here.
> >>>> + fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> >>>> + qemu_opt_get_number(opts, "throttling.bps-total", 0);
> >>>> + fst->cfg.buckets[THROTTLE_BPS_READ].avg =
> >>>> + qemu_opt_get_number(opts, "throttling.bps-read", 0);
> >>>> + fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> >>>> + qemu_opt_get_number(opts, "throttling.bps-write", 0);
> >>>> + fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> >>>> + qemu_opt_get_number(opts, "throttling.iops-total", 0);
> >>>> + fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> >>>> + qemu_opt_get_number(opts, "throttling.iops-read", 0);
> >>>> + fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> >>>> + qemu_opt_get_number(opts, "throttling.iops-write", 0);
> >>>> +
> >>>> + fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> >>>> + qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
> >>>> + fst->cfg.buckets[THROTTLE_BPS_READ].max =
> >>>> + qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
> >>>> + fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> >>>> + qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
> >>>> + fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> >>>> + qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
> >>>> + fst->cfg.buckets[THROTTLE_OPS_READ].max =
> >>>> + qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
> >>>> + fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> >>>> + qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
> >>>> +
> >>>> + fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
> >>>> + qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
> >>>> + fst->cfg.buckets[THROTTLE_BPS_READ].burst_length =
> >>>> + qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
> >>>> + fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
> >>>> + qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
> >>>> + fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
> >>>> + qemu_opt_get_number(opts, "throttling.iops-total-max-length",
> >>>> 1);
> >>>> + fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
> >>>> + qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
> >>>> + fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
> >>>> + qemu_opt_get_number(opts, "throttling.iops-write-max-length",
> >>>> 1);
> >>>> + fst->cfg.op_size =
> >>>> + qemu_opt_get_number(opts, "throttling.iops-size", 0);
> >>>> +
> >>>> + throttle_is_valid(&fst->cfg, errp);
> >>>> +}
> >>>> +
> >>>> +void fsdev_throttle_init(FsThrottle *fst)
> >>>> +{
> >>>> + if (throttle_enabled(&fst->cfg)) {
> >>>> + throttle_init(&fst->ts);
> >>>> + throttle_timers_init(&fst->tt,
> >>>> + qemu_get_aio_context(),
> >>>> + QEMU_CLOCK_REALTIME,
> >>>> + fsdev_throttle_read_timer_cb,
> >>>> + fsdev_throttle_write_timer_cb,
> >>>> + fst);
> >>>> + throttle_config(&fst->ts, &fst->tt, &fst->cfg);
> >>>> + qemu_co_queue_init(&fst->throttled_reqs[0]);
> >>>> + qemu_co_queue_init(&fst->throttled_reqs[1]);
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool
> >>>> is_write,
> >>>> + struct iovec *iov, int
> >>>> iovcnt)
> >>>> +{
> >>>> + if (throttle_enabled(&fst->cfg)) {
> >>>> + if (throttle_schedule_timer(&fst->ts, &fst->tt, is_write) ||
> >>>> + !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {
> >>>> + qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> >>>> + }
> >>>> +
> >>>> + throttle_account(&fst->ts, is_write, iov_size(iov, iovcnt));
> >>>> +
> >>>> + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write]) &&
> >>>> + !throttle_schedule_timer(&fst->ts, &fst->tt, is_write)) {
> >>>> + qemu_co_queue_next(&fst->throttled_reqs[is_write]);
> >>>> + }
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +void fsdev_throttle_cleanup(FsThrottle *fst)
> >>>> +{
> >>>> + if (throttle_enabled(&fst->cfg)) {
> >>>> + throttle_timers_destroy(&fst->tt);
> >>>> + }
> >>>> +}
> >>>> diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h
> >>>> new file mode 100644
> >>>> index 0000000..e418643
> >>>> --- /dev/null
> >>>> +++ b/fsdev/qemu-fsdev-throttle.h
> >>>> @@ -0,0 +1,39 @@
> >>>> +/*
> >>>> + * Fsdev Throttle
> >>>> + *
> >>>> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> >>>> + *
> >>>> + * Author: Pradeep Jagadeesh <address@hidden>
> >>>> + *
> >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
> >>>> + * (at your option) any later version.
> >>>> + *
> >>>> + * See the COPYING file in the top-level directory for details.
> >>>> + *
> >>>> + */
> >>>> +
> >>>> +#ifndef _FSDEV_THROTTLE_H
> >>>> +#define _FSDEV_THROTTLE_H
> >>>> +
> >>>> +#include "block/aio.h"
> >>>> +#include "qemu/main-loop.h"
> >>>> +#include "qemu/coroutine.h"
> >>>> +#include "qapi/error.h"
> >>>> +#include "qemu/throttle.h"
> >>>> +
> >>>> +typedef struct FsThrottle {
> >>>> + ThrottleState ts;
> >>>> + ThrottleTimers tt;
> >>>> + ThrottleConfig cfg;
> >>>> + CoQueue throttled_reqs[2];
> >>>> +} FsThrottle;
> >>>> +
> >>>> +void fsdev_throttle_parse_opts(QemuOpts *, FsThrottle *, Error **);
> >>>> +
> >>>> +void fsdev_throttle_init(FsThrottle *);
> >>>> +
> >>>> +void coroutine_fn fsdev_co_throttle_request(FsThrottle *, bool ,
> >>>> + struct iovec *, int);
> >>>> +
> >>>> +void fsdev_throttle_cleanup(FsThrottle *);
> >>>> +#endif /* _FSDEV_THROTTLE_H */
> >>>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> >>>> index 845675e..828348d 100644
> >>>> --- a/hw/9pfs/9p-local.c
> >>>> +++ b/hw/9pfs/9p-local.c
> >>>> @@ -1209,6 +1209,7 @@ static int local_parse_opts(QemuOpts *opts, struct
> >>>> FsDriverEntry *fse)
> >>>> {
> >>>> const char *sec_model = qemu_opt_get(opts, "security_model");
> >>>> const char *path = qemu_opt_get(opts, "path");
> >>>> + Error *err = NULL;
> >>>>
> >>>> if (!sec_model) {
> >>>> error_report("Security model not specified, local fs needs
> >>>> security model");
> >>>> @@ -1237,6 +1238,13 @@ static int local_parse_opts(QemuOpts *opts,
> >>>> struct FsDriverEntry *fse)
> >>>> error_report("fsdev: No path specified");
> >>>> return -1;
> >>>> }
> >>>> +
> >>>> + fsdev_throttle_parse_opts(opts, &fse->fst, &err);
> >>>> + if (err) {
> >>>> + error_reportf_err(err, "Throttle configuration is not valid: ");
> >>>> + return -1;
> >>>> + }
> >>>> +
> >>>> fse->path = g_strdup(path);
> >>>>
> >>>> return 0;
> >>>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> >>>> index 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);
> >>>
> >>
> >
>
>