From 9f0b45852550088b9dc04224c33ddde387469d4b Mon Sep 17 00:00:00 2001 From: Emeric Brun Date: Mon, 23 Oct 2017 14:39:51 +0200 Subject: [PATCH] MEDIUM: threads/server: Use the server lock to protect health check and cli concurrency --- include/proto/checks.h | 8 ++++++-- src/checks.c | 18 +++++++++++++----- src/hlua_fcn.c | 26 ++++++++++++++++++++++++++ src/server.c | 34 +++++++++++++++++++++++++++++----- src/stats.c | 4 ++-- 5 files changed, 76 insertions(+), 14 deletions(-) diff --git a/include/proto/checks.h b/include/proto/checks.h index 98dca25c7..b0b8c7d19 100644 --- a/include/proto/checks.h +++ b/include/proto/checks.h @@ -37,11 +37,15 @@ extern struct data_cb check_conn_cb; */ static inline void health_adjust(struct server *s, short status) { + SPIN_LOCK(SERVER_LOCK, &s->lock); /* return now if observing nor health check is not enabled */ - if (!s->observe || !s->check.task) + if (!s->observe || !s->check.task) { + SPIN_UNLOCK(SERVER_LOCK, &s->lock); return; + } - return __health_adjust(s, status); + __health_adjust(s, status); + SPIN_UNLOCK(SERVER_LOCK, &s->lock); } const char *init_check(struct check *check, int type); diff --git a/src/checks.c b/src/checks.c index d07e508fc..8411fefae 100644 --- a/src/checks.c +++ b/src/checks.c @@ -650,6 +650,7 @@ static void chk_report_conn_err(struct check *check, int errno_bck, int expired) } } + SPIN_LOCK(SERVER_LOCK, &check->server->lock); if (check->state & CHK_ST_PORT_MISS) { /* NOTE: this is reported after tries */ chunk_printf(chk, "No port available for the TCP connection"); @@ -692,6 +693,7 @@ static void chk_report_conn_err(struct check *check, int errno_bck, int expired) else /* HTTP, SMTP, ... */ set_server_check_status(check, HCHK_STATUS_L7TOUT, err_msg); } + SPIN_UNLOCK(SERVER_LOCK, &check->server->lock); return; } @@ -784,15 +786,21 @@ static void event_srv_chk_r(struct connection *conn) int done; unsigned short msglen; + SPIN_LOCK(SERVER_LOCK, &check->server->lock); + if (unlikely(check->result == CHK_RES_FAILED)) goto out_wakeup; - if (conn->flags & CO_FL_HANDSHAKE) + if (conn->flags & CO_FL_HANDSHAKE) { + SPIN_UNLOCK(SERVER_LOCK, &check->server->lock); return; + } /* wake() will take care of calling tcpcheck_main() */ - if (check->type == PR_O2_TCPCHK_CHK) + if (check->type == PR_O2_TCPCHK_CHK) { + SPIN_UNLOCK(SERVER_LOCK, &check->server->lock); return; + } /* Warning! Linux returns EAGAIN on SO_ERROR if data are still available * but the connection was closed on the remote end. Fortunately, recv still @@ -896,7 +904,6 @@ static void event_srv_chk_r(struct connection *conn) !isdigit((unsigned char) *(check->bi->data + 2))) { cut_crlf(check->bi->data); set_server_check_status(check, HCHK_STATUS_L7RSP, check->bi->data); - goto out_wakeup; } @@ -1163,6 +1170,7 @@ static void event_srv_chk_r(struct connection *conn) else { if (!done) goto wait_more_data; + /* it seems we have a OK packet but without a valid length, * it must be a protocol error */ @@ -1224,6 +1232,7 @@ static void event_srv_chk_r(struct connection *conn) else { if (!done) goto wait_more_data; + /* it seems we have a Handshake Initialization packet but without a valid length, * it must be a protocol error */ @@ -1260,7 +1269,6 @@ static void event_srv_chk_r(struct connection *conn) if ((msglen > 2) || (memcmp(check->bi->data + 2 + msglen, "\x02\x01\x01\x61", 4) != 0)) { set_server_check_status(check, HCHK_STATUS_L7RSP, "Not LDAPv3 protocol"); - goto out_wakeup; } @@ -1273,7 +1281,6 @@ static void event_srv_chk_r(struct connection *conn) if ((msglen > 4) || (memcmp(check->bi->data + 7 + msglen, "\x0a\x01", 2) != 0)) { set_server_check_status(check, HCHK_STATUS_L7RSP, "Not LDAPv3 protocol"); - goto out_wakeup; } @@ -1314,6 +1321,7 @@ static void event_srv_chk_r(struct connection *conn) break; } /* switch */ + SPIN_UNLOCK(SERVER_LOCK, &check->server->lock); out_wakeup: /* collect possible new errors */ if (conn->flags & CO_FL_ERROR) diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c index a4adb2178..60becb02f 100644 --- a/src/hlua_fcn.c +++ b/src/hlua_fcn.c @@ -626,7 +626,9 @@ int hlua_server_shut_sess(lua_State *L) struct server *srv; srv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &srv->lock); srv_shutdown_streams(srv, SF_ERR_KILLED); + SPIN_UNLOCK(SERVER_LOCK, &srv->lock); return 0; } @@ -635,7 +637,9 @@ int hlua_server_set_drain(lua_State *L) struct server *srv; srv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &srv->lock); srv_adm_set_drain(srv); + SPIN_UNLOCK(SERVER_LOCK, &srv->lock); return 0; } @@ -644,7 +648,9 @@ int hlua_server_set_maint(lua_State *L) struct server *srv; srv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &srv->lock); srv_adm_set_maint(srv); + SPIN_UNLOCK(SERVER_LOCK, &srv->lock); return 0; } @@ -653,7 +659,9 @@ int hlua_server_set_ready(lua_State *L) struct server *srv; srv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &srv->lock); srv_adm_set_ready(srv); + SPIN_UNLOCK(SERVER_LOCK, &srv->lock); return 0; } @@ -662,9 +670,11 @@ int hlua_server_check_enable(lua_State *L) struct server *sv; sv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &sv->lock); if (sv->check.state & CHK_ST_CONFIGURED) { sv->check.state |= CHK_ST_ENABLED; } + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); return 0; } @@ -673,9 +683,11 @@ int hlua_server_check_disable(lua_State *L) struct server *sv; sv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &sv->lock); if (sv->check.state & CHK_ST_CONFIGURED) { sv->check.state &= ~CHK_ST_ENABLED; } + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); return 0; } @@ -685,8 +697,10 @@ int hlua_server_check_force_up(lua_State *L) sv = hlua_check_server(L, 1); if (!(sv->track)) { + SPIN_LOCK(SERVER_LOCK, &sv->lock); sv->check.health = sv->check.rise + sv->check.fall - 1; srv_set_running(sv, "changed from Lua script", NULL); + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); } return 0; } @@ -697,8 +711,10 @@ int hlua_server_check_force_nolb(lua_State *L) sv = hlua_check_server(L, 1); if (!(sv->track)) { + SPIN_LOCK(SERVER_LOCK, &sv->lock); sv->check.health = sv->check.rise + sv->check.fall - 1; srv_set_stopping(sv, "changed from Lua script", NULL); + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); } return 0; } @@ -708,10 +724,12 @@ int hlua_server_check_force_down(lua_State *L) struct server *sv; sv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &sv->lock); if (!(sv->track)) { sv->check.health = 0; srv_set_stopped(sv, "changed from Lua script", NULL); } + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); return 0; } @@ -720,9 +738,11 @@ int hlua_server_agent_enable(lua_State *L) struct server *sv; sv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &sv->lock); if (sv->agent.state & CHK_ST_CONFIGURED) { sv->agent.state |= CHK_ST_ENABLED; } + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); return 0; } @@ -731,9 +751,11 @@ int hlua_server_agent_disable(lua_State *L) struct server *sv; sv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &sv->lock); if (sv->agent.state & CHK_ST_CONFIGURED) { sv->agent.state &= ~CHK_ST_ENABLED; } + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); return 0; } @@ -742,10 +764,12 @@ int hlua_server_agent_force_up(lua_State *L) struct server *sv; sv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &sv->lock); if (sv->agent.state & CHK_ST_ENABLED) { sv->agent.health = sv->agent.rise + sv->agent.fall - 1; srv_set_running(sv, "changed from Lua script", NULL); } + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); return 0; } @@ -754,10 +778,12 @@ int hlua_server_agent_force_down(lua_State *L) struct server *sv; sv = hlua_check_server(L, 1); + SPIN_LOCK(SERVER_LOCK, &sv->lock); if (sv->agent.state & CHK_ST_ENABLED) { sv->agent.health = 0; srv_set_stopped(sv, "changed from Lua script", NULL); } + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); return 0; } diff --git a/src/server.c b/src/server.c index f0b912f66..2d0e3b4f9 100644 --- a/src/server.c +++ b/src/server.c @@ -880,8 +880,11 @@ void srv_set_stopped(struct server *s, const char *reason, struct check *check) } srv_register_update(s); - for (srv = s->trackers; srv; srv = srv->tracknext) + for (srv = s->trackers; srv; srv = srv->tracknext) { + SPIN_LOCK(SERVER_LOCK, &srv->lock); srv_set_stopped(srv, NULL, NULL); + SPIN_UNLOCK(SERVER_LOCK, &srv->lock); + } } /* Marks server up regardless of its checks' statuses and provided it isn't @@ -919,8 +922,11 @@ void srv_set_running(struct server *s, const char *reason, struct check *check) s->next_state = SRV_ST_RUNNING; srv_register_update(s); - for (srv = s->trackers; srv; srv = srv->tracknext) + for (srv = s->trackers; srv; srv = srv->tracknext) { + SPIN_LOCK(SERVER_LOCK, &srv->lock); srv_set_running(srv, NULL, NULL); + SPIN_UNLOCK(SERVER_LOCK, &srv->lock); + } } /* Marks server stopping regardless of its checks' statuses and provided it @@ -957,8 +963,11 @@ void srv_set_stopping(struct server *s, const char *reason, struct check *check) } srv_register_update(s); - for (srv = s->trackers; srv; srv = srv->tracknext) + for (srv = s->trackers; srv; srv = srv->tracknext) { + SPIN_LOCK(SERVER_LOCK, &srv->lock); srv_set_stopping(srv, NULL, NULL); + SPIN_LOCK(SERVER_LOCK, &srv->lock); + } } /* Enables admin flag (among SRV_ADMF_*) on server . This is used to @@ -997,8 +1006,11 @@ void srv_set_admin_flag(struct server *s, enum srv_admin mode, const char *cause else if (mode & SRV_ADMF_DRAIN) mode = SRV_ADMF_IDRAIN; - for (srv = s->trackers; srv; srv = srv->tracknext) + for (srv = s->trackers; srv; srv = srv->tracknext) { + SPIN_LOCK(SERVER_LOCK, &srv->lock); srv_set_admin_flag(srv, mode, cause); + SPIN_LOCK(SERVER_LOCK, &srv->lock); + } } /* Disables admin flag (among SRV_ADMF_*) on server . This is used to @@ -1032,8 +1044,11 @@ void srv_clr_admin_flag(struct server *s, enum srv_admin mode) else if (mode & SRV_ADMF_DRAIN) mode = SRV_ADMF_IDRAIN; - for (srv = s->trackers; srv; srv = srv->tracknext) + for (srv = s->trackers; srv; srv = srv->tracknext) { + SPIN_LOCK(SERVER_LOCK, &srv->lock); srv_clr_admin_flag(srv, mode); + SPIN_UNLOCK(SERVER_LOCK, &srv->lock); + } } /* principle: propagate maint and drain to tracking servers. This is useful @@ -1047,11 +1062,13 @@ static void srv_propagate_admin_state(struct server *srv) return; for (srv2 = srv->trackers; srv2; srv2 = srv2->tracknext) { + SPIN_LOCK(SERVER_LOCK, &srv2->lock); if (srv->next_admin & (SRV_ADMF_MAINT | SRV_ADMF_CMAINT)) srv_set_admin_flag(srv2, SRV_ADMF_IMAINT, NULL); if (srv->next_admin & SRV_ADMF_DRAIN) srv_set_admin_flag(srv2, SRV_ADMF_IDRAIN, NULL); + SPIN_UNLOCK(SERVER_LOCK, &srv2->lock); } } @@ -2772,6 +2789,7 @@ static void srv_update_state(struct server *srv, int version, char **params) if (msg->len) goto out; + SPIN_LOCK(SERVER_LOCK, &srv->lock); /* recover operational state and apply it to this server * and all servers tracking this one */ switch (srv_op_state) { @@ -2901,6 +2919,7 @@ static void srv_update_state(struct server *srv, int version, char **params) if (port_str) srv->svc_port = port; + SPIN_UNLOCK(SERVER_LOCK, &srv->lock); break; default: @@ -4013,6 +4032,8 @@ static int cli_parse_set_server(char **args, struct appctx *appctx, void *privat if (!sv) return 1; + SPIN_LOCK(SERVER_LOCK, &sv->lock); + if (strcmp(args[3], "weight") == 0) { warning = server_parse_weight_change_request(sv, args[4]); if (warning) { @@ -4126,6 +4147,7 @@ static int cli_parse_set_server(char **args, struct appctx *appctx, void *privat appctx->ctx.cli.severity = LOG_ERR; appctx->ctx.cli.msg = "can't unset 'port' since MAPPORTS is in use.\n"; appctx->st0 = CLI_ST_PRINT; + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); return 1; } sv->check.port = i; @@ -4140,6 +4162,7 @@ static int cli_parse_set_server(char **args, struct appctx *appctx, void *privat appctx->ctx.cli.severity = LOG_ERR; appctx->ctx.cli.msg = "set server / addr requires an address and optionally a port.\n"; appctx->st0 = CLI_ST_PRINT; + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); return 1; } else { @@ -4174,6 +4197,7 @@ static int cli_parse_set_server(char **args, struct appctx *appctx, void *privat appctx->ctx.cli.msg = "'set server ' only supports 'agent', 'health', 'state', 'weight', 'addr', 'fqdn' and 'check-port'.\n"; appctx->st0 = CLI_ST_PRINT; } + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); return 1; } diff --git a/src/stats.c b/src/stats.c index e026f36a5..f68831933 100644 --- a/src/stats.c +++ b/src/stats.c @@ -2764,6 +2764,7 @@ static int stats_process_http_post(struct stream_interface *si) reprocess = 1; } else if ((sv = findserver(px, value)) != NULL) { + SPIN_LOCK(SERVER_LOCK, &sv->lock); switch (action) { case ST_ADM_ACTION_DISABLE: if (!(sv->cur_admin & SRV_ADMF_FMAINT)) { @@ -2880,17 +2881,16 @@ static int stats_process_http_post(struct stream_interface *si) if (px->state != PR_STSTOPPED) { struct stream *sess, *sess_bck; - SPIN_LOCK(SERVER_LOCK, &sv->lock); list_for_each_entry_safe(sess, sess_bck, &sv->actconns, by_srv) if (sess->srv_conn == sv) stream_shutdown(sess, SF_ERR_KILLED); - SPIN_UNLOCK(SERVER_LOCK, &sv->lock); altered_servers++; total_servers++; } break; } + SPIN_UNLOCK(SERVER_LOCK, &sv->lock); } else { /* the server name is unknown or ambiguous (duplicate names) */ total_servers++;