On Wed, 21 Sep 2016 12:22:25 +0200
Pradeep Jagadeesh <pradeep.jagade...@huawei.com> wrote:

> Hi Greg,
> 
> Thanks for having a look at patchset.
> See the replies below.
> 
> > On Fri, 16 Sep 2016 04:33:36 -0400
> > Pradeep Jagadeesh <pradeepkiruv...@gmail.com> wrote:
> >  
> >> Uses throttling APIs to limit I/O bandwidth and number of operations on the
> >> fsdev devices.
> >>
> >> Signed-off-by: Pradeep Jagadeesh <pradeep.jagade...@huawei.com>
> >> ---  
> >
> > Hi Pradeep,
> >
> > Please find some comments below.
> >  
> >>  fsdev/Makefile.objs         |   1 +
> >>  fsdev/file-op-9p.h          |   3 +
> >>  fsdev/qemu-fsdev-opts.c     |  76 ++++++++++++++++++
> >>  fsdev/qemu-fsdev-throttle.c | 191 
> >> ++++++++++++++++++++++++++++++++++++++++++++
> >>  fsdev/qemu-fsdev-throttle.h |  42 ++++++++++  
> >
> > Is the qemu- prefix really needed ?  
> Just followed the previous file naming convention,
> like qemu-fsdev-opts.c
> >  
> >>  hw/9pfs/9p-local.c          |  11 ++-
> >>  hw/9pfs/9p.c                |   7 ++
> >>  hw/9pfs/cofile.c            |   3 +
> >>  8 files changed, 332 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.
> >>
> >> 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..0adccd1
> >> --- /dev/null
> >> +++ b/fsdev/qemu-fsdev-throttle.c
> >> @@ -0,0 +1,191 @@
> >> +/*
> >> + * Fsdev Throttle
> >> + *
> >> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> >> + *
> >> + * Author: Pradeep Jagadeesh <pradeep.jagade...@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)
> >> +{
> >> +    if (fst->any_timer_armed[is_write]) {
> >> +        return true;
> >> +    } else {
> >> +        return throttle_schedule_timer(&fst->ts, &fst->tt, is_write);
> >> +    }
> >> +}
> >> +
> >> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool 
> >> is_write)
> >> +{
> >> +    bool must_wait = fsdev_throttle_check_for_wait(fst, is_write);  
> >
> > Let's add a newline here.  
> Done!
> >  
> >> +    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 fsdev_throttle_timer_cb(FsThrottle *fst, bool is_write)
> >> +{
> >> +    bool empty_queue;  
> >
> > Same here.  
> Done!
> >  
> >> +    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);
> >> +        fsdev_throttle_schedule_next_request(fst, is_write);
> >> +        qemu_mutex_unlock(&fst->lock);
> >> +    }
> >> +}
> >> +
> >> +bool fsdev_throttle_enabled(FsThrottle *fst)
> >> +{
> >> +    return fst->enabled;
> >> +}
> >> +
> >> +static void fsdev_iolimits_enable(FsThrottle *fst)
> >> +{
> >> +    fst->enabled = true;
> >> +}
> >> +
> >> +static void fsdev_iolimits_disable(FsThrottle *fst)
> >> +{
> >> +    fst->enabled = false;
> >> +}
> >> +  
> >
> > fsdev_throttle_configure_iolimits() is the only function that will ever
> > write to fst->enabled. The helpers seem to indicate the opposite. It is
> > better to open code in fsdev_throttle_configure_iolimits().  
> You mean just use  fst->enabled = true; inside the 
> fsdev_throttle_configure_iolimits().?
> If that is the case, I thought these helpers are useful in future when 
> we have qmp interfaces to enable,disable the throttle dynamically/run-time.

Hmm... in this case, something looks wrong.

If we start with a valid throttling config, then disable it with QMP and then
unrealize the device, we won't do the cleanup... fst->enabled looks like
a premature optimization which causes confusion actually.

I'd prefer fsdev to behave like blockdev, which doesn't have an enabled flag.

Please drop it completely. :)

> >  
> >> +static void fsdev_throttle_read_timer_cb(void *opaque)
> >> +{
> >> +    fsdev_throttle_timer_cb(opaque, false);
> >> +}
> >> +
> >> +static void fsdev_throttle_write_timer_cb(void *opaque)
> >> +{
> >> +    fsdev_throttle_timer_cb(opaque, 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_report("Throttle configuration is not valid");  
> >
> > throttle_is_valid() already populates err with a meaningful message and
> > you should use it:
> >
> >         error_reportf_err(err, "Throttle configuration is not valid: ");
> >  
> OK,fixed.
> >> +        return -1;
> >> +    }
> >> +    if (throttle_enabled(&fst->cfg)) {
> >> +        fst->aioctx = qemu_get_aio_context();
> >> +        if (!fst->aioctx) {
> >> +            error_report("Failed to create AIO Context");
> >> +            return -1;
> >> +        }  
> >
> > qemu_get_aio_context() returns qemu_aio_context which is initialized in
> > qemu_init_main_loop(). It is guarantied to be non NULL and if it is, we'd
> > better dump core.
> >  
> OK, Is there any specific dump api that exists in qemu?

QEMU uses g_assert() for elaborate checks but you really don't need to check
the return value of qemu_get_aio_context(). Just grep through the code to
be convinced :)

> >> +        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]);
> >> +        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);
> >> +        fsdev_iolimits_enable(fst);
> >> +   } else {
> >> +        fsdev_iolimits_disable(fst);
> >> +   }
> >> +   return 0;
> >> +}
> >> +
> >> +void fsdev_throttle_request(FsThrottle *fst, bool is_write, ssize_t bytes)
> >> +{
> >> +    if (fsdev_throttle_enabled(fst)) {
> >> +        qemu_mutex_lock(&fst->lock);
> >> +        bool must_wait = fsdev_throttle_check_for_wait(fst, is_write);  
> >
> > Even if the QEMU code is supposed to follow C99, I think it is prettier to
> > declare variables at the beginning of the code block.
> >
> >            bool must_wait;  
> OK,fixed.
> >
> >            qemu_mutex_lock(&fst->lock);
> >            must_wait = fsdev_throttle_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);
> >> +       fsdev_throttle_schedule_next_request(fst, is_write);
> >> +       qemu_mutex_unlock(&fst->lock);
> >> +    }
> >> +}
> >> +
> >> +void fsdev_throttle_cleanup(FsThrottle *fst)
> >> +{
> >> +    throttle_timers_destroy(&fst->tt);
> >> +    qemu_mutex_destroy(&fst->lock);
> >> +}
> >> diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h
> >> new file mode 100644
> >> index 0000000..73dd367
> >> --- /dev/null
> >> +++ b/fsdev/qemu-fsdev-throttle.h
> >> @@ -0,0 +1,42 @@
> >> +/*
> >> + * Fsdev Throttle
> >> + *
> >> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH
> >> + *
> >> + * Author: Pradeep Jagadeesh <pradeep.jagade...@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;
> >> +    bool enabled;
> >> +    CoQueue      throttled_reqs[2];
> >> +    unsigned     pending_reqs[2];
> >> +    bool any_timer_armed[2];
> >> +    QemuMutex lock;
> >> +} FsThrottle;
> >> +
> >> +bool fsdev_throttle_enabled(FsThrottle *);
> >> +
> >> +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *);
> >> +
> >> +void fsdev_throttle_request(FsThrottle *, bool , ssize_t);
> >> +
> >> +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..b1009fc 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
> >> @@ -1213,6 +1213,8 @@ 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");
> >>
> >> +    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 +1242,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, 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..3748ba9 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 */  
> >
> > I'm still thinking this comment paraphrases the code :)  
> I missed it!:)
> >  
> >> +    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 (fsdev_throttle_enabled(s->ctx.fst)) {
> >> +        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..65897d5 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)
> >> @@ -242,6 +243,7 @@ int v9fs_co_pwritev(V9fsPDU *pdu, V9fsFidState *fidp,
> >>      int err;
> >>      V9fsState *s = pdu->s;
> >>
> >> +    fsdev_throttle_request(s->ctx.fst, true, iov->iov_len);  
> >
> > I think it would make more sense to move this after the cancellation point. 
> >  
> Done!
> >  
> >>      if (v9fs_request_cancelled(pdu)) {
> >>          return -EINTR;
> >>      }
> >> @@ -261,6 +263,7 @@ int v9fs_co_preadv(V9fsPDU *pdu, V9fsFidState *fidp,
> >>      int err;
> >>      V9fsState *s = pdu->s;
> >>
> >> +    fsdev_throttle_request(s->ctx.fst, false, iov->iov_len);  
> >
> > Same here.
> >  
> Done!
> >>      if (v9fs_request_cancelled(pdu)) {
> >>          return -EINTR;
> >>      }  
> >
> > Cheers.
> >
> > --
> > Greg
> >  
> Regards,
> Pradeep
> 


Reply via email to