From 362acf10b30766973cd75886831ff46c49d65652 Mon Sep 17 00:00:00 2001 From: Christof Schmitt 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 Reviewed-by: Jeremy Allison (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 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 Reviewed-by: Jeremy Allison (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 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 Reviewed-by: Jeremy Allison (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 @@ stat cache -256 +512 100 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 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 Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison 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