[PATCH] A few tdb cleanups and a bit more

Volker Lendecke Volker.Lendecke at SerNet.DE
Tue Oct 23 07:45:22 UTC 2018


Hi!

Review appreciated!

https://gitlab.com/samba-team/devel/samba/pipelines/33948435

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 cf380a60fd3f38a18f4ab38453179d0fff5068b3 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 22 Oct 2018 07:42:14 +0200
Subject: [PATCH 01/10] tdb: Avoid casts

We have %PRIu32 and %zu these days

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tdb/common/open.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/tdb/common/open.c b/lib/tdb/common/open.c
index 8baa7e40de0..44bb3a0009f 100644
--- a/lib/tdb/common/open.c
+++ b/lib/tdb/common/open.c
@@ -281,10 +281,10 @@ check_local_settings:
 
 	if (tdb_mutex_size(tdb) != header->mutex_size) {
 		TDB_LOG((tdb, TDB_DEBUG_ERROR, "tdb_mutex_open_ok[%s]: "
-			 "Mutex size changed from %u to %u\n.",
+			 "Mutex size changed from %"PRIu32" to %zu\n.",
 			 tdb->name,
-			 (unsigned int)header->mutex_size,
-			 (unsigned int)tdb_mutex_size(tdb)));
+			 header->mutex_size,
+			 tdb_mutex_size(tdb)));
 		return false;
 	}
 
-- 
2.11.0


From efec9ee9dab7483cf52feebb9641c76f5056e988 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 22 Oct 2018 07:08:58 +0200
Subject: [PATCH 02/10] tdb: Use explicit initialization

