Re: [Qemu-devel] [PATCH v6 9/9] qapi: query-blockstat: add driver specific file-posix stats

[Markus Armbruster]

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

[...]