On 12/16/14 09:04, 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:
>
> 1. We are calling stat and opening a file on every invocation of
> the API. However, there are cases where the stats should NOT be
> changing between successive calls (if a domain is running, no
> one should be changing the physical size of a block device or raw
> image behind our backs; capacity of read-only files should not
> be changing; and we are the gateway to the block-resize command
> to know when the capacity of read-write files should be changing).
> True, we still have to use stat in some cases (a sparse raw file
> changes allocation if it is read-write and the amount of holes is
> changing, and a read-write qcow2 image stored in a file changes
> physical size if it was not fully pre-allocated). But for
> read-only images, even this should be something we can remember
> from the previous time, rather than repeating every call.
>
> 2. We want to enhance the power of virDomainListGetStats, by
> sharing code. But we already have a virStorageSourcePtr for
> each disk, and it would be easier to reuse the common structure
> than to have to worry about the one-off virDomainBlockInfoPtr.
>
> While this patch does not optimize reuse of information in point
> 1, it does get us closer to being able to do so; by updating a
> structure that survives between consecutive calls.
>
> * src/util/virstoragefile.h (_virStorageSource): Add physical, to
> mirror virDomainBlockInfo; rearrange fields to match public struct.
> (virStorageSourceCopy): Copy the new field.
> * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Store into
> storage source, then copy to block info.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++++++++++--------
> src/util/virstoragefile.c | 3 ++-
> src/util/virstoragefile.h | 3 ++-
> 3 files changed, 38 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8775ab2..b13c5e1 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11045,6 +11045,26 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
>
> disk = vm->def->disks[idx];
>
> + /* FIXME: For an offline domain, we always want to check current
> + * on-disk statistics (as users have been known to change offline
> + * images behind our backs). For a running domain, however, it
> + * would be nice to avoid opening a file (particularly since
> + * reading a file while qemu is writing it risks the reader seeing
> + * bogus data), or even avoid a stat, if the information
> + * remembered from the previous run is still viable.
> + *
> + * 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.
> + */
I still don't see this comment vanishing in this series. Are you planing
on getting rid of the code that opens the file on disk while the VM is
alive?
> if (virStorageSourceIsLocalStorage(disk->src)) {
> if (!disk->src->path) {
> virReportError(VIR_ERR_INVALID_ARG,
ACK
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list