11:06 a.m.
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/