Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files

[Greg Kurz]

On Mon, 23 Jan 2017 10:58:19 +0100
Pradeep Jagadeesh <address@hidden> wrote:

> On 1/23/2017 10:47 AM, Greg Kurz wrote:
> > On Mon, 23 Jan 2017 04:19:55 -0500
> > Pradeep Jagadeesh <address@hidden> wrote:
> >  
> >> This will allow other subsystems (i.e. fsdev) to implement throttling
> >> without duplicating the command line options.
> >>
> >> Signed-off-by: Pradeep Jagadeesh <address@hidden>
> >> ---  
> >
> > 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.

> > - 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
> 
> 
> >
> > 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  
> >  
>