Workaround for Varnish-Cache VC#4013: Wrong trim use, inefficient copy

https://github.com/varnishcache/varnish-cache/pull/4013 fixes two
issues in Varnish-Cache, which are relevant for SLASH/fellow and of
which the first is the root cause of #33.

This commit works around these issues until the fix gets merged:

Because of the wrong use of the .objtrimstore API function by
varnish-cache, we remove it from our obj_methods and exploit the fact
that varnish-cache always sets the OA_LEN attribute when the object is
complete: We move the trimstore function there, effectively calling it
at the right time only.

The inefficient memory allocation fixed in the second commit of
VC#4013 is particularly relevant for fellow, because it causes the
allocation code to assume that the object might grow up to the maximum
possible size, which causes a substantial over-allocation. We work
around this issue for the case that a 304 copy is made from fellow to
fellow by using private thread-local storage to emulate basically the
same function as the #4013 fix.

Closes #33
Ref https://github.com/varnishcache/varnish-cache/pull/4013

src/fellow_cache.c

@@ -4254,6 +4254,10 @@ fellow_busy_obj_trimstore(struct fellow_busy *fbo)
 	fcsl = fbo->body_seglist;
 	CHECK_OBJ_NOTNULL(fcsl, FELLOW_CACHE_SEGLIST_MAGIC);
 	fcs = fbo->body_seg;
+#ifdef VC_4013_WORKAROUND
+	if (fcs == NULL)
+		return;
+#endif
 	CHECK_OBJ_NOTNULL(fcs, FELLOW_CACHE_SEG_MAGIC);
 	fc = fbo->fc;
 	CHECK_OBJ_NOTNULL(fc, FELLOW_CACHE_MAGIC);

src/fellow_cache_storage.h

@@ -46,3 +46,10 @@ fib(uint64_t n, uint8_t bits)
 	assert(r < (size_t)1 << bits);
 	return (r);
 }
+
+/*
+ * XXX to be removed when
+ * https://github.com/varnishcache/varnish-cache/pull/4013
+ * is merged
+ */
+#define VC_4013_WORKAROUND

src/fellow_storage.c

@@ -777,6 +777,41 @@ fellow_stream_f(void *priv, unsigned flush, const void *ptr, ssize_t len)
 	return (r);
 }
 
+#ifdef VC_4013_WORKAROUND
+/* to avoid the inefficient allocation if we copy a fellow object for 304, we
+ * store the length remaining from OA_LEN and use it instead */
+static pthread_key_t vc4013_key;
+static void __attribute__((constructor))
+init_vc4013(void)
+{
+	PTOK(pthread_key_create(&vc4013_key, NULL));
+}
+
+struct vc4013 {
+	unsigned	magic;
+#define VC4013_MAGIC	0xaf1f05b9
+	struct boc	*boc;
+	// not yet allocated
+	size_t		l;
+	// current allocation
+	uint8_t		*p;
+	size_t		pl;
+};
+
+// do do {} while() because the local variable needs to be in scope
+#define VC4013_INIT(wrk, oc)				\
+	struct vc4013 hack[1];				\
+	INIT_OBJ(hack, VC4013_MAGIC);			\
+	hack->l = ObjGetLen(wrk, oc);			\
+	assert(hack->l > 0);				\
+	PTOK(pthread_setspecific(vc4013_key, hack));
+#define VC4013_FINI() PTOK(pthread_setspecific(vc4013_key, NULL))
+#else
+#define VC4013_INIT(wrk, oc) (void)0
+#define VC4013_FINI() (void)0
+#endif
+
+
 static int v_matchproto_(objiterator_f)
 sfemem_iterator(struct worker *wrk, struct objcore *oc,
     void *priv, objiterate_f *func, int final)
@@ -792,7 +827,9 @@ sfemem_iterator(struct worker *wrk, struct objcore *oc,
 	boc = HSH_RefBoc(oc);
 
 	if (boc == NULL) {
+		VC4013_INIT(wrk, oc);
 		fcr = fellow_cache_obj_iter(stvfe->fc, fco, priv, func, final);
+		VC4013_FINI();
 		return (stvfe_fcr_handle_iter(wrk, oc, stv, fcr));
 	}
 
@@ -839,7 +876,9 @@ sfedsk_iterator(struct worker *wrk, struct objcore *dskoc,
 
 	AZ(HSH_RefBoc(dskoc));
 
+	VC4013_INIT(wrk, dskoc);
 	fcr = fellow_cache_obj_iter(stvfe->fc, fcoc.fco, priv, func, final);
+	VC4013_FINI();
 	fcoc_fini(stvfe->fc, &fcoc);
 	return (stvfe_fcr_handle_iter(wrk, dskoc, stv, fcr));
 }
@@ -868,7 +907,44 @@ sfemem_getspace(struct worker *wrk, struct objcore *oc, ssize_t *ssz,
 	assert(*ssz > 0);
 	sz = (size_t)*ssz;
 
+#ifdef VC_4013_WORKAROUND
+	struct vc4013 *hack;
+	struct boc *boc;
+
+	CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
+	boc = oc->boc;
+	CHECK_OBJ_NOTNULL(boc, BOC_MAGIC);
+
+	hack = pthread_getspecific(vc4013_key);
+	if (hack != NULL && (hack->boc == NULL || hack->boc == boc)) {
+		CHECK_OBJ(hack, VC4013_MAGIC);
+		hack->boc = boc;
+		AN(hack->l);
+		fcr.status = fcr_ok;
+		if (hack->pl == 0) {
+			hack->p = NULL;
+			hack->pl = hack->l;
+			fcr = fellow_busy_obj_getspace(sfe_fbo(oc),
+			    &hack->pl, &hack->p);
+			if (fcr.status != fcr_ok)
+				goto out;
+			if (hack->pl < hack->l)
+				hack->l -= hack->pl;
+			else
+				hack->l = 0;
+		}
+		AN(hack->pl);
+		AN(hack->p);
+		*ptr = hack->p;
+		sz = vmin(hack->pl, sz);
+		hack->p += sz;
+		hack->pl -= sz;
+		goto out;
+	}
+#endif
+
 	fcr = fellow_busy_obj_getspace(sfe_fbo(oc), &sz, ptr);
+  out:
 	if (fcr.status != fcr_ok) {
 		VSLb(wrk->vsl, SLT_Error, "GetSpace: %s",
 		    fcr.r.err ? fcr.r.err : "Unknown error");
@@ -889,6 +965,7 @@ sfemem_extend(struct worker *wrk, struct objcore *oc, ssize_t l)
 	fellow_busy_obj_extend(sfe_fbo(oc), (size_t)l);
 }
 
+#ifndef VC_4013_WORKAROUND
 static void v_matchproto_(objtrimstore_f)
 sfemem_trimstore(struct worker *wrk, struct objcore *oc)
 {
@@ -897,6 +974,7 @@ sfemem_trimstore(struct worker *wrk, struct objcore *oc)
 
 	fellow_busy_obj_trimstore(sfe_fbo(oc));
 }
+#endif
 
 static const uint16_t oa_inlog_bit = 1 << (int)OA_INLOG;
 
@@ -1074,6 +1152,13 @@ sfemem_setattr(struct worker *wrk, struct objcore *memoc, enum obj_attr attr,
 	assert_memstv(stvfe, stv);
 	assert(len >= 0);
 
+#ifdef VC_4013_WORKAROUND
+	if (attr == OA_LEN) {
+		VSLb(wrk->vsl, SLT_Debug, "OA_LEN trim oc %p", memoc);
+		fellow_busy_obj_trimstore(sfe_fbo(memoc));
+	}
+#endif
+
 	return (fellow_busy_setattr(sfe_fbo(memoc), attr, (size_t)len, ptr));
 }
 
@@ -1108,7 +1193,11 @@ static const struct obj_methods sfemem_methods = {
 	.objgetattr	= sfemem_getattr,
 	.objsetattr	= sfemem_setattr,
 	// optional
+#ifndef VC_4013_WORKAROUND
 	.objtrimstore	= sfemem_trimstore,
+#else
+	.objtrimstore	= NULL,
+#endif
 	.objbocdone	= sfemem_bocdone,
 	.objslim	= sfemem_objslim,
 	.objtouch	= sfemem_touch,