Attachment 12345 Details for Bug 12005 – Patch for 4.5 cherry-picked from master

[patch] Patch for 4.5 cherry-picked from master

v45-dbwrap_ctdb-ltdb-empty-records.patch (text/plain), 8.28 KB, created by Ralph Böhme on 2016-08-09 08:00:36 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ralph Böhme

Created: 2016-08-09 08:00:36 UTC

Size: 8.28 KB

patch

obsolete

>From f0eb0d4c51147a22249e6c72afa41bb84c7618fa Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Wed, 20 Jul 2016 12:36:24 +0200
>Subject: [PATCH 1/3] smbd: ignore ctdb tombstone records in
> fetch_share_mode_unlocked_parser()
>
>dbwrap_parse_record() can return ctdb tombstone records from the lctdb,
>ignore them.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=12005
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Stefan Metzmacher <metze@samba.org>
>(cherry picked from commit 7147859c7afc1344e76485e2cbc286679110d96e)
>---
> source3/locking/share_mode_lock.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/source3/locking/share_mode_lock.c b/source3/locking/share_mode_lock.c
>index b5a63f8..f738323 100644
>--- a/source3/locking/share_mode_lock.c
>+++ b/source3/locking/share_mode_lock.c
>@@ -628,6 +628,12 @@ static void fetch_share_mode_unlocked_parser(
> 	struct share_mode_lock *lck = talloc_get_type_abort(
> 		private_data, struct share_mode_lock);
> 
>+	if (data.dsize == 0) {
>+		/* Likely a ctdb tombstone record, ignore it */
>+		lck->data = NULL;
>+		return;
>+	}
>+
> 	lck->data = parse_share_modes(lck, key, data);
> }
> 
>-- 
>2.7.4
>
>
>From 1939776359c85890eba2713f8a67110b596098f0 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Sat, 23 Jul 2016 11:08:13 +0200
>Subject: [PATCH 2/3] s4/torture: add a test for ctdb-tombstrone-record
> deadlock
>
>This tests for a possible deadlock between smbd and ctdb dealing with
>ctdb tombstone records.
>
>Commit 925625b52886d40b50fc631bad8bdc81970f7598 explains the deadlock in
>more details and contains the fix. It's a fix for a regression
>introduced by the patch for bug 10008 (1cae59ce112c).
>
>If you ever want to use this test against that specific commit:
>
>$ git checkout 925625b52886d40b50fc631bad8bdc81970f7598
>$ git cherry-pick THIS_COMMIT
>
>This should not deadlock on a ctdb cluster.
>
>$ git revert 925625b52886d40b50fc631bad8bdc81970f7598
>
>This will deadlock.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=12005
>
>Pair-Programmed-With: Michael Adam <obnox@samba.org>
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Signed-off-by: Michael Adam <obnox@samba.org>
>(cherry picked from commit b17e2f5c740fb081c007ed2e1c23138ffcba1469)
>---
> source4/torture/smb2/lock.c | 64 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
>
>diff --git a/source4/torture/smb2/lock.c b/source4/torture/smb2/lock.c
>index 68e353d..3900abf 100644
>--- a/source4/torture/smb2/lock.c
>+++ b/source4/torture/smb2/lock.c
>@@ -3033,6 +3033,69 @@ done:
> 	return ret;
> }
> 
>+/**
>+ * Test lock interaction between smbd and ctdb with tombstone records.
>+ *
>+ * Re-locking an unlocked record could lead to a deadlock between
>+ * smbd and ctdb. Make sure we don't regress.
>+ *
>+ * https://bugzilla.samba.org/show_bug.cgi?id=12005
>+ * https://bugzilla.samba.org/show_bug.cgi?id=10008
>+ */
>+static bool test_deadlock(struct torture_context *torture,
>+			  struct smb2_tree *tree)
>+{
>+	NTSTATUS status;
>+	bool ret = true;
>+	struct smb2_handle _h;
>+	struct smb2_handle *h = NULL;
>+	uint8_t buf[200];
>+	const char *fname = BASEDIR "\\deadlock.txt";
>+
>+	if (!lpcfg_clustering(torture->lp_ctx)) {
>+		torture_skip(torture, "Test must be run on a ctdb cluster\n");
>+		return true;
>+	}
>+
>+	status = torture_smb2_testdir(tree, BASEDIR, &_h);
>+	torture_assert_ntstatus_ok(torture, status,
>+				   "torture_smb2_testdir failed");
>+	smb2_util_close(tree, _h);
>+
>+	status = torture_smb2_testfile(tree, fname, &_h);
>+	torture_assert_ntstatus_ok_goto(torture, status, ret, done,
>+					"torture_smb2_testfile failed");
>+	h = &_h;
>+
>+	ZERO_STRUCT(buf);
>+	status = smb2_util_write(tree, *h, buf, 0, ARRAY_SIZE(buf));
>+	torture_assert_ntstatus_ok_goto(torture, status, ret, done,
>+					"smb2_util_write failed");
>+
>+	status = test_smb2_lock(tree, *h, 0, 1, true);
>+	torture_assert_ntstatus_ok_goto(torture, status, ret, done,
>+					"test_smb2_lock failed");
>+
>+	status = test_smb2_unlock(tree, *h, 0, 1);
>+	torture_assert_ntstatus_ok_goto(torture, status, ret, done,
>+					"test_smb2_unlock failed");
>+
>+	status = test_smb2_lock(tree, *h, 0, 1, true);
>+	torture_assert_ntstatus_ok_goto(torture, status, ret, done,
>+					"test_smb2_lock failed");
>+
>+	status = test_smb2_unlock(tree, *h, 0, 1);
>+	torture_assert_ntstatus_ok_goto(torture, status, ret, done,
>+					"test_smb2_unlock failed");
>+
>+done:
>+	if (h != NULL) {
>+		smb2_util_close(tree, *h);
>+	}
>+	smb2_deltree(tree, BASEDIR);
>+	return ret;
>+}
>+
> /* basic testing of SMB2 locking
> */
> struct torture_suite *torture_smb2_lock_init(void)
>@@ -3068,6 +3131,7 @@ struct torture_suite *torture_smb2_lock_init(void)
> 	torture_suite_add_2smb2_test(suite, "overlap", test_overlap);
> 	torture_suite_add_1smb2_test(suite, "truncate", test_truncate);
> 	torture_suite_add_1smb2_test(suite, "replay", test_replay);
>+	torture_suite_add_1smb2_test(suite, "ctdb-delrec-deadlock", test_deadlock);
> 
> 	suite->description = talloc_strdup(suite, "SMB2-LOCK tests");
> 
>-- 
>2.7.4
>
>
>From f946992ee67c903e0ab166dc9cec36da54a9fe0d Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Mon, 8 Aug 2016 16:58:51 +0200
>Subject: [PATCH 3/3] dbwrap_ctdb: treat empty records in ltdb as non-existing
>
>When fetching records from remote ctdb nodes via ctdbd_parse() or in
>db_ctdb_traverse(), we already check for tombstone records and skip
>them. This was originally also done for the ltdb checks.
>
>See also bug: https://bugzilla.samba.org/show_bug.cgi?id=10008
>(commit 1cae59ce112ccb51b45357a52b902f80fce1eef1).
>
>Commit 925625b52886d40b50fc631bad8bdc81970f7598 reverted part of the
>patch of bug 10008 due to a deadlock it introduced.
>
>This patch re-introduces the consistent treatment of empty records in
>the ltdb but avoids the deadlock by correctly signalling
>NT_STATUS_NOT_FOUND if an empty record is found authoritatively in
>the ltdb and not calling ctdb in this case.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=12005
>
>Pair-Programmed-With: Michael Adam <obnox@samba.org>
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Signed-off-by: Michael Adam <obnox@samba.org>
>
>Autobuild-User(master): Michael Adam <obnox@samba.org>
>Autobuild-Date(master): Tue Aug  9 04:38:44 CEST 2016 on sn-devel-144
>
>(cherry picked from commit 25df582739918b7afd4e5497eaffe279e2d92cd1)
>---
> source3/lib/dbwrap/dbwrap_ctdb.c | 27 ++++++++++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
>diff --git a/source3/lib/dbwrap/dbwrap_ctdb.c b/source3/lib/dbwrap/dbwrap_ctdb.c
>index d86d078..10f5f50 100644
>--- a/source3/lib/dbwrap/dbwrap_ctdb.c
>+++ b/source3/lib/dbwrap/dbwrap_ctdb.c
>@@ -1221,6 +1221,7 @@ struct db_ctdb_parse_record_state {
> 	uint32_t my_vnn;
> 	bool ask_for_readonly_copy;
> 	bool done;
>+	bool empty_record;
> };
> 
> static void db_ctdb_parse_record_parser(
>@@ -1240,7 +1241,16 @@ static void db_ctdb_parse_record_parser_nonpersistent(
> 		(struct db_ctdb_parse_record_state *)private_data;
> 
> 	if (db_ctdb_can_use_local_hdr(header, state->my_vnn, true)) {
>-		state->parser(key, data, state->private_data);
>+		/*
>+		 * A record consisting only of the ctdb header can be
>+		 * a validly created empty record or a tombstone
>+		 * record of a deleted record (not vacuumed yet). Mark
>+		 * it accordingly.
>+		 */
>+		state->empty_record = (data.dsize == 0);
>+		if (!state->empty_record) {
>+			state->parser(key, data, state->private_data);
>+		}
> 		state->done = true;
> 	} else {
> 		/*
>@@ -1267,6 +1277,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;
>@@ -1298,6 +1309,20 @@ static NTSTATUS db_ctdb_parse_record(struct db_context *db, TDB_DATA key,
> 	status = db_ctdb_ltdb_parse(
> 		ctx, key, db_ctdb_parse_record_parser_nonpersistent, &state);
> 	if (NT_STATUS_IS_OK(status) && state.done) {
>+		if (state.empty_record) {
>+			/*
>+			 * We know authoritatively, that this is an empty
>+			 * record. Since ctdb does not distinguish between empty
>+			 * and deleted records, this can be a record stored as
>+			 * empty or a not-yet-vacuumed tombstone record of a
>+			 * deleted record. Now Samba right now can live without
>+			 * empty records, so we can safely report this record
>+			 * as non-existing.
>+			 *
>+			 * See bugs 10008 and 12005.
>+			 */
>+			return NT_STATUS_NOT_FOUND;
>+		}
> 		return NT_STATUS_OK;
> 	}
> 
>-- 
>2.7.4
>

Flags:

obnox: review+

Actions: View

Attachments on bug 12005: 12253 | 12254 | 12255 | 12345 | 12346