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