Patchwork [13/18] blockdev: Rename I/O throttling options for QMP

login
register
mail settings
Submitter Kevin Wolf
Date July 23, 2013, 1:03 p.m.
Message ID <1374584606-5615-14-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/261079/
State New
Headers show

Comments

Kevin Wolf - July 23, 2013, 1:03 p.m.
In QMP, we want to use dashes instead of underscores in QMP argument
names, and use nested options for throttling. Convert them for -drive
before calling into the code that will be shared between -drive and
blockdev-add.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 13 deletions(-)
Eric Blake - July 26, 2013, 4:10 p.m.
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> In QMP, we want to use dashes instead of underscores in QMP argument
> names, and use nested options for throttling. Convert them for -drive
> before calling into the code that will be shared between -drive and
> blockdev-add.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 39 insertions(+), 13 deletions(-)
> 

This patch will probably conflict with Benoît's work on leaky bucket
throttling; can the two of you decide which one should go in first?  Are
we trying to target both this series and leaky bucket throttling for 1.6?

> @@ -485,17 +486,17 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>  
>      /* disk I/O throttling */
>      io_limits.bps[BLOCK_IO_LIMIT_TOTAL]  =
> -                           qemu_opt_get_number(opts, "bps", 0);
> +        qemu_opt_get_number(opts, "throttling.bps-total", 0);

I like the rename to bps-total, to make it more obvious why it is
provided in addition to bps-{read,write}.

>      io_limits.bps[BLOCK_IO_LIMIT_READ]   =
> -                           qemu_opt_get_number(opts, "bps_rd", 0);
> +        qemu_opt_get_number(opts, "throttling.bps-read", 0);
>      io_limits.bps[BLOCK_IO_LIMIT_WRITE]  =
> -                           qemu_opt_get_number(opts, "bps_wr", 0);
> +        qemu_opt_get_number(opts, "throttling.bps-write", 0);

Thank you for spelling out the names; QMP doesn't need the abbreviations
that the command line has.

