Re: [Qemu-block] [PATCH 1/7] qapi: add unmap to BlockDeviceStats

[Alberto Garcia]

On Mon 20 Nov 2017 05:50:58 PM CET, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  qapi/block-core.json       | 20 ++++++++++++++++++++
>  include/block/accounting.h |  1 +
>  block/qapi.c               |  6 ++++++
>  3 files changed, 27 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 76bf50f..ba2705d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -730,6 +730,23 @@
>  # @timed_stats: Statistics specific to the set of previously defined
>  #               intervals of time (Since 2.5)
>  #
> +# @unmap_bytes: The number of bytes unmapped by the device (Since 2.12)
> +#
> +# @unmap_operations: The number of unmap operations performed by the device
> +#                    (Since 2.12)
> +#
> +# @unmap_total_time_ns: Total time spent on unmap operations in nano-seconds
> +#                       (Since 2.12)
> +#
> +# @unmap_merged: Number of unmap requests that have been merged into another
> +#                request (Since 2.12)
> +#
> +# @failed_unmap_operations: The number of failed unmap operations performed
> +#                           by the device (Since 2.12)
> +#
> +# @invalid_unmap_operations: The number of invalid unmap operations performed
> +#                            by the device (Since 2.12)
> +#
>  # Since: 0.14.0

Admittedtly this is just a style issue and I don't know what others
think, but wouldn't it be nicer if the documentation of new fields is
located together with the existing ones?

E.g

 @rd_bytes
 @wr_bytes
address@hidden
 @rd_operations
 @wr_operations
 @flush_operations
address@hidden

[...]

 @failed_rd_operations
 @failed_wr_operations
 @failed_flush_operations
address@hidden

etc.

>      ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
>      ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
> +    ds->unmap_bytes = stats->nr_bytes[BLOCK_ACCT_UNMAP];
>      ds->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
>      ds->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
> +    ds->unmap_operations = stats->nr_ops[BLOCK_ACCT_UNMAP];

Essentially like this :)

The patch is fine with me either way.

Reviewed-by: Alberto Garcia <address@hidden>

Berto