[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 9/9] qapi: query-blockstat: add driver specif
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v6 9/9] qapi: query-blockstat: add driver specific file-posix stats |
Date: |
Thu, 13 Dec 2018 13:20:16 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
I'm reviewing just the QAPI schema today.
Anton Nefedov <address@hidden> writes:
> A block driver can provide a callback to report driver-specific
> statistics.
>
> file-posix driver now reports discard statistics
>
> Signed-off-by: Anton Nefedov <address@hidden>
> ---
> qapi/block-core.json | 38 ++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 1 +
> include/block/block_int.h | 1 +
> block.c | 9 +++++++++
> block/file-posix.c | 32 ++++++++++++++++++++++++++++++++
> block/qapi.c | 5 +++++
> 6 files changed, 86 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 959358ccc4..b100e852c7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -877,6 +877,41 @@
> '*x_wr_latency_histogram': 'BlockLatencyHistogramInfo',
> '*x_flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>
> +##
> +# @BlockStatsSpecificFile:
> +#
> +# File driver statistics
> +#
> +# @discard-nb-ok: The number of succeeded discard operations performed by
successful discard operations
> +# the driver.
> +#
> +# @discard-nb-failed: The number of failed discard operations performed by
> +# the driver.
> +#
> +# @discard-bytes-ok: The number of bytes discarded by the driver.
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'BlockStatsSpecificFile',
> + 'data': {
> + 'discard-nb-ok': 'int',
> + 'discard-nb-failed': 'int',
> + 'discard-bytes-ok': 'int' } }
Should these be unsigned?
For what it's worth, similar counters nearby are also 'int'.
> +
> +##
> +# @BlockStatsSpecific:
> +#
> +# Block driver specific statistics
> +#
> +# Since: 4.0
> +##
> +{ 'union': 'BlockStatsSpecific',
> + 'base': { 'driver': 'BlockdevDriver' },
> + 'discriminator': 'driver',
> + 'data': {
> + 'file': 'BlockStatsSpecificFile',
> + 'host_device': 'BlockStatsSpecificFile' } }
> +
> ##
> # @BlockStats:
> #
> @@ -892,6 +927,8 @@
> #
> # @stats: A @BlockDeviceStats for the device.
> #
> +# @driver-specific: Optional driver-specific stats. (Since 4.0)
> +#
> # @parent: This describes the file block device if it has one.
> # Contains recursively the statistics of the underlying
> # protocol (e.g. the host file for a qcow2 image). If there is
> @@ -905,6 +942,7 @@
> { 'struct': 'BlockStats',
> 'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
> 'stats': 'BlockDeviceStats',
> + '*driver-specific': 'BlockStatsSpecific',
> '*parent': 'BlockStats',
> '*backing': 'BlockStats'} }
>
Feels awkward.
When is @driver-specific present? Exactly when the driver is 'file' or
'host_device'? If that's correct, then turning BlockStats into a union
would be clearer and reduce parenthesises on the wire:
{ 'union': 'BlockStats',
'base': {
'driver': 'BlockdevDriver',
... all the other existing members of BlockStats ... }
'discriminator': 'driver',
'data': {
'file': 'BlockStatsSpecificFile',
'host_device': 'BlockStatsSpecificFile' } }
[...]