[4.1] bf752ef Avoid leaking an OH ref on reembark failure
Dag Haavi Finstad
daghf at varnish-software.com
Thu Feb 22 12:40:11 UTC 2018
commit bf752eff529f8b1569a2caa5b4a8af9d9499e49c
Author: Dag Haavi Finstad <daghf at varnish-software.com>
Date: Thu Feb 22 13:29:38 2018 +0100
Avoid leaking an OH ref on reembark failure
This is a backport of 5cc47eaa8.
With this commit hsh_rush has been split into hsh_rush and
hsh_rush_clean. The former needs to be called while holding the OH lock,
and the latter needs to be called without holding the lock.
The reason for this added complexity is that we can't hold the lock
while calling HSH_DerefObjHead.
Fixes: #2495
diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c
index 5f657f9..30e2a06 100644
--- a/bin/varnishd/cache/cache_hash.c
+++ b/bin/varnishd/cache/cache_hash.c
@@ -65,6 +65,12 @@
static const struct hash_slinger *hash;
static struct objhead *private_oh;
+struct rush {
+ unsigned magic;
+#define RUSH_MAGIC 0xa1af5f01
+ VTAILQ_HEAD(,req) reqs;
+};
+
/*---------------------------------------------------------------------*/
static struct objcore *
@@ -524,15 +530,16 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp,
*/
static void
-hsh_rush(struct worker *wrk, struct objhead *oh)
+hsh_rush(struct worker *wrk, struct objhead *oh, struct rush *r)
{
unsigned u;
struct req *req;
- struct sess *sp;
struct waitinglist *wl;
CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
+ CHECK_OBJ_NOTNULL(r, RUSH_MAGIC);
+ assert(VTAILQ_EMPTY(&r->reqs));
Lck_AssertHeld(&oh->mtx);
wl = oh->waitinglist;
CHECK_OBJ_ORNULL(wl, WAITINGLIST_MAGIC);
@@ -547,21 +554,14 @@ hsh_rush(struct worker *wrk, struct objhead *oh)
AZ(req->wrk);
VTAILQ_REMOVE(&wl->list, req, w_list);
DSL(DBG_WAITINGLIST, req->vsl->wid, "off waiting list");
- if (SES_Reschedule_Req(req)) {
+ if (DO_DEBUG(DBG_FAILRESCHED) || SES_Reschedule_Req(req)) {
/*
* In case of overloads, we ditch the entire
* waiting list.
*/
wrk->stats->busy_wakeup--;
while (1) {
- wrk->stats->busy_killed++;
- AN (req->vcl);
- VCL_Rel(&req->vcl);
- sp = req->sp;
- CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
- CNT_AcctLogCharge(wrk->stats, req);
- Req_Release(req);
- SES_Delete(sp, SC_OVERLOAD, NAN);
+ VTAILQ_INSERT_TAIL(&r->reqs, req, w_list);
req = VTAILQ_FIRST(&wl->list);
if (req == NULL)
break;
@@ -580,6 +580,36 @@ hsh_rush(struct worker *wrk, struct objhead *oh)
}
}
+static void
+hsh_rush_clean(struct worker *wrk, struct rush *r)
+{
+ struct req *req;
+ struct sess *sp;
+
+ CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
+ CHECK_OBJ_NOTNULL(r, RUSH_MAGIC);
+
+ while (!VTAILQ_EMPTY(&r->reqs)) {
+ req = VTAILQ_FIRST(&r->reqs);
+ CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
+ VTAILQ_REMOVE(&r->reqs, req, w_list);
+ wrk->stats->busy_killed++;
+ AN (req->vcl);
+ VCL_Rel(&req->vcl);
+ sp = req->sp;
+ CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
+ CNT_AcctLogCharge(wrk->stats, req);
+ CHECK_OBJ_NOTNULL(req->hash_objhead, OBJHEAD_MAGIC);
+ AN(HSH_DerefObjHead(wrk, &req->hash_objhead));
+ AZ(req->hash_objhead);
+ Req_Release(req);
+ SES_Delete(sp, SC_OVERLOAD, NAN);
+ DSL(DBG_WAITINGLIST, req->vsl->wid,
+ "kill from waiting list");
+ }
+
+}
+
/*---------------------------------------------------------------------
* Purge an entire objhead
*/
@@ -719,11 +749,14 @@ void
HSH_Unbusy(struct worker *wrk, struct objcore *oc)
{
struct objhead *oh;
+ struct rush rush;
CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
oh = oc->objhead;
CHECK_OBJ(oh, OBJHEAD_MAGIC);
+ INIT_OBJ(&rush, RUSH_MAGIC);
+ VTAILQ_INIT(&rush.reqs);
AN(oc->stobj->stevedore);
AN(oc->flags & OC_F_BUSY);
@@ -744,8 +777,9 @@ HSH_Unbusy(struct worker *wrk, struct objcore *oc)
VTAILQ_INSERT_HEAD(&oh->objcs, oc, list);
oc->flags &= ~OC_F_BUSY;
if (oh->waitinglist != NULL)
- hsh_rush(wrk, oh);
+ hsh_rush(wrk, oh, &rush);
Lck_Unlock(&oh->mtx);
+ hsh_rush_clean(wrk, &rush);
}
/*---------------------------------------------------------------------
@@ -800,6 +834,7 @@ HSH_DerefObjCore(struct worker *wrk, struct objcore **ocp)
{
struct objcore *oc;
struct objhead *oh;
+ struct rush rush;
unsigned r;
AN(ocp);
@@ -812,6 +847,8 @@ HSH_DerefObjCore(struct worker *wrk, struct objcore **ocp)
oh = oc->objhead;
CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
+ INIT_OBJ(&rush, RUSH_MAGIC);
+ VTAILQ_INIT(&rush.reqs);
Lck_Lock(&oh->mtx);
assert(oh->refcnt > 0);
@@ -819,8 +856,9 @@ HSH_DerefObjCore(struct worker *wrk, struct objcore **ocp)
if (!r)
VTAILQ_REMOVE(&oh->objcs, oc, list);
if (oh->waitinglist != NULL)
- hsh_rush(wrk, oh);
+ hsh_rush(wrk, oh, &rush);
Lck_Unlock(&oh->mtx);
+ hsh_rush_clean(wrk, &rush);
if (r != 0)
return (r);
@@ -842,6 +880,7 @@ int
HSH_DerefObjHead(struct worker *wrk, struct objhead **poh)
{
struct objhead *oh;
+ struct rush rush;
int r;
CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
@@ -867,12 +906,15 @@ HSH_DerefObjHead(struct worker *wrk, struct objhead **poh)
* just make the hold the same ref's as objcore, that would
* confuse hashers.
*/
+ INIT_OBJ(&rush, RUSH_MAGIC);
+ VTAILQ_INIT(&rush.reqs);
while (oh->waitinglist != NULL) {
Lck_Lock(&oh->mtx);
assert(oh->refcnt > 0);
r = oh->refcnt;
- hsh_rush(wrk, oh);
+ hsh_rush(wrk, oh, &rush);
Lck_Unlock(&oh->mtx);
+ hsh_rush_clean(wrk, &rush);
if (r > 1)
break;
usleep(100000);
diff --git a/bin/varnishtest/tests/c00013.vtc b/bin/varnishtest/tests/c00013.vtc
index 689e118..d89b198 100644
--- a/bin/varnishtest/tests/c00013.vtc
+++ b/bin/varnishtest/tests/c00013.vtc
@@ -1,4 +1,4 @@
-varnishtest "Test parking second request on backend delay"
+varnishtest "Test parking second request on backend delay (waitinglist)"
server s1 {
rxreq
@@ -45,3 +45,60 @@ client c1 -wait
varnish v1 -expect busy_sleep == 1
varnish v1 -expect busy_wakeup == 1
+
+##################################################
+# Now try again where getting a thread fails
+
+barrier b3 cond 2
+barrier b4 cond 2
+
+server s3 {
+ rxreq
+ expect req.url == "/foo"
+ send "HTTP/1.0 200 OK\r\nConnection: close\r\n\r\n"
+ delay .2
+ barrier b3 sync
+ delay .2
+ send "line1\n"
+ delay .2
+ barrier b4 sync
+ send "line2\n"
+} -start
+
+varnish v3 -vcl+backend {
+ sub vcl_backend_fetch {
+ set bereq.backend = s3;
+ }
+ sub vcl_backend_response {
+ set beresp.do_stream = false;
+ }
+} -start
+
+varnish v3 -cliok "param.set debug +failresched"
+
+varnish v3 -cliok "param.set debug +syncvsl"
+
+client c3 -connect ${v3_sock} {
+ txreq -url "/foo" -hdr "client: c3"
+ rxresp
+ expect resp.status == 200
+ expect resp.bodylen == 12
+ expect resp.http.x-varnish == "1001"
+} -start
+
+barrier b3 sync
+
+client c4 -connect ${v3_sock} {
+ txreq -url "/foo" -hdr "client: c4"
+ delay .2
+ barrier b4 sync
+ expect_close
+} -run
+
+client c3 -wait
+
+varnish v1 -vsl_catchup
+varnish v3 -expect busy_sleep >= 1
+varnish v3 -expect busy_wakeup == 0
+varnish v3 -expect busy_killed == 1
+varnish v3 -expect sc_overload == 1
diff --git a/include/tbl/debug_bits.h b/include/tbl/debug_bits.h
index fa0d32f..4b3d74c 100644
--- a/include/tbl/debug_bits.h
+++ b/include/tbl/debug_bits.h
@@ -43,4 +43,5 @@ DEBUG_BIT(FLUSH_HEAD, flush_head, "Flush after http1 head")
DEBUG_BIT(VTC_MODE, vtc_mode, "Varnishtest Mode")
DEBUG_BIT(WITNESS, witness, "Emit WITNESS lock records")
DEBUG_BIT(VSM_KEEP, vsm_keep, "Keep the VSM file on restart")
+DEBUG_BIT(FAILRESCHED, failresched, "Fail from waiting list")
/*lint -restore */
More information about the varnish-commit
mailing list