[PATCHES] Convert gencache to dbwrap to enable mutexes
Christof Schmitt
cs at samba.org
Fri Jun 13 15:41:23 MDT 2014
On Fri, Jun 06, 2014 at 04:27:00PM -0700, Christof Schmitt wrote:
> On Sat, Jun 07, 2014 at 12:47:15AM +0200, Michael Adam wrote:
> > On 2014-06-06 at 12:40 -0700, Christof Schmitt wrote:
> > > On Fri, Jun 06, 2014 at 09:23:35PM +0200, Michael Adam wrote:
> > > > sn-devel-104 currently does not support robust mutexes, sorry...
> > > >
> > > > The sn-devel-124 based on ubuntu 12.04 that metze is preparing
> > > > will support them. But having one test host which does support
> > > > them is a good test as this case proves.
> > > >
> > > > Wouldn't the proper fix be to convert the db to us dbwrap
> > > > instead of tdb_open_log ?
> > >
> > > I can take a look at that, but doesn't the current dbwrap code try to
> > > open the database in clustering mode first? Do we need an extra flag for
> > > dbwrap_open to force a db to be local?
> >
> > You can either use "ctdb:gencache_notrans.tdb = no" in
> > smb.conf and use db_open(), (see source3/lib/dbwrap/dbwrap_open.c)
> > or directly use dbwrap_local_open() (or db_open_tdb()).
> >
> > Using db_open, you can also control mutexes via
> > "dbwrap_tdb_mutexes:gencache_notrans.tdb = yes/no".
>
> The check for mutex usage is in db_open and that function automatically
> uses ctdb (unless disabled through the config). I don't think that it
> makes sense to have the gencache_notrans tdb managed by ctdb, so
> disabling the cluster usage should be the default for this db. I see two
> options here:
> 1) Add a flag to db_open to specify that this db should not be clustered.
> 2) Also add the mutex check to dbwrap_local_open, maybe in helper function.
>
> > Right, at the tdb level this is deliberately so.
> > If you pass TDB_MUTEX_LOCKING to tdb_open, it will fail
> > the open if the runtime check fails. I.e. if the caller
> > wants mutexes and tdb_open succees, the callser should
> > have mutexes. The one dynamic caller-level check we have
> > is in db_open() and I think it should stay the only one.
>
> Does this mean that you would prefer all tdbs to be used through
> db_open? I agree with having only one check, i am just trying to find
> out where that should be.
The attached patches convert gencache to use dbwrap and also introduce a
flag to only use local databases in db_open. I have not really tested
them, so there might still be bugs. Any comments if that is a good
approach?
Christof
-------------- next part --------------
>From d426f866319bb7a6fcdcb9a20c77becb074e9ebe Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Mon, 9 Jun 2014 12:21:06 -0700
Subject: [PATCH 1/3] gencache: Always free data from stabilize record in gencache_set_data_blob
This is unlikely, but better be safe than to leak memory.
---
source3/lib/gencache.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 0fb1fd8..2efbdf6 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -327,9 +327,10 @@ bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob,
last_stabilize = 0;
databuf = tdb_fetch_compat(cache_notrans, last_stabilize_key());
- if ((databuf.dptr != NULL)
- && (databuf.dptr[databuf.dsize-1] == '\0')) {
- last_stabilize = atoi((char *)databuf.dptr);
+ if (databuf.dptr != NULL) {
+ if (databuf.dptr[databuf.dsize-1] == '\0') {
+ last_stabilize = atoi((char *)databuf.dptr);
+ }
SAFE_FREE(databuf.dptr);
}
if ((last_stabilize
--
1.7.1
>From 6bb4dbe2d73ae4c2c9ad83f164fc52b1dd26c937 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 13 Jun 2014 12:23:14 -0700
Subject: [PATCH 2/3] dbwrap: Add flag to force local database in db_open
---
lib/dbwrap/dbwrap.h | 1 +
source3/lib/dbwrap/dbwrap_open.c | 2 +-
2 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/lib/dbwrap/dbwrap.h b/lib/dbwrap/dbwrap.h
index d289243..00da8a7 100644
--- a/lib/dbwrap/dbwrap.h
+++ b/lib/dbwrap/dbwrap.h
@@ -38,6 +38,7 @@ enum dbwrap_lock_order {
#define DBWRAP_FLAG_NONE 0x0000000000000000ULL
#define DBWRAP_FLAG_OPTIMIZE_READONLY_ACCESS 0x0000000000000001ULL
+#define DBWRAP_FLAG_LOCAL_ONLY 0x0000000000000002ULL
/* The following definitions come from lib/dbwrap.c */
diff --git a/source3/lib/dbwrap/dbwrap_open.c b/source3/lib/dbwrap/dbwrap_open.c
index 64f484e..5913b37 100644
--- a/source3/lib/dbwrap/dbwrap_open.c
+++ b/source3/lib/dbwrap/dbwrap_open.c
@@ -114,7 +114,7 @@ struct db_context *db_open(TALLOC_CTX *mem_ctx,
sockname = lp_ctdbd_socket();
- if (lp_clustering()) {
+ if (lp_clustering() && !(dbwrap_flags & DBWRAP_FLAG_LOCAL_ONLY)) {
const char *partname;
if (!socket_exist(sockname)) {
--
1.7.1
>From ca0f6abf676104d93aa85f53bc5a6a10d0c756ad Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 6 Jun 2014 13:48:08 -0700
Subject: [PATCH 3/3] gencache: Convert to dbwrap interface
Use db_open to automatically enable mutexes when present. Specify
DBWRAP_FLAG_LOCAL_ONLY to keep the database per-node, even in a
clustered environment.
---
source3/lib/gencache.c | 197 ++++++++++++++++++++++++++----------------------
1 files changed, 107 insertions(+), 90 deletions(-)
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 2efbdf6..fafbd7a 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -25,6 +25,8 @@
#include "system/filesys.h"
#include "system/glob.h"
#include "util_tdb.h"
+#include "dbwrap/dbwrap.h"
+#include "dbwrap/dbwrap_open.h"
#include "memcache.h"
#undef DBGC_CLASS
@@ -36,8 +38,8 @@
#define BLOB_TYPE "DATA_BLOB"
#define BLOB_TYPE_LEN 9
-static struct tdb_context *cache;
-static struct tdb_context *cache_notrans;
+static struct db_context *cache;
+static struct db_context *cache_notrans;
static int cache_notrans_seqnum;
/**
@@ -68,12 +70,15 @@ static bool gencache_init(void)
DEBUG(5, ("Opening cache file at %s\n", cache_fname));
- cache = tdb_open_log(cache_fname, 0, TDB_DEFAULT|TDB_INCOMPATIBLE_HASH, open_flags, 0644);
+ cache = db_open(NULL, cache_fname, 0,
+ TDB_DEFAULT|TDB_INCOMPATIBLE_HASH, open_flags,
+ 0644, DBWRAP_LOCK_ORDER_NONE,
+ DBWRAP_FLAG_LOCAL_ONLY);
if (cache) {
int ret;
- ret = tdb_check(cache, NULL, NULL);
+ ret = dbwrap_check(cache);
if (ret != 0) {
- tdb_close(cache);
+ TALLOC_FREE(cache);
/*
* Retry with CLEAR_IF_FIRST.
@@ -85,18 +90,23 @@ static bool gencache_init(void)
* CLEAR_IF_FIRST databases, so lets use it here to
* clean up a broken database.
*/
- cache = tdb_open_log(cache_fname, 0,
- TDB_DEFAULT|
- TDB_INCOMPATIBLE_HASH|
- TDB_CLEAR_IF_FIRST,
- open_flags, 0644);
+ cache = db_open(NULL, cache_fname, 0,
+ TDB_DEFAULT|
+ TDB_INCOMPATIBLE_HASH|
+ TDB_CLEAR_IF_FIRST,
+ open_flags, 0644,
+ DBWRAP_LOCK_ORDER_NONE,
+ DBWRAP_FLAG_LOCAL_ONLY);
}
}
if (!cache && (errno == EACCES)) {
open_flags = O_RDONLY;
- cache = tdb_open_log(cache_fname, 0, TDB_DEFAULT|TDB_INCOMPATIBLE_HASH, open_flags,
- 0644);
+ cache = db_open(NULL, cache_fname, 0,
+ TDB_DEFAULT|TDB_INCOMPATIBLE_HASH,
+ open_flags, 0644,
+ DBWRAP_LOCK_ORDER_NONE,
+ DBWRAP_FLAG_LOCAL_ONLY);
if (cache) {
DEBUG(5, ("gencache_init: Opening cache file %s read-only.\n", cache_fname));
}
@@ -111,17 +121,18 @@ static bool gencache_init(void)
DEBUG(5, ("Opening cache file at %s\n", cache_fname));
- cache_notrans = tdb_open_log(cache_fname, 0,
- TDB_CLEAR_IF_FIRST|
- TDB_INCOMPATIBLE_HASH|
- TDB_SEQNUM|
- TDB_NOSYNC,
- open_flags, 0644);
+ cache_notrans = db_open(NULL, cache_fname, 0,
+ TDB_CLEAR_IF_FIRST|
+ TDB_INCOMPATIBLE_HASH|
+ TDB_SEQNUM|
+ TDB_NOSYNC,
+ open_flags, 0644,
+ DBWRAP_LOCK_ORDER_NONE,
+ DBWRAP_FLAG_LOCAL_ONLY);
if (cache_notrans == NULL) {
DEBUG(5, ("Opening %s failed: %s\n", cache_fname,
strerror(errno)));
- tdb_close(cache);
- cache = NULL;
+ TALLOC_FREE(cache);
return false;
}
@@ -255,7 +266,7 @@ static bool gencache_have_val(const char *keystr, const DATA_BLOB *data,
bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob,
time_t timeout)
{
- int ret;
+ NTSTATUS status;
TDB_DATA databuf;
char* val;
time_t last_stabilize;
@@ -298,13 +309,13 @@ bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob,
(int)(timeout - time(NULL)),
timeout > time(NULL) ? "ahead" : "in the past"));
- ret = tdb_store_bystring(
- cache_notrans, keystr,
- make_tdb_data((uint8_t *)val, talloc_array_length(val)),
- 0);
+ status = dbwrap_store_bystring(cache_notrans, keystr,
+ make_tdb_data((uint8_t *)val,
+ talloc_array_length(val)),
+ 0);
TALLOC_FREE(val);
- if (ret != 0) {
+ if (!NT_STATUS_IS_OK(status)) {
return false;
}
@@ -326,12 +337,13 @@ bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob,
*/
last_stabilize = 0;
- databuf = tdb_fetch_compat(cache_notrans, last_stabilize_key());
- if (databuf.dptr != NULL) {
+ status = dbwrap_fetch(cache_notrans, talloc_tos(),
+ last_stabilize_key(), &databuf);
+ if (NT_STATUS_IS_OK(status)) {
if (databuf.dptr[databuf.dsize-1] == '\0') {
last_stabilize = atoi((char *)databuf.dptr);
}
- SAFE_FREE(databuf.dptr);
+ TALLOC_FREE(databuf.dptr);
}
if ((last_stabilize
+ lp_parm_int(-1, "gencache", "stabilize_interval", 300))
@@ -420,7 +432,7 @@ struct gencache_parse_state {
bool is_memcache;
};
-static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
+static void gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
{
struct gencache_parse_state *state;
DATA_BLOB blob;
@@ -429,11 +441,11 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
bool ret;
if (data.dptr == NULL) {
- return -1;
+ return;
}
ret = gencache_pull_timeout((char *)data.dptr, &t, &endptr);
if (!ret) {
- return -1;
+ return;
}
state = (struct gencache_parse_state *)private_data;
blob = data_blob_const(
@@ -445,8 +457,6 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
data_blob_const(key.dptr, key.dsize),
data_blob_const(data.dptr, data.dsize));
}
-
- return 0;
}
bool gencache_parse(const char *keystr,
@@ -457,7 +467,7 @@ 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;
+ NTSTATUS status;
if (keystr == NULL) {
return false;
@@ -479,7 +489,7 @@ bool gencache_parse(const char *keystr,
* Make sure that nobody has changed the gencache behind our
* back.
*/
- int current_seqnum = tdb_get_seqnum(cache_notrans);
+ int current_seqnum = dbwrap_get_seqnum(cache_notrans);
if (current_seqnum == cache_notrans_seqnum) {
/*
* Ok, our memcache is still current, use it without
@@ -497,12 +507,17 @@ bool gencache_parse(const char *keystr,
state.is_memcache = false;
- ret = tdb_parse_record(cache_notrans, key, gencache_parse_fn, &state);
- if (ret == 0) {
+ status = dbwrap_parse_record(cache_notrans, key, gencache_parse_fn,
+ &state);
+ if (NT_STATUS_IS_OK(status)) {
return true;
}
- ret = tdb_parse_record(cache, key, gencache_parse_fn, &state);
- return (ret == 0);
+ status = dbwrap_parse_record(cache, key, gencache_parse_fn, &state);
+ if (NT_STATUS_IS_OK(status)) {
+ return true;
+ }
+
+ return false;
}
struct gencache_get_data_blob_state {
@@ -598,8 +613,7 @@ struct stabilize_state {
bool written;
bool error;
};
-static int stabilize_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
- void *priv);
+static int stabilize_fn(struct db_record *rec, void *priv);
/**
* Stabilize gencache
@@ -611,6 +625,7 @@ static int stabilize_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
bool gencache_stabilize(void)
{
struct stabilize_state state;
+ NTSTATUS status;
int res;
char *now;
@@ -618,77 +633,77 @@ bool gencache_stabilize(void)
return false;
}
- res = tdb_transaction_start_nonblock(cache);
- if (res != 0) {
- if (tdb_error(cache) == TDB_ERR_NOLOCK)
- {
- /*
- * Someone else already does the stabilize,
- * this does not have to be done twice
- */
- return true;
- }
+ status = dbwrap_transaction_start_nonblock(cache);
+ if (NT_STATUS_EQUAL(status, NT_STATUS_FILE_LOCK_CONFLICT)) {
+ /*
+ * Someone else already does the stabilize,
+ * this does not have to be done twice
+ */
+ return true;
+ }
+ if (!NT_STATUS_IS_OK(status)) {
DEBUG(10, ("Could not start transaction on gencache.tdb: "
- "%s\n", tdb_errorstr_compat(cache)));
+ "%s\n", nt_errstr(status)));
return false;
}
- res = tdb_transaction_start(cache_notrans);
- if (res != 0) {
- tdb_transaction_cancel(cache);
+ res = dbwrap_transaction_start(cache_notrans);
+ if (!NT_STATUS_IS_OK(status)) {
+ dbwrap_transaction_cancel(cache);
DEBUG(10, ("Could not start transaction on "
- "gencache_notrans.tdb: %s\n",
- tdb_errorstr_compat(cache_notrans)));
+ "gencache_notrans.tdb\n"));
return false;
}
state.error = false;
state.written = false;
- res = tdb_traverse(cache_notrans, stabilize_fn, &state);
- if ((res < 0) || state.error) {
- tdb_transaction_cancel(cache_notrans);
- tdb_transaction_cancel(cache);
+ status = dbwrap_traverse(cache_notrans, stabilize_fn, &state, 0);
+
+ if (!NT_STATUS_IS_OK(status) || state.error) {
+ dbwrap_transaction_cancel(cache_notrans);
+ dbwrap_transaction_cancel(cache);
return false;
}
if (!state.written) {
- tdb_transaction_cancel(cache_notrans);
- tdb_transaction_cancel(cache);
+ dbwrap_transaction_cancel(cache_notrans);
+ dbwrap_transaction_cancel(cache);
return true;
}
- res = tdb_transaction_commit(cache);
+ res = dbwrap_transaction_commit(cache);
if (res != 0) {
- DEBUG(10, ("tdb_transaction_commit on gencache.tdb failed: "
- "%s\n", tdb_errorstr_compat(cache)));
- tdb_transaction_cancel(cache_notrans);
+ DEBUG(10, ("dbwrap_transaction_commit on gencache.tdb "
+ "failed\n"));
+ dbwrap_transaction_cancel(cache_notrans);
return false;
}
- res = tdb_transaction_commit(cache_notrans);
+ res = dbwrap_transaction_commit(cache_notrans);
if (res != 0) {
- DEBUG(10, ("tdb_transaction_commit on gencache.tdb failed: "
- "%s\n", tdb_errorstr_compat(cache)));
+ DEBUG(10, ("dbwrap_transaction_commit on gencache_notrans.tdb "
+ "failed\n"));
return false;
}
now = talloc_asprintf(talloc_tos(), "%d", (int)time(NULL));
if (now != NULL) {
- tdb_store(cache_notrans, last_stabilize_key(),
- string_term_tdb_data(now), 0);
+ dbwrap_store(cache_notrans, last_stabilize_key(),
+ string_term_tdb_data(now), 0);
TALLOC_FREE(now);
}
return true;
}
-static int stabilize_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
- void *priv)
+static int stabilize_fn(struct db_record *rec, void *priv)
{
struct stabilize_state *state = (struct stabilize_state *)priv;
- int res;
+ NTSTATUS status;
time_t timeout;
+ TDB_DATA key = dbwrap_record_get_key(rec);
+ TDB_DATA val = dbwrap_record_get_value(rec);
if (tdb_data_cmp(key, last_stabilize_key()) == 0) {
return 0;
@@ -699,29 +714,30 @@ static int stabilize_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
return 0;
}
if ((timeout < time(NULL)) || (val.dsize == 0)) {
- res = tdb_delete(cache, key);
- if ((res != 0) && (tdb_error(cache) == TDB_ERR_NOEXIST)) {
- res = 0;
+ status = dbwrap_delete(cache, key);
+ if (NT_STATUS_EQUAL(status, NT_STATUS_NOT_FOUND)) {
+ status = NT_STATUS_OK;
} else {
state->written = true;
}
} else {
- res = tdb_store(cache, key, val, 0);
- if (res == 0) {
+ status = dbwrap_store(cache, key, val, 0);
+ if (NT_STATUS_IS_OK(status)) {
state->written = true;
}
}
- if (res != 0) {
+ if (!NT_STATUS_IS_OK(status)) {
DEBUG(10, ("Transfer to gencache.tdb failed: %s\n",
- tdb_errorstr_compat(cache)));
+ nt_errstr(status)));
state->error = true;
return -1;
}
- if (tdb_delete(cache_notrans, key) != 0) {
+ status = dbwrap_delete(cache_notrans, key);
+ if (!NT_STATUS_IS_OK(status)) {
DEBUG(10, ("tdb_delete from gencache_notrans.tdb failed: "
- "%s\n", tdb_errorstr_compat(cache_notrans)));
+ "%s\n", nt_errstr(status)));
state->error = true;
return -1;
}
@@ -798,8 +814,7 @@ struct gencache_iterate_blobs_state {
bool in_persistent;
};
-static int gencache_iterate_blobs_fn(struct tdb_context *tdb, TDB_DATA key,
- TDB_DATA data, void *priv)
+static int gencache_iterate_blobs_fn(struct db_record *rec, void *priv)
{
struct gencache_iterate_blobs_state *state =
(struct gencache_iterate_blobs_state *)priv;
@@ -807,11 +822,13 @@ static int gencache_iterate_blobs_fn(struct tdb_context *tdb, TDB_DATA key,
char *free_key = NULL;
time_t timeout;
char *endptr;
+ TDB_DATA key = dbwrap_record_get_key(rec);
+ TDB_DATA data = dbwrap_record_get_value(rec);
if (tdb_data_cmp(key, last_stabilize_key()) == 0) {
return 0;
}
- if (state->in_persistent && tdb_exists(cache_notrans, key)) {
+ if (state->in_persistent && dbwrap_exists(cache_notrans, key)) {
return 0;
}
@@ -866,10 +883,10 @@ void gencache_iterate_blobs(void (*fn)(const char *key, DATA_BLOB value,
state.private_data = private_data;
state.in_persistent = false;
- tdb_traverse(cache_notrans, gencache_iterate_blobs_fn, &state);
+ dbwrap_traverse(cache_notrans, gencache_iterate_blobs_fn, &state, NULL);
state.in_persistent = true;
- tdb_traverse(cache, gencache_iterate_blobs_fn, &state);
+ dbwrap_traverse(cache, gencache_iterate_blobs_fn, &state, NULL);
}
/**
--
1.7.1
More information about the samba-technical
mailing list