Change in vdsm[master]: pad memory volume only when the storage domain is file based

ahadas at redhat.com ahadas at redhat.com
Sun Apr 6 07:58:06 UTC 2014


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 <ahadas at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at gmail.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list