Re: [Qemu-block] [PATCH v7 5/6] block: add throttle block filter driver

[Stefan Hajnoczi]

On Tue, Aug 22, 2017 at 01:15:34PM +0300, Manos Pitsidianakis wrote:
> @@ -3095,6 +3096,20 @@
>  '*tls-creds': 'str' } }
>  
>  ##
> +# @BlockdevOptionsThrottle:
> +#
> +# Driver specific block device options for the throttle driver
> +#
> +# @throttle-group:   the name of the throttle-group object to use. It
> +#must already exist.
> +# @file: reference to or definition of the data source block 
> device
> +# Since: 2.11
> +##
> +{ 'struct': 'BlockdevOptionsThrottle',
> +  'data': { '*throttle-group': 'str',

This field is optional but:

  group_name = qemu_opt_get(opts, QEMU_OPT_THROTTLE_GROUP_NAME);
  if (!group_name) {
  error_setg(errp, "Please specify a throttle group.");
  ret = EINVAL;
  goto fin;

Should it be mandatory?

> +'file' : 'BlockdevRef'
> + } }
> +##
>  # @BlockdevOptions:
>  #
>  # Options for creating a block device.  Many options are available for all
> @@ -3155,6 +3170,7 @@
>'replication':'BlockdevOptionsReplication',
>'sheepdog':   'BlockdevOptionsSheepdog',
>'ssh':'BlockdevOptionsSsh',
> +  'throttle':   'BlockdevOptionsThrottle',
>'vdi':'BlockdevOptionsGenericFormat',
>'vhdx':   'BlockdevOptionsGenericFormat',
>'vmdk':   'BlockdevOptionsGenericCOWFormat',
> diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
> index 82f030523f..ec969e84fe 100644
> --- a/include/block/throttle-groups.h
> +++ b/include/block/throttle-groups.h
> @@ -76,5 +76,6 @@ void coroutine_fn 
> throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
>  void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
> AioContext *new_context);
>  void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
> +bool throttle_group_exists(const char *name);
>  
>  #endif
> diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
> index 182b7896e1..3528a8f4a2 100644
> --- a/include/qemu/throttle-options.h
> +++ b/include/qemu/throttle-options.h
> @@ -29,6 +29,7 @@
>  #define QEMU_OPT_BPS_WRITE_MAX "bps-write-max"
>  #define QEMU_OPT_BPS_WRITE_MAX_LENGTH "bps-write-max-length"
>  #define QEMU_OPT_IOPS_SIZE "iops-size"
> +#define QEMU_OPT_THROTTLE_GROUP_NAME "throttle-group"
>  
>  #define THROTTLE_OPT_PREFIX "throttling."
>  #define THROTTLE_OPTS \
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index a4268a954e..4b483a16d4 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -101,7 +101,7 @@ static ThrottleGroup *throttle_group_by_name(const char 
> *name)
>  return NULL;
>  }
>  

/* Must be called with the global mutex held */
> -static bool throttle_group_exists(const char *name)
> +bool throttle_group_exists(const char *name)
>  {
>  return throttle_group_by_name(name) ? true : false;
>  }

Re: [Qemu-block] [PATCH v7 5/6] block: add throttle block filter driver

[Manos Pitsidianakis]

On Tue, Aug 22, 2017 at 04:16:26PM +0200, Alberto Garcia wrote:

On Tue 22 Aug 2017 12:15:34 PM CEST, Manos Pitsidianakis wrote:

@@ -548,6 +548,11 @@ void throttle_group_unregister_tgm(ThrottleGroupMember 
*tgm)
 ThrottleGroupMember *token;
 int i;

+if (!ts) {
+/* Discard uninitialized tgm */
+return;
+}
+


Is this change needed in case throttle_configure_tgm() fails?


Yes, now all errors are before throttle_group_register_tgm(), therefore 
the unregister part in throttle_close() will not have valid data.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v7 5/6] block: add throttle block filter driver

[Alberto Garcia]

On Tue 22 Aug 2017 12:15:34 PM CEST, Manos Pitsidianakis wrote:
>  ##
> +# @BlockdevOptionsThrottle:
> +#
> +# Driver specific block device options for the throttle driver
> +#
> +# @throttle-group:   the name of the throttle-group object to use. It
> +#must already exist.
> +# @file: reference to or definition of the data source block 
> device
> +# Since: 2.11
> +##
> +{ 'struct': 'BlockdevOptionsThrottle',
> +  'data': { '*throttle-group': 'str',
> +'file' : 'BlockdevRef'
> + } }
> +##

I guess throttle-group is not optional here anymore ?

> @@ -548,6 +548,11 @@ void throttle_group_unregister_tgm(ThrottleGroupMember 
> *tgm)
>  ThrottleGroupMember *token;
>  int i;
>  
> +if (!ts) {
> +/* Discard uninitialized tgm */
> +return;
> +}
> +

Is this change needed in case throttle_configure_tgm() fails?

