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

[Pradeep Jagadeesh]

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?
> - post the cover+both patches with a v13 prefix
For both patches with V13?

-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