| Message ID | 20190516143314.81302-10-anton.nefedov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | discard blockstats | expand |
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. > + > ## > # @BlockStats: > # > @@ -971,6 +1006,8 @@ > # > # @stats: A @BlockDeviceStats for the device. > # > +# @driver-specific: Optional driver-specific stats. (Since 4.1) > +# > # @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 > @@ -984,6 +1021,7 @@ > { 'struct': 'BlockStats', > 'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str', > 'stats': 'BlockDeviceStats', > + '*driver-specific': 'BlockStatsSpecific', > '*parent': 'BlockStats', > '*backing': 'BlockStats'} } > [...] > diff --git a/block/file-posix.c b/block/file-posix.c > index 76d54b3a85..a2f01cfe10 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -160,9 +160,9 @@ typedef struct BDRVRawState { > bool drop_cache; > bool check_cache_dropped; > struct { > - int64_t discard_nb_ok; > - int64_t discard_nb_failed; > - int64_t discard_bytes_ok; > + uint64_t discard_nb_ok; > + uint64_t discard_nb_failed; > + uint64_t discard_bytes_ok; (I don’t know why you didn’t introduce these fields with these types in the previous patch then.) Max > } stats; > > PRManager *pr_mgr;
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. 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 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. 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?) >> diff --git a/block/file-posix.c b/block/file-posix.c >> index 76d54b3a85..a2f01cfe10 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -160,9 +160,9 @@ typedef struct BDRVRawState { >> bool drop_cache; >> bool check_cache_dropped; >> struct { >> - int64_t discard_nb_ok; >> - int64_t discard_nb_failed; >> - int64_t discard_bytes_ok; >> + uint64_t discard_nb_ok; >> + uint64_t discard_nb_failed; >> + uint64_t discard_bytes_ok; > > (I don’t know why you didn’t introduce these fields with these types in > the previous patch then.) > Ouch, squashed the changes to the wrong commit, obviously
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
On 21/8/2019 2:21 PM, Max Reitz wrote: > 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? > So, you mean count it all on BDS level _instead_ of SCSI/IDE level? Now it's: "device": "drive-scsi0-0-0-0", "node-name": "#block151", "stats": { "unmap_operations": 0, <--- filled by SCSI [..] "parent": { "node-name": "#block056", "stats": { "unmap_operations": 0, <--- not filled "driver-stats": { <--- filled by file-posix driver "type": "file", "data": { "discard_bytes_ok": 0, "discard_nb_failed": 0, "discard_nb_ok": 0 } } }, Every level may drop some requests (i.e. qcow2 won't pass requests smaller than a cluster size) i.e. BlockBackend stats >= BDS format driver stats >= BDS protocol driver stats and the difference between them (mostly between the top and the bottom ones) is interesting here too; good to know whether it's a guest who doesn't send requests, or QEMU that limits them. >> 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). >
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' } } + ## # @BlockStats: # @@ -971,6 +1006,8 @@ # # @stats: A @BlockDeviceStats for the device. # +# @driver-specific: Optional driver-specific stats. (Since 4.1) +# # @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 @@ -984,6 +1021,7 @@ { 'struct': 'BlockStats', 'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str', 'stats': 'BlockDeviceStats', + '*driver-specific': 'BlockStatsSpecific', '*parent': 'BlockStats', '*backing': 'BlockStats'} } diff --git a/include/block/block.h b/include/block/block.h index 5e2b98b0ee..b182f0c7ae 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -490,6 +490,7 @@ int bdrv_get_flags(BlockDriverState *bs); int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs, Error **errp); +BlockStatsSpecific *bdrv_get_specific_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 94d45c9708..dc3bc97ea3 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -358,6 +358,7 @@ struct BlockDriver { int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi); ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs, Error **errp); + BlockStatsSpecific *(*bdrv_get_specific_stats)(BlockDriverState *bs); int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov, diff --git a/block.c b/block.c index 6999aad446..f68fb5aaec 100644 --- a/block.c +++ b/block.c @@ -4942,6 +4942,15 @@ ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs, return NULL; } +BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs) +{ + BlockDriver *drv = bs->drv; + if (!drv || !drv->bdrv_get_specific_stats) { + return NULL; + } + return drv->bdrv_get_specific_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 76d54b3a85..a2f01cfe10 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -160,9 +160,9 @@ typedef struct BDRVRawState { bool drop_cache; bool check_cache_dropped; struct { - int64_t discard_nb_ok; - int64_t discard_nb_failed; - int64_t discard_bytes_ok; + uint64_t discard_nb_ok; + uint64_t discard_nb_failed; + uint64_t discard_bytes_ok; } stats; PRManager *pr_mgr; @@ -2723,6 +2723,36 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) return 0; } +static BlockStatsSpecificFile get_blockstats_specific_file(BlockDriverState *bs) +{ + BDRVRawState *s = bs->opaque; + return (BlockStatsSpecificFile) { + .discard_nb_ok = s->stats.discard_nb_ok, + .discard_nb_failed = s->stats.discard_nb_failed, + .discard_bytes_ok = s->stats.discard_bytes_ok, + }; +} + +static BlockStatsSpecific *raw_get_specific_stats(BlockDriverState *bs) +{ + BlockStatsSpecific *stats = g_new(BlockStatsSpecific, 1); + + stats->driver = BLOCKDEV_DRIVER_FILE; + stats->u.file = get_blockstats_specific_file(bs); + + return stats; +} + +static BlockStatsSpecific *hdev_get_specific_stats(BlockDriverState *bs) +{ + BlockStatsSpecific *stats = g_new(BlockStatsSpecific, 1); + + stats->driver = BLOCKDEV_DRIVER_HOST_DEVICE; + stats->u.host_device = get_blockstats_specific_file(bs); + + return stats; +} + static QemuOptsList raw_create_opts = { .name = "raw-create-opts", .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head), @@ -2922,6 +2952,7 @@ BlockDriver bdrv_file = { .bdrv_get_info = raw_get_info, .bdrv_get_allocated_file_size = raw_get_allocated_file_size, + .bdrv_get_specific_stats = raw_get_specific_stats, .bdrv_check_perm = raw_check_perm, .bdrv_set_perm = raw_set_perm, .bdrv_abort_perm_update = raw_abort_perm_update, @@ -3400,6 +3431,7 @@ static BlockDriver bdrv_host_device = { .bdrv_get_info = raw_get_info, .bdrv_get_allocated_file_size = raw_get_allocated_file_size, + .bdrv_get_specific_stats = hdev_get_specific_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 f9447a3297..3afcb9dc5c 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -537,6 +537,11 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs, s->stats->wr_highest_offset = stat64_get(&bs->wr_highest_offset); + s->driver_specific = bdrv_get_specific_stats(bs); + if (s->driver_specific) { + s->has_driver_specific = true; + } + if (bs->file) { s->has_parent = true; s->parent = bdrv_query_bds_stats(bs->file->bs, blk_level);