[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