qemu: let blockinfo reuse virStorageSource

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@redhat.com>

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9b399d025946c1b159fb17e3735dc841920e2768..f0d7af70df05c098a472e7a81046db7c77449eb0 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11016,6 +11016,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
     off_t end;
     virStorageSourcePtr meta = NULL;
     virDomainDiskDefPtr disk = NULL;
+    virStorageSourcePtr src;
     struct stat sb;
     int idx;
     int format;
@@ -11055,41 +11056,62 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
     }
 
     disk = vm->def->disks[idx];
-
-    if (virStorageSourceIsLocalStorage(disk->src)) {
-        if (!disk->src->path) {
+    src = disk->src;
+
+    /* 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
+     * reading 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.
+     */
+    if (virStorageSourceIsLocalStorage(src)) {
+        if (!src->path) {
             virReportError(VIR_ERR_INVALID_ARG,
                            _("disk '%s' does not currently have a source assigned"),
                            path);
             goto endjob;
         }
 
-        if ((fd = qemuOpenFile(driver, vm, disk->src->path, O_RDONLY,
+        if ((fd = qemuOpenFile(driver, vm, src->path, O_RDONLY,
                                NULL, NULL)) == -1)
             goto endjob;
 
         if (fstat(fd, &sb) < 0) {
             virReportSystemError(errno,
-                                 _("cannot stat file '%s'"), disk->src->path);
+                                 _("cannot stat file '%s'"), src->path);
             goto endjob;
         }
 
         if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) {
             virReportSystemError(errno, _("cannot read header '%s'"),
-                                 disk->src->path);
+                                 src->path);
             goto endjob;
         }
     } else {
-        if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0)
+        if (virStorageFileInitAs(src, cfg->user, cfg->group) < 0)
             goto endjob;
 
-        if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER,
+        if ((len = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER,
                                             &buf)) < 0)
             goto endjob;
 
-        if (virStorageFileStat(disk->src, &sb) < 0) {
+        if (virStorageFileStat(src, &sb) < 0) {
             virReportSystemError(errno, _("failed to stat remote file '%s'"),
-                                 NULLSTR(disk->src->path));
+                                 NULLSTR(src->path));
             goto endjob;
         }
     }
@@ -11105,27 +11127,27 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
             goto endjob;
         }
 
-        if ((format = virStorageFileProbeFormatFromBuf(disk->src->path,
+        if ((format = virStorageFileProbeFormatFromBuf(src->path,
                                                        buf, len)) < 0)
             goto endjob;
     }
 
-    if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
+    if (!(meta = virStorageFileGetMetadataFromBuf(src->path, buf, len,
                                                   format, NULL)))
         goto endjob;
 
     /* Get info for normal formats */
     if (S_ISREG(sb.st_mode) || fd == -1) {
 #ifndef WIN32
-        info->physical = (unsigned long long)sb.st_blocks *
+        src->physical = (unsigned long long)sb.st_blocks *
             (unsigned long long)DEV_BSIZE;
 #else
-        info->physical = sb.st_size;
+        src->physical = sb.st_size;
 #endif
         /* Regular files may be sparse, so logical size (capacity) is not same
          * as actual physical above
          */
-        info->capacity = sb.st_size;
+        src->capacity = sb.st_size;
     } else {
         /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
          * be 64 bits on all platforms.
@@ -11136,22 +11158,22 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
                                  _("failed to seek to end of %s"), path);
             goto endjob;
         }
-        info->physical = end;
-        info->capacity = end;
+        src->physical = end;
+        src->capacity = end;
     }
 
     /* If the file we probed has a capacity set, then override
      * what we calculated from file/block extents */
     if (meta->capacity)
-        info->capacity = meta->capacity;
+        src->capacity = meta->capacity;
 
     /* Set default value .. */
-    info->allocation = info->physical;
+    src->allocation = src->physical;
 
     /* ..but if guest is not using raw disk format and on a block device,
      * then query highest allocated extent from QEMU
      */
-    if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK &&
+    if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_BLOCK &&
         format != VIR_STORAGE_FILE_RAW &&
         S_ISBLK(sb.st_mode)) {
         qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -11168,13 +11190,19 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
         qemuDomainObjEnterMonitor(driver, vm);
         ret = qemuMonitorGetBlockExtent(priv->mon,
                                         disk->info.alias,
-                                        &info->allocation);
+                                        &src->allocation);
         qemuDomainObjExitMonitor(driver, vm);
 
     } else {
         ret = 0;
     }
 
+    if (ret == 0) {
+        info->capacity = src->capacity;
+        info->allocation = src->allocation;
+        info->physical = src->physical;
+    }
+
  endjob:
     if (!qemuDomainObjEndJob(driver, vm))
         vm = NULL;

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 9efe7480d04b5f973dada9ec2cd5b0fb1ba65ff4..2be6c34e8784794cadaee87d1b63c1f4590d091f 100644 (file)
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1824,8 +1824,9 @@ virStorageSourceCopy(const virStorageSource *src,
     ret->type = src->type;
     ret->protocol = src->protocol;
     ret->format = src->format;
-    ret->allocation = src->allocation;
     ret->capacity = src->capacity;
+    ret->allocation = src->allocation;
+    ret->physical = src->physical;
     ret->readonly = src->readonly;
     ret->shared = src->shared;
 

diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index e05b8438b45004d0ff1e9ac6620fc4fe778f8bb3..b4c38086831d9309cfc9768227184e9e39388ff1 100644 (file)
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -257,8 +257,9 @@ struct _virStorageSource {
 
     virStoragePermsPtr perms;
     virStorageTimestampsPtr timestamps;
-    unsigned long long allocation; /* in bytes, 0 if unknown */
     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 */
     size_t nseclabels;
     virSecurityDeviceLabelDefPtr *seclabels;