[master] 4e2c50da9 Serialize VCL temperature transitions with VRT_AddDirector()

[Nils Goroll]

commit 4e2c50da9b4b66a39191dcc7ea11bc4e3d11ab6c
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Wed Jul 31 14:20:30 2024 +0200

    Serialize VCL temperature transitions with VRT_AddDirector()
    
    VRT_AddDirector() reads the temperature under vcl_mtx and only warms up a newly
    created director for a warm VCL.
    
    vcl_set_state, when transitioning to a cold temperature, implicitly assumes that
    all directors are warm, as it sends a cold event after changing the temperature.
    
    Before this commit, we had no guarantee which temperature VRT_AddDirector()
    would read while racing with vcl_set_state, it could really be any except
    VCL_TEMP_INIT.
    
    By adding this serialization on vcl_mtx, we make sure that the critical region
    of VRT_AddDirector executes either before the temperature change or after. If
    before, a warm event is generated for the newly added director, followed by a
    cold event from vcl_set_state. If after, VRT_AddDirector() should find the vcl
    temperature as COOLING and not add a backend (backround: VRT_AddDirector()
    should only be called while holding a vcl reference via
    VRT_VCL_Prevent_{Cold,Discard}()).
    
    The second change, to also momentarily hold vcl_mtx for the transition to a WARM
    vcl, is not to prevent a known race, but rather to ensure visibility of the new
    temperature across all threads.
    
    I _suspect_ that this change might also fix
    https://github.com/nigoroll/libvmod-dynamic/issues/117 , but due to lack of a
    reproducer, this is speculative. Besides the visibility issue, another potential
    reason is that vcl_set_state could also race VCL_Rel() (likely called via
    VRT_VCL_Allow_{Cold,Discard}()).

diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index 897563b09..262b48538 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -602,8 +602,10 @@ vcl_set_state(struct vcl *vcl, const char *state, struct vsb **msg)
 		if (vcl->temp == VCL_TEMP_COLD)
 			break;
 		if (vcl->busy == 0 && vcl->temp->is_warm) {
+			Lck_Lock(&vcl_mtx);
 			vcl->temp = VTAILQ_EMPTY(&vcl->ref_list) ?
 			    VCL_TEMP_COLD : VCL_TEMP_COOLING;
+			Lck_Unlock(&vcl_mtx);
 			vcl_BackendEvent(vcl, VCL_EVENT_COLD);
 			AZ(vcl_send_event(vcl, VCL_EVENT_COLD, msg));
 			AZ(*msg);
@@ -627,7 +629,9 @@ vcl_set_state(struct vcl *vcl, const char *state, struct vsb **msg)
 			i = -1;
 		}
 		else {
+			Lck_Lock(&vcl_mtx);
 			vcl->temp = VCL_TEMP_WARM;
+			Lck_Unlock(&vcl_mtx);
 			i = vcl_send_event(vcl, VCL_EVENT_WARM, msg);
 			if (i == 0) {
 				vcl_BackendEvent(vcl, VCL_EVENT_WARM);