[PATCHES v2 BUG 13865] Properly track the size of talloc objects

Christof Schmitt cs at samba.org
Fri Apr 5 22:52:30 UTC 2019


On Fri, Apr 05, 2019 at 03:43:45PM -0700, Jeremy Allison wrote:
> On Fri, Apr 05, 2019 at 03:34:17PM -0700, Christof Schmitt wrote:
> > 
> > Anything we choose is a guess, since every workload is different. On a
> > quick test to open multiple files with smbclient and then checking size
> > allocated under 'struct memcache' in smbcontrol pool-usage, shows that
> > each open file increases the memory usage by something like 300 to 700
> > bytes.
> 
> Actually that data is very useful.
> 
> > So that 16k for each share mode seems high, although that might increase
> > with concurrent access to the same file. 4k for each getwd cache also
> > seems high, that seems to mainly depend on the length of the path name.
> 
> With much concurrent access we store more share mode data,
> but reasonably that value is probably at max 1K per file
> then.
> 
> > Rounding the share mode size up to 1024 bytes and the getwd length to
> > 1024, and assuming one getwd cache for each open file (probably also too
> > high), that would yield 100 * 1024 + 100 * 1024 = 200k
> > 
> > That would be more in line with doubling the state cache size from 256
> > to 512 (kB).
> 
> Yeah, thinking about this some more I'm OK with 512k or 1Mb.
> 
> I'll let you chose and send the patch :-).

Let's go with 512kB then.

> Remember to tag with bug:
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13865
> 
> So we can keep everything together for the back-port.
> FYI, I already set an autobuild going on sn-devel with your
> fixes.

Ok. I attached all patches, including the one for the size change.

Christof
-------------- next part --------------
From d43fcd188b35424aa260036e93534d331cea3afd Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at 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 at samba.org>
---
 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 14a41c32e4f12012339d8b8c0c593939a23dba7c Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at 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 at samba.org>
---
 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 61f6898ec295cec9c71a151d8c6e028b19b28289 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at 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 at samba.org>
---
 docs-xml/smbdotconf/filename/maxstatcachesize.xml | 2 +-
 source3/param/loadparm.c                          | 2 +-
 2 files changed, 2 insertions(+), 2 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/source3/param/loadparm.c b/source3/param/loadparm.c
index 6aa84018111..703460e4c47 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -698,7 +698,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 e70e4c58d8dc1ba68579b27043f73705ad54d515 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at 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 at samba.org>
---
 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 219ac4a370c..782980dec3a 100644
--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -11155,13 +11155,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;
@@ -11187,6 +11188,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);
 
@@ -11250,6 +11253,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



More information about the samba-technical mailing list