Ocfs2 devel - Dec 2015 - [PATCH] ocfs2: dlm: fix recursive locking deadlock

Hi Mark,

On 12/19/2015 07:23 AM, Mark Fasheh wrote:
> On Tue, Dec 15, 2015 at 09:43:48AM +0800, Junxiao Bi wrote:
>> Hi Mark,
>>
>> On 12/15/2015 03:18 AM, Mark Fasheh wrote:
>>> On Mon, Dec 14, 2015 at 02:03:17PM +0800, Junxiao Bi wrote:
>>>>> Second, this issue can be reproduced in old Linux kernels
(e.g. 3.16.7-24)? there should not be any regression issue?
>>>> Maybe just hard to reproduce, ocfs2 supports recursive locking.
>>>
>>> In what sense? The DLM might but the FS should never be making use
of such a
>>> mechanism (it would be for userspace users).
>> See commit 743b5f1434f5 ("ocfs2: take inode lock in
>> ocfs2_iop_set/get_acl()"), it used recursive locking and caused a
>> deadlock, the call trace is in this patch's log.
> 
> Ahh ok so it's part of the buggy patch.
> 
> 
>>> We really can't add recursive locks without this getting
rejected upstream.
>>> There's a whole slew of reasons why we don't like those in
the kernel.
>> Is there any harm to support this lock in kernel?
> 
> Yeah so you can google search on why recursive locks are considered harmful
> by many programmers and in the Linux Kernel they are a big 'No No'.
We used
> to have one recursive lock (the 'big kernel lock') which took a
large effort
> to clean up.
> 
> Most objections are going to come down to the readability of the code and
> the nasty bugs that can come about as a result. Here's a random blog
post I
> found explaining some of this:
> 
> http://blog.stephencleary.com/2013/04/recursive-re-entrant-locks.html
Good doc. Thank you for sharing it. Learned more about recursive locking.

But I am afraid, cluster lock suffers all drawbacks mentioned in that
doc since it is stick to node. P1 and P2 can hold one EX lock at the
same time, is it a kind of recursive locking for cluster lock?
Maybe it is just a bad naming of recursive locking. If two processes in
one node are allowed to hold one EX lock at the same time, why one
process not allowed to lock one EX lock two times?

Thanks,
Junxiao.
> 
> I'm pretty sure Linus has a rant about it somewhere too you can find.
> 
> 
> But basically your approach to fixing situations like this is going to be
> refactoring the code in a readable manner such that the lock is only taken
> once per code path.
> 
> Hope that all helps,
> 	--Mark
> 
> 
> --
> Mark Fasheh
>

On Mon, Dec 21, 2015 at 01:12:34PM +0800, Junxiao Bi
wrote:
> Hi Mark,
> 
> On 12/19/2015 07:23 AM, Mark Fasheh wrote:
> > On Tue, Dec 15, 2015 at 09:43:48AM +0800, Junxiao Bi wrote:
> >> Hi Mark,
> >>
> >> On 12/15/2015 03:18 AM, Mark Fasheh wrote:
> >>> On Mon, Dec 14, 2015 at 02:03:17PM +0800, Junxiao Bi wrote:
> >>>>> Second, this issue can be reproduced in old Linux
kernels (e.g. 3.16.7-24)? there should not be any regression issue?
> >>>> Maybe just hard to reproduce, ocfs2 supports recursive
locking.
> >>>
> >>> In what sense? The DLM might but the FS should never be making
use of such a
> >>> mechanism (it would be for userspace users).
> >> See commit 743b5f1434f5 ("ocfs2: take inode lock in
> >> ocfs2_iop_set/get_acl()"), it used recursive locking and
caused a
> >> deadlock, the call trace is in this patch's log.
> > 
> > Ahh ok so it's part of the buggy patch.
> > 
> > 
> >>> We really can't add recursive locks without this getting
rejected upstream.
> >>> There's a whole slew of reasons why we don't like
those in the kernel.
> >> Is there any harm to support this lock in kernel?
> > 
> > Yeah so you can google search on why recursive locks are considered
harmful
> > by many programmers and in the Linux Kernel they are a big 'No
No'. We used
> > to have one recursive lock (the 'big kernel lock') which took
a large effort
> > to clean up.
> > 
> > Most objections are going to come down to the readability of the code
and
> > the nasty bugs that can come about as a result. Here's a random
blog post I
> > found explaining some of this:
> > 
> > http://blog.stephencleary.com/2013/04/recursive-re-entrant-locks.html
> Good doc. Thank you for sharing it. Learned more about recursive locking.
Np, glad it helped.

> But I am afraid, cluster lock suffers all drawbacks mentioned in that
> doc since it is stick to node. P1 and P2 can hold one EX lock at the
> same time, is it a kind of recursive locking for cluster lock?
> Maybe it is just a bad naming of recursive locking. If two processes in
> one node are allowed to hold one EX lock at the same time, why one
> process not allowed to lock one EX lock two times?
Yeah I think maybe it's 'bad naming' as you say? What dlmglue is
doing we
call 'lock caching'.

It's possible to use the dlm without caching the locks but that means every
process wanting a cluster lock will have to make a dlm request. So instead,
we hold onto the lock until another node wants it. However, this effectively
makes each lock node-wide (as opposed to per-process which we would get
without the lock caching).

For local process locking, we use the VFS locks (inode mutex, etc).

So the cluster lock is never being acquired more than once, it gets
acquired and dlmglue caches access to it in a fair way. Hence it is not
being acquired 'recusively'.

Callers of dlmglue honor the usual locking rules - the same ones that we
honor with local locks. For example, they must be properly ordered with
respect to other locks.


As you point out, we could turn the dlmglue locking into a true recursive
locking scheme. But if we do that, then we are changing the design of
dlmglue and introducing recursive locking into a kernel which does not hav
it. The 2nd point is self explanatory. The reason I would also not want to
change the design of dlmglue is that I don't feel this bug requires such a
drastic measure.

Thanks,
	--Mark

--
Mark Fasheh