Hi Alberto,

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:

--- 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.

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

I cleaned up the code as you suggested, its working fine.

-Pradeep


@@ -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.

@@ -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...

     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.

Berto



Reply via email to