Let the compiler figure out the optimal code

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tdb/common/open.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/tdb/common/open.c b/lib/tdb/common/open.c
index 44bb3a0009f..899a2fcaffb 100644
--- a/lib/tdb/common/open.c
+++ b/lib/tdb/common/open.c
@@ -297,7 +297,7 @@ _PUBLIC_ struct tdb_context *tdb_open_ex(const char *name, int hash_size, int td
 				tdb_hash_func hash_fn)
 {
 	int orig_errno = errno;
-	struct tdb_header header;
+	struct tdb_header header = {{0}};
 	struct tdb_context *tdb;
 	struct stat st;
 	int rev = 0;
@@ -309,8 +309,6 @@ _PUBLIC_ struct tdb_context *tdb_open_ex(const char *name, int hash_size, int td
 	uint32_t magic1, magic2;
 	int ret;
 
-	ZERO_STRUCT(header);
-
 	if (!(tdb = (struct tdb_context *)calloc(1, sizeof *tdb))) {
 		/* Can't log this */
 		errno = ENOMEM;
-- 
2.11.0


From 58c4e163604527308ff088cfad5b14b04eb330a5 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 22 Oct 2018 10:14:23 +0200
Subject: [PATCH 03/10] tdb: Fix a typo

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tdb/common/freelist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/tdb/common/freelist.c b/lib/tdb/common/freelist.c
index 8870a418328..e4bbfce0027 100644
--- a/lib/tdb/common/freelist.c
+++ b/lib/tdb/common/freelist.c
@@ -199,7 +199,7 @@ static int merge_with_left_record(struct tdb_context *tdb,
  *   0 if left was not a free record
  *   1 if left was free and successfully merged.
  *
- * The currend record is handed in with pointer and fully read record.
+ * The current record is handed in with pointer and fully read record.
  *
  * The left record pointer and struct can be retrieved as result
  * in lp and lr;
-- 
2.11.0


From f4bf89fcb018d792c6bd79e1cb66811fd7fdf4be Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 22 Oct 2018 10:10:24 +0200
Subject: [PATCH 04/10] tdb: Remove "USE_RIGHT_MERGES" code

This has not been activated by default for ages and can be very
inefficient. With check_merge_with_left_record() we have an
alternative that will merge freelist records while we walk it
anyway. This has reduced fragmentation significantly

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tdb/common/freelist.c | 57 -----------------------------------------------
 1 file changed, 57 deletions(-)

diff --git a/lib/tdb/common/freelist.c b/lib/tdb/common/freelist.c
index e4bbfce0027..5afd89bc554 100644
--- a/lib/tdb/common/freelist.c
+++ b/lib/tdb/common/freelist.c
@@ -27,12 +27,6 @@
 
 #include "tdb_private.h"
 
-/* 'right' merges can involve O(n^2) cost when combined with a
-   traverse, so they are disabled until we find a way to do them in
-   O(1) time
-*/
-#define USE_RIGHT_MERGES 0
-
 /* read a freelist record and check for simple errors */
 int tdb_rec_free_read(struct tdb_context *tdb, tdb_off_t off, struct tdb_record *rec)
 {
@@ -61,30 +55,6 @@ int tdb_rec_free_read(struct tdb_context *tdb, tdb_off_t off, struct tdb_record
 	return 0;
 }
 
-
-#if USE_RIGHT_MERGES
-/* Remove an element from the freelist.  Must have alloc lock. */
-static int remove_from_freelist(struct tdb_context *tdb, tdb_off_t off, tdb_off_t next)
-{
-	tdb_off_t last_ptr, i;
-
-	/* read in the freelist top */
-	last_ptr = FREELIST_TOP;
-	while (tdb_ofs_read(tdb, last_ptr, &i) != -1 && i != 0) {
-		if (i == off) {
-			/* We've found it! */
-			return tdb_ofs_write(tdb, last_ptr, &next);
-		}
-		/* Follow chain (next offset is at start of record) */
-		last_ptr = i;
-	}
-	tdb->ecode = TDB_ERR_CORRUPT;
-	TDB_LOG((tdb, TDB_DEBUG_FATAL,"remove_from_freelist: not on list at off=%u\n", off));
-	return -1;
-}
-#endif
-
-
 /* update a record tailer (must hold allocation lock) */
 static int update_tailer(struct tdb_context *tdb, tdb_off_t offset,
 			 const struct tdb_record *rec)
@@ -318,33 +288,6 @@ int tdb_free(struct tdb_context *tdb, tdb_off_t offset, struct tdb_record *rec)
 		goto fail;
 	}
 
-#if USE_RIGHT_MERGES
-	/* Look right first (I'm an Australian, dammit) */
-	if (offset + sizeof(*rec) + rec->rec_len + sizeof(*rec) <= tdb->map_size) {
-		tdb_off_t right = offset + sizeof(*rec) + rec->rec_len;
-		struct tdb_record r;
-
-		if (tdb->methods->tdb_read(tdb, right, &r, sizeof(r), DOCONV()) == -1) {
-			TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_free: right read failed at %u\n", right));
-			goto left;
-		}
-
-		/* If it's free, expand to include it. */
-		if (r.magic == TDB_FREE_MAGIC) {
-			if (remove_from_freelist(tdb, right, r.next) == -1) {
-				TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_free: right free failed at %u\n", right));
-				goto left;
-			}
-			rec->rec_len += sizeof(r) + r.rec_len;
-			if (update_tailer(tdb, offset, rec) == -1) {
-				TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_free: update_tailer failed at %u\n", offset));
-				goto fail;
-			}
-		}
-	}
-left:
-#endif
-
 	ret = check_merge_with_left_record(tdb, offset, rec, NULL, NULL);
 	if (ret == -1) {
 		goto fail;
-- 
2.11.0


From d04c72a321240fceaea894181ad3089e8a917467 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 22 Oct 2018 17:18:43 +0200
Subject: [PATCH 05/10] tdbtorture: No transaction with mutexes

Right now we don't do transactions with mutexed tdbs. tdbtorture -m
locks up. I haven't really investigated why that is the case. The lockup
confused me quite a bit until I figured out it works fine as long as it
does not do transactions, which is all we need right now.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tdb/tools/tdbtorture.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/lib/tdb/tools/tdbtorture.c b/lib/tdb/tools/tdbtorture.c
index 3640dc7ed6c..1655c28dfc8 100644
--- a/lib/tdb/tools/tdbtorture.c
+++ b/lib/tdb/tools/tdbtorture.c
@@ -93,6 +93,19 @@ static int cull_traverse(struct tdb_context *tdb, TDB_DATA key, TDB_DATA dbuf,
 	return 0;
 }
 
+static bool do_transaction(void)
+{
+#if TRANSACTION_PROB
+	if (mutex) {
+		return false;
+	}
+	if (random() % TRANSACTION_PROB == 0) {
+		return true;
+	}
+#endif
+	return false;
+}
+
 static void addrec_db(void)
 {
 	int klen, dlen;
@@ -118,16 +131,15 @@ static void addrec_db(void)
 	}
 #endif
 
-#if TRANSACTION_PROB
 	if (in_transaction == 0 &&
-	    (always_transaction || random() % TRANSACTION_PROB == 0)) {
+	    (always_transaction || do_transaction())) {
 		if (tdb_transaction_start(db) != 0) {
 			fatal("tdb_transaction_start failed");
 		}
 		in_transaction++;
 		goto next;
 	}
-	if (in_transaction && random() % TRANSACTION_PROB == 0) {
+	if (in_transaction && do_transaction()) {
 		if (random() % TRANSACTION_PREPARE_PROB == 0) {
 			if (tdb_transaction_prepare_commit(db) != 0) {
 				fatal("tdb_transaction_prepare_commit failed");
@@ -139,14 +151,13 @@ static void addrec_db(void)
 		in_transaction--;
 		goto next;
 	}
-	if (in_transaction && random() % TRANSACTION_PROB == 0) {
+	if (in_transaction && do_transaction()) {
 		if (tdb_transaction_cancel(db) != 0) {
 			fatal("tdb_transaction_cancel failed");
 		}
 		in_transaction--;
 		goto next;
 	}
-#endif
 
 #if DELETE_PROB
 	if (random() % DELETE_PROB == 0) {
-- 
2.11.0


From e71cc11a1d7f6d17791ac042f7382a997b2a098c Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 23 Oct 2018 05:53:24 +0200
Subject: [PATCH 06/10] tdbtorture: Align integer types

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 lib/tdb/tools/tdbtorture.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/tdb/tools/tdbtorture.c b/lib/tdb/tools/tdbtorture.c
index 1655c28dfc8..7d08d4f6775 100644
--- a/lib/tdb/tools/tdbtorture.c
+++ b/lib/tdb/tools/tdbtorture.c
@@ -31,7 +31,7 @@ static int in_transaction;
 static int error_count;
 static int always_transaction = 0;
 static int hash_size = 2;
-static int loopnum;
+static unsigned loopnum;
 static int count_pipe;
 static bool mutex = false;
 static struct tdb_logging_context log_ctx;
-- 
2.11.0


From 10447b9bcb825f1f3c89c1bf42e5c3e7d0cb815e Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 24 Sep 2018 06:40:46 -0700
Subject: [PATCH 07/10] smbd: Replace some GUID_string by GUID_buf_string

It's only debug statements, but I would like to promote the
stack-allocation routines as good practice where they make sense.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/smbd/smbXsrv_client.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c
index 9fc8bdae1cf..6db34d7c174 100644
--- a/source3/smbd/smbXsrv_client.c
+++ b/source3/smbd/smbXsrv_client.c
@@ -133,8 +133,9 @@ static struct db_record *smbXsrv_client_global_fetch_locked(
 	rec = dbwrap_fetch_locked(db, mem_ctx, key);
 
 	if (rec == NULL) {
+		struct GUID_txt_buf buf;
 		DBG_DEBUG("Failed to lock guid [%s], key '%s'\n",
-			  GUID_string(talloc_tos(), client_guid),
+			  GUID_buf_string(client_guid, &buf),
 			  hex_encode_talloc(talloc_tos(), key.dptr, key.dsize));
 	}
 
@@ -533,13 +534,14 @@ NTSTATUS smbXsrv_client_create(TALLOC_CTX *mem_ctx,
 
 	if (DEBUGLVL(DBGLVL_DEBUG)) {
 		struct smbXsrv_clientB client_blob;
+		struct GUID_txt_buf buf;
 
 		ZERO_STRUCT(client_blob);
 		client_blob.version = SMBXSRV_VERSION_0;
 		client_blob.info.info0 = client;
 
 		DBG_DEBUG("client_guid[%s] stored\n",
-			  GUID_string(talloc_tos(), &global->client_guid));
+			  GUID_buf_string(&global->client_guid, &buf));
 		NDR_PRINT_DEBUG(smbXsrv_clientB, &client_blob);
 	}
 
@@ -626,9 +628,13 @@ static void smbXsrv_client_connection_pass_loop(struct tevent_req *subreq)
 
 	if (!GUID_equal(&client->global->client_guid, &pass_info0->client_guid))
 	{
+		struct GUID_txt_buf buf1, buf2;
+
 		DBG_WARNING("client's client_guid [%s] != passed guid [%s]\n",
-			GUID_string(talloc_tos(), &client->global->client_guid),
-			GUID_string(talloc_tos(), &pass_info0->client_guid));
+			    GUID_buf_string(&client->global->client_guid,
+					    &buf1),
+			    GUID_buf_string(&pass_info0->client_guid,
+					    &buf2));
 		if (DEBUGLVL(DBGLVL_WARNING)) {
 			NDR_PRINT_DEBUG(smbXsrv_connection_passB, &pass_blob);
 		}
@@ -700,9 +706,10 @@ NTSTATUS smbXsrv_client_update(struct smbXsrv_client *client)
 	NTSTATUS status;
 
 	if (client->global->db_rec != NULL) {
+		struct GUID_txt_buf buf;
 		DBG_ERR("guid [%s]: Called with db_rec != NULL'\n",
-			GUID_string(talloc_tos(),
-			&client->global->client_guid));
+			GUID_buf_string(&client->global->client_guid,
+					&buf));
 		return NT_STATUS_INTERNAL_ERROR;
 	}
 
@@ -716,21 +723,25 @@ NTSTATUS smbXsrv_client_update(struct smbXsrv_client *client)
 
 	status = smbXsrv_client_global_store(client->global);
 	if (!NT_STATUS_IS_OK(status)) {
+		struct GUID_txt_buf buf;
 		DBG_ERR("client_guid[%s] store failed - %s\n",
-			GUID_string(talloc_tos(), &client->global->client_guid),
+			GUID_buf_string(&client->global->client_guid,
+					&buf),
 			nt_errstr(status));
 		return status;
 	}
 
 	if (DEBUGLVL(DBGLVL_DEBUG)) {
 		struct smbXsrv_clientB client_blob;
+		struct GUID_txt_buf buf;
 
 		ZERO_STRUCT(client_blob);
 		client_blob.version = SMBXSRV_VERSION_0;
 		client_blob.info.info0 = client;
 
 		DBG_DEBUG("client_guid[%s] stored\n",
-			GUID_string(talloc_tos(), &client->global->client_guid));
+			  GUID_buf_string(&client->global->client_guid,
+					  &buf));
 		NDR_PRINT_DEBUG(smbXsrv_clientB, &client_blob);
 	}
 
@@ -743,8 +754,10 @@ NTSTATUS smbXsrv_client_remove(struct smbXsrv_client *client)
 	NTSTATUS status;
 
 	if (client->global->db_rec != NULL) {
+		struct GUID_txt_buf buf;
 		DBG_ERR("client_guid[%s]: Called with db_rec != NULL'\n",
-			GUID_string(talloc_tos(), &client->global->client_guid));
+			GUID_buf_string(&client->global->client_guid,
+					&buf));
 		return NT_STATUS_INTERNAL_ERROR;
 	}
 
@@ -762,21 +775,23 @@ NTSTATUS smbXsrv_client_remove(struct smbXsrv_client *client)
 
 	status = smbXsrv_client_global_remove(client->global);
 	if (!NT_STATUS_IS_OK(status)) {
+		struct GUID_txt_buf buf;
 		DBG_ERR("client_guid[%s] store failed - %s\n",
-			GUID_string(talloc_tos(), &client->global->client_guid),
+			GUID_buf_string(&client->global->client_guid, &buf),
 			nt_errstr(status));
 		return status;
 	}
 
 	if (DEBUGLVL(DBGLVL_DEBUG)) {
 		struct smbXsrv_clientB client_blob;
+		struct GUID_txt_buf buf;
 
 		ZERO_STRUCT(client_blob);
 		client_blob.version = SMBXSRV_VERSION_0;
 		client_blob.info.info0 = client;
 
 		DBG_DEBUG("client_guid[%s] stored\n",
-			GUID_string(talloc_tos(), &client->global->client_guid));
+			  GUID_buf_string(&client->global->client_guid, &buf));
 		NDR_PRINT_DEBUG(smbXsrv_clientB, &client_blob);
 	}
 
-- 
2.11.0


From 4171a4ccf512bc53dfc0616d1301aa1eb4e4a8e6 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 18 Oct 2018 05:46:03 +0200
Subject: [PATCH 08/10] lib: Avoid an "includes.h"

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 libcli/security/dom_sid.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libcli/security/dom_sid.c b/libcli/security/dom_sid.c
index 17ac0560d83..b876fc8b780 100644
--- a/libcli/security/dom_sid.c
+++ b/libcli/security/dom_sid.c
@@ -20,7 +20,10 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
 
-#include "includes.h"
+#include "replace.h"
+#include "lib/util/data_blob.h"
+#include "system/locale.h"
+#include "lib/util/debug.h"
 #include "librpc/gen_ndr/security.h"
 #include "dom_sid.h"
 
-- 
2.11.0


From 8a9fa964e796e1ef97b9bc0c86904f1f5938e8e9 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 24 Sep 2018 15:46:27 +0200
Subject: [PATCH 09/10] smbd: Slightly optimize delay_rename_for_lease_break

Do the checks with increasing cost, possibly avoid more expensive ones

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/smbd/smb2_setinfo.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/source3/smbd/smb2_setinfo.c b/source3/smbd/smb2_setinfo.c
index 9b4620a5276..11b126aa794 100644
--- a/source3/smbd/smb2_setinfo.c
+++ b/source3/smbd/smb2_setinfo.c
@@ -203,13 +203,19 @@ static struct tevent_req *delay_rename_for_lease_break(struct tevent_req *req,
 	for (i=0; i<d->num_share_modes; i++) {
 		struct share_mode_entry *e = &d->share_modes[i];
 		struct share_mode_lease *l = NULL;
-		uint32_t e_lease_type = get_lease_type(d, e);
+		uint32_t e_lease_type;
 		uint32_t break_to;
 
 		if (e->op_type != LEASE_OPLOCK) {
 			continue;
 		}
 
+		e_lease_type = get_lease_type(d, e);
+
+		if (!(e_lease_type & SMB2_LEASE_HANDLE)) {
+			continue;
+		}
+
 		l = &d->leases[e->lease_idx];
 
 		if (smb2_lease_equal(fsp_client_guid(fsp),
@@ -223,10 +229,6 @@ static struct tevent_req *delay_rename_for_lease_break(struct tevent_req *req,
 			continue;
 		}
 
-		if (!(e_lease_type & SMB2_LEASE_HANDLE)) {
-			continue;
-		}
-
 		delay = true;
 		break_to = (e_lease_type & ~SMB2_LEASE_HANDLE);
 
-- 
2.11.0


From a30141e34c1fbbaa17e90f4e399fbcd208775676 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 19 Sep 2018 16:26:09 +0200
Subject: [PATCH 10/10] smbd: Move a variable declaration closer to its use

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/smbd/open.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 6bfd5ba1371..82c640e5e9a 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -4962,13 +4962,13 @@ static NTSTATUS lease_match(connection_struct *conn,
 		for (j=0; j<d->num_share_modes; j++) {
 			struct share_mode_entry *e = &d->share_modes[j];
 			uint32_t e_lease_type = get_lease_type(d, e);
-			struct share_mode_lease *l = NULL;
 
 			if (share_mode_stale_pid(d, j)) {
 				continue;
 			}
 
 			if (e->op_type == LEASE_OPLOCK) {
+				struct share_mode_lease *l = NULL;
 				l = &lck->data->leases[e->lease_idx];
 				if (!smb2_lease_key_equal(&l->lease_key,
 							  lease_key)) {
-- 
2.11.0



More information about the samba-technical mailing list