Arik Hadas has posted comments on this change. Change subject: pad memory volume only when the storage domain is file based ......................................................................
Patch Set 3: (3 comments) http://gerrit.ovirt.org/#/c/26407/3/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 3544: 'elapsedTimeOffset': time.time() - self._startTime} Line 3545: Line 3546: def _padMemoryVolume(memoryVolPath, sdUUID): Line 3547: sdType = sd.name2type( Line 3548: self.cif.irs.getStorageDomainInfo(sdUUID)['info']['type']) > Let's not use this costly (and possibly failing) call. We already have: ok, I can switch to use it if we don't need to know the particular type of the domain in case it is file-based storage (see the comment below), but is it less costly to access the file than query "getStorageDomainInfo" ? Line 3549: if sdType in sd.FILE_DOMAIN_TYPES: Line 3550: if sdType == sd.NFS_DOMAIN: Line 3551: oop.getProcessPool(sdUUID).fileUtils. \ Line 3552: padToBlockSize(memoryVolPath) Line 3550: if sdType == sd.NFS_DOMAIN: Line 3551: oop.getProcessPool(sdUUID).fileUtils. \ Line 3552: padToBlockSize(memoryVolPath) Line 3553: else: Line 3554: fileUtils.padToBlockSize(memoryVolPath) > Why don't we need oop here? What about cifs, posix and gluster? if I recall correctly, I was asked to do it that way because I've been told that in NFS the operation might get stuck and in other types of storage the operation will just fail on error. Edu? Line 3555: Line 3556: snap = xml.dom.minidom.Element('domainsnapshot') Line 3557: disks = xml.dom.minidom.Element('disks') Line 3558: newDrives = {} Line 3685: # This code should be removed once qemu-img will handle files Line 3686: # with size that is not multiple of block size correctly. Line 3687: if memoryParams: Line 3688: _padMemoryVolume(memoryVolPath, memoryVol['domainID']) Line 3689: self.cif.teardownVolumePath(memoryVol) > +1, but please do that in a separate patch! ok, I will fix it in a separate patch Line 3690: Line 3691: for drive in newDrives.values(): # Update the drive information Line 3692: try: Line 3693: self.updateDriveParameters(drive) -- To view, visit http://gerrit.ovirt.org/26407 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0cebdbba2cc763963b64dcbc142f979802f18c16 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Eduardo <ewars...@gmail.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
