Dan Kenigsberg has posted comments on this change.

Change subject: vm: increase the volume extension on storage migration
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(3 inline comments)

minor comments.

....................................................
File vdsm/libvirtvm.py
Line 1038:         return iface
Line 1039: 
Line 1040: 
Line 1041: class Drive(LibvirtVmDevice):
Line 1042:     VOLWM_CHUNK_MB = config.getint('irs', 
'volume_utilization_chunk_mb')
what does VOLWM stand for?
Line 1043:     VOLWM_FREE_PCT = 100 - config.getint('irs', 
'volume_utilization_percent')
Line 1044:     VOLWM_CHUNK_REPLICATE_MULT = 2  # Chunk multiplier during 
replication
Line 1045: 
Line 1046:     def __init__(self, conf, log, **kwargs):


Line 1078:         Returns the watermark limit, when the LV usage reaches this 
limit an
Line 1079:         extension is in order (thin provisioning on block devices).
Line 1080:         """
Line 1081:         return (self.VOLWM_FREE_PCT * self.volExtensionChunk *
Line 1082:                 (constants.MEGAB / 100))
The original calculation, avoiding the parenthesis, is slightly more exact. 
"100" divides the bigger number, not MEGAB. I see no reason to change that in 
this patch.
Line 1083: 
Line 1084:     def getNextVolumeExtSize(self):
Line 1085:         """
Line 1086:         Returns the next volume size in megabytes. This value is 
based on the


Line 1080:         """
Line 1081:         return (self.VOLWM_FREE_PCT * self.volExtensionChunk *
Line 1082:                 (constants.MEGAB / 100))
Line 1083: 
Line 1084:     def getNextVolumeExtSize(self):
the name confused me to think you are calculating the size of the next 
extension. actually it is is the next target size for the volume. I'd drop the 
Ext.
Line 1085:         """
Line 1086:         Returns the next volume size in megabytes. This value is 
based on the
Line 1087:         volExtensionChunk property and it's the size that should be 
requested
Line 1088:         for the next LV extension.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib61375613712feb7118a80c50b73e678d257f251
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to