qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
To: Greg Kurz <groug@kaod.org>,
	Pradeep Jagadeesh <pradeepkiruvale@gmail.com>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>,
	Alberto Garcia <berto@igalia.com>,
	qemu-devel@nongnu.org,
	Claudio Fontana <claudio.fontana@huawei.com>
Subject: Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices
Date: Wed, 21 Sep 2016 12:22:25 +0200	[thread overview]
Message-ID: <33d1fc4d-8c04-ada5-4f5a-a4396678e4d4@huawei.com> (raw)
In-Reply-To: <20160921001500.0de87dd0@bahia.lab.toulouse-stg.fr.ibm.com>

Hi Greg,

Thanks for having a look at patchset.
See the replies below.

> On Fri, 16 Sep 2016 04:33:36 -0400
> Pradeep Jagadeesh <pradeepkiruvale@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.jagadeesh@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.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.
>> + *
>> + */
>> +
>> +#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.
>
>> +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?
>> +        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.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 "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	other threads:[~2016-09-21 10:23 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-16  8:33 [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices Pradeep Jagadeesh
2016-09-19 15:41 ` Alberto Garcia
2016-09-20  9:02   ` Pradeep Jagadeesh
2016-09-20 21:56     ` Greg Kurz
2016-09-22 11:29   ` Pradeep Jagadeesh
2016-10-17  9:19   ` Pradeep Jagadeesh
2016-10-18 16:19     ` Alberto Garcia
2016-10-19  8:57       ` Pradeep Jagadeesh
2016-10-19 16:29       ` Pradeep Jagadeesh
2016-10-20 12:50         ` Alberto Garcia
2016-10-20 13:32           ` Greg Kurz
2016-10-20 13:49             ` Pradeep Jagadeesh
2016-10-20 13:48           ` Pradeep Jagadeesh
2016-09-20 22:15 ` Greg Kurz
2016-09-21 10:22   ` Pradeep Jagadeesh [this message]
2016-09-21 12:10     ` Greg Kurz
2016-09-22 11:46       ` Pradeep Jagadeesh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=33d1fc4d-8c04-ada5-4f5a-a4396678e4d4@huawei.com \
    --to=pradeep.jagadeesh@huawei.com \
    --cc=aneesh.kumar@linux.vnet.ibm.com \
    --cc=berto@igalia.com \
    --cc=claudio.fontana@huawei.com \
    --cc=groug@kaod.org \
    --cc=pradeepkiruvale@gmail.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).