diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h index b52cbab01a..ee9ba5e950 100644 --- a/bin/varnishd/cache/cache.h +++ b/bin/varnishd/cache/cache.h @@ -356,6 +356,7 @@ struct objcore { uint16_t oa_present; unsigned timer_idx; // XXX 4Gobj limit + unsigned waitinglist_gen; vtim_real last_lru; VTAILQ_ENTRY(objcore) hsh_list; VTAILQ_ENTRY(objcore) lru_list; @@ -473,6 +474,7 @@ struct req { stream_close_t doclose; unsigned restarts; unsigned esi_level; + unsigned waitinglist_gen; /* Delivery mode */ unsigned res_mode; @@ -499,9 +501,6 @@ struct req { struct objcore *body_oc; - /* The busy objhead we sleep on */ - struct objhead *hash_objhead; - /* Built Vary string == workspace reservation */ uint8_t *vary_b; uint8_t *vary_e; diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index 11ed71eef4..8bbf85a710 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -101,6 +101,7 @@ hsh_newobjhead(void) ALLOC_OBJ(oh, OBJHEAD_MAGIC); XXXAN(oh); oh->refcnt = 1; + oh->waitinglist_gen = 1; VTAILQ_INIT(&oh->objcs); VTAILQ_INIT(&oh->waitinglist); Lck_New(&oh->mtx, lck_objhdr); @@ -355,6 +356,41 @@ hsh_insert_busyobj(const struct worker *wrk, struct objhead *oh) return (oc); } +/*--------------------------------------------------------------------- +*/ + +static unsigned +hsh_rush_match(struct req *req) +{ + struct objhead *oh; + struct objcore *oc; + const uint8_t *vary; + + oc = req->objcore; + CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC); + + AZ(oc->flags & OC_F_BUSY); + AZ(oc->flags & OC_F_PRIVATE); + if (oc->flags & (OC_F_WITHDRAWN|OC_F_HFM|OC_F_HFP|OC_F_CANCEL| + OC_F_FAILED)) + return (0); + + if (req->vcf != NULL) /* NB: must operate under oh lock. */ + return (0); + + oh = oc->objhead; + CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC); + + if (req->hash_ignore_vary) + return (1); + if (!ObjHasAttr(req->wrk, oc, OA_VARY)) + return (1); + + vary = ObjGetAttr(req->wrk, oc, OA_VARY, NULL); + AN(vary); + return (VRY_Match(req, vary)); +} + /*--------------------------------------------------------------------- */ @@ -383,6 +419,7 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp) CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); CHECK_OBJ_NOTNULL(wrk->wpriv, WORKER_PRIV_MAGIC); CHECK_OBJ_NOTNULL(req->http, HTTP_MAGIC); + CHECK_OBJ_ORNULL(req->objcore, OBJCORE_MAGIC); CHECK_OBJ_ORNULL(req->vcf, VCF_MAGIC); AN(hash); @@ -390,15 +427,32 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp) if (DO_DEBUG(DBG_HASHEDGE)) hsh_testmagic(req->digest); - if (req->hash_objhead != NULL) { + if (req->objcore != NULL && hsh_rush_match(req)) { + TAKE_OBJ_NOTNULL(oc, &req->objcore, OBJCORE_MAGIC); + *ocp = oc; + oh = oc->objhead; + Lck_Lock(&oh->mtx); + oc->hits++; + boc_progress = oc->boc == NULL ? -1 : oc->boc->fetched_so_far; + AN(hsh_deref_objhead_unlock(wrk, &oh, oc)); + Req_LogHit(wrk, req, oc, boc_progress); + /* NB: since this hit comes from the waiting list instead of + * a regular lookup, grace is not considered. The object is + * fresh in the context of the waiting list, even expired: it + * was successfully just [re]validated by a fetch task. + */ + return (HSH_HIT); + } + + if (req->objcore != NULL) { /* * This req came off the waiting list, and brings an - * oh refcnt with it. + * oh refcnt and an incompatible oc refcnt with it, + * the latter acquired during rush hour. */ - CHECK_OBJ_NOTNULL(req->hash_objhead, OBJHEAD_MAGIC); - oh = req->hash_objhead; + oh = req->objcore->objhead; + (void)HSH_DerefObjCore(wrk, &req->objcore); Lck_Lock(&oh->mtx); - req->hash_objhead = NULL; } else { AN(wrk->wpriv->nobjhead); oh = hash->lookup(wrk, req->digest, &wrk->wpriv->nobjhead); @@ -581,13 +635,14 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp) AZ(req->hash_ignore_busy); /* - * The objhead reference transfers to the sess, we get it - * back when the sess comes off the waiting list and - * calls us again + * The objhead reference is held by req while it is parked on the + * waiting list. The oh pointer is taken back from the objcore that + * triggers a rush of req off the waiting list. */ - req->hash_objhead = oh; + assert(oh->refcnt > 1); + req->wrk = NULL; - req->waitinglist = 1; + req->waitinglist_gen = oh->waitinglist_gen; if (DO_DEBUG(DBG_WAITINGLIST)) VSLb(req->vsl, SLT_Debug, "on waiting list <%p>", oh); @@ -623,25 +678,32 @@ hsh_rush1(const struct worker *wrk, struct objcore *oc, struct rush *r) AZ(oc->flags & OC_F_BUSY); AZ(oc->flags & OC_F_PRIVATE); + max = cache_param->rush_exponent; if (oc->flags & (OC_F_WITHDRAWN|OC_F_FAILED)) max = 1; - else if (oc->flags & (OC_F_HFM|OC_F_HFP|OC_F_CANCEL|OC_F_DYING)) - max = cache_param->rush_exponent; - else - max = INT_MAX; /* XXX: temp */ assert(max > 0); + if (oc->waitinglist_gen == 0) { + oc->waitinglist_gen = oh->waitinglist_gen; + oh->waitinglist_gen++; + } + for (i = 0; i < max; i++) { req = VTAILQ_FIRST(&oh->waitinglist); + CHECK_OBJ_ORNULL(req, REQ_MAGIC); if (req == NULL) break; - CHECK_OBJ_NOTNULL(req, REQ_MAGIC); - wrk->stats->busy_wakeup++; + if (req->waitinglist_gen > oc->waitinglist_gen) + continue; + AZ(req->wrk); VTAILQ_REMOVE(&oh->waitinglist, req, w_list); VTAILQ_INSERT_TAIL(&r->reqs, req, w_list); - req->waitinglist = 0; + req->objcore = oc; } + + oc->refcnt += i; + wrk->stats->busy_wakeup += i; } /*--------------------------------------------------------------------- @@ -902,8 +964,8 @@ HSH_Withdraw(struct worker *wrk, struct objcore **ocp) assert(oc->refcnt == 1); assert(oh->refcnt > 0); oc->flags = OC_F_WITHDRAWN; - hsh_rush1(wrk, oc, &rush); - AZ(HSH_DerefObjCoreUnlock(wrk, &oc)); + hsh_rush1(wrk, oc, &rush); /* grabs up to 1 oc ref */ + assert(HSH_DerefObjCoreUnlock(wrk, &oc) <= 1); hsh_rush2(wrk, &rush); } diff --git a/bin/varnishd/cache/cache_objhead.h b/bin/varnishd/cache/cache_objhead.h index 94ef81db52..162aaeb7fa 100644 --- a/bin/varnishd/cache/cache_objhead.h +++ b/bin/varnishd/cache/cache_objhead.h @@ -40,6 +40,7 @@ struct objhead { struct lock mtx; VTAILQ_HEAD(,objcore) objcs; uint8_t digest[DIGEST_LEN]; + unsigned waitinglist_gen; VTAILQ_HEAD(, req) waitinglist; /*---------------------------------------------------- diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c index d59a2024c7..b70bd7dfb4 100644 --- a/bin/varnishd/cache/cache_req_fsm.c +++ b/bin/varnishd/cache/cache_req_fsm.c @@ -557,20 +557,23 @@ cnt_lookup(struct worker *wrk, struct req *req) { struct objcore *oc, *busy; enum lookup_e lr; - int had_objhead = 0; + int had_objcore = 0; CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); CHECK_OBJ_NOTNULL(req, REQ_MAGIC); - AZ(req->objcore); AZ(req->stale_oc); AN(req->vcl); VRY_Prep(req); - AZ(req->objcore); - if (req->hash_objhead) - had_objhead = 1; + if (req->waitinglist_gen) { + CHECK_OBJ_NOTNULL(req->objcore, OBJCORE_MAGIC); + req->waitinglist_gen = 0; + had_objcore = 1; + } else + AZ(req->objcore); + wrk->strangelove = 0; lr = HSH_Lookup(req, &oc, &busy); if (lr == HSH_BUSY) { @@ -586,7 +589,7 @@ cnt_lookup(struct worker *wrk, struct req *req) if ((unsigned)wrk->strangelove >= cache_param->vary_notice) VSLb(req->vsl, SLT_Notice, "vsl: High number of variants (%d)", wrk->strangelove); - if (had_objhead) + if (had_objcore) VSLb_ts_req(req, "Waitinglist", W_TIM_real(wrk)); if (req->vcf != NULL) { diff --git a/bin/varnishd/cache/cache_vary.c b/bin/varnishd/cache/cache_vary.c index fa9053966e..7a3962bdc2 100644 --- a/bin/varnishd/cache/cache_vary.c +++ b/bin/varnishd/cache/cache_vary.c @@ -224,8 +224,7 @@ vry_cmp(const uint8_t *v1, const uint8_t *v2) void VRY_Prep(struct req *req) { - if (req->hash_objhead == NULL) { - /* Not a waiting list return */ + if (req->waitinglist_gen == 0) { AZ(req->vary_b); AZ(req->vary_e); (void)WS_ReserveAll(req->ws); diff --git a/bin/varnishtest/tests/c00125.vtc b/bin/varnishtest/tests/c00125.vtc new file mode 100644 index 0000000000..8fbca4f46b --- /dev/null +++ b/bin/varnishtest/tests/c00125.vtc @@ -0,0 +1,156 @@ +varnishtest "successful expired waiting list hit" + +barrier b1 cond 2 +barrier b2 cond 2 +barrier b3 cond 2 +barrier b4 cond 2 + + +server s1 { + rxreq + expect req.http.user-agent == c1 + expect req.http.bgfetch == false + barrier b1 sync + barrier b2 sync + txresp -hdr "Cache-Control: max-age=60" -hdr "Age: 120" + + rxreq + expect req.http.user-agent == c3 + expect req.http.bgfetch == true + txresp + + # The no-cache case only works with a complicit VCL, for now. + rxreq + expect req.http.user-agent == c4 + expect req.http.bgfetch == false + barrier b3 sync + barrier b4 sync + txresp -hdr "Cache-Control: no-cache" + + rxreq + expect req.http.user-agent == c6 + expect req.http.bgfetch == false + txresp -hdr "Cache-Control: no-cache" +} -start + +varnish v1 -cliok "param.set default_grace 1h" +varnish v1 -cliok "param.set thread_pools 1" +varnish v1 -cliok "param.set debug +syncvsl,+waitinglist" +varnish v1 -vcl+backend { + sub vcl_backend_fetch { + set bereq.http.bgfetch = bereq.is_bgfetch; + } + sub vcl_beresp_stale { + # We just validated a stale object, do not mark it as + # uncacheable. The object remains available for grace + # hits and background fetches. + return; + } + sub vcl_beresp_control { + if (beresp.http.cache-control == "no-cache") { + # Keep beresp.uncacheable clear. + return; + } + } + sub vcl_deliver { + set resp.http.obj-hits = obj.hits; + set resp.http.obj-ttl = obj.ttl; + } +} -start + +client c1 { + txreq -url "/stale-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == 1001 + expect resp.http.obj-hits == 0 + expect resp.http.obj-ttl < 0 +} -start + +barrier b1 sync + +client c2 { + txreq -url "/stale-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == "1004 1002" + expect resp.http.obj-hits == 1 + expect resp.http.obj-ttl < 0 +} -start + +varnish v1 -expect busy_sleep == 1 +barrier b2 sync + +client c1 -wait +client c2 -wait + +varnish v1 -vsl_catchup + +varnish v1 -expect cache_miss == 1 +varnish v1 -expect cache_hit == 1 +varnish v1 -expect cache_hit_grace == 0 +varnish v1 -expect s_bgfetch == 0 + +client c3 { + txreq -url "/stale-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == "1006 1002" + expect resp.http.obj-hits == 2 + expect resp.http.obj-ttl < 0 +} -run + +varnish v1 -vsl_catchup + +varnish v1 -expect cache_miss == 1 +varnish v1 -expect cache_hit == 2 +varnish v1 -expect cache_hit_grace == 1 +varnish v1 -expect s_bgfetch == 1 + +# The only way for a plain no-cache to be hit is to have a non-zero keep. +varnish v1 -cliok "param.set default_ttl 0" +varnish v1 -cliok "param.set default_grace 0" +varnish v1 -cliok "param.set default_keep 1h" + +client c4 { + txreq -url "/no-cache-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == 1009 + expect resp.http.obj-hits == 0 + expect resp.http.obj-ttl <= 0 +} -start + +barrier b3 sync + +client c5 { + txreq -url "/no-cache-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == "1012 1010" + expect resp.http.obj-hits == 1 + expect resp.http.obj-ttl <= 0 +} -start + +varnish v1 -expect busy_sleep == 2 +barrier b4 sync + +client c4 -wait +client c5 -wait + +varnish v1 -vsl_catchup + +varnish v1 -expect cache_miss == 2 +varnish v1 -expect cache_hit == 3 +varnish v1 -expect cache_hit_grace == 1 +varnish v1 -expect s_bgfetch == 1 + +# No hit when not on the waiting list +client c6 { + txreq -url "/no-cache-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == 1014 + expect resp.http.obj-hits == 0 + expect resp.http.obj-ttl <= 0 +} -run diff --git a/include/tbl/req_flags.h b/include/tbl/req_flags.h index 6a9e6acbb1..15a1b430e6 100644 --- a/include/tbl/req_flags.h +++ b/include/tbl/req_flags.h @@ -37,7 +37,6 @@ REQ_FLAG(hash_ignore_busy, 1, 1, "") REQ_FLAG(hash_ignore_vary, 1, 1, "") REQ_FLAG(hash_always_miss, 1, 1, "") REQ_FLAG(is_hit, 0, 0, "") -REQ_FLAG(waitinglist, 0, 0, "") REQ_FLAG(want100cont, 0, 0, "") REQ_FLAG(late100cont, 0, 0, "") REQ_FLAG(req_reset, 0, 0, "")