> +static int throttle_configure_tgm(BlockDriverState *bs,
> +  ThrottleGroupMember *tgm,
> +  QDict *options, Error **errp)
> +{
> +int ret;
> +const char *group_name;
> +Error *local_err = NULL;
> +QemuOpts *opts = qemu_opts_create(_opts, NULL, 0, _abort);
> +
> +qemu_opts_absorb_qdict(opts, options, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +ret = -EINVAL;
> +goto fin;
> +}
> +
> +group_name = qemu_opt_get(opts, QEMU_OPT_THROTTLE_GROUP_NAME);
> +if (!group_name) {
> +error_setg(errp, "Please specify a throttle group.");
> +ret = EINVAL;
> +goto fin;
> +} else if (!throttle_group_exists(group_name)) {
> +error_setg(errp, "Throttle group '%s' does not exist.", group_name);
> +ret = EINVAL;
> +goto fin;
> +}

It should be -EINVAL (negative value) in both cases.

Berto

[Qemu-block] [PATCH v7 5/6] block: add throttle block filter driver

[Manos Pitsidianakis]

block/throttle.c uses existing I/O throttle infrastructure inside a
block filter driver. I/O operations are intercepted in the filter's
read/write coroutines, and referred to block/throttle-groups.c

The driver can be used with the syntax
-drive driver=throttle,file.filename=foo.qcow2,throttle-group=bar

which registers the throttle filter node with the ThrottleGroup 'bar'. The
given group must be created beforehand with object-add or -object.

Signed-off-by: Manos Pitsidianakis 
---
 qapi/block-core.json|  18 ++-
 include/block/throttle-groups.h |   1 +
 include/qemu/throttle-options.h |   1 +
 block/throttle-groups.c |   7 +-
 block/throttle.c| 250 
 block/Makefile.objs |   1 +
 6 files changed, 276 insertions(+), 2 deletions(-)
 create mode 100644 block/throttle.c

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0bdc69aa5f..405c181954 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -,6 +,7 @@
 # Drivers that are supported in block device operations.
 #
 # @vxhs: Since 2.10
+# @throttle: Since 2.11
 #
 # Since: 2.9
 ##
@@ -2231,7 +2232,7 @@
 'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
 'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
 'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
-'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
+'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
 
 ##
 # @BlockdevOptionsFile:
@@ -3095,6 +3096,20 @@
 '*tls-creds': 'str' } }
 
 ##
+# @BlockdevOptionsThrottle:
+#
+# Driver specific block device options for the throttle driver
+#
+# @throttle-group:   the name of the throttle-group object to use. It
+#must already exist.
+# @file: reference to or definition of the data source block device
+# Since: 2.11
+##
+{ 'struct': 'BlockdevOptionsThrottle',
+  'data': { '*throttle-group': 'str',
+'file' : 'BlockdevRef'
+ } }
+##
 # @BlockdevOptions:
 #
 # Options for creating a block device.  Many options are available for all
@@ -3155,6 +3170,7 @@
   'replication':'BlockdevOptionsReplication',
   'sheepdog':   'BlockdevOptionsSheepdog',
   'ssh':'BlockdevOptionsSsh',
+  'throttle':   'BlockdevOptionsThrottle',
   'vdi':'BlockdevOptionsGenericFormat',
   'vhdx':   'BlockdevOptionsGenericFormat',
   'vmdk':   'BlockdevOptionsGenericCOWFormat',
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 82f030523f..ec969e84fe 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -76,5 +76,6 @@ void coroutine_fn 
throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
 void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
AioContext *new_context);
 void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
+bool throttle_group_exists(const char *name);
 
 #endif
diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index 182b7896e1..3528a8f4a2 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -29,6 +29,7 @@
 #define QEMU_OPT_BPS_WRITE_MAX "bps-write-max"
 #define QEMU_OPT_BPS_WRITE_MAX_LENGTH "bps-write-max-length"
 #define QEMU_OPT_IOPS_SIZE "iops-size"
+#define QEMU_OPT_THROTTLE_GROUP_NAME "throttle-group"
 
 #define THROTTLE_OPT_PREFIX "throttling."
 #define THROTTLE_OPTS \
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index a4268a954e..4b483a16d4 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -101,7 +101,7 @@ static ThrottleGroup *throttle_group_by_name(const char 
*name)
 return NULL;
 }
 
-static bool throttle_group_exists(const char *name)
+bool throttle_group_exists(const char *name)
 {
 return throttle_group_by_name(name) ? true : false;
 }
@@ -548,6 +548,11 @@ void throttle_group_unregister_tgm(ThrottleGroupMember 
*tgm)
 ThrottleGroupMember *token;
 int i;
 
+if (!ts) {
+/* Discard uninitialized tgm */
+return;
+}
+
 assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);
 assert(qemu_co_queue_empty(>throttled_reqs[0]));
 assert(qemu_co_queue_empty(>throttled_reqs[1]));
diff --git a/block/throttle.c b/block/throttle.c
new file mode 100644
index 00..910de27dcd
--- /dev/null
+++ b/block/throttle.c
@@ -0,0 +1,250 @@
+/*
+ * QEMU block throttling filter driver infrastructure
+ *
+ * Copyright (c) 2017 Manos Pitsidianakis
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that