[libvirt] [PATCH 1/4] latency: Introduce new members for _virDomainBlockStats

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org