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: 6597 bytes --]

On 21.08.19 13:00, Anton Nefedov wrote:
> 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.

Sorry, what I meant wasn’t adding a separate “discard” group, but just
filling in the existing “unmap” fields.  As far as I understand, if we
had BlockAcctStats for each BDS, the file node’s unmap stats would be
the same as its discard stats, wouldn’t it?

> 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

You can simply make the fields optional.  (Then the first case is
“present, but 0”, and the second is “not present”.)

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

That’s indeed a problem, yes. :-)

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

Yes, if we put BlockAcctStats at the node level, we should track all
operations, I suppose.  That would require adding accounting code in
many places, so it wouldn’t be easy, correct.

I think it would be the better solution, but you’re right in that it’s
probably not worth it.

But I do think it would be good if we could get away from a
driver-specific structure (unless we really need it; and I don’t think
we do if we just make the stats fields optional).

Max


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