Mailing List Archive

Author: phk
Date: 2008-05-31 23:26:20 +0200 (Sat, 31 May 2008)
New Revision: 2645

Modified:
trunk/varnish-cache/bin/varnishd/cache.h
trunk/varnish-cache/bin/varnishd/cache_ban.c
trunk/varnish-cache/bin/varnishd/cache_hash.c
trunk/varnish-cache/include/cli.h
trunk/varnish-cache/lib/libvarnish/cli.c
Log:

Overhaul the regexp purge/ban code, with a detour around the CLI code.


CLI code:

In CLI help, don't list commands with no syntax description.

Add a CLI_HIDDEN macro to construct such entries. They are
useful as backwards compatibility entries which we do not want
to show in the help.

CLI interface to BAN (purge) code:

Get the CLI names right for purging so they are purge.FOO instead
of FOO.purge.

This means that you should use "purge.url" and "purge.hash"
instead of "url.purge" and "hash.purge".

Add compat entries for the old, and keep them through the 2.x
release series.


Add purge.list command to list purges currently in effect.
NB: This is not 100% locking safe, so don't abuse it.


BAN (purge) code:

Add reference counting and GC to bans.

Since we now have full reference counting, drop the sequence
number based soft references and go to "hard" pointer
references from the object to the purges.

Give the "ban" structure the miniobj treatment while we are
at it.

The cost of this is a lock operation to update refcounts
once all applicable bans have been checked on an object.

There is no locking cost if there is no bans to check.

Add explicit call to new BAN_DestroyObj() when objects are
destroyed to release the refcount.

When we release an object refcount in BAN_DestroyObj(),
check if we can destroy the last purge in the list also.

We only destroy one ban per BAN_DestroyObj() call, to avoid
getting stuck too long time, (tacitly assuming that we will
destroy more objects than bans.)



Modified: trunk/varnish-cache/bin/varnishd/cache.h
===================================================================
--- trunk/varnish-cache/bin/varnishd/cache.h 2008-05-31 09:34:28 UTC (rev 2644)
+++ trunk/varnish-cache/bin/varnishd/cache.h 2008-05-31 21:26:20 UTC (rev 2645)
@@ -83,6 +83,7 @@
struct esi_bit;
struct vrt_backend;
struct cli_proto;
+struct ban;

/*--------------------------------------------------------------------*/

@@ -244,7 +245,7 @@
struct ws ws_o[1];
unsigned char *vary;

- unsigned ban_seq;
+ struct ban *ban;

unsigned pass;

@@ -420,6 +421,7 @@
void AddBan(const char *, int hash);
void BAN_Init(void);
void BAN_NewObj(struct object *o);
+void BAN_DestroyObj(struct object *o);
int BAN_CheckObject(struct object *o, const char *url, const char *hash);

/* cache_center.c [CNT] */

Modified: trunk/varnish-cache/bin/varnishd/cache_ban.c
===================================================================
--- trunk/varnish-cache/bin/varnishd/cache_ban.c 2008-05-31 09:34:28 UTC (rev 2644)
+++ trunk/varnish-cache/bin/varnishd/cache_ban.c 2008-05-31 21:26:20 UTC (rev 2645)
@@ -28,7 +28,7 @@
*
* $Id$
*
- * Ban processing
+ * Ban ("purge") processing
*/

#include "config.h"
@@ -45,24 +45,33 @@
#include "cache.h"

struct ban {
+ unsigned magic;
+#define BAN_MAGIC 0x700b08ea
VTAILQ_ENTRY(ban) list;
- unsigned gen;
+ unsigned refcount;
regex_t regexp;
char *ban;
int hash;
};

-static VTAILQ_HEAD(,ban) ban_head = VTAILQ_HEAD_INITIALIZER(ban_head);
-static unsigned ban_next;
-static struct ban *ban_start;
+static VTAILQ_HEAD(banhead,ban) ban_head = VTAILQ_HEAD_INITIALIZER(ban_head);
+static MTX ban_mtx;

+/*
+ * We maintain ban_start as a pointer to the first element of the list
+ * as a separate variable from the VTAILQ, to avoid depending on the
+ * internals of the VTAILQ macros. We tacitly assume that a pointer
+ * write is always atomic in doing so.
+ */
+static struct ban * volatile ban_start;
+
void
AddBan(const char *regexp, int hash)
{
struct ban *b;
int i;

- b = calloc(sizeof *b, 1);
+ ALLOC_OBJ(b, BAN_MAGIC);
XXXAN(b);

i = regcomp(&b->regexp, regexp, REG_EXTENDED | REG_ICASE | REG_NOSUB);
@@ -71,60 +80,152 @@

(void)regerror(i, &b->regexp, buf, sizeof buf);
VSL(SLT_Debug, 0, "REGEX: <%s>", buf);
+ return;
}
b->hash = hash;
- b->gen = ++ban_next;
b->ban = strdup(regexp);
+ AN(b->ban);
+ LOCK(&ban_mtx);
VTAILQ_INSERT_HEAD(&ban_head, b, list);
ban_start = b;
+ UNLOCK(&ban_mtx);
}

void
BAN_NewObj(struct object *o)
{

- o->ban_seq = ban_next;
+ CHECK_OBJ_NOTNULL(o, OBJECT_MAGIC);
+ AZ(o->ban);
+ LOCK(&ban_mtx);
+ o->ban = ban_start;
+ ban_start->refcount++;
+ UNLOCK(&ban_mtx);
}

+void
+BAN_DestroyObj(struct object *o)
+{
+ struct ban *b;
+
+ CHECK_OBJ_NOTNULL(o, OBJECT_MAGIC);
+ if (o->ban == NULL)
+ return;
+ CHECK_OBJ_NOTNULL(o->ban, BAN_MAGIC);
+ LOCK(&ban_mtx);
+ o->ban->refcount--;
+ o->ban = NULL;
+
+ /* Check if we can purge the last ban entry */
+ b = VTAILQ_LAST(&ban_head, banhead);
+ if (b != VTAILQ_FIRST(&ban_head) && b->refcount == 0)
+ VTAILQ_REMOVE(&ban_head, b, list);
+ else
+ b = NULL;
+ UNLOCK(&ban_mtx);
+ if (b != NULL) {
+ free(b->ban);
+ regfree(&b->regexp);
+ FREE_OBJ(b);
+ }
+
+}
+
int
BAN_CheckObject(struct object *o, const char *url, const char *hash)
{
- struct ban *b, *b0;
- int i;
+ struct ban *b;
+ struct ban * volatile b0;

+ CHECK_OBJ_NOTNULL(o, OBJECT_MAGIC);
+ CHECK_OBJ_NOTNULL(o->ban, BAN_MAGIC);
+
b0 = ban_start;
- for (b = b0;
- b != NULL && b->gen > o->ban_seq;
- b = VTAILQ_NEXT(b, list)) {
- i = regexec(&b->regexp, b->hash ? hash : url, 0, NULL, 0);
- if (!i)
- return (1);
+
+ if (b0 == o->ban)
+ return (0);
+
+ /*
+ * This loop is safe without locks, because we know we hold
+ * a refcount on a ban somewhere in the list and we do not
+ * inspect the list past that ban.
+ */
+ for (b = b0; b != o->ban; b = VTAILQ_NEXT(b, list)) {
+ if (!regexec(&b->regexp, b->hash ? hash : url, 0, NULL, 0))
+ break;
}
- o->ban_seq = b0->gen;
- return (0);
+
+ LOCK(&ban_mtx);
+ o->ban->refcount--;
+ if (b == o->ban) /* not banned */
+ b0->refcount++;
+ UNLOCK(&ban_mtx);
+
+ if (b == o->ban) { /* not banned */
+ o->ban = b0;
+ return (0);
+ } else {
+ o->ban = NULL;
+ return (1);
+ }
}

+/*--------------------------------------------------------------------
+ * CLI functions to add bans
+ */
+
static void
-ccf_url_purge(struct cli *cli, const char * const *av, void *priv)
+ccf_purge_url(struct cli *cli, const char * const *av, void *priv)
{

(void)priv;
AddBan(av[2], 0);
- cli_out(cli, "PURGE %s\n", av[2]);
+ cli_out(cli, "URL_PURGE %s\n", av[2]);
}

static void
-ccf_hash_purge(struct cli *cli, const char * const *av, void *priv)
+ccf_purge_hash(struct cli *cli, const char * const *av, void *priv)
{

(void)priv;
AddBan(av[2], 1);
- cli_out(cli, "PURGE %s\n", av[2]);
+ cli_out(cli, "HASH_PURGE %s\n", av[2]);
}

+static void
+ccf_purge_list(struct cli *cli, const char * const *av, void *priv)
+{
+ struct ban * volatile b0;
+
+ (void)av;
+ (void)priv;
+ /*
+ * XXX: Strictly speaking, this loop traversal is not lock-safe
+ * XXX: because we might inspect the last ban while it gets
+ * XXX: destroyed. To properly fix this, we would need to either
+ * XXX: hold the lock over the entire loop, or grab refcounts
+ * XXX: under lock for each element of the list.
+ * XXX: We do neither, and hope for the best.
+ */
+ for (b0 = ban_start; b0 != NULL; b0 = VTAILQ_NEXT(b0, list)) {
+ if (b0->refcount == 0 && VTAILQ_NEXT(b0, list) == NULL)
+ break;
+ cli_out(cli, "%5u %s \"%s\"\n",
+ b0->refcount, b0->hash ? "hash" : "url ", b0->ban);
+ }
+}
+
static struct cli_proto ban_cmds[] = {
- { CLI_URL_PURGE, ccf_url_purge },
- { CLI_HASH_PURGE, ccf_hash_purge },
+ /*
+ * XXX: COMPAT: Retain these two entries for entire 2.x series
+ * XXX: COMPAT: to stay compatible with 1.x series syntax.
+ */
+ { CLI_HIDDEN("url.purge", 1, 1) ccf_purge_url },
+ { CLI_HIDDEN("hash.purge", 1, 1) ccf_purge_hash },
+
+ { CLI_PURGE_URL, ccf_purge_url },
+ { CLI_PURGE_HASH, ccf_purge_hash },
+ { CLI_PURGE_LIST, ccf_purge_list },
{ NULL }
};

@@ -132,6 +233,7 @@
BAN_Init(void)
{

+ MTX_INIT(&ban_mtx);
CLI_AddFuncs(PUBLIC_CLI, ban_cmds);
- AddBan("\001", 0);
+ AddBan("^\001$", 0);
}

Modified: trunk/varnish-cache/bin/varnishd/cache_hash.c
===================================================================
--- trunk/varnish-cache/bin/varnishd/cache_hash.c 2008-05-31 09:34:28 UTC (rev 2644)
+++ trunk/varnish-cache/bin/varnishd/cache_hash.c 2008-05-31 21:26:20 UTC (rev 2645)
@@ -263,6 +263,10 @@
grace_o->refcnt++;
}
UNLOCK(&oh->mtx);
+ /*
+ * XXX: This may be too early, relative to pass objects.
+ * XXX: possibly move to when we commit to have it in the cache.
+ */
BAN_NewObj(o);
return (o);
}
@@ -364,6 +368,7 @@
if (r != 0)
return;

+ BAN_DestroyObj(o);
DSL(0x40, SLT_Debug, 0, "Object %u workspace min free %u",
o->xid, WS_Free(o->ws_o));


Modified: trunk/varnish-cache/include/cli.h
===================================================================
--- trunk/varnish-cache/include/cli.h 2008-05-31 09:34:28 UTC (rev 2644)
+++ trunk/varnish-cache/include/cli.h 2008-05-31 21:26:20 UTC (rev 2645)
@@ -60,20 +60,26 @@
"\tReturns the TTL, size and checksum of the object.", \
1, 1

-#define CLI_URL_PURGE \
- "url.purge", \
- "url.purge <regexp>", \
+#define CLI_PURGE_URL \
+ "purge.url", \
+ "purge.url <regexp>", \
"\tAll objects where the urls matches regexp will be " \
"marked obsolete.", \
1, 1

-#define CLI_HASH_PURGE \
- "hash.purge", \
- "hash.purge <regexp>", \
+#define CLI_PURGE_HASH \
+ "purge.hash", \
+ "purge.hash <regexp>", \
"\tAll objects where the hash string matches regexp will be " \
"marked obsolete.", \
1, 1

+#define CLI_PURGE_LIST \
+ "purge.list", \
+ "purge.list", \
+ "\tList the active purges.", \
+ 0, 0
+
#define CLI_URL_STATUS \
"url.status", \
"url.status <url>", \
@@ -206,12 +212,15 @@
"\tClose connection", \
0, 0

-# define CLI_SERVER_STATUS \
+#define CLI_SERVER_STATUS \
"status", \
"status", \
"\tCheck status of Varnish cache process.", \
0, 0

+#define CLI_HIDDEN(foo, min_arg, max_arg) \
+ foo, NULL, NULL, min_arg, max_arg,
+
/*
* Status/return codes in the CLI protocol
*/

Modified: trunk/varnish-cache/lib/libvarnish/cli.c
===================================================================
--- trunk/varnish-cache/lib/libvarnish/cli.c 2008-05-31 09:34:28 UTC (rev 2644)
+++ trunk/varnish-cache/lib/libvarnish/cli.c 2008-05-31 21:26:20 UTC (rev 2645)
@@ -55,10 +55,13 @@

if (av[2] == NULL || *av[2] == '-') {
for (cp = priv; cp->request != NULL; cp++)
- cli_out(cli, "%s\n", cp->syntax);
+ if (cp->syntax != NULL)
+ cli_out(cli, "%s\n", cp->syntax);
return;
}
for (cp = priv; cp->request != NULL; cp++) {
+ if (cp->syntax == NULL)
+ continue;
if (!strcmp(cp->request, av[2])) {
cli_out(cli, "%s\n%s\n", cp->syntax, cp->help);
return;