Skip to content

Commit

Permalink
Serialize VCL temperature transitions with VRT_AddDirector()
Browse files Browse the repository at this point in the history
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
nigoroll/libvmod-dynamic#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}()).
  • Loading branch information
nigoroll committed Jul 31, 2024
1 parent d43e225 commit 4e2c50d
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions bin/varnishd/cache/cache_vcl.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down

0 comments on commit 4e2c50d

Please sign in to comment.