From c880c32b16dfd633aa7a8290b74dd46fc4f004cb Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 18 Mar 2025 11:24:56 +0100 Subject: [PATCH] MINOR: stream: decrement srv->served after detaching from the list In commit 3372a2ea00 ("BUG/MEDIUM: queues: Stricly respect maxconn for outgoing connections"), it has been ensured that srv->served is held as long as possible around the periods where a stream is attached to a server. However, it's decremented early when entering sess_change_server, and actually just before detaching from that server's list. While there is theoretically nothing wrong with this, it prevents us from looking at this counter to know if streams are still using a server or not. We could imagine decrementing it much later but that wouldn't work with leastconn, since that algo needs ->served to be final before calling lbprm.server_drop_conn(). Thus what we're doing here is to detach from the server, then decrement ->served, and only then call the LB callback to update the server's position in the tree. At this moment the stream doesn't know the server anymore anyway (except via this function's local variable) so it's safe to consider that no stream knows the server once the variable reaches zero. --- src/stream.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/stream.c b/src/stream.c index 2f3a381af..78a5b7e63 100644 --- a/src/stream.c +++ b/src/stream.c @@ -2749,22 +2749,32 @@ void sess_change_server(struct stream *strm, struct server *newsrv) * invocation of sess_change_server(). */ - /* - * It is assumed if the stream has a non-NULL srv_conn, then its - * served field has been incremented, so we have to decrement it now. - */ - if (oldsrv) - _HA_ATOMIC_DEC(&oldsrv->served); - - if (oldsrv == newsrv) + if (oldsrv == newsrv) { + /* + * It is assumed if the stream has a non-NULL srv_conn, then its + * served field has been incremented, so we have to decrement it now. + */ + if (oldsrv) + _HA_ATOMIC_DEC(&oldsrv->served); return; + } if (oldsrv) { + /* Note: we cannot decrement served after calling server_drop_conn + * because that one may rely on served (e.g. for leastconn). The + * real need here is to make sure that when served==0, no stream + * knows the server anymore, mainly for server removal purposes, + * and that removal will be done under isolation anyway. Thus by + * decrementing served after detaching from the list, we're + * guaranteeing that served==0 implies that no stream is in the + * list anymore, which is a sufficient guarantee for tha goal. + */ _HA_ATOMIC_DEC(&oldsrv->proxy->served); + stream_del_srv_conn(strm); + _HA_ATOMIC_DEC(&oldsrv->served); __ha_barrier_atomic_store(); if (oldsrv->proxy->lbprm.server_drop_conn) oldsrv->proxy->lbprm.server_drop_conn(oldsrv); - stream_del_srv_conn(strm); } if (newsrv) { -- 2.39.5