[PATCH] CTDB locking changes

[Volker Lendecke]

On Mon, Jul 28, 2014 at 12:22:56PM +1000, Amitay Isaacs wrote:
> On Wed, Jul 23, 2014 at 1:11 PM, Amitay Isaacs <amitay at gmail.com> wrote:
> 
> > Hi,
> >
> > Here are the patches that clean up locking in CTDB.
> >
> > The major changes include:
> >
> > 1. Removal of multiple lock requests per context feature.
> >
> > This was a really bad idea and caused out-of-order scheduling of lock
> > requests.  The code was already commented and not in use.
> >
> > 2. Immediate scheduling of lock request provided there aren't max number
> > of requests active.
> >
> > The scheme to avoid scheduling record lock requests which are already
> > active, caused high CPU usage when there were lots of pending requests.
> > The implementation of pending and active queues using linked list is a bad
> > choice of data structure for this purpose.  In future, it might be possible
> > to improve the scheduling using per database queues rather than a single
> > queue.
> >
> 
> > Minor changes include:
> >
> > 1. Removing unused elements of data structures, unused function
> >
> > 2. Add a new tunable LockProcessesPerDB.  Default is 200.
> >
> >  3. Update num_pending statistics when lock is scheduled and not when lock
> > is obtained to give the correct picture of how many lock requests are still
> > in the pending queue.
> >
> > Please review and  push if ok.
> >
> > Amitay.
> >
> 
> Single queue for pending requests is not good enough.  On a loaded system
> there can thousands of lock requests in the pending queue.  In that case,
> per database queues definitely improves the cpu utilization for
> scheduling.  Here are all the locking changes with additional patch for per
> database queues.
> 
> I would appreciate review and comments/suggestions.

Pushed. Attached find two cosmetic patches.

There's one question that I did not really fully understand:
Why do we have lock_ctx->request as a separate entity? Is
this legacy from when there was a queue of request and can
be removed at some point?

Thanks,

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From aca30de0063948e168696f5914d412c9dbbdf58d Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 4 Aug 2014 12:41:06 +0000
Subject: [PATCH 1/2] ctdb-locking: TALLOC_FREE copes with NULL

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 ctdb/server/ctdb_lock.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ctdb/server/ctdb_lock.c b/ctdb/server/ctdb_lock.c
index a403e25..a7df6cf 100644
--- a/ctdb/server/ctdb_lock.c
+++ b/ctdb/server/ctdb_lock.c
@@ -432,9 +432,7 @@ static void ctdb_lock_handler(struct tevent_context *ev,
 	lock_ctx = talloc_get_type_abort(private_data, struct lock_context);
 
 	/* cancel the timeout event */
-	if (lock_ctx->ttimer) {
-		TALLOC_FREE(lock_ctx->ttimer);
-	}
+	TALLOC_FREE(lock_ctx->ttimer);
 
 	t = timeval_elapsed(&lock_ctx->start_time);
 	id = lock_bucket_id(t);
-- 
1.7.9.5


From eb84c4b99239053565ec416de31333f571c478e6 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 4 Aug 2014 13:57:12 +0000
Subject: [PATCH 2/2] ctdb-locking: Simplify ctdb_find_lock_context()

I like early returns that avoid else branches :-)

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 ctdb/server/ctdb_lock.c |   67 +++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/ctdb/server/ctdb_lock.c b/ctdb/server/ctdb_lock.c
index a7df6cf..72352af 100644
--- a/ctdb/server/ctdb_lock.c
+++ b/ctdb/server/ctdb_lock.c
@@ -664,51 +664,50 @@ struct lock_context *ctdb_find_lock_context(struct ctdb_context *ctdb)
 	struct ctdb_db_context *ctdb_db;
 
 	/* First check if there are database lock requests */
-	lock_ctx = ctdb->lock_pending;
-	while (lock_ctx != NULL) {
-		next_ctx = lock_ctx->next;
-		if (! lock_ctx->request) {
-			DEBUG(DEBUG_INFO, ("Removing lock context without lock request\n"));
-			DLIST_REMOVE(ctdb->lock_pending, lock_ctx);
-			CTDB_DECREMENT_STAT(ctdb, locks.num_pending);
-			if (lock_ctx->ctdb_db) {
-				CTDB_DECREMENT_DB_STAT(lock_ctx->ctdb_db, locks.num_pending);
-			}
-			talloc_free(lock_ctx);
-		} else {
-			/* Found a lock context with lock requests */
-			break;
+
+	for (lock_ctx = ctdb->lock_pending; lock_ctx != NULL;
+	     lock_ctx = next_ctx) {
+
+		if (lock_ctx->request != NULL) {
+			/* Found a lock context with a request */
+			return lock_ctx;
 		}
-		lock_ctx = next_ctx;
-	}
 
-	if (lock_ctx) {
-		return lock_ctx;
+		next_ctx = lock_ctx->next;
+
+		DEBUG(DEBUG_INFO, ("Removing lock context without lock "
+				   "request\n"));
+		DLIST_REMOVE(ctdb->lock_pending, lock_ctx);
+		CTDB_DECREMENT_STAT(ctdb, locks.num_pending);
+		if (lock_ctx->ctdb_db) {
+			CTDB_DECREMENT_DB_STAT(lock_ctx->ctdb_db,
+					       locks.num_pending);
+		}
+		talloc_free(lock_ctx);
 	}
 
 	/* Next check database queues */
 	for (ctdb_db = ctdb->db_list; ctdb_db; ctdb_db = ctdb_db->next) {
-		if (ctdb_db->lock_num_current == ctdb->tunable.lock_processes_per_db) {
+		if (ctdb_db->lock_num_current ==
+		    ctdb->tunable.lock_processes_per_db) {
 			continue;
 		}
-		lock_ctx = ctdb_db->lock_pending;
-		while (lock_ctx != NULL) {
+
+		for (lock_ctx = ctdb_db->lock_pending; lock_ctx != NULL;
+		     lock_ctx = next_ctx) {
+
 			next_ctx = lock_ctx->next;
-			if (! lock_ctx->request) {
-				DEBUG(DEBUG_INFO, ("Removing lock context without lock request\n"));
-				DLIST_REMOVE(ctdb_db->lock_pending, lock_ctx);
-				CTDB_DECREMENT_STAT(ctdb, locks.num_pending);
-				CTDB_DECREMENT_DB_STAT(ctdb_db, locks.num_pending);
-				talloc_free(lock_ctx);
-			} else {
-				break;
-			}
 
-			lock_ctx = next_ctx;
-		}
+			if (lock_ctx->request != NULL) {
+				return lock_ctx;
+			}
 
-		if (lock_ctx) {
-			return lock_ctx;
+			DEBUG(DEBUG_INFO, ("Removing lock context without "
+					   "lock request\n"));
+			DLIST_REMOVE(ctdb_db->lock_pending, lock_ctx);
+			CTDB_DECREMENT_STAT(ctdb, locks.num_pending);
+			CTDB_DECREMENT_DB_STAT(ctdb_db, locks.num_pending);
+			talloc_free(lock_ctx);
 		}
 	}
 
-- 
1.7.9.5