multipathd: make uxlsnr errors really fatal
v3 cover multipathd: make uxlsnr errors really fatal
Hi Christophe,
this series, based on top of the recently submitted "various multipath-tools patches (v2)" and "checkers overhaul (v3)" series, fixes a problem that I recently observed: despite ee01e841 "multipathd: handle errors in uxlsnr as fatal", multipathd sometimes doesn't quit when the socket setup fails.
While at that, I stumbled upon init_path_check_interval(), wondered about its purpose, and came to the conclusion that can be quite easily obsoleted.
----
Changes v2->v3:
04/07: Fixed cleanup code path (Ben).
Changes v1->v2:
04/07: Fixed problem observed by Ben.
Martin Wilck (7):
libmultipath: set pp->checkint in store_pathinfo()
multipathd: remove init_path_check_interval()
multipathd: print error message if checkint is not initialized
multipathd: open client socket early
multipathd: set DAEMON_CONFIGURE from uxlsnr thread
multipathd: make DAEMON_SHUTDOWN a terminal state
multipathd: only grab conf once for filter_path()
libmultipath/defaults.h | 1 + libmultipath/dict.c | 14 ++++- libmultipath/discovery.c | 1 + libmultipath/structs.c | 1 + multipathd/cli_handlers.c | 10 ++-- multipathd/main.c | 109 +++++++++++++++++++++++++------------- multipathd/uxlsnr.c | 14 +---- multipathd/uxlsnr.h | 5 +- 8 files changed, 98 insertions(+), 57 deletions(-)
Signature · 1 line
2.19.1
PATCH v3 1/7 libmultipath: set pp->checkint in store_pathinfo()
store_pathinfo is called with valid conf pointer anyway, so checkint is available. pp->checkint is now valid for every path after path_discovery().
This fixes a bad conf access in cli_add_path().
--- libmultipath/discovery.c | 1 + multipathd/cli_handlers.c | 1 - multipathd/main.c | 2 -- 3 files changed, 1 insertion(+), 3 deletions(-)
libmultipath/discovery.c +1 -0
@@ -103,6 +103,7 @@ store_pathinfo (vector pathvec, struct config *conf, err =store_path (pathvec, pp);if (err) goto out; + pp->checkint = conf->checkint; out:if (err)
multipathd/cli_handlers.c +0 -1
@@ -736,7 +736,6 @@ cli_add_path (void * v, char ** reply, int * len, void * data)condlog (0 ,"%s: failed to store path info" , param);return 1 ; } - pp->checkint = conf->checkint; }return ev_add_path (pp, vecs,1 ); blacklisted:
multipathd/main.c +0 -2
@@ -2324,8 +2324,6 @@ configure (struct vectors * vecs)free_path (pp); i--; } -else - pp->checkint = conf->checkint;pthread_cleanup_pop (1 ); }if (map_discovery (vecs)) {
Signature · 1 line
2.19.1
PATCH v3 2/7 multipathd: remove init_path_check_interval()
In reply to Martin WilckAfter "libmultipath: set pp->checkint in store_pathinfo()", pp-checkint should always be properly initialized, so this code is not needed any more.
--- multipathd/main.c | 14 -------------- 1 file changed, 14 deletions(-)
multipathd/main.c +0 -14
@@ -2133,19 +2133,6 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)return 1 ; } -static void init_path_check_interval (struct vectors *vecs) -{ -struct config *conf; -struct path *pp; -unsigned int i; - - vector_foreach_slot (vecs->pathvec, pp, i) { - conf =get_multipath_config (); - pp->checkint = conf->checkint; -put_multipath_config (conf); - } -} -static void *checkerloop (void *ap) {
@@ -2729,7 +2716,6 @@ child (void * param)*/ post_config_state (DAEMON_CONFIGURE ); -init_path_check_interval (vecs);if (poll_dmevents) {if (init_dmevent_waiter (vecs)) {
Signature · 1 line
2.19.1
PATCH v3 3/7 multipathd: print error message if checkint is not initialized
In reply to Martin WilckThis is just a safety measure in case I overlooked something wrt the checkint initialization. It could be reverted once we know the error message isn't triggered.
--- libmultipath/defaults.h | 1 + libmultipath/dict.c | 14 ++++++++++++-- libmultipath/structs.c | 1 + multipathd/main.c | 6 ++++++ 4 files changed, 20 insertions(+), 2 deletions(-)
libmultipath/defaults.h +1 -0
@@ -46,6 +46,7 @@#define DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT 1#define DEFAULT_ALL_TG_PT ALL_TG_PT_OFF +#define CHECKINT_UNDEF (~0U)#define DEFAULT_CHECKINT 5#define MAX_CHECKINT (a) (a << 2)
libmultipath/dict.c +12 -2
@@ -264,7 +264,17 @@ snprint_mp_ ## option (struct config *conf, char * buff, int len, \return function (buff, len, mpe->option); \ } -declare_def_handler(checkint, set_int) +static int checkint_handler (struct config *conf,vector strvec) +{ +int rc =set_int (strvec, &conf->checkint); + +if (rc) +return rc; +if (conf->checkint ==CHECKINT_UNDEF ) + conf->checkint--; +return 0 ; +} + declare_def_snprint(checkint, print_int)declare_def_handler (max_checkint, set_int)
@@ -1563,7 +1573,7 @@ init_keywords(vector keywords) {install_keyword_root ("defaults" ,NULL );install_keyword ("verbosity" , &def_verbosity_handler, &snprint_def_verbosity); -install_keyword ("polling_interval" , &def_checkint_handler, &snprint_def_checkint); +install_keyword ("polling_interval" , &checkint_handler, &snprint_def_checkint);install_keyword ("max_polling_interval" , &def_max_checkint_handler, &snprint_def_max_checkint);install_keyword ("reassign_maps" , &def_reassign_maps_handler, &snprint_def_reassign_maps);install_keyword ("multipath_dir" , &def_multipath_dir_handler, &snprint_def_multipath_dir);
libmultipath/structs.c +1 -0
@@ -100,6 +100,7 @@ alloc_path (void) pp->fd =-1 ; pp->tpgs =TPGS_UNDEF ; pp->priority =PRIO_UNDEF ; + pp->checkint =CHECKINT_UNDEF ;checker_clear (&pp->checker);dm_path_to_gen (pp)->ops = &dm_gen_path_ops; pp->hwe =vector_alloc ();
multipathd/main.c +6 -0
@@ -1842,6 +1842,12 @@ check_path (struct vectors * vecs, struct path * pp, int ticks) max_checkint = conf->max_checkint; verbosity = conf->verbosity;put_multipath_config (conf); + +if (pp->checkint ==CHECKINT_UNDEF ) { +condlog (0 ,"%s: BUG: checkint is not set" , pp->dev); + pp->checkint = checkint; + }; +if (!pp->mpp && pp->initialized ==INIT_MISSING_UDEV ) {if (pp->retriggers < retrigger_tries) { condlog(2, "%s: triggering change event to reinitialize",
Signature · 1 line
2.19.1
PATCH v3 4/7 multipathd: open client socket early
In reply to Martin WilckOpen the unix socket in multipathd code and pass the fd to uxsock_listen(). This will enable us to make the main thread wait for successful socket initialization in a follow-up patch.
--- multipathd/main.c | 27 ++++++++++++++++++++++----- multipathd/uxlsnr.c | 14 ++------------ multipathd/uxlsnr.h | 5 +++-- 3 files changed, 27 insertions(+), 19 deletions(-)
multipathd/main.c +22 -5
@@ -66,6 +66,7 @@ static int use_watchdog;#include "pgpolicies.h" #include "uevent.h" #include "log.h" +#include "uxsock.h" #include "mpath_cmd.h" #include "mpath_persist.h"
@@ -1496,12 +1497,24 @@ uevqloop (void * ap)static void *uxlsnrloop (void * ap) { +long ux_sock; + +pthread_cleanup_push (rcu_unregister,NULL ); +rcu_register_thread (); + + ux_sock =ux_socket_listen (DEFAULT_SOCKET ); +if (ux_sock ==-1 ) { +condlog (1 ,"could not create uxsock: %d" , errno); +exit_daemon (); + goto out; + } +pthread_cleanup_push (uxsock_cleanup, (void *)ux_sock); +if (cli_init ()) {condlog (1 ,"Failed to init uxsock listener" ); -return NULL ; +exit_daemon (); + goto out_sock; } -pthread_cleanup_push (rcu_unregister,NULL ); -rcu_register_thread ();set_handler_callback (LIST +PATHS , cli_list_paths);set_handler_callback (LIST +PATHS +FMT , cli_list_paths_fmt);set_handler_callback (LIST +PATHS +RAW +FMT , cli_list_paths_raw);
@@ -1556,8 +1569,12 @@ uxlsnrloop (void * ap)set_handler_callback (UNSETPRKEY +MAP , cli_unsetprkey);umask (077 ); -uxsock_listen (&uxsock_trigger, ap); -pthread_cleanup_pop (1 ); +uxsock_listen (&uxsock_trigger, ux_sock, ap); + +out_sock: +pthread_cleanup_pop (1 );/* uxsock_cleanup */ +out: +pthread_cleanup_pop (1 );/* rcu_unregister */ return NULL ; }
multipathd/uxlsnr.c +2 -12
@@ -165,24 +165,15 @@ void uxsock_cleanup(void *arg)/* * entry point */ -void *uxsock_listen (uxsock_trigger_fn uxsock_trigger,void * trigger_data) +void *uxsock_listen (uxsock_trigger_fn uxsock_trigger,long ux_sock, +void * trigger_data) { -long ux_sock;int rlen;char *inbuf;char *reply;sigset_t mask;int old_clients =MIN_POLLS ; - ux_sock =ux_socket_listen (DEFAULT_SOCKET ); - -if (ux_sock ==-1 ) { -condlog (1 ,"could not create uxsock: %d" , errno); -exit_daemon (); - } - -pthread_cleanup_push (uxsock_cleanup, (void *)ux_sock); -condlog (3 ,"uxsock: startup listener" ); polls = (struct pollfd *)MALLOC ((MIN_POLLS +1 ) *sizeof (struct pollfd ));if (!polls) {
@@ -322,6 +313,5 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data) } } -pthread_cleanup_pop (1 );return NULL ; }
multipathd/uxlsnr.h +3 -2
@@ -5,7 +5,8 @@typedef int (uxsock_trigger_fn )(char *,char **,int *,bool ,void *); -void *uxsock_listen (uxsock_trigger_fn uxsock_trigger, -void * trigger_data); +void uxsock_cleanup (void *arg); +void *uxsock_listen (uxsock_trigger_fn uxsock_trigger,long ux_sock, +void * trigger_data);#endif
Signature · 1 line
2.19.1
Re: [PATCH v3 4/7] multipathd: open client socket early
Quoted 4 lines · Martin Wilck
Open the unix socket in multipathd code and pass the fd to
uxsock_listen(). This will enable us to make the main thread wait for successful socket initialization in a follow-up patch.
Quoted 117 lines
Signed-off-by Martin Wilck <[email protected]>--- multipathd/main.c | 27 ++++++++++++++++++++++----- multipathd/uxlsnr.c | 14 ++------------ multipathd/uxlsnr.h | 5 +++-- 3 files changed, 27 insertions(+), 19 deletions(-)multipathd/main.c +22
-5@@ -66,6 +66,7 @@ static int use_watchdog;#include "pgpolicies.h" #include "uevent.h" #include "log.h" +#include "uxsock.h" #include "mpath_cmd.h" #include "mpath_persist.h" @@ -1496,12 +1497,24 @@ uevqloop (void * ap)static void *uxlsnrloop (void * ap) { +long ux_sock; + +pthread_cleanup_push (rcu_unregister,NULL ); +rcu_register_thread (); + + ux_sock =ux_socket_listen (DEFAULT_SOCKET ); +if (ux_sock ==-1 ) { +condlog (1 ,"could not create uxsock: %d" , errno); +exit_daemon (); + goto out; + } +pthread_cleanup_push (uxsock_cleanup, (void *)ux_sock); +if (cli_init ()) {condlog (1 ,"Failed to init uxsock listener" ); -return NULL ; +exit_daemon (); + goto out_sock; } -pthread_cleanup_push (rcu_unregister,NULL ); -rcu_register_thread ();set_handler_callback (LIST +PATHS , cli_list_paths);set_handler_callback (LIST +PATHS +FMT , cli_list_paths_fmt);set_handler_callback (LIST +PATHS +RAW +FMT , cli_list_paths_raw);@@ -1556,8 +1569,12 @@ uxlsnrloop (void * ap)set_handler_callback (UNSETPRKEY +MAP , cli_unsetprkey);umask (077 ); -uxsock_listen (&uxsock_trigger, ap); -pthread_cleanup_pop (1 ); +uxsock_listen (&uxsock_trigger, ux_sock, ap); + +out_sock: +pthread_cleanup_pop (1 );/* uxsock_cleanup */ +out: +pthread_cleanup_pop (1 );/* rcu_unregister */ return NULL ; }multipathd/uxlsnr.c +2
-12@@ -165,24 +165,15 @@ void uxsock_cleanup(void *arg)/* * entry point */ -void *uxsock_listen (uxsock_trigger_fn uxsock_trigger,void * trigger_data) +void *uxsock_listen (uxsock_trigger_fn uxsock_trigger,long ux_sock, +void * trigger_data) { -long ux_sock;int rlen;char *inbuf;char *reply;sigset_t mask;int old_clients =MIN_POLLS ; - ux_sock =ux_socket_listen (DEFAULT_SOCKET ); - -if (ux_sock ==-1 ) { -condlog (1 ,"could not create uxsock: %d" , errno); -exit_daemon (); - } - -pthread_cleanup_push (uxsock_cleanup, (void *)ux_sock); -condlog (3 ,"uxsock: startup listener" ); polls = (struct pollfd *)MALLOC ((MIN_POLLS +1 ) *sizeof (struct pollfd ));if (!polls) {@@ -322,6 +313,5 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data) } } -pthread_cleanup_pop (1 );return NULL ; }multipathd/uxlsnr.h +3
-2@@ -5,7 +5,8 @@typedef int (uxsock_trigger_fn )(char *,char **,int *,bool ,void *); -void *uxsock_listen (uxsock_trigger_fn uxsock_trigger, -void * trigger_data); +void uxsock_cleanup (void *arg); +void *uxsock_listen (uxsock_trigger_fn uxsock_trigger,long ux_sock, +void * trigger_data);#endif Signature · 1 line
2.19.1
PATCH v3 5/7 multipathd: set DAEMON_CONFIGURE from uxlsnr thread
In reply to Martin WilckCommit ee01e841 had the intention to make multipathd quit if the client socket couldn't be set up, because the unix socket listener is vital for signal handling in multipathd. But during startup, this condition might be lost if the main thread doesn't wait for the unix listener to initialize.
--- multipathd/main.c | 43 ++++++++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 11 deletions(-)
multipathd/main.c +32 -11
@@ -207,9 +207,8 @@ static void config_cleanup(void *arg)pthread_mutex_unlock (&config_lock); } -void post_config_state (enum daemon_status state) +static void __post_config_state (enum daemon_status state) { -pthread_mutex_lock (&config_lock);if (state != running_state) {enum daemon_status old_state = running_state;
@@ -219,7 +218,14 @@ void post_config_state(enum daemon_status state)do_sd_notify (old_state);#endif } -pthread_mutex_unlock (&config_lock); +} + +void post_config_state (enum daemon_status state) +{ +pthread_mutex_lock (&config_lock); +pthread_cleanup_push (config_cleanup,NULL ); +__post_config_state (state); +pthread_cleanup_pop (1 ); }int set_config_state (enum daemon_status state)
@@ -1515,6 +1521,10 @@ uxlsnrloop (void * ap)exit_daemon (); goto out_sock; } + +/* Tell main thread that thread has started */ +post_config_state (DAEMON_CONFIGURE ); +set_handler_callback (LIST +PATHS , cli_list_paths);set_handler_callback (LIST +PATHS +FMT , cli_list_paths_fmt);set_handler_callback (LIST +PATHS +RAW +FMT , cli_list_paths_raw);
@@ -2734,11 +2744,26 @@ child (void * param)*/ conf =NULL ; -/* -* Signal start of configuration -*/ -post_config_state (DAEMON_CONFIGURE ); +pthread_cleanup_push (config_cleanup,NULL ); +pthread_mutex_lock (&config_lock); +__post_config_state (DAEMON_IDLE ); + rc =pthread_create (&uxlsnr_thr, &misc_attr, uxlsnrloop, vecs); +if (!rc) { +/* Wait for uxlsnr startup */ +while (running_state ==DAEMON_IDLE ) +pthread_cond_wait (&config_cond, &config_lock); + } +pthread_cleanup_pop (1 ); + +if (rc) { +condlog (0 ,"failed to create cli listener: %d" , rc); + goto failed; + } +else if (running_state !=DAEMON_CONFIGURE ) { +condlog (0 ,"cli listener failed to start" ); + goto failed; + }if (poll_dmevents) {if (init_dmevent_waiter (vecs)) {
@@ -2761,10 +2786,6 @@ child (void * param) goto failed; }pthread_attr_destroy (&uevent_attr); -if ((rc =pthread_create (&uxlsnr_thr, &misc_attr, uxlsnrloop, vecs))) { -condlog (0 ,"failed to create cli listener: %d" , rc); - goto failed; - }/* * start threads
Signature · 1 line
2.19.1
PATCH v3 6/7 multipathd: make DAEMON_SHUTDOWN a terminal state
In reply to Martin WilckIt can happen that, before the main thread reacts on DAEMON_SHUTDOWN and starts cancelling threads, another thread resets the state to something else. Fix that.
--- multipathd/cli_handlers.c | 9 +++++++-- multipathd/main.c | 10 +++++++--- 2 files changed, 14 insertions(+), 5 deletions(-)
multipathd/cli_handlers.c +7 -2
@@ -1124,12 +1124,17 @@ cli_switch_group(void * v, char ** reply, int * len, void * data)int cli_reconfigure (void * v,char ** reply,int * len,void * data) { +int rc; +condlog (2 ,"reconfigure (operator)" ); -if (set_config_state (DAEMON_CONFIGURE ) ==ETIMEDOUT ) { + rc =set_config_state (DAEMON_CONFIGURE ); +if (rc ==ETIMEDOUT ) {condlog (2 ,"timeout starting reconfiguration" );return 1 ; - } + }else if (rc ==EINVAL ) +/* daemon shutting down */ +return 1 ;return 0 ; }
multipathd/main.c +7 -3
@@ -209,7 +209,7 @@ static void config_cleanup(void *arg)static void __post_config_state (enum daemon_status state) { -if (state != running_state) { +if (state != running_state && running_state !=DAEMON_SHUTDOWN ) {enum daemon_status old_state = running_state; running_state = state;
@@ -237,7 +237,9 @@ int set_config_state(enum daemon_status state)if (running_state != state) {enum daemon_status old_state = running_state; -if (running_state !=DAEMON_IDLE ) { +if (running_state ==DAEMON_SHUTDOWN ) + rc =EINVAL ; +else if (running_state !=DAEMON_IDLE ) {struct timespec ts;clock_gettime (CLOCK_MONOTONIC , &ts);
@@ -2212,7 +2214,9 @@ checkerloop (void *ap)if (rc ==ETIMEDOUT ) {condlog (4 ,"timeout waiting for DAEMON_IDLE" );continue ; - } + }else if (rc ==EINVAL ) +/* daemon shutdown */ +break ;pthread_cleanup_push (cleanup_lock, &vecs->lock);lock (&vecs->lock);
Signature · 1 line
2.19.1
PATCH v3 7/7 multipathd: only grab conf once for filter_path()
In reply to Martin WilckThis saves a possibly large number of cleanup push/pop calls and slightly improves readability.
--- multipathd/main.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
multipathd/main.c +4 -3
@@ -2340,16 +2340,17 @@ configure (struct vectors * vecs) goto fail; } + conf =get_multipath_config (); +pthread_cleanup_push (put_multipath_config, conf); vector_foreach_slot (vecs->pathvec, pp, i){ - conf =get_multipath_config (); -pthread_cleanup_push (put_multipath_config, conf);if (filter_path (conf, pp) >0 ){vector_del_slot (vecs->pathvec, i);free_path (pp); i--; } -pthread_cleanup_pop (1 ); } +pthread_cleanup_pop (1 ); +if (map_discovery (vecs)) {condlog (0 ,"configure failed at map discovery" ); goto fail;
Signature · 1 line
2.19.1
Merged.
Thanks.
Quoted 42 lines · Martin Wilck
Hi Christophe,
this series, based on top of the recently submitted "various multipath-tools patches (v2)" and "checkers overhaul (v3)" series, fixes a problem that I recently observed: despite
ee01e841"multipathd: handle errors in uxlsnr as fatal", multipathd sometimes doesn't quit when the socket setup fails.While at that, I stumbled upon
init_path_check_interval(), wondered about its purpose, and came to the conclusion that can be quite easily obsoleted.----
Changesv2->v3:
04/07: Fixed cleanup code path (Ben).Changes
v1->v2:
04/07: Fixed problem observed by Ben.Martin Wilck (7):
libmultipath: setpp->checkintinstore_pathinfo()
multipathd: removeinit_path_check_interval()
multipathd: print error message if checkint is not initialized
multipathd: open client socket early
multipathd: setDAEMON_CONFIGUREfrom uxlsnr thread
multipathd: makeDAEMON_SHUTDOWNa terminal state
multipathd: only grab conf once forfilter_path()libmultipath/defaults.h | 1 + libmultipath/dict.c | 14 ++++- libmultipath/discovery.c | 1 + libmultipath/structs.c | 1 + multipathd/cli_handlers.c | 10 ++-- multipathd/main.c | 109 +++++++++++++++++++++++++------------- multipathd/uxlsnr.c | 14 +---- multipathd/uxlsnr.h | 5 +- 8 files changed, 98 insertions(+), 57 deletions(-)--
2.19.1