Change in vdsm[master]: defer grant untill all resource manager locks are freed

Saggi Mizrahi has uploaded a new change for review.

Change subject: defer grant untill all resource manager locks are freed
......................................................................

defer grant untill all resource manager locks are freed

Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
---
M vdsm/storage/resourceManager.py
1 file changed, 2 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Defer grant until all resource manager locks are...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#754445 - Defer grant until all resource manager locks are freed
......................................................................


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

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Defer grant until all resource manager locks are...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#754445 - Defer grant until all resource manager locks are freed
......................................................................


Patch Set 2:

oh, Saggi, what's this funny "redeadlock" on this change's branch?

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Defer grant until all resource manager locks are...

Saggi Mizrahi has posted comments on this change.

Change subject: BZ#754445 - Defer grant until all resource manager locks are freed
......................................................................


Patch Set 2:

Ok, let me see if I understand what you are saying.
"Please don't fix the deadlock in the resource system, instead make some fix that
might work and will be harder to understand.
Next time when someone uses registerResource() and tries to acquire or release a lock in
the callback and gets into the deadlock just tell them to find another way to hack around
it."

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Defer grant until all resource manager locks are...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#754445 - Defer grant until all resource manager locks are freed
......................................................................


Patch Set 2:

Ayal, I want to solve this in Z-steam, and do not think that it makes sense to wait for an
overhaul. Can we settle for this patch as a stopgap?

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Defer grant until all resource manager locks are...

Saggi Mizrahi has posted comments on this change.

Change subject: BZ#754445 - Defer grant until all resource manager locks are freed
......................................................................


Patch Set 2:

After talking with Danken I see there is a proper misunderstanding.

First of all the fix is a two liner using the same trick used in registerResource to
prevent the very same problem. This is not a big complex change. It should have been there
in the first place. It's part of the desing.

What Ayal is suggesting would work, but it would only work for this particular instance
which is not the true case the callbacks where designed for. So it's not idiomatic.

The resource should be put in the callback. This is the only logical way it can work
otherwise you wouldn't have an resource object to release().

There is no reason we should prevent release() in the callback. If the operation is simple
there is no need to put over complicated queues and messaging and waiting threads and
other stuff to actually be able to release the resource once you're done with it.

I really don't see why there was even an argument, a two line proper fix is better
then any hack. Even if I didn't stress all of the above, callbacks should not be
trusted because you have no idea what they do so you should try and keep yourself from
deadlocking because of them.

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Defer grant until all resource manager locks are...

Ayal Baron has posted comments on this change.

Change subject: BZ#754445 - Defer grant until all resource manager locks are freed
......................................................................


Patch Set 2:

The proper fix would have been to detect and throw an exception in this case.  Solving the
case where the callback is messing around with the lock works, but is hacky.
It's like running:
with self.lock:
    do something
    if whatever:
        self.lock.release()

that is obviously not the right scope for dealing with the lock.

"Fixing" it to support the above works but is obviously the wrong thing to do
and that is exactly what you did in this patch (and previously with registerResource).

The fact that you're under the nextRequest lock yet cannot grant it the resource in
this state should have let you in on the fact that something is wrong,

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Defer grant until all resource manager locks are...

Saggi Mizrahi has posted comments on this change.

Change subject: BZ#754445 - Defer grant until all resource manager locks are freed
......................................................................


Patch Set 2:

No it isn't if you want context semantics you use acquireResource

registerResource as signal\slot semantics

signal.connect(lock.onLockAcquire, doStuff)
keepOnDoingStuff()

doStuff(lock):
  stuff()
  lock.release()

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Defer grant until all resource manager locks are...

Ayal Baron has posted comments on this change.

Change subject: BZ#754445 - Defer grant until all resource manager locks are freed
......................................................................


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

After a long discussion in irc, we agreed that although the code is overly complex and
leads to errors, my suggested solution would require more acrobatics because the caller
does not have a reference to the resource, only to the request, making it complex as
well.
Long term we should get rid of the entire mechanism to reduce complexity (and solve in a
simpler way)
changing to +1

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Defer grant until all resource manager locks are...

Ayal Baron has posted comments on this change.

Change subject: BZ#754445 - Defer grant until all resource manager locks are freed
......................................................................


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

Of course I was right and the code is still buggy and there is a race here (nextRequest
can get cancelled before grant is actually called outside the protection of
nextRequest.syncRoot lock

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Seperate granting from callback emitting

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#754445 - Seperate granting from callback emitting
......................................................................


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

(2 inline comments)

minor comments about comments, rest seems good, but requires serious Hateyazation. Did you
open a ticket?

....................................................
Commit Message
Line 11: idea how long it will take and what it will do with the resource for
/me no English, but please add Period after "the resource", and fix
"cruisal".

....................................................
File vdsm/storage/resourceManager.py
Line 452:         # Code in a nested function's body may access but not rebind
I suppose that with the replacement of list to Queue, the comment has rotten.

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Seperate granting from callback emitting

Ayal Baron has posted comments on this change.

Change subject: BZ#754445 - Seperate granting from callback emitting
......................................................................


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

(4 inline comments)

....................................................
Commit Message
Line 13: On the other hand granting the request has to be synchronious and
also fix synchronious

Line 14: transactional to prevent races with cancel. To keep both demands I split
also, you're repeating yourself (Granting has to happen... On the other hand granting
...)

....................................................
File vdsm/storage/resourceManager.py
Line 452:         # Code in a nested function's body may access but not rebind
Actually I don't see the difference between list and queue in this respect so I'm
not sure the comment should change.
however the apostrophe in wouldn't is after the 'n', not before.

Line 470:         return resource.get()
if for some reason resource queue is empty then the thread would hang here forever. the
previous version of the code handled this state

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Haim Ateya <hateya(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Separate granting from callback emitting

Ayal Baron has posted comments on this change.

Change subject: BZ#754445 - Separate granting from callback emitting
......................................................................


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

(1 inline comment)

....................................................
Commit Message
Line 11: anything about. For that reason it is crucial to run the callback
does not **** anything about
????

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Haim Ateya <hateya(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Separate granting from callback emitting

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#754445 - Separate granting from callback emitting
......................................................................


Patch Set 9: Verified; Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Haim Ateya <hateya(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#754445 - Separate granting from callback emitting

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: BZ#754445 - Separate granting from callback emitting
......................................................................


BZ#754445 - Separate granting from callback emitting

Granting has to happen inside the request transaction to prevent races
with cancel\timeouts. The callback is code that the subsystem does not
know anything about. For that reason it is crucial to run the callback
outside of the lock scope. To keep both demands I split up emit() from
grant().

Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
---
M vdsm/storage/resourceManager.py
1 file changed, 23 insertions(+), 23 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved


--
To view, visit http://gerrit.ovirt.org/254
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic12d170e64399e37a555960c03804778ad7d053b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Haim Ateya <hateya(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;