Hi Poul-Henning, I was looking at this patch, and I believe there is a problem with regard to how you implemented this.
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. 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. Original patch can be seen here: https://www.varnish-cache.org/patchwork/patch/66/ -Martin On Thu, Nov 22, 2012 at 1:57 PM, Poul-Henning Kamp <[email protected]>wrote: > commit a6b98489174224c65357728036ca9f963d47ac86 > Author: Poul-Henning Kamp <[email protected]> > Date: Thu Nov 22 12:56:54 2012 +0000 > > Eliminated duplicated code for trimming the tail of the banlist. > > Original patch by: martin > > diff --git a/bin/varnishd/cache/cache_ban.c > b/bin/varnishd/cache/cache_ban.c > index 105a06f..688842b 100644 > --- a/bin/varnishd/cache/cache_ban.c > +++ b/bin/varnishd/cache/cache_ban.c > @@ -846,7 +846,7 @@ ban_CheckLast(void) > static int > ban_lurker_work(struct worker *wrk, struct vsl_log *vsl) > { > - struct ban *b, *b0, *b2; > + struct ban *b, *b0; > struct objhead *oh; > struct objcore *oc, *oc2; > struct object *o; > @@ -856,18 +856,6 @@ ban_lurker_work(struct worker *wrk, struct vsl_log > *vsl) > AN(pass & BAN_F_LURK); > AZ(pass & ~BAN_F_LURK); > > - /* First route the last ban(s) */ > - do { > - Lck_Lock(&ban_mtx); > - b2 = ban_CheckLast(); > - if (b2 != NULL) > - /* Notify stevedores */ > - STV_BanInfo(BI_DROP, b2->spec, ban_len(b2->spec)); > - Lck_Unlock(&ban_mtx); > - if (b2 != NULL) > - BAN_Free(b2); > - } while (b2 != NULL); > - > /* > * Find out if we have any bans we can do something about > * If we find any, tag them with our pass number. > @@ -1004,7 +992,7 @@ ban_lurker(struct worker *wrk, void *priv) > (void)priv; > while (1) { > > - while (cache_param->ban_lurker_sleep == 0.0) { > + do { > /* > * Ban-lurker is disabled: > * Clean the last ban, if possible, and sleep > @@ -1018,18 +1006,19 @@ ban_lurker(struct worker *wrk, void *priv) > Lck_Unlock(&ban_mtx); > if (bf != NULL) > BAN_Free(bf); > - else > - VTIM_sleep(1.0); > - } > - > - i = ban_lurker_work(wrk, &vsl); > - VSL_Flush(&vsl, 0); > - WRK_SumStat(wrk); > - if (i) { > - VTIM_sleep(cache_param->ban_lurker_sleep); > - } else { > - VTIM_sleep(1.0); > + } while (bf != NULL); > + > + if (cache_param->ban_lurker_sleep != 0.0) { > + do { > + i = ban_lurker_work(wrk, &vsl); > + VSL_Flush(&vsl, 0); > + WRK_SumStat(wrk); > + if (i) > + VTIM_sleep( > + cache_param->ban_lurker_sleep); > + } while (i); > } > + VTIM_sleep(0.609); // Random, non-magic > } > NEEDLESS_RETURN(NULL); > } > > _______________________________________________ > varnish-commit mailing list > [email protected] > https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit > -- <http://varnish-software.com>*Martin Blix Grydeland* Senior Developer | Varnish Software AS Cell: +47 21 98 92 60 We Make Websites Fly!
_______________________________________________ varnish-dev mailing list [email protected] https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
