| Message ID | 20181023220552.1306-6-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipathd: make uxlsnr errors really fatal | expand |
On Wed, Oct 24, 2018 at 12:05:50AM +0200, Martin Wilck wrote: > Commit 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. > > Fixes: ee01e841 "multipathd: handle errors in uxlsnr as fatal" > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > multipathd/main.c | 43 ++++++++++++++++++++++++++++++++----------- > 1 file changed, 32 insertions(+), 11 deletions(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 50f69171..c71e7d03 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -205,9 +205,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; > > @@ -217,7 +216,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) > @@ -1511,6 +1517,10 @@ uxlsnrloop (void * ap) > condlog(1, "Failed to init uxsock listener"); > return NULL; > } > + > + /* 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); > @@ -2726,11 +2736,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); I don't really understand the need for __post_config_state(). Couldn't you just call post_config_state(DAEMON_IDLE), and then grab the config_lock after checking that the thread was created successfully. If the scheduler decides to run the uxlsnrloop thread before continuing with the child thread, it would be better if uxlsnrloop thread was free to grab the config lock anyway. At any rate, what you have is perfectly correct, so Reviewed-by: Benjamin Marzinsk <bmarzins@redhat.com> > + 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)) { > @@ -2753,10 +2778,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 > -- > 2.19.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Fri, 2018-10-26 at 15:19 -0500, Benjamin Marzinski wrote: > On Wed, Oct 24, 2018 at 12:05:50AM +0200, Martin Wilck wrote: > > Commit 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. > > > > Fixes: ee01e841 "multipathd: handle errors in uxlsnr as fatal" > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > --- > > multipathd/main.c | 43 ++++++++++++++++++++++++++++++++----------- > > 1 file changed, 32 insertions(+), 11 deletions(-) > > > > diff --git a/multipathd/main.c b/multipathd/main.c > > index 50f69171..c71e7d03 100644 > > --- a/multipathd/main.c > > +++ b/multipathd/main.c > > @@ -2726,11 +2736,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); > > I don't really understand the need for > __post_config_state(). Couldn't > you just call post_config_state(DAEMON_IDLE), and then grab the > config_lock after checking that the thread was created > successfully. If > the scheduler decides to run the uxlsnrloop thread before continuing > with the child thread, it would be better if uxlsnrloop thread was > free > to grab the config lock anyway. I wanted to enforce ordering here. The uxlsnr won't be able to grab the lock and change running_state until the child enters pthread_cond_wait() below, which is intentional. running_state shouldn't be read or written without holding the lock, so this actually saves us an unlock/lock operation, as child() would otherwise need to release the lock before calling pthread_create(), and reacquire it afterwards. This way the startup sequence is fully deterministic and doesn't depend on scheduler decisions. I like it that way :-) > At any rate, what you have is perfectly correct, so > > Reviewed-by: Benjamin Marzinsk <bmarzins@redhat.com> Thanks a lot. Martin > > > + 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)) { > > @@ -2753,10 +2778,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 > > -- > > 2.19.1
diff --git a/multipathd/main.c b/multipathd/main.c index 50f69171..c71e7d03 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -205,9 +205,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; @@ -217,7 +216,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) @@ -1511,6 +1517,10 @@ uxlsnrloop (void * ap) condlog(1, "Failed to init uxsock listener"); return NULL; } + + /* 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); @@ -2726,11 +2736,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)) { @@ -2753,10 +2778,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
Commit 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. Fixes: ee01e841 "multipathd: handle errors in uxlsnr as fatal" Signed-off-by: Martin Wilck <mwilck@suse.com> --- multipathd/main.c | 43 ++++++++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 11 deletions(-)