r10421: following on discussions with simo, I have worked out a way of

allowing searches to proceed while another process is in a
transaction, then only upgrading the transaction lock to a write lock
on commit.

The solution is:

 - split tdb_traverse() into two calls, called tdb_traverse() and
   tdb_traverse_read(). The _read() version only gets read locks, and
   will fail any write operations made in the callback from the
   traverse.

 - the normal tdb_traverse() call allows for read or write operations
   in the callback, but gets the transaction lock, preventing
   transastions from starting inside the traverse

In addition we enforce the following rule that you may not start a
transaction within a traverse callback, although you can start a
traverse within a transaction

With these rules in place I believe all the deadlock possibilities are
removed, and we can now allow for searches to happen in parallel with
transactions
(This used to be commit 7dd31288a701d772e45b1960ac4ce4cc1be782ed)

1 files changed, 20 insertions, 9 deletions

diff --git a/source4/lib/tdb/common/transaction.c b/source4/lib/tdb/common/transaction.c
index b9d44a7283..75e91c56cc 100644
--- a/source4/lib/tdb/common/transaction.c
+++ b/source4/lib/tdb/common/transaction.c
@@ -372,6 +372,15 @@ int tdb_transaction_start(struct tdb_context *tdb)
 		return -1;
 	}
 
+	if (tdb->travlocks.next != NULL) {
+		/* you cannot use transactions inside a traverse (although you can use
+		   traverse inside a transaction) as otherwise you can end up with
+		   deadlock */
+		TDB_LOG((tdb, 0, "tdb_transaction_start: cannot start a transaction within a traverse\n"));
+		tdb->ecode = TDB_ERR_LOCK;
+		return -1;
+	}
+
 	tdb->transaction = calloc(sizeof(struct tdb_transaction), 1);
 	if (tdb->transaction == NULL) {
 		tdb->ecode = TDB_ERR_OOM;
@@ -388,15 +397,9 @@ int tdb_transaction_start(struct tdb_context *tdb)
 		return -1;
 	}
 	
-	/* get a write lock from the freelist to the end of file. It
-	   would be much better to make this a read lock as it would
-	   increase parallelism, but it could lead to deadlocks on
-	   commit when a write lock needs to be taken. 
-
-	   TODO: look at alternative locking strategies to allow this
-	   to be a read lock 
-	*/
-	if (tdb_brlock_len(tdb, FREELIST_TOP, F_WRLCK, F_SETLKW, 0, 0) == -1) {
+	/* get a read lock from the freelist to the end of file. This
+	   is upgraded to a write lock during the commit */
+	if (tdb_brlock_len(tdb, FREELIST_TOP, F_RDLCK, F_SETLKW, 0, 0) == -1) {
 		TDB_LOG((tdb, 0, "tdb_transaction_start: failed to get hash locks\n"));
 		tdb->ecode = TDB_ERR_LOCK;
 		goto fail;
@@ -763,6 +766,14 @@ int tdb_transaction_commit(struct tdb_context *tdb)
 		return -1;
 	}
 
+	/* upgrade the main transaction lock region to a write lock */
+	if (tdb_brlock_len(tdb, FREELIST_TOP, F_WRLCK, F_SETLKW, 0, 0) == -1) {
+		TDB_LOG((tdb, 0, "tdb_transaction_start: failed to upgrade hash locks\n"));
+		tdb->ecode = TDB_ERR_LOCK;
+		tdb_transaction_cancel(tdb);
+		return -1;
+	}
+
 	/* get the global lock - this prevents new users attaching to the database
 	   during the commit */
 	if (tdb_brlock_len(tdb, GLOBAL_LOCK, F_WRLCK, F_SETLKW, 0, 1) == -1) {