BUG/MEDIUM: lb/threads: always properly lock LB algorithms on maintenance operations
Since commit 3ff577e ("MAJOR: server: make server state changes
synchronous again"), srv_update_status() calls the various maintenance
operations of the LB algorithms (->set_server_up, ->set_server_down,
->update_server_weight()). These ones are called with a single thread
guaranteed by the rendez-vous point, so the fact that they're lacking
some locks has no effect. However we'll need to remove the rendez-vous
point so we have to take care of properly locking all the LB algos.
The comments have been properly updated on the various functions to
mention their locking expectations. All these functions are called
with the server lock held, and all of them now support concurrent
calls by using the lbprm's lock.
This fix doesn't need to be backported at the moment, though if any
check-specific issue surfaced in 1.8, it could make sense to reuse it.
diff --git a/src/lb_chash.c b/src/lb_chash.c
index e3bf65d..7e188ac 100644
--- a/src/lb_chash.c
+++ b/src/lb_chash.c
@@ -66,6 +66,8 @@
* as many times as its weight indicates it. If it's there too often, we remove
* the last occurrences. If it's not there enough, we add more occurrences. To
* remove a server from the tree, normally call this with eweight=0.
+ *
+ * The server's lock and the lbprm's lock must be held.
*/
static inline void chash_queue_dequeue_srv(struct server *s)
{
@@ -112,6 +114,8 @@
* It is not important whether the server was already down or not. It is not
* important either that the new state is completely down (the caller may not
* know all the variables of a server's state).
+ *
+ * The server's lock must be held. The lbprm lock will be used.
*/
static void chash_set_server_status_down(struct server *srv)
{
@@ -120,6 +124,8 @@
if (!srv_lb_status_changed(srv))
return;
+ HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock);
+
if (srv_willbe_usable(srv))
goto out_update_state;
@@ -155,6 +161,8 @@
update_backend_weight(p);
out_update_state:
srv_lb_commit_status(srv);
+
+ HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock);
}
/* This function updates the server trees according to server <srv>'s new
@@ -163,6 +171,8 @@
* important either that the new state is completely UP (the caller may not
* know all the variables of a server's state). This function will not change
* the weight of a server which was already up.
+ *
+ * The server's lock must be held. The lbprm lock will be used.
*/
static void chash_set_server_status_up(struct server *srv)
{
@@ -171,6 +181,8 @@
if (!srv_lb_status_changed(srv))
return;
+ HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock);
+
if (!srv_willbe_usable(srv))
goto out_update_state;
@@ -211,10 +223,14 @@
update_backend_weight(p);
out_update_state:
srv_lb_commit_status(srv);
+
+ HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock);
}
/* This function must be called after an update to server <srv>'s effective
* weight. It may be called after a state change too.
+ *
+ * The server's lock must be held. The lbprm lock may be used.
*/
static void chash_update_server_weight(struct server *srv)
{
@@ -248,6 +264,8 @@
return;
}
+ HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock);
+
/* only adjust the server's presence in the tree */
chash_queue_dequeue_srv(srv);
@@ -258,6 +276,8 @@
update_backend_weight(p);
srv_lb_commit_status(srv);
+
+ HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock);
}
/*
@@ -303,21 +323,29 @@
unsigned int dn, dp;
int loop;
+ HA_SPIN_LOCK(LBPRM_LOCK, &p->lbprm.lock);
+
if (p->srv_act)
root = &p->lbprm.chash.act;
- else if (p->lbprm.fbck)
- return p->lbprm.fbck;
+ else if (p->lbprm.fbck) {
+ nsrv = p->lbprm.fbck;
+ goto out;
+ }
else if (p->srv_bck)
root = &p->lbprm.chash.bck;
- else
- return NULL;
+ else {
+ nsrv = NULL;
+ goto out;
+ }
/* find the node after and the node before */
next = eb32_lookup_ge(root, hash);
if (!next)
next = eb32_first(root);
- if (!next)
- return NULL; /* tree is empty */
+ if (!next) {
+ nsrv = NULL; /* tree is empty */
+ goto out;
+ }
prev = eb32_prev(next);
if (!prev)
@@ -349,6 +377,8 @@
nsrv = eb32_entry(next, struct tree_occ, node)->server;
}
+ out:
+ HA_SPIN_UNLOCK(LBPRM_LOCK, &p->lbprm.lock);
return nsrv;
}