Re: [Qemu-devel] [PATCH v4 2/4] fsdev-throttle-qmp: move struct ThrottleLimits to new file

[xiezhide]

-----Original Message-----
From: Eric Blake [mailto:address@hidden 
Sent: 2018年11月16日 5:41
To: xiezhide <address@hidden>; address@hidden
Cc: address@hidden; address@hidden; address@hidden; address@hidden; zengcanfu 
00215970 <address@hidden>; Jinxuefeng <address@hidden>; Chenhui (Felix, Euler) 
<address@hidden>
Subject: Re: [PATCH v4 2/4] fsdev-throttle-qmp: move struct ThrottleLimits to 
new file

On 11/15/18 2:55 AM, xiezhide wrote:
> this patch move ThrottleLimits to new file and rename struct field 
> with common format

As written, you need s/move/moves/ and s/rename/renames/ to match the singular 
actor 'this patch'.  Or, if you stick with my preference for imperative sense, 
s/this patch move/Move/

s/to new/to a new/

> 
> Signed-off-by: xiezhide <address@hidden>
> ---

> +++ b/Makefile
> @@ -106,6 +106,7 @@ GENERATED_FILES += qapi/qapi-types-sockets.h 
> qapi/qapi-types-sockets.c
>   GENERATED_FILES += qapi/qapi-types-tpm.h qapi/qapi-types-tpm.c
>   GENERATED_FILES += qapi/qapi-types-trace.h qapi/qapi-types-trace.c
>   GENERATED_FILES += qapi/qapi-types-transaction.h 
> qapi/qapi-types-transaction.c
> +GENERATED_FILES += qapi/qapi-types-tlimits.h 
> +qapi/qapi-types-tlimits.c

tlimits comes before tpm, not after transaction.  (Multiple times in this file)

> +++ b/Makefile.objs
> @@ -20,6 +20,7 @@ util-obj-y += qapi/qapi-types-sockets.o
>   util-obj-y += qapi/qapi-types-tpm.o
>   util-obj-y += qapi/qapi-types-trace.o
>   util-obj-y += qapi/qapi-types-transaction.o
> +util-obj-y += qapi/qapi-types-tlimits.o

Here too.


>   { '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' } }
> -

> -{ 'struct': 'ThrottleLimits',
> -  'data': { '*iops-total' : 'int', '*iops-total-max' : 'int',
> -            '*iops-total-max-length' : 'int', '*iops-read' : 'int',
> -            '*iops-read-max' : 'int', '*iops-read-max-length' : 'int',
> -            '*iops-write' : 'int', '*iops-write-max' : 'int',
> -            '*iops-write-max-length' : 'int', '*bps-total' : 'int',
> -            '*bps-total-max' : 'int', '*bps-total-max-length' : 'int',
> -            '*bps-read' : 'int', '*bps-read-max' : 'int',
> -            '*bps-read-max-length' : 'int', '*bps-write' : 'int',
> -            '*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
> -            '*iops-size' : 'int' } }
> +  'base': 'ThrottleLimits',
> +  'data': { '*device': 'str', '*id': 'str', '*group': 'str' } }

The old code has QMP using 'bps_wr' for BlockIOThrottle, but 'bps-write' 
for ThrottleLimits. The new code...

> +++ b/qapi/tlimits.json

> +{ 'struct': 'ThrottleLimits',
> +  'data': { '*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' } }

...is sticking with the BlockIOThrottle naming.  I don't see any use of 
ThrottleLimits in QAPI code (quick check: grep bps-wr qapi/qapi-introspect.c 
comes up empty), so we SHOULD be okay with regards to back-compat.  But I'd 
still split this patch into multiple
pieces: 1. Rename the ThrottleLimits member names (and give justification why 
such rename doesn't break back-compat). 2. Rewrite BlockIOThrottle with 
ThrottleLimits as its base class. 3. Move ThrottleLimits into a new file for 
future reuse.  (Maybe 2 and 3 can be squashed into a single patch)

> diff --git a/util/throttle.c b/util/throttle.c index e7db2ad..b421e33 
> 100644
> --- a/util/throttle.c
> +++ b/util/throttle.c
> @@ -496,98 +496,97 @@ void throttle_account(ThrottleState *ts, bool is_write, 
> uint64_t size)
>   void throttle_limits_to_config(ThrottleLimits *arg, ThrottleConfig *cfg,
>                                  Error **errp)
>   {
> -    if (arg->has_bps_total) {
> -        cfg->buckets[THROTTLE_BPS_TOTAL].avg = arg->bps_total;
> +    if (arg->has_bps) {
> +        cfg->buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;

Otherwise, the churn from renaming members (part 1) makes it hard to see if the 
code was properly moved into a new file.

Split this patch to three patches in v5

Thanks
Kidd