[PATCH] lib: Fix gencache_del
Volker Lendecke
Volker.Lendecke at SerNet.DE
Fri Mar 4 07:15:51 UTC 2016
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!
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 df6c9aee496654314ab56414453be22c4e33ab5d 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/6] lib: Fix gencache_del
"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.
Signed-off-by: Volker Lendecke <vl at samba.org>
---
source3/lib/gencache.c | 47 ++++++++++++++++++++++++++++-------------------
1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index c353aa6..95da5d6 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 = true;
+ int ret;
if (keystr == NULL) {
return false;
@@ -395,28 +405,27 @@ 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);
+ } else {
+ result = false;
}
- 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.7.9.5
From 4421a667b5ff4a3d4d6f1fd95b84bc7e269b070f 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 2/6] 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 95da5d6..049ea39 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);
@@ -455,7 +452,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)
@@ -478,12 +474,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;
}
@@ -494,7 +484,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) {
@@ -510,31 +499,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.7.9.5
From 59d6253ae5d59ccac218c59df31f4f5410d9d8ad 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 3/6] gencache: 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 049ea39..3bd4307 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -425,7 +425,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;
@@ -434,10 +434,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) {
@@ -465,7 +465,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;
}
@@ -708,7 +708,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;
}
@@ -747,7 +747,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;
@@ -862,7 +862,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.7.9.5
From 469917ea6461254193d941e40f316fe0e28705b0 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 4/6] 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 3bd4307..a400cc1 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -425,7 +425,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;
@@ -443,8 +443,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;
}
@@ -459,19 +459,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;
@@ -842,7 +842,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;
@@ -862,10 +862,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 (fnmatch(state->pattern, keystr, 0) != 0) {
goto done;
@@ -876,8 +875,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.7.9.5
From db6364198d840d46a6dc9895ae1a814b8b41e26f 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 5/6] gencache: 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 a400cc1..3ff83e4 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.7.9.5
From 1c1d46e79f883b9d7a8d34ec36c01617c7aee57a 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 6/6] gencache: Avoid looking at 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 3ff83e4..5a5df9b 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -452,6 +452,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)
@@ -474,6 +475,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;
}
@@ -498,13 +503,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.7.9.5
More information about the samba-technical
mailing list