Mailing List Archive

commit 89c1688f8845f7630dcc0955ed748861156d74c2
Author: Nils Goroll <nils.goroll@uplex.de>
Date: Tue Oct 31 13:46:37 2023 +0100

Call trimstore only once when copying the body after a 304

It is my understanding that the objtrimstore stevedore API function is
only to be called once when the object is complete, which I believe is
also in line with the comment on ObjExtend() that "The final flag must
be set on the last call".

If this understanding of the API is correct, we did not adhere to it in
the fetch code when we made a copy of an existing stale object after a
304 response: There, we iterated over the stale object and did not set
the final flag just once when the object was complete, but rather after
each storage segment was copied.

This commit fixes this, adds some pedentry to the simple storage and
extends b00062.vtc to test this behavior specifically. On top, g6.vtc
also triggered without the fix but the duplicate trim detection in place.

This issue has originally surfaced in the SLASH/fellow storage where the
trimstore function implicitly asserted to only be called once.

Ref 115742b07c8bad6d465f1c981ee264f934a4492b
Ref https://gitlab.com/uplex/varnish/slash/-/issues/33

diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 55c14f1d4..e365f6c88 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -769,7 +769,8 @@ vbf_objiterate(void *priv, unsigned flush, const void *ptr, ssize_t len)
uint8_t *pd;

CAST_OBJ_NOTNULL(bo, priv, BUSYOBJ_MAGIC);
- (void)flush;
+
+ flush &= OBJ_ITER_END;

while (len > 0) {
l = len;
@@ -777,7 +778,7 @@ vbf_objiterate(void *priv, unsigned flush, const void *ptr, ssize_t len)
return (1);
l = vmin(l, len);
memcpy(pd, ps, l);
- VFP_Extend(bo->vfc, l, l == len ? VFP_END : VFP_OK);
+ VFP_Extend(bo->vfc, l, flush && l == len ? VFP_END : VFP_OK);
ps += l;
len -= l;
}
diff --git a/bin/varnishd/storage/storage_simple.c b/bin/varnishd/storage/storage_simple.c
index c94dcccb4..b91fbcb32 100644
--- a/bin/varnishd/storage/storage_simple.c
+++ b/bin/varnishd/storage/storage_simple.c
@@ -44,6 +44,9 @@
/* Flags for allocating memory in sml_stv_alloc */
#define LESS_MEM_ALLOCED_IS_OK 1

+// marker pointer for sml_trimstore
+static void *trim_once = &trim_once;
+
/*-------------------------------------------------------------------*/

static struct storage *
@@ -257,7 +260,8 @@ sml_bocfini(const struct stevedore *stv, struct boc *boc)
CHECK_OBJ_NOTNULL(stv, STEVEDORE_MAGIC);
CHECK_OBJ_NOTNULL(boc, BOC_MAGIC);

- if (boc->stevedore_priv == NULL)
+ if (boc->stevedore_priv == NULL ||
+ boc->stevedore_priv == trim_once)
return;

/* Free any leftovers from Trim */
@@ -533,6 +537,10 @@ sml_trimstore(struct worker *wrk, struct objcore *oc)
stv = oc->stobj->stevedore;
CHECK_OBJ_NOTNULL(stv, STEVEDORE_MAGIC);

+ if (oc->boc->stevedore_priv != NULL)
+ WRONG("sml_trimstore already called");
+ oc->boc->stevedore_priv = trim_once;
+
if (stv->sml_free == NULL)
return;

@@ -566,7 +574,6 @@ sml_trimstore(struct worker *wrk, struct objcore *oc)
VTAILQ_INSERT_HEAD(&o->list, st1, list);
Lck_Unlock(&oc->boc->mtx);
/* sml_bocdone frees this */
- AZ(oc->boc->stevedore_priv);
oc->boc->stevedore_priv = st;
}

diff --git a/bin/varnishtest/tests/b00062.vtc b/bin/varnishtest/tests/b00062.vtc
index 511a4b416..4a828a53b 100644
--- a/bin/varnishtest/tests/b00062.vtc
+++ b/bin/varnishtest/tests/b00062.vtc
@@ -2,7 +2,11 @@ varnishtest "Test that we properly wait for certain 304 cases"

server s1 {
rxreq
- txresp -hdr "Last-Modified: Wed, 11 Sep 2013 13:36:55 GMT" -body "Geoff Still Rules"
+ txresp -hdr "Last-Modified: Wed, 11 Sep 2013 13:36:55 GMT" \
+ -hdr "Geoff: Still Rules" \
+ -bodylen 130560
+
+ # 2*64k-512 ^^^ see sml_trimstore() st->space - st->len < 512

# The IMS request we will spend some time to process for the sake of
# this test.
@@ -17,7 +21,7 @@ server s1 {
txresp -body "x"
} -start

-varnish v1 -vcl+backend {
+varnish v1 -arg "-p fetch_maxchunksize=64k" -vcl+backend {
sub vcl_backend_response {
set beresp.ttl = 1s;
set beresp.grace = 1s;
@@ -30,7 +34,8 @@ client c1 {
txreq
rxresp
expect resp.status == 200
- expect resp.body == "Geoff Still Rules"
+ expect resp.http.Geoff == "Still Rules"
+ expect resp.bodylen == 130560
} -run

# let the object's ttl and grace expire
@@ -42,7 +47,8 @@ client c2 {
rxresp
# we did not disable grace in the request, so we should get the graced object here
expect resp.status == 200
- expect resp.body == "Geoff Still Rules"
+ expect resp.http.Geoff == "Still Rules"
+ expect resp.bodylen == 130560
} -start

delay .1
@@ -52,7 +58,8 @@ client c3 {
txreq
rxresp
expect resp.status == 200
- expect resp.body == "Geoff Still Rules"
+ expect resp.http.Geoff == "Still Rules"
+ expect resp.bodylen == 130560
} -start

client c2 -wait
_______________________________________________
varnish-commit mailing list
varnish-commit@varnish-cache.org
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit