Attachment 15050 Details for Bug 13865 – patches for 4.9

[patch] patches for 4.9

patches-bug-13865-4.9 (text/plain), 12.28 KB, created by Christof Schmitt on 2019-04-08 23:25:53 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Christof Schmitt

Created: 2019-04-08 23:25:53 UTC

Size: 12.28 KB

patch

obsolete

>From 362acf10b30766973cd75886831ff46c49d65652 Mon Sep 17 00:00:00 2001
>From: Christof Schmitt <cs@samba.org>
>Date: Mon, 1 Apr 2019 15:38:59 -0700
>Subject: [PATCH 1/4] memcache: Introduce struct for storing talloc pointer
>
>This allows extending the additional data stored for talloced objects
>later.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13865
>
>Signed-off-by: Christof Schmitt <cs@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit 7c44f2f76eefb9156cb1d170c92b4ff07dd6a3d5)
>---
> lib/util/memcache.c | 35 ++++++++++++++++++++---------------
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
>diff --git a/lib/util/memcache.c b/lib/util/memcache.c
>index 819ba44b443..d0250c6b3ee 100644
>--- a/lib/util/memcache.c
>+++ b/lib/util/memcache.c
>@@ -27,6 +27,10 @@
> 
> static struct memcache *global_cache;
> 
>+struct memcache_talloc_value {
>+	void *ptr;
>+};
>+
> struct memcache_element {
> 	struct rb_node rb_node;
> 	struct memcache_element *prev, *next;
>@@ -180,19 +184,19 @@ void *memcache_lookup_talloc(struct memcache *cache, enum memcache_number n,
> 			     DATA_BLOB key)
> {
> 	DATA_BLOB value;
>-	void *result;
>+	struct memcache_talloc_value mtv;
> 
> 	if (!memcache_lookup(cache, n, key, &value)) {
> 		return NULL;
> 	}
> 
>-	if (value.length != sizeof(result)) {
>+	if (value.length != sizeof(mtv)) {
> 		return NULL;
> 	}
> 
>-	memcpy(&result, value.data, sizeof(result));
>+	memcpy(&mtv, value.data, sizeof(mtv));
> 
>-	return result;
>+	return mtv.ptr;
> }
> 
> static void memcache_delete_element(struct memcache *cache,
>@@ -204,12 +208,12 @@ static void memcache_delete_element(struct memcache *cache,
> 
> 	if (memcache_is_talloc(e->n)) {
> 		DATA_BLOB cache_key, cache_value;
>-		void *ptr;
>+		struct memcache_talloc_value mtv;
> 
> 		memcache_element_parse(e, &cache_key, &cache_value);
>-		SMB_ASSERT(cache_value.length == sizeof(ptr));
>-		memcpy(&ptr, cache_value.data, sizeof(ptr));
>-		TALLOC_FREE(ptr);
>+		SMB_ASSERT(cache_value.length == sizeof(mtv));
>+		memcpy(&mtv, cache_value.data, sizeof(mtv));
>+		TALLOC_FREE(mtv.ptr);
> 	}
> 
> 	cache->size -= memcache_element_size(e->keylength, e->valuelength);
>@@ -275,10 +279,11 @@ void memcache_add(struct memcache *cache, enum memcache_number n,
> 
> 		if (value.length <= cache_value.length) {
> 			if (memcache_is_talloc(e->n)) {
>-				void *ptr;
>-				SMB_ASSERT(cache_value.length == sizeof(ptr));
>-				memcpy(&ptr, cache_value.data, sizeof(ptr));
>-				TALLOC_FREE(ptr);
>+				struct memcache_talloc_value mtv;
>+
>+				SMB_ASSERT(cache_value.length == sizeof(mtv));
>+				memcpy(&mtv, cache_value.data, sizeof(mtv));
>+				TALLOC_FREE(mtv.ptr);
> 			}
> 			/*
> 			 * We can reuse the existing record
>@@ -334,8 +339,8 @@ void memcache_add(struct memcache *cache, enum memcache_number n,
> void memcache_add_talloc(struct memcache *cache, enum memcache_number n,
> 			 DATA_BLOB key, void *pptr)
> {
>+	struct memcache_talloc_value mtv;
> 	void **ptr = (void **)pptr;
>-	void *p;
> 
> 	if (cache == NULL) {
> 		cache = global_cache;
>@@ -344,8 +349,8 @@ void memcache_add_talloc(struct memcache *cache, enum memcache_number n,
> 		return;
> 	}
> 
>-	p = talloc_move(cache, ptr);
>-	memcache_add(cache, n, key, data_blob_const(&p, sizeof(p)));
>+	mtv.ptr = talloc_move(cache, ptr);
>+	memcache_add(cache, n, key, data_blob_const(&mtv, sizeof(mtv)));
> }
> 
> void memcache_flush(struct memcache *cache, enum memcache_number n)
>-- 
>2.17.0
>
>
>From eab9eabb9e1b82b40fc84d96fe440a9b86e4654d Mon Sep 17 00:00:00 2001
>From: Christof Schmitt <cs@samba.org>
>Date: Mon, 1 Apr 2019 16:23:35 -0700
>Subject: [PATCH 2/4] memcache: Properly track the size of talloc objects
>
>With memcache_add_talloc, the talloc object becomes part of the pool and
>the memcache_element stores a pointer to the talloc object. The
>size of the the talloc object was not used when tracking the used space,
>allowing the cache to grow larger than defined in the memcache_init
>call.
>
>Fix this by adding the size of the talloc object to the used space.
>
>Also record the initial size of the talloc object for proper adjustment
>of the used space in the cache later. This is in case the size of the
>talloc object is modified while being owned by the cache (e.g.
>allocating talloc child objects). This should never happen, but better
>be safe than ending up with a broken cache usage counter.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13865
>
>Signed-off-by: Christof Schmitt <cs@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit a04ca6f3438595ba7e1a110877f53d1cac0f0402)
>---
> lib/util/memcache.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
>diff --git a/lib/util/memcache.c b/lib/util/memcache.c
>index d0250c6b3ee..1e616bd0e9a 100644
>--- a/lib/util/memcache.c
>+++ b/lib/util/memcache.c
>@@ -29,6 +29,7 @@ static struct memcache *global_cache;
> 
> struct memcache_talloc_value {
> 	void *ptr;
>+	size_t len;
> };
> 
> struct memcache_element {
>@@ -213,6 +214,7 @@ static void memcache_delete_element(struct memcache *cache,
> 		memcache_element_parse(e, &cache_key, &cache_value);
> 		SMB_ASSERT(cache_value.length == sizeof(mtv));
> 		memcpy(&mtv, cache_value.data, sizeof(mtv));
>+		cache->size -= mtv.len;
> 		TALLOC_FREE(mtv.ptr);
> 	}
> 
>@@ -283,6 +285,7 @@ void memcache_add(struct memcache *cache, enum memcache_number n,
> 
> 				SMB_ASSERT(cache_value.length == sizeof(mtv));
> 				memcpy(&mtv, cache_value.data, sizeof(mtv));
>+				cache->size -= mtv.len;
> 				TALLOC_FREE(mtv.ptr);
> 			}
> 			/*
>@@ -290,6 +293,14 @@ void memcache_add(struct memcache *cache, enum memcache_number n,
> 			 */
> 			memcpy(cache_value.data, value.data, value.length);
> 			e->valuelength = value.length;
>+
>+			if (memcache_is_talloc(e->n)) {
>+				struct memcache_talloc_value mtv;
>+
>+				SMB_ASSERT(cache_value.length == sizeof(mtv));
>+				memcpy(&mtv, cache_value.data, sizeof(mtv));
>+				cache->size += mtv.len;
>+			}
> 			return;
> 		}
> 
>@@ -333,6 +344,13 @@ void memcache_add(struct memcache *cache, enum memcache_number n,
> 	DLIST_ADD(cache->mru, e);
> 
> 	cache->size += element_size;
>+	if (memcache_is_talloc(e->n)) {
>+		struct memcache_talloc_value mtv;
>+
>+		SMB_ASSERT(cache_value.length == sizeof(mtv));
>+		memcpy(&mtv, cache_value.data, sizeof(mtv));
>+		cache->size += mtv.len;
>+	}
> 	memcache_trim(cache);
> }
> 
>@@ -349,6 +367,7 @@ void memcache_add_talloc(struct memcache *cache, enum memcache_number n,
> 		return;
> 	}
> 
>+	mtv.len = talloc_total_size(*ptr);
> 	mtv.ptr = talloc_move(cache, ptr);
> 	memcache_add(cache, n, key, data_blob_const(&mtv, sizeof(mtv)));
> }
>-- 
>2.17.0
>
>
>From fa67ef6e73b20edfc9978d9c408f38593be80762 Mon Sep 17 00:00:00 2001
>From: Christof Schmitt <cs@samba.org>
>Date: Fri, 5 Apr 2019 15:43:21 -0700
>Subject: [PATCH 3/4] memcache: Increase size of default memcache to 512k
>
>With the fixed accounting of talloc objects, the default cache size
>needs to increase. The exact increase required depends on the workloads,
>going form 256k to 512k seems like a reasonable guess.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13865
>
>Signed-off-by: Christof Schmitt <cs@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit 9ff5c0bab76c5d3d7bea1fcb79861d0c9a3b9839)
>---
> docs-xml/smbdotconf/filename/maxstatcachesize.xml | 2 +-
> lib/param/loadparm.c                              | 2 +-
> source3/param/loadparm.c                          | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/docs-xml/smbdotconf/filename/maxstatcachesize.xml b/docs-xml/smbdotconf/filename/maxstatcachesize.xml
>index 63f91e70fd2..866d74d6ffd 100644
>--- a/docs-xml/smbdotconf/filename/maxstatcachesize.xml
>+++ b/docs-xml/smbdotconf/filename/maxstatcachesize.xml
>@@ -13,6 +13,6 @@
> 	</para>
> </description>
> <related>stat cache</related>
>-<value type="default">256</value>
>+<value type="default">512</value>
> <value type="example">100</value>
> </samba:parameter>
>diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c
>index 1debcfff127..15b1440694c 100644
>--- a/lib/param/loadparm.c
>+++ b/lib/param/loadparm.c
>@@ -2929,7 +2929,7 @@ struct loadparm_context *loadparm_init(TALLOC_CTX *mem_ctx)
> 
> 	lpcfg_do_global_parameter(lp_ctx, "durable handles", "yes");
> 
>-	lpcfg_do_global_parameter(lp_ctx, "max stat cache size", "256");
>+	lpcfg_do_global_parameter(lp_ctx, "max stat cache size", "512");
> 
> 	lpcfg_do_global_parameter(lp_ctx, "ldap passwd sync", "no");
> 
>diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
>index 322934c55f0..fb95ccd823d 100644
>--- a/source3/param/loadparm.c
>+++ b/source3/param/loadparm.c
>@@ -696,7 +696,7 @@ static void init_globals(struct loadparm_context *lp_ctx, bool reinit_globals)
> 	Globals.nt_status_support = true; /* Use NT status by default. */
> 	Globals.smbd_profiling_level = 0;
> 	Globals.stat_cache = true;	/* use stat cache by default */
>-	Globals.max_stat_cache_size = 256; /* 256k by default */
>+	Globals.max_stat_cache_size = 512; /* 512k by default */
> 	Globals.restrict_anonymous = 0;
> 	Globals.client_lanman_auth = false;	/* Do NOT use the LanMan hash if it is available */
> 	Globals.client_plaintext_auth = false;	/* Do NOT use a plaintext password even if is requested by the server */
>-- 
>2.17.0
>
>
>From e30246986d15304d2d803a54b1359417e700a9f2 Mon Sep 17 00:00:00 2001
>From: Christof Schmitt <cs@samba.org>
>Date: Thu, 28 Mar 2019 10:46:43 -0700
>Subject: [PATCH 4/4] torture: Add test for talloc size accounting in memcache
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13865
>
>Signed-off-by: Christof Schmitt <cs@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>
>Autobuild-User(master): Jeremy Allison <jra@samba.org>
>Autobuild-Date(master): Sat Apr  6 06:08:42 UTC 2019 on sn-devel-144
>
>(cherry picked from commit b7028c42462c34cf86cb949bfdb16ebc7ed0a6c6)
>---
> source3/torture/torture.c | 70 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 69 insertions(+), 1 deletion(-)
>
>diff --git a/source3/torture/torture.c b/source3/torture/torture.c
>index 854f6a775f6..2b020ca0f9c 100644
>--- a/source3/torture/torture.c
>+++ b/source3/torture/torture.c
>@@ -10989,13 +10989,14 @@ static bool data_blob_equal(DATA_BLOB a, DATA_BLOB b)
> static bool run_local_memcache(int dummy)
> {
> 	struct memcache *cache;
>-	DATA_BLOB k1, k2, k3;
>+	DATA_BLOB k1, k2, k3, k4, k5;
> 	DATA_BLOB d1, d3;
> 	DATA_BLOB v1, v3;
> 
> 	TALLOC_CTX *mem_ctx;
> 	char *ptr1 = NULL;
> 	char *ptr2 = NULL;
>+	char *ptr3 = NULL;
> 
> 	char *str1, *str2;
> 	size_t size1, size2;
>@@ -11021,6 +11022,8 @@ static bool run_local_memcache(int dummy)
> 	k1 = data_blob_const("d1", 2);
> 	k2 = data_blob_const("d2", 2);
> 	k3 = data_blob_const("d3", 2);
>+	k4 = data_blob_const("d4", 2);
>+	k5 = data_blob_const("d5", 2);
> 
> 	memcache_add(cache, STAT_CACHE, k1, d1);
> 
>@@ -11084,6 +11087,71 @@ static bool run_local_memcache(int dummy)
> 		return false;
> 	}
> 
>+	/*
>+	 * Test that talloc size also is accounted in memcache and
>+	 * causes purge of other object.
>+	 */
>+
>+	str1 = talloc_zero_size(mem_ctx, 100);
>+	str2 = talloc_zero_size(mem_ctx, 100);
>+
>+	memcache_add_talloc(cache, GETWD_CACHE, k4, &str1);
>+	memcache_add_talloc(cache, GETWD_CACHE, k5, &str1);
>+
>+	ptr3 = memcache_lookup_talloc(cache, GETWD_CACHE, k4);
>+	if (ptr3 != NULL) {
>+		printf("Did find k4, should have been purged\n");
>+		return false;
>+	}
>+
>+	/*
>+	 * Test that adding a duplicate non-talloced
>+	 * key/value on top of a talloced key/value takes account
>+	 * of the talloc_freed value size.
>+	 */
>+	TALLOC_FREE(cache);
>+	TALLOC_FREE(mem_ctx);
>+
>+	mem_ctx = talloc_init("key_replace");
>+	if (mem_ctx == NULL) {
>+		return false;
>+	}
>+
>+	cache = memcache_init(NULL, sizeof(void *) == 8 ? 200 : 100);
>+	if (cache == NULL) {
>+		return false;
>+	}
>+
>+	/*
>+	 * Add a 100 byte talloced string. This will
>+	 * store a (4 or 8 byte) pointer and record the
>+	 * total talloced size.
>+	 */
>+	str1 = talloc_zero_size(mem_ctx, 100);
>+	memcache_add_talloc(cache, GETWD_CACHE, k4, &str1);
>+	/*
>+	 * Now overwrite with a small talloced
>+	 * value. This should fit in the existing size
>+	 * and the total talloced size should be removed
>+	 * from the cache size.
>+	 */
>+	str1 = talloc_zero_size(mem_ctx, 2);
>+	memcache_add_talloc(cache, GETWD_CACHE, k4, &str1);
>+	/*
>+	 * Now store a 20 byte string. If the
>+	 * total talloced size wasn't accounted for
>+	 * and removed in the overwrite, then this
>+	 * will evict k4.
>+	 */
>+	str2 = talloc_zero_size(mem_ctx, 20);
>+	memcache_add_talloc(cache, GETWD_CACHE, k5, &str2);
>+
>+	ptr3 = memcache_lookup_talloc(cache, GETWD_CACHE, k4);
>+	if (ptr3 == NULL) {
>+		printf("Did not find k4, should not have been purged\n");
>+		return false;
>+	}
>+
> 	TALLOC_FREE(cache);
> 	TALLOC_FREE(mem_ctx);
> 
>-- 
>2.17.0
>

Flags:

jra: review+

Actions: View

Attachments on bug 13865: 15021 | 15023 | 15050 | 15051