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

[Anton Nefedov]

On 7/6/2018 6:09 PM, Markus Armbruster wrote:
> 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?

Sorry, yes, exactly

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

You earlier proposed:

>>> 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',
>>>         ... } }

But I meant to leave it as:

+ { 'union': 'BlockDriverStats':
+     'base': { 'driver': 'BlockdevDriver' },
+     'discriminator': 'driver',
+     'data': {
+         'file': 'BlockDriverStatsFile' } }


  { 'struct': 'BlockStats',
    'data': {'*device': 'str', '*node-name': 'str',
             'stats': 'BlockDeviceStats',
+            '*driver-stats': 'BlockDriverStats',
             '*parent': 'BlockStats',
             '*backing': 'BlockStats'} }

so those block backends which do not provide driver stats do not need to
set BlockdevDriver field.