From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54554) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byyCI-0004h3-HR for qemu-devel@nongnu.org; Tue, 25 Oct 2016 05:39:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byyCF-0004Tl-Cg for qemu-devel@nongnu.org; Tue, 25 Oct 2016 05:39:06 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:4374) by eggs.gnu.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.71) (envelope-from ) id 1byyCF-0004Ow-1D for qemu-devel@nongnu.org; Tue, 25 Oct 2016 05:39:03 -0400 References: <1477148842-44794-1-git-send-email-pradeep.jagadeesh@huawei.com> From: Pradeep Jagadeesh Message-ID: <58b4b619-24c2-8daa-b188-b0482494f106@huawei.com> Date: Tue, 25 Oct 2016 11:37:48 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [V7 1/1] fsdev: add IO throttle support to fsdev devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , Pradeep Jagadeesh , "Aneesh Kumar K.V" , Greg Kurz Cc: qemu-devel@nongnu.org, Claudio Fontana 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 >