Change in vdsm[ovirt-3.5]: virt: Fix limit when calculating next volume size

nsoffer at redhat.com nsoffer at redhat.com
Mon Feb 23 19:39:10 UTC 2015


Hello Adam Litke, Dan Kenigsberg, Francesco Romani,

I'd like you to do a code review.  Please visit

    http://gerrit.ovirt.org/38088

to review the following change.

Change subject: virt: Fix limit when calculating next volume size
......................................................................

virt: Fix limit when calculating next volume size

Drive.getNextVolumeSize() limit was wrong in two ways. First,
Drive.truesize is *not* the drive true size on block-based drive (it
is a duplicate of Drive.apparentsize). Second, Drive.truesize is using
bytes but the code assumed that it is using megabytes. The result was
that next volume size was limited only by the vg free size.

Drive.getNextVolumeSize() requires now a capacity argument.  To prevent
the confusion with mixing different size units, we use now safer naming
convention.

Vm.extendDriveVolume() requires now a capacity argument. The capacity
was already available but unused in the drive monitoring code.

When starting live storage migration or live merge, we use now
libvirt.virDomain.blockInfo() to get drive capacity for the initial
extension.

This is a partial fix for bug 1176673. This does not fix the root cause
which seems to be regression in libvirt in RHEL 7.1 and Fedora 21, but
it limits the damage caused by this bug. Without this patch, after live
storage migration, a drive will be extended without limit until it uses
all the free space on a vg. With this patch, a drive will be extended up
to the drive virtual size, making it practically preallocated.

Change-Id: I4098dfc07184085e613f17b2c48d32e47888106c
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
Bug-Url: https://bugzilla.redhat.com/1176673
Reviewed-on: http://gerrit.ovirt.org/37726
Reviewed-by: Francesco Romani <fromani at redhat.com>
Reviewed-by: Adam Litke <alitke at redhat.com>
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
M vdsm/virt/vm.py
1 file changed, 16 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/88/38088/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 6bc66c9..5585265 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -1348,17 +1348,19 @@
         return (self.VOLWM_FREE_PCT * self.volExtensionChunk *
                 constants.MEGAB / 100)
 
-    def getNextVolumeSize(self, curSize):
+    def getNextVolumeSize(self, curSize, capacity):
         """
         Returns the next volume size in megabytes. This value is based on the
         volExtensionChunk property and it's the size that should be requested
         for the next LV extension.  curSize is the current size of the volume
         to be extended.  For the leaf volume curSize == self.apparentsize.
         For internal volumes it is discovered by calling irs.getVolumeSize().
+        capacity is the maximum size of the volume. It can be discovered using
+        libvirt.virDomain.blockInfo() or qemuimg.info().
         """
-        nextSize = (self.volExtensionChunk +
-                    ((curSize + constants.MEGAB - 1) / constants.MEGAB))
-        return min(nextSize, self.truesize)
+        curSizeMB = (curSize + constants.MEGAB - 1) / constants.MEGAB
+        nextSizeMB = curSizeMB + self.volExtensionChunk
+        return min(nextSizeMB, capacity / constants.MEGAB)
 
     @property
     def chunked(self):
@@ -2556,17 +2558,18 @@
                 "%s, capacity: %s, allocated: %s, physical: %s)",
                 volumeID, drive.domainID, drive.apparentsize, capacity,
                 alloc, physical)
-            self.extendDriveVolume(drive, volumeID, physical)
+            self.extendDriveVolume(drive, volumeID, physical, capacity)
 
         return len(extend) > 0
 
-    def extendDriveVolume(self, vmDrive, volumeID, curSize):
+    def extendDriveVolume(self, vmDrive, volumeID, curSize, capacity):
         """
         Extend drive volume and its replica volume during replication.
 
         Must be called only when the drive or the replica are chunked.
         """
-        newSize = vmDrive.getNextVolumeSize(curSize)  # newSize is in megabytes
+        # newSize is in megabytes
+        newSize = vmDrive.getNextVolumeSize(curSize, capacity)
 
         if getattr(vmDrive, 'diskReplicate', None):
             volInfo = {'poolID': vmDrive.diskReplicate['poolID'],
@@ -4588,8 +4591,10 @@
 
         if srcDrive.chunked:
             try:
-                self.extendDriveVolume(srcDrive, srcDrive.volumeID,
-                                       srcDrive.apparentsize)
+                capacity, alloc, physical = self._dom.blockInfo(
+                    srcDrive.path, 0)
+                self.extendDriveVolume(srcDrive, srcDrive.volumeID, physical,
+                                       capacity)
             except Exception:
                 self.log.exception("Initial extension request failed for %s",
                                    srcDrive.name)
@@ -5897,7 +5902,8 @@
         # plus a bit more to accomodate additional writes to 'top' during the
         # live merge operation.
         if drive.chunked:
-            self.extendDriveVolume(drive, baseVolUUID, topSize)
+            capacity, alloc, physical = self._dom.blockInfo(drive.path, 0)
+            self.extendDriveVolume(drive, baseVolUUID, topSize, capacity)
 
         # Trigger the collection of stats before returning so that callers
         # of getVmStats after this returns will see the new job


-- 
To view, visit http://gerrit.ovirt.org/38088
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4098dfc07184085e613f17b2c48d32e47888106c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>


More information about the vdsm-patches mailing list