| Message ID | 1516366207-109842-9-git-send-email-anton.nefedov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > --- > +++ b/qapi/block-core.json > @@ -774,6 +774,40 @@ > 'timed_stats': ['BlockDeviceTimedStats'] } } > > ## > +# @BlockDriverStatsFile: > +# > +# File driver statistics > +# > +# @discard_nb_ok: The number of succeeded 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 2.12 > +## > +{ 'struct': 'BlockDriverStatsFile', > + 'data': { > + 'discard_nb_ok': 'int', > + 'discard_nb_failed': 'int', > + 'discard_bytes_ok': 'int' > + } } New interfaces should prefer '-' over '_', where possible (a reason for using '_' is if the fields are alongside pre-existing fields that already used '_'). Let's see how this gets used...[1] > + > +## > +# @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. > + > +## > # @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', ...[1] You are using it alongside a struct that already uses '-' (node-name), so you should use dashes. So, the difference between your proposal (a simple union) and using a "flat union", on the wire, is yours: "return": { ..., "driver-stats": { "type": "file", "data": { "discard_nb_ok: ... } } } vs. a flat union: "return": { ..., "driver-stats": { "driver": "file", "discard-nb-ok": ... } } where you can benefit from less nesting and a saner discriminator name.
On 22/1/2018 11:55 PM, Eric Blake wrote: > 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> >> --- > >> +++ b/qapi/block-core.json >> @@ -774,6 +774,40 @@ >> 'timed_stats': ['BlockDeviceTimedStats'] } } >> >> ## >> +# @BlockDriverStatsFile: >> +# >> +# File driver statistics >> +# >> +# @discard_nb_ok: The number of succeeded 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 2.12 >> +## >> +{ 'struct': 'BlockDriverStatsFile', >> + 'data': { >> + 'discard_nb_ok': 'int', >> + 'discard_nb_failed': 'int', >> + 'discard_bytes_ok': 'int' >> + } } > > New interfaces should prefer '-' over '_', where possible (a reason for > using '_' is if the fields are alongside pre-existing fields that > already used '_'). Let's see how this gets used...[1] > >> + >> +## >> +# @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. > >> + >> +## >> # @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', > > ...[1] You are using it alongside a struct that already uses '-' > (node-name), so you should use dashes. > > So, the difference between your proposal (a simple union) and using a > "flat union", on the wire, is yours: > > "return": { ..., "driver-stats": { "type": "file", "data": { > "discard_nb_ok: ... } } } > > vs. a flat union: > > "return": { ..., "driver-stats": { "driver": "file", "discard-nb-ok": > ... } } > > where you can benefit from less nesting and a saner discriminator name. > Right, forgot about those unions.. Will fix. (I guess I will need an extra enum, like BlockdevDriverWithStats with a single 'file' member, otherwise it seems to require to define data for each BlockdevDriver type) /Anton
On 01/23/2018 05:28 AM, Anton Nefedov wrote: >>> + >>> +## >>> +# @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. > > Right, forgot about those unions.. Will fix. > > (I guess I will need an extra enum, like BlockdevDriverWithStats with a > single 'file' member, otherwise it seems to require to define data for > each BlockdevDriver type) Kevin also recently requested an easier way to make a flat union that uses only a subset of a larger enum. Maybe it's worth investing some time in a QAPI generator patch to make this part easier (I have a potential patch floating on one of my older branches that would allow 'branch':{} rather than having to declare a dummy type, but that's slightly different than not even supplying the branches that you aren't implementing).
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> >> --- > >> +++ b/qapi/block-core.json >> @@ -774,6 +774,40 @@ >> 'timed_stats': ['BlockDeviceTimedStats'] } } >> >> ## >> +# @BlockDriverStatsFile: >> +# >> +# File driver statistics >> +# >> +# @discard_nb_ok: The number of succeeded 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 2.12 >> +## >> +{ 'struct': 'BlockDriverStatsFile', >> + 'data': { >> + 'discard_nb_ok': 'int', >> + 'discard_nb_failed': 'int', >> + 'discard_bytes_ok': 'int' >> + } } > > New interfaces should prefer '-' over '_', where possible (a reason for > using '_' is if the fields are alongside pre-existing fields that > already used '_'). Let's see how this gets used...[1] > >> + >> +## >> +# @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', ... } } > ...[1] You are using it alongside a struct that already uses '-' > (node-name), so you should use dashes. > > So, the difference between your proposal (a simple union) and using a > "flat union", on the wire, is yours: > > "return": { ..., "driver-stats": { "type": "file", "data": { > "discard_nb_ok: ... } } } > > vs. a flat union: > > "return": { ..., "driver-stats": { "driver": "file", "discard-nb-ok": > ... } } > > where you can benefit from less nesting and a saner discriminator name. My proposal peels off yet another level of nesting.
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: >>> + >>> +## >>> +# @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', > ... } } > >> ...[1] You are using it alongside a struct that already uses '-' >> (node-name), so you should use dashes. >> >> So, the difference between your proposal (a simple union) and using a >> "flat union", on the wire, is yours: >> >> "return": { ..., "driver-stats": { "type": "file", "data": { >> "discard_nb_ok: ... } } } >> >> vs. a flat union: >> >> "return": { ..., "driver-stats": { "driver": "file", "discard-nb-ok": >> ... } } >> >> where you can benefit from less nesting and a saner discriminator name. > > My proposal peels off yet another level of nesting. > The output is better indeed, thanks; a little drawback is now we need to pass the whole BlockStats to the driver so it fills its stats. e.g. the interface: void (*bdrv_get_stats)(BlockDriverState *bs, BlockStats *stats); And that BlockdevDriver subset type (or a generator patch) still seems to be needed { 'enum' : 'BlockdevDriverWithStats', 'data' : [ 'file' ] }
On 12/2/2018 7:38 PM, Anton Nefedov wrote: > > > 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: >>>> + >>>> +## >>>> +# @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', >> ... } } >> >>> ...[1] You are using it alongside a struct that already uses '-' >>> (node-name), so you should use dashes. >>> >>> So, the difference between your proposal (a simple union) and using a >>> "flat union", on the wire, is yours: >>> >>> "return": { ..., "driver-stats": { "type": "file", "data": { >>> "discard_nb_ok: ... } } } >>> >>> vs. a flat union: >>> >>> "return": { ..., "driver-stats": { "driver": "file", "discard-nb-ok": >>> ... } } >>> >>> where you can benefit from less nesting and a saner discriminator name. >> >> My proposal peels off yet another level of nesting. >> > > The output is better indeed, thanks; a little drawback is now we need to > pass the whole BlockStats to the driver so it fills its stats. > > e.g. the interface: > > void (*bdrv_get_stats)(BlockDriverState *bs, BlockStats *stats); > > And that BlockdevDriver subset type (or a generator patch) still seems > to be needed > > { 'enum' : 'BlockdevDriverWithStats', > 'data' : [ 'file' ] } Hmm, actually it seems the driver-specific-stats should either be optional, or we need to handle the rest of the drivers in the generated code. (i.e. with the dummy enum as above, what should the mandatory 'driver' field be when there's e.g. nbd driver? It will default to 0, and that is BLOCKDEV_DRIVER_WITH_STATS_FILE, so it will be interpreted as 'file' in qmp output) If we patch the generator, I guess we could add smth like a new tag to the union that has no data for some discriminators? e.g. { 'union': 'BlockStats', 'base': {'*device': 'str', '*node-name': 'str', 'stats': 'BlockDeviceStats', '*parent': 'BlockStats', '*backing': 'BlockStats', 'driver': 'BlockdevDriver'}, 'discriminator': 'driver', 'data-optional': { <---- instead of 'data' 'file': 'BlockDriverStatsFile', } } Then the generator would need to: - pick either 'data-optional' or 'data' members - skip 'data missing branch' check for such unions - do not abort() when visiting the union and discriminator pointing to no data type I can try and implement this if there's no other suggestion? /Anton
On 02/15/2018 04:23 AM, Anton Nefedov wrote: >> The output is better indeed, thanks; a little drawback is now we need to >> pass the whole BlockStats to the driver so it fills its stats. >> >> e.g. the interface: >> >> void (*bdrv_get_stats)(BlockDriverState *bs, BlockStats *stats); >> >> And that BlockdevDriver subset type (or a generator patch) still seems >> to be needed >> >> { 'enum' : 'BlockdevDriverWithStats', >> 'data' : [ 'file' ] } > > Hmm, actually it seems the driver-specific-stats should either be > optional, or we need to handle the rest of the drivers in the generated > code. No, we don't want a new enum. Rather, use the existing enum, and use an empty type for all the branches of the union that have no additional stats to add. > > (i.e. with the dummy enum as above, what should the mandatory 'driver' > field be when there's e.g. nbd driver? It will default to 0, and that is > BLOCKDEV_DRIVER_WITH_STATS_FILE, so it will be interpreted as 'file' in > qmp output) > > > If we patch the generator, I guess we could add smth like a new tag to > the union that has no data for some discriminators? Right now, the way to write a union where only some branches add fields is: { 'struct': 'SomeEmptyType', 'data': {} } { 'union': 'MyUnion', 'base':'Base', 'discriminator':'foo', 'data': { 'file': 'ExtraFields', 'nbd': 'SomeEmptyType', ... } } I also have a patch that I can revive (posted months ago) that would allow: 'data': { 'file': 'ExtraFields', 'nbd': {}, ... } } so that we don't have to explicitly create a pointless empty type for the branches that don't care. Meanwhile, here's an example post that uses the explicit empty type idiom: https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg03991.html > > e.g. > > { 'union': 'BlockStats', > 'base': {'*device': 'str', '*node-name': 'str', > 'stats': 'BlockDeviceStats', > '*parent': 'BlockStats', > '*backing': 'BlockStats', > 'driver': 'BlockdevDriver'}, > 'discriminator': 'driver', > 'data-optional': { <---- instead of 'data' > 'file': 'BlockDriverStatsFile', > } } > > Then the generator would need to: > - pick either 'data-optional' or 'data' members I'd rather keep it 'data': {} for the list of branches that have additional members, and add a new boolean, maybe 'partial-data':true, as the witness of whether the generator requires coverage for all enum values, or will implicitly allow the uncovered enum values as though they had an empty type. > - skip 'data missing branch' check for such unions > - do not abort() when visiting the union and discriminator pointing > to no data type > > I can try and implement this if there's no other suggestion? Or I could fold that into my patch from months ago. Let's see what I can come up with this week.
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. I guess we could add a default ('unspecified'?) to BlockdevDriver enum? 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?
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?
On 7/6/2018 6:09 PM, Markus Armbruster wrote: > 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? > 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.
On 06/07/2018 10:23 AM, Anton Nefedov wrote: >>> 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 Okay, that sounds like the driver stats have to be optional, present only when blk->bs is non-NULL. > >>> 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', > >>> ... } } That would require 'driver' to always resolve to something, even when there is no driver (unless we create a superset enum that adds 'none' beyond what 'BlockdevDriver' supports). > > 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. This makes the most sense to me - we're stuck with a layer of nesting, but that's because driver-stats truly is optional (we don't always have access to a driver).
Eric Blake <eblake@redhat.com> writes: > On 06/07/2018 10:23 AM, Anton Nefedov wrote: >>>> 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 > > Okay, that sounds like the driver stats have to be optional, present > only when blk->bs is non-NULL. > >> >>>> 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', >> >>> ... } } > > That would require 'driver' to always resolve to something, even when > there is no driver (unless we create a superset enum that adds 'none' > beyond what 'BlockdevDriver' supports). > >> >> 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. > > This makes the most sense to me - we're stuck with a layer of nesting, > but that's because driver-stats truly is optional (we don't always > have access to a driver). Agree. Now we have to name the thing. You propose @driver-stats. Servicable, but let's review how existing driver-specific things are named. BlockdevOptions and BlockdevCreateOptions have them inline, thus no name. ImageInfo has '*format-specific': 'ImageInfoSpecific'. If we follow ImageInfo precedence, we get '*driver-specific': 'BlockStatsSpecific'. driver-specific I like well enough. BlockStatsSpecific less so. BlockStatsDriverSpecific feels better, but perhaps a bit long.
On 8/6/2018 8:29 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 06/07/2018 10:23 AM, Anton Nefedov wrote: >>>>> 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 >> >> Okay, that sounds like the driver stats have to be optional, present >> only when blk->bs is non-NULL. >> >>> >>>>> 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', >>> >>> ... } } >> >> That would require 'driver' to always resolve to something, even when >> there is no driver (unless we create a superset enum that adds 'none' >> beyond what 'BlockdevDriver' supports). >> >>> >>> 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. >> >> This makes the most sense to me - we're stuck with a layer of nesting, >> but that's because driver-stats truly is optional (we don't always >> have access to a driver). > > Agree. > > Now we have to name the thing. You propose @driver-stats. Servicable, > but let's review how existing driver-specific things are named. > > BlockdevOptions and BlockdevCreateOptions have them inline, thus no > name. > > ImageInfo has '*format-specific': 'ImageInfoSpecific'. > > If we follow ImageInfo precedence, we get '*driver-specific': > 'BlockStatsSpecific'. driver-specific I like well enough. > BlockStatsSpecific less so. BlockStatsDriverSpecific feels better, but > perhaps a bit long. > following it further we will have BlockStatsDriverSpecificFile which is even longer. Personally, both '*driver-specific': 'BlockDriverStats'; 'BlockDriverStatsFile' '*driver-specific': 'BlockStatsSpecific'; 'BlockStatsSpecificFile' look right to me. Function signatures look ok too except that BlockDriverStats is easy to confuse with BlockDriverState BlockDriverStats *bdrv_get_stats(BlockDriverState *bs); BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs); so maybe BlockStatsSpecific indeed?
Anton Nefedov <anton.nefedov@virtuozzo.com> writes: > On 8/6/2018 8:29 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 06/07/2018 10:23 AM, Anton Nefedov wrote: >>>>>> 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 >>> >>> Okay, that sounds like the driver stats have to be optional, present >>> only when blk->bs is non-NULL. >>> >>>> >>>>>> 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', >>>> >>> ... } } >>> >>> That would require 'driver' to always resolve to something, even when >>> there is no driver (unless we create a superset enum that adds 'none' >>> beyond what 'BlockdevDriver' supports). >>> >>>> >>>> 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. >>> >>> This makes the most sense to me - we're stuck with a layer of nesting, >>> but that's because driver-stats truly is optional (we don't always >>> have access to a driver). >> >> Agree. >> >> Now we have to name the thing. You propose @driver-stats. Servicable, >> but let's review how existing driver-specific things are named. >> >> BlockdevOptions and BlockdevCreateOptions have them inline, thus no >> name. >> >> ImageInfo has '*format-specific': 'ImageInfoSpecific'. >> >> If we follow ImageInfo precedence, we get '*driver-specific': >> 'BlockStatsSpecific'. driver-specific I like well enough. >> BlockStatsSpecific less so. BlockStatsDriverSpecific feels better, but >> perhaps a bit long. >> > > following it further we will have BlockStatsDriverSpecificFile which is > even longer. > > Personally, both > '*driver-specific': 'BlockDriverStats'; 'BlockDriverStatsFile' > '*driver-specific': 'BlockStatsSpecific'; 'BlockStatsSpecificFile' > look right to me. > > Function signatures look ok too except that BlockDriverStats is easy to > confuse with BlockDriverState > > BlockDriverStats *bdrv_get_stats(BlockDriverState *bs); > BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs); > > so maybe BlockStatsSpecific indeed? Works for me, but block maintainers have veto powers :)
diff --git a/qapi/block-core.json b/qapi/block-core.json index 3fa2d3a..0d9dc01 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -774,6 +774,40 @@ 'timed_stats': ['BlockDeviceTimedStats'] } } ## +# @BlockDriverStatsFile: +# +# File driver statistics +# +# @discard_nb_ok: The number of succeeded 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 2.12 +## +{ 'struct': 'BlockDriverStatsFile', + 'data': { + 'discard_nb_ok': 'int', + 'discard_nb_failed': 'int', + 'discard_bytes_ok': 'int' + } } + +## +# @BlockDriverStats: +# +# Statistics of a block driver (driver-specific) +# +# Since: 2.12 +## +{ 'union': 'BlockDriverStats', + 'data': { + 'file': 'BlockDriverStatsFile' + } } + +## # @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', '*parent': 'BlockStats', '*backing': 'BlockStats'} } diff --git a/include/block/block.h b/include/block/block.h index 9b12774..2f20697 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -473,6 +473,7 @@ const char *bdrv_get_device_or_node_name(const BlockDriverState *bs); int bdrv_get_flags(BlockDriverState *bs); int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs); +BlockDriverStats *bdrv_get_driver_stats(BlockDriverState *bs); void bdrv_round_to_clusters(BlockDriverState *bs, int64_t offset, int64_t bytes, int64_t *cluster_offset, diff --git a/include/block/block_int.h b/include/block/block_int.h index 29cafa4..6f56aeb 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -269,6 +269,7 @@ struct BlockDriver { Error **errp); int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi); ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs); + BlockDriverStats *(*bdrv_get_stats)(BlockDriverState *bs); int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov, diff --git a/block.c b/block.c index a8da4f2..8c03587 100644 --- a/block.c +++ b/block.c @@ -4062,6 +4062,15 @@ ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs) return NULL; } +BlockDriverStats *bdrv_get_driver_stats(BlockDriverState *bs) +{ + BlockDriver *drv = bs->drv; + if (!drv || !drv->bdrv_get_stats) { + return NULL; + } + return drv->bdrv_get_stats(bs); +} + void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event) { if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) { diff --git a/block/file-posix.c b/block/file-posix.c index 544ae58..3ab92e6 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2240,6 +2240,25 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) return 0; } +static BlockDriverStats *raw_get_stats(BlockDriverState *bs) +{ + BDRVRawState *s = bs->opaque; + BlockDriverStats *stats = g_new(BlockDriverStats, 1); + + *stats = (BlockDriverStats){ + .type = BLOCK_DRIVER_STATS_KIND_FILE, + .u.file.data = g_new(BlockDriverStatsFile, 1), + }; + + *stats->u.file.data = (BlockDriverStatsFile){ + .discard_nb_ok = s->stats.discard_nb_ok, + .discard_nb_failed = s->stats.discard_nb_failed, + .discard_bytes_ok = s->stats.discard_bytes_ok, + }; + + return stats; +} + static QemuOptsList raw_create_opts = { .name = "raw-create-opts", .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head), @@ -2312,6 +2331,7 @@ BlockDriver bdrv_file = { .bdrv_get_info = raw_get_info, .bdrv_get_allocated_file_size = raw_get_allocated_file_size, + .bdrv_get_stats = raw_get_stats, .bdrv_check_perm = raw_check_perm, .bdrv_set_perm = raw_set_perm, .bdrv_abort_perm_update = raw_abort_perm_update, @@ -2790,6 +2810,7 @@ static BlockDriver bdrv_host_device = { .bdrv_get_info = raw_get_info, .bdrv_get_allocated_file_size = raw_get_allocated_file_size, + .bdrv_get_stats = raw_get_stats, .bdrv_check_perm = raw_check_perm, .bdrv_set_perm = raw_set_perm, .bdrv_abort_perm_update = raw_abort_perm_update, diff --git a/block/qapi.c b/block/qapi.c index 6e110f2..4191e6c 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -489,6 +489,11 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs, s->stats->wr_highest_offset = stat64_get(&bs->wr_highest_offset); + s->driver_stats = bdrv_get_driver_stats(bs); + if (s->driver_stats) { + s->has_driver_stats = true; + } + if (bs->file) { s->has_parent = true; s->parent = bdrv_query_bds_stats(bs->file->bs, blk_level);