[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats |
Date: |
Thu, 07 Jun 2018 17:09:58 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Anton Nefedov <address@hidden> writes:
> On 3/2/2018 6:59 PM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>>
>>> On 01/19/2018 06:50 AM, 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 <address@hidden>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>> ---
> [..]
>>>> +##
>>>> +# @BlockDriverStats:
>>>> +#
>>>> +# Statistics of a block driver (driver-specific)
>>>> +#
>>>> +# Since: 2.12
>>>> +##
>>>> +{ 'union': 'BlockDriverStats',
>>>> + 'data': {
>>>> + 'file': 'BlockDriverStatsFile'
>>>> + } }
>>>
>>> Markus has been adamant that we add no new "simple unions" (unions with
>>> a 'discriminator' field) - because they are anything but simple in the
>>> long run.
>>
>> Indeed. You could make this a flat union, similar to BlockdevOptions:
>>
>> { 'union': 'BlockDriverStats':
>> 'base': { 'driver': 'BlockdevDriver' },
>> 'discriminator': 'driver',
>> 'data': {
>> 'file': 'BlockDriverStatsFile',
>> ... } }
>>
>> However:
>>
>>>> +
>>>> +##
>>>> # @BlockStats:
>>>> #
>>>> # Statistics of a virtual block device or a block backing device.
>>>> @@ -785,6 +819,8 @@
>>>> #
>>>> # @stats: A @BlockDeviceStats for the device.
>>>> #
>>>> +# @driver-stats: Optional driver-specific statistics. (Since 2.12)
>>>> +#
>>>> # @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
>>>> @@ -798,6 +834,7 @@
>>>> { 'struct': 'BlockStats',
>>>> 'data': {'*device': 'str', '*node-name': 'str',
>>>> 'stats': 'BlockDeviceStats',
>>>> + '*driver-stats': 'BlockDriverStats',
>>
>> You're adding a union of driver-specific stats to a struct of generic
>> stats. That's unnecessarily complicated. Instead, turn the struct of
>> generic stats into a flat union, like this:
>>
>> { 'union': 'BlockStats',
>> 'base': { ... the generic stats, i.e. the members of BlockStats
>> before this patch ...
>> 'driver': 'BlockdevDriver' }
>> 'discriminator': 'driver',
>> 'data': {
>> 'file': 'BlockDriverStatsFile',
>> ... } }
>>
>
> hi,
>
> (resurrecting this series now that there is no-more-dummy-enums patchset
> https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06836.html )
>
> If we introduce BlockdevDriver as a discriminator as Markus suggests
> above, we need some way to define its value.
> I guess one would be to check blk->bs->drv->format_name but it won't
> always work; often it's even blk->bs == NULL.
There is no blk->bs, at least not if blk is a BlockBackend *.
I figure the problem you're trying to describe is query-blockstats
running into BlockBackends that aren't associated with a
BlockDriverState (blk->root is null), and thus aren't associated with a
BlockDriver. Correct?
> I guess we could add a default ('unspecified'?) to BlockdevDriver enum?
This part I understand, but...
> But I'd rather leave an optional BlockDriverStats union (but make it
> flat). Only the drivers that provide these stats will need to set
> BlockdevDriver field. What do you think?
I'm not sure I got this part. Care to sketch the QAPI schema snippet?