[master] 77110d7bd Return a consistent boc state from ObjWaitExtend()
Nils Goroll
nils.goroll at uplex.de
Mon May 27 15:59:02 UTC 2024
commit 77110d7bda11e7feddd89a58b5aaa0e0c77159b5
Author: Nils Goroll <nils.goroll at uplex.de>
Date: Tue May 21 17:16:05 2024 +0200
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.
diff --git a/bin/varnishd/cache/cache_obj.c b/bin/varnishd/cache/cache_obj.c
index ec819bf8c..21c3d8f91 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 add300e76..9bd5e2490 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 6626d3a0d..68c780552 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 139d4c93d..29a409b6c 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.
+
* for backends using the ``.via`` attribute to connect through a
*proxy*, the ``connect_timeout``, ``first_byte_timeout`` and
``between_bytes_timeout`` attributes are now inherited from *proxy*
diff --git a/include/vrt.h b/include/vrt.h
index a7ed22bab..707035497 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 (2024-05-27)
+ * [cache_varnishd.h] 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
More information about the varnish-commit
mailing list