| Message ID | 1374584606-5615-14-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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.
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
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.
> 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
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.
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", },{
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(-)