[PATCH] lib: Fix gencache_del
Volker Lendecke
Volker.Lendecke at SerNet.DE
Sun Mar 6 15:24:33 UTC 2016
On Fri, Mar 04, 2016 at 01:18:47PM +0100, Volker Lendecke wrote:
> On Fri, Mar 04, 2016 at 08:15:51AM +0100, Volker Lendecke wrote:
> > On Thu, Mar 03, 2016 at 07:08:18PM +0100, Michael Adam wrote:
> > > Unfortunately, this fails make test due to a slight
> > > change in semantics of delete:
> >
> > Attached find a patchset that survived a private autobuild for me.
> >
> > Comments welcome!
>
> Not a good day for me today. Working on the next version of this
> patchset.
Next try. Comments 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 c63ffdaf8a93a225f7cf3d558375e2095190e17e Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 3 Mar 2016 15:59:05 +0100
Subject: [PATCH 1/8] lib: skip deleted entries in gencache_iterate
"net cache flush" can give nasty error messages like
Couldn't delete entry! key = IDMAP/UID2SID/12345
These happen when there's an already deleted entry in
gencache_notrans.tdb, indicated by a 0 timeout. This happens if two
gencache_del function calls have happened right after the other and a
gencache_stabilize has not wiped them.
In gencache_iterate, don't show these deleted entries
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/lib/gencache.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index c353aa6..29fadec 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -894,6 +894,11 @@ static int gencache_iterate_blobs_fn(struct tdb_context *tdb, TDB_DATA key,
}
endptr += 1;
+ if (timeout == 0) {
+ /* delete marker */
+ goto done;
+ }
+
if (fnmatch(state->pattern, keystr, 0) != 0) {
goto done;
}
--
1.9.1
From 33b8fe4649c0ae0ef2e02bf5af2653a2d6040f9c Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 3 Mar 2016 15:59:05 +0100
Subject: [PATCH 2/8] lib: Simplify gencache_del
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/lib/gencache.c | 45 ++++++++++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 29fadec..b0242f6 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -370,6 +370,15 @@ done:
return ret == 0;
}
+static void gencache_del_parser(time_t timeout, DATA_BLOB blob,
+ void *private_data)
+{
+ if (timeout != 0) {
+ bool *exists = private_data;
+ *exists = true;
+ }
+}
+
/**
* Delete one entry from the cache file.
*
@@ -381,9 +390,10 @@ done:
bool gencache_del(const char *keystr)
{
- bool exists, was_expired;
- bool ret = false;
- DATA_BLOB value;
+ TDB_DATA key = string_term_tdb_data(keystr);
+ bool exists = false;
+ bool result = false;
+ int ret;
if (keystr == NULL) {
return false;
@@ -395,28 +405,25 @@ bool gencache_del(const char *keystr)
DEBUG(10, ("Deleting cache entry (key=[%s])\n", keystr));
- /*
- * We delete an element by setting its timeout to 0. This way we don't
- * have to do a transaction on gencache.tdb every time we delete an
- * element.
- */
+ ret = tdb_chainlock(cache_notrans->tdb, key);
+ if (ret == -1) {
+ return false;
+ }
- exists = gencache_get_data_blob(keystr, NULL, &value, NULL,
- &was_expired);
+ gencache_parse(keystr, gencache_del_parser, &exists);
- if (!exists && was_expired) {
+ if (exists) {
/*
- * gencache_get_data_blob has implicitly deleted this
- * entry, so we have to return success here.
+ * We delete an element by setting its timeout to
+ * 0. This way we don't have to do a transaction on
+ * gencache.tdb every time we delete an element.
*/
- return true;
+ result = gencache_set(keystr, "", 0);
}
- if (exists) {
- data_blob_free(&value);
- ret = gencache_set(keystr, "", 0);
- }
- return ret;
+ tdb_chainunlock(cache_notrans->tdb, key);
+
+ return result;
}
static bool gencache_pull_timeout(char *val, time_t *pres, char **pendptr)
--
1.9.1
From 8bbf982717e0f83bd3f846a4f2673c1b02dbdad9 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 6 Mar 2016 10:27:06 +0100
Subject: [PATCH 3/8] lib: Fix a typo in gencache
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/lib/gencache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index b0242f6..e3f8609 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -807,7 +807,7 @@ static int wipe_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
* @param timeout pointer to a time_t that is filled with entry's
* timeout
*
- * @retval true when entry is successfuly fetched
+ * @retval true when entry is successfully fetched
* @retval false for failure
**/
--
1.9.1
From 227da37e57d0f5af5b7a74d5b2199714e6b92be3 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 22 Jul 2015 15:50:00 +0200
Subject: [PATCH 4/8] lib: Remove memcache from gencache
The main reason for this was to avoid access to the fcntl-governed transaction
based gencache.tdb. A later patch will make this unnecessary by filling
gencache_notrans more aggressively.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/lib/gencache.c | 36 ------------------------------------
1 file changed, 36 deletions(-)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index e3f8609..a9b7fab 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -26,7 +26,6 @@
#include "system/glob.h"
#include "util_tdb.h"
#include "tdb_wrap/tdb_wrap.h"
-#include "../lib/util/memcache.h"
#undef DBGC_CLASS
#define DBGC_CLASS DBGC_TDB
@@ -35,7 +34,6 @@
static struct tdb_wrap *cache;
static struct tdb_wrap *cache_notrans;
-static int cache_notrans_seqnum;
/**
* @file gencache.c
@@ -124,7 +122,6 @@ static bool gencache_init(void)
cache_notrans = tdb_wrap_open(NULL, cache_fname, 0,
TDB_CLEAR_IF_FIRST|
TDB_INCOMPATIBLE_HASH|
- TDB_SEQNUM|
TDB_NOSYNC|
TDB_MUTEX_LOCKING,
open_flags, 0644);
@@ -453,7 +450,6 @@ static bool gencache_pull_timeout(char *val, time_t *pres, char **pendptr)
struct gencache_parse_state {
void (*parser)(time_t timeout, DATA_BLOB blob, void *private_data);
void *private_data;
- bool is_memcache;
};
static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
@@ -476,12 +472,6 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
endptr+1, data.dsize - PTR_DIFF(endptr+1, data.dptr));
state->parser(t, blob, state->private_data);
- if (!state->is_memcache) {
- memcache_add(NULL, GENCACHE_RAM,
- data_blob_const(key.dptr, key.dsize),
- data_blob_const(data.dptr, data.dsize));
- }
-
return 0;
}
@@ -492,7 +482,6 @@ bool gencache_parse(const char *keystr,
{
struct gencache_parse_state state;
TDB_DATA key = string_term_tdb_data(keystr);
- DATA_BLOB memcache_val;
int ret;
if (keystr == NULL) {
@@ -508,31 +497,6 @@ bool gencache_parse(const char *keystr,
state.parser = parser;
state.private_data = private_data;
- if (memcache_lookup(NULL, GENCACHE_RAM,
- data_blob_const(key.dptr, key.dsize),
- &memcache_val)) {
- /*
- * Make sure that nobody has changed the gencache behind our
- * back.
- */
- int current_seqnum = tdb_get_seqnum(cache_notrans->tdb);
- if (current_seqnum == cache_notrans_seqnum) {
- /*
- * Ok, our memcache is still current, use it without
- * going to the tdb files.
- */
- state.is_memcache = true;
- gencache_parse_fn(key, make_tdb_data(memcache_val.data,
- memcache_val.length),
- &state);
- return true;
- }
- memcache_flush(NULL, GENCACHE_RAM);
- cache_notrans_seqnum = current_seqnum;
- }
-
- state.is_memcache = false;
-
ret = tdb_parse_record(cache_notrans->tdb, key,
gencache_parse_fn, &state);
if (ret == 0) {
--
1.9.1
From 72aa817f5b7c5ffb1b653e2bb4b37bb738ed220f Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 22 Jul 2015 16:00:03 +0200
Subject: [PATCH 5/8] lib: Make gencache_pull_timeout look at uint8_t
At this point we're still looking at TDB_DATA. This patch moves the casts to a
more appropriate place.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/lib/gencache.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index a9b7fab..90e6b69 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -423,7 +423,7 @@ bool gencache_del(const char *keystr)
return result;
}
-static bool gencache_pull_timeout(char *val, time_t *pres, char **pendptr)
+static bool gencache_pull_timeout(uint8_t *val, time_t *pres, char **pendptr)
{
time_t res;
char *endptr;
@@ -432,10 +432,10 @@ static bool gencache_pull_timeout(char *val, time_t *pres, char **pendptr)
return false;
}
- res = strtol(val, &endptr, 10);
+ res = strtol((char *)val, &endptr, 10);
if ((endptr == NULL) || (*endptr != '/')) {
- DEBUG(2, ("Invalid gencache data format: %s\n", val));
+ DEBUG(2, ("Invalid gencache data format: %s\n", (char *)val));
return false;
}
if (pres != NULL) {
@@ -463,7 +463,7 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
if (data.dptr == NULL) {
return -1;
}
- ret = gencache_pull_timeout((char *)data.dptr, &t, &endptr);
+ ret = gencache_pull_timeout(data.dptr, &t, &endptr);
if (!ret) {
return -1;
}
@@ -706,7 +706,7 @@ static int stabilize_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
return 0;
}
- if (!gencache_pull_timeout((char *)val.dptr, &timeout, NULL)) {
+ if (!gencache_pull_timeout(val.dptr, &timeout, NULL)) {
DEBUG(10, ("Ignoring invalid entry\n"));
return 0;
}
@@ -745,7 +745,7 @@ static int wipe_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
return 0;
}
- ok = gencache_pull_timeout((char *)val.dptr, &timeout, NULL);
+ ok = gencache_pull_timeout(val.dptr, &timeout, NULL);
if (!ok) {
DEBUG(10, ("Ignoring invalid entry\n"));
return 0;
@@ -860,7 +860,7 @@ static int gencache_iterate_blobs_fn(struct tdb_context *tdb, TDB_DATA key,
}
}
- if (!gencache_pull_timeout((char *)data.dptr, &timeout, &endptr)) {
+ if (!gencache_pull_timeout(data.dptr, &timeout, &endptr)) {
goto done;
}
endptr += 1;
--
1.9.1
From 658fede98563586d9c4edeb7111846517a674d0f Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 22 Jul 2015 16:03:47 +0200
Subject: [PATCH 6/8] lib: Simplify gencache_pull_timeout callers
gencache_pull_timeout used to point at the "/" right after the timeout. None
of the callers was interested in the "/", they are interested in the payload.
Increment the endpointer in gencache_pull_timeout and rename it.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/lib/gencache.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 90e6b69..67a4638 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -423,7 +423,7 @@ bool gencache_del(const char *keystr)
return result;
}
-static bool gencache_pull_timeout(uint8_t *val, time_t *pres, char **pendptr)
+static bool gencache_pull_timeout(uint8_t *val, time_t *pres, char **payload)
{
time_t res;
char *endptr;
@@ -441,8 +441,8 @@ static bool gencache_pull_timeout(uint8_t *val, time_t *pres, char **pendptr)
if (pres != NULL) {
*pres = res;
}
- if (pendptr != NULL) {
- *pendptr = endptr;
+ if (payload != NULL) {
+ *payload = endptr+1;
}
return true;
}
@@ -457,19 +457,19 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
struct gencache_parse_state *state;
DATA_BLOB blob;
time_t t;
- char *endptr;
+ char *payload;
bool ret;
if (data.dptr == NULL) {
return -1;
}
- ret = gencache_pull_timeout(data.dptr, &t, &endptr);
+ ret = gencache_pull_timeout(data.dptr, &t, &payload);
if (!ret) {
return -1;
}
state = (struct gencache_parse_state *)private_data;
blob = data_blob_const(
- endptr+1, data.dsize - PTR_DIFF(endptr+1, data.dptr));
+ payload, data.dsize - PTR_DIFF(payload, data.dptr));
state->parser(t, blob, state->private_data);
return 0;
@@ -840,7 +840,7 @@ static int gencache_iterate_blobs_fn(struct tdb_context *tdb, TDB_DATA key,
char *keystr;
char *free_key = NULL;
time_t timeout;
- char *endptr;
+ char *payload;
if (tdb_data_cmp(key, last_stabilize_key()) == 0) {
return 0;
@@ -860,10 +860,9 @@ static int gencache_iterate_blobs_fn(struct tdb_context *tdb, TDB_DATA key,
}
}
- if (!gencache_pull_timeout(data.dptr, &timeout, &endptr)) {
+ if (!gencache_pull_timeout(data.dptr, &timeout, &payload)) {
goto done;
}
- endptr += 1;
if (timeout == 0) {
/* delete marker */
@@ -879,8 +878,8 @@ static int gencache_iterate_blobs_fn(struct tdb_context *tdb, TDB_DATA key,
keystr, timestring(talloc_tos(), timeout)));
state->fn(keystr,
- data_blob_const(endptr,
- data.dsize - PTR_DIFF(endptr, data.dptr)),
+ data_blob_const(payload,
+ data.dsize - PTR_DIFF(payload, data.dptr)),
timeout, state->private_data);
done:
--
1.9.1
From 1db29ea5b857c9a9d34e1f8e8fb162138ecf7bb9 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 3 Mar 2016 17:41:34 +0100
Subject: [PATCH 7/8] lib: Avoid looking at fcntl'ed gencache.tdb
gencache_notrans.tdb is much cheaper to look at than gencache.tdb because it's
mutexed and thus avoids expensive fcntl locks. This patch aggressively uses the
shared _notrans tdb for both positive and negative entries. It's a replacement
for the memcache copy in every process that was removed a few patches ago.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/lib/gencache.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 67a4638..e506b3b 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -450,6 +450,7 @@ static bool gencache_pull_timeout(uint8_t *val, time_t *pres, char **payload)
struct gencache_parse_state {
void (*parser)(time_t timeout, DATA_BLOB blob, void *private_data);
void *private_data;
+ bool copy_to_notrans;
};
static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
@@ -472,6 +473,10 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
payload, data.dsize - PTR_DIFF(payload, data.dptr));
state->parser(t, blob, state->private_data);
+ if (state->copy_to_notrans) {
+ tdb_store(cache_notrans->tdb, key, data, 0);
+ }
+
return 0;
}
@@ -496,13 +501,35 @@ bool gencache_parse(const char *keystr,
state.parser = parser;
state.private_data = private_data;
+ state.copy_to_notrans = false;
+
+ ret = tdb_chainlock(cache_notrans->tdb, key);
+ if (ret != 0) {
+ return false;
+ }
ret = tdb_parse_record(cache_notrans->tdb, key,
gencache_parse_fn, &state);
if (ret == 0) {
+ tdb_chainunlock(cache_notrans->tdb, key);
return true;
}
+
+ state.copy_to_notrans = true;
+
ret = tdb_parse_record(cache->tdb, key, gencache_parse_fn, &state);
+
+ if ((ret == -1) && (tdb_error(cache->tdb) == TDB_ERR_NOEXIST)) {
+ /*
+ * The record does not exist. Set a delete-marker in
+ * gencache_notrans, so that we don't have to look at
+ * the fcntl-based cache again.
+ */
+ gencache_set(keystr, "", 0);
+ }
+
+ tdb_chainunlock(cache_notrans->tdb, key);
+
return (ret == 0);
}
--
1.9.1
From 9b072cf2652e8401016d3a3da7bc140bd8510cce Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 3 Mar 2016 17:39:09 +0100
Subject: [PATCH 8/8] lib: Avoid a gencache_parse when setting a delete marker
We know that we want to put something into _notrans, no point in
doing another round trip into gencache.tdb.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/lib/gencache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index e506b3b..84d273e 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -296,7 +296,7 @@ bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob,
return false;
}
- if (gencache_have_val(keystr, blob, timeout)) {
+ if ((timeout != 0) && gencache_have_val(keystr, blob, timeout)) {
DEBUG(10, ("Did not store value for %s, we already got it\n",
keystr));
return true;
--
1.9.1
More information about the samba-technical
mailing list