Change in vdsm[master]: Avoid template deactivation and lock.

Eduardo has uploaded a new change for review.

Change subject: Avoid template deactivation and lock.
......................................................................

Avoid template deactivation and lock.

Change-Id: Ieedf863ac967f34405f038201bac324c52fbbe89
---
M vdsm/storage/blockVolume.py
M vdsm/storage/volume.py
2 files changed, 39 insertions(+), 18 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/63/863/1
--
To view, visit http://gerrit.ovirt.org/863
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieedf863ac967f34405f038201bac324c52fbbe89
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Avoid template deactivation and lock.
......................................................................

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

(4 inline comments)

....................................................
Commit Message
Line 8: 
describe the intention:
We would like to avoid taking re-acquiring resources for Vm's disks on Vdsm restart.

However, we want to keep the template active after it is used by an spm verb. Instead of counting the references to the template and deactivating when the refcount is 0, we leak the activation on first use and never deactivate the template.

this may lead to performance hit once we have thousands of templates.

....................................................
File vdsm/storage/blockVolume.py
Line 408:         # Will log a false positive error for templates.
if volUUID is a template

Line 425:                 # A parent from other image is a template.
other->another

Line 489:             return tags[tagPrefix[:-1]]
into previous patch

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Avoid template deactivation and lock.
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/blockVolume.py
Line 489:             return tags[tagPrefix[:-1]]
do it on _getVolumeTags

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Introducing the template activation leak.

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Introducing the template activation leak.
......................................................................

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

(3 inline comments)

....................................................
File vdsm/storage/blockVolume.py
Line 420:             pvolUUID = tags[TAG_PREFIX_PARENT]
Should be without  '_' in TAG_PREFIX_PARENT

Line 421:             imgUUID = tags[TAG_PREFIX_IMAGE]
Same

....................................................
File vdsm/storage/volume.py
Line 551:         if not isTemplate:
Please add comment to explain

--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Introducing the template activation leak.

Ayal Baron has posted comments on this change.

Change subject: Introducing the template activation leak.
......................................................................

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

(1 inline comment)

This and the parent patch should be 1 patch.  This separation makes no sense and make the process lengthy and dumb.

....................................................
File vdsm/storage/blockVolume.py
Line 426:                 pimgUUID = ptags[TAG_PREFIX_IMAGE]
s/TAG_PREFIX_IMAGE/tagImageKey/

--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Introducing the template activation leak.

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Introducing the template activation leak.
......................................................................

Patch Set 4: Looks good to me, but someone else must approve

--
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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Introducing the template activation leak.

Ayal Baron has posted comments on this change.

Change subject: Introducing the template activation leak.
......................................................................

Patch Set 4: Looks good to me, approved

--
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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: Introducing the template activation leak.

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Yaniv Bronhaim

Change in vdsm[master]: Introducing the template activation leak.

Itamar Heim has posted comments on this change.

Change subject: Introducing the template activation leak.
......................................................................

Patch Set 4:

pinf

Change in vdsm[master]: Introducing the template activation leak.

Itamar Heim has abandoned this change.

Change subject: Introducing the template activation leak.
......................................................................

Abandoned

abandoning - old. no reply. not touched for a while. please restore if relevant