Yaniv Bronhaim has uploaded a new change for review.
Change subject: Adding new resourceFactory that only opens fd ......................................................................
Adding new resourceFactory that only opens fd
Avoiding activate every time we try to access lv and only open fd instead. This replace the refCounter and the call to lvchange anytime we prepare volume.
Change-Id: Ic936c8959562a262de5ceba8ca4f68bc53b5cae2 Signed-off-by: Yaniv Bronhaim ybronhei@redhat.com --- M vdsm/storage/blockVolume.py M vdsm/storage/resourceFactories.py 2 files changed, 51 insertions(+), 7 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/84/8084/1
diff --git a/vdsm/storage/blockVolume.py b/vdsm/storage/blockVolume.py index 38285c1..2324ee7 100644 --- a/vdsm/storage/blockVolume.py +++ b/vdsm/storage/blockVolume.py @@ -35,7 +35,7 @@ import resourceManager as rm from threadLocal import vars from sdc import sdCache -from resourceFactories import LVM_ACTIVATION_NAMESPACE +import resourceFactories as rf import fileUtils
TAG_PREFIX_MD = "MD_" @@ -75,8 +75,22 @@ def __init__(self, repoPath, sdUUID, imgUUID, volUUID): self.metaoff = None volume.Volume.__init__(self, repoPath, sdUUID, imgUUID, volUUID) - self.lvmActivationNamespace = sd.getNamespace(self.sdUUID, - LVM_ACTIVATION_NAMESPACE) + self._registerResourceNamespaces(sdUUID,volUUID) + + def _registerResourceNamespaces(self, sdUUID, volUUID): + """ + Register resources namespaces and create + factories for it. + """ + rmanager = rm.ResourceManager.getInstance() + # Register lvm activation resource namespace for the underlying VG + volWatcherFactory = rf.VolumeWatcherFactory(sdUUID, volUUID) + self.volWatcherNamespace = rf.VOLUME_WATCHER_NAMESPACE + try: + rmanager.registerNamespace(self.volWatcherNamespace, volWatcherFactory) + except Exception: + self.log.warn("Resource namespace %s already registered", self.volWatcherNamespace) +
def validate(self): try: @@ -356,8 +370,14 @@ if setrw: self.setrw(rw=rw) access = rm.LockType.exclusive if rw else rm.LockType.shared - activation = rmanager.acquireResource(self.lvmActivationNamespace, + + # active volume + self.activateVolumes(self.volUUID) + + # acquireResource - open file + activation = rmanager.acquireResource(self.volWatcherNamespace, self.volUUID, access) + activation.autoRelease = False
@classmethod @@ -369,9 +389,10 @@ """ cls.log.info("Tearing down volume %s/%s justme %s" % (sdUUID, volUUID, justme)) - lvmActivationNamespace = sd.getNamespace(sdUUID, - LVM_ACTIVATION_NAMESPACE) - rmanager.releaseResource(lvmActivationNamespace, volUUID) + + cls.deactivateVolumes(volUUID) + + rmanager.releaseResource(cls.volWatcherNamespace, volUUID) if not justme: try: pvolUUID = _getVolumeTag(sdUUID, volUUID, TAG_PREFIX_PARENT) diff --git a/vdsm/storage/resourceFactories.py b/vdsm/storage/resourceFactories.py index 02aba0f..8893f70 100644 --- a/vdsm/storage/resourceFactories.py +++ b/vdsm/storage/resourceFactories.py @@ -31,11 +31,34 @@ LVM_ACTIVATION_NAMESPACE = 'lvmActivationNS' IMAGE_NAMESPACE = 'imageNS' VOLUME_NAMESPACE = 'volumeNS' +VOLUME_WATCHER_NAMESPACE = 'volWatchNS'
rmanager = rm.ResourceManager.getInstance()
log = logging.getLogger('Storage.ResourcesFactories')
+class VolumeWatcher(object): + def __init__(self, vgName, lvName): + self._path = lvm.lvPath(vgName, lvName) + try: + self._f = open(self._path) + except IOError: + log.error("Cannot open device %s", self._path) + + def close(self): + try: + self._f.close() + except: + log.error("error closing device %s", self._path) + +class VolumeWatcherFactory(rm.SimpleResourceFactory): + def __init__(self, vgName, lvName): + rm.SimpleResourceFactory.__init__(self) + self._lv = lvName + self._vg = vgName + + def createResource(self): + return VolumeWatcher(self._lv, self._vg)
class LvmActivation(object): """
-- To view, visit http://gerrit.ovirt.org/8084 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: Ic936c8959562a262de5ceba8ca4f68bc53b5cae2 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim ybronhei@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Adding new resourceFactory that only opens fd ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(3 inline comments)
.................................................... File vdsm/storage/blockVolume.py Line 84: """ Line 85: rmanager = rm.ResourceManager.getInstance() Line 86: # Register lvm activation resource namespace for the underlying VG Line 87: volWatcherFactory = rf.VolumeWatcherFactory(sdUUID, volUUID) Line 88: self.volWatcherNamespace = rf.VOLUME_WATCHER_NAMESPACE If it's the same namespace every-time, you could put this somewhere centralized. Line 89: try: Line 90: rmanager.registerNamespace(self.volWatcherNamespace, Line 91: volWatcherFactory) Line 92: except Exception:
Line 393: """ Line 394: cls.log.info("Tearing down volume %s/%s justme %s" Line 395: % (sdUUID, volUUID, justme)) Line 396: Line 397: cls.deactivateVolumes(volUUID) You need to release the file before deactivating. Line 398: Line 399: resourcename = volUUID + ':' + sdUUID Line 400: rmanager.releaseResource(cls.volWatcherNamespace, resourcename) Line 401: if not justme:
Line 395: % (sdUUID, volUUID, justme)) Line 396: Line 397: cls.deactivateVolumes(volUUID) Line 398: Line 399: resourcename = volUUID + ':' + sdUUID It's customary to separate the VG from the LV with a '/' Line 400: rmanager.releaseResource(cls.volWatcherNamespace, resourcename) Line 401: if not justme: Line 402: try: Line 403: pvolUUID = _getVolumeTag(sdUUID, volUUID, TAG_PREFIX_PARENT)
-- To view, visit http://gerrit.ovirt.org/8084 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic936c8959562a262de5ceba8ca4f68bc53b5cae2 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Adding new resourceFactory that only opens fd ......................................................................
Patch Set 5: I would prefer that you didn't submit this
(5 inline comments)
.................................................... File vdsm/storage/blockVolume.py Line 87: volWatcherFactory = rf.VolumeWatcherFactory(sdUUID, volUUID) Line 88: self.volWatcherNamespace = rf.VOLUME_WATCHER_NAMESPACE Line 89: try: Line 90: rmanager.registerNamespace(self.volWatcherNamespace, Line 91: volWatcherFactory) You could register this in the dispatcher now that it has a constant namespace Line 92: except Exception: Line 93: self.log.warn("Resource namespace %s already registered", Line 94: self.volWatcherNamespace) Line 95:
.................................................... File vdsm/storage/resourceFactories.py Line 30: Line 31: LVM_ACTIVATION_NAMESPACE = 'lvmActivationNS' Line 32: IMAGE_NAMESPACE = 'imageNS' Line 33: VOLUME_NAMESPACE = 'volumeNS' Line 34: VOLUME_WATCHER_NAMESPACE = 'volWatchNS' I'd call it VolumeHandler Line 35: Line 36: rmanager = rm.ResourceManager.getInstance() Line 37: Line 38: log = logging.getLogger('Storage.ResourcesFactories')
Line 62: """ Line 63: Return :keyword:`True` if a resource with that name is producible with Line 64: this factory. Line 65: """ Line 66: return True Check if the file exists, could detect bugs in a cleaner way Line 67: Line 68: def createResource(self, resourceName, lockType): Line 69: try: Line 70: lvvg = resourceName.split('/')
Line 66: return True Line 67: Line 68: def createResource(self, resourceName, lockType): Line 69: try: Line 70: lvvg = resourceName.split('/') lv, vg = .... Line 71: return VolumeWatcher(lvvg[0], lvvg[1]) Line 72: except: Line 73: log.error("error creating resource name: %s", resourceName) Line 74:
Line 69: try: Line 70: lvvg = resourceName.split('/') Line 71: return VolumeWatcher(lvvg[0], lvvg[1]) Line 72: except: Line 73: log.error("error creating resource name: %s", resourceName) Propagate the error or the resource manager will think that everything was OK Line 74: Line 75: Line 76: class LvmActivation(object): Line 77: """
-- To view, visit http://gerrit.ovirt.org/8084 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic936c8959562a262de5ceba8ca4f68bc53b5cae2 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Adding new resourceFactory that only opens fd ......................................................................
Patch Set 7: Looks good to me, but someone else must approve
(2 inline comments)
Please open preinteg ticket
.................................................... File vdsm/storage/blockVolume.py Line 372: self.setrw(rw=rw) Line 373: access = rm.LockType.exclusive if rw else rm.LockType.shared Line 374: Line 375: # active volume Line 376: self.activateVolumes(self.volUUID) pass list Line 377: Line 378: resourcename = self.volUUID + '/' + self.sdUUID Line 379: Line 380: # acquireResource - open file
Line 394: % (sdUUID, volUUID, justme)) Line 395: Line 396: resourcename = volUUID + '/' + sdUUID Line 397: rmanager.releaseResource(cls.volWatcherNamespace, resourcename) Line 398: cls.deactivateVolumes(volUUID) pass a list Line 399: Line 400: if not justme: Line 401: try: Line 402: pvolUUID = _getVolumeTag(sdUUID, volUUID, TAG_PREFIX_PARENT)
-- To view, visit http://gerrit.ovirt.org/8084 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic936c8959562a262de5ceba8ca4f68bc53b5cae2 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Adding new resourceFactory that only opens fd ......................................................................
Patch Set 8: Looks good to me, but someone else must approve
Open a preinteg ticket
-- To view, visit http://gerrit.ovirt.org/8084 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic936c8959562a262de5ceba8ca4f68bc53b5cae2 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com
Douglas Schilling Landgraf has posted comments on this change.
Change subject: Adding new resourceFactory that only opens fd ......................................................................
Patch Set 8: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/8084 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic936c8959562a262de5ceba8ca4f68bc53b5cae2 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com
Ayal Baron has posted comments on this change.
Change subject: Adding new resourceFactory that only opens fd ......................................................................
Patch Set 8: I would prefer that you didn't submit this
-- To view, visit http://gerrit.ovirt.org/8084 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic936c8959562a262de5ceba8ca4f68bc53b5cae2 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Adding new resourceFactory that only opens fd ......................................................................
Patch Set 10: I would prefer that you didn't submit this
(8 inline comments)
.................................................... File vdsm/storage/blockVolume.py Line 88: self.volWatcherNamespace = rf.VOLUME_WATCHER_NAMESPACE Line 89: try: Line 90: rmanager.registerNamespace(self.volWatcherNamespace, Line 91: volWatcherFactory) Line 92: except Exception: No need to log as it is expected behavior. Line 93: self.log.warn("Resource namespace %s already registered", Line 94: self.volWatcherNamespace) Line 95: Line 96: def validate(self):
Line 372: self.setrw(rw=rw) Line 373: access = rm.LockType.exclusive if rw else rm.LockType.shared Line 374: Line 375: # active volume Line 376: self.activateVolumes(self.volUUID) This needs to be a list\tuple with one item Line 377: Line 378: resourcename = self.volUUID + '/' + self.sdUUID Line 379: Line 380: # acquireResource - open file
Line 394: % (sdUUID, volUUID, justme)) Line 395: Line 396: resourcename = volUUID + '/' + sdUUID Line 397: rmanager.releaseResource(cls.volWatcherNamespace, resourcename) Line 398: cls.deactivateVolumes(volUUID) list\tuple Line 399: Line 400: if not justme: Line 401: try: Line 402: pvolUUID = _getVolumeTag(sdUUID, volUUID, TAG_PREFIX_PARENT)
.................................................... File vdsm/storage/resourceFactories.py Line 43: self._path = lvm.lvPath(lvName, vgName) Line 44: try: Line 45: self._f = open(self._path) Line 46: except Exception as e: Line 47: log.error("Cannot open device %s: %s", self._path, e) Don't catch the exception! Line 48: Line 49: def close(self): Line 50: try: Line 51: self._f.close()
Line 49: def close(self): Line 50: try: Line 51: self._f.close() Line 52: except Exception as e: Line 53: log.error("Cannot close device %s: %s", self._path, e) don't catch the exception! Line 54: Line 55: Line 56: class VolumeWatcherFactory(object): Line 57: def __init__(self, vgName, lvName):
Line 66: return True Line 67: Line 68: def createResource(self, resourceName, lockType): Line 69: try: Line 70: lvvg = resourceName.split('/') lv, vg = Line 71: return VolumeWatcher(lvvg[0], lvvg[1]) Line 72: except: Line 73: log.error("error creating resource name: %s", resourceName) Line 74:
Line 67: Line 68: def createResource(self, resourceName, lockType): Line 69: try: Line 70: lvvg = resourceName.split('/') Line 71: return VolumeWatcher(lvvg[0], lvvg[1]) return VolumeWatcher(lv, vg) Line 72: except: Line 73: log.error("error creating resource name: %s", resourceName) Line 74: Line 75:
Line 69: try: Line 70: lvvg = resourceName.split('/') Line 71: return VolumeWatcher(lvvg[0], lvvg[1]) Line 72: except: Line 73: log.error("error creating resource name: %s", resourceName) Don't catch the exception! Line 74: Line 75: Line 76: class LvmActivation(object): Line 77: """
-- To view, visit http://gerrit.ovirt.org/8084 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic936c8959562a262de5ceba8ca4f68bc53b5cae2 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com
Yaniv Bronhaim has abandoned this change.
Change subject: Adding new resourceFactory that only opens fd ......................................................................
Patch Set 10: Abandoned
-- To view, visit http://gerrit.ovirt.org/8084 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon Gerrit-Change-Id: Ic936c8959562a262de5ceba8ca4f68bc53b5cae2 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com
vdsm-patches@lists.fedorahosted.org