RFC: dbwrap_ctdb and empty vs deleted records
Ralph Boehme
slow at samba.org
Mon Jul 4 11:43:00 UTC 2016
Hi!
Attached is a RFC patch for bug
<https://bugzilla.samba.org/show_bug.cgi?id=12005>
From the bugreport:
---8<---
On a Samba 4.4.4 ctdb cluster the following messages where
(repeatedly) found in the log:
[2016/06/30 08:45:02.645180, 1]
../librpc/ndr/ndr.c:578(ndr_pull_error)
ndr_pull_error(11): Pull bytes 8 (../librpc/ndr/ndr_basic.c:236)
[2016/06/30 08:45:02.645213, 1]
../source3/locking/share_mode_lock.c:312(parse_share_modes)
ndr_pull_share_mode_lock failed: Buffer Size Error
Looking closer this is coming from ctdb tombstone records in the node
local ltdb.
...
I see two ways to address this:
o add yet another check for tombstone records to a dbwrap caller, or
o fix dbwrap
In dbwrap we already check for tombstone records in at least two
places:
o when fetching records from remote ctdb nodes via ctdbd_parse()
o in db_ctdb_traverse()
Just db_ctdb_ltdb_parser() lacks a check for tombstone records.
---8<---
This had been fixed in 1cae59ce112ccb51b45357a52b902f80fce1eef1 for
bug https://bugzilla.samba.org/show_bug.cgi?id=10008, but had to be
reverted in because of a deadlock (commit message has the details).
I *think* my patch might be a proper fix without the risk of a
deadlock, because it *won't* call out to ctdb but return ENOENT (im
terms of NTSTATUS).
I'd highly appreciate some feedback. In case we don't want to take the
risk of this change, I'll prepare a patch for parse_share_modes() and
callers.
Cheerio!
-slow
-------------- next part --------------
From d6e7c74929c29f6b66e1a26312f8bbacb241172a Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 1 Jul 2016 17:45:53 +0200
Subject: [PATCH 1/2] dbwrap_ctdb: move struct db_ctdb_parse_record_state
Will be needed in the next commit. No change in behavour.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=12005
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/lib/dbwrap/dbwrap_ctdb.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/source3/lib/dbwrap/dbwrap_ctdb.c b/source3/lib/dbwrap/dbwrap_ctdb.c
index df5a34f..c557902 100644
--- a/source3/lib/dbwrap/dbwrap_ctdb.c
+++ b/source3/lib/dbwrap/dbwrap_ctdb.c
@@ -99,6 +99,14 @@ static int db_ctdb_ltdb_parser(TDB_DATA key, TDB_DATA data,
return 0;
}
+struct db_ctdb_parse_record_state {
+ void (*parser)(TDB_DATA key, TDB_DATA data, void *private_data);
+ void *private_data;
+ uint32_t my_vnn;
+ bool ask_for_readonly_copy;
+ bool done;
+};
+
static NTSTATUS db_ctdb_ltdb_parse(
struct db_ctdb_ctx *db, TDB_DATA key,
void (*parser)(TDB_DATA key, struct ctdb_ltdb_header *header,
@@ -1215,14 +1223,6 @@ static struct db_record *db_ctdb_try_fetch_locked(struct db_context *db,
return fetch_locked_internal(ctx, mem_ctx, key, true);
}
-struct db_ctdb_parse_record_state {
- void (*parser)(TDB_DATA key, TDB_DATA data, void *private_data);
- void *private_data;
- uint32_t my_vnn;
- bool ask_for_readonly_copy;
- bool done;
-};
-
static void db_ctdb_parse_record_parser(
TDB_DATA key, struct ctdb_ltdb_header *header,
TDB_DATA data, void *private_data)
--
2.5.0
From 0e23ae553d1745f00d1cbd066eae591120cead9f Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 1 Jul 2016 17:21:04 +0200
Subject: [PATCH 2/2] dbwrap_ctdb: check for ltdb tombstone records
When fetching records from remote ctdb nodes via ctdbd_parse() or in
db_ctdb_traverse(), we already checks for tombstone records and skip
them. Add the same check to db_ctdb_ltdb_parser() when fetching records
from the ltbd.
See also bug: https://bugzilla.samba.org/show_bug.cgi?id=10008
Commit 925625b52886d40b50fc631bad8bdc81970f7598 reverted part of the
patch in bug 10008 due to a deadlock it introduced.
This patch avoids the deadlock because it ensures db_ctdb_parse_record()
returns NT_STATUS_NOT_FOUND if an empty record is found in the ltdb and
not calling ctdb.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=12005
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/lib/dbwrap/dbwrap_ctdb.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/source3/lib/dbwrap/dbwrap_ctdb.c b/source3/lib/dbwrap/dbwrap_ctdb.c
index c557902..26defa4 100644
--- a/source3/lib/dbwrap/dbwrap_ctdb.c
+++ b/source3/lib/dbwrap/dbwrap_ctdb.c
@@ -79,6 +79,7 @@ struct db_ctdb_ltdb_parse_state {
void (*parser)(TDB_DATA key, struct ctdb_ltdb_header *header,
TDB_DATA data, void *private_data);
void *private_data;
+ bool empty_record;
};
static int db_ctdb_ltdb_parser(TDB_DATA key, TDB_DATA data,
@@ -91,6 +92,15 @@ static int db_ctdb_ltdb_parser(TDB_DATA key, TDB_DATA data,
return -1;
}
+ if (data.dsize == sizeof(struct ctdb_ltdb_header)) {
+ /*
+ * Possible record tombstone or just a fresh and empty
+ * record
+ */
+ state->empty_record = true;
+ return -1;
+ }
+
state->parser(
key, (struct ctdb_ltdb_header *)data.dptr,
make_tdb_data(data.dptr + sizeof(struct ctdb_ltdb_header),
@@ -105,6 +115,7 @@ struct db_ctdb_parse_record_state {
uint32_t my_vnn;
bool ask_for_readonly_copy;
bool done;
+ bool empty_record;
};
static NTSTATUS db_ctdb_ltdb_parse(
@@ -113,6 +124,8 @@ static NTSTATUS db_ctdb_ltdb_parse(
TDB_DATA data, void *private_data),
void *private_data)
{
+ struct db_ctdb_parse_record_state *rec_state =
+ (struct db_ctdb_parse_record_state *)private_data;
struct db_ctdb_ltdb_parse_state state;
int ret;
@@ -122,6 +135,9 @@ static NTSTATUS db_ctdb_ltdb_parse(
ret = tdb_parse_record(db->wtdb->tdb, key, db_ctdb_ltdb_parser,
&state);
if (ret == -1) {
+ if (state.empty_record) {
+ rec_state->empty_record = true;
+ }
return NT_STATUS_NOT_FOUND;
}
return NT_STATUS_OK;
@@ -1267,6 +1283,7 @@ static NTSTATUS db_ctdb_parse_record(struct db_context *db, TDB_DATA key,
state.parser = parser;
state.private_data = private_data;
state.my_vnn = ctdbd_vnn(ctx->conn);
+ state.empty_record = false;
if (ctx->transaction != NULL) {
struct db_ctdb_transaction_handle *h = ctx->transaction;
@@ -1301,6 +1318,18 @@ static NTSTATUS db_ctdb_parse_record(struct db_context *db, TDB_DATA key,
return NT_STATUS_OK;
}
+ if (NT_STATUS_EQUAL(status, NT_STATUS_NOT_FOUND) && state.empty_record) {
+ /*
+ * This is an empty record. ctdbd does not distinguish
+ * between empty and deleted records. Samba right now
+ * can live without empty records, so lets treat
+ * zero-size (i.e. deleted) records as non-existing.
+ *
+ * See bugs 10008 and 12005.
+ */
+ return NT_STATUS_NOT_FOUND;
+ }
+
ret = ctdbd_parse(ctx->conn, ctx->db_id, key,
state.ask_for_readonly_copy, parser, private_data);
if (ret != 0) {
--
2.5.0
More information about the samba-technical
mailing list