[libvirt] [PATCH v2 12/12] getstats: start crawling backing chain for qemu

[Eric Blake]

On 12/16/2014 08:45 AM, Peter Krempa wrote:
> On 12/16/14 09:04, Eric Blake wrote:
>> Wire up backing chain recursion.  Note that for now, we just use
>> the same allocation numbers for read-only backing files as what
>> offline domains would report.  It is not the correct allocation
>> number for qcow2 over block devices during block-commit, and it

Stale comments; I'll update them.

>> misses out on the fact that qemu also reports read statistics on
>> backing files that are worth knowing about (seriously - for a
>> thin-provisioned setup, it would be nice to easily get at a count
>> of how many reads were serviced from the backing file in relation
>> to reads serviced by the active layer).  But it is at least
>> sufficient to prove that the algorithm is working, and to let
>> other people start coding to the interface while waiting for
>> later patches that get the correct information.
>>

>> $ virsh domstats --block --backing testvm2
>> Domain: 'testvm2'
>>   block.count=3
>>   block.0.name=vda
>>   block.0.path=/tmp/wrapper.qcow2
>>   block.0.rd.reqs=1
>>   block.0.rd.bytes=512
>>   block.0.rd.times=28858
>>   block.0.wr.reqs=0
>>   block.0.wr.bytes=0
>>   block.0.wr.times=0
>>   block.0.fl.reqs=0
>>   block.0.fl.times=0
>>   block.0.allocation=0
>>   block.0.capacity=1310720000
>>   block.0.physical=200704
>>   block.1.name=vda
>>   block.1.path=/dev/sda6
>>   block.1.backingIndex=1
>>   block.1.allocation=1073741824
>>   block.1.capacity=1310720000
>>   block.1.physical=1073741824

actually even longer - I get block.1.rd.reqs and so forth as well.


>> -        if (qemuDomainGetStatsOneBlock(driver, cfg, dom, record, maxparams,
>> -                                       disk, disk->src, i, abbreviated,
>> -                                       stats) < 0)
>> -            goto cleanup;
>> +        while (src && (!backing_idx || backing)) {
> 
> I'd make the special case of the first element a bit more obvious by
> using "backing_idx == 0" rather than the negation sign used usually with
> pointers.

Will fix.

> 
>> +            if (qemuDomainGetStatsOneBlock(driver, cfg, dom, record, maxparams,
>> +                                           disk, src, visited, backing_idx,
>> +                                           abbreviated, stats) < 0)
>> +                goto cleanup;
>> +            visited++;
>> +            backing_idx++;
>> +            src = src->backingStore;
>> +        }
>>      }
>>
>>      ret = 0;
> 
> As an additional thought. The stats are now extermely long for VMs with
> a long backing chain and few of the stats tend to be 0 always. We could
> start skipping them perhaps (at least for the non-top, devices)

That can be a followup patch (I agree with it, but it's too late for me
to fix tonight...)

> 
> ACK.

I've pushed 11 and 12 with fixes.  Thanks for the reviews.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141217/f41cc3be/attachment-0001.sig>