[PATCH] tdb and gencache cleanups

Volker Lendecke Volker.Lendecke at SerNet.DE
Thu Aug 17 08:28:37 UTC 2017


Hi!

Attached find a small tdb refactoring for easier reading and two
cleanup patches for gencache.

Review appreciated!

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 b3980304eb7ce6a8b3d6cbb304596b807b28d0c4 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 16 Aug 2017 15:21:14 +0200
Subject: [PATCH 1/3] tdb: Clarify the CLEAR_IF_FIRST locked logic

This is another level of indentation, but it took me a while staring at the
if-condition to find that "locked" was assigned the result of "==0", not the
return value of tdb_nest_lock().

Best viewed with "git show -b".

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tdb/common/open.c | 70 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/lib/tdb/common/open.c b/lib/tdb/common/open.c
index f3ef856eae1..cfb476d2385 100644
--- a/lib/tdb/common/open.c
+++ b/lib/tdb/common/open.c
@@ -300,7 +300,8 @@ _PUBLIC_ struct tdb_context *tdb_open_ex(const char *name, int hash_size, int td
 	struct tdb_header header;
 	struct tdb_context *tdb;
 	struct stat st;
-	int rev = 0, locked = 0;
+	int rev = 0;
+	bool locked = false;
 	unsigned char *vp;
 	uint32_t vertest;
 	unsigned v;
@@ -512,37 +513,42 @@ _PUBLIC_ struct tdb_context *tdb_open_ex(const char *name, int hash_size, int td
 
 	/* we need to zero database if we are the only one with it open */
 	if ((tdb_flags & TDB_CLEAR_IF_FIRST) &&
-	    (!tdb->read_only) &&
-	    (locked = (tdb_nest_lock(tdb, ACTIVE_LOCK, F_WRLCK, TDB_LOCK_NOWAIT|TDB_LOCK_PROBE) == 0))) {
-		ret = tdb_brlock(tdb, F_WRLCK, FREELIST_TOP, 0,
-				 TDB_LOCK_WAIT);
-		if (ret == -1) {
-			TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_open_ex: "
-				 "tdb_brlock failed for %s: %s\n",
-				 name, strerror(errno)));
-			goto fail;
-		}
-		ret = tdb_new_database(tdb, &header, hash_size);
-		if (ret == -1) {
-			TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_open_ex: "
-				 "tdb_new_database failed for %s: %s\n",
-				 name, strerror(errno)));
-			tdb_unlockall(tdb);
-			goto fail;
-		}
-		ret = tdb_brunlock(tdb, F_WRLCK, FREELIST_TOP, 0);
-		if (ret == -1) {
-			TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_open_ex: "
-				 "tdb_unlockall failed for %s: %s\n",
-				 name, strerror(errno)));
-			goto fail;
-		}
-		ret = lseek(tdb->fd, 0, SEEK_SET);
-		if (ret == -1) {
-			TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_open_ex: "
-				 "lseek failed for %s: %s\n",
-				 name, strerror(errno)));
-			goto fail;
+	    (!tdb->read_only)) {
+		ret = tdb_nest_lock(tdb, ACTIVE_LOCK, F_WRLCK,
+				    TDB_LOCK_NOWAIT|TDB_LOCK_PROBE);
+		locked = (ret == 0);
+
+		if (locked) {
+			ret = tdb_brlock(tdb, F_WRLCK, FREELIST_TOP, 0,
+					 TDB_LOCK_WAIT);
+			if (ret == -1) {
+				TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_open_ex: "
+					 "tdb_brlock failed for %s: %s\n",
+					 name, strerror(errno)));
+				goto fail;
+			}
+			ret = tdb_new_database(tdb, &header, hash_size);
+			if (ret == -1) {
+				TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_open_ex: "
+					 "tdb_new_database failed for "
+					 "%s: %s\n", name, strerror(errno)));
+				tdb_unlockall(tdb);
+				goto fail;
+			}
+			ret = tdb_brunlock(tdb, F_WRLCK, FREELIST_TOP, 0);
+			if (ret == -1) {
+				TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_open_ex: "
+					 "tdb_unlockall failed for %s: %s\n",
+					 name, strerror(errno)));
+				goto fail;
+			}
+			ret = lseek(tdb->fd, 0, SEEK_SET);
+			if (ret == -1) {
+				TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_open_ex: "
+					 "lseek failed for %s: %s\n",
+					 name, strerror(errno)));
+				goto fail;
+			}
 		}
 	}
 
-- 
2.11.0


From f1957a2b3c1661c97f408e083ec52f1ef68b9210 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 16 Aug 2017 16:22:27 +0200
Subject: [PATCH 2/3] gencache: Remove tdb_check from gencache_init()

This was legacy from times when we had just one non-transactioned gencache.tdb.
With the split into transactioned gencache.tdb and fast, non-transactioned,
mutexed clear-if-first gencache_notrans.tdb this has become unnecessary.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/gencache.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 1572825f605..24edcc7fc4e 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -74,29 +74,6 @@ static bool gencache_init(void)
 	cache = tdb_wrap_open(NULL, cache_fname, hash_size,
 			      TDB_DEFAULT|TDB_INCOMPATIBLE_HASH,
 			      open_flags, 0644);
-	if (cache) {
-		int ret;
-		ret = tdb_check(cache->tdb, NULL, NULL);
-		if (ret != 0) {
-			TALLOC_FREE(cache);
-
-			/*
-			 * Retry with CLEAR_IF_FIRST.
-			 *
-			 * Warning: Converting this to dbwrap won't work
-			 * directly. gencache.c does transactions on this tdb,
-			 * and dbwrap forbids this for CLEAR_IF_FIRST
-			 * databases. tdb does allow transactions on
-			 * CLEAR_IF_FIRST databases, so lets use it here to
-			 * clean up a broken database.
-			 */
-			cache = tdb_wrap_open(NULL, cache_fname, hash_size,
-					      TDB_DEFAULT|
-					      TDB_INCOMPATIBLE_HASH|
-					      TDB_CLEAR_IF_FIRST,
-					      open_flags, 0644);
-		}
-	}
 
 	if (!cache && (errno == EACCES)) {
 		open_flags = O_RDONLY;
-- 
2.11.0


From 3b99f77eeea7fc2189e8aa6f0b3e505d11a0e116 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 16 Aug 2017 17:37:41 +0200
Subject: [PATCH 3/3] gencache: Simplify gencache_stabilize

The only record that must remain in gencache_notrans.tdb is the last_stabilize
marker. Use tdb_wipe_all and store the marker under the allrecord lock.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/gencache.c | 55 +++++++++-----------------------------------------
 1 file changed, 10 insertions(+), 45 deletions(-)

diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 24edcc7fc4e..e73d1c52a0e 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -599,9 +599,6 @@ struct stabilize_state {
 static int stabilize_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
 			void *priv);
 
-static int wipe_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
-		   void *priv);
-
 /**
  * Stabilize gencache
  *
@@ -667,20 +664,10 @@ bool gencache_stabilize(void)
 		return false;
 	}
 
-	res = tdb_traverse(cache_notrans->tdb, wipe_fn, NULL);
+	res = tdb_wipe_all(cache_notrans->tdb);
 	if (res < 0) {
-		DEBUG(10, ("tdb_traverse with wipe_fn on gencache_notrans.tdb "
-			  "failed: %s\n",
-			   tdb_errorstr(cache_notrans->tdb)));
-		tdb_unlockall(cache_notrans->tdb);
-		return false;
-	}
-
-	res = tdb_unlockall(cache_notrans->tdb);
-	if (res != 0) {
-		DEBUG(10, ("tdb_unlockall on gencache.tdb failed: "
-			   "%s\n", tdb_errorstr(cache->tdb)));
-		return false;
+		DBG_DEBUG("tdb_wipe_all on gencache_notrans.tdb failed: %s\n",
+			  tdb_errorstr(cache_notrans->tdb));
 	}
 
 	now = talloc_asprintf(talloc_tos(), "%d", (int)time(NULL));
@@ -690,6 +677,13 @@ bool gencache_stabilize(void)
 		TALLOC_FREE(now);
 	}
 
+	res = tdb_unlockall(cache_notrans->tdb);
+	if (res != 0) {
+		DEBUG(10, ("tdb_unlockall on gencache.tdb failed: "
+			   "%s\n", tdb_errorstr(cache->tdb)));
+		return false;
+	}
+
 	return true;
 }
 
@@ -731,35 +725,6 @@ static int stabilize_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
 	return 0;
 }
 
-static int wipe_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
-		   void *priv)
-{
-	int res;
-	bool ok;
-	time_t timeout;
-
-	res = tdb_data_cmp(key, last_stabilize_key());
-	if (res == 0) {
-		return 0;
-	}
-
-	ok = gencache_pull_timeout(val.dptr, &timeout, NULL);
-	if (!ok) {
-		DEBUG(10, ("Ignoring invalid entry\n"));
-		return 0;
-	}
-
-	res = tdb_delete(tdb, key);
-	if (res != 0) {
-		DEBUG(10, ("tdb_delete from gencache_notrans.tdb failed: "
-			   "%s\n", tdb_errorstr(cache_notrans->tdb)));
-		return -1;
-	}
-
-	return 0;
-}
-
-
 /**
  * Get existing entry from the cache file.
  *
-- 
2.11.0



More information about the samba-technical mailing list