On 12/08/2014 06:03 AM, Peter Krempa wrote:
> On 12/06/14 09:14, Eric Blake wrote:
>> Right now, grabbing blockinfo always calls stat on the disk, then
>> opens the image to determine the capacity, using a throw-away
>> virStorageSourcePtr. This has a couple of drawbacks:
>>
>> + * For read-only disks, nothing should be changing unless the user
>> + * has requested a block-commit action. For read-write disks, we
>> + * know some special cases: capacity should not change without a
>> + * block-resize (where capacity is the only stat that requires
>> + * opening a file, and even then, only for non-raw files); and
>> + * physical size of a raw image or of a block device should
>> + * likewise not be changing without block-resize. On the other
>> + * hand, allocation of a raw file can change (if the file is
>> + * sparse, but the amount of sparseness changes due to writes or
>> + * punching holes), and physical size of a non-raw file can
>> + * change.
>
> For a live VM we should grab all of the above directly from the monitor
> and not ever touch the files on the disk. We do that already for the
> bulk stats and for getting the right size when doing storage migration.
Agreed. On my next spin, I'll see about getting block info for live vms
done right. (But first, I'm still in the middle of building a scratch
image with this series, if only so that we have an easier binary to play
with for integration and testing purposes).
>
> This function unfortunately is legacy code compared to the stuff I've
> pointed out
Yes, blockinfo is quite old, so here's hoping I don't break any
semantics in touching it.
>> +++ b/src/util/virstoragefile.h
>> @@ -257,8 +257,9 @@ struct _virStorageSource {
>>
>> virStoragePermsPtr perms;
>> virStorageTimestampsPtr timestamps;
>> - unsigned long long allocation; /* in bytes, 0 if unknown */
>
> Spurious move?
>
>> unsigned long long capacity; /* in bytes, 0 if unknown */
>> + unsigned long long allocation; /* in bytes, 0 if unknown */
>> + unsigned long long physical; /* in bytes, 0 if unknown */
You know, Adam asked the same question last time :)
https://www.redhat.com/archives/libvir-list/2014-November/msg00599.html
It's intentional, to match the order in the public header for block
info. But I'll update the commit message to make it obvious.
>> size_t nseclabels;
>> virSecurityDeviceLabelDefPtr *seclabels;
>>
>
> Also an addition to virStorageSourceCopy is missing.
>
> ACK with the tweak to virStorageSourceCopy
I'll probably wait on this patch until I see how the refactoring goes
for splitting offline vs. online stat gathering.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list