Yaniv Bronhaim has posted comments on this change.
Change subject: Introducing the template activation leak.
......................................................................
Patch Set 4: (8 inline comments)
....................................................
File vdsm/storage/blockVolume.py
Line 377: lvm.activateLVs(self.sdUUID, self.volUUID)
Line 378:
Line 379: def deactivate(self):
Line 380: # TODO: use reference count here
Line 381: lvm.deactivateLVs(self.sdUUID, self.volUUID)
Do you use deactivate?
Why do you want to add ref counter here also? all the reason of that is to skip
resourceManager..
Line 382:
Line 383: @logskip("ResourceManager")
Line 384: def llPrepare(self, rw=False, setrw=False):
Line 385: """
Line 404: If justme is false, the entire COW chain should be torn down.
Line 405: """
Line 406: cls.log.info("Tearing down volume %s/%s justme %s" % (sdUUID,
volUUID, justme))
Line 407: tagParentKey, tagParentSep, parentTail =
TAG_PREFIX_PARENT.partition('_')
Line 408: tagImageKey, tagImageSep, imageTail =
TAG_PREFIX_IMAGE.partition('_')
It looks weird... I'd prefer 2 different constants for the separator sign and
prefix key for Parent and Image. This way it simpler and easy to understand.
Line 409: lvmActivationNamespace = sd.getNamespace(sdUUID,
LVM_ACTIVATION_NAMESPACE)
Line 410: # Will log a false positive error if volUUID is a template.
Line 411: rmanager.releaseResource(lvmActivationNamespace, volUUID)
Line 412: if not justme:
Line 406: cls.log.info("Tearing down volume %s/%s justme %s" % (sdUUID,
volUUID, justme))
Line 407: tagParentKey, tagParentSep, parentTail =
TAG_PREFIX_PARENT.partition('_')
Line 408: tagImageKey, tagImageSep, imageTail =
TAG_PREFIX_IMAGE.partition('_')
Line 409: lvmActivationNamespace = sd.getNamespace(sdUUID,
LVM_ACTIVATION_NAMESPACE)
Line 410: # Will log a false positive error if volUUID is a template.
Who? Resource manager? The comment here needs more info..
Line 411: rmanager.releaseResource(lvmActivationNamespace, volUUID)
Line 412: if not justme:
Line 413: try:
Line 414: tags = _getVolumeTags(sdUUID, volUUID, tagParentSep)
Line 410: # Will log a false positive error if volUUID is a template.
Line 411: rmanager.releaseResource(lvmActivationNamespace, volUUID)
Line 412: if not justme:
Line 413: try:
Line 414: tags = _getVolumeTags(sdUUID, volUUID, tagParentSep)
If you add default value for the same value you pass here, why do you do all
that unnecessary operations to get this separator?
Line 415: except Exception, e:
Line 416: # If storage not accessible or lvm error occurred
Line 417: # we will failure to get the parent volume.
Line 418: # We can live with it and still succeed in volume's
teardown.
Line 418: # We can live with it and still succeed in volume's
teardown.
Line 419: pvolUUID = volume.BLANK_UUID
Line 420: cls.log.warn("Failure to get parent of volume %s/%s
(%s)" % (sdUUID, volUUID, e))
Line 421:
Line 422: pvolUUID = tags[tagParentKey]
same.. you can have constant for tagParentKey and tagImageKey, it's never
changed...
Line 423: imgUUID = tags[tagImageKey]
Line 424: if pvolUUID != volume.BLANK_UUID:
Line 425: ptags = _getVolumeTags(sdUUID, pvolUUID, tagParentSep)
Line 426: pimgUUID = ptags[tagImageKey]
Line 484: imgUUIDs.remove(imgUUID)
Line 485:
Line 486: return imgUUIDs
Line 487:
Line 488: def getVolumeTag(self, tagPrefix):
1. same here, with '_' (separator) constant it is much simpler.
2. this function should look like:
def getVolumeTag(self, tagPrefix):
return _getVolumeTags(self.sdUUID, self.volUUID)[tagPrefix]
(using sep=SPERATOR_SIGN as default value to _getVolumeTags)
If you want, you can verify inside that this value exist like you do here, but
I think it should be handled by the caller..
Line 489: # ('PU', '_', '') = "PU_".partition('_')
Line 490: tagKey, tagSep, tail = tagPrefix.partition('_')
Line 491: tags = _getVolumeTags(self.sdUUID, self.volUUID, tagSep)
Line 492: try:
....................................................
File vdsm/storage/volume.py
Line 547:
Line 548: if not chainrw and rw and self.isInternal() and setrw and
not self.recheckIfLeaf():
Line 549: raise se.InternalVolumeNonWritable(self)
Line 550:
Line 551: if not isTemplate:
I agree but I'd add comment here that explains that if volume isShared we only
activate lv instead of calling prepare function that does reference counting..
Line 552: self.llPrepare(rw=rw, setrw=setrw)
Line 553: try:
Line 554: # Mtime is the time of the last prepare for RW
Line 555: if rw:
Line 564: self.teardown(self.sdUUID, self.volUUID)
Line 565: raise e
Line 566: # Do not take locks on templates at all, but activate them.
Line 567: elif self.__class__.__name__ == "BlockVolume":
Line 568: self.activate()
Can't you just check if isShared() in blockVolume::teardown before you release
the resource?
Line 569:
Line 570: return True
Line 571:
Line 572: @classmethod
--
To view, visit http://gerrit.ovirt.org/863
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieedf863ac967f34405f038201bac324c52fbbe89
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
