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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5592 bytes --]

On 16.05.19 16:33, Anton Nefedov wrote:
> A block driver can provide a callback to report driver-specific
> statistics.
> 
> file-posix driver now reports discard statistics
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Acked-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/block-core.json      | 38 ++++++++++++++++++++++++++++++++++++++
>  include/block/block.h     |  1 +
>  include/block/block_int.h |  1 +
>  block.c                   |  9 +++++++++
>  block/file-posix.c        | 38 +++++++++++++++++++++++++++++++++++---
>  block/qapi.c              |  5 +++++
>  6 files changed, 89 insertions(+), 3 deletions(-)


> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 55194f84ce..368e09ae37 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -956,6 +956,41 @@
>             '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
>             '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>  
> +##
> +# @BlockStatsSpecificFile:
> +#
> +# File driver statistics
> +#
> +# @discard-nb-ok: The number of successful discard operations performed by
> +#                 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.1
> +##
> +{ 'struct': 'BlockStatsSpecificFile',
> +  'data': {
> +      'discard-nb-ok': 'uint64',
> +      'discard-nb-failed': 'uint64',
> +      'discard-bytes-ok': 'uint64' } }
> +
> +##
> +# @BlockStatsSpecific:
> +#
> +# Block driver specific statistics
> +#
> +# Since: 4.1
> +##
> +{ 'union': 'BlockStatsSpecific',
> +  'base': { 'driver': 'BlockdevDriver' },
> +  'discriminator': 'driver',
> +  'data': {
> +      'file': 'BlockStatsSpecificFile',
> +      'host_device': 'BlockStatsSpecificFile' } }

I would like to use these chance to complain that I find this awkward.
My problem is that I don’t know how any management application is
supposed to reasonably consume this.  It feels weird to potentially have
to recognize the result for every block driver.

I would now like to note that I’m clearly not in a position to block
this at this point, because I’ve had a year to do so, I didn’t, so it
would be unfair to do it now.

(Still, I feel like if I have a concern, I should raise it, even if it’s
too late.)

I know Markus has proposed this, but I don’t understand why.  He set
ImageInfoSpecific as a precedence, but that has a different reasoning
behind it.  The point for that is that it simply doesn’t work any other
way, because it is clearly format-specific information that cannot be
shared between drivers.  Anything that can be shared is put into
ImageInfo (like the cluster size).

We have the same constellation here, BlockStats contains common stuff,
and BlockStatsSpecific would contain driver-specific stuff.  But to me,
BlockStatsSpecificFile doesn’t look very special.  It looks like it just
duplicates fields that already exist in BlockDeviceStats.


(Furthermore, most of ImageInfoSpecific is actually not useful to
management software, but only as an information for humans (and having
such a structure for that is perfectly fine).  But these stats don’t
really look like something for immediate human consumption.)


So I wonder why you don’t just put this information into
BlockDeviceStats.  From what I can tell looking at
bdrv_query_bds_stats() and qmp_query_blockstats(), the @stats field is
currently completely 0 if @query-nodes is true.

(Furthermore, I wonder whether it would make sense to re-add
BlockAcctStats to each BDS and then let the generic block code do the
accounting on it.  I moved it to the BB in 7f0e9da6f13 because we didn’t
care about node-level information at the time, but maybe it’s time to
reconsider.)


Anyway, as I’ve said, I fully understand that complaining about a design
decision is just unfair at this point, so this is not a veto.

> +
>  ##
>  # @BlockStats:
>  #
> @@ -971,6 +1006,8 @@
>  #
>  # @stats:  A @BlockDeviceStats for the device.
>  #
> +# @driver-specific: Optional driver-specific stats. (Since 4.1)
> +#
>  # @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
> @@ -984,6 +1021,7 @@
>  { 'struct': 'BlockStats',
>    'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
>             'stats': 'BlockDeviceStats',
> +           '*driver-specific': 'BlockStatsSpecific',
>             '*parent': 'BlockStats',
>             '*backing': 'BlockStats'} }
>  

[...]

> diff --git a/block/file-posix.c b/block/file-posix.c
> index 76d54b3a85..a2f01cfe10 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -160,9 +160,9 @@ typedef struct BDRVRawState {
>      bool drop_cache;
>      bool check_cache_dropped;
>      struct {
> -        int64_t discard_nb_ok;
> -        int64_t discard_nb_failed;
> -        int64_t discard_bytes_ok;
> +        uint64_t discard_nb_ok;
> +        uint64_t discard_nb_failed;
> +        uint64_t discard_bytes_ok;

(I don’t know why you didn’t introduce these fields with these types in
the previous patch then.)

Max

>      } stats;
>  
>      PRManager *pr_mgr;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]