[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[Cluster-devel] [PATCH] work around rgmanager-caused DLM race condition, BZ #193128, pass 2



These patches allows users of magma to use NULL locks (where supported)
and attempt to convert in a loop, rather than take/release locks in a
tight loop as is currently done.

-- Lon
Index: lib/global.c
===================================================================
RCS file: /cvs/cluster/cluster/magma/lib/Attic/global.c,v
retrieving revision 1.9.2.1
diff -u -r1.9.2.1 global.c
--- lib/global.c	17 Jan 2005 18:40:05 -0000	1.9.2.1
+++ lib/global.c	15 Jun 2006 16:38:47 -0000
@@ -390,13 +390,66 @@
 int
 clu_lock(char *resource, int flags, void **lockpp)
 {
-	int ret, block = 0, err;
+	int ret = 0, block = 0, conv = 0, err;
 
 	block = !(flags & CLK_NOWAIT);
 
+	/*
+	   Bug 193128
+
+	   Try to use a conversion lock mechanism when possible
+	   If the caller calls explicitly with a NULL lock, then
+	   assume the caller knows what it is doing.
+
+	   Only take the NULL lock if:
+	   (a) the user isn't specifying CONVERT; if they are, they
+	       know what they are doing.
+
+	   ...and one of...
+
+	   (b) This is a blocking call, or
+	   (c) The user requested a NULL lock explicitly.  In this case,
+	       short-out early; there's no reason to convert a NULL lock
+	       to a NULL lock.
+	 */
+	if (!(flags & CLK_CONVERT) &&
+	    (block || ((flags & CLK_EX) == 0))) {
+		/* Acquire NULL lock */
+		pthread_rwlock_wrlock(&dflt_lock);
+		ret = cp_lock(_cpp, resource, CLK_NULL, lockpp);
+		err = errno;
+		pthread_rwlock_unlock(&dflt_lock);
+		if (ret == 0) {
+			if ((flags & CLK_EX) == 0) {
+				/* User only wanted a NULL lock... */
+				return 0;
+			}
+			/*
+			   Ok, NULL lock was taken, rest of blocking
+			   call should be done using lock conversions.
+			 */
+			flags |= CLK_CONVERT;
+			conv = 1;
+		} else {
+			switch(err) {
+			case EINVAL:
+				/* Oops, null locks don't work on this
+				   plugin; use normal spam mode */
+				break;
+			default:
+				errno = err;
+				return -1;
+			}
+		}
+	}
+
 	while (1) {
+		/*
+		printf("upgrading to requested lock %04x...\n", flags);
+		*/
 		pthread_rwlock_wrlock(&dflt_lock);
-		ret = cp_lock(_cpp, resource, flags | CLK_NOWAIT, lockpp);
+		ret = cp_lock(_cpp, resource,
+			      flags | CLK_NOWAIT, lockpp);
 		err = errno;
 		pthread_rwlock_unlock(&dflt_lock);
 
@@ -407,7 +460,16 @@
 
 		break;
 	}
-			
+
+	if (ret != 0 && conv) {
+		/* If we get some other error, release the NL lock we
+		 took so we don't leak locks*/
+		pthread_rwlock_wrlock(&dflt_lock);
+		cp_unlock(_cpp, resource, lockpp);
+		pthread_rwlock_unlock(&dflt_lock);
+		errno = err;
+	}
+
 	return ret;
 }
 
Index: lib/magma.h
===================================================================
RCS file: /cvs/cluster/cluster/magma/lib/Attic/magma.h,v
retrieving revision 1.9.2.3
diff -u -r1.9.2.3 magma.h
--- lib/magma.h	20 Jan 2006 16:04:08 -0000	1.9.2.3
+++ lib/magma.h	15 Jun 2006 16:38:48 -0000
@@ -110,7 +110,11 @@
 #define CLK_WRITE	(1<<1)	/** Write lock */
 #define CLK_READ	(1<<2)	/** Read lock */
 #define CLK_HOLDER	(1<<3)  /** Return the holder node ID if lock is held*/
-#define CLK_EX		(CLK_READ|CLK_WRITE)
+#define CLK_CONVERT	(1<<4)	/** Convert this lock to a new type (may not
+				    be supported on all plugins; caller
+				    must be aware of this fact */
+#define CLK_EX		(CLK_READ|CLK_WRITE) /** Exclusive lock */
+#define CLK_NULL	(0)	/** No bits set */
 
 
 #define NODE_ID_NONE	((uint64_t)-1) /** The nonexistent cluster member */
Index: dumb/dumb.c
===================================================================
RCS file: /cvs/cluster/cluster/magma-plugins/dumb/Attic/dumb.c,v
retrieving revision 1.2
diff -u -r1.2 dumb.c
--- dumb/dumb.c	29 Jun 2004 20:25:49 -0000	1.2
+++ dumb/dumb.c	15 Jun 2006 16:37:28 -0000
@@ -35,7 +35,7 @@
 #include <netinet/in.h>
 #include <assert.h>
 
-#define MODULE_DESCRIPTION "Dumb Plugin v1.1"
+#define MODULE_DESCRIPTION "Dumb Plugin v1.1.1"
 #define MODULE_AUTHOR      "Lon Hohberger"
 #define DUMB_LOCK_PATH	   "/tmp/magma-dumb"
 
@@ -158,6 +158,12 @@
 
 	//printf("DUMB: %s called\n", __FUNCTION__);
 
+	if ((flags & CLK_EX) == 0) {
+		/* NULL lock not supported */
+		errno = EINVAL;
+		return -1;
+	}
+
 	fdp = malloc(sizeof(int));
 	if (!fdp)
 		return -1;
Index: gulm/gulm-lock.c
===================================================================
RCS file: /cvs/cluster/cluster/magma-plugins/gulm/Attic/gulm-lock.c,v
retrieving revision 1.5.2.3
diff -u -r1.5.2.3 gulm-lock.c
--- gulm/gulm-lock.c	27 Jan 2006 20:55:06 -0000	1.5.2.3
+++ gulm/gulm-lock.c	15 Jun 2006 16:37:28 -0000
@@ -269,13 +269,14 @@
 
 	*lockpp = NULL;
 
-	if (flags & CLK_EX) {
+	if ((flags & CLK_EX) == CLK_EX) {
 		state = lg_lock_state_Exclusive;
 	} else if (flags & CLK_READ) {
 		state = lg_lock_state_Shared;
 	} else if (flags & CLK_WRITE) {
 		state = lg_lock_state_Exclusive;
 	} else {
+		/* NULL Locks not supported on GULM */
 		errno = EINVAL;
 		return -1;
 	}
Index: gulm/gulm.c
===================================================================
RCS file: /cvs/cluster/cluster/magma-plugins/gulm/Attic/gulm.c,v
retrieving revision 1.6.2.6
diff -u -r1.6.2.6 gulm.c
--- gulm/gulm.c	24 Jan 2006 19:30:44 -0000	1.6.2.6
+++ gulm/gulm.c	15 Jun 2006 16:37:28 -0000
@@ -34,7 +34,7 @@
 #include <sys/types.h>
 #include <linux/unistd.h>
 
-#define MODULE_DESCRIPTION "GuLM Plugin v1.0.4"
+#define MODULE_DESCRIPTION "GuLM Plugin v1.0.5"
 #define MODULE_AUTHOR      "Lon Hohberger"
 
 
Index: sm/sm.c
===================================================================
RCS file: /cvs/cluster/cluster/magma-plugins/sm/Attic/sm.c,v
retrieving revision 1.9.2.8
diff -u -r1.9.2.8 sm.c
--- sm/sm.c	15 May 2006 16:59:11 -0000	1.9.2.8
+++ sm/sm.c	15 Jun 2006 16:37:29 -0000
@@ -35,7 +35,7 @@
 #include <sys/types.h>
 #include <sys/select.h>
 
-#define MODULE_DESCRIPTION "CMAN/SM Plugin v1.1.6"
+#define MODULE_DESCRIPTION "CMAN/SM Plugin v1.1.7.1"
 #define MODULE_AUTHOR      "Lon Hohberger"
 
 #define DLM_LS_NAME	   "Magma"
@@ -47,6 +47,7 @@
 
 /* Internal */
 static inline int _dlm_release_lockspace(sm_priv_t *p);
+static inline int _dlm_unlock(sm_priv_t *p, struct dlm_lksb *lksb);
 
 
 /*
@@ -560,24 +561,13 @@
 {
         int ret;
 
-        /*
-         * per pjc: create/open lockspace when first lock is taken
-         */
-        ret = dlm_ls_lock(p->ls, mode, lksb, options, resource,
-                          strlen(resource), 0, ast_function, lksb,
-                          NULL, NULL);
-
-        if (ret < 0) {
-#if 0
-                if (errno == ENOENT) {
-                        /* This should not happen if we have a lock
-                           ref open in the LS ! */A
-                        assert(0);
-                }
-#endif
+	/* Ok, we have the NL lock.  Now convert it. */
+        ret = dlm_ls_lock(p->ls, mode, lksb, options,
+			  resource, strlen(resource), 0, ast_function,
+			  lksb, NULL, NULL);
 
+        if (ret < 0)
                 return -1;
-        }
 
         if ((ret = (wait_for_dlm_event(p->ls) < 0))) {
                 fprintf(stderr, "wait_for_dlm_event: %d / %d\n",
@@ -585,6 +575,7 @@
                 return -1;
         }
 
+	/* Got the lock ! */
         return 0;
 }
 
@@ -760,42 +751,59 @@
 
 	p = (sm_priv_t *)self->cp_private.p_data;
 	assert(p);
-	*lockpp = NULL;
-	if (flags & CLK_EX) {
+
+	if ((flags & CLK_EX) == CLK_EX) {
 		mode = LKM_EXMODE;
 	} else if (flags & CLK_READ) {
 		mode = LKM_PRMODE;
 	} else if (flags & CLK_WRITE) {
 		mode = LKM_PWMODE;
-	} else {
-		errno = EINVAL;
-		return -1;
+	} else if ((flags & CLK_EX) == 0){
+		mode = LKM_NLMODE;
 	}
 
 	if (flags & CLK_NOWAIT)
 		options = LKF_NOQUEUE;
+	if (flags & CLK_CONVERT)
+		flags &= ~CLK_HOLDER; /* CLK_HOLDER mutually exclusive with
+					 CLK_CONVERT */
 
 	/* Allocate our lock structure. */
 	sz = (sizeof(*lksb) > sizeof(uint64_t) ? sizeof(*lksb) :
 	      sizeof(uint64_t));
 
-	lksb = malloc(sz);
-	assert(lksb);
-	memset(lksb, 0, sz);
-
-	while(!p->ls) {
+	while(!p->ls)
 		_dlm_acquire_lockspace(p, DLM_LS_NAME);
-	}
 	assert(p->ls);
 
+	/* If we've got a non-zero pointer and we're being called with
+	   the CLK_CONVERT flag, then assume it's a previous lksb with
+	   a held lock. */
+	if ((flags & CLK_CONVERT) && *lockpp) {
+		lksb = (struct lksb *)*lockpp;
+		options |= LKF_CONVERT;
+	} else {
+		lksb = malloc(sz);
+		assert(lksb);
+		memset(lksb, 0, sz);
+	}
+
+	/* Take the real lock, or at least, try to. */
 	ret = _dlm_lock(p, mode, lksb, options, resource);
 
-	switch(lksb->sb_status) {
-	case 0:
+	/* Got the lock? */
+	if (ret == 0) {
 		*lockpp = (void *)lksb;
 		return 0;
+	}
+
+	switch(errno) {
 	case EAGAIN:
-		if ((flags & CLK_HOLDER) &&
+		if (flags & CLK_CONVERT) {
+			*lockpp = (void *)lksb;
+			/* Nothing special here */
+			/* Lock is busy */
+		} else if ((flags & CLK_HOLDER) &&
 		    (_get_holder(resource, p, mode, &holder) == 0)) {
 			memset(lksb, 0, sz);
 			*((uint64_t *)lksb) = holder;
@@ -806,7 +814,7 @@
 		errno = EAGAIN;
 		return -1;
 	default:
-		fprintf(stderr, "_dlm_lock: %d / %d\n", ret, lksb->sb_status);
+		fprintf(stderr, "_dlm_lock: %d / %d\n", ret, errno);
 		ret = lksb->sb_status;
 		free(lksb);
 		errno = ret;
Index: src/daemons/fo_domain.c
===================================================================
RCS file: /cvs/cluster/cluster/rgmanager/src/daemons/fo_domain.c,v
retrieving revision 1.5.2.3
diff -u -r1.5.2.3 fo_domain.c
--- src/daemons/fo_domain.c	12 May 2006 21:28:31 -0000	1.5.2.3
+++ src/daemons/fo_domain.c	15 Jun 2006 16:36:42 -0000
@@ -332,8 +332,10 @@
 	fod_t *fod = NULL;
 	int found = 0;
 	int owned_by_node = 0, started = 0, no_owner = 0;
+#ifndef NO_CCS
 	rg_state_t svc_state;
 	void *lockp;
+#endif
 
 	ENTER();
 
Index: src/daemons/groups.c
===================================================================
RCS file: /cvs/cluster/cluster/rgmanager/src/daemons/groups.c,v
retrieving revision 1.8.2.16
diff -u -r1.8.2.16 groups.c
--- src/daemons/groups.c	12 Jun 2006 18:40:11 -0000	1.8.2.16
+++ src/daemons/groups.c	15 Jun 2006 16:36:42 -0000
@@ -757,7 +757,6 @@
 	resource_node_t *curr;
 	char *name;
 	rg_state_t svcblk;
-	void *lockp;
 
 	pthread_rwlock_rdlock(&resource_lock);
 	list_do(&_tree, curr) {
@@ -814,7 +813,6 @@
 	char *name;
 	rg_state_t svcblk;
 	int need_kill;
-	void *lockp;
 
 	clulog(LOG_INFO, "Stopping changed resources.\n");
 
Index: src/daemons/rg_state.c
===================================================================
RCS file: /cvs/cluster/cluster/rgmanager/src/daemons/rg_state.c,v
retrieving revision 1.4.2.13
diff -u -r1.4.2.13 rg_state.c
--- src/daemons/rg_state.c	26 May 2006 15:32:00 -0000	1.4.2.13
+++ src/daemons/rg_state.c	15 Jun 2006 16:36:43 -0000
@@ -117,6 +117,7 @@
 	struct timeval start, now;
 	uint64_t nodeid, *p;
 	int flags;
+	int conv = 0, err;
 	int block = !(dflt_flags & CLK_NOWAIT);
 
 	/* Holder not supported for this call */
@@ -128,6 +129,37 @@
 		gettimeofday(&start, NULL);
 		start.tv_sec += 30;
 	}
+
+	/* Ripped from global.c in magma */
+	if (!(dflt_flags & CLK_CONVERT) &&
+	    (block || ((dflt_flags & CLK_EX) == 0))) {
+		/* Acquire NULL lock */
+		ret = clu_lock(resource, CLK_NULL, lockpp);
+		err = errno;
+		if (ret == 0) {
+			if ((flags & CLK_EX) == 0) {
+				/* User only wanted a NULL lock... */
+				return 0;
+			}
+			/*
+			   Ok, NULL lock was taken, rest of blocking
+			   call should be done using lock conversions.
+			 */
+			flags |= CLK_CONVERT;
+			conv = 1;
+		} else {
+			switch(err) {
+			case EINVAL:
+				/* Oops, null locks don't work on this
+				   plugin; use normal spam mode */
+				break;
+			default:
+				errno = err;
+				return -1;
+			}
+		}
+	}
+
 	while (1) {
 		if (block) {
 			gettimeofday(&now, NULL);
@@ -145,9 +177,14 @@
 		}
 
 		*lockpp = NULL;
-		ret = clu_lock(resource, flags | CLK_NOWAIT, lockpp);
 
-		if ((ret != 0) && (errno == EAGAIN) && block) {
+		/* Take the lock (convert if possible). */
+		ret = clu_lock(resource, flags | CLK_NOWAIT |
+			       ((conv && !timed_out) ? CLK_CONVERT : 0),
+			       lockpp);
+		err = errno;
+
+		if ((ret != 0) && (err == EAGAIN) && block) {
 			if (timed_out) {
 				p = (uint64_t *)*lockpp;
 				if (p) {
@@ -176,6 +213,16 @@
 		break;
 	}
 
+	/* Fatal error.  If we took an automatic NL lock with the hopes of
+	   converting it, release the lock before returning */
+	if (conv == 1 && ret < 0) {
+		clu_unlock(resource, *lockpp);
+		*lockpp = NULL;
+	}
+
+	if (ret < 0)
+		errno = err;
+
 	return ret;
 }
 

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]