Change in vdsm[master]: [WIP] BZ#844656 Release the lock during _findDomain

[Federico Simoncelli]

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 
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

[Federico Simoncelli]

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 
Gerrit-Reviewer: Federico Simoncelli

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 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

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 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

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 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Omri Hochman 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

[Jenkins CI RO]

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Omri Hochman 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

[Jenkins CI RO]

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Omri Hochman 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

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.

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

[Dan Kenigsberg]

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.

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

Itamar Heim has posted comments on this change.

Change subject: BZ#844656 Release the _syncroot lock during _findDomain
......................................................................

Patch Set 7:

ping

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

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.

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

[Federico Simoncelli]

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.

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

Itamar Heim has posted comments on this change.

Change subject: BZ#844656 Release the _syncroot lock during _findDomain
......................................................................

Patch Set 7:

and? ping

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

[Nir Soffer]

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]()

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

[Jenkins CI RO]

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

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

[Jenkins CI RO]

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

Change in vdsm[master]: BZ#844656 Release the _syncroot lock during _findDomain

[e-mail] 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