[libvirt] [PATCH v2 05/12] getstats: rearrange blockinfo gathering

[Eric Blake]

On 12/16/2014 04:33 PM, Eric Blake wrote:
> On 12/16/2014 06:54 AM, Peter Krempa wrote:
>> On 12/16/14 09:04, Eric Blake wrote:
>>> We want to avoid read()ing a file while qemu is running.  We still
>>> have to open() block devices to determine their physical size, but
>>> that is safer.  This patch rearranges code to make it easier to
>>> skip the metadata collection when possible.
>>>
>>> * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Check for empty
>>> disk up front.  Perform stat first.  Place metadata reading next
>>> to use.
>>>
> 
>>> +        if (stat(disk->src->path, &sb) < 0) {
>>> +            virReportSystemError(errno,
>>> +                                 _("cannot stat file '%s'"), disk->src->path);
>>>              goto endjob;
>>>          }
>>
>> Um this will cause problems on NFS. The code after that that you are
>> moving uses the FD to do the stat() call. The fd is opened by the helper
>> that opens the file with correct perms.
> 
> Oh, you're right.  I'll see how hairy it is to pull this patch out of
> the current series, and save the rework of avoiding open()ing the files
> to later (I may end up with a situation that is less efficient, by
> possibly open()ing some files twice for offline domains, once for
> capacity, and once for determining the last offset of a block device,
> instead of the current code that tries to do both from a single fd but
> becomes harder to untangle).  1-4 are pushed, and I'm seeing what
> happens to the rest of the series if I leave this out for now...
> 

>> Otherwise looks okay.
>> 
>> Peter
> 

Turned out to be a little bit hairy; what I ended up pushing was JUST
the refactoring for earlier checking for empty images and later grouping
of read-related code, leaving the open-code untouched for later:

diff --git c/src/qemu/qemu_driver.c i/src/qemu/qemu_driver.c
index 75ea218..3d81a5b 100644
--- c/src/qemu/qemu_driver.c
+++ i/src/qemu/qemu_driver.c
@@ -11057,6 +11057,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,

     disk = vm->def->disks[idx];
     src = disk->src;
+    if (virStorageSourceIsEmpty(src)) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("disk '%s' does not currently have a source
assigned"),
+                       path);
+        goto endjob;
+    }

     /* FIXME: For an offline domain, we always want to check current
      * on-disk statistics (as users have been known to change offline
@@ -11079,13 +11085,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
      * 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, src->path, O_RDONLY,
                                NULL, NULL)) == -1)
             goto endjob;
@@ -11116,26 +11115,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
         }
     }

-    /* Probe for magic formats */
-    if (virDomainDiskGetFormat(disk)) {
-        format = virDomainDiskGetFormat(disk);
-    } else {
-        if (!cfg->allowDiskFormatProbing) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("no disk format for %s and probing is
disabled"),
-                           path);
-            goto endjob;
-        }
-
-        if ((format = virStorageFileProbeFormatFromBuf(src->path,
-                                                       buf, len)) < 0)
-            goto endjob;
-    }
-
-    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
@@ -11164,6 +11143,21 @@ qemuDomainGetBlockInfo(virDomainPtr dom,

     /* If the file we probed has a capacity set, then override
      * what we calculated from file/block extents */
+    if (!(format = src->format)) {
+        if (!cfg->allowDiskFormatProbing) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("no disk format for %s and probing is
disabled"),
+                           path);
+            goto endjob;
+        }
+
+        if ((format = virStorageFileProbeFormatFromBuf(src->path,
+                                                       buf, len)) < 0)
+            goto endjob;
+    }
+    if (!(meta = virStorageFileGetMetadataFromBuf(src->path, buf, len,
+                                                  format, NULL)))
+        goto endjob;
     if (meta->capacity)
         src->capacity = meta->capacity;



-- 
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/20141216/25fa5415/attachment-0001.sig>