[Cluster-devel] [PATCH 13/17] dlm: fix _can_be_granted() for lock at the head of covert queue.

[Cluster-devel] [PATCH 13/17] dlm: fix _can_be_granted() for lock at the head of covert queue.

[tsutomu.owa]

If there is a lock resource conflict on multiple nodes, the lock on
convert queue may not be granted forever.

EX.)
grant queue:
    node0 grmode NL / rqmode IV
    node1 grmode NL / rqmode IV

convert queue:
    node2 grmode NL / rqmode EX
    node3 grmode PR / rqmode EX

wait queue:
    node4 grmode IV / rqmode PR
    node5 grmode IV / rqmode PR

When the lock conversion (node PR -> NL) of node 0 is completed, the lock
of node 2 should be grantable. However, __can_be_granted() returns 0
because the grmode of the lock on node 3 in convert queue is PR.

When checking the lock at the head of convert queue, exclude
queue_conflict() targeting convert queue.

Signed-off-by: Tadashi Miyauchi <miyauchi@toshiba-tops.co.jp>
Signed-off-by: Tsutomu Owa <tsutomu.owa@toshiba.co.jp>
---
 fs/dlm/lock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c
index 35502d4..dcdd26d 100644
--- a/fs/dlm/lock.c
+++ b/fs/dlm/lock.c
@@ -2336,7 +2336,8 @@ static int _can_be_granted(struct dlm_rsb *r, struct dlm_lkb *lkb, int now,
 	 * locks
 	 */
 
-	if (queue_conflict(&r->res_convertqueue, lkb))
+	if (!first_in_list(lkb, &r->res_convertqueue) &&
+	    queue_conflict(&r->res_convertqueue, lkb))
 		return 0;
 
 	/*
-- 
2.7.4

[Cluster-devel] [PATCH 13/17] dlm: fix _can_be_granted() for lock at the head of covert queue.

[David Teigland]

On Wed, Aug 09, 2017 at 05:51:37AM +0000, tsutomu.owa at toshiba.co.jp wrote:
> If there is a lock resource conflict on multiple nodes, the lock on
> convert queue may not be granted forever.
> 
> EX.)
> grant queue:
>     node0 grmode NL / rqmode IV
>     node1 grmode NL / rqmode IV
> 
> convert queue:
>     node2 grmode NL / rqmode EX
>     node3 grmode PR / rqmode EX
> 
> wait queue:
>     node4 grmode IV / rqmode PR
>     node5 grmode IV / rqmode PR
> 
> When the lock conversion (node PR -> NL) of node 0 is completed, the lock
> of node 2 should be grantable. However, __can_be_granted() returns 0
> because the grmode of the lock on node 3 in convert queue is PR.
> 
> When checking the lock at the head of convert queue, exclude
> queue_conflict() targeting convert queue.

This example doesn't look right.  node2's NL->EX cannot be granted because
it conflicts with the PR lock held by node3.  (The grmode is still valid
when a lock is on the convert queue.)

There are two valid outcomes in the example above, either 1) node3 PR->EX
is granted, or 2) node4 and node5 PR requests are granted.  What have you
seen the dlm do in this state?  If it does not grant anything, that would
be a bug.

Based on the sequence of events you describe, I think that the correct
outcome would be 1 (granting node3's PR->EX), based on this rule:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/dlm/lock.c#n2429


> -	if (queue_conflict(&r->res_convertqueue, lkb))
> +	if (!first_in_list(lkb, &r->res_convertqueue) &&
> +	    queue_conflict(&r->res_convertqueue, lkb))
>  		return 0;

[Cluster-devel] [PATCH 13/17] dlm: fix _can_be_granted() for lock at the head of covert queue.

[David Teigland]

On Wed, Aug 09, 2017 at 11:41:44AM -0500, David Teigland wrote:
> On Wed, Aug 09, 2017 at 05:51:37AM +0000, tsutomu.owa at toshiba.co.jp wrote:
> > If there is a lock resource conflict on multiple nodes, the lock on
> > convert queue may not be granted forever.
> > 
> > EX.)
> > grant queue:
> >     node0 grmode NL / rqmode IV
> >     node1 grmode NL / rqmode IV
> > 
> > convert queue:
> >     node2 grmode NL / rqmode EX
> >     node3 grmode PR / rqmode EX
> > 
> > wait queue:
> >     node4 grmode IV / rqmode PR
> >     node5 grmode IV / rqmode PR
> > 
> > When the lock conversion (node PR -> NL) of node 0 is completed, the lock
> > of node 2 should be grantable. However, __can_be_granted() returns 0
> > because the grmode of the lock on node 3 in convert queue is PR.
> > 
> > When checking the lock at the head of convert queue, exclude
> > queue_conflict() targeting convert queue.
> 
> This example doesn't look right.  node2's NL->EX cannot be granted because
> it conflicts with the PR lock held by node3.  (The grmode is still valid
> when a lock is on the convert queue.)

After looking more closely, this is a subtle form of conversion deadlock,
and this exact case is described in the comment here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/dlm/lock.c#n2218

This should be handled by the dlm canceling one of the converting locks
(returning it to the grant queue with IV rqmode) and returning -EDEADLK to
the application.  There is a FIXME in the code highlighting a case you
could be hitting:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/dlm/lock.c#n2504

If you are running into that FIXME, you should see these log messages:

 if (deadlk) {
        log_print("WARN: pending deadlock %x node %d %s",
                  lkb->lkb_id, lkb->lkb_nodeid, r->res_name);
        dlm_dump_rsb(r);
        continue;
 }

[Cluster-devel] [PATCH 13/17] dlm: fix _can_be_granted() for lock at the head of covert queue.

[tsutomu.owa]

Hi,

>> After looking more closely, this is a subtle form of conversion deadlock,
>> and this exact case is described in the comment here:

thanks, we withdraw this patch for now since we need to look into more.

-- owa

-----Original Message-----
From: David Teigland [mailto:teigland at redhat.com] 
Sent: Thursday, August 10, 2017 3:48 AM
To: owa tsutomu(?? ? ??? ?????????????)
Cc: cluster-devel at redhat.com; miyauchi tadashi(?? ?? ???? ?????????)
Subject: Re: [Cluster-devel] [PATCH 13/17] dlm: fix _can_be_granted() for lock at the head of covert queue.

On Wed, Aug 09, 2017 at 11:41:44AM -0500, David Teigland wrote:
> On Wed, Aug 09, 2017 at 05:51:37AM +0000, tsutomu.owa at toshiba.co.jp wrote:
> > If there is a lock resource conflict on multiple nodes, the lock on
> > convert queue may not be granted forever.
> > 
> > EX.)
> > grant queue:
> >     node0 grmode NL / rqmode IV
> >     node1 grmode NL / rqmode IV
> > 
> > convert queue:
> >     node2 grmode NL / rqmode EX
> >     node3 grmode PR / rqmode EX
> > 
> > wait queue:
> >     node4 grmode IV / rqmode PR
> >     node5 grmode IV / rqmode PR
> > 
> > When the lock conversion (node PR -> NL) of node 0 is completed, the lock
> > of node 2 should be grantable. However, __can_be_granted() returns 0
> > because the grmode of the lock on node 3 in convert queue is PR.
> > 
> > When checking the lock at the head of convert queue, exclude
> > queue_conflict() targeting convert queue.
> 
> This example doesn't look right.  node2's NL->EX cannot be granted because
> it conflicts with the PR lock held by node3.  (The grmode is still valid
> when a lock is on the convert queue.)

After looking more closely, this is a subtle form of conversion deadlock,
and this exact case is described in the comment here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/dlm/lock.c#n2218

This should be handled by the dlm canceling one of the converting locks
(returning it to the grant queue with IV rqmode) and returning -EDEADLK to
the application.  There is a FIXME in the code highlighting a case you
could be hitting:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/dlm/lock.c#n2504

If you are running into that FIXME, you should see these log messages:

 if (deadlk) {
        log_print("WARN: pending deadlock %x node %d %s",
                  lkb->lkb_id, lkb->lkb_nodeid, r->res_name);
        dlm_dump_rsb(r);
        continue;
 }