diff --git a/postfix/HISTORY b/postfix/HISTORY index f2ed020e8..b64c69144 100644 --- a/postfix/HISTORY +++ b/postfix/HISTORY @@ -16930,11 +16930,16 @@ Apologies for any names omitted. Cleanup: don't log vstream_tweak "connection reset by peer" errors. File: util/vstream_tweak.c. -20110903 +20110904-7 - Bugfix: master daemon panic with an "at process limit X" - error, when "postfix reload" reduced the process limit for - some Postfix service from (a value larger than the current - process count for that service) to (a value <= the current - process count), and then a new connection was made to that - service. File: master/master_avail.c. + Bugfix: master daemon panic with "master_spawn: at process + limit", when "postfix reload" reduces the process limit + from (a value larger than the current process count for + some service) to (a value <= the current process count), + and then a new connection is made to that service. This + structural solution centralizes the decision to monitor a + service port (or not). To improve robustness against future + code changes, it clarifies some of the internal dependencies + that exist inside the master daemon. Files: master/master.h, + master/master_avail.c, master/master_conf.c, + master/master_service.c, master/master_spawn.c. diff --git a/postfix/src/global/mail_version.h b/postfix/src/global/mail_version.h index e861bf990..f737cc081 100644 --- a/postfix/src/global/mail_version.h +++ b/postfix/src/global/mail_version.h @@ -20,7 +20,7 @@ * Patches change both the patchlevel and the release date. Snapshots have no * patchlevel; they change the release date only. */ -#define MAIL_RELEASE_DATE "20110905" +#define MAIL_RELEASE_DATE "20110907" #define MAIL_VERSION_NUMBER "2.9" #ifdef SNAPSHOT diff --git a/postfix/src/master/master.h b/postfix/src/master/master.h index 77c3e73ca..954dac4f7 100644 --- a/postfix/src/master/master.h +++ b/postfix/src/master/master.h @@ -64,9 +64,11 @@ typedef struct MASTER_SERV { #define MASTER_FLAG_CONDWAKE (1<<2) /* wake up if actually used */ #define MASTER_FLAG_INETHOST (1<<3) /* endpoint name specifies host */ #define MASTER_FLAG_LOCAL_ONLY (1<<4) /* no remote clients */ +#define MASTER_FLAG_LISTEN (1<<5) /* monitor this port */ #define MASTER_THROTTLED(f) ((f)->flags & MASTER_FLAG_THROTTLE) #define MASTER_MARKED_FOR_DELETION(f) ((f)->flags & MASTER_FLAG_MARK) +#define MASTER_LISTENING(f) ((f)->flags & MASTER_FLAG_LISTEN) #define MASTER_LIMIT_OK(limit, count) ((limit) == 0 || ((count) < (limit))) @@ -135,7 +137,10 @@ extern void master_vars_init(void); extern MASTER_SERV *master_head; extern void master_start_service(MASTER_SERV *); extern void master_stop_service(MASTER_SERV *); -extern void master_restart_service(MASTER_SERV *); +extern void master_restart_service(MASTER_SERV *, int); + +#define DO_CONF_RELOAD 1 /* config files were reloaded */ +#define NO_CONF_RELOAD 0 /* no config file was reloaded */ /* * master_events.c diff --git a/postfix/src/master/master_avail.c b/postfix/src/master/master_avail.c index 472b59a1a..f3573d138 100644 --- a/postfix/src/master/master_avail.c +++ b/postfix/src/master/master_avail.c @@ -26,15 +26,17 @@ /* available process, or this module causes a new process to be /* created to service the request. /* -/* When the service runs out of process slots, a warning is logged. -/* When the service is eligible for stress-mode operation, servers -/* are restarted and new servers are created with stress mode enabled. +/* When the service runs out of process slots, and the service +/* is eligible for stress-mode operation, a warning is logged, +/* servers are asked to restart at their convenience, and new +/* servers are created with stress mode enabled. /* /* master_avail_listen() ensures that someone monitors the service's /* listen socket for connection requests (as long as resources /* to handle connection requests are available). This function may -/* be called at random. When the maximum number of servers is running, -/* connection requests are left in the system queue. +/* be called at random times, but it must be called after each status +/* change of a service (throttled, process limit, etc.) or child +/* process (taken, available, dead, etc.). /* /* master_avail_cleanup() should be called when the named service /* is taken out of operation. It terminates child processes by @@ -42,9 +44,13 @@ /* /* master_avail_more() should be called when the named process /* has become available for servicing new connection requests. +/* This function updates the process availability status and +/* counter, and implicitly calls master_avail_listen(). /* /* master_avail_less() should be called when the named process /* has become unavailable for servicing new connection requests. +/* This function updates the process availability status and +/* counter, and implicitly calls master_avail_listen(). /* DIAGNOSTICS /* Panic: internal inconsistencies. /* BUGS @@ -81,16 +87,10 @@ static void master_avail_event(int event, char *context) { MASTER_SERV *serv = (MASTER_SERV *) context; time_t now; - int n; if (event == 0) /* XXX Can this happen? */ - return; - /* XXX Should check these when the process or service status is changed. */ - if (!MASTER_LIMIT_OK(serv->max_proc, serv->total_proc) - || MASTER_THROTTLED(serv)) { /* XXX interface botch */ - for (n = 0; n < serv->listen_fd_count; n++) - event_disable_readwrite(serv->listen_fd[n]); - } else { + msg_panic("master_avail_event: null event"); + else { /* * When all servers for a public internet service are busy, we start @@ -112,51 +112,70 @@ static void master_avail_event(int event, char *context) && !MASTER_LIMIT_OK(serv->max_proc, serv->total_proc + 1)) { now = event_time(); if (serv->stress_expire_time < now) - master_restart_service(serv); + master_restart_service(serv, NO_CONF_RELOAD); serv->stress_expire_time = now + 1000; } master_spawn(serv); } } -/* master_avail_listen - make sure that someone monitors the listen socket */ +/* master_avail_listen - enforce the socket monitoring policy */ void master_avail_listen(MASTER_SERV *serv) { const char *myname = "master_avail_listen"; + int listen_flag; time_t now; int n; /* + * Caution: several other master_XXX modules call master_avail_listen(), + * master_avail_more() or master_avail_less(). To avoid mutual dependency + * problems, the code below invokes no code in other master_XXX modules, + * and modifies no data that is maintained by other master_XXX modules. + * * When no-one else is monitoring the service's listen socket, start * monitoring the socket for connection requests. All this under the * restriction that we have sufficient resources to service a connection * request. */ if (msg_verbose) - msg_info("%s: avail %d total %d max %d", myname, + msg_info("%s: %s avail %d total %d max %d", myname, serv->name, serv->avail_proc, serv->total_proc, serv->max_proc); - if (serv->avail_proc < 1 && !MASTER_THROTTLED(serv)) { - if (MASTER_LIMIT_OK(serv->max_proc, serv->total_proc)) { - if (msg_verbose) - msg_info("%s: enable events %s", myname, serv->name); - for (n = 0; n < serv->listen_fd_count; n++) - event_enable_read(serv->listen_fd[n], master_avail_event, - (char *) serv); - } else if ((serv->flags & MASTER_FLAG_LOCAL_ONLY) == 0 - && serv->max_proc != 1/* XXX postscreen(8) */ - && (now = event_time()) - serv->busy_warn_time > 1000) { - serv->busy_warn_time = now; - msg_warn("service \"%s\" (%s) has reached its process limit \"%d\": " - "new clients may experience noticeable delays", - serv->ext_name, serv->name, serv->max_proc); - msg_warn("to avoid this condition, increase the process count " - "in master.cf or reduce the service time per client"); - if (serv->stress_param_val) + if (MASTER_THROTTLED(serv) || serv->avail_proc > 0) { + listen_flag = 0; + } else if (MASTER_LIMIT_OK(serv->max_proc, serv->total_proc)) { + listen_flag = 1; + } else { + listen_flag = 0; + if (serv->stress_param_val != 0) { + now = event_time(); + if (serv->busy_warn_time < now - 1000) { + serv->busy_warn_time = now; + msg_warn("service \"%s\" (%s) has reached its process limit \"%d\": " + "new clients may experience noticeable delays", + serv->ext_name, serv->name, serv->max_proc); + msg_warn("to avoid this condition, increase the process count " + "in master.cf or reduce the service time per client"); msg_warn("see http://www.postfix.org/STRESS_README.html for " "examples of stress-adapting configuration settings"); + } } } + if (listen_flag && !MASTER_LISTENING(serv)) { + if (msg_verbose) + msg_info("%s: enable events %s", myname, serv->name); + for (n = 0; n < serv->listen_fd_count; n++) + event_enable_read(serv->listen_fd[n], master_avail_event, + (char *) serv); + serv->flags |= MASTER_FLAG_LISTEN; + } else if (!listen_flag && MASTER_LISTENING(serv)) { + if (msg_verbose) + msg_info("%s: disable events %s", myname, serv->name); + for (n = 0; n < serv->listen_fd_count; n++) + event_disable_readwrite(serv->listen_fd[n]); + serv->flags &= ~MASTER_FLAG_LISTEN; + } } /* master_avail_cleanup - cleanup */ @@ -167,8 +186,18 @@ void master_avail_cleanup(MASTER_SERV *serv) master_delete_children(serv); /* XXX calls * master_avail_listen */ - for (n = 0; n < serv->listen_fd_count; n++) - event_disable_readwrite(serv->listen_fd[n]); /* XXX must be last */ + + /* + * This code is redundant because master_delete_children() throttles the + * service temporarily before calling master_avail_listen/less(), which + * then turn off read events. This temporary throttling is not documented + * (it is only an optimization), and therefore we must not depend on it. + */ + if (MASTER_LISTENING(serv)) { + for (n = 0; n < serv->listen_fd_count; n++) + event_disable_readwrite(serv->listen_fd[n]); + serv->flags &= ~MASTER_FLAG_LISTEN; + } } /* master_avail_more - one more available child process */ @@ -176,9 +205,13 @@ void master_avail_cleanup(MASTER_SERV *serv) void master_avail_more(MASTER_SERV *serv, MASTER_PROC *proc) { const char *myname = "master_avail_more"; - int n; /* + * Caution: several other master_XXX modules call master_avail_listen(), + * master_avail_more() or master_avail_less(). To avoid mutual dependency + * problems, the code below invokes no code in other master_XXX modules, + * and modifies no data that is maintained by other master_XXX modules. + * * This child process has become available for servicing connection * requests, so we can stop monitoring the service's listen socket. The * child will do it for us. @@ -189,10 +222,7 @@ void master_avail_more(MASTER_SERV *serv, MASTER_PROC *proc) msg_panic("%s: process already available", myname); serv->avail_proc++; proc->avail = MASTER_STAT_AVAIL; - if (msg_verbose) - msg_info("%s: disable events %s", myname, serv->name); - for (n = 0; n < serv->listen_fd_count; n++) - event_disable_readwrite(serv->listen_fd[n]); + master_avail_listen(serv); } /* master_avail_less - one less available child process */ @@ -202,6 +232,11 @@ void master_avail_less(MASTER_SERV *serv, MASTER_PROC *proc) const char *myname = "master_avail_less"; /* + * Caution: several other master_XXX modules call master_avail_listen(), + * master_avail_more() or master_avail_less(). To avoid mutual dependency + * problems, the code below invokes no code in other master_XXX modules, + * and modifies no data that is maintained by other master_XXX modules. + * * This child is no longer available for servicing connection requests. * When no child processes are available, start monitoring the service's * listen socket for new connection requests. diff --git a/postfix/src/master/master_conf.c b/postfix/src/master/master_conf.c index 98f7f73d3..ba1e6e19b 100644 --- a/postfix/src/master/master_conf.c +++ b/postfix/src/master/master_conf.c @@ -135,7 +135,7 @@ void master_config(void) SWAP(char *, serv->path, entry->path); SWAP(ARGV *, serv->args, entry->args); SWAP(char *, serv->stress_param_val, entry->stress_param_val); - master_restart_service(serv); + master_restart_service(serv, DO_CONF_RELOAD); free_master_ent(entry); } } diff --git a/postfix/src/master/master_service.c b/postfix/src/master/master_service.c index 62289b308..d5663b9ca 100644 --- a/postfix/src/master/master_service.c +++ b/postfix/src/master/master_service.c @@ -12,15 +12,18 @@ /* void master_stop_service(serv) /* MASTER_SERV *serv; /* -/* void master_restart_service(serv) +/* void master_restart_service(serv, conf_reload) /* MASTER_SERV *serv; +/* int conf_reload; /* DESCRIPTION /* master_start_service() enables the named service. /* /* master_stop_service() disables named service. /* /* master_restart_service() requests all running child processes to -/* commit suicide. This is typically used after a configuration reload. +/* commit suicide. The conf_reload argument is either DO_CONF_RELOAD +/* (configuration files were reloaded, re-evaluate the child process +/* creation policy) or NO_CONF_RELOAD. /* DIAGNOSTICS /* BUGS /* SEE ALSO @@ -87,7 +90,7 @@ void master_stop_service(MASTER_SERV *serv) /* master_restart_service - restart service after configuration reload */ -void master_restart_service(MASTER_SERV *serv) +void master_restart_service(MASTER_SERV *serv, int conf_reload) { /* @@ -101,4 +104,10 @@ void master_restart_service(MASTER_SERV *serv) */ master_status_init(serv); master_wakeup_init(serv); + + /* + * Respond to configuration change. + */ + if (conf_reload) + master_avail_listen(serv); } diff --git a/postfix/src/master/master_spawn.c b/postfix/src/master/master_spawn.c index 2d92a0d70..6318e6bc2 100644 --- a/postfix/src/master/master_spawn.c +++ b/postfix/src/master/master_spawn.c @@ -110,7 +110,7 @@ static void master_unthrottle(MASTER_SERV *serv) event_cancel_timer(master_unthrottle_wrapper, (char *) serv); if (msg_verbose) msg_info("throttle released for command %s", serv->path); - master_avail_listen(serv); /* XXX interface botch */ + master_avail_listen(serv); } } @@ -130,6 +130,7 @@ static void master_throttle(MASTER_SERV *serv) serv->throttle_delay); if (msg_verbose) msg_info("throttling command %s", serv->path); + master_avail_listen(serv); } } @@ -275,8 +276,7 @@ static void master_delete_child(MASTER_PROC *proc) serv->total_proc--; if (proc->avail == MASTER_STAT_AVAIL) master_avail_less(serv, proc); - else if (MASTER_LIMIT_OK(serv->max_proc, serv->total_proc) - && serv->avail_proc < 1) + else master_avail_listen(serv); binhash_delete(master_child_table, (char *) &proc->pid, sizeof(proc->pid), (void (*) (char *)) 0);