> +DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> +{
> +    /* Change legacy command line options into QMP ones */
> +    qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
> +    qemu_opt_rename(all_opts, "iops_rd", "throttling.iops-read");
> +    qemu_opt_rename(all_opts, "iops_wr", "throttling.iops-write");

Of course, the intent here is to preserve back-compat (the old spelling
continues to work, even if it differs from the QMP spelling).  But will
this patch also allow the command line to learn support for the new
spellings?  If so, is that worth mentioning as an intentional side
effect in the commit message?

Reviewed-by: Eric Blake <eblake@redhat.com>
Kevin Wolf - July 26, 2013, 4:26 p.m.
Am 26.07.2013 um 18:10 hat Eric Blake geschrieben:
> On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> > In QMP, we want to use dashes instead of underscores in QMP argument
> > names, and use nested options for throttling. Convert them for -drive
> > before calling into the code that will be shared between -drive and
> > blockdev-add.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  blockdev.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 39 insertions(+), 13 deletions(-)
> > 
> 
> This patch will probably conflict with Benoît's work on leaky bucket
> throttling; can the two of you decide which one should go in first?  Are
> we trying to target both this series and leaky bucket throttling for 1.6?

If you complete the review before I leave today, I might still send a
pull request, but as I'm going to disable blockdev-add and the new
options once again for 1.6, it doesn't really matter that much.

Benoît's series is for 1.7 as well, if I understood Stefan correctly. He
said he was going to merge a bug fix part of it for 1.6 and leave the
rest for 1.7. (I haven't been following the throttling series myself,
that's why I can't comment in much more detail.)

> > @@ -485,17 +486,17 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> >  
> >      /* disk I/O throttling */
> >      io_limits.bps[BLOCK_IO_LIMIT_TOTAL]  =
> > -                           qemu_opt_get_number(opts, "bps", 0);
> > +        qemu_opt_get_number(opts, "throttling.bps-total", 0);
> 
> I like the rename to bps-total, to make it more obvious why it is
> provided in addition to bps-{read,write}.
> 
> >      io_limits.bps[BLOCK_IO_LIMIT_READ]   =
> > -                           qemu_opt_get_number(opts, "bps_rd", 0);
> > +        qemu_opt_get_number(opts, "throttling.bps-read", 0);
> >      io_limits.bps[BLOCK_IO_LIMIT_WRITE]  =
> > -                           qemu_opt_get_number(opts, "bps_wr", 0);
> > +        qemu_opt_get_number(opts, "throttling.bps-write", 0);
> 
> Thank you for spelling out the names; QMP doesn't need the abbreviations
> that the command line has.
> 
> > +DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> > +{
> > +    /* Change legacy command line options into QMP ones */
> > +    qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
> > +    qemu_opt_rename(all_opts, "iops_rd", "throttling.iops-read");
> > +    qemu_opt_rename(all_opts, "iops_wr", "throttling.iops-write");
> 
> Of course, the intent here is to preserve back-compat (the old spelling
> continues to work, even if it differs from the QMP spelling).  But will
> this patch also allow the command line to learn support for the new
> spellings?  If so, is that worth mentioning as an intentional side
> effect in the commit message?

Yes, of course the command line will accept the new spellings as well.
I'll update the commit message.

Kevin
Eric Blake - July 26, 2013, 4:44 p.m.
On 07/26/2013 10:26 AM, Kevin Wolf wrote:

>> This patch will probably conflict with Benoît's work on leaky bucket
>> throttling; can the two of you decide which one should go in first?  Are
>> we trying to target both this series and leaky bucket throttling for 1.6?
> 
> If you complete the review before I leave today, I might still send a
> pull request, but as I'm going to disable blockdev-add and the new
> options once again for 1.6, it doesn't really matter that much.

In other words, just as it was in 1.5, the new parser is cool enough to
implement the framework now to ease backport efforts, but untested
enough that we'd rather defer use of that framework until after we are
back out of freeze.

> 
> Benoît's series is for 1.7 as well, if I understood Stefan correctly. He

Even though the latest subject line requests for-1.6?
https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg04005.html

> said he was going to merge a bug fix part of it for 1.6 and leave the
> rest for 1.7. (I haven't been following the throttling series myself,
> that's why I can't comment in much more detail.)

I guess I also need to comment on that series - we're late enough that
bug fixes are okay, but new options are risky; and the tail end of that
series adds new options to throttling as part of switching to a new
algorithm.
Kevin Wolf - July 26, 2013, 4:54 p.m.
Am 26.07.2013 um 18:44 hat Eric Blake geschrieben:
> On 07/26/2013 10:26 AM, Kevin Wolf wrote:
> 
> >> This patch will probably conflict with Benoît's work on leaky bucket
> >> throttling; can the two of you decide which one should go in first?  Are
> >> we trying to target both this series and leaky bucket throttling for 1.6?
> > 
> > If you complete the review before I leave today, I might still send a
> > pull request, but as I'm going to disable blockdev-add and the new
> > options once again for 1.6, it doesn't really matter that much.
> 
> In other words, just as it was in 1.5, the new parser is cool enough to
> implement the framework now to ease backport efforts, but untested
> enough that we'd rather defer use of that framework until after we are
> back out of freeze.

Right. Before actually committing to the interface, I'd like you to have
some real libvirt code running on it. I assume that's doable in the 1.7
time frame, right?

> > Benoît's series is for 1.7 as well, if I understood Stefan correctly. He
> 
> Even though the latest subject line requests for-1.6?
> https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg04005.html

Yes, it was discussed on IRC, Benoît will target 1.7 now.

> > said he was going to merge a bug fix part of it for 1.6 and leave the
> > rest for 1.7. (I haven't been following the throttling series myself,
> > that's why I can't comment in much more detail.)
> 
> I guess I also need to comment on that series - we're late enough that
> bug fixes are okay, but new options are risky; and the tail end of that
> series adds new options to throttling as part of switching to a new
> algorithm.

Indeed, adding new options and switching the whole algorithm that late in
the cycle is something that I would find a little bit too scary.

Kevin
Eric Blake - July 26, 2013, 5:01 p.m.
On 07/26/2013 10:54 AM, Kevin Wolf wrote:

>>> If you complete the review before I leave today, I might still send a
>>> pull request, but as I'm going to disable blockdev-add and the new
>>> options once again for 1.6, it doesn't really matter that much.
>>
>> In other words, just as it was in 1.5, the new parser is cool enough to
>> implement the framework now to ease backport efforts, but untested
>> enough that we'd rather defer use of that framework until after we are
>> back out of freeze.
> 
> Right. Before actually committing to the interface, I'd like you to have
> some real libvirt code running on it. I assume that's doable in the 1.7
> time frame, right?

Yes, I can commit to having libvirt coded up to use QMP fd-passing for
backing file chains prior to qemu 1.7, which will pretty much be a
full-stack test of everything you've been incrementally adding.
Benoît Canet - July 26, 2013, 7:35 p.m.
> This patch will probably conflict with Benoît's work on leaky bucket
> throttling; can the two of you decide which one should go in first?  Are
> we trying to target both this series and leaky bucket throttling for 1.6?

I will to rebase my serie on top of this.

However if anyone has suggestions for the names of the new options the leaky
bucket serie add it would probably avoid an extra code review.

Best regards

Benoît
Eric Blake - July 26, 2013, 7:54 p.m.
On 07/26/2013 01:35 PM, Benoît Canet wrote:
>> This patch will probably conflict with Benoît's work on leaky bucket
>> throttling; can the two of you decide which one should go in first?  Are
>> we trying to target both this series and leaky bucket throttling for 1.6?
> 
> I will to rebase my serie on top of this.

s/serie/series/

[Stupid English, where every rule has an exception.  Here, the exception
is that "series" is the correct spelling of both singular and plural
form.  Don't fret, you're not the first non-native speaker to be tripped
up by this oddity.]

> 
> However if anyone has suggestions for the names of the new options the leaky
> bucket serie add it would probably avoid an extra code review.

As I mentioned on that series, one possibility would be to have:

'*throttling': { 'bps-read': nnn, <up to 6 members, as now> }
'*throttling-threshold': { 'bps-read': nnn, ...}

so that instead of naming 6 new members, you are just naming 1 new
struct that shares the same 6 member names as the first struct.

Patch

diff --git a/blockdev.c b/blockdev.c
index 8ff8ed3..5403188 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -312,7 +312,8 @@  static bool do_check_io_limits(BlockIOLimit *io_limits, Error **errp)
     return true;
 }
 
-DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
+static DriveInfo *blockdev_init(QemuOpts *all_opts,
+                                BlockInterfaceType block_default_type)
 {
     const char *buf;
     const char *file = NULL;
@@ -485,17 +486,17 @@  DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
 
     /* disk I/O throttling */
     io_limits.bps[BLOCK_IO_LIMIT_TOTAL]  =
-                           qemu_opt_get_number(opts, "bps", 0);
+        qemu_opt_get_number(opts, "throttling.bps-total", 0);
     io_limits.bps[BLOCK_IO_LIMIT_READ]   =
-                           qemu_opt_get_number(opts, "bps_rd", 0);
+        qemu_opt_get_number(opts, "throttling.bps-read", 0);
     io_limits.bps[BLOCK_IO_LIMIT_WRITE]  =
-                           qemu_opt_get_number(opts, "bps_wr", 0);
+        qemu_opt_get_number(opts, "throttling.bps-write", 0);
     io_limits.iops[BLOCK_IO_LIMIT_TOTAL] =
-                           qemu_opt_get_number(opts, "iops", 0);
+        qemu_opt_get_number(opts, "throttling.iops-total", 0);
     io_limits.iops[BLOCK_IO_LIMIT_READ]  =
-                           qemu_opt_get_number(opts, "iops_rd", 0);
+        qemu_opt_get_number(opts, "throttling.iops-read", 0);
     io_limits.iops[BLOCK_IO_LIMIT_WRITE] =
-                           qemu_opt_get_number(opts, "iops_wr", 0);
+        qemu_opt_get_number(opts, "throttling.iops-write", 0);
 
     if (!do_check_io_limits(&io_limits, &error)) {
         error_report("%s", error_get_pretty(error));
@@ -726,6 +727,31 @@  err:
     return NULL;
 }
 
+static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to)
+{
+    const char *value;
+
+    value = qemu_opt_get(opts, from);
+    if (value) {
+        qemu_opt_set(opts, to, value);
+        qemu_opt_unset(opts, from);
+    }
+}
+
+DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
+{
+    /* Change legacy command line options into QMP ones */
+    qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
+    qemu_opt_rename(all_opts, "iops_rd", "throttling.iops-read");
+    qemu_opt_rename(all_opts, "iops_wr", "throttling.iops-write");
+
+    qemu_opt_rename(all_opts, "bps", "throttling.bps-total");
+    qemu_opt_rename(all_opts, "bps_rd", "throttling.bps-read");
+    qemu_opt_rename(all_opts, "bps_wr", "throttling.bps-write");
+
+    return blockdev_init(all_opts, block_default_type);
+}
+
 void do_commit(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
@@ -1855,27 +1881,27 @@  QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",
         },{
-            .name = "iops",
+            .name = "throttling.iops-total",
             .type = QEMU_OPT_NUMBER,
             .help = "limit total I/O operations per second",
         },{
-            .name = "iops_rd",
+            .name = "throttling.iops-read",
             .type = QEMU_OPT_NUMBER,
             .help = "limit read operations per second",
         },{
-            .name = "iops_wr",
+            .name = "throttling.iops-write",
             .type = QEMU_OPT_NUMBER,
             .help = "limit write operations per second",
         },{
-            .name = "bps",
+            .name = "throttling.bps-total",
             .type = QEMU_OPT_NUMBER,
             .help = "limit total bytes per second",
         },{
-            .name = "bps_rd",
+            .name = "throttling.bps-read",
             .type = QEMU_OPT_NUMBER,
             .help = "limit read bytes per second",
         },{
-            .name = "bps_wr",
+            .name = "throttling.bps-write",
             .type = QEMU_OPT_NUMBER,
             .help = "limit write bytes per second",
         },{