From: Greg Kurz <groug@kaod.org>
To: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
Cc: Pradeep Jagadeesh <pradeepkiruvale@gmail.com>,
Alberto Garcia <berto@igalia.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
Date: Mon, 23 Jan 2017 15:48:56 +0100 [thread overview]
Message-ID: <20170123154856.6dd25aed@bahia.lan> (raw)
In-Reply-To: <48B294AA6E8CDE42B0CA548318C1256301A3DF5F@lhreml501-mbb>
On Mon, 23 Jan 2017 14:02:33 +0000
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:
> -----Original Message-----
> From: Greg Kurz [mailto:groug@kaod.org]
> Sent: Monday, January 23, 2017 11:28 AM
> To: Pradeep Jagadeesh
> Cc: Pradeep Jagadeesh; Alberto Garcia; qemu-devel@nongnu.org
> Subject: Re: [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
>
> On Mon, 23 Jan 2017 10:58:19 +0100
> Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote:
>
> > On 1/23/2017 10:47 AM, Greg Kurz wrote:
> > > On Mon, 23 Jan 2017 04:19:55 -0500
> > > Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:
> > >
> > >> This will allow other subsystems (i.e. fsdev) to implement
> > >> throttling without duplicating the command line options.
> > >>
> > >> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> > >> ---
> > >
> > > This patch depends on your other patch "[PATCH V12] fsdev: add IO
> > > throttle support to fsdev devices", which I haven't reviewed yet
> > > BTW. Both patches should really be sent in a single series.
> > OK
> > >
> > > Please do the following:
> > > - fix the changelog in the other patch
> > > - write a cover letter to provide some context on this feature: why is it
> > > needed ? why the QMP part has been left aside ? what are the remaining
> > > efforts to make that feature fully implemented and useful ?
> > This cover letter should be part of the .patch file right?
>
> No. It is a separate mail with some introductory text to describe what the series tries to achieve and any other interesting details, such as usage examples, limitations, remaining work to be done... etc... It is usually referred to as PATCH 0/N.
>
> See https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg01255.html for a typical example.
>
> [Pradeep Jagadeesh] Ok thanks, for the clarification.
> So, now in new e-mail I do not need to send the whole .patch file right?
> Just the diff what it has in the beginning.
No patch in the cover letter. See below.
> I am asking this because, so far I used to sent through git send-email.
> This is my first patch please bare with my silly questions!:)
>
So when you have to send a patch series, you typically do the following, as
indicated in the git-send-email manual page:
$ git format-patch --cover-letter -M origin/master -o outgoing/
$ edit outgoing/0000-*
$ git send-email outgoing/*
The generated outgoing/0000-* file initially contains a *** BLURB HERE ***
line. This is where you should put the introductory text for the series.
> > > - post the cover+both patches with a v13 prefix
> > For both patches with V13?
> >
>
> Yes. The version is more a series attribute: V13 introduces a second patch to remove duplicate lines and fixes the changelog of the first patch.
>
> [Pradeep Jagadeesh] Ok
>
> -Pradeep
>
> > -Pradeep
> >
> >
> > >
> > > Cheers.
> > >
> > > --
> > > Greg
> > >
> > >> blockdev.c | 80 ++---------------------------------
> > >> fsdev/qemu-fsdev-opts.c | 80 ++---------------------------------
> > >> include/qemu/throttle-options.h | 93
> > >> +++++++++++++++++++++++++++++++++++++++++
> > >> 3 files changed, 101 insertions(+), 152 deletions(-) create mode
> > >> 100644 include/qemu/throttle-options.h
> > >>
> > >> There is some code duplication around the command line options.
> > >> This patch is a first proposal to reduce the duplication.
> > >>
> > >> diff --git a/blockdev.c b/blockdev.c index 245e1e1..1da6b7e 100644
> > >> --- a/blockdev.c
> > >> +++ b/blockdev.c
> > >> @@ -52,6 +52,7 @@
> > >> #include "sysemu/arch_init.h"
> > >> #include "qemu/cutils.h"
> > >> #include "qemu/help_option.h"
> > >> +#include "qemu/throttle-options.h"
> > >>
> > >> static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
> > >> QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states);
> > >> @@ -4000,82 +4001,9 @@ QemuOptsList qemu_common_drive_opts = {
> > >> .type = QEMU_OPT_BOOL,
> > >> .help = "open drive file as read-only",
> > >> },{
> > >> - .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",
> > >> - },{
> > >> + },
> > >> + THROTTLE_OPTS
> > >> + {
> > >> .name = "throttling.group",
> > >> .type = QEMU_OPT_STRING,
> > >> .help = "name of the block throttling group", diff
> > >> --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c index
> > >> 385423f0..13597a3 100644
> > >> --- a/fsdev/qemu-fsdev-opts.c
> > >> +++ b/fsdev/qemu-fsdev-opts.c
> > >> @@ -9,6 +9,7 @@
> > >> #include "qemu/config-file.h"
> > >> #include "qemu/option.h"
> > >> #include "qemu/module.h"
> > >> +#include "qemu/throttle-options.h"
> > >>
> > >> static QemuOptsList qemu_fsdev_opts = {
> > >> .name = "fsdev",
> > >> @@ -37,83 +38,10 @@ 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",
> > >> },
> > >> +
> > >> + THROTTLE_OPTS
> > >> +
> > >> { /*End of list */ }
> > >> },
> > >> };
> > >> diff --git a/include/qemu/throttle-options.h
> > >> b/include/qemu/throttle-options.h new file mode 100644 index
> > >> 0000000..a2fb817
> > >> --- /dev/null
> > >> +++ b/include/qemu/throttle-options.h
> > >> @@ -0,0 +1,93 @@
> > >> +/*
> > >> + * QEMU throttling command line options
> > >> + *
> > >> + * 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 THROTTLE_OPTIONS_H
> > >> +#define THROTTLE_OPTIONS_H
> > >> +
> > >> +#define THROTTLE_OPTS \
> > >> + { \
> > >> + .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",\
> > >> + },
> > >> +
> > >> +#endif
> > >
> >
>
next prev parent reply other threads:[~2017-01-23 14:49 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-23 9:19 [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files Pradeep Jagadeesh
2017-01-23 9:47 ` Greg Kurz
2017-01-23 9:58 ` Pradeep Jagadeesh
2017-01-23 10:27 ` Greg Kurz
2017-01-23 14:02 ` Pradeep Jagadeesh
2017-01-23 14:48 ` Greg Kurz [this message]
2017-01-23 15:01 ` Pradeep Jagadeesh
2017-01-23 11:30 ` Stefan Hajnoczi
2017-01-23 15:19 ` Eric Blake
2017-01-23 15:43 ` 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=20170123154856.6dd25aed@bahia.lan \
--to=groug@kaod.org \
--cc=berto@igalia.com \
--cc=pradeep.jagadeesh@huawei.com \
--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).