Rewrite the ban-lurker.

Use a mark&mark strategy to test all obj.* bans even if there are
req.* bans in the way.

bin/varnishd/cache_ban.c

@@ -65,12 +65,14 @@ struct ban {
 	unsigned		flags;
 #define BAN_F_GONE		(1 << 0)
 #define BAN_F_REQ		(1 << 2)
-#define BAN_F_LURK		(3 << 3)	/* ban-lurker-color */
+#define BAN_F_LURK		(3 << 6)	/* ban-lurker-color */
 	VTAILQ_HEAD(,objcore)	objcore;
 	struct vsb		*vsb;
 	uint8_t			*spec;
 };
 
+#define LURK_SHIFT 6
+
 struct ban_test {
 	uint8_t			arg1;
 	const char		*arg1_spec;
@@ -401,6 +403,7 @@ BAN_Insert(struct ban *b)
 	/* Hunt down duplicates, and mark them as gone */
 	bi = b;
 	pcount = 0;
+	Lck_Lock(&ban_mtx);
 	while(bi != be) {
 		bi = VTAILQ_NEXT(bi, list);
 		if (bi->flags & BAN_F_GONE)
@@ -409,9 +412,9 @@ BAN_Insert(struct ban *b)
 		if (memcmp(b->spec + 8, bi->spec + 8, ln - 8))
 			continue;
 		bi->flags |= BAN_F_GONE;
+		VSC_C_main->n_ban_gone++;
 		pcount++;
 	}
-	Lck_Lock(&ban_mtx);
 	be->refcount--;
 	VSC_C_main->n_ban_dups += pcount;
 	Lck_Unlock(&ban_mtx);
@@ -627,7 +630,13 @@ ban_evaluate(const uint8_t *bs, const struct http *objhttp,
 }
 
 /*--------------------------------------------------------------------
- * Check an object any fresh bans
+ * Check an object against all applicable bans
+ *
+ * Return:
+ *	-1 not all bans checked, but none of the checked matched
+ *		Only if !has_req
+ *	0 No bans matched, object moved to ban_start.
+ *	1 Ban matched, object removed from ban list.
  */
 
 static int
@@ -661,6 +670,12 @@ ban_check_object(struct object *o, const struct sess *sp, int has_req)
 		CHECK_OBJ_NOTNULL(b, BAN_MAGIC);
 		if (b->flags & BAN_F_GONE)
 			continue;
+		if ((b->flags & BAN_F_LURK) &&
+		    (b->flags & BAN_F_LURK) == (oc->flags & OC_F_LURK)) {
+			AZ(b->flags & BAN_F_REQ);
+			/* Lurker already tested this */
+			continue;
+		}
 		if (!has_req && (b->flags & BAN_F_REQ)) {
 			/*
 			 * We cannot test this one, but there might
@@ -671,23 +686,27 @@ ban_check_object(struct object *o, const struct sess *sp, int has_req)
 			break;
 	}
 
+	Lck_Lock(&ban_mtx);
+	VSC_C_main->n_ban_obj_test++;
+	VSC_C_main->n_ban_re_test += tests;
+
 	if (b == oc->ban && skipped > 0) {
+		AZ(has_req);
+		Lck_Unlock(&ban_mtx);
 		/*
 		 * Not banned, but some tests were skipped, so we cannot know
 		 * for certain that it cannot be, so we just have to give up.
 		 */
-		return (0);
+		return (-1);
 	}
 
-	Lck_Lock(&ban_mtx);
 	oc->ban->refcount--;
 	VTAILQ_REMOVE(&oc->ban->objcore, oc, ban_list);
 	if (b == oc->ban) {	/* not banned */
+		b->flags &= ~BAN_F_LURK;
 		VTAILQ_INSERT_TAIL(&b0->objcore, oc, ban_list);
 		b0->refcount++;
 	}
-	VSC_C_main->n_ban_obj_test++;
-	VSC_C_main->n_ban_re_test += tests;
 	Lck_Unlock(&ban_mtx);
 
 	if (b == oc->ban) {	/* not banned */
@@ -709,7 +728,7 @@ int
 BAN_CheckObject(struct object *o, const struct sess *sp)
 {
 
-	return (ban_check_object(o, sp, 1));
+	return (ban_check_object(o, sp, 1) > 0);
 }
 
 static struct ban *
@@ -720,6 +739,8 @@ ban_CheckLast(void)
 	Lck_AssertHeld(&ban_mtx);
 	b = VTAILQ_LAST(&ban_head, banhead_s);
 	if (b != VTAILQ_FIRST(&ban_head) && b->refcount == 0) {
+		if (b->flags & BAN_F_GONE)
+			VSC_C_main->n_ban_gone--;
 		VSC_C_main->n_ban--;
 		VSC_C_main->n_ban_retire++;
 		VTAILQ_REMOVE(&ban_head, b, list);
@@ -730,84 +751,144 @@ ban_CheckLast(void)
 }
 
 /*--------------------------------------------------------------------
- * Ban tail lurker thread
+ * Ban lurker thread
  */
 
 static int
-ban_lurker_work(const struct sess *sp)
+ban_lurker_work(const struct sess *sp, unsigned pass)
 {
-	struct ban *b, *bf;
+	struct ban *b, *b0, *b2;
 	struct objhead *oh;
 	struct objcore *oc, *oc2;
 	struct object *o;
 	int i;
 
-	Lck_Lock(&ban_mtx);
+	AN(pass & BAN_F_LURK);
+	AZ(pass & ~BAN_F_LURK);
 
-	/* First try to route the last ban */
-	bf = ban_CheckLast();
-	if (bf != NULL) {
+	/* First route the last ban(s) */
+	do {
+		Lck_Lock(&ban_mtx);
+		b2 = ban_CheckLast();
 		Lck_Unlock(&ban_mtx);
-		BAN_Free(bf);
-		return (0);
-	}
+		if (b2 != NULL)
+			BAN_Free(b2);
+	} while (b2 != NULL);
 
-	/* Find the last ban give up, if we have only one */
-	b = VTAILQ_LAST(&ban_head, banhead_s);
-	if (b == ban_start) {
-		Lck_Unlock(&ban_mtx);
-		return (0);
+	/*
+	 * Find out if we have any bans we can do something about
+	 * If we find any, tag them with our pass number.
+	 */
+	i = 0;
+	b0 = NULL;
+	VTAILQ_FOREACH(b, &ban_head, list) {
+		if (b->flags & BAN_F_GONE)
+			continue;
+		if (b->flags & BAN_F_REQ)
+			continue;
+		if (b == VTAILQ_LAST(&ban_head, banhead_s))
+			continue;
+		if (b0 == NULL)
+			b0 = b;
+		i++;
+		b->flags &= ~BAN_F_LURK;
+		b->flags |= pass;
 	}
-
-	/* Find the first object on it, if any */
-	oc = VTAILQ_FIRST(&b->objcore);
-	if (oc == NULL) {
-		Lck_Unlock(&ban_mtx);
+	if (params->diag_bitmap & 0x80000)
+		VSL(SLT_Debug, 0, "lurker: %d actionable bans", i);
+	if (i == 0)
 		return (0);
-	}
 
-	/* Try to lock the objhead */
-	oh = oc->objhead;
-	CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
-	if (Lck_Trylock(&oh->mtx)) {
+	VTAILQ_FOREACH_REVERSE(b, &ban_head, banhead_s, list) {
+		if (params->diag_bitmap & 0x80000)
+			VSL(SLT_Debug, 0, "lurker doing %f %d",
+			    ban_time(b->spec), b->refcount);
+		while (1) {
+			Lck_Lock(&ban_mtx);
+			oc = VTAILQ_FIRST(&b->objcore);
+			if (oc == NULL)
+				break;
+			CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
+			if (params->diag_bitmap & 0x80000)
+				VSL(SLT_Debug, 0, "test: %p %d %d",
+				    oc, oc->flags & OC_F_LURK, pass);
+			if ((oc->flags & OC_F_LURK) == pass)
+				break;
+			oh = oc->objhead;
+			CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
+			if (Lck_Trylock(&oh->mtx)) {
+				Lck_Unlock(&ban_mtx);
+				TIM_sleep(params->ban_lurker_sleep);
+				continue;
+			}
+			/*
+			 * See if the objcore is still on the objhead since
+			 * we race against HSH_Deref() which comes in the
+			 * opposite locking order.
+			 */
+			VTAILQ_FOREACH(oc2, &oh->objcs, list)
+				if (oc == oc2)
+					break;
+			if (oc2 == NULL) {
+				Lck_Unlock(&oh->mtx);
+				Lck_Unlock(&ban_mtx);
+				TIM_sleep(params->ban_lurker_sleep);
+				continue;
+			}
+			/*
+			 * Grab a reference to the OC and we can let go of
+			 * the BAN mutex
+			 */
+			AN(oc->refcnt);
+			oc->refcnt++;
+			oc->flags &= ~OC_F_LURK;
+			Lck_Unlock(&ban_mtx);
+			/*
+			 * Get the object and check it against all relevant bans
+			 */
+			o = oc_getobj(sp->wrk, oc);
+			i = ban_check_object(o, sp, 0);
+			if (params->diag_bitmap & 0x80000)
+				VSL(SLT_Debug, 0, "lurker got: %p %d",
+				    oc, i);
+			if (i == -1) {
+				/* Not banned, not moved */
+				oc->flags |= pass;
+				Lck_Lock(&ban_mtx);
+				VTAILQ_REMOVE(&b->objcore, oc, ban_list);
+				VTAILQ_INSERT_TAIL(&b->objcore, oc, ban_list);
+				Lck_Unlock(&ban_mtx);
+			}
+			Lck_Unlock(&oh->mtx);
+			if (params->diag_bitmap & 0x80000)
+				VSL(SLT_Debug, 0, "lurker done: %p %d %d",
+				    oc, oc->flags & OC_F_LURK, pass);
+			(void)HSH_Deref(sp->wrk, NULL, &o);
+			TIM_sleep(params->ban_lurker_sleep);
+		}
+		Lck_AssertHeld(&ban_mtx);
+		if (!(b->flags & BAN_F_REQ)) {
+			if (!(b->flags & BAN_F_GONE)) {
+				b->flags |= BAN_F_GONE;
+				VSC_C_main->n_ban_gone++;
+			}
+			if (params->diag_bitmap & 0x80000)
+				VSL(SLT_Debug, 0, "lurker BAN %f now gone",
+				    ban_time(b->spec));
+		}
 		Lck_Unlock(&ban_mtx);
-		return (0);
-	}
-
-	/*
-	 * See if the objcore is still on the objhead since we race against
-	 * HSH_Deref() which comes in the opposite locking order.
-	 */
-	VTAILQ_FOREACH(oc2, &oh->objcs, list)
-		if (oc == oc2)
+		TIM_sleep(params->ban_lurker_sleep);
+		if (b == b0)
 			break;
-	if (oc2 == NULL) {
-		Lck_Unlock(&oh->mtx);
-		Lck_Unlock(&ban_mtx);
-		return (0);
 	}
-	/*
-	 * Grab a reference to the OC and we can let go of the BAN mutex
-	 */
-	AN(oc->refcnt);
-	oc->refcnt++;
-	Lck_Unlock(&ban_mtx);
-
-	/*
-	 * Get the object and check it against all relevant bans
-	 */
-	o = oc_getobj(sp->wrk, oc);
-	i = ban_check_object(o, sp, 0);
-	Lck_Unlock(&oh->mtx);
-	WSP(sp, SLT_Debug, "lurker: %p %g %d", oc, o->exp.ttl, i);
-	(void)HSH_Deref(sp->wrk, NULL, &o);
-	return (i);
+	return (1);
 }
 
 static void * __match_proto__(bgthread_t)
 ban_lurker(struct sess *sp, void *priv)
 {
 	struct ban *bf;
+	unsigned pass = (1 << LURK_SHIFT);
 
 	int i = 0;
 	(void)priv;
@@ -827,14 +908,18 @@ ban_lurker(struct sess *sp, void *priv)
 				TIM_sleep(1.0);
 		}
 
-		i = ban_lurker_work(sp);
+		i = ban_lurker_work(sp, pass);
 		WSL_Flush(sp->wrk, 0);
 		WRK_SumStat(sp->wrk);
-
-		if (i != 0)
+		if (i) {
+			pass += (1 << LURK_SHIFT);
+			pass &= BAN_F_LURK;
+			if (pass == 0)
+				pass += (1 << LURK_SHIFT);
 			TIM_sleep(params->ban_lurker_sleep);
-		else
+		} else {
 			TIM_sleep(1.0);
+		}
 	}
 	NEEDLESS_RETURN(NULL);
 }
@@ -948,13 +1033,20 @@ ccf_ban_list(struct cli *cli, const char * const *av, void *priv)
 
 	VCLI_Out(cli, "Present bans:\n");
 	VTAILQ_FOREACH(b, &ban_head, list) {
-		if (b == bl)
+		if (b == bl && !(params->diag_bitmap & 0x80000))
 			break;
 		VCLI_Out(cli, "%10.6f %5u%s\t", ban_time(b->spec),
 		    bl == b ? b->refcount - 1 : b->refcount,
 		    b->flags & BAN_F_GONE ? "G" : " ");
 		ban_render(cli, b->spec);
 		VCLI_Out(cli, "\n");
+		if (params->diag_bitmap & 0x80000) {
+			Lck_Lock(&ban_mtx);
+			struct objcore *oc;
+			VTAILQ_FOREACH(oc, &b->objcore, ban_list)
+				VCLI_Out(cli, "     %p\n", oc);
+			Lck_Unlock(&ban_mtx);
+		}
 	}
 
 	BAN_TailDeref(&bl);
@@ -973,10 +1065,14 @@ BAN_Init(void)
 
 	Lck_New(&ban_mtx, lck_ban);
 	CLI_AddFuncs(ban_cmds);
+	assert(BAN_F_LURK == OC_F_LURK);
+	AN((1 << LURK_SHIFT) & BAN_F_LURK);
+	AN((2 << LURK_SHIFT) & BAN_F_LURK);
 
 	ban_magic = BAN_New();
 	AN(ban_magic);
 	ban_magic->flags |= BAN_F_GONE;
+	VSC_C_main->n_ban_gone++;
 	BAN_Insert(ban_magic);
 	WRK_BgThread(&ban_thread, "ban-lurker", ban_lurker, NULL);
 }

bin/varnishd/mgt_param.c

@@ -833,6 +833,7 @@ static const struct parspec input_parspec[] = {
 		"  0x00010000 - synchronize shmlog.\n"
 		"  0x00020000 - synchronous start of persistence.\n"
 		"  0x00040000 - release VCL early.\n"
+		"  0x00080000 - ban-lurker debugging.\n"
 		"  0x80000000 - do edge-detection on digest.\n"
 		"Use 0x notation and do the bitor in your head :-)\n",
 		0,

bin/varnishtest/tests/c00049.vtc

+varnishtest "ban lurker test"
+
+
+server s1 {
+	rxreq
+	expect req.url == "/alpha"
+	txresp -hdr "Foo: /alpha"
+
+	rxreq
+	expect req.url == "/beta"
+	txresp -hdr "Foo: /beta"
+
+	rxreq
+	expect req.url == "/gamma"
+	txresp -hdr "Foo: /gamma"
+
+	rxreq
+	expect req.url == "/delta"
+	txresp -hdr "Foo: /delta"
+
+	rxreq
+	expect req.url == "/alpha"
+	txresp -hdr "Foo: /alpha2"
+
+	rxreq
+	expect req.url == "/beta"
+	txresp -hdr "Foo: /beta2"
+
+	rxreq
+	expect req.url == "/delta"
+	txresp -hdr "Foo: /delta2"
+
+} -start
+
+varnish v1 -vcl+backend {
+} -start
+
+varnish v1 -cliok "param.set ban_lurker_sleep 0"
+varnish v1 -cliok "param.set diag_bitmap 0x80000"
+
+varnish v1 -cliok "ban.list"
+
+client c1 {
+	txreq -url "/alpha"
+	rxresp
+	expect resp.http.foo == /alpha
+} -run
+
+delay 0.1
+varnish v1 -cliok "ban req.url == /alpha"
+varnish v1 -cliok "ban.list"
+varnish v1 -expect n_ban == 2
+varnish v1 -expect n_ban_gone == 1
+
+client c1 {
+	txreq -url "/beta"
+	rxresp
+	expect resp.http.foo == /beta
+} -run
+
+delay 0.1
+varnish v1 -cliok "ban obj.http.foo == /beta"
+varnish v1 -cliok "ban.list"
+varnish v1 -expect n_ban == 3
+
+client c1 {
+	txreq -url "/gamma"
+	rxresp
+	expect resp.http.foo == /gamma
+} -run
+
+delay 0.1
+varnish v1 -cliok "ban obj.http.foo == /gamma"
+varnish v1 -cliok "ban.list"
+varnish v1 -expect n_ban == 4
+
+client c1 {
+	txreq -url "/delta"
+	rxresp
+	expect resp.http.foo == /delta
+} -run
+
+delay 0.1
+varnish v1 -cliok "ban req.url == /delta"
+
+varnish v1 -expect n_ban_gone == 1
+varnish v1 -cliok "ban obj.http.foo == /gamma"
+# Dup-check should have added one
+varnish v1 -expect n_ban_gone == 2
+
+varnish v1 -cliok "ban req.url == /epsilon"
+varnish v1 -cliok "ban.list"
+varnish v1 -expect n_ban == 7
+varnish v1 -expect n_ban_gone == 2
+
+varnish v1 -cliok "param.set ban_lurker_sleep .01"
+delay 1
+varnish v1 -cliok "param.set ban_lurker_sleep .00"
+varnish v1 -cliok "ban.list"
+varnish v1 -expect n_ban == 7
+varnish v1 -expect n_ban_gone == 4
+
+client c1 {
+	txreq -url "/alpha"
+	rxresp
+	expect resp.http.foo == /alpha2
+} -run
+
+delay 1
+varnish v1 -cliok "ban.list"
+varnish v1 -expect n_ban == 4
+
+client c1 {
+	txreq -url "/beta"
+	rxresp
+	expect resp.http.foo == /beta2
+} -run
+
+varnish v1 -cliok "ban.list"
+
+
+client c1 {
+	txreq -url "/delta"
+	rxresp
+	expect resp.http.foo == /delta2
+} -run
+
+delay 1
+varnish v1 -cliok "ban.list"
+varnish v1 -expect n_ban == 1
+varnish v1 -expect n_ban_gone == 0