Hi Poul-Henning,<div><br></div><div>I was looking at this patch, and I believe there is a problem with regard to how you implemented this.</div><div><br></div><div>I believe the loop in ban_lurker() around successful ban_lurker_work calls when the ban_lurker is enabled, has the potential of running for much longer than intended. So long as there is a new ban added at an interval shorter than 2*ban_lurker_sleep, this loop will continue running, and then no tail bans will ever be dropped.</div>
<div><br></div><div>So I believe the structure in the original patch is better, where only the outer loop exists, and any droppable bans are removed on every loop. The return value of ban_lurker_work will then only influence for how long to sleep before looping again. </div>
<div><br></div><div>Original patch can be seen here: <a href="https://www.varnish-cache.org/patchwork/patch/66/">https://www.varnish-cache.org/patchwork/patch/66/</a></div><div><br></div><div>-Martin<br><br><div class="gmail_quote">
On Thu, Nov 22, 2012 at 1:57 PM, Poul-Henning Kamp <span dir="ltr"><<a href="mailto:phk@varnish-cache.org" target="_blank">phk@varnish-cache.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
commit a6b98489174224c65357728036ca9f963d47ac86<br>
Author: Poul-Henning Kamp <phk@FreeBSD.org><br>
Date: Thu Nov 22 12:56:54 2012 +0000<br>
<br>
Eliminated duplicated code for trimming the tail of the banlist.<br>
<br>
Original patch by: martin<br>
<br>
diff --git a/bin/varnishd/cache/cache_ban.c b/bin/varnishd/cache/cache_ban.c<br>
index 105a06f..688842b 100644<br>
--- a/bin/varnishd/cache/cache_ban.c<br>
+++ b/bin/varnishd/cache/cache_ban.c<br>
@@ -846,7 +846,7 @@ ban_CheckLast(void)<br>
static int<br>
ban_lurker_work(struct worker *wrk, struct vsl_log *vsl)<br>
{<br>
- struct ban *b, *b0, *b2;<br>
+ struct ban *b, *b0;<br>
struct objhead *oh;<br>
struct objcore *oc, *oc2;<br>
struct object *o;<br>
@@ -856,18 +856,6 @@ ban_lurker_work(struct worker *wrk, struct vsl_log *vsl)<br>
AN(pass & BAN_F_LURK);<br>
AZ(pass & ~BAN_F_LURK);<br>
<br>
- /* First route the last ban(s) */<br>
- do {<br>
- Lck_Lock(&ban_mtx);<br>
- b2 = ban_CheckLast();<br>
- if (b2 != NULL)<br>
- /* Notify stevedores */<br>
- STV_BanInfo(BI_DROP, b2->spec, ban_len(b2->spec));<br>
- Lck_Unlock(&ban_mtx);<br>
- if (b2 != NULL)<br>
- BAN_Free(b2);<br>
- } while (b2 != NULL);<br>
-<br>
/*<br>
* Find out if we have any bans we can do something about<br>
* If we find any, tag them with our pass number.<br>
@@ -1004,7 +992,7 @@ ban_lurker(struct worker *wrk, void *priv)<br>
(void)priv;<br>
while (1) {<br>
<br>
- while (cache_param->ban_lurker_sleep == 0.0) {<br>
+ do {<br>
/*<br>
* Ban-lurker is disabled:<br>
* Clean the last ban, if possible, and sleep<br>
@@ -1018,18 +1006,19 @@ ban_lurker(struct worker *wrk, void *priv)<br>
Lck_Unlock(&ban_mtx);<br>
if (bf != NULL)<br>
BAN_Free(bf);<br>
- else<br>
- VTIM_sleep(1.0);<br>
- }<br>
-<br>
- i = ban_lurker_work(wrk, &vsl);<br>
- VSL_Flush(&vsl, 0);<br>
- WRK_SumStat(wrk);<br>
- if (i) {<br>
- VTIM_sleep(cache_param->ban_lurker_sleep);<br>
- } else {<br>
- VTIM_sleep(1.0);<br>
+ } while (bf != NULL);<br>
+<br>
+ if (cache_param->ban_lurker_sleep != 0.0) {<br>
+ do {<br>
+ i = ban_lurker_work(wrk, &vsl);<br>
+ VSL_Flush(&vsl, 0);<br>
+ WRK_SumStat(wrk);<br>
+ if (i)<br>
+ VTIM_sleep(<br>
+ cache_param->ban_lurker_sleep);<br>
+ } while (i);<br>
}<br>
+ VTIM_sleep(0.609); // Random, non-magic<br>
}<br>
NEEDLESS_RETURN(NULL);<br>
}<br>
<br>
_______________________________________________<br>
varnish-commit mailing list<br>
<a href="mailto:varnish-commit@varnish-cache.org">varnish-commit@varnish-cache.org</a><br>
<a href="https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit" target="_blank">https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit</a><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div><table border="0" cellpadding="0" cellspacing="0" style="font-size:12px;line-height:1.5em;font-family:'Helvetica Neue',Arial,sans-serif;color:rgb(102,102,102);width:550px;border-top-width:1px;border-top-style:solid;border-top-color:rgb(238,238,238);border-bottom-width:1px;border-bottom-style:solid;border-bottom-color:rgb(238,238,238);margin-top:20px;padding-top:5px;padding-bottom:5px">
<tbody><tr><td width="100"><a href="http://varnish-software.com" target="_blank"><img src="http://www.varnish-software.com/static/media/logo-email.png"></a><span></span><span></span></td><td><strong style="font-size:14px;color:rgb(34,34,34)">Martin Blix Grydeland</strong><br>
Senior Developer | Varnish Software AS<br>Cell: +47 21 98 92 60<br><span style="font-weight:bold">We Make Websites Fly!</span></td></tr></tbody></table></div><br>
</div>