Closed AlveElde closed 3 months ago
Can't this be done with just a few lines of changes to EXP_Rearm()
to change the logging and a wrapper to avoid the ttl reset? I do see the point, but the patch includes refactoring which I do not see the motivation for and code duplication which I think we should avoid.
I think one goal is precisely to not prevent EXP_Rearm()
from extending the life of an object while ensuring that a soft purge doesn't.
Changing the behavior of EXP_Rearm()
is going to break several VMODs, so I figured that providing a new function was the way to go.
Regarding code duplication, I tried to strike a middleground between code reuse and readability. But maybe there is a better way to structure it that I'm not seeing?
How would changing the logging break VMODs?
I did not mean to suggest to change EXP_Rearm()
other than the logging. I suggested adding EXP_Reduce()
as a trivial wrapper of EXP_Rearm()
.
But maybe there is a better way to structure it that I'm not seeing?
I had something like this in mind:
diff --git a/bin/varnishd/cache/cache_expire.c b/bin/varnishd/cache/cache_expire.c
index 2b27371ba..2d8b34de4 100644
--- a/bin/varnishd/cache/cache_expire.c
+++ b/bin/varnishd/cache/cache_expire.c
@@ -262,6 +262,23 @@ EXP_Rearm(struct objcore *oc, vtim_real now,
oc->timer_when, when, oc->flags);
}
+
+void
+EXP_Reduce(struct objcore *oc, vtim_real now,
+ vtim_real ttl, vtim_real grace, vtim_real keep)
+{
+
+ CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
+
+ if (!isnan(ttl)) {
+ ttl = now + ttl - oc->t_origin;
+ if (ttl >= oc->ttl)
+ ttl = nan("");
+ }
+
+ EXP_Rearm(oc, now, ttl, grace, keep);
+}
+
/*--------------------------------------------------------------------
* Handle stuff in the inbox
*/
Ah, now I get what you mean, that is a clever solution I had not considered. I will reattempt the patch series based on your example.
also approved by @bsdphk during bugwash
Hi, Is it possible to report this fix on 7.5 branch and release a minor version ? We have a similar behavior when expiring objects that could look like this.
This PR resolves an issue where softpurging an object would reset its expiry timer to the start of the objects grace period. Repeated softpurges of the same object would keep it away from expiry indefinetly, and softpurging an object in keep would bring it back into grace.
The effects of the current softpurge implementation are most severe when the cache contains a high number of objects that are regularly hit by key-based invalidation. This can result in a growing number of objects in cache, increasing load on the expiry thread mailbox, and contention on the
exp
mutex. Grace hits on stale objects long past their indended lifetime can also occur.This patch series introduces
EXP_Reduce()
as an alternative toEXP_Rearm()
for softpurging applications.purge.soft()
has been changed to use the new function, and the VCC has been updated with the following: