tdb: Remove locking from tdb_traverse_read()

This restores the original intent of tdb_traverse_read() in
7dd31288a701d772e45b1960ac4ce4cc1be782ed

This is needed to avoid a deadlock with tdb_lockall() and the
transaction start, as ldb_tdb should take the allrecord lock during a
search (which calls tdb_traverse), and can otherwise deadlock against
a transaction starting in another process

We add a test to show that a transaction can now start while a read
traverse is in progress

This allows more operations to happen in parallel.  The blocking point
is moved to the prepare commit.

This in turn permits a roughly doubling of unindexed search
performance, because currently ldb_tdb omits to take the lock due to
an unrelated bug, but taking the allrecord lock triggers the
above-mentioned deadlock.

This behaviour was added in 251aaafe3a9213118ac3a92def9ab2104c40d12a for
Solaris 10 in 2005. But the run-fcntl-deadlock test works also on Solaris 10,
see https://lists.samba.org/archive/samba-technical/2017-April/119876.html.

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Stefan Metzmacher <metze@samba.org>

diff --git a/lib/tdb/common/traverse.c b/lib/tdb/common/traverse.c
index f33ef34ab8d344ad633396580457a63ebc543420..f62306e5560975999c5cb9df4e303a78f3a59a73 100644 (file)
--- a/lib/tdb/common/traverse.c
+++ b/lib/tdb/common/traverse.c
@@ -244,7 +244,7 @@ out:
 
 
 /*
-  a read style traverse - temporarily marks the db read only
+  a read style traverse - temporarily marks each record read only
 */
 _PUBLIC_ int tdb_traverse_read(struct tdb_context *tdb,
                      tdb_traverse_func fn, void *private_data)
@@ -252,19 +252,11 @@ _PUBLIC_ int tdb_traverse_read(struct tdb_context *tdb,
        struct tdb_traverse_lock tl = { NULL, 0, 0, F_RDLCK };
        int ret;
 
-       /* we need to get a read lock on the transaction lock here to
-          cope with the lock ordering semantics of solaris10 */
-       if (tdb_transaction_lock(tdb, F_RDLCK, TDB_LOCK_WAIT)) {
-               return -1;
-       }
-
        tdb->traverse_read++;
        tdb_trace(tdb, "tdb_traverse_read_start");
        ret = tdb_traverse_internal(tdb, fn, private_data, &tl);
        tdb->traverse_read--;
 
-       tdb_transaction_unlock(tdb, F_RDLCK);
-
        return ret;
 }
 

diff --git a/lib/tdb/test/run-nested-traverse.c b/lib/tdb/test/run-nested-traverse.c
index 22ee3e2a2a625b5f974c75d09d2dfd8ea491adf1..aeaa08597932f849c94e7b53ee2ec34db3ea37ba 100644 (file)
--- a/lib/tdb/test/run-nested-traverse.c
+++ b/lib/tdb/test/run-nested-traverse.c
@@ -41,7 +41,30 @@ static int traverse2(struct tdb_context *tdb, TDB_DATA key, TDB_DATA data,
        return 0;
 }
 
-static int traverse1(struct tdb_context *tdb, TDB_DATA key, TDB_DATA data,
+static int traverse1r(struct tdb_context *tdb, TDB_DATA key, TDB_DATA data,
+                    void *p)
+{
+       ok1(correct_key(key));
+       ok1(correct_data(data));
+       ok1(external_agent_operation(agent, TRANSACTION_START, tdb_name(tdb))
+           == SUCCESS);
+       ok1(external_agent_operation(agent, STORE, tdb_name(tdb))
+           == SUCCESS);
+       ok1(external_agent_operation(agent, TRANSACTION_COMMIT, tdb_name(tdb))
+           == WOULD_HAVE_BLOCKED);
+       tdb_traverse(tdb, traverse2, NULL);
+
+       /* That should *not* release the all-records lock! */
+       ok1(external_agent_operation(agent, TRANSACTION_START, tdb_name(tdb))
+           == SUCCESS);
+       ok1(external_agent_operation(agent, STORE, tdb_name(tdb))
+           == SUCCESS);
+       ok1(external_agent_operation(agent, TRANSACTION_COMMIT, tdb_name(tdb))
+           == WOULD_HAVE_BLOCKED);
+       return 0;
+}
+
+static int traverse1w(struct tdb_context *tdb, TDB_DATA key, TDB_DATA data,
                     void *p)
 {
        ok1(correct_key(key));
@@ -50,7 +73,7 @@ static int traverse1(struct tdb_context *tdb, TDB_DATA key, TDB_DATA data,
            == WOULD_HAVE_BLOCKED);
        tdb_traverse(tdb, traverse2, NULL);
 
-       /* That should *not* release the transaction lock! */
+       /* That should *not* release the all-records lock! */
        ok1(external_agent_operation(agent, TRANSACTION_START, tdb_name(tdb))
            == WOULD_HAVE_BLOCKED);
        return 0;
@@ -80,8 +103,8 @@ int main(int argc, char *argv[])
        data.dsize = strlen("world");
 
        ok1(tdb_store(tdb, key, data, TDB_INSERT) == 0);
-       tdb_traverse(tdb, traverse1, NULL);
-       tdb_traverse_read(tdb, traverse1, NULL);
+       tdb_traverse(tdb, traverse1w, NULL);
+       tdb_traverse_read(tdb, traverse1r, NULL);
        tdb_close(tdb);
 
        return exit_status();