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

[Anton Nefedov <anton.nefedov@virtuozzo.com>]

On 12/8/2019 10:04 PM, Max Reitz wrote:
> 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.
> 

hi!

Having both "unmap" and "discard" stats in the same list feels weird.
The intention is that unmap belongs to the device level, and discard is
from the driver level. Now we have a separate structure named "driver-
specific". Could also be called "driver-stats".

We could make this structure non-optional, present for all driver
types, as indeed there is nothing special about discard stats. But then
we need some way to distinguish
  - discard_nb_ok == 0 as no request reached the driver level
  - discard_nb_ok == 0 as the driver does not support the accounting

Yes, putting the accounting in the generic code would help, but do we
really want to burden it with accounting too? Tracking that each and
every case is covered with all those block_acct_done() invalid() and
failed() can really be a pain.

And what accounting should be there? All the operations? Measuring
discards at both device and BDS level is useful since discards are
optional. Double-measuring reads&writes is probably not so useful (RMW 
accounting? Read stats for the backing images?)


>> 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.)
> 

Ouch, squashed the changes to the wrong commit, obviously