On Wed, Apr 27, 2016 at 01:10:56PM +0200, Hannes Reinecke wrote:
Oh, I did have one other thought.
If you're not changing the signal mask in uevent_listen, can't you just
use poll? Otherwise people are going to try and figure out what's going
on with the signals that they're missing (at least that's what I did).
> For initial configuration multipathd waits until it has synchronized
> with the existing setup. On larger systems this takes up quite
> some time (I've measured 80 seconds on a system with 1024 paths)
> causing systemd to stall and the system to fail booting.
> This patch makes the initial configuration asynchronous, and
> using the same codepath as the existing 'reconfigure' CLI
> command.
>
> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
> ---
> libmultipath/uevent.c | 10 +--
> multipathd/cli_handlers.c | 14 ++-
> multipathd/main.c | 219 ++++++++++++++++++++++++++++++----------------
> multipathd/main.h | 2 +
> 4 files changed, 157 insertions(+), 88 deletions(-)
>
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index 478c6ce..fbe9c44 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -529,8 +529,6 @@ int uevent_listen(struct udev *udev)
> }
>
> pthread_sigmask(SIG_SETMASK, NULL, &mask);
> - sigdelset(&mask, SIGHUP);
> - sigdelset(&mask, SIGUSR1);
> events = 0;
> while (1) {
> struct uevent *uev;
> @@ -561,9 +559,11 @@ int uevent_listen(struct udev *udev)
> continue;
> }
> if (fdcount < 0) {
> - if (errno != EINTR)
> - condlog(0, "error receiving "
> - "uevent message: %m");
> + if (errno == EINTR)
> + continue;
> +
> + condlog(0, "error receiving "
> + "uevent message: %m");
> err = -errno;
> break;
> }
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index dbdcbc2..0397353 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -909,17 +909,13 @@ cli_switch_group(void * v, char ** reply, int * len, void * data)
> int
> cli_reconfigure(void * v, char ** reply, int * len, void * data)
> {
> - struct vectors * vecs = (struct vectors *)data;
> -
> - if (need_to_delay_reconfig(vecs)) {
> - conf->delayed_reconfig = 1;
> - condlog(2, "delaying reconfigure (operator)");
> - return 0;
> - }
> -
> condlog(2, "reconfigure (operator)");
>
> - return reconfigure(vecs);
> + if (set_config_state(DAEMON_CONFIGURE) == ETIMEDOUT) {
> + condlog(2, "timeout starting reconfiguration");
> + return 1;
> + }
> + return 0;
> }
>
> int
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 77eb498..41b5a49 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -97,10 +97,11 @@ struct mpath_event_param
> unsigned int mpath_mx_alloc_len;
>
> int logsink;
> -enum daemon_status running_state;
> +enum daemon_status running_state = DAEMON_INIT;
> pid_t daemon_pid;
> +pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
> +pthread_cond_t config_cond = PTHREAD_COND_INITIALIZER;
>
> -static sem_t exit_sem;
> /*
> * global copy of vecs for use in sig handlers
> */
> @@ -108,6 +109,94 @@ struct vectors * gvecs;
>
> struct udev * udev;
>
> +const char *
> +daemon_status(void)
> +{
> + switch (running_state) {
> + case DAEMON_INIT:
> + return "init";
> + case DAEMON_START:
> + return "startup";
> + case DAEMON_CONFIGURE:
> + return "configure";
> + case DAEMON_IDLE:
> + return "idle";
> + case DAEMON_RUNNING:
> + return "running";
> + case DAEMON_SHUTDOWN:
> + return "shutdown";
> + }
> + return NULL;
> +}
> +
> +/*
> + * I love you too, systemd ...
> + */
> +const char *
> +sd_notify_status(void)
> +{
> + switch (running_state) {
> + case DAEMON_INIT:
> + return "STATUS=init";
> + case DAEMON_START:
> + return "STATUS=startup";
> + case DAEMON_CONFIGURE:
> + return "STATUS=configure";
> + case DAEMON_IDLE:
> + return "STATUS=idle";
> + case DAEMON_RUNNING:
> + return "STATUS=running";
> + case DAEMON_SHUTDOWN:
> + return "STATUS=shutdown";
> + }
> + return NULL;
> +}
> +
> +static void config_cleanup(void *arg)
> +{
> + pthread_mutex_unlock(&config_lock);
> +}
> +
> +void post_config_state(enum daemon_status state)
> +{
> + pthread_mutex_lock(&config_lock);
> + if (state != running_state) {
> + running_state = state;
> + pthread_cond_broadcast(&config_cond);
> +#ifdef USE_SYSTEMD
> + sd_notify(0, sd_notify_status());
> +#endif
> + }
> + pthread_mutex_unlock(&config_lock);
> +}
> +
> +int set_config_state(enum daemon_status state)
> +{
> + int rc = 0;
> +
> + pthread_cleanup_push(config_cleanup, NULL);
> + pthread_mutex_lock(&config_lock);
> + if (running_state != state) {
> + if (running_state != DAEMON_IDLE) {
> + struct timespec ts;
> +
> + clock_gettime(CLOCK_REALTIME, &ts);
> + ts.tv_sec += 1;
> + rc = pthread_cond_timedwait(&config_cond,
> + &config_lock, &ts);
> + }
> + if (!rc) {
> + running_state = state;
> + pthread_cond_broadcast(&config_cond);
> +#ifdef USE_SYSTEMD
> + sd_notify(0, sd_notify_status());
> +#endif
> + }
> + }
> + pthread_cleanup_pop(1);
> + return rc;
> +}
> +
> static int
> need_switch_pathgroup (struct multipath * mpp, int refresh)
> {
> @@ -352,7 +441,7 @@ ev_add_map (char * dev, char * alias, struct vectors * vecs)
> if (mpp) {
> if (mpp->wait_for_udev > 1) {
> if (update_map(mpp, vecs))
> - /* setup multipathd removed the map */
> + /* setup multipathd removed the map */
> return 1;
> }
> if (mpp->wait_for_udev) {
> @@ -360,7 +449,7 @@ ev_add_map (char * dev, char * alias, struct vectors * vecs)
> if (conf->delayed_reconfig &&
> !need_to_delay_reconfig(vecs)) {
> condlog(2, "reconfigure (delayed)");
> - reconfigure(vecs);
> + set_config_state(DAEMON_CONFIGURE);
> return 0;
> }
> }
> @@ -903,6 +992,16 @@ uev_trigger (struct uevent * uev, void * trigger_data)
> if (uev_discard(uev->devpath))
> return 0;
>
> + pthread_cleanup_push(config_cleanup, NULL);
> + pthread_mutex_lock(&config_lock);
> + if (running_state != DAEMON_IDLE &&
> + running_state != DAEMON_RUNNING)
> + pthread_cond_wait(&config_cond, &config_lock);
> + pthread_cleanup_pop(1);
> +
> + if (running_state == DAEMON_SHUTDOWN)
> + return 0;
> +
> pthread_cleanup_push(cleanup_lock, &vecs->lock);
> lock(vecs->lock);
> pthread_testcancel();
> @@ -1031,25 +1130,7 @@ uxlsnrloop (void * ap)
> void
> exit_daemon (void)
> {
> - sem_post(&exit_sem);
> -}
> -
> -const char *
> -daemon_status(void)
> -{
> - switch (running_state) {
> - case DAEMON_INIT:
> - return "init";
> - case DAEMON_START:
> - return "startup";
> - case DAEMON_CONFIGURE:
> - return "configure";
> - case DAEMON_RUNNING:
> - return "running";
> - case DAEMON_SHUTDOWN:
> - return "shutdown";
> - }
> - return NULL;
> + post_config_state(DAEMON_SHUTDOWN);
> }
>
> static void
> @@ -1178,7 +1259,7 @@ missing_uev_wait_tick(struct vectors *vecs)
> if (timed_out && conf->delayed_reconfig &&
> !need_to_delay_reconfig(vecs)) {
> condlog(2, "reconfigure (delayed)");
> - reconfigure(vecs);
> + set_config_state(DAEMON_CONFIGURE);
> }
> }
>
> @@ -1541,11 +1622,18 @@ checkerloop (void *ap)
>
> while (1) {
> struct timeval diff_time, start_time, end_time;
> - int num_paths = 0, ticks = 0, signo, strict_timing;
> + int num_paths = 0, ticks = 0, signo, strict_timing, rc = 0;
> sigset_t mask;
>
> if (gettimeofday(&start_time, NULL) != 0)
> start_time.tv_sec = 0;
> +
> + rc = set_config_state(DAEMON_RUNNING);
> + if (rc == ETIMEDOUT) {
> + condlog(4, "timeout waiting for DAEMON_IDLE");
> + continue;
> + }
> +
> pthread_cleanup_push(cleanup_lock, &vecs->lock);
> lock(vecs->lock);
> pthread_testcancel();
> @@ -1600,6 +1688,7 @@ checkerloop (void *ap)
> }
> }
>
> + post_config_state(DAEMON_IDLE);
> if (!strict_timing)
> sleep(1);
> else {
> @@ -1734,8 +1823,6 @@ reconfigure (struct vectors * vecs)
> struct config * old = conf;
> int retval = 1;
>
> - running_state = DAEMON_CONFIGURE;
> -
> /*
> * free old map and path vectors ... they use old conf state
> */
> @@ -1765,8 +1852,6 @@ reconfigure (struct vectors * vecs)
> }
> uxsock_timeout = conf->uxsock_timeout;
>
> - running_state = DAEMON_RUNNING;
> -
> return retval;
> }
>
> @@ -1819,20 +1904,9 @@ signal_set(int signo, void (*func) (int))
> void
> handle_signals(void)
> {
> - if (reconfig_sig && running_state == DAEMON_RUNNING) {
> - pthread_cleanup_push(cleanup_lock,
> - &gvecs->lock);
> - lock(gvecs->lock);
> - pthread_testcancel();
> - if (need_to_delay_reconfig(gvecs)) {
> - conf->delayed_reconfig = 1;
> - condlog(2, "delaying reconfigure (signal)");
> - }
> - else {
> - condlog(2, "reconfigure (signal)");
> - reconfigure(gvecs);
> - }
> - lock_cleanup_pop(gvecs->lock);
> + if (reconfig_sig) {
> + condlog(2, "reconfigure (signal)");
> + set_config_state(DAEMON_CONFIGURE);
> }
> if (log_reset_sig) {
> condlog(2, "reset log (signal)");
> @@ -1966,7 +2040,6 @@ child (void * param)
> char *envp;
>
> mlockall(MCL_CURRENT | MCL_FUTURE);
> - sem_init(&exit_sem, 0, 0);
> signal_init();
>
> udev = udev_new();
> @@ -1987,11 +2060,8 @@ child (void * param)
> exit(1);
> }
>
> - running_state = DAEMON_START;
> + post_config_state(DAEMON_START);
>
> -#ifdef USE_SYSTEMD
> - sd_notify(0, "STATUS=startup");
> -#endif
> condlog(2, "--------start up--------");
> condlog(2, "read " DEFAULT_CONFIGFILE);
>
> @@ -2067,6 +2137,11 @@ child (void * param)
> }
> #endif
> /*
> + * Signal start of configuration
> + */
> + post_config_state(DAEMON_CONFIGURE);
> +
> + /*
> * Start uevent listener early to catch events
> */
> if ((rc = pthread_create(&uevent_thr, &uevent_attr, ueventloop, udev))) {
> @@ -2078,21 +2153,6 @@ child (void * param)
> condlog(0, "failed to create cli listener: %d", rc);
> goto failed;
> }
> - /*
> - * fetch and configure both paths and multipaths
> - */
> -#ifdef USE_SYSTEMD
> - sd_notify(0, "STATUS=configure");
> -#endif
> - running_state = DAEMON_CONFIGURE;
> -
> - lock(vecs->lock);
> - if (configure(vecs, 1)) {
> - unlock(vecs->lock);
> - condlog(0, "failure during configuration");
> - goto failed;
> - }
> - unlock(vecs->lock);
>
> /*
> * start threads
> @@ -2107,20 +2167,32 @@ child (void * param)
> }
> pthread_attr_destroy(&misc_attr);
>
> - running_state = DAEMON_RUNNING;
> #ifdef USE_SYSTEMD
> - sd_notify(0, "READY=1\nSTATUS=running");
> + sd_notify(0, "READY=1");
> #endif
>
> - /*
> - * exit path
> - */
> - while(sem_wait(&exit_sem) != 0); /* Do nothing */
> + while (running_state != DAEMON_SHUTDOWN) {
> + pthread_cleanup_push(config_cleanup, NULL);
> + pthread_mutex_lock(&config_lock);
> + if (running_state != DAEMON_CONFIGURE &&
> + running_state != DAEMON_SHUTDOWN) {
> + pthread_cond_wait(&config_cond, &config_lock);
> + }
> + pthread_cleanup_pop(1);
> + if (running_state == DAEMON_CONFIGURE) {
> + pthread_cleanup_push(cleanup_lock, &vecs->lock);
> + lock(vecs->lock);
> + pthread_testcancel();
> + if (!need_to_delay_reconfig(vecs)) {
> + reconfigure(vecs);
> + } else {
> + conf->delayed_reconfig = 1;
> + }
> + lock_cleanup_pop(vecs->lock);
> + post_config_state(DAEMON_IDLE);
> + }
> + }
>
> -#ifdef USE_SYSTEMD
> - sd_notify(0, "STATUS=shutdown");
> -#endif
> - running_state = DAEMON_SHUTDOWN;
> lock(vecs->lock);
> if (conf->queue_without_daemon == QUE_NO_DAEMON_OFF)
> vector_foreach_slot(vecs->mpvec, mpp, i)
> @@ -2253,7 +2325,6 @@ main (int argc, char *argv[])
> int foreground = 0;
>
> logsink = 1;
> - running_state = DAEMON_INIT;
> dm_init();
>
> if (getuid() != 0) {
> diff --git a/multipathd/main.h b/multipathd/main.h
> index d1a6d71..10b3a6d 100644
> --- a/multipathd/main.h
> +++ b/multipathd/main.h
> @@ -7,6 +7,7 @@ enum daemon_status {
> DAEMON_INIT,
> DAEMON_START,
> DAEMON_CONFIGURE,
> + DAEMON_IDLE,
> DAEMON_RUNNING,
> DAEMON_SHUTDOWN,
> };
> @@ -26,6 +27,7 @@ int ev_remove_path (struct path *, struct vectors *);
> int ev_add_map (char *, char *, struct vectors *);
> int ev_remove_map (char *, char *, int, struct vectors *);
> void sync_map_state (struct multipath *);
> +int set_config_state(enum daemon_status);
> void * mpath_alloc_prin_response(int prin_sa);
> int prin_do_scsi_ioctl(char *, int rq_servact, struct prin_resp * resp,
> int noisy);
> --
> 2.6.6
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
