[PATCH 02/15] Add a BAN_Shutdown() routine that is called before STV_Close(), and makes sure that the ban lists will not change until we exit.
Martin Blix Grydeland
martin at varnish-software.com
Wed Nov 7 12:32:05 CET 2012
This is to give persistent stevedores a chance to clean up their ban
lists without having to worry about the lists changing under them
(normally the ban callbacks are called under the ban mutex, but not so
during exit).
Also remove the duplicate drop tail bans code. That is now only done
in the main thread loop.
---
bin/varnishd/cache/cache.h | 3 +-
bin/varnishd/cache/cache_ban.c | 112 ++++++++++++++++++++++++++-------------
bin/varnishd/cache/cache_main.c | 1 +
bin/varnishd/cache/cache_vrt.c | 8 +--
4 files changed, 81 insertions(+), 43 deletions(-)
diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
index 6d4fe86..8a73aa7 100644
--- a/bin/varnishd/cache/cache.h
+++ b/bin/varnishd/cache/cache.h
@@ -746,8 +746,9 @@ struct ban *BAN_New(void);
int BAN_AddTest(struct cli *, struct ban *, const char *, const char *,
const char *);
void BAN_Free(struct ban *b);
-void BAN_Insert(struct ban *b);
+int BAN_Insert(struct ban *b);
void BAN_Init(void);
+void BAN_Shutdown(void);
void BAN_NewObjCore(struct objcore *oc);
void BAN_DestroyObj(struct objcore *oc);
int BAN_CheckObject(struct object *o, struct req *sp);
diff --git a/bin/varnishd/cache/cache_ban.c b/bin/varnishd/cache/cache_ban.c
index aa8bca9..7f02f90 100644
--- a/bin/varnishd/cache/cache_ban.c
+++ b/bin/varnishd/cache/cache_ban.c
@@ -104,6 +104,7 @@ static struct ban *ban_magic;
static pthread_t ban_thread;
static struct ban * volatile ban_start;
static bgthread_t ban_lurker;
+static int ban_shutdown = 0;
/*--------------------------------------------------------------------
* BAN string magic markers
@@ -372,9 +373,14 @@ BAN_AddTest(struct cli *cli, struct ban *b, const char *a1, const char *a2,
* 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.
+ *
+ * Returns:
+ * 0: Ban successfully inserted
+ * -1: Ban not inserted due to shutdown in progress. Caller should
+ * free the ban structure
*/
-void
+int
BAN_Insert(struct ban *b)
{
struct ban *bi, *be;
@@ -383,6 +389,9 @@ BAN_Insert(struct ban *b)
CHECK_OBJ_NOTNULL(b, BAN_MAGIC);
+ if (ban_shutdown)
+ return (-1);
+
AZ(VSB_finish(b->vsb));
ln = VSB_len(b->vsb);
assert(ln >= 0);
@@ -401,6 +410,11 @@ BAN_Insert(struct ban *b)
b->vsb = NULL;
Lck_Lock(&ban_mtx);
+ if (ban_shutdown) {
+ /* Check again, we might have raced */
+ Lck_Unlock(&ban_mtx);
+ return (-1);
+ }
VTAILQ_INSERT_HEAD(&ban_head, b, list);
ban_start = b;
VSC_C_main->bans++;
@@ -418,7 +432,7 @@ BAN_Insert(struct ban *b)
Lck_Unlock(&ban_mtx);
if (be == NULL)
- return;
+ return (0);
/* Hunt down duplicates, and mark them as gone */
bi = b;
@@ -436,6 +450,8 @@ BAN_Insert(struct ban *b)
}
be->refcount--;
Lck_Unlock(&ban_mtx);
+
+ return (0);
}
/*--------------------------------------------------------------------
@@ -519,6 +535,7 @@ BAN_Reload(const uint8_t *ban, unsigned len)
double t0, t1, t2 = 9e99;
ASSERT_CLI();
+ AZ(ban_shutdown);
t0 = ban_time(ban);
assert(len == ban_len(ban));
@@ -595,6 +612,7 @@ BAN_Compile(void)
{
ASSERT_CLI();
+ AZ(ban_shutdown);
/* Notify stevedores */
STV_BanInfo(BI_NEW, ban_magic->spec, ban_len(ban_magic->spec));
@@ -802,7 +820,7 @@ ban_CheckLast(void)
static int
ban_lurker_work(struct worker *wrk, struct vsl_log *vsl, unsigned pass)
{
- struct ban *b, *b0, *b2;
+ struct ban *b, *b0;
struct objhead *oh;
struct objcore *oc, *oc2;
struct object *o;
@@ -811,18 +829,6 @@ ban_lurker_work(struct worker *wrk, struct vsl_log *vsl, unsigned pass)
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.
@@ -853,6 +859,8 @@ ban_lurker_work(struct worker *wrk, struct vsl_log *vsl, unsigned pass)
ban_time(b->spec), b->refcount);
while (1) {
Lck_Lock(&ban_mtx);
+ if (ban_shutdown)
+ break;
oc = VTAILQ_FIRST(&b->objcore);
if (oc == NULL)
break;
@@ -938,6 +946,8 @@ ban_lurker_work(struct worker *wrk, struct vsl_log *vsl, unsigned pass)
ban_time(b->spec));
}
Lck_Unlock(&ban_mtx);
+ if (ban_shutdown)
+ break;
VTIM_sleep(cache_param->ban_lurker_sleep);
if (b == b0)
break;
@@ -951,18 +961,16 @@ ban_lurker(struct worker *wrk, void *priv)
struct ban *bf;
unsigned pass = (1 << LURK_SHIFT);
struct vsl_log vsl;
+ double ban_sleep;
- int i = 0;
VSL_Setup(&vsl, NULL, 0);
(void)priv;
- while (1) {
+ while (!ban_shutdown) {
+ ban_sleep = 1.0;
- while (cache_param->ban_lurker_sleep == 0.0) {
- /*
- * Ban-lurker is disabled:
- * Clean the last ban, if possible, and sleep
- */
+ /* Clear any gone last bans */
+ do {
Lck_Lock(&ban_mtx);
bf = ban_CheckLast();
if (bf != NULL)
@@ -972,23 +980,29 @@ ban_lurker(struct worker *wrk, void *priv)
Lck_Unlock(&ban_mtx);
if (bf != NULL)
BAN_Free(bf);
- else
- VTIM_sleep(1.0);
- }
+ } while (bf != NULL);
- i = ban_lurker_work(wrk, &vsl, pass);
- VSL_Flush(&vsl, 0);
- WRK_SumStat(wrk);
- if (i) {
- pass += (1 << LURK_SHIFT);
- pass &= BAN_F_LURK;
- if (pass == 0)
+ if (cache_param->ban_lurker_sleep != 0.0) {
+ /* Ban lurker enabled */
+ if (ban_lurker_work(wrk, &vsl, pass)) {
pass += (1 << LURK_SHIFT);
- VTIM_sleep(cache_param->ban_lurker_sleep);
- } else {
- VTIM_sleep(1.0);
+ pass &= BAN_F_LURK;
+ if (pass == 0)
+ pass += (1 << LURK_SHIFT);
+ ban_sleep = cache_param->ban_lurker_sleep;
+ }
+ VSL_Flush(&vsl, 0);
+ WRK_SumStat(wrk);
}
+
+ if (ban_shutdown)
+ break;
+
+ VTIM_sleep(ban_sleep);
}
+
+ pthread_exit(0);
+
NEEDLESS_RETURN(NULL);
}
@@ -1031,7 +1045,12 @@ ccf_ban(struct cli *cli, const char * const *av, void *priv)
BAN_Free(b);
return;
}
- BAN_Insert(b);
+ if (BAN_Insert(b) < 0) {
+ BAN_Free(b);
+ VCLI_Out(cli, "Shutdown in progress");
+ VCLI_SetResult(cli, CLIS_CANT);
+ return;
+ }
}
static void
@@ -1145,3 +1164,24 @@ BAN_Init(void)
VSC_C_main->bans_gone++;
BAN_Insert(ban_magic);
}
+
+/*--------------------------------------------------------------------
+ * Initiate a shutdown of the ban system.
+ *
+ * When this function returns, no new bans will be accepted, and no
+ * bans will be dropped, so that no STV_BanInfo calls will be
+ * executed. Also shuts down the ban lurker.
+ */
+
+void
+BAN_Shutdown(void)
+{
+ void *status;
+
+ Lck_Lock(&ban_mtx);
+ ban_shutdown = 1;
+ Lck_Unlock(&ban_mtx);
+
+ AZ(pthread_join(ban_thread, &status));
+ AZ(status);
+}
diff --git a/bin/varnishd/cache/cache_main.c b/bin/varnishd/cache/cache_main.c
index 73f965e..6e4a832 100644
--- a/bin/varnishd/cache/cache_main.c
+++ b/bin/varnishd/cache/cache_main.c
@@ -231,6 +231,7 @@ child_main(void)
CLI_Run();
+ BAN_Shutdown();
STV_close();
printf("Child dies\n");
diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c
index ed7ae59..0082bdc 100644
--- a/bin/varnishd/cache/cache_vrt.c
+++ b/bin/varnishd/cache/cache_vrt.c
@@ -458,11 +458,9 @@ VRT_ban(const struct req *req, char *cmds, ...)
a1 = va_arg(ap, char *);
good = 1;
}
- if (!good)
+ if (!good || BAN_Insert(b) < 0)
/* XXX: report error how ? */
BAN_Free(b);
- else
- BAN_Insert(b);
}
/*--------------------------------------------------------------------*/
@@ -505,11 +503,9 @@ VRT_ban_string(const struct req *req, const char *str)
if (strcmp(av[i++], "&&"))
break;
}
- if (!good)
+ if (!good || BAN_Insert(b) < 0)
/* XXX: report error how ? */
BAN_Free(b);
- else
- BAN_Insert(b);
VAV_Free(av);
}
--
1.7.9.5
More information about the varnish-dev
mailing list