9:17 a.m.
The QEMU side patches are not posted yet, but considering what these
patche series need is only a fixed interface provided by QEMU, posting
it for early reviewing, won't push it before QEMU patches are got pushed
even it got ACK.
Osier
9:17 a.m.
New subject: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats
The new disk latency related members include:
rd_total_times
wr_total_times
flush_operations
flush_total_times
---
include/libvirt/libvirt.h.in | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index b1bda31..cf6bf60 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -562,8 +562,12 @@ typedef struct _virDomainBlockStats virDomainBlockStatsStruct;
struct _virDomainBlockStats {
long long rd_req; /* number of read requests */
long long rd_bytes; /* number of read bytes */
+ long long rd_total_times; /* total time spend on cache reads in nano-seconds */
long long wr_req; /* number of write requests */
long long wr_bytes; /* number of written bytes */
+ long long wr_total_times; /* total time spend on cache writes in nano-seconds */
+ long long flush_req; /* number of flushed bytes */
+ long long flush_total_times; /* total time spend on cache flushes in nano-seconds */
long long errs; /* In Xen this returns the mysterious 'oo_req'. */
};
--
1.7.6
8:53 a.m.
New subject: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats
On Fri, Aug 12, 2011 at 10:17:24PM +0800, Osier Yang wrote:
The new disk latency related members include:
rd_total_times
wr_total_times
flush_operations
flush_total_times
---
include/libvirt/libvirt.h.in | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index b1bda31..cf6bf60 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -562,8 +562,12 @@ typedef struct _virDomainBlockStats virDomainBlockStatsStruct;
struct _virDomainBlockStats {
long long rd_req; /* number of read requests */
long long rd_bytes; /* number of read bytes */
+ long long rd_total_times; /* total time spend on cache reads in nano-seconds */
long long wr_req; /* number of write requests */
long long wr_bytes; /* number of written bytes */
+ long long wr_total_times; /* total time spend on cache writes in nano-seconds */
+ long long flush_req; /* number of flushed bytes */
+ long long flush_total_times; /* total time spend on cache flushes in nano-seconds */
long long errs; /* In Xen this returns the mysterious 'oo_req'. */
};
NACK, it is forbidden to modify any structs in the public header
files since that changes the ABI.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:39 a.m.
New subject: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats
On 08/12/2011 07:53 AM, Daniel P. Berrange wrote:
On Fri, Aug 12, 2011 at 10:17:24PM +0800, Osier Yang wrote:
> The new disk latency related members include:
> rd_total_times
> wr_total_times
> flush_operations
> flush_total_times
> ---
> include/libvirt/libvirt.h.in | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index b1bda31..cf6bf60 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -562,8 +562,12 @@ typedef struct _virDomainBlockStats virDomainBlockStatsStruct;
> struct _virDomainBlockStats {
> long long rd_req; /* number of read requests */
> long long rd_bytes; /* number of read bytes */
> + long long rd_total_times; /* total time spend on cache reads in nano-seconds */
> long long wr_req; /* number of write requests */
> long long wr_bytes; /* number of written bytes */
> + long long wr_total_times; /* total time spend on cache writes in nano-seconds */
> + long long flush_req; /* number of flushed bytes */
> + long long flush_total_times; /* total time spend on cache flushes in nano-seconds
*/
> long long errs; /* In Xen this returns the mysterious 'oo_req'. */
> };
NACK, it is forbidden to modify any structs in the public header
files since that changes the ABI.
However, what we CAN do, without breaking ABI, is the following:
In libvirt.h.in:
add a new struct virDomainBlockStatsStructV1 with layout identical to
the old virDomainBlockStatsStruct.
add a new typedef virDomainBlockStatsStructV2 that maps to the modified
virDomainBlockStatsStruct.
modify struct virDomainBlockStatsStruct to add new fields, but _only_ at
the end of the struct - this struct could later grow if we need to go to
a v3.
Leave virDomainBlockStatsPtr as a typedef for *virDomainBlockStatsStruct.
In libvirt.c:
int
virDomainBlockStats (virDomainPtr dom, const char *path,
virDomainBlockStatsPtr stats, size_t size)
{
virConnectPtr conn;
struct _virDomainBlockStatsStructV1 statsv1 = { -1, -1, -1, -1, -1 };
struct _virDomainBlockStatsStructV2 statsv2 = { -1, -1, -1, -1, -1,
-1 ... };
VIR_DOMAIN_DEBUG
...
if (!path || !stats || size > sizeof(statsv2)) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
conn = dom->conn;
if (conn->driver->domainBlockStatsv2) {
if (conn->driver->domainBlockStatsv2 (dom, path, &statsv2) == -1)
goto error;
memcpy (stats, &statsv2, size);
return 0;
}
if (conn->driver->domainBlockStats) {
if (conn->driver->domainBlockStats (dom, path, &statsv1) == -1)
goto error;
if (size > sizeof(statsv1)) {
memset(((char *)stats)+sizeof(statsv1), -1, size -
sizeof(statsv1));
size = sizeof(statsv1);
}
memcpy (stats, &statsv1, size);
return 0;
}
...
In driver.h:
Update the domainBlockStats callback to explicitly take
*virDomainBlockStatsStructV1, and add a new domainBlockStats2 callback
that explicitly takes *virDomainBlockStatsStructV2.
In remote_protocol.x, add a new RPC for domain_block_stats_2 that passes
the larger struct over the wire; leaving the existing RPC tied to the
smaller v1 struct.
In qemu_driver.c, implement the new driver callback as the only way to
get at the additional fields.
This proposal is ABI safe (no function signatures changed and no new
entry points are added - all the decisions on how many bytes to populate
are based on an existing argument, and all APIs used pointers which are
constant size rather than copies where the larger struct would cause
problems). However, it has the following effects, including one slight
API ramification:
1. all existing clients that were pre-compiled against the older
libvirt.h should have already been calling
virDomainBlockStats(...,sizeof(virDomainBlockStatsStruct)), where the
compilation was done with the struct at the smaller size. This will
safely call the older RPC, and skip out on any new fields, whether
talking to an old or new server.
2. recompiling an existing client against newer libvirt.h with no other
changes will automatically pick up the larger struct. The sizeof()
argument will change. Newer servers will recognize the larger struct
and automatically call the newer RPC, getting all the new fields.
However, older servers will now reject the larger sizeof as too large -
this is a subtle limitation, if clients are recompiled without being
aware of the difference it will cause!
3. recompiling an existing client with a slight change
(s/virDomainBlockStatsPtr/virDomainBlockStatsStructV1Ptr/g) will force
the smaller sizeof(), so that the client can continue to talk to older
servers, but will lose out on the new fields in the v2 struct. This is
the mitigation to use to avoid the breakage in point 2.
4. writing a new client that only has to target 0.9.5 or newer should
explicitly use virDomainBlockStatsStructV2Ptr, so that the client will
safely talk to newer servers and gracefully be rejected by older
servers. And by explicitly using the v2 struct instead of the generic
virDomainBlockStatsPtr, the client has protected itself from a
recompilation to a future v3 of virDomainBlockStatsPtr causing the 0.9.5
support to stop working (that is, by exposing all three types: V1, V2,
and generic, an application can decide whether recompilation should be
tied to a specific struct level, or to the latest version available).
If we decide that point 2 is too onerous (that is, if we want to
guarantee that recompilation of existing clients against newer libvirt.h
will not break the ability of that client to talk to older servers),
then we can slightly tweak the changes made in libvirt.h.in so as to
keep virDomainBlockStatsPtr tied to the v1 struct size, and only use new
type names for larger structs.
Next question is what additional fields to provide? We don't want to
make a v3 any sooner than needed, so v2 should have more than just
latency, but should also include things like cdrom tray status.
Remember that the public API documents that all stats not supported for
a given drive by the given hypervisor will be set to -1, so it doesn't
hurt to add more fields than what can be populated, so long as at least
one combination of hypervisor and underlying block type can usefully use
that field.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:06 a.m.
New subject: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats
On Wed, Aug 17, 2011 at 09:39:34AM -0600, Eric Blake wrote:
On 08/12/2011 07:53 AM, Daniel P. Berrange wrote:
>On Fri, Aug 12, 2011 at 10:17:24PM +0800, Osier Yang wrote:
>>The new disk latency related members include:
>> rd_total_times
>> wr_total_times
>> flush_operations
>> flush_total_times
>>---
>> include/libvirt/libvirt.h.in | 4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>>diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>>index b1bda31..cf6bf60 100644
>>--- a/include/libvirt/libvirt.h.in
>>+++ b/include/libvirt/libvirt.h.in
>>@@ -562,8 +562,12 @@ typedef struct _virDomainBlockStats
virDomainBlockStatsStruct;
>> struct _virDomainBlockStats {
>> long long rd_req; /* number of read requests */
>> long long rd_bytes; /* number of read bytes */
>>+ long long rd_total_times; /* total time spend on cache reads in nano-seconds
*/
>> long long wr_req; /* number of write requests */
>> long long wr_bytes; /* number of written bytes */
>>+ long long wr_total_times; /* total time spend on cache writes in nano-seconds
*/
>>+ long long flush_req; /* number of flushed bytes */
>>+ long long flush_total_times; /* total time spend on cache flushes in
nano-seconds */
>> long long errs; /* In Xen this returns the mysterious 'oo_req'. */
>> };
>
>NACK, it is forbidden to modify any structs in the public header
>files since that changes the ABI.
However, what we CAN do, without breaking ABI, is the following:
In libvirt.h.in:
add a new struct virDomainBlockStatsStructV1 with layout identical
to the old virDomainBlockStatsStruct.
add a new typedef virDomainBlockStatsStructV2 that maps to the
modified virDomainBlockStatsStruct.
modify struct virDomainBlockStatsStruct to add new fields, but
_only_ at the end of the struct - this struct could later grow if we
need to go to a v3.
I would be tempted to keep virDomainBlockStatsStruct as-is,
just add virDomainBlockStatsStructV2 as the larger one. That keeps
the default the small structure, and minimize the risk of
incompatibilities.
Leave virDomainBlockStatsPtr as a typedef for
*virDomainBlockStatsStruct.
In libvirt.c:
int
virDomainBlockStats (virDomainPtr dom, const char *path,
virDomainBlockStatsPtr stats, size_t size)
{
virConnectPtr conn;
struct _virDomainBlockStatsStructV1 statsv1 = { -1, -1, -1, -1, -1 };
just struct _virDomainBlockStatsStruct statsv1 = ...
struct _virDomainBlockStatsStructV2 statsv2 = { -1, -1, -1, -1,
-1, -1 ... };
VIR_DOMAIN_DEBUG
...
if (!path || !stats || size > sizeof(statsv2)) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
conn = dom->conn;
if (conn->driver->domainBlockStatsv2) {
if (conn->driver->domainBlockStatsv2 (dom, path, &statsv2) == -1)
goto error;
memcpy (stats, &statsv2, size);
return 0;
}
if (conn->driver->domainBlockStats) {
if (conn->driver->domainBlockStats (dom, path, &statsv1) == -1)
goto error;
if (size > sizeof(statsv1)) {
memset(((char *)stats)+sizeof(statsv1), -1, size -
sizeof(statsv1));
size = sizeof(statsv1);
}
memcpy (stats, &statsv1, size);
return 0;
}
...
In driver.h:
Update the domainBlockStats callback to explicitly take
*virDomainBlockStatsStructV1, and add a new domainBlockStats2
callback that explicitly takes *virDomainBlockStatsStructV2.
I would not change the old entry point, just add the new one
In remote_protocol.x, add a new RPC for domain_block_stats_2 that
passes the larger struct over the wire; leaving the existing RPC
tied to the smaller v1 struct.
agreed
In qemu_driver.c, implement the new driver callback as the only way
to get at the additional fields.
agreed too
This proposal is ABI safe (no function signatures changed and no new
entry points are added - all the decisions on how many bytes to
populate are based on an existing argument, and all APIs used
pointers which are constant size rather than copies where the larger
struct would cause problems). However, it has the following
effects, including one slight API ramification:
Well it does change the signature, in a compatible way ... but. Which
is why I would drop the notion of a V1 and just add a V2 structure (and
associated pointers).
1. all existing clients that were pre-compiled against the older
libvirt.h should have already been calling
virDomainBlockStats(...,sizeof(virDomainBlockStatsStruct)), where
the compilation was done with the struct at the smaller size. This
will safely call the older RPC, and skip out on any new fields,
whether talking to an old or new server.
2. recompiling an existing client against newer libvirt.h with no
other changes will automatically pick up the larger struct. The
sizeof() argument will change. Newer servers will recognize the
larger struct and automatically call the newer RPC, getting all the
new fields. However, older servers will now reject the larger sizeof
as too large - this is a subtle limitation, if clients are
recompiled without being aware of the difference it will cause!
that I'm not sure about being the right way
3. recompiling an existing client with a slight change
(s/virDomainBlockStatsPtr/virDomainBlockStatsStructV1Ptr/g) will
force the smaller sizeof(), so that the client can continue to talk
to older servers, but will lose out on the new fields in the v2
struct. This is the mitigation to use to avoid the breakage in
point 2.
Agreed
4. writing a new client that only has to target 0.9.5 or newer
should explicitly use virDomainBlockStatsStructV2Ptr, so that the
client will safely talk to newer servers and gracefully be rejected
by older servers. And by explicitly using the v2 struct instead of
the generic virDomainBlockStatsPtr, the client has protected itself
from a recompilation to a future v3 of virDomainBlockStatsPtr
causing the 0.9.5 support to stop working (that is, by exposing all
three types: V1, V2, and generic, an application can decide whether
recompilation should be tied to a specific struct level, or to the
latest version available).
Agreed
If we decide that point 2 is too onerous (that is, if we want to
guarantee that recompilation of existing clients against newer
libvirt.h will not break the ability of that client to talk to older
servers), then we can slightly tweak the changes made in
libvirt.h.in so as to keep virDomainBlockStatsPtr tied to the v1
struct size, and only use new type names for larger structs.
yeah I would keep V1 the default and avoid 2/
There is a small problem in this scheme though: the python bindings
they don't take a structure as a parameter so there is no way to
distinguish, I would just explicitely get a
class virDomain:
[...]
def blockStatsV2(self, path):
"""Extracts extended block device statistics for a domain
"""
ret = libvirtmod.virDomainBlockStatsV2(self._o, path)
if ret is None: raise libvirtError ('virDomainBlockStats() failed',
dom=self)
return ret
from the generator, and in python/libvirt-override.c provide an
explicit entry point
libvirt_virDomainBlockStatsV2()
using virDomainBlockStatsStructV2 statsv2; and using that for the call
That is API clean except for python but I don't think it's a problem
since it's limited to the interaction between libvirtmod.so and
libvirt.py so fairly well defined. The python client wanting the
extra data will need to call the new method but that's Okay I think
Next question is what additional fields to provide? We don't
want
to make a v3 any sooner than needed, so v2 should have more than
just latency, but should also include things like cdrom tray status.
Remember that the public API documents that all stats not supported
for a given drive by the given hypervisor will be set to -1, so it
doesn't hurt to add more fields than what can be populated, so long
as at least one combination of hypervisor and underlying block type
can usefully use that field.
I'm tempted to add some provision for extra data by packing an
long long unused [20] at the end of the struct, that we could use
later once we find the need for it without requiring protocol
or structure extensions,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
11:10 a.m.
New subject: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats
On Wed, Aug 17, 2011 at 09:39:34AM -0600, Eric Blake wrote:
On 08/12/2011 07:53 AM, Daniel P. Berrange wrote:
>On Fri, Aug 12, 2011 at 10:17:24PM +0800, Osier Yang wrote:
>>The new disk latency related members include:
>> rd_total_times
>> wr_total_times
>> flush_operations
>> flush_total_times
>>---
>> include/libvirt/libvirt.h.in | 4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>>diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>>index b1bda31..cf6bf60 100644
>>--- a/include/libvirt/libvirt.h.in
>>+++ b/include/libvirt/libvirt.h.in
>>@@ -562,8 +562,12 @@ typedef struct _virDomainBlockStats
virDomainBlockStatsStruct;
>> struct _virDomainBlockStats {
>> long long rd_req; /* number of read requests */
>> long long rd_bytes; /* number of read bytes */
>>+ long long rd_total_times; /* total time spend on cache reads in nano-seconds
*/
>> long long wr_req; /* number of write requests */
>> long long wr_bytes; /* number of written bytes */
>>+ long long wr_total_times; /* total time spend on cache writes in nano-seconds
*/
>>+ long long flush_req; /* number of flushed bytes */
>>+ long long flush_total_times; /* total time spend on cache flushes in
nano-seconds */
>> long long errs; /* In Xen this returns the mysterious 'oo_req'. */
>> };
>
>NACK, it is forbidden to modify any structs in the public header
>files since that changes the ABI.
However, what we CAN do, without breaking ABI, is the following:
In libvirt.h.in:
add a new struct virDomainBlockStatsStructV1 with layout identical
to the old virDomainBlockStatsStruct.
add a new typedef virDomainBlockStatsStructV2 that maps to the
modified virDomainBlockStatsStruct.
modify struct virDomainBlockStatsStruct to add new fields, but
_only_ at the end of the struct - this struct could later grow if we
need to go to a v3.
Leave virDomainBlockStatsPtr as a typedef for *virDomainBlockStatsStruct.
In libvirt.c:
int
virDomainBlockStats (virDomainPtr dom, const char *path,
virDomainBlockStatsPtr stats, size_t size)
{
virConnectPtr conn;
struct _virDomainBlockStatsStructV1 statsv1 = { -1, -1, -1, -1, -1 };
struct _virDomainBlockStatsStructV2 statsv2 = { -1, -1, -1, -1,
-1, -1 ... };
VIR_DOMAIN_DEBUG
...
if (!path || !stats || size > sizeof(statsv2)) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
conn = dom->conn;
if (conn->driver->domainBlockStatsv2) {
if (conn->driver->domainBlockStatsv2 (dom, path, &statsv2) == -1)
goto error;
memcpy (stats, &statsv2, size);
return 0;
}
if (conn->driver->domainBlockStats) {
if (conn->driver->domainBlockStats (dom, path, &statsv1) == -1)
goto error;
if (size > sizeof(statsv1)) {
memset(((char *)stats)+sizeof(statsv1), -1, size -
sizeof(statsv1));
size = sizeof(statsv1);
}
memcpy (stats, &statsv1, size);
return 0;
}
...
In driver.h:
Update the domainBlockStats callback to explicitly take
*virDomainBlockStatsStructV1, and add a new domainBlockStats2
callback that explicitly takes *virDomainBlockStatsStructV2.
In remote_protocol.x, add a new RPC for domain_block_stats_2 that
passes the larger struct over the wire; leaving the existing RPC
tied to the smaller v1 struct.
In qemu_driver.c, implement the new driver callback as the only way
to get at the additional fields.
This proposal is ABI safe (no function signatures changed and no new
entry points are added - all the decisions on how many bytes to
populate are based on an existing argument, and all APIs used
pointers which are constant size rather than copies where the larger
struct would cause problems). However, it has the following
effects, including one slight API ramification:
1. all existing clients that were pre-compiled against the older
libvirt.h should have already been calling
virDomainBlockStats(...,sizeof(virDomainBlockStatsStruct)), where
the compilation was done with the struct at the smaller size. This
will safely call the older RPC, and skip out on any new fields,
whether talking to an old or new server.
2. recompiling an existing client against newer libvirt.h with no
other changes will automatically pick up the larger struct. The
sizeof() argument will change. Newer servers will recognize the
larger struct and automatically call the newer RPC, getting all the
new fields. However, older servers will now reject the larger sizeof
as too large - this is a subtle limitation, if clients are
recompiled without being aware of the difference it will cause!
That last point is a significant problem. While we have technically
preserved "ABI" compatibility, we have not preserved actual real
world application API usage compatibility.
I don't really like the idea of having this kind of special case
functionality in just one of our APIs, where our current policy
is always to add new APIs. While this is a no doubt a very neat
trick in terms of ABI compatibility, I think we should go for
clarity & simplicity.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
12:15 p.m.
New subject: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats
On 08/17/2011 10:10 AM, Daniel P. Berrange wrote:
> 2. recompiling an existing client against newer libvirt.h with
no
> other changes will automatically pick up the larger struct. The
> sizeof() argument will change. Newer servers will recognize the
> larger struct and automatically call the newer RPC, getting all the
> new fields. However, older servers will now reject the larger sizeof
> as too large - this is a subtle limitation, if clients are
> recompiled without being aware of the difference it will cause!
That last point is a significant problem. While we have technically
preserved "ABI" compatibility, we have not preserved actual real
world application API usage compatibility.
I don't really like the idea of having this kind of special case
functionality in just one of our APIs, where our current policy
is always to add new APIs. While this is a no doubt a very neat
trick in terms of ABI compatibility, I think we should go for
clarity& simplicity.
OK, both DV and you have convinced me that API compatibility is
important, and that we must not get in the situation of point 2 - that
is, a solution is viable only if any application that uses the type
virDomainBlockStatsPtr is always referring to the v1 struct size, and
the only way to get the larger struct is to use a new struct name.
But I'm unclear from your answer whether you are opposed to the overall
idea of reusing virDomainBlockStats() with the size argument as the key
on which struct was passed in (in which case, we need to rethink
everything about my proposal - and rebasing to a newer .so is the only
way to take advantage of the new fields), or just the idea of an API
break by how I first described the changes (in which case I am confident
that we can tweak libvirt.h.in to guarantee a naming choice that
preserves API compatibility across recompilation, while most of my
proposal stands as-is where it remains possible to backport the new
fields to older libvirtd installations since no .so entry points are
changed). DV also seemed to be in favor of reusing the existing
function call with a new struct, as long as there is no API break for
existing code. And personally, I see value in being able to add a new
struct without requiring a rebase to a new ABI.
I just audited libvirt.c, and found that the following libvirt
interfaces that take a size parameter, and could be extensible by use of
the size field:
virDomainBlockStats
virDomainInterfaceStats
The following libvirt interfaces take a flags parameter, and could be
extensible by setting a flag bit that says a larger struct is in use:
virDomainGetControlInfo
virNodeGetCPUStats
virNodeGetMemoryStats
virDomainMemoryStats
virDomainGetBlockInfo
virDomainGetBlockJobInfo
Meanwhile, the following APIs which take a pointer to a struct are
non-extensible (neither size nor flags as an escape hatch to indicate
explicit use of a larger struct), extending any of these would require a
new API function:
virDomainGetInfo
virNodeGetInfo
virDomainGetVcpus
virDomainGetSecurityLabel
virDomainGetSecurityModel
virStoragePoolGetInfo
virStorageVolGetInfo
virDomainGetJobInfo
As a final question for this mail, if we decide that reusing the
existing ABI with a new struct size is not desirable, then would you
rather that the new virDomainBlockStats2 API (or whatever we name the
new function) would also take a (non-extensible) struct, or would it be
better to rewrite it in terms of 'virTypedParameterPtr params, int
*nparams' so that all future extensibility is covered by adding
name-type-value tuples rather than dealing with future struct sizing?
I'd rather go through the pain of an ABI addition at most once, rather
than once per time that we identify new fields to add.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:40 a.m.
New subject: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats
On 08/17/2011 12:15 PM, Eric Blake wrote:
I just audited libvirt.c, and found that the following libvirt
interfaces that take a size parameter, and could be extensible by use of
the size field:
virDomainBlockStats
virDomainInterfaceStats
The following libvirt interfaces take a flags parameter, and could be
extensible by setting a flag bit that says a larger struct is in use:
virDomainGetControlInfo
virNodeGetCPUStats
virNodeGetMemoryStats
virDomainMemoryStats
virDomainMemoryStats() was designed to be regularly extended by using
the 'nr_stats' parameter. When I added this API, we had this discusson
and the design emerged specifically for the kind of extension being
discussed here. If a newer client talks to an older server, the effect
is that the client asks for more stats than are available. The old
server simply returns the stats it knows about. Wouldn't this be the
best approach to use for block stats as well? Am I missing a downside
to the virDomainMemoryStats() approach?
virDomainGetBlockInfo
virDomainGetBlockJobInfo
Meanwhile, the following APIs which take a pointer to a struct are
non-extensible (neither size nor flags as an escape hatch to indicate
explicit use of a larger struct), extending any of these would require a
new API function:
virDomainGetInfo
virNodeGetInfo
virDomainGetVcpus
virDomainGetSecurityLabel
virDomainGetSecurityModel
virStoragePoolGetInfo
virStorageVolGetInfo
virDomainGetJobInfo
--
Adam Litke
IBM Linux Technology Center
10:02 a.m.
New subject: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats
On 08/18/2011 08:40 AM, Adam Litke wrote:
> The following libvirt interfaces take a flags parameter, and
could be
> extensible by setting a flag bit that says a larger struct is in use:
> virDomainGetControlInfo
> virNodeGetCPUStats
> virNodeGetMemoryStats
> virDomainMemoryStats
virDomainMemoryStats() was designed to be regularly extended by using
the 'nr_stats' parameter.
Oh, right. In my audit, I forgot that virNodeMemoryStats is a subset of
virTypedParameter (it's hard-coded to long long stats only); any call
that uses the virTypedParameter paradigm is extensible by adding more
name-value (or name-type-value) tuples. Still, virNodeGetMemoryStats
has a flags argument, so if we ever come up with a memory stat that is
better represented as a different type than long long, we could use the
flag bit to state that virTypedParameter is in use instead.
When I added this API, we had this discusson
and the design emerged specifically for the kind of extension being
discussed here. If a newer client talks to an older server, the effect
is that the client asks for more stats than are available. The old
server simply returns the stats it knows about. Wouldn't this be the
best approach to use for block stats as well? Am I missing a downside
to the virDomainMemoryStats() approach?
No, other than the fact that it is using its own name-value pair instead
of reusing the more generic name-type-value tuple of virTypedParameter,
but I recall that we discussed that at the time and agreed to use of a
less generic type.
And your question is precisely the tradeoff we're discussing for
virDomainBlockStats - do we reuse the function call as is with a larger
size, or do we add a new function? And if we add a new function, do we
follow the virTypedParameter/virNodeMemoryStats paradigm of arbitrarily
long name-[type-]value listings, or do we follow the paradigm of passing
back a struct with every known value?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:46 p.m.
New subject: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats
于 2011年08月18日 01:15, Eric Blake 写道:
On 08/17/2011 10:10 AM, Daniel P. Berrange wrote:
>> 2. recompiling an existing client against newer libvirt.h with no
>> other changes will automatically pick up the larger struct. The
>> sizeof() argument will change. Newer servers will recognize the
>> larger struct and automatically call the newer RPC, getting all the
>> new fields. However, older servers will now reject the larger sizeof
>> as too large - this is a subtle limitation, if clients are
>> recompiled without being aware of the difference it will cause!
>
> That last point is a significant problem. While we have technically
> preserved "ABI" compatibility, we have not preserved actual real
> world application API usage compatibility.
>
> I don't really like the idea of having this kind of special case
> functionality in just one of our APIs, where our current policy
> is always to add new APIs. While this is a no doubt a very neat
> trick in terms of ABI compatibility, I think we should go for
> clarity& simplicity.
OK, both DV and you have convinced me that API compatibility is
important, and that we must not get in the situation of point 2 - that
is, a solution is viable only if any application that uses the type
virDomainBlockStatsPtr is always referring to the v1 struct size, and
the only way to get the larger struct is to use a new struct name.
But I'm unclear from your answer whether you are opposed to the
overall idea of reusing virDomainBlockStats() with the size argument
as the key on which struct was passed in (in which case, we need to
rethink everything about my proposal - and rebasing to a newer .so is
the only way to take advantage of the new fields), or just the idea of
an API break by how I first described the changes (in which case I am
confident that we can tweak libvirt.h.in to guarantee a naming choice
that preserves API compatibility across recompilation, while most of
my proposal stands as-is where it remains possible to backport the new
fields to older libvirtd installations since no .so entry points are
changed). DV also seemed to be in favor of reusing the existing
function call with a new struct, as long as there is no API break for
existing code. And personally, I see value in being able to add a new
struct without requiring a rebase to a new ABI.
I just audited libvirt.c, and found that the following libvirt
interfaces that take a size parameter, and could be extensible by use
of the size field:
virDomainBlockStats
virDomainInterfaceStats
The following libvirt interfaces take a flags parameter, and could be
extensible by setting a flag bit that says a larger struct is in use:
virDomainGetControlInfo
virNodeGetCPUStats
virNodeGetMemoryStats
virDomainMemoryStats
virDomainGetBlockInfo
virDomainGetBlockJobInfo
Meanwhile, the following APIs which take a pointer to a struct are
non-extensible (neither size nor flags as an escape hatch to indicate
explicit use of a larger struct), extending any of these would require
a new API function:
virDomainGetInfo
virNodeGetInfo
virDomainGetVcpus
virDomainGetSecurityLabel
virDomainGetSecurityModel
virStoragePoolGetInfo
virStorageVolGetInfo
virDomainGetJobInfo
As a final question for this mail, if we decide that reusing the
existing ABI with a new struct size is not desirable, then would you
rather that the new virDomainBlockStats2 API (or whatever we name the
new function) would also take a (non-extensible) struct, or would it
be better to rewrite it in terms of 'virTypedParameterPtr params, int
*nparams' so that all future extensibility is covered by adding
name-type-value tuples rather than dealing with future struct sizing?
I'd rather go through the pain of an ABI addition at most once, rather
than once per time that we identify new fields to add.
Vote for the second method (rewrite the struct in terms of
virTypedParameterPtr).
Also we might need to use virTypedParameterPtr when implementing v2 APIs for
these functions?
virDomainGetInfo
virNodeGetInfo
virDomainGetVcpus
virDomainGetSecurityLabel
virDomainGetSecurityModel
virStoragePoolGetInfo
virStorageVolGetInfo
virDomainGetJobInfo
Osier
9:29 a.m.
New subject: [libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats
On Wed, Aug 17, 2011 at 11:15:29AM -0600, Eric Blake wrote:
But I'm unclear from your answer whether you are opposed to the
overall idea of reusing virDomainBlockStats() with the size argument
as the key on which struct was passed in [...]
I will just say that (rightly or wrongly) the size field was designed
to allow the struct to be increased in size in future.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
9:17 a.m.
New subject: [libvirt] [PATCH 2/4] latency: Parse the new disk latency keys in qemu monitor functions
---
src/qemu/qemu_monitor.c | 12 ++++++++++++
src/qemu/qemu_monitor.h | 4 ++++
src/qemu/qemu_monitor_json.c | 36 +++++++++++++++++++++++++++++++++++-
src/qemu/qemu_monitor_json.h | 5 ++++-
src/qemu/qemu_monitor_text.c | 39 +++++++++++++++++++++++++++++----------
src/qemu/qemu_monitor_text.h | 4 ++++
6 files changed, 88 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index db6107c..aac4b21 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1201,8 +1201,12 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon,
const char *devname,
long long *rd_req,
long long *rd_bytes,
+ long long *rd_total_times,
long long *wr_req,
long long *wr_bytes,
+ long long *wr_total_times,
+ long long *flush_req,
+ long long *flush_total_times,
long long *errs)
{
int ret;
@@ -1217,12 +1221,20 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon,
if (mon->json)
ret = qemuMonitorJSONGetBlockStatsInfo(mon, devname,
rd_req, rd_bytes,
+ rd_total_times,
wr_req, wr_bytes,
+ wr_total_times,
+ flush_req,
+ flush_total_times,
errs);
else
ret = qemuMonitorTextGetBlockStatsInfo(mon, devname,
rd_req, rd_bytes,
+ rd_total_times,
wr_req, wr_bytes,
+ wr_total_times,
+ flush_req,
+ flush_total_times,
errs);
return ret;
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index f241c9e..d7695e9 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -212,8 +212,12 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon,
const char *devname,
long long *rd_req,
long long *rd_bytes,
+ long long *rd_total_times,
long long *wr_req,
long long *wr_bytes,
+ long long *wr_total_times,
+ long long *flush_req,
+ long long *flush_total_times,
long long *errs);
int qemuMonitorGetBlockExtent(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 2a9a078..c917978 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1318,8 +1318,12 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
const char *devname,
long long *rd_req,
long long *rd_bytes,
+ long long *rd_total_times,
long long *wr_req,
long long *wr_bytes,
+ long long *wr_total_times,
+ long long *flush_req,
+ long long *flush_total_times,
long long *errs)
{
int ret;
@@ -1330,7 +1334,9 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
virJSONValuePtr reply = NULL;
virJSONValuePtr devices;
- *rd_req = *rd_bytes = *wr_req = *wr_bytes = *errs = 0;
+ *rd_req = *rd_bytes = *rd_total_times = 0;
+ *wr_req = *wr_bytes = *wr_total_times = 0;
+ *flush_req = *flush_total_times = *errs = 0;
if (!cmd)
return -1;
@@ -1396,6 +1402,13 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
"rd_operations");
goto cleanup;
}
+ if (virJSONValueObjectGetNumberLong(stats, "rd_total_times_ns",
+ rd_total_times) < 0) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "rd_total_times_ns");
+ goto cleanup;
+ }
if (virJSONValueObjectGetNumberLong(stats, "wr_bytes", wr_bytes) <
0) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot read %s statistic"),
@@ -1408,6 +1421,27 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
"wr_operations");
goto cleanup;
}
+ if (virJSONValueObjectGetNumberLong(stats, "wr_total_times_ns",
+ wr_total_times) < 0) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "wr_total_times_ns");
+ goto cleanup;
+ }
+ if (virJSONValueObjectGetNumberLong(stats, "flush_operations",
+ flush_req) < 0) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "flush_operations");
+ goto cleanup;
+ }
+ if (virJSONValueObjectGetNumberLong(stats, "flush_total_times_ns",
+ flush_total_times) < 0) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "flush_total_times_ns");
+ goto cleanup;
+ }
}
if (!found) {
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 9512793..a1ab39d 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -64,14 +64,17 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
const char *devname,
long long *rd_req,
long long *rd_bytes,
+ long long *rd_total_times,
long long *wr_req,
long long *wr_bytes,
+ long long *wr_total_times,
+ long long *flush_req,
+ long long *flush_total_times,
long long *errs);
int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon,
const char *devname,
unsigned long long *extent);
-
int qemuMonitorJSONSetVNCPassword(qemuMonitorPtr mon,
const char *password);
int qemuMonitorJSONSetPassword(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 7bf733d..fbea855 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -674,8 +674,12 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon,
const char *devname,
long long *rd_req,
long long *rd_bytes,
+ long long *rd_total_times,
long long *wr_req,
long long *wr_bytes,
+ long long *wr_total_times,
+ long long *flush_req,
+ long long *flush_total_times,
long long *errs)
{
char *info = NULL;
@@ -702,11 +706,9 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon,
goto cleanup;
}
- *rd_req = -1;
- *rd_bytes = -1;
- *wr_req = -1;
- *wr_bytes = -1;
- *errs = -1;
+ *rd_req = *rd_bytes = *rd_total_times = -1;
+ *wr_req = *wr_bytes = *wr_total_times = -1;
+ *flush_req = *flush_total_times = *errs = -1;
/* The output format for both qemu & KVM is:
* blockdevice: rd_bytes=% wr_bytes=% rd_operations=% wr_operations=%
@@ -734,23 +736,40 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon,
while (*p) {
if (STRPREFIX (p, "rd_bytes=")) {
- p += 9;
+ p += strlen("rd_bytes");
if (virStrToLong_ll (p, &dummy, 10, rd_bytes) == -1)
VIR_DEBUG ("error reading rd_bytes: %s", p);
} else if (STRPREFIX (p, "wr_bytes=")) {
- p += 9;
+ p += strlen("wr_bytes");
if (virStrToLong_ll (p, &dummy, 10, wr_bytes) == -1)
VIR_DEBUG ("error reading wr_bytes: %s", p);
} else if (STRPREFIX (p, "rd_operations=")) {
- p += 14;
+ p += strlen("rd_operations");
if (virStrToLong_ll (p, &dummy, 10, rd_req) == -1)
VIR_DEBUG ("error reading rd_req: %s", p);
} else if (STRPREFIX (p, "wr_operations=")) {
- p += 14;
+ p += strlen("wr_operations=");
if (virStrToLong_ll (p, &dummy, 10, wr_req) == -1)
VIR_DEBUG ("error reading wr_req: %s", p);
- } else
+ } else if (STRPREFIX (p, "rd_total_times_ns=")) {
+ p += strlen("rd_total_times_ns=");
+ if (virStrToLong_ll (p, &dummy, 10, rd_total_times) == -1)
+ VIR_DEBUG ("error reading rd_total_times: %s", p);
+ } else if (STRPREFIX (p, "wr_total_times_ns=")) {
+ p += strlen("wr_total_times_ns=");
+ if (virStrToLong_ll (p, &dummy, 10, wr_total_times) == -1)
+ VIR_DEBUG ("error reading wr_total_times: %s", p);
+ } else if (STRPREFIX (p, "flush_operations=")) {
+ p += strlen("flush_operations=");
+ if (virStrToLong_ll (p, &dummy, 10, flush_req) == -1)
+ VIR_DEBUG ("error reading flush_req: %s", p);
+ } else if (STRPREFIX (p, "flush_total_times_ns=")) {
+ p += strlen("flush_total_times_ns=");
+ if (virStrToLong_ll (p, &dummy, 10, flush_total_times) == -1)
+ VIR_DEBUG ("error reading flush_total_times: %s", p);
+ } else {
VIR_DEBUG ("unknown block stat near %s", p);
+ }
/* Skip to next label. */
p = strchr (p, ' ');
diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index b250738..f7d9f5c 100644
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -61,8 +61,12 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon,
const char *devname,
long long *rd_req,
long long *rd_bytes,
+ long long *rd_total_times,
long long *wr_req,
long long *wr_bytes,
+ long long *wr_total_times,
+ long long *flush_req,
+ long long *flush_total_times,
long long *errs);
int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon,
const char *devname,
--
1.7.6
9:17 a.m.
New subject: [libvirt] [PATCH 3/4] latency: Update handler in qemu driver.
* src/qemu/qemu_driver.c: Update function qemudDomainBlockStats to
support the new disk latency related members.
---
src/qemu/qemu_driver.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ce19be7..36d0d3f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6801,8 +6801,12 @@ qemudDomainBlockStats (virDomainPtr dom,
disk->info.alias,
&stats->rd_req,
&stats->rd_bytes,
+ &stats->rd_total_times,
&stats->wr_req,
&stats->wr_bytes,
+ &stats->wr_total_times,
+ &stats->flush_req,
+ &stats->flush_total_times,
&stats->errs);
qemuDomainObjExitMonitor(driver, vm);
--
1.7.6
9:17 a.m.
New subject: [libvirt] [PATCH 4/4] latency: Update virsh command domblkstat to support the new members
---
tools/virsh.c | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index b053ed0..555f278 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1082,12 +1082,25 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd)
if (stats.rd_bytes >= 0)
vshPrint (ctl, "%s rd_bytes %lld\n", device, stats.rd_bytes);
+ if (stats.rd_total_times >= 0)
+ vshPrint (ctl, "%sns rd_total_times %lld\n", device,
stats.rd_total_times);
+
if (stats.wr_req >= 0)
vshPrint (ctl, "%s wr_req %lld\n", device, stats.wr_req);
if (stats.wr_bytes >= 0)
vshPrint (ctl, "%s wr_bytes %lld\n", device, stats.wr_bytes);
+ if (stats.wr_total_times >= 0)
+ vshPrint (ctl, "%sns wr_total_times %lld\n", device,
stats.wr_total_times);
+
+ if (stats.flush_req >= 0)
+ vshPrint (ctl, "%s flush_req %lld\n", device, stats.flush_req);
+
+ if (stats.flush_total_times >= 0)
+ vshPrint (ctl, "%sns flush_total_times %lld\n", device,
+ stats.flush_total_times);
+
if (stats.errs >= 0)
vshPrint (ctl, "%s errs %lld\n", device, stats.errs);
--
1.7.6
4841
days inactive
4851
days old
13 comments
6 participants
participants (6)
-
Adam Litke -
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Osier Yang -
Richard W.M. Jones