From feb6cbff4d586bec1545f731377217b06241744f Mon Sep 17 00:00:00 2001 From: Christof Schmitt Date: Tue, 26 Mar 2019 08:33:08 -0700 Subject: [PATCH 1/3] 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. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13865 Signed-off-by: Christof Schmitt Reviewed-by: Jeremy Allison --- lib/util/memcache.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/util/memcache.c b/lib/util/memcache.c index 819ba44b443..2066fc5b2dd 100644 --- a/lib/util/memcache.c +++ b/lib/util/memcache.c @@ -209,6 +209,7 @@ static void memcache_delete_element(struct memcache *cache, memcache_element_parse(e, &cache_key, &cache_value); SMB_ASSERT(cache_value.length == sizeof(ptr)); memcpy(&ptr, cache_value.data, sizeof(ptr)); + cache->size -= talloc_total_size(ptr); TALLOC_FREE(ptr); } @@ -248,8 +249,8 @@ void memcache_delete(struct memcache *cache, enum memcache_number n, memcache_delete_element(cache, e); } -void memcache_add(struct memcache *cache, enum memcache_number n, - DATA_BLOB key, DATA_BLOB value) +static void memcache_do_add(struct memcache *cache, enum memcache_number n, + DATA_BLOB key, DATA_BLOB value, size_t talloc_size) { struct memcache_element *e; struct rb_node **p; @@ -278,6 +279,7 @@ void memcache_add(struct memcache *cache, enum memcache_number n, void *ptr; SMB_ASSERT(cache_value.length == sizeof(ptr)); memcpy(&ptr, cache_value.data, sizeof(ptr)); + cache->size -= talloc_total_size(ptr); TALLOC_FREE(ptr); } /* @@ -328,9 +330,16 @@ void memcache_add(struct memcache *cache, enum memcache_number n, DLIST_ADD(cache->mru, e); cache->size += element_size; + cache->size += talloc_size; memcache_trim(cache); } +void memcache_add(struct memcache *cache, enum memcache_number n, + DATA_BLOB key, DATA_BLOB value) +{ + memcache_do_add(cache, n, key, value, 0); +} + void memcache_add_talloc(struct memcache *cache, enum memcache_number n, DATA_BLOB key, void *pptr) { @@ -345,7 +354,8 @@ void memcache_add_talloc(struct memcache *cache, enum memcache_number n, } p = talloc_move(cache, ptr); - memcache_add(cache, n, key, data_blob_const(&p, sizeof(p))); + memcache_do_add(cache, n, key, data_blob_const(&p, sizeof(p)), + talloc_total_size(p)); } void memcache_flush(struct memcache *cache, enum memcache_number n) -- 2.21.0.392.gf8f6787159e-goog From 54f3e08690db17a3ff8dfd2ff011c46ad4966875 Mon Sep 17 00:00:00 2001 From: Christof Schmitt Date: Thu, 28 Mar 2019 10:46:43 -0700 Subject: [PATCH 2/3] torture: Add test for talloc size accounting in memcache BUG: https://bugzilla.samba.org/show_bug.cgi?id=13865 Signed-off-by: Christof Schmitt --- 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.21.0.392.gf8f6787159e-goog From 2dc57a3141d236c968cbbcfd8fa62c73382eeb9e Mon Sep 17 00:00:00 2001 From: Christof Schmitt Date: Tue, 26 Mar 2019 09:43:59 -0700 Subject: [PATCH 3/3] docs: Update description for 'max stat cache' The size specified here reflects the total size of all caches, not just the stat cache. It might also make sense to rename this parameter, but for for now at update the description. Signed-off-by: Christof Schmitt --- docs-xml/smbdotconf/filename/maxstatcachesize.xml | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/docs-xml/smbdotconf/filename/maxstatcachesize.xml b/docs-xml/smbdotconf/filename/maxstatcachesize.xml index 63f91e70fd2..d49b240ba6f 100644 --- a/docs-xml/smbdotconf/filename/maxstatcachesize.xml +++ b/docs-xml/smbdotconf/filename/maxstatcachesize.xml @@ -3,13 +3,14 @@ type="integer" xmlns:samba="http://www.samba.org/samba/DTD/samba-doc"> - This parameter limits the size in memory of any - stat cache being used - to speed up case insensitive name mappings. It represents - the number of kilobyte (1024) units the stat cache can use. - A value of zero, meaning unlimited, is not advisable due to - increased memory usage. You should not need to change this - parameter. + This parameter limits the total size of memory being + used for internal caches. This includes the stat cache used to speed up case + insensitive name mappings. The value of this parameter + represents the number of kilobyte (1024) units the caches can + use in total. A value of zero, meaning unlimited, is not + advisable due to increased memory usage. You should not need + to change this parameter. stat cache -- 2.21.0.392.gf8f6787159e-goog