[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