All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
@ 2017-01-23  9:19 Pradeep Jagadeesh
  2017-01-23  9:47 ` Greg Kurz
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Pradeep Jagadeesh @ 2017-01-23  9:19 UTC (permalink / raw)
  To: Greg Kurz; +Cc: Pradeep Jagadeesh, Alberto Garcia, qemu-devel

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
  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 11:30 ` Stefan Hajnoczi
  2017-01-23 15:19 ` Eric Blake
  2 siblings, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2017-01-23  9:47 UTC (permalink / raw)
  To: Pradeep Jagadeesh; +Cc: Pradeep Jagadeesh, Alberto Garcia, qemu-devel

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.

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

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
  2017-01-23  9:47 ` Greg Kurz
@ 2017-01-23  9:58   ` Pradeep Jagadeesh
  2017-01-23 10:27     ` Greg Kurz
  0 siblings, 1 reply; 10+ messages in thread
From: Pradeep Jagadeesh @ 2017-01-23  9:58 UTC (permalink / raw)
  To: Greg Kurz, Pradeep Jagadeesh; +Cc: Alberto Garcia, qemu-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
  2017-01-23  9:58   ` Pradeep Jagadeesh
@ 2017-01-23 10:27     ` Greg Kurz
  2017-01-23 14:02       ` Pradeep Jagadeesh
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2017-01-23 10:27 UTC (permalink / raw)
  To: Pradeep Jagadeesh; +Cc: Pradeep Jagadeesh, Alberto Garcia, qemu-devel

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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
  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 11:30 ` Stefan Hajnoczi
  2017-01-23 15:19 ` Eric Blake
  2 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2017-01-23 11:30 UTC (permalink / raw)
  To: Pradeep Jagadeesh
  Cc: Greg Kurz, Alberto Garcia, Pradeep Jagadeesh, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On Mon, Jan 23, 2017 at 04:19:55AM -0500, Pradeep Jagadeesh 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>
> ---
>  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.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
  2017-01-23 10:27     ` Greg Kurz
@ 2017-01-23 14:02       ` Pradeep Jagadeesh
  2017-01-23 14:48         ` Greg Kurz
  0 siblings, 1 reply; 10+ messages in thread
From: Pradeep Jagadeesh @ 2017-01-23 14:02 UTC (permalink / raw)
  To: Greg Kurz, Pradeep Jagadeesh
  Cc: Pradeep Jagadeesh, Alberto Garcia, qemu-devel



-----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.
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!:)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
  2017-01-23 14:02       ` Pradeep Jagadeesh
@ 2017-01-23 14:48         ` Greg Kurz
  2017-01-23 15:01           ` Pradeep Jagadeesh
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2017-01-23 14:48 UTC (permalink / raw)
  To: Pradeep Jagadeesh; +Cc: Pradeep Jagadeesh, Alberto Garcia, qemu-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
  2017-01-23 14:48         ` Greg Kurz
@ 2017-01-23 15:01           ` Pradeep Jagadeesh
  0 siblings, 0 replies; 10+ messages in thread
From: Pradeep Jagadeesh @ 2017-01-23 15:01 UTC (permalink / raw)
  To: Greg Kurz; +Cc: Pradeep Jagadeesh, Alberto Garcia, qemu-devel

On 1/23/2017 3:48 PM, Greg Kurz wrote:
> 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.
>
Thanks for clarification.

-Pradeep

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
  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 11:30 ` Stefan Hajnoczi
@ 2017-01-23 15:19 ` Eric Blake
  2017-01-23 15:43   ` Pradeep Jagadeesh
  2 siblings, 1 reply; 10+ messages in thread
From: Eric Blake @ 2017-01-23 15:19 UTC (permalink / raw)
  To: Pradeep Jagadeesh, Greg Kurz
  Cc: Alberto Garcia, Pradeep Jagadeesh, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

On 01/23/2017 03:19 AM, Pradeep Jagadeesh wrote:

s/throtlle/throttle/ in the subject line (as is, the subject is rather
long, and missing a space after ':')

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH V1] throttle:Removed duplicate throtlle code from block and 9p files
  2017-01-23 15:19 ` Eric Blake
@ 2017-01-23 15:43   ` Pradeep Jagadeesh
  0 siblings, 0 replies; 10+ messages in thread
From: Pradeep Jagadeesh @ 2017-01-23 15:43 UTC (permalink / raw)
  To: Eric Blake, Pradeep Jagadeesh, Greg Kurz; +Cc: Alberto Garcia, qemu-devel

On 1/23/2017 4:19 PM, Eric Blake wrote:
> On 01/23/2017 03:19 AM, Pradeep Jagadeesh wrote:
>
> s/throtlle/throttle/ in the subject line (as is, the subject is rather
> long, and missing a space after ':')
>
Thanks for reviewing the code. I fixed the subject and shortened it.

-Pradeep

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-01-23 15:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.