[PATCHES BUG 13865] memcache tracking of talloc object sizes
Jeremy Allison
jra at samba.org
Fri Mar 29 20:07:42 UTC 2019
On Fri, Mar 29, 2019 at 01:00:33PM -0700, Jeremy Allison via samba-technical wrote:
> Actually I'm wrong - we can't mix talloc/non-talloc
> stores here (it crashes, I tested :-). But we still
> need to take the talloc'ed size into account.
>
> > >
> > > 281 TALLOC_FREE(ptr);
> > > 282 }
> > > 283 /*
> > > 284 * We can reuse the existing record
> > > 285 */
> > > 286 memcpy(cache_value.data, value.data, value.length);
> > > 287 e->valuelength = value.length;
> > > 288 return;
> > > 289 }
> > >
> > > Probably need to add a test for this too...
> >
> > Yes, will do. I will be busy with something else this afternoon, but i
> > should have an update for this early next week.
>
> I'm nearly done with the fix and test. If I can
> get it working I'll post for your review.
Here is the updated patchset containing the fix for the
corner case and additional test for it. I checked that
with the corner case fix commented out the test fails,
but passes with it added.
CI build here:
https://gitlab.com/samba-team/devel/samba/pipelines/54311307
Please review and push if happy !
Jeremy.
-------------- next part --------------
From feb6cbff4d586bec1545f731377217b06241744f Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
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 <cs at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
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 <cs at samba.org>
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 <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.21.0.392.gf8f6787159e-goog
From 2dc57a3141d236c968cbbcfd8fa62c73382eeb9e Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
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 <cs at samba.org>
---
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">
<description>
- <para>This parameter limits the size in memory of any
- <parameter moreinfo="none">stat cache</parameter> 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.
+ <para>This parameter limits the total size of memory being
+ used for internal caches. This includes the <parameter
+ moreinfo="none">stat cache</parameter> 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.
</para>
</description>
<related>stat cache</related>
--
2.21.0.392.gf8f6787159e-goog
More information about the samba-technical
mailing list