From 53c1b893d85831fb996a3d8ca1bae66164ba4d89 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 395e78d28852b9ce030b1c22f88060b51b024003 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 6a3eeef2fd4b629b10dcf0c6c91c5e4f38e9dd48 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 f31ef2319ac..e4d27cae8ea 100644 --- a/lib/param/loadparm.c +++ b/lib/param/loadparm.c @@ -2930,7 +2930,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 29d9d59390b..c8a07c954f9 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 0c674db886f6b4e7b9b522642c1e468862bb554d 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 4abd80253e5..3507e4f198b 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -11149,13 +11149,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; @@ -11181,6 +11182,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); @@ -11244,6 +11247,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