Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver

[Claudio Fontana]

Hi Pradeep,

two comment below:

On 09.09.2016 11:10, 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 <address@hidden>
> ---
>  fsdev/file-op-9p.h      |   3 +
>  fsdev/qemu-fsdev-opts.c |  52 +++++++++++++
>  hw/9pfs/9p-local.c      |  18 ++++-
>  hw/9pfs/9p-throttle.c   | 201 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/9pfs/9p-throttle.h   |  46 +++++++++++
>  hw/9pfs/9p.c            |   7 ++
>  hw/9pfs/Makefile.objs   |   1 +
>  7 files changed, 326 insertions(+), 2 deletions(-)
>  create mode 100644 hw/9pfs/9p-throttle.c
>  create mode 100644 hw/9pfs/9p-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
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 6db9fea..e86b91a 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 "hw/9pfs/9p-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..2774855 100644
> --- a/fsdev/qemu-fsdev-opts.c
> +++ b/fsdev/qemu-fsdev-opts.c
> @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
>          }, {
>              .name = "sock_fd",
>              .type = QEMU_OPT_NUMBER,
> +        }, {
> +            .name = "bps",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit total bytes per second",
> +        }, {
> +            .name = "bps_rd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit read bytes per second",
> +        }, {
> +            .name = "bps_wr",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit write bytes per second",
> +        }, {
> +            .name = "iops",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit total io operations per second",
> +        }, {
> +            .name = "iops_rd",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit read operations per second ",
> +        }, {
> +            .name = "iops_wr",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "limit write operations per second",
> +        }, {
> +            .name = "bps_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes burst",
> +        }, {
> +            .name = "bps_rd_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes read burst",
> +        }, {
> +            .name = "bps_wr_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum bytes write burst",
> +        }, {
> +            .name = "iops_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations burst",
> +        }, {
> +            .name = "iops_rd_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations read burst",
> +        }, {
> +            .name = "iops_wr_max",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "maximum io operations write burst",
> +        }, {
> +            .name = "iops_size",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "io iops-size",
>          },
>  
>          { /*End of list */ }
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 3f271fc..49c2819 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, 
> V9fsFidOpenState *fs,
>                              const struct iovec *iov,
>                              int iovcnt, off_t offset)
>  {
> +    throttle9p_request(ctx->fst, false, iov->iov_len);
> +
>  #ifdef CONFIG_PREADV
>      return preadv(fs->fd, iov, iovcnt, offset);
>  #else
> @@ -436,8 +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, 
> V9fsFidOpenState *fs,
>                               const struct iovec *iov,
>                               int iovcnt, off_t offset)
>  {
> -    ssize_t ret
> -;
> +    ssize_t ret;
> +
> +    throttle9p_request(ctx->fst, true, iov->iov_len);
> +
>  #ifdef CONFIG_PREADV
>      ret = pwritev(fs->fd, iov, iovcnt, offset);
>  #else
> @@ -1213,6 +1217,9 @@ 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");
>  
> +    /* get the throttle structure */
> +    FsThrottle *fst = &fse->fst;
> +
>      if (!sec_model) {
>          error_report("Security model not specified, local fs needs security 
> model");
>          error_printf("valid options are:"
> @@ -1240,6 +1247,13 @@ static int local_parse_opts(QemuOpts *opts, struct 
> FsDriverEntry *fse)
>          error_report("fsdev: No path specified");
>          return -1;
>      }
> +
> +    throttle9p_enable_io_limits(opts, fst);
> +
> +    if (throttle9p_get_io_limits_state(fst)) {
> +        throttle9p_configure_iolimits(opts, fst);
> +    }
> +
>      fse->path = g_strdup(path);
>  
>      return 0;
> diff --git a/hw/9pfs/9p-throttle.c b/hw/9pfs/9p-throttle.c
> new file mode 100644
> index 0000000..f2a7ba5
> --- /dev/null
> +++ b/hw/9pfs/9p-throttle.c
> @@ -0,0 +1,201 @@
> +/*
> + * 9P Throttle
> + *
> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Pradeep Jagadeesh <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.
> + *
> + * See the COPYING file in the top-level directory for details.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "fsdev/qemu-fsdev.h"   /* local_ops */
> +#include "qemu/cutils.h"
> +#include "qemu/error-report.h"
> +#include <libgen.h>
> +#include <linux/fs.h>
> +#include <sys/ioctl.h>
> +#include <string.h>
> +#include "fsdev/file-op-9p.h"
> +#include "9p-throttle.h"
> +
> +void throttle9p_enable_io_limits(QemuOpts *opts, FsThrottle *fst)
> +{
> +    const float bps = qemu_opt_get_number(opts, "bps", 0);
> +    const float iops = qemu_opt_get_number(opts, "iops", 0);
> +    const float rdbps = qemu_opt_get_number(opts, "bps_rd", 0);
> +    const float wrbps = qemu_opt_get_number(opts, "bps_wr", 0);
> +    const float rdops = qemu_opt_get_number(opts, "iops_rd", 0);
> +    const float wrops = qemu_opt_get_number(opts, "iops_wr", 0);
> +
> +    if (bps > 0 || iops > 0 || rdbps > 0 ||
> +        wrbps > 0 || rdops > 0 || wrops > 0) {
> +        fst->io_limits_enabled = true;
> +    } else {
> +        fst->io_limits_enabled = false;
> +    }
> +}
> +
> +static bool throttle9p_check_for_wait(FsThrottle *fst, bool is_write)
> +{
> +    if (fst->any_timer_armed[is_write]) {
> +        return true;
> +    } else {
> +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> +    }
> +}
> +
> +static void throttle9p_schedule_next_request(FsThrottle *fst, bool is_write)
> +{
> +    bool must_wait = throttle9p_check_for_wait(fst, is_write);
> +    if (!fst->pending_reqs[is_write]) {
> +        return;
> +    }
> +    if (!must_wait) {
> +        if (qemu_in_coroutine() &&
> +            qemu_co_queue_next(&fst->throttled_reqs[is_write])) {
> +            ;
> +       } else {
> +           int64_t now = qemu_clock_get_ns(fst->tt.clock_type);
> +           timer_mod(fst->tt.timers[is_write], now + 1);
> +           fst->any_timer_armed[is_write] = true;
> +       }
> +   }
> +}
> +
> +static void throttle9p_timer_cb(FsThrottle *fst, bool is_write)
> +{
> +    bool empty_queue;
> +    qemu_mutex_lock(&fst->lock);
> +    fst->any_timer_armed[is_write] = false;
> +    qemu_mutex_unlock(&fst->lock);
> +    empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]);
> +    if (empty_queue) {
> +        qemu_mutex_lock(&fst->lock);
> +        throttle9p_schedule_next_request(fst, is_write);
> +        qemu_mutex_unlock(&fst->lock);
> +    }
> +}
> +
> +
> +bool throttle9p_get_io_limits_state(FsThrottle *fst)
> +{
> +
> +    return fst->io_limits_enabled;
> +}
> +
> +static void throttle9p_read_timer_cb(void *opaque)
> +{
> +    throttle9p_timer_cb(opaque, false);
> +}
> +
> +static void throttle9p_write_timer_cb(void *opaque)
> +{
> +    throttle9p_timer_cb(opaque, true);
> +}
> +
> +void throttle9p_configure_iolimits(QemuOpts *opts, FsThrottle *fst)
> +{
> +    memset(&fst->ts, 1, sizeof(fst->ts));
> +    memset(&fst->tt, 1, sizeof(fst->tt));
> +    memset(&fst->cfg, 0, sizeof(fst->cfg));

why do you memset these structs to those values?
->ts and ->tt to all 01h bytes and cfg to 00h?

There are already initialization functions for those which you call below,
throttle_init, throttle_timers_init and throttle_config_init,
so I think these memsets should be removed.


> +    fst->aioctx = qemu_get_aio_context();
> +
> +    if (!fst->aioctx) {
> +        error_report("Failed to create AIO Context");
> +        exit(1);
> +    }
> +    throttle_init(&fst->ts);
> +    throttle_timers_init(&fst->tt,
> +                         fst->aioctx,
> +                         QEMU_CLOCK_REALTIME,
> +                         throttle9p_read_timer_cb,
> +                         throttle9p_write_timer_cb,
> +                         fst);
> +    throttle_config_init(&fst->cfg);
> +    g_assert(throttle_is_valid(&fst->cfg, NULL));
> +
> +    qemu_co_queue_init(&fst->throttled_reqs[0]);
> +    qemu_co_queue_init(&fst->throttled_reqs[1]);
> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
> +          qemu_opt_get_number(opts, "bps", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
> +          qemu_opt_get_number(opts, "bps_rd", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
> +          qemu_opt_get_number(opts, "bps_wr", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
> +          qemu_opt_get_number(opts, "iops", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
> +          qemu_opt_get_number(opts, "iops_rd", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
> +          qemu_opt_get_number(opts, "iops_wr", 0);
> +
> +    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
> +          qemu_opt_get_number(opts, "bps_max", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
> +          qemu_opt_get_number(opts, "bps_rd_max", 0);
> +    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
> +          qemu_opt_get_number(opts, "bps_wr_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
> +          qemu_opt_get_number(opts, "iops_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_READ].max =
> +          qemu_opt_get_number(opts, "iops_rd_max", 0);
> +    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
> +          qemu_opt_get_number(opts, "iops_wr_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, "iops_size", 0);
> +
> +    throttle_config(&fst->ts, &fst->tt, &fst->cfg);
> +    if (!throttle_is_valid(&fst->cfg, NULL)) {
> +        return;

What is the expected state if we bail at this point?
Is QEMU expected to work anyway in this case? Ignoring the limits?
 
The data structures initialized below will instead not be, including the 
fst->lock.

Ciao,

Claudio

> +    }
> +
> +    g_assert(fst->tt.timers[0]);
> +    g_assert(fst->tt.timers[1]);
> +    fst->pending_reqs[0] = 0;
> +    fst->pending_reqs[1] = 0;
> +    fst->any_timer_armed[0] = false;
> +    fst->any_timer_armed[1] = false;
> +    qemu_mutex_init(&fst->lock);
> +}
> +
> +void throttle9p_request(FsThrottle *fst, bool is_write, ssize_t bytes)
> +{
> +    if (fst->io_limits_enabled) {
> +        qemu_mutex_lock(&fst->lock);
> +        bool must_wait = throttle9p_check_for_wait(fst, is_write);
> +        if (must_wait || fst->pending_reqs[is_write]) {
> +            fst->pending_reqs[is_write]++;
> +            qemu_mutex_unlock(&fst->lock);
> +            qemu_co_queue_wait(&fst->throttled_reqs[is_write]);
> +            qemu_mutex_lock(&fst->lock);
> +            fst->pending_reqs[is_write]--;
> +       }
> +       throttle_account(&fst->ts, is_write, bytes);
> +       throttle9p_schedule_next_request(fst, is_write);
> +       qemu_mutex_unlock(&fst->lock);
> +    }
> +}
> +
> +void throttle9p_cleanup(FsThrottle *fst)
> +{
> +    throttle_timers_destroy(&fst->tt);
> +    qemu_mutex_destroy(&fst->lock);
> +}
> diff --git a/hw/9pfs/9p-throttle.h b/hw/9pfs/9p-throttle.h
> new file mode 100644
> index 0000000..0f7551d
> --- /dev/null
> +++ b/hw/9pfs/9p-throttle.h
> @@ -0,0 +1,46 @@
> +/*
> + * 9P 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 _9P_THROTTLE_H
> +#define _9P_THROTTLE_H
> +
> +#include <stdbool.h>
> +#include <stdint.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;
> +    bool io_limits_enabled;
> +    CoQueue      throttled_reqs[2];
> +    unsigned     pending_reqs[2];
> +    bool any_timer_armed[2];
> +    QemuMutex lock;
> +} FsThrottle;
> +
> +void throttle9p_enable_io_limits(QemuOpts *, FsThrottle *);
> +
> +bool throttle9p_get_io_limits_state(FsThrottle *);
> +
> +void throttle9p_configure_iolimits(QemuOpts *, FsThrottle *);
> +
> +void throttle9p_request(FsThrottle *, bool , ssize_t);
> +
> +void throttle9p_cleanup(FsThrottle *);
> +#endif /* _9P_THROTTLE_H */
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index dfe293d..7be926a 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error 
> **errp)
>          error_setg(errp, "share path %s is not a directory", fse->path);
>          goto out;
>      }
> +
> +    /* Throttle structure initialization */
> +    s->ctx.fst = &fse->fst;
> +
>      v9fs_path_free(&path);
>  
>      rc = 0;
> @@ -3504,6 +3508,9 @@ out:
>  
>  void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
>  {
> +    if (throttle9p_get_io_limits_state(s->ctx.fst)) {
> +        throttle9p_cleanup(s->ctx.fst);
> +    }
>      g_free(s->ctx.fs_root);
>      g_free(s->tag);
>  }
> diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
> index da0ae0c..07523f1 100644
> --- a/hw/9pfs/Makefile.objs
> +++ b/hw/9pfs/Makefile.objs
> @@ -5,5 +5,6 @@ common-obj-y += coth.o cofs.o codir.o cofile.o
>  common-obj-y += coxattr.o 9p-synth.o
>  common-obj-$(CONFIG_OPEN_BY_HANDLE) +=  9p-handle.o
>  common-obj-y += 9p-proxy.o
> +common-obj-y += 9p-throttle.o
>  
>  obj-y += virtio-9p-device.o
>