| Message ID | 87909e13adeb2eb2c71d6613b40d3c9b534db9eb.1443793122.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 02, 2015 at 05:26:16PM +0300, Alberto Garcia wrote: > query-blockstats always returns a BlockDeviceStats structure for each > BDS, regardless of whether it implements accounting or not. Since the > field is mandatory there's no way to tell that from the values alone. > > This field solves that problem by indicating which BDSs support I/O > accounting. I'm not sure I understand the problem. If I/O accounting isn't being used then all fields will be 0?
On Tue 13 Oct 2015 05:38:30 PM CEST, Stefan Hajnoczi wrote: >> query-blockstats always returns a BlockDeviceStats structure for each >> BDS, regardless of whether it implements accounting or not. Since the >> field is mandatory there's no way to tell that from the values alone. >> >> This field solves that problem by indicating which BDSs support I/O >> accounting. > > I'm not sure I understand the problem. > > If I/O accounting isn't being used then all fields will be 0? Yes, but there's no way to tell if that happens because I/O accounting is not supported or because there hasn't been any I/O yet. There's one additional problem: this patch assumes that accounting is supported if this BDS is attached to a BlockBackend. But we don't know if the device model supports accounting or not, I still need to figure out what's the best way to do it. Berto
On Thu, Oct 15, 2015 at 03:06:19PM +0200, Alberto Garcia wrote: > On Tue 13 Oct 2015 05:38:30 PM CEST, Stefan Hajnoczi wrote: > > >> query-blockstats always returns a BlockDeviceStats structure for each > >> BDS, regardless of whether it implements accounting or not. Since the > >> field is mandatory there's no way to tell that from the values alone. > >> > >> This field solves that problem by indicating which BDSs support I/O > >> accounting. > > > > I'm not sure I understand the problem. > > > > If I/O accounting isn't being used then all fields will be 0? > > Yes, but there's no way to tell if that happens because I/O accounting > is not supported or because there hasn't been any I/O yet. > > There's one additional problem: this patch assumes that accounting is > supported if this BDS is attached to a BlockBackend. But we don't know > if the device model supports accounting or not, I still need to figure > out what's the best way to do it. Is there a corresponding libvirt patch or why does it matter whether the QMP client can detect whether blockstats are available? Stefan
On Thu 15 Oct 2015 04:58:22 PM CEST, Stefan Hajnoczi wrote: >> > If I/O accounting isn't being used then all fields will be 0? >> >> Yes, but there's no way to tell if that happens because I/O >> accounting is not supported or because there hasn't been any I/O yet. >> >> There's one additional problem: this patch assumes that accounting is >> supported if this BDS is attached to a BlockBackend. But we don't >> know if the device model supports accounting or not, I still need to >> figure out what's the best way to do it. > > Is there a corresponding libvirt patch or why does it matter whether > the QMP client can detect whether blockstats are available? I'm thinking that keeping this patch as it is now is probably not very useful. Block statistics are kept in the BlockBackend, so the only BDS that is going to have data != 0 when you call query-blockstats is the topmost one. There's probably no need to have an additional flag for this. If you disconnect a BlockBackend from a device model that implements accounting and then connect it to one that does not, there's no way for the client to know that. That's probably worth exposing in the API, but this patch does not do that yet, so I think we can skip it for now. Berto
On Fri, Oct 16, 2015 at 11:49:00AM +0200, Alberto Garcia wrote: > On Thu 15 Oct 2015 04:58:22 PM CEST, Stefan Hajnoczi wrote: > >> > If I/O accounting isn't being used then all fields will be 0? > >> > >> Yes, but there's no way to tell if that happens because I/O > >> accounting is not supported or because there hasn't been any I/O yet. > >> > >> There's one additional problem: this patch assumes that accounting is > >> supported if this BDS is attached to a BlockBackend. But we don't > >> know if the device model supports accounting or not, I still need to > >> figure out what's the best way to do it. > > > > Is there a corresponding libvirt patch or why does it matter whether > > the QMP client can detect whether blockstats are available? > > I'm thinking that keeping this patch as it is now is probably not very > useful. > > Block statistics are kept in the BlockBackend, so the only BDS that is > going to have data != 0 when you call query-blockstats is the topmost > one. There's probably no need to have an additional flag for this. > > If you disconnect a BlockBackend from a device model that implements > accounting and then connect it to one that does not, there's no way for > the client to know that. That's probably worth exposing in the API, but > this patch does not do that yet, so I think we can skip it for now. Okay. Stefan
diff --git a/block/qapi.c b/block/qapi.c index 66f2604..518b658 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -343,6 +343,8 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs, s->node_name = g_strdup(bdrv_get_node_name(bs)); } + s->supports_stats = bs->blk != NULL; + s->stats = g_malloc0(sizeof(*s->stats)); if (bs->blk) { BlockAcctStats *stats = blk_get_stats(bs->blk); diff --git a/qapi/block-core.json b/qapi/block-core.json index a529e2f..903884b 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -471,6 +471,8 @@ # # @node-name: #optional The node name of the device. (Since 2.3) # +# @supports_stats: Whether the device supports I/O stats (Since 2.5) +# # @stats: A @BlockDeviceStats for the device. # # @parent: #optional This describes the file block device if it has one. @@ -482,6 +484,7 @@ ## { 'struct': 'BlockStats', 'data': {'*device': 'str', '*node-name': 'str', + 'supports_stats': 'bool', 'stats': 'BlockDeviceStats', '*parent': 'BlockStats', '*backing': 'BlockStats'} } diff --git a/qmp-commands.hx b/qmp-commands.hx index 13c1bdc..2f48bb6 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -2496,6 +2496,7 @@ value is a json-array of all devices. Each json-object contain the following: - "device": device name (json-string) +- "supports_stats": whether the device supports I/O stats (json-bool) - "stats": A json-object with the statistics information, it contains: - "rd_bytes": bytes read (json-int) - "wr_bytes": bytes written (json-int) @@ -2528,6 +2529,7 @@ Example: { "device":"ide0-hd0", "parent":{ + "supports_stats":true, "stats":{ "wr_highest_offset":3686448128, "wr_bytes":9786368, @@ -2543,6 +2545,7 @@ Example: "idle_time_ns":2953431879 } }, + "supports_stats":true, "stats":{ "wr_highest_offset":2821110784, "wr_bytes":9786368, @@ -2560,6 +2563,7 @@ Example: }, { "device":"ide1-cd0", + "supports_stats":false, "stats":{ "wr_highest_offset":0, "wr_bytes":0, @@ -2576,6 +2580,7 @@ Example: }, { "device":"floppy0", + "supports_stats":false, "stats":{ "wr_highest_offset":0, "wr_bytes":0, @@ -2592,6 +2597,7 @@ Example: }, { "device":"sd0", + "supports_stats":false, "stats":{ "wr_highest_offset":0, "wr_bytes":0,
query-blockstats always returns a BlockDeviceStats structure for each BDS, regardless of whether it implements accounting or not. Since the field is mandatory there's no way to tell that from the values alone. This field solves that problem by indicating which BDSs support I/O accounting. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qapi.c | 2 ++ qapi/block-core.json | 3 +++ qmp-commands.hx | 6 ++++++ 3 files changed, 11 insertions(+)