[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