Nir Soffer has posted comments on this change.

Change subject: ResourceManagerLock: Disable autoRelease
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/63967/1//COMMIT_MSG
Commit Message:

Line 10: resource (prefering instead to use the ResourceManager instance
Line 11: methods).  Unfortunately the underlying resource has autoRelease
Line 12: behavior on by default when the object is destroyed.  This is causing
Line 13: the lock to be released immediately after it is acquired!  Disable
Line 14: autoRelease to get the correct behavior.
> how could acquire() ever work? or is it some kind of a recent regression?
acquire() is used as context manager in most of the code - the context
manager probably keeps a reference to the ResourceRef returned from acquire.

If we had any issues, we would get failures in the logs when releasing locks.
This is how Adam found this issue.

In the task module, the task is using resourceManager.Owner, which keep
a reference to the ResourceRef in a dict, and remove the reference after
releasing the lock.
See vdsm/storage/resourceManager.py:729

In blockSD module, there is one site using without context manager, 
and it uses the same logic - disable autoRelease on the returned 
ResourceRef before it goes out of scope.
See vdsm/storage/blockVolume.py:377

I plan to kill the autorelease feature, this is unwanted and evil feature.
Locks should be used only with context manager, and there is no need
for automatic releasing, especially when this is implemented by starting
a new thread in __del__.

This "feature" is probably one of the reasons why resourceManager is
so complex.
Line 15: 
Line 16: Change-Id: Ie70dca0079f773362bfebe6b9a08677c94314e68


-- 
To view, visit https://gerrit.ovirt.org/63967
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie70dca0079f773362bfebe6b9a08677c94314e68
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <ali...@redhat.com>
Gerrit-Reviewer: Adam Litke <ali...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Shmuel Leib Melamud <smela...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Reply via email to