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@xxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
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
--
Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
