From c4fdf09c0584a3cee32b92f01b7958c72776aedc Mon Sep 17 00:00:00 2001
From: Oran Agra <oran@redislabs.com>
Date: Sun, 6 Dec 2020 14:51:22 +0200
Subject: [PATCH] prevent client tracking from causing feedback loop in
performEvictions (#8100)
When client tracking is enabled signalModifiedKey can increase memory usage,
this can cause the loop in performEvictions to keep running since it was measuring
the memory usage impact of signalModifiedKey.
The section that measures the memory impact of the eviction should be just on dbDelete,
excluding keyspace notification, client tracking, and propagation to AOF and replicas.
This resolves part of the problem described in #8069
p.s. fix took 1 minute, test took about 3 hours to write.
---
src/evict.c | 4 +++-
tests/unit/maxmemory.tcl | 52 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/src/evict.c b/src/evict.c
index 3de328fc0..3642352ad 100644
--- a/src/evict.c
+++ b/src/evict.c
@@ -629,6 +629,8 @@ int performEvictions(void) {
* we are freeing removing the key, but we can't account for
* that otherwise we would never exit the loop.
*
+ * Same for CSC invalidation messages generated by signalModifiedKey.
+ *
* AOF and Output buffer memory will be freed eventually so
* we only care about memory used by the key space. */
delta = (long long) zmalloc_used_memory();
@@ -637,12 +639,12 @@ int performEvictions(void) {
dbAsyncDelete(db,keyobj);
else
dbSyncDelete(db,keyobj);
- signalModifiedKey(NULL,db,keyobj);
latencyEndMonitor(eviction_latency);
latencyAddSampleIfNeeded("eviction-del",eviction_latency);
delta -= (long long) zmalloc_used_memory();
mem_freed += delta;
server.stat_evictedkeys++;
+ signalModifiedKey(NULL,db,keyobj);
notifyKeyspaceEvent(NOTIFY_EVICTED, "evicted",
keyobj, db->id);
decrRefCount(keyobj);
diff --git a/tests/unit/maxmemory.tcl b/tests/unit/maxmemory.tcl
index 19983de9b..d4ad3ff03 100644
--- a/tests/unit/maxmemory.tcl
+++ b/tests/unit/maxmemory.tcl
@@ -260,3 +260,55 @@ start_server {tags {"maxmemory"}} {
r dbsize
} {4098}
}
+
+start_server {tags {"maxmemory"}} {
+ test {client tracking don't cause eviction feedback loop} {
+ r config set maxmemory 0
+ r config set maxmemory-policy allkeys-lru
+ r config set maxmemory-eviction-tenacity 100
+
+ # 10 clients listening on tracking messages
+ set clients {}
+ for {set j 0} {$j < 10} {incr j} {
+ lappend clients [redis_deferring_client]
+ }
+ foreach rd $clients {
+ $rd HELLO 3
+ $rd read ; # Consume the HELLO reply
+ $rd CLIENT TRACKING on
+ $rd read ; # Consume the CLIENT reply
+ }
+
+ # populate 300 keys, with long key name and short value
+ for {set j 0} {$j < 300} {incr j} {
+ set key $j[string repeat x 1000]
+ r set $key x
+
+ # for each key, enable caching for this key
+ foreach rd $clients {
+ $rd get $key
+ $rd read
+ }
+ }
+
+ # set the memory limit which will cause a few keys to be evicted
+ # we need to make sure to evict keynames of a total size of more than
+ # 16kb since the (PROTO_REPLY_CHUNK_BYTES), only after that the
+ # invalidation messages have a chance to trigger further eviction.
+ set used [s used_memory]
+ set limit [expr {$used - 40000}]
+ r config set maxmemory $limit
+
+ # make sure some eviction happened
+ set evicted [s evicted_keys]
+ if {$::verbose} { puts "evicted: $evicted" }
+ assert_range $evicted 10 50
+ foreach rd $clients {
+ $rd read ;# make sure we have some invalidation message waiting
+ $rd close
+ }
+
+ # make sure we didn't drain the database
+ assert_range [r dbsize] 200 300
+ }
+}