a6b9848 Eliminated duplicated code for trimming the tail of the banlist.

[Martin Blix Grydeland]

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 <phk at varnish-cache.org>wrote:

> commit a6b98489174224c65357728036ca9f963d47ac86
> Author: Poul-Henning Kamp <phk at FreeBSD.org>
> 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
> varnish-commit at varnish-cache.org
> 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!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.varnish-cache.org/lists/pipermail/varnish-dev/attachments/20121129/08ab8a95/attachment.html>