|
|
- commit 0ff395c154ad827c0c30eefc9371ba7f7c171027
- Author: Willy Tarreau <w@1wt.eu>
- Date: Tue Jul 30 11:59:34 2019 +0200
-
- BUG/MAJOR: queue/threads: avoid an AB/BA locking issue in process_srv_queue()
-
- A problem involving server slowstart was reported by @max2k1 in issue #197.
- The problem is that pendconn_grab_from_px() takes the proxy lock while
- already under the server's lock while process_srv_queue() first takes the
- proxy's lock then the server's lock.
-
- While the latter seems more natural, it is fundamentally incompatible with
- mayn other operations performed on servers, namely state change propagation,
- where the proxy is only known after the server and cannot be locked around
- the servers. Howwever reversing the lock in process_srv_queue() is trivial
- and only the few functions related to dynamic cookies need to be adjusted
- for this so that the proxy's lock is taken for each server operation. This
- is possible because the proxy's server list is built once at boot time and
- remains stable. So this is what this patch does.
-
- The comments in the proxy and server structs were updated to mention this
- rule that the server's lock may not be taken under the proxy's lock but
- may enclose it.
-
- Another approach could consist in using a second lock for the proxy's queue
- which would be different from the regular proxy's lock, but given that the
- operations above are rare and operate on small servers list, there is no
- reason for overdesigning a solution.
-
- This fix was successfully tested with 10000 servers in a backend where
- adjusting the dyncookies in loops over the CLI didn't have a measurable
- impact on the traffic.
-
- The only workaround without the fix is to disable any occurrence of
- "slowstart" on server lines, or to disable threads using "nbthread 1".
-
- This must be backported as far as 1.8.
-
- (cherry picked from commit 5e83d996cf965ee5ac625f702a446f4d8c80a220)
- Signed-off-by: Willy Tarreau <w@1wt.eu>
-
- diff --git a/include/types/proxy.h b/include/types/proxy.h
- index ca24dbfe..2518f88d 100644
- --- a/include/types/proxy.h
- +++ b/include/types/proxy.h
- @@ -487,7 +487,7 @@ struct proxy {
- * name is used
- */
- struct list filter_configs; /* list of the filters that are declared on this proxy */
- - __decl_hathreads(HA_SPINLOCK_T lock);
- + __decl_hathreads(HA_SPINLOCK_T lock); /* may be taken under the server's lock */
- };
-
- struct switching_rule {
- diff --git a/include/types/server.h b/include/types/server.h
- index 4a077268..e0534162 100644
- --- a/include/types/server.h
- +++ b/include/types/server.h
- @@ -319,7 +319,7 @@ struct server {
- } ssl_ctx;
- #endif
- struct dns_srvrq *srvrq; /* Pointer representing the DNS SRV requeest, if any */
- - __decl_hathreads(HA_SPINLOCK_T lock);
- + __decl_hathreads(HA_SPINLOCK_T lock); /* may enclose the proxy's lock, must not be taken under */
- struct {
- const char *file; /* file where the section appears */
- struct eb32_node id; /* place in the tree of used IDs */
- diff --git a/src/proxy.c b/src/proxy.c
- index ae761ead..a537e0b1 100644
- --- a/src/proxy.c
- +++ b/src/proxy.c
- @@ -1940,9 +1940,12 @@ static int cli_parse_enable_dyncookie_backend(char **args, char *payload, struct
- if (!px)
- return 1;
-
- + /* Note: this lock is to make sure this doesn't change while another
- + * thread is in srv_set_dyncookie().
- + */
- HA_SPIN_LOCK(PROXY_LOCK, &px->lock);
- -
- px->ck_opts |= PR_CK_DYNAMIC;
- + HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
-
- for (s = px->srv; s != NULL; s = s->next) {
- HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
- @@ -1950,8 +1953,6 @@ static int cli_parse_enable_dyncookie_backend(char **args, char *payload, struct
- HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
- }
-
- - HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
- -
- return 1;
- }
-
- @@ -1971,9 +1972,12 @@ static int cli_parse_disable_dyncookie_backend(char **args, char *payload, struc
- if (!px)
- return 1;
-
- + /* Note: this lock is to make sure this doesn't change while another
- + * thread is in srv_set_dyncookie().
- + */
- HA_SPIN_LOCK(PROXY_LOCK, &px->lock);
- -
- px->ck_opts &= ~PR_CK_DYNAMIC;
- + HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
-
- for (s = px->srv; s != NULL; s = s->next) {
- HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
- @@ -1984,8 +1988,6 @@ static int cli_parse_disable_dyncookie_backend(char **args, char *payload, struc
- HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
- }
-
- - HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
- -
- return 1;
- }
-
- @@ -2021,10 +2023,13 @@ static int cli_parse_set_dyncookie_key_backend(char **args, char *payload, struc
- return 1;
- }
-
- + /* Note: this lock is to make sure this doesn't change while another
- + * thread is in srv_set_dyncookie().
- + */
- HA_SPIN_LOCK(PROXY_LOCK, &px->lock);
- -
- free(px->dyncookie_key);
- px->dyncookie_key = newkey;
- + HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
-
- for (s = px->srv; s != NULL; s = s->next) {
- HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
- @@ -2032,8 +2037,6 @@ static int cli_parse_set_dyncookie_key_backend(char **args, char *payload, struc
- HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
- }
-
- - HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
- -
- return 1;
- }
-
- diff --git a/src/queue.c b/src/queue.c
- index f4a94530..6aa54170 100644
- --- a/src/queue.c
- +++ b/src/queue.c
- @@ -312,16 +312,16 @@ void process_srv_queue(struct server *s)
- struct proxy *p = s->proxy;
- int maxconn;
-
- - HA_SPIN_LOCK(PROXY_LOCK, &p->lock);
- HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
- + HA_SPIN_LOCK(PROXY_LOCK, &p->lock);
- maxconn = srv_dynamic_maxconn(s);
- while (s->served < maxconn) {
- int ret = pendconn_process_next_strm(s, p);
- if (!ret)
- break;
- }
- - HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
- HA_SPIN_UNLOCK(PROXY_LOCK, &p->lock);
- + HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
- }
-
- /* Adds the stream <strm> to the pending connection queue of server <strm>->srv
- @@ -424,7 +424,8 @@ int pendconn_redistribute(struct server *s)
- /* Check for pending connections at the backend, and assign some of them to
- * the server coming up. The server's weight is checked before being assigned
- * connections it may not be able to handle. The total number of transferred
- - * connections is returned.
- + * connections is returned. It must be called with the server lock held, and
- + * will take the proxy's lock.
- */
- int pendconn_grab_from_px(struct server *s)
- {
- diff --git a/src/server.c b/src/server.c
- index a96f1ef6..236d6bae 100644
- --- a/src/server.c
- +++ b/src/server.c
- @@ -125,7 +125,7 @@ static inline void srv_check_for_dup_dyncookie(struct server *s)
- }
-
- /*
- - * Must be called with the server lock held.
- + * Must be called with the server lock held, and will grab the proxy lock.
- */
- void srv_set_dyncookie(struct server *s)
- {
- @@ -137,15 +137,17 @@ void srv_set_dyncookie(struct server *s)
- int addr_len;
- int port;
-
- + HA_SPIN_LOCK(PROXY_LOCK, &p->lock);
- +
- if ((s->flags & SRV_F_COOKIESET) ||
- !(s->proxy->ck_opts & PR_CK_DYNAMIC) ||
- s->proxy->dyncookie_key == NULL)
- - return;
- + goto out;
- key_len = strlen(p->dyncookie_key);
-
- if (s->addr.ss_family != AF_INET &&
- s->addr.ss_family != AF_INET6)
- - return;
- + goto out;
- /*
- * Buffer to calculate the cookie value.
- * The buffer contains the secret key + the server IP address
- @@ -174,7 +176,7 @@ void srv_set_dyncookie(struct server *s)
- hash_value = XXH64(tmpbuf, buffer_len, 0);
- memprintf(&s->cookie, "%016llx", hash_value);
- if (!s->cookie)
- - return;
- + goto out;
- s->cklen = 16;
-
- /* Don't bother checking if the dyncookie is duplicated if
- @@ -183,6 +185,8 @@ void srv_set_dyncookie(struct server *s)
- */
- if (!(s->next_admin & SRV_ADMF_FMAINT))
- srv_check_for_dup_dyncookie(s);
- + out:
- + HA_SPIN_UNLOCK(PROXY_LOCK, &p->lock);
- }
-
- /*
|
