From: Willy Tarreau Date: Wed, 16 Jun 2021 06:42:23 +0000 (+0200) Subject: BUG/MAJOR: queue: set SF_ASSIGNED when setting strm->target on dequeue X-Git-Tag: v2.5-dev1~110 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7867cebf31403b6d7bab97cb3047e6d77238fcdc;p=thirdparty%2Fhaproxy.git BUG/MAJOR: queue: set SF_ASSIGNED when setting strm->target on dequeue Commit 82cd5c13a ("OPTIM: backend: skip LB when we know the backend is full") has uncovered a long-burried bug in the dequeing code: when a server releases a connection, it picks a new one from the proxy's or its queue. Technically speaking it only picks a pendconn which is a link between a position in the queue and a stream. It then sets this pendconn's target to itself, and wakes up the stream's task so that it can try to connect again. The stream then goes through the regular connection setup phases, calls back_try_conn_req() which calls pendconn_dequeue(), which sets the stream's target to the pendconn's and releases the pendconn. It then reaches assign_server() which sees no SF_ASSIGNED and calls assign_server_and_queue() to perform load balancing or queuing. This one first destroys the stream's target and gets ready to perform load balancing. At this point we're load-balancing for no reason since we already knew what server was available. And this is where the commit above comes into play: the check for the backend's queue above may detect other connections that arrived in between, and will immediately return FULL, forcing this request back into the queue. If the server had a very low maxconn (e.g. 1 due to a long slowstart), it's possible that this evicted connection was the last one on the server and that no other one will ever be present to process the queue. Usually a regularly processed request will still have its own srv_conn that will be used during stream_free() to dequeue other connections. But if the server had a down-up cycle, then a call to pendconn_grab_from_px() may start to dequeue entries which had no srv_conn and which will have no server slot to offer when they expire, thus maintaining the situation above forever. Worse, as new requests arrive, there are always some requests in the queue and the situation feeds on itself. The correct fix here is to properly set SF_ASSIGNED in pendconn_dequeue() when the stream's target is assigned (as it's what this flag means), so as to avoid a load-balancing pass when dequeuing. Many thanks to Pierre Cheynier for the numerous detailed traces he provided that helped narrow this problem down. This could be backported to all stable versions, but in practice only 2.3 and above are really affected since the presence of the commit above. Given how tricky this code is it's better to limit it to those versions that really need it. --- diff --git a/src/queue.c b/src/queue.c index 1fadd10081..6ff3404413 100644 --- a/src/queue.c +++ b/src/queue.c @@ -555,11 +555,15 @@ int pendconn_dequeue(struct stream *strm) /* the pendconn is not queued anymore and will not be so we're safe * to proceed. */ - if (p->target) - strm->target = &p->target->obj_type; - strm->flags &= ~(SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET); strm->flags |= p->strm_flags & (SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET); + + if (p->target) { + /* a server picked this pendconn, it must skip LB */ + strm->target = &p->target->obj_type; + strm->flags |= SF_ASSIGNED; + } + strm->pend_pos = NULL; pool_free(pool_head_pendconn, p); return 0;