From b458b2999b749950c5ea9468cfa5ab74782848c3 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 29 Jul 2021 12:11:29 +0300 Subject: Fix LRU blue moon bug in RESTORE, RDB loading, module API (#9279) The `lru_clock` and `lru` bits in `robj` save the least significant 24 bits of the unixtime (seconds since 1/1/1970), and wrap around every 194 days. The `objectSetLRUOrLFU` function, which is used in RESTORE with IDLETIME argument, and also in replica or master loading an RDB that contains LRU, and by a module API had a bug that's triggered when that happens. The scenario was that the idle time that came from the user, let's say RESTORE command is about 1000 seconds (e.g. in the `RESTORE can set LRU` test we have), and the current `lru_clock` just wrapped around and is less than 1000 (i.e. a period of 1000 seconds once in some 6 months), the expression in that function would produce a negative value and the code (and comment) specified that the best way to solve that is push the idle time backwards into the past by 3 months. i.e. an idle time of 3 months instead of 1000 seconds. instead, the right thing to do is to unwrap it, and put it near LRU_CLOCK_MAX. since now `lru_clock` is smaller than `obj->lru` it will be unwrapped again by `estimateObjectIdleTime`. bug was introduced by 052e03495f, but the code before it also seemed wrong. --- src/object.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'src/object.c') diff --git a/src/object.c b/src/object.c index 59ae47f2a..05831156c 100644 --- a/src/object.c +++ b/src/object.c @@ -1201,13 +1201,13 @@ int objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle, * below statement will expand to lru_idle*1000/1000. */ lru_idle = lru_idle*lru_multiplier/LRU_CLOCK_RESOLUTION; long lru_abs = lru_clock - lru_idle; /* Absolute access time. */ - /* If the LRU field underflow (since LRU it is a wrapping - * clock), the best we can do is to provide a large enough LRU - * that is half-way in the circular LRU clock we use: this way - * the computed idle time for this object will stay high for quite - * some time. */ + /* If the LRU field underflows (since lru_clock is a wrapping clock), + * we need to make it positive again. This be handled by the unwrapping + * code in estimateObjectIdleTime. I.e. imagine a day when lru_clock + * wrap arounds (happens once in some 6 months), and becomes a low + * value, like 10, an lru_idle of 1000 should be near LRU_CLOCK_MAX. */ if (lru_abs < 0) - lru_abs = (lru_clock+(LRU_CLOCK_MAX/2)) % LRU_CLOCK_MAX; + lru_abs += LRU_CLOCK_MAX; val->lru = lru_abs; return 1; } -- cgit v1.2.1