qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [V7 1/1] fsdev: add IO throttle support to fsdev device


From: Pradeep Jagadeesh
Subject: Re: [Qemu-devel] [V7 1/1] fsdev: add IO throttle support to fsdev devices
Date: Tue, 25 Oct 2016 11:37:48 +0200
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0

On 10/24/2016 3:00 PM, Alberto Garcia wrote:
On Sat 22 Oct 2016 05:07:22 PM CEST, Pradeep Jagadeesh wrote:

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.

Ok, I think this is almost ready. There's only a few corrections and
style fixes left.

Hi Alberto, thanks for having a look at the patch.
I will send the patch early next week as I am out of office.

-Pradeep


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

(Optional) You can put these two in the same line:

common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o

+++ b/fsdev/qemu-fsdev-throttle.c

In this file there's many lines that are incorrectly indented.
Indentation should be four spaces. The following functions have lines
that need to be corrected:

fsdev_throttle_check_for_wait
fsdev_throttle_schedule_next_request
fsdev_parse_io_limit_opts
fsdev_throttle_configure_iolimits
fsdev_co_throttle_request

Check also the other changes in your patch in case there are more lines
with wrong indentation.

+static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write)

"Check for wait" doesn't sound like correct English to me. How about
simply fsdev_throttle_schedule_timer()? Or fsdev_throttle_must_wait()

+static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool 
is_write)
+{
+   if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {

Here's an example of a line with the wrong indentation. It uses three
spaces, it should use four.

+       qemu_co_queue_next(&fst->throttled_reqs[is_write]);

And this one uses seven, it should use eight. There are more cases like
this, I only highlighted a couple of them.

+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_reportf_err(err, "Throttle configuration is not valid: ");
+        return -1;
+    }
+    if (throttle_enabled(&fst->cfg)) {
+        g_assert((fst->aioctx = qemu_get_aio_context()));
+        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]);
+   }
+   return 0;
+}

Here you're parsing the command-line options and initializing the
throtting structures in local_parse_opts(). Then you're cleaning up in
v9fs_device_unrealize_common().

I have two questions:

a) You could split fsdev_throttle_configure_iolimits() in two:
   1) fsdev_throttle_parse_opts(), called from local_parse_opts()
   2) fsdev_throttle_init(), called from v9fs_device_realize_common().

b) Why are you only doing this for the "local" fsdriver, and not for
   "handle" or "proxy"? Isn't it possible to do throttling with those
   as well?

+static int get_num_bytes(struct iovec *iov, int iovcnt)
+{
+    int i;
+    unsigned int bytes = 0;

You changed 'bytes' from int to unsigned as I suggested, but you still
return an int, and in fsdev_co_throttle_request() you also get an
int. Please use unsigned in all these cases.

+int  fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *);
+
+void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write,
+                            struct iovec *iov, int iovcnt);

Please align this line with the parameters of the first line.

Also, why do you include the name of the parameters in this function
prototype but not in the other two?

@@ -1240,6 +1240,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, &fse->fst) < 0) {
+        return -1;
+    }

If you create fsdev_throttle_parse_opts() as I suggest, I'd rather print
the error message here as the other error messages in this function.

I think I haven't forgotten anything, everything else looks fine. I'd
still like to have all the throtting parameters merged so we don't need
to duplicate them, but that can be done in the future (I can even take
care of it if I find a bit of time).

Thanks for the patch!

Berto





reply via email to

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