[Cluster-devel] [PATCH 13/18] [try #2] DLM: fix conversion deadlock when DLM_LKF_NODLCKWT flag is set

[tsutomu.owa at toshiba.co.jp]

Hi,

> Thanks for sorting this out.  Could we expand this with a description of
> the externally visible effects from the change?

This fix has no effect except when using DLM_LKF_NODLCKWT flag.
We are using fsdlm with ocfs2. Currently, DLM_LKF_NODLCKWT flag is used
only by ocfs2. ocfs2 does not expect a cancel operation by detecting
conversion deadlock when calling dlm_lock(). ocfs2 is implemented to
perform a cancel operation by requesting BASTs (callback).

> Is this the main change in behavior you are looking for?  It seems to make
> sense.  Do you find that this case is occurring during normal operation?

Conversion deadlock was infrequently seen when multiple nodes mount/umount
ocfs2 concurrently. It seems that it happens mainly after the master node
exits from lock space and the recovery process has run on another node.
However, since it depends on the operation timing for each node whether
it occurs or not, it will also occur in another cases.

> I did not previously expect that this code path would be used, but if it
> is, then we should change the FIXME comment to explain how and when it
> happens, and the warning and dump_rsb should be suppresssed.

How about with the corrections below?
(Let us leave the fix of FIXME to you.)

thanks,
-- owa
p.s. Could we re-send try#3 patch not to add extra/unused "int exflags" in grant_pending_convert()?

--- fs/dlm/lock.c.orig  2017-09-13 14:29:37.604795992 +0900
+++ fs/dlm/lock.c       2017-09-13 14:41:26.013722764 +0900
@@ -2544,18 +2544,21 @@
 		}
 
 		if (deadlk) {
-			log_print("WARN: pending deadlock %x node %d %s",
-				  lkb->lkb_id, lkb->lkb_nodeid, r->res_name);
-			dlm_dump_rsb(r);
 			/*
 			 * If DLM_LKB_NODLKWT flag is set and conversion
 			 * deadlock is detected, we request blocking AST and
 			 * down (or cancel) conversion.
 			 */
-			if (lkb->lkb_exflags & DLM_LKF_NODLCKWT &&
-			    lkb->lkb_highbast < lkb->lkb_rqmode) {
-				queue_bast(r, lkb, lkb->lkb_rqmode);
-				lkb->lkb_highbast = lkb->lkb_rqmode;
+			if (lkb->lkb_exflags & DLM_LKF_NODLCKWT) {
+				if (lkb->lkb_highbast < lkb->lkb_rqmode) {
+					queue_bast(r, lkb, lkb->lkb_rqmode);
+					lkb->lkb_highbast = lkb->lkb_rqmode;
+				}
+			} else {
+				log_print("WARN: pending deadlock %x node %d %s",
+					  lkb->lkb_id, lkb->lkb_nodeid,
+					  r->res_name);
+				dlm_dump_rsb(r);
 			}
 			continue;
 		}
---

-----Original Message-----
From: David Teigland [mailto:teigland at redhat.com] 
Sent: Wednesday, September 13, 2017 1:56 AM
To: owa tsutomu(大輪 勤 ＴＭＣ ○ＳＳＤジ□ＥＳ技○ＥＳ五)
Cc: cluster-devel at redhat.com; miyauchi tadashi(宮内 忠志 ＴＯＰＳ （ＳＷ開）［基本］)
Subject: Re: [Cluster-devel] [PATCH 13/18] [try #2] DLM: fix conversion deadlock when DLM_LKF_NODLCKWT flag is set

On Tue, Sep 12, 2017 at 09:01:31AM +0000, tsutomu.owa at toshiba.co.jp wrote:
> When the DLM_LKF_NODLCKWT flag was set, even if conversion deadlock
> was detected, the caller of can_be_granted() was unknown.
> We change the behavior of can_be_granted() and change it to detect
> conversion deadlock regardless of whether the DLM_LKF_NODLCKWT flag
> is set or not. And depending on whether the DLM_LKF_NODLCKWT flag
> is set or not, we change the behavior at the caller of can_be_granted().

Thanks for sorting this out.  Could we expand this with a description of
the externally visible effects from the change?

> @@ -2549,6 +2548,17 @@ static int grant_pending_convert(struct dlm_rsb *r, int high, int *cw,
>  			log_print("WARN: pending deadlock %x node %d %s",
>  				  lkb->lkb_id, lkb->lkb_nodeid, r->res_name);
>  			dlm_dump_rsb(r);
> +			/*
> +			 * If DLM_LKB_NODLKWT flag is set and conversion
> +			 * deadlock is detected, we request blocking AST and
> +			 * down (or cancel) conversion.
> +			 */
> +			if (lkb->lkb_exflags & DLM_LKF_NODLCKWT &&
> +			    lkb->lkb_highbast < lkb->lkb_rqmode) {
> +				queue_bast(r, lkb, lkb->lkb_rqmode);
> +				lkb->lkb_highbast = lkb->lkb_rqmode;
> +			}

Is this the main change in behavior you are looking for?  It seems to make
sense.  Do you find that this case is occurring during normal operation?
I did not previously expect that this code path would be used, but if it
is, then we should change the FIXME comment to explain how and when it
happens, and the warning and dump_rsb should be suppresssed.

> +
>  			continue;
>  		}
>  
> @@ -3124,7 +3134,7 @@ static int do_convert(struct dlm_rsb *r, struct dlm_lkb *lkb)
>  	   deadlock, so we leave it on the granted queue and return EDEADLK in
>  	   the ast for the convert. */
>  
> -	if (deadlk) {
> +	if (deadlk && !(lkb->lkb_exflags & DLM_LKF_NODLCKWT)) {
>  		/* it's left on the granted queue */
>  		revert_lock(r, lkb);
>  		queue_cast(r, lkb, -EDEADLK);
> -- 
> 2.7.4