Re: [PATCH 5/7] multipathd: set DAEMON_CONFIGURE from uxlsnr thread

From: Martin Wilck <mwilck@suse.com>
To: Benjamin Marzinski <bmarzins@redhat.com>, mwilck+gmail@suse.de
Cc: dm-devel@redhat.com
Subject: Re: [PATCH 5/7] multipathd: set DAEMON_CONFIGURE from uxlsnr thread
Date: Tue, 30 Oct 2018 17:55:06 +0100	[thread overview]
Message-ID: <a631d7c5aff604bb0b86bf919d9f6f2125e7f131.camel@suse.com> (raw)
In-Reply-To: <20181026201923.GJ7100@octiron.msp.redhat.com>

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
-- 
Dr. Martin Wilck <mwilck@suse.com>, 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@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel