From 705b1e5d35786951b3dba22622eed79b3f64d101 Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Tue, 21 May 2024 17:16:05 +0200 Subject: [PATCH] Return a consistent boc state from ObjWaitExtend() Clients to the Object API need to know not only the current extension (new length) of streaming objects, but also the streaming state - in particular BOS_FINISHED and BOS_FAILED. The latter for obvious reasons, and the former to call the delivery function with OBJ_ITER_END, which then likely results in VDP_END sent down the delivery pipeline. Background: It is important for efficient delivery to not receive an additional VDP_END with a null buffer, but rather combined with the last chunk of data, so, consequently, it is important to reliably send OBJ_INTER_END also with the last chunk of data. Consequent to all of this, ObjWaitExtend() callers need to know when BOS_FINISHED has been reached for some extension. The current API, however, does not provide a consistent view of the streaming state, which is only available from within the critical region of ObjWaitExtend(). Thus, we add the streaming state as an optional return value. With this commit, we also remove a superfluous line to set rv again: Because boc->fetched_so_far must only be updated while holding the boc mutex, reading the value again provides no benefit. --- bin/varnishd/cache/cache_obj.c | 11 +++++++---- bin/varnishd/cache/cache_varnishd.h | 2 +- bin/varnishd/storage/storage_simple.c | 18 +++++------------- doc/changes.rst | 5 +++++ include/vrt.h | 4 +++- 5 files changed, 21 insertions(+), 19 deletions(-) diff --git a/bin/varnishd/cache/cache_obj.c b/bin/varnishd/cache/cache_obj.c index ec819bf8cb7..21c3d8f9139 100644 --- a/bin/varnishd/cache/cache_obj.c +++ b/bin/varnishd/cache/cache_obj.c @@ -265,8 +265,10 @@ ObjExtend(struct worker *wrk, struct objcore *oc, ssize_t l, int final) */ uint64_t -ObjWaitExtend(const struct worker *wrk, const struct objcore *oc, uint64_t l) +ObjWaitExtend(const struct worker *wrk, const struct objcore *oc, uint64_t l, + enum boc_state_e *statep) { + enum boc_state_e state; uint64_t rv; CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); @@ -282,15 +284,16 @@ ObjWaitExtend(const struct worker *wrk, const struct objcore *oc, uint64_t l) oc->boc->delivered_so_far = l; PTOK(pthread_cond_signal(&oc->boc->cond)); } - if (rv > l || oc->boc->state >= BOS_FINISHED) + state = oc->boc->state; + if (rv > l || state >= BOS_FINISHED) break; (void)Lck_CondWait(&oc->boc->cond, &oc->boc->mtx); } - rv = oc->boc->fetched_so_far; Lck_Unlock(&oc->boc->mtx); + if (statep != NULL) + *statep = state; return (rv); } - /*==================================================================== */ diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h index add300e76c6..9bd5e249041 100644 --- a/bin/varnishd/cache/cache_varnishd.h +++ b/bin/varnishd/cache/cache_varnishd.h @@ -335,7 +335,7 @@ void ObjDestroy(const struct worker *, struct objcore **); int ObjGetSpace(struct worker *, struct objcore *, ssize_t *sz, uint8_t **ptr); void ObjExtend(struct worker *, struct objcore *, ssize_t l, int final); uint64_t ObjWaitExtend(const struct worker *, const struct objcore *, - uint64_t l); + uint64_t l, enum boc_state_e *statep); void ObjSetState(struct worker *, const struct objcore *, enum boc_state_e next); void ObjWaitState(const struct objcore *, enum boc_state_e want); diff --git a/bin/varnishd/storage/storage_simple.c b/bin/varnishd/storage/storage_simple.c index 6626d3a0d6c..68c780552ab 100644 --- a/bin/varnishd/storage/storage_simple.c +++ b/bin/varnishd/storage/storage_simple.c @@ -362,23 +362,16 @@ sml_iterator(struct worker *wrk, struct objcore *oc, } while (1) { ol = len; - nl = ObjWaitExtend(wrk, oc, ol); - if (boc->state == BOS_FAILED) { + nl = ObjWaitExtend(wrk, oc, ol, &state); + if (state == BOS_FAILED) { ret = -1; break; } if (nl == ol) { - /* - * note: the unguarded boc->state read could be - * outdated, in which case we call ObjWaitExtend() again - * for error handling but otherwise cause no harm. When - * using this code as an example, DO NOT rely on - * boc->state to be consistent - */ - if (boc->state == BOS_FINISHED) - break; - continue; + assert(state == BOS_FINISHED); + break; } + assert(nl > ol); Lck_Lock(&boc->mtx); AZ(VTAILQ_EMPTY(&obj->list)); if (checkpoint == NULL) { @@ -417,7 +410,6 @@ sml_iterator(struct worker *wrk, struct objcore *oc, st = VTAILQ_PREV(st, storagehead, list); if (st != NULL && st->len == 0) st = NULL; - state = boc->state; Lck_Unlock(&boc->mtx); assert(l > 0 || state == BOS_FINISHED); u = 0; diff --git a/doc/changes.rst b/doc/changes.rst index e4181508381..60158aa5fa8 100644 --- a/doc/changes.rst +++ b/doc/changes.rst @@ -41,6 +41,11 @@ Varnish Cache NEXT (2024-09-15) .. PLEASE keep this roughly in commit order as shown by git-log / tig (new to old) +* The ObjWaitExtend() Object API function gained a ``statep`` argument + to optionally return the busy object state consistent with the + current extension. A ``NULL`` value may be passed if the caller does + not require it. + ================================ Varnish Cache 7.5.0 (2024-03-18) ================================ diff --git a/include/vrt.h b/include/vrt.h index a7ed22babbb..edc2a520511 100644 --- a/include/vrt.h +++ b/include/vrt.h @@ -48,7 +48,7 @@ #define VRT_MAJOR_VERSION 19U -#define VRT_MINOR_VERSION 0U +#define VRT_MINOR_VERSION 1U /*********************************************************************** * Major and minor VRT API versions. @@ -58,6 +58,8 @@ * binary/load-time compatible, increment MAJOR version * * NEXT (2024-09-15) + * 19.1 (xxx) + * ObjWaitExtend() gained statep argument * 19.0 (2024-03-18) * [cache.h] (struct req).filter_list renamed to vdp_filter_list * order of vcl/vmod and director COLD events reversed to directors first