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

[Markus Armbruster <armbru@redhat.com>]

Anton Nefedov <anton.nefedov@virtuozzo.com> writes:

> On 3/2/2018 6:59 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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 <anton.nefedov@virtuozzo.com>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
> [..]
>>>> +##
>>>> +# @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?