[master] 991d8d1a1 properly maintain the obans list when pruning the ban list tail
Nils Goroll
nils.goroll at uplex.de
Fri May 31 16:27:08 UTC 2019
commit 991d8d1a1685536fe1010f124c6a6c0e67d14622
Author: Nils Goroll <nils.goroll at uplex.de>
Date: Fri May 31 15:51:08 2019 +0200
properly maintain the obans list when pruning the ban list tail
background: When the ban lurker has finished working the bottom of the
ban list, conceptually we mark all bans it has evaluated as completed
and then remove the tail of the ban list which has no references any
more.
Yet, for efficiency, we first remove the tail and then mark only those
bans completed, which we did not remove. Doing so depends on knowing
where in the (obans) list of bans to be completed is the new tail of
the bans list after pruning.
5dd54f8390739c62c201e62c36eb515b1e03c2ee was intended to solve this,
but the fix was incomplete (and also unnecessarily complicated): For
example when a duplicate ban was issued, ban_lurker_test_ban() could
remove a ban from the obans list which later happens to become the new
ban tail.
We now - hopefully - solve the problem for real by properly cleaning
the obans list when we prune the ban list.
Fixes #3006
Fixes #2779
Fixes #2556 for real (5dd54f8390739c62c201e62c36eb515b1e03c2ee was
incomplete)
diff --git a/bin/varnishd/cache/cache_ban_lurker.c b/bin/varnishd/cache/cache_ban_lurker.c
index 7be660cb8..2d7f7f03f 100644
--- a/bin/varnishd/cache/cache_ban_lurker.c
+++ b/bin/varnishd/cache/cache_ban_lurker.c
@@ -58,19 +58,18 @@ ban_kick_lurker(void)
* still referenced. For already completed bans, we update statistics
* accordingly, but otherwise just skip the completion step and remove directly
*
- * return 1 if we removed the victim, 0 otherwise
+ * if an obans list is passed, we clean its tail as well
*/
-static int
-ban_cleantail(const struct ban *victim)
+static void
+ban_cleantail(struct banhead_s *obans)
{
struct ban *b, *bt;
struct banhead_s freelist = VTAILQ_HEAD_INITIALIZER(freelist);
- int r = 0;
/* handle the zero-length tail unprotected */
if (VTAILQ_LAST(&ban_head, banhead_s) == VTAILQ_FIRST(&ban_head))
- return (r);
+ return;
Lck_Lock(&ban_mtx);
do {
@@ -99,13 +98,27 @@ ban_cleantail(const struct ban *victim)
Lck_Unlock(&ban_mtx);
- VTAILQ_FOREACH_SAFE(b, &freelist, list, bt) {
- if (b == victim)
- r = 1;
- BAN_Free(b);
+ /* oban order is head to tail, freelist tail to head */
+ if (obans != NULL)
+ bt = VTAILQ_LAST(obans, banhead_s);
+ else
+ bt = NULL;
+
+ if (bt != NULL) {
+ VTAILQ_FOREACH(b, &freelist, list) {
+ if (b != bt)
+ continue;
+ VTAILQ_REMOVE(obans, b, l_list);
+ bt = VTAILQ_LAST(obans, banhead_s);
+ if (bt == NULL)
+ break;
+ }
}
- return (r);
+ VTAILQ_FOREACH_SAFE(b, &freelist, list, bt)
+ BAN_Free(b);
+
+ return;
}
/*--------------------------------------------------------------------
@@ -343,7 +356,7 @@ ban_lurker_work(struct worker *wrk, struct vsl_log *vsl)
dt = 49.62; // Random, non-magic
if (cache_param->ban_lurker_sleep == 0) {
- (void)ban_cleantail(NULL);
+ ban_cleantail(NULL);
return (dt);
}
if (cache_param->ban_cutoff > 0)
@@ -379,42 +392,18 @@ ban_lurker_work(struct worker *wrk, struct vsl_log *vsl)
}
/*
- * conceptually, all obans are now completed. Remove the tail. If it
- * contained the first oban, all obans were on the tail and we're
- * done.
+ * conceptually, all obans are now completed. Remove the tail.
+ * If any bans to be completed remain after the tail is cut,
+ * mark them completed
*/
- if (ban_cleantail(VTAILQ_FIRST(&obans)))
- return (dt);
+ ban_cleantail(&obans);
if (VTAILQ_FIRST(&obans) == NULL)
return (dt);
- /*
- * Mark remaining bans completed: the tail of the obans list is now
- * removed, but iterating over it is safe until we hit the new ban list
- * tail
- *
- * bans at the tail of the list may have been completed by other means
- * and, consequently, may have been removed from obans, so we skip all
- * already completed bans at the tail.
- *
- * While traversing the ban list backwards, we check if we pass by the
- * first oban, in which case we're done.
- */
- bd = VTAILQ_LAST(&ban_head, banhead_s);
- while (bd->flags & BANS_FLAG_COMPLETED) {
- if (bd == VTAILQ_FIRST(&ban_head) ||
- bd == VTAILQ_FIRST(&obans))
- return (dt);
- bd = VTAILQ_PREV(bd, banhead_s, list);
- }
-
Lck_Lock(&ban_mtx);
- VTAILQ_FOREACH(b, &obans, l_list) {
+ VTAILQ_FOREACH(b, &obans, l_list)
ban_mark_completed(b);
- if (b == bd)
- break;
- }
Lck_Unlock(&ban_mtx);
return (dt);
}
diff --git a/bin/varnishtest/tests/r03006.vtc b/bin/varnishtest/tests/r03006.vtc
new file mode 100644
index 000000000..9a48e7536
--- /dev/null
+++ b/bin/varnishtest/tests/r03006.vtc
@@ -0,0 +1,54 @@
+varnishtest "test ban lurker destination being completed by dup ban"
+
+server s1 -repeat 4 -keepalive {
+ rxreq
+ txresp
+} -start
+
+varnish v1 -vcl+backend {} -start
+
+varnish v1 -cliok "param.set ban_lurker_age 99"
+
+# this ban becomes the pruned tail
+varnish v1 -cliok "ban obj.http.t == 1"
+client c1 {
+ txreq -url /1
+ rxresp
+} -run
+
+# this ban becomes the new tail
+varnish v1 -cliok "ban obj.http.t == 2"
+client c1 {
+ txreq -url /2
+ rxresp
+} -run
+
+# req ban to define where the tail goes (at t == 2)
+varnish v1 -cliok "ban req.http.barrier == here"
+client c1 {
+ txreq -url /3
+ rxresp
+} -run
+
+# dup ban to trigger #3006
+varnish v1 -cliok "ban obj.http.t == 2"
+client c1 {
+ txreq -url /4
+ rxresp
+} -run
+
+varnish v1 -cliok "ban.list"
+
+varnish v1 -cliok "param.set ban_lurker_age 0.1"
+
+varnish v1 -cliok "ban.list"
+
+delay 2
+
+varnish v1 -expect bans == 3
+varnish v1 -expect bans_completed == 2
+
+varnish v1 -cliok "ban.list"
+
+varnish v1 -expect bans == 3
+varnish v1 -expect bans_completed == 2
More information about the varnish-commit
mailing list