[master] e8c77d893 Add a director release callback to let go of all backends
Nils Goroll
nils.goroll at uplex.de
Mon Mar 6 15:19:07 UTC 2023
commit e8c77d893eab70debd1018c0b59aed89dab3db79
Author: Nils Goroll <nils.goroll at uplex.de>
Date: Mon Feb 27 22:14:53 2023 +0100
Add a director release callback to let go of all backends
Before this patch, layered directors needed to be destroyed top to
bottom, and whenever that order was missed, we would panic, because a
to-be-destroyed director still had references to it.
One special case where this issue would always trigger are looped
directors. Those should not be used and will cause havoc, which is a
separate issue #3899. But we should still be able to unconfigure such
a configuration.
We solve the destruction order issue by making it a two step process:
When a director is destroyed through VRT_DelDirector, a new release
function is called, which has to disassociate any backends. The
director then loses a reference, and when all references are gone, the
destroy function is called.
The new callback would not be necessary for the cases in varnish-cache
today, directors could simply disassociate any backends before calling
VRT_DelDirector. But this would complicate or even make impossible
transfer of director ownership, where the code responsible for
creating a director is not the same as the one calling
VRT_DelDirector(). As a side effect, it also helps clarity.
Fixes #3895
diff --git a/bin/varnishd/cache/cache_vrt_vcl.c b/bin/varnishd/cache/cache_vrt_vcl.c
index 01c660220..3ab464c07 100644
--- a/bin/varnishd/cache/cache_vrt_vcl.c
+++ b/bin/varnishd/cache/cache_vrt_vcl.c
@@ -289,6 +289,9 @@ VRT_DelDirector(VCL_BACKEND *dirp)
vdir = dir->vdir;
CHECK_OBJ_NOTNULL(vdir, VCLDIR_MAGIC);
+ if (vdir->methods->release != NULL)
+ vdir->methods->release(vdir->dir);
+
if (vdir->flags & VDIR_FLG_NOREFCNT) {
vdir->flags &= ~VDIR_FLG_NOREFCNT;
AZ(vcldir_deref(vdir));
diff --git a/bin/varnishtest/tests/r03895.vtc b/bin/varnishtest/tests/r03895.vtc
new file mode 100644
index 000000000..372b94513
--- /dev/null
+++ b/bin/varnishtest/tests/r03895.vtc
@@ -0,0 +1,48 @@
+
+
+varnishtest "looped backends"
+
+server s1 {
+} -start
+
+server s2 {
+} -start
+
+server s3 {
+} -start
+
+server s4 {
+} -start
+
+varnish v1 -vcl+backend {
+ import directors;
+ import std;
+
+ sub vcl_init {
+ new rr = directors.round_robin();
+ rr.add_backend(s1);
+ rr.add_backend(s2);
+ rr.add_backend(s3);
+ rr.add_backend(s4);
+ new rr2 = directors.round_robin();
+ rr2.add_backend(rr.backend());
+
+ rr.add_backend(rr2.backend());
+ }
+} -start
+
+varnish v1 -vcl+backend {
+ import directors;
+ import std;
+
+ sub vcl_init {
+ new rr2 = directors.round_robin();
+ rr2.add_backend(s1);
+ rr2.add_backend(s2);
+ rr2.add_backend(s3);
+ rr2.add_backend(s4);
+ }
+}
+
+varnish v1 -cliok "vcl.discard vcl1"
+varnish v1 -cliok "vcl.list"
diff --git a/doc/changes.rst b/doc/changes.rst
index fbda69fe8..d53f67986 100644
--- a/doc/changes.rst
+++ b/doc/changes.rst
@@ -91,6 +91,18 @@ Varnish Cache NEXT (2023-03-15)
* The ``VRT_new_backend_clustered()`` and ``VRT_new_backend()``
signatures have been changed
+* Directors which take and hold references to other directors via
+ ``VRT_Assign_Backend()`` (typically any director which has other
+ directors as backends) are now expected to implement the new
+ ``.release`` callback of type ``void
+ vdi_release_f(VCL_BACKEND)``. This function is called by
+ ``VRT_DelDirector()``. The implementation is expected drop any
+ backend references which the director holds (again using
+ ``VRT_Assign_Backend()`` with ``NULL`` as the second argument).
+
+ Failure to implement this callback can result in deadlocks, in
+ particular during VCL discard.
+
================================
Varnish Cache 7.2.0 (2022-09-15)
================================
diff --git a/doc/sphinx/whats-new/changes-trunk.rst b/doc/sphinx/whats-new/changes-trunk.rst
index 8333e2bde..fc1e56cdb 100644
--- a/doc/sphinx/whats-new/changes-trunk.rst
+++ b/doc/sphinx/whats-new/changes-trunk.rst
@@ -172,4 +172,13 @@ There is a new ``authority`` field for via backends in ``struct vrt_backend``.
There is a new ``exp_close`` field in ``struct vrt_backend_probe``.
+Directors which take and hold references to other directors via
+``VRT_Assign_Backend()`` (typically any director which has other
+directors as backends) are now expected to implement the new
+``.release`` callback of type ``void
+vdi_release_f(VCL_BACKEND)``. This function is called by
+``VRT_DelDirector()``. The implementation is expected drop any backend
+references which the director holds (again using
+``VRT_Assign_Backend()`` with ``NULL`` as the second argument).
+
*eof*
diff --git a/include/vrt.h b/include/vrt.h
index 5d01f542d..adb7713d5 100644
--- a/include/vrt.h
+++ b/include/vrt.h
@@ -64,6 +64,7 @@
* VRT_new_backend() signature changed
* VRT_new_backend_clustered() signature changed
* authority field added to struct vrt_backend
+ * release field added to struct vdi_methods
* 16.0 (2022-09-15)
* VMOD C-prototypes moved into JSON
* VRT_AddVDP() deprecated
@@ -692,6 +693,7 @@ typedef VCL_IP vdi_getip_f(VRT_CTX, VCL_BACKEND);
typedef void vdi_finish_f(VRT_CTX, VCL_BACKEND);
typedef stream_close_t vdi_http1pipe_f(VRT_CTX, VCL_BACKEND);
typedef void vdi_event_f(VCL_BACKEND, enum vcl_event_e);
+typedef void vdi_release_f(VCL_BACKEND);
typedef void vdi_destroy_f(VCL_BACKEND);
typedef void vdi_panic_f(VCL_BACKEND, struct vsb *);
typedef void vdi_list_f(VRT_CTX, VCL_BACKEND, struct vsb *, int, int);
@@ -707,6 +709,9 @@ struct vdi_methods {
vdi_getip_f *getip;
vdi_finish_f *finish;
vdi_event_f *event;
+ // called by VRT_DelDirector: deref all backends
+ vdi_release_f *release;
+ // when refcount goes 0
vdi_destroy_f *destroy;
vdi_panic_f *panic;
vdi_list_f *list;
diff --git a/vmod/vmod_directors.c b/vmod/vmod_directors.c
index 2130c4b55..7f429c9cc 100644
--- a/vmod/vmod_directors.c
+++ b/vmod/vmod_directors.c
@@ -92,17 +92,27 @@ vdir_new(VRT_CTX, struct vdir **vdp, const char *vcl_name,
AN(vd->healthy);
}
+void
+vdir_release(struct vdir *vd)
+{
+ unsigned u;
+
+ CHECK_OBJ_NOTNULL(vd, VDIR_MAGIC);
+
+ for (u = 0; u < vd->n_backend; u++)
+ VRT_Assign_Backend(&vd->backend[u], NULL);
+ vd->n_backend = 0;
+}
+
void
vdir_delete(struct vdir **vdp)
{
struct vdir *vd;
- unsigned u;
TAKE_OBJ_NOTNULL(vd, vdp, VDIR_MAGIC);
AZ(vd->dir);
- for (u = 0; u < vd->n_backend; u++)
- VRT_Assign_Backend(&vd->backend[u], NULL);
+ AZ(vd->n_backend);
free(vd->backend);
free(vd->weight);
AZ(pthread_rwlock_destroy(&vd->mtx));
diff --git a/vmod/vmod_directors.h b/vmod/vmod_directors.h
index 00b47b131..5cb13f67d 100644
--- a/vmod/vmod_directors.h
+++ b/vmod/vmod_directors.h
@@ -46,6 +46,7 @@ struct vdir {
void vdir_new(VRT_CTX, struct vdir **vdp, const char *vcl_name,
const struct vdi_methods *, void *priv);
+void vdir_release(struct vdir *vd);
void vdir_delete(struct vdir **vdp);
void vdir_rdlock(struct vdir *vd);
void vdir_wrlock(struct vdir *vd);
diff --git a/vmod/vmod_directors_fall_back.c b/vmod/vmod_directors_fall_back.c
index 0e5d159cb..b24e25e8e 100644
--- a/vmod/vmod_directors_fall_back.c
+++ b/vmod/vmod_directors_fall_back.c
@@ -175,6 +175,16 @@ vmod_fallback_resolve(VRT_CTX, VCL_BACKEND dir)
return (be);
}
+static void v_matchproto_(vdi_release_f)
+vmod_fallback_release(VCL_BACKEND dir)
+{
+ struct vmod_directors_fallback *fallback;
+
+ CHECK_OBJ_NOTNULL(dir, DIRECTOR_MAGIC);
+ CAST_OBJ_NOTNULL(fallback, dir->priv, VMOD_DIRECTORS_FALLBACK_MAGIC);
+ vdir_release(fallback->vd);
+}
+
static void v_matchproto_(vdi_destroy_f)
vmod_fallback_destroy(VCL_BACKEND dir)
{
@@ -191,6 +201,7 @@ static const struct vdi_methods vmod_fallback_methods[1] = {{
.type = "fallback",
.healthy = vmod_fallback_healthy,
.resolve = vmod_fallback_resolve,
+ .release = vmod_fallback_release,
.destroy = vmod_fallback_destroy,
.list = vmod_fallback_list
}};
diff --git a/vmod/vmod_directors_hash.c b/vmod/vmod_directors_hash.c
index cfa212b1c..fd8d42ab2 100644
--- a/vmod/vmod_directors_hash.c
+++ b/vmod/vmod_directors_hash.c
@@ -45,6 +45,16 @@ struct vmod_directors_hash {
struct vdir *vd;
};
+static void v_matchproto_(vdi_release_f)
+vmod_hash_release(VCL_BACKEND dir)
+{
+ struct vmod_directors_hash *hash;
+
+ CHECK_OBJ_NOTNULL(dir, DIRECTOR_MAGIC);
+ CAST_OBJ_NOTNULL(hash, dir->priv, VMOD_DIRECTORS_HASH_MAGIC);
+ vdir_release(hash->vd);
+}
+
static void v_matchproto_(vdi_destroy_f)
vmod_hash_destroy(VCL_BACKEND dir)
{
@@ -59,6 +69,7 @@ vmod_hash_destroy(VCL_BACKEND dir)
static const struct vdi_methods vmod_hash_methods[1] = {{
.magic = VDI_METHODS_MAGIC,
.type = "hash",
+ .release = vmod_hash_release,
.destroy = vmod_hash_destroy
}};
diff --git a/vmod/vmod_directors_random.c b/vmod/vmod_directors_random.c
index f58acd702..3d430609e 100644
--- a/vmod/vmod_directors_random.c
+++ b/vmod/vmod_directors_random.c
@@ -86,6 +86,16 @@ vmod_random_resolve(VRT_CTX, VCL_BACKEND dir)
return (be);
}
+static void v_matchproto_(vdi_release_f)
+vmod_random_release(VCL_BACKEND dir)
+{
+ struct vmod_directors_random *random;
+
+ CHECK_OBJ_NOTNULL(dir, DIRECTOR_MAGIC);
+ CAST_OBJ_NOTNULL(random, dir->priv, VMOD_DIRECTORS_RANDOM_MAGIC);
+ vdir_release(random->vd);
+}
+
static void v_matchproto_(vdi_destroy_f)
vmod_random_destroy(VCL_BACKEND dir)
{
@@ -102,6 +112,7 @@ static const struct vdi_methods vmod_random_methods[1] = {{
.type = "random",
.healthy = vmod_random_healthy,
.resolve = vmod_random_resolve,
+ .release = vmod_random_release,
.destroy = vmod_random_destroy,
.list = vmod_random_list
}};
diff --git a/vmod/vmod_directors_round_robin.c b/vmod/vmod_directors_round_robin.c
index 1cc6ac82d..015e060da 100644
--- a/vmod/vmod_directors_round_robin.c
+++ b/vmod/vmod_directors_round_robin.c
@@ -96,6 +96,16 @@ vmod_rr_resolve(VRT_CTX, VCL_BACKEND dir)
return (be);
}
+static void v_matchproto_(vdi_release_f)
+vmod_rr_release(VCL_BACKEND dir)
+{
+ struct vmod_directors_round_robin *rr;
+
+ CHECK_OBJ_NOTNULL(dir, DIRECTOR_MAGIC);
+ CAST_OBJ_NOTNULL(rr, dir->priv, VMOD_DIRECTORS_ROUND_ROBIN_MAGIC);
+ vdir_release(rr->vd);
+}
+
static void v_matchproto_(vdi_destroy_f)
vmod_rr_destroy(VCL_BACKEND dir)
{
@@ -112,6 +122,7 @@ static const struct vdi_methods vmod_rr_methods[1] = {{
.type = "round-robin",
.healthy = vmod_rr_healthy,
.resolve = vmod_rr_resolve,
+ .release = vmod_rr_release,
.destroy = vmod_rr_destroy,
.list = vmod_rr_list
}};
diff --git a/vmod/vmod_directors_shard.c b/vmod/vmod_directors_shard.c
index bdd3e0134..8ef2a4f91 100644
--- a/vmod/vmod_directors_shard.c
+++ b/vmod/vmod_directors_shard.c
@@ -205,6 +205,15 @@ shard__assert(void)
assert(t2a == t2b);
}
+static void v_matchproto_(vdi_release_f)
+vmod_shard_release(VCL_BACKEND dir)
+{
+ struct sharddir *shardd;
+
+ CAST_OBJ_NOTNULL(shardd, dir->priv, SHARDDIR_MAGIC);
+ sharddir_release(shardd);
+}
+
static void v_matchproto_(vdi_destroy_f)
vmod_shard_destroy(VCL_BACKEND dir)
{
@@ -219,6 +228,7 @@ static const struct vdi_methods vmod_shard_methods[1] = {{
.type = "shard",
.resolve = vmod_shard_resolve,
.healthy = vmod_shard_healthy,
+ .release = vmod_shard_release,
.destroy = vmod_shard_destroy,
.list = vmod_shard_list
}};
diff --git a/vmod/vmod_directors_shard_cfg.c b/vmod/vmod_directors_shard_cfg.c
index 35ecfa9c1..aec4a0901 100644
--- a/vmod/vmod_directors_shard_cfg.c
+++ b/vmod/vmod_directors_shard_cfg.c
@@ -449,7 +449,7 @@ shardcfg_backend_add(struct backend_reconfig *re,
bb[i].replicas = replicas;
}
-static void
+void
shardcfg_backend_clear(struct sharddir *shardd)
{
unsigned i;
@@ -675,10 +675,8 @@ shardcfg_reconfigure(VRT_CTX, struct sharddir *shardd, VCL_INT replicas)
void
shardcfg_delete(const struct sharddir *shardd)
{
- unsigned i;
- for (i = 0; i < shardd->n_backend; i++)
- shardcfg_backend_free(&shardd->backend[i]);
+ AZ(shardd->n_backend);
if (shardd->backend)
free(shardd->backend);
if (shardd->hashcircle)
diff --git a/vmod/vmod_directors_shard_dir.c b/vmod/vmod_directors_shard_dir.c
index f587cd19c..cedfcdcd7 100644
--- a/vmod/vmod_directors_shard_dir.c
+++ b/vmod/vmod_directors_shard_dir.c
@@ -204,6 +204,13 @@ sharddir_set_param(struct sharddir *shardd,
shardd->param = param;
}
+void
+sharddir_release(struct sharddir *shardd)
+{
+ CHECK_OBJ_NOTNULL(shardd, SHARDDIR_MAGIC);
+ shardcfg_backend_clear(shardd);
+}
+
void
sharddir_delete(struct sharddir **sharddp)
{
diff --git a/vmod/vmod_directors_shard_dir.h b/vmod/vmod_directors_shard_dir.h
index d3cc52998..fdf475144 100644
--- a/vmod/vmod_directors_shard_dir.h
+++ b/vmod/vmod_directors_shard_dir.h
@@ -112,6 +112,7 @@ void sharddir_new(struct sharddir **sharddp, const char *vcl_name,
const struct vmod_directors_shard_param *param);
void sharddir_set_param(struct sharddir *shardd,
const struct vmod_directors_shard_param *param);
+void sharddir_release(struct sharddir *shardd);
void sharddir_delete(struct sharddir **sharddp);
void sharddir_rdlock(struct sharddir *shardd);
void sharddir_wrlock(struct sharddir *shardd);
@@ -121,5 +122,6 @@ VCL_BACKEND sharddir_pick_be(VRT_CTX, struct sharddir *, uint32_t, VCL_INT,
VCL_REAL, VCL_BOOL, VCL_ENUM healthy);
/* in shard_cfg.c */
+void shardcfg_backend_clear(struct sharddir *shardd);
void shardcfg_delete(const struct sharddir *shardd);
VCL_DURATION shardcfg_get_rampup(const struct sharddir *shardd, unsigned host);
More information about the varnish-commit
mailing list