Re: [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code

[Eric Blake <eblake@redhat.com>]

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

On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
> This patchset removes the throttle redundant code that was present
> in block and fsdev files.
> 
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---

I think you want portions of this patch to come first; you want to
refactor out the IOThrottle portion of existing types, and then extend
IOThrottle to be used in more places.

>  blockdev.c                      | 106 ++++-----------------------------------
>  fsdev/Makefile.objs             |   1 +
>  fsdev/qemu-fsdev-throttle.c     |  94 +---------------------------------
>  fsdev/qemu-fsdev-throttle.h     |   3 +-
>  hmp.c                           |  39 +++++++--------
>  include/qemu/throttle-options.h |   5 ++
>  qapi/block-core.json            |  76 +---------------------------
>  qapi/iothrottle.json            |   4 +-
>  util/Makefile.objs              |   1 +
>  util/throttle-options.c         | 108 ++++++++++++++++++++++++++++++++++++++++
>  10 files changed, 151 insertions(+), 286 deletions(-)
>  create mode 100644 util/throttle-options.c
> 

> +++ b/fsdev/qemu-fsdev-throttle.c
> @@ -33,56 +33,7 @@ void fsdev_set_io_throttle(IOThrottle *arg, FsThrottle *fst, Error **errp)
>  {
>      ThrottleConfig cfg;
>  
> -    throttle_config_init(&cfg);
> -    cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
> -    cfg.buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
> -    cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
> -

This code was added in the last patch - which makes for a lot of
unnecessary churn.  Again, the best series does refactoring of existing
code first, then adds the new code that takes advantage of the
refactored entry points, so that the new code isn't churning.


> +++ b/hmp.c
> @@ -1554,20 +1554,25 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>  }
>  
> +static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict)
> +{
> +    iot->has_device = true;
> +    iot->device = (char *) qdict_get_str(qdict, "device");

Is casting away const going to result in a double free of memory later
on?  Or put another way, should this be a g_strdup'd copy (which you
then have to make sure is not leaked)?

> +    iot->bps = qdict_get_int(qdict, "bps");
> +    iot->bps_rd = qdict_get_int(qdict, "bps_rd");
> +    iot->bps_wr = qdict_get_int(qdict, "bps_wr");
> +    iot->iops = qdict_get_int(qdict, "iops");
> +    iot->iops_rd = qdict_get_int(qdict, "iops_rd");
> +    iot->iops_wr = qdict_get_int(qdict, "iops_wr");
> +}
> +
>  void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> -    BlockIOThrottle throttle = {
> -        .has_device = true,
> -        .device = (char *) qdict_get_str(qdict, "device"),

At any rate, I see it is just code motion.  You are refactoring too much
in one patch here; I'd consider splitting it along the lines of one
patch that adds hmp_initialize_io_throttle; and another patch that adds
parse_io_throttle_options(), so that it's easier to see just one piece
of code motion at a time, rather than trying to track multiple motions
in the same large patch.

> +++ b/qapi/block-core.json
> @@ -1758,86 +1758,14 @@
>  #
>  # A set of parameters describing block throttling.
>  #

> -# @iops_size: an I/O size in bytes (Since 1.7)
> +# @iothrottle: throttle configuration (Since 1.7)

NACK. This is an incompatible change; you are breaking the QMP wire
structure (that is, where I used to pass "arguments":{"device":"foo",
"bps_rd":1, "group":"bar" }, your change would now require me to pass
"arguments":{"iothrottle":{"device":"foo", "bps_rd":1}, "group":"bar"};
the extra nesting is what breaks things).

>  #
>  # @group: throttle group name (Since 2.4)
>  #
>  # Since: 1.1
>  ##
>  { 'struct': 'BlockIOThrottle',
> -  'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
> -            'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
> -            '*bps_max': 'int', '*bps_rd_max': 'int',
> -            '*bps_wr_max': 'int', '*iops_max': 'int',
> -            '*iops_rd_max': 'int', '*iops_wr_max': 'int',
> -            '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
> -            '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
> -            '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
> -            '*iops_size': 'int', '*group': 'str' } }
> +  'data': { '*iothrottle': 'IOThrottle', '*group': 'str' } }

What you REALLY want is a compatible change, namely:

{ 'struct': 'BlockIOThrottle', 'base': 'IOThrottle',
  'data': { '*group': 'str' } }

which says that all fields of IOThrottle are inherited at the same level
as the additional field 'group'.

For that matter, are you sure that 'group' is the only field that should
not be directly in IOThrottle, or should 'device' also be specific to
BlockIOThrottle (I'm not sure whether you were actually using 'device'
in the fsdev case, or if 'id' was sufficient).

>  
>  ##
>  # @block-stream:
> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
> index f7b615d..58f4520 100644
> --- a/qapi/iothrottle.json
> +++ b/qapi/iothrottle.json
> @@ -14,6 +14,8 @@
>  #
>  # @device: Block device name
>  #
> +# @id: The name or QOM path of the guest device (since: 2.8)
> +#

You've missed 2.8 by a long shot.  Oh - this is an argument that it is
'id' (and not 'device') that does not belong here, but should be placed
alongside 'group' in the BlockIOThrottle subtype.

>  # @bps: total throughput limit in bytes per second
>  #
>  # @bps_rd: read throughput limit in bytes per second
> @@ -80,7 +82,7 @@
>  # Since: 2.10
>  ##
>  { 'struct': 'IOThrottle',
> -  'data': { '*device': 'str', 'bps': 'int', 'bps_rd': 'int',
> +  'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
>              'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
>              '*bps_max': 'int', '*bps_rd_max': 'int',
>              '*bps_wr_max': 'int', '*iops_max': 'int',


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