Federico Simoncelli has uploaded a new change for review.
Change subject: [WIP] BZ#844656 Release the lock during _findDomain ......................................................................
[WIP] BZ#844656 Release the lock during _findDomain
Signed-off-by: Federico Simoncelli fsimonce@redhat.com Change-Id: I8088d5fe716a3a08c3e5cef2d2d9a654ee96f60a --- M vdsm/storage/sdc.py 1 file changed, 21 insertions(+), 7 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/22/6822/1 -- To view, visit http://gerrit.ovirt.org/6822 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I8088d5fe716a3a08c3e5cef2d2d9a654ee96f60a Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com
Federico Simoncelli has posted comments on this change.
Change subject: [WIP] BZ#844656 Release the lock during _findDomain ......................................................................
Patch Set 1: (1 inline comment)
.................................................... File vdsm/storage/sdc.py Line 62: self._syncroot = threading.Condition() This actually requires an RLock (to be implemented in betterPthread)
-- To view, visit http://gerrit.ovirt.org/6822 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I8088d5fe716a3a08c3e5cef2d2d9a654ee96f60a Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: [WIP] BZ#844656 Release the lock during _findDomain ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(2 inline comments)
I've put my version of the locking scheme. I'm sure you can figure out the advantages.
.................................................... File vdsm/storage/sdc.py Line 115: with self._syncroot: with self._syncroot: while True: domain = self._domainCache.get(sdUUID) if domain is not None: return domain
if sdUUID in self.__inProgress: self._syncroot.wait() continue
self._inProgress.add(sdUUID)
try: domain = self._findDomain(sdUUID) self.__domainCache[sdUUID] = domain return Domain finally: with self._syncroot: self._inProgress.remove(sdUUID) self._syncroot.notifyAll()
Line 135: if domain: Why put in finally then? Either put inside the try or in an else clause.
-- To view, visit http://gerrit.ovirt.org/6822 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I8088d5fe716a3a08c3e5cef2d2d9a654ee96f60a Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Federico Simoncelli has posted comments on this change.
Change subject: [WIP] BZ#844656 Release the lock during _findDomain ......................................................................
Patch Set 2: (2 inline comments)
.................................................... File vdsm/storage/sdc.py Line 115: with self._syncroot: I'm not completely happy in assuming that this is atomic and I wanted to protect it with _syncroot.
self.__domainCache[sdUUID] = domain
Anyway there are a couple of things that mitigate the problem: 1) we are guaranteed that only one thread (per sdUUID) reached the _findDomain part, therefore we can't have concurrent updates of the same sdUUID. 2) updating two different sdUUID at the same time leave the dictionary consistent. That said I think we could go ahead and use this approach for a more elegant implementation.
Line 135: if domain: This was related to the comment above.
-- To view, visit http://gerrit.ovirt.org/6822 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I8088d5fe716a3a08c3e5cef2d2d9a654ee96f60a Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 3: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/6822 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I8088d5fe716a3a08c3e5cef2d2d9a654ee96f60a Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 4: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/6822 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I8088d5fe716a3a08c3e5cef2d2d9a654ee96f60a Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Omri Hochman has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 7: Verified
-- To view, visit http://gerrit.ovirt.org/6822 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I8088d5fe716a3a08c3e5cef2d2d9a654ee96f60a Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Omri Hochman hoomri@gmail.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 7: No score
Build Started http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/41/
-- To view, visit http://gerrit.ovirt.org/6822 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I8088d5fe716a3a08c3e5cef2d2d9a654ee96f60a Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Omri Hochman hoomri@gmail.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 7:
Build Successful
http://jenkins.ovirt.info/job/vdsm_unit_tests_by_patch/41/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/6822 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I8088d5fe716a3a08c3e5cef2d2d9a654ee96f60a Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Omri Hochman hoomri@gmail.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Saggi Mizrahi has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 7:
Should this be abandoned? It still looks good but I wonder if a rebase and a retest is in order.
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 7: Code-Review-1
Indeed, after a year, a rebase is welcome.
Itamar Heim has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 7:
ping
Itamar Heim has abandoned this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Abandoned
abandoning per no reply. please restore if still relevant.
Federico Simoncelli has restored this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Restored
I remember this as being pretty nice to have.
Itamar Heim has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 7:
and? ping
Nir Soffer has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 7:
(2 comments)
http://gerrit.ovirt.org/#/c/6822/7/vdsm/storage/sdc.py File vdsm/storage/sdc.py:
Line 59: Line 60: def __init__(self, storage_repo): Line 61: # Acquire order: self._syncProxyCache, self._syncDomainCache Line 62: self._syncProxyCache = threading.Lock() Line 63: self._syncDomainCache = threading.Condition(threading.Lock()) This names are little confusing. None of these caches, and they are not the same type of object - one a lock, the other a condition.
Seems that they should be named:
self._proxyCahceLock self._domainCacheCond Line 64: Line 65: self.__proxyCache = {} Line 66: self.__domainCache = {} Line 67: self.__inProgress = set()
Line 78: lvm.invalidateCache() Line 79: self.storageStale = False Line 80: Line 81: # This must be accessed holding the _syncProxyCache lock Line 82: def __getDomainFromCache(self, sdUUID): Maybe name this __getDomainFromCacheUnlocked to make it more clear that it must be locked? Same for __cleanStaleWeakrefs. Line 83: if self.storageStale == True: Line 84: return None Line 85: try: Line 86: return self.__proxyCache[sdUUID]()
Jenkins CI RO has abandoned this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Abandoned
Abandoned due to no activity - please restore if still relevant
Jenkins CI RO has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 7:
Abandoned due to no activity - please restore if still relevant
automation@ovirt.org has posted comments on this change.
Change subject: BZ#844656 Release the _syncroot lock during _findDomain ......................................................................
Patch Set 7:
* Update tracker::IGNORE, no Bug-Url found
vdsm-patches@lists.fedorahosted.org