Re: [Qemu-devel] [PATCH V11 1/1] fsdev: add IO throttle support to fsdev devices

[Greg Kurz <groug@kaod.org>]

On Tue, 15 Nov 2016 10:13:59 +0100
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:

> On 11/14/2016 6:57 PM, Greg Kurz wrote:
> > On Mon, 14 Nov 2016 10:03:40 +0100
> > Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:
> >  
> >> On 11/12/2016 3:13 PM, Greg Kurz wrote:  
> >>> On Fri, 11 Nov 2016 03:54:27 -0500
> >>> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> 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 <pradeep.jagadeesh@huawei.com>
> >>>> Reviewed-by: Alberto Garcia" <berto@igalia.com>
> >>>> ---  
> >>>
> >>> 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 <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 "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);  
> >>>  
> >>  
> >  
> 
>