qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devi


From: Pradeep Jagadeesh
Subject: Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices
Date: Tue, 20 Sep 2016 11:02:07 +0200
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0

Hi Alberto,

Thanks for having look at the patch.
My replies are inline.
On Fri 16 Sep 2016 10:33:36 AM CEST, Pradeep Jagadeesh wrote:

Hi,

first of all, sorry for the late reply! Here are my comments:
No problem!

--- 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-size",
+            .type = QEMU_OPT_NUMBER,
+            .help = "when limiting by iops max size of an I/O in bytes",

It would be nice if we could factor these so we don't have to have them
twice in the code. I think it should be doable with qemu_opts_append().

It can be done later in a separate patch if you prefer.
OK,as of now I have just copied. We can rework and remove the redundant code later.

+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;

You based your implementation on block/throttle-groups.c. I think yours
can be made simpler because one of the problems with mine is that it
needs to support multiple parallel I/O operations on the same throttling
group, and that's why the locking rules are more complex. With a single
user per ThrottleConfig this is not necessary.

Have you checked if you really need them? My impression is that you
might be able to get rid of the 'lock', 'any_timer_armed' and
'pending_reqs' fields.

Please check commit 76f4afb40fa076ed23fe0ab42c7a768ddb71123f to see how
was the transition from single-drive throttling to group throttling,
specifically the bdrv_io_limits_intercept() function. You will see that
it was simpler.
OK,I am not sure,I will have to try. I will look into this and let you know.

@@ -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;
+

This could go to a separate patch, but I'm fine if you include it in
this one.
I have already included as Greg suggested.

@@ -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;
+

I'd remove the empty line between the declarations of 'path' and 'fst',
however...
OK

     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;
+    }

...you don't need to declare fst here, you could also pass &fse->fst
directly.
OK

-Pradeep


Berto





reply via email to

[Prev in Thread] Current Thread [Next in Thread]