r2645 - in trunk/varnish-cache: bin/varnishd include lib/libvarnish

phk at projects.linpro.no phk at projects.linpro.no
Sat May 31 23:26:22 CEST 2008


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;




More information about the varnish-commit mailing list