[master] 4c18047 Overhaul the ban_lurker to avoid a race condition where it checks an object at the same time a request-lookup does.
Poul-Henning Kamp
phk at varnish-cache.org
Fri May 6 13:10:27 CEST 2011
commit 4c18047ef64fb5a40f229dfb0e30dc85530ffb7e
Author: Poul-Henning Kamp <phk at FreeBSD.org>
Date: Fri May 6 11:07:11 2011 +0000
Overhaul the ban_lurker to avoid a race condition where it checks
an object at the same time a request-lookup does.
The functional solution to this race is to hold the objhdr mutex,
which we already hold briefly to get the refcount, also when we do
the check.
In terms of souce code this inlines the problematic HSH_FindBan()
function in the lurker.
And since that was major surgery af few other acts of improvement
was carried out also.
Most notably, we will now scan all applicable bans in the lurker
and not give up on the first ban that tests req.* variables.
diff --git a/bin/varnishd/cache_ban.c b/bin/varnishd/cache_ban.c
index 2cac80f..94ff84d 100644
--- a/bin/varnishd/cache_ban.c
+++ b/bin/varnishd/cache_ban.c
@@ -421,7 +421,7 @@ ban_check_object(struct object *o, const struct sess *sp, int has_req)
struct objcore *oc;
struct ban_test *bt;
struct ban * volatile b0;
- unsigned tests;
+ unsigned tests, skipped;
CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
CHECK_OBJ_NOTNULL(o, OBJECT_MAGIC);
@@ -441,12 +441,19 @@ ban_check_object(struct object *o, const struct sess *sp, int has_req)
* inspect the list past that ban.
*/
tests = 0;
+ skipped = 0;
for (b = b0; b != oc->ban; b = VTAILQ_NEXT(b, list)) {
CHECK_OBJ_NOTNULL(b, BAN_MAGIC);
if (b->flags & BAN_F_GONE)
continue;
- if (!has_req && (b->flags & BAN_F_REQ))
- return (0);
+ if (!has_req && (b->flags & BAN_F_REQ)) {
+ /*
+ * We cannot test this one, but there might
+ * be other bans that match, so we soldier on
+ */
+ skipped++;
+ continue;
+ }
VTAILQ_FOREACH(bt, &b->tests, list) {
tests++;
if (bt->func(bt, o, sp))
@@ -456,6 +463,14 @@ ban_check_object(struct object *o, const struct sess *sp, int has_req)
break;
}
+ if (b == oc->ban && skipped > 0) {
+ /*
+ * Not banned, but some tests were skipped, so we cannot know
+ * for certain that it cannot be, so we just have to give up.
+ */
+ return (0);
+ }
+
Lck_Lock(&ban_mtx);
oc->ban->refcount--;
VTAILQ_REMOVE(&oc->ban->objcore, oc, ban_list);
@@ -487,67 +502,105 @@ int
BAN_CheckObject(struct object *o, const struct sess *sp)
{
- return ban_check_object(o, sp, 1);
+ return (ban_check_object(o, sp, 1));
}
/*--------------------------------------------------------------------
* Ban tail lurker thread
*/
-static void * __match_proto__(bgthread_t)
-ban_lurker(struct sess *sp, void *priv)
+static void
+ban_lurker_work(const struct sess *sp)
{
struct ban *b, *bf;
- struct objcore *oc;
+ struct objhead *oh;
+ struct objcore *oc, *oc2;
struct object *o;
int i;
- (void)priv;
- while (1) {
- WSL_Flush(sp->wrk, 0);
- WRK_SumStat(sp->wrk);
- if (params->ban_lurker_sleep == 0.0) {
- AZ(sleep(1));
- continue;
- }
- Lck_Lock(&ban_mtx);
+ WSL_Flush(sp->wrk, 0);
+ WRK_SumStat(sp->wrk);
- /* First try to route the last ban */
- bf = BAN_CheckLast();
- if (bf != NULL) {
- Lck_Unlock(&ban_mtx);
- BAN_Free(bf);
- TIM_sleep(params->ban_lurker_sleep);
- continue;
- }
- /* Then try to poke the first object on the last ban */
- oc = NULL;
- while (1) {
- b = VTAILQ_LAST(&ban_head, banhead_s);
- if (b == ban_start)
- break;
- oc = VTAILQ_FIRST(&b->objcore);
- if (oc == NULL)
- break;
- HSH_FindBan(sp, &oc);
+ Lck_Lock(&ban_mtx);
+
+ /* First try to route the last ban */
+ bf = BAN_CheckLast();
+ if (bf != NULL) {
+ Lck_Unlock(&ban_mtx);
+ BAN_Free(bf);
+ return;
+ }
+
+ /* Find the last ban give up, if we have only one */
+ b = VTAILQ_LAST(&ban_head, banhead_s);
+ if (b == ban_start) {
+ Lck_Unlock(&ban_mtx);
+ return;
+ }
+
+ /* Find the first object on it, if any */
+ oc = VTAILQ_FIRST(&b->objcore);
+ if (oc == NULL) {
+ Lck_Unlock(&ban_mtx);
+ return;
+ }
+
+ /* Try to lock the objhead */
+ oh = oc->objhead;
+ CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
+ if (Lck_Trylock(&oh->mtx)) {
+ Lck_Unlock(&ban_mtx);
+ return;
+ }
+
+ /*
+ * See if the objcore is still on the objhead since we race against
+ * HSH_Deref() which comes in the opposite locking order.
+ */
+ VTAILQ_FOREACH(oc2, &oh->objcs, list)
+ if (oc == oc2)
break;
- }
+ if (oc2 == NULL) {
+ Lck_Unlock(&oh->mtx);
Lck_Unlock(&ban_mtx);
- if (oc == NULL) {
+ return;
+ }
+ /*
+ * Grab a reference to the OC and we can let go of the BAN mutex
+ */
+ AN(oc->refcnt);
+ oc->refcnt++;
+ Lck_Unlock(&ban_mtx);
+
+ /*
+ * Get the object and check it against all relevant bans
+ */
+ o = oc_getobj(sp->wrk, oc);
+ i = ban_check_object(o, sp, 0);
+ Lck_Unlock(&oh->mtx);
+ WSP(sp, SLT_Debug, "lurker: %p %g %d", oc, o->exp.ttl, i);
+ (void)HSH_Deref(sp->wrk, NULL, &o);
+}
+
+static void * __match_proto__(bgthread_t)
+ban_lurker(struct sess *sp, void *priv)
+{
+
+ (void)priv;
+ while (1) {
+ if (params->ban_lurker_sleep == 0.0) {
+ /* Lurker is disabled. */
TIM_sleep(1.0);
continue;
}
- // AZ(oc->flags & OC_F_PERSISTENT);
- o = oc_getobj(sp->wrk, oc);
- i = ban_check_object(o, sp, 0);
- WSP(sp, SLT_Debug, "lurker: %p %g %d", oc, o->exp.ttl, i);
- (void)HSH_Deref(sp->wrk, NULL, &o);
TIM_sleep(params->ban_lurker_sleep);
+ ban_lurker_work(sp);
+ WSL_Flush(sp->wrk, 0);
+ WRK_SumStat(sp->wrk);
}
NEEDLESS_RETURN(NULL);
}
-
/*--------------------------------------------------------------------
* Release a tail reference
*/
@@ -680,12 +733,14 @@ BAN_Compile(void)
av = ParseArgv(b->test, 0);
XXXAN(av);
XXXAZ(av[0]);
- for (i = 1; av[i] != NULL; i += 3) {
- if (i != 1) {
- AZ(strcmp(av[i], "&&"));
- i++;
- }
- AZ(BAN_AddTest(NULL, b, av[i], av[i + 1], av[i + 2]));
+ XXXAN(av[1]);
+ XXXAN(av[2]);
+ XXXAN(av[3]);
+ AZ(BAN_AddTest(NULL, b, av[1], av[2], av[3]));
+ for (i = 4; av[i] != NULL; i += 4) {
+ AZ(strcmp(av[i], "&&"));
+ AZ(BAN_AddTest(NULL, b,
+ av[i + 1], av[i + 2], av[i + 3]));
}
}
ban_start = VTAILQ_FIRST(&ban_head);
diff --git a/bin/varnishd/cache_hash.c b/bin/varnishd/cache_hash.c
index bb18642..c25d0ab 100644
--- a/bin/varnishd/cache_hash.c
+++ b/bin/varnishd/cache_hash.c
@@ -710,43 +710,6 @@ HSH_Deref(struct worker *w, struct objcore *oc, struct object **oo)
return (0);
}
-/*--------------------------------------------------------------------
- * This one is slightly tricky. This is called from the BAN module
- * to try to wash the object which holds the oldest ban.
- * We compete against HSH_Deref() which comes in the opposite
- * locking order, we need to hold the BAN mutex, to stop the
- * BAN_DestroyObj() call in HSH_Deref(), so that the objhead
- * will not be removed under us.
- * NB: Do not call this function any other way or from any other
- * NB: place in the code. It will not work for you.
- */
-
-void
-HSH_FindBan(const struct sess *sp, struct objcore **oc)
-{
- struct objcore *oc1, *oc2;
- struct objhead *oh;
-
- oc1 = *oc;
- CHECK_OBJ_NOTNULL(oc1, OBJCORE_MAGIC);
- oh = oc1->objhead;
- CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
- if (Lck_Trylock(&oh->mtx)) {
- *oc = NULL;
- return;
- }
- VTAILQ_FOREACH(oc2, &oh->objcs, list)
- if (oc1 == oc2)
- break;
- if (oc2 != NULL)
- (void)oc_getobj(sp->wrk, oc2);
- if (oc2 != NULL)
- oc2->refcnt++;
- Lck_Unlock(&oh->mtx);
- *oc = oc2;
-}
-
-
void
HSH_Init(void)
{
diff --git a/bin/varnishd/hash_slinger.h b/bin/varnishd/hash_slinger.h
index 65367ad..42fa50b 100644
--- a/bin/varnishd/hash_slinger.h
+++ b/bin/varnishd/hash_slinger.h
@@ -59,7 +59,6 @@ void HSH_Ref(struct objcore *o);
void HSH_Drop(struct sess *sp);
void HSH_Init(void);
void HSH_AddString(const struct sess *sp, const char *str);
-void HSH_FindBan(const struct sess *sp, struct objcore **oc);
struct objcore *HSH_Insert(const struct sess *sp);
void HSH_Purge(const struct sess *, struct objhead *, double ttl, double grace);
void HSH_config(const char *h_arg);
More information about the varnish-commit
mailing list