[dm-devel] [PATCH] multipathd: use nanosleep for sleeping

[Benjamin Marzinski]

On Tue, Mar 06, 2018 at 09:37:13PM +0100, Martin Wilck wrote:
> On Mon, 2018-03-05 at 19:20 -0600, Benjamin Marzinski wrote:
> > In order to safely use SIGALRM in a multi-threaded program, only one
> > thread can schedule and wait on SIGALRM at a time. All other threads
> > must have SIGALRM blocked, and be unable to schedule an alarm. The
> > strict_timing code in checkerloop was unblocking SIGALRM, and calling
> > setitimer(), without any locking.  Instead, it should use nanosleep()
> > to sleep for the correct length of time, since that doesn't depend or
> > interfere with signals. io_err_stat_loop() was calling usleep()
> > without
> > any locking. According to the POSIX standards, the result of a
> > SIGARLM
> > occuring during a usleep call is undefined, even if the calling
> > thread
> > blocks the signal. While this is unlikely to cause a problem on a
> > modern linux system, nanosleep is guaranteed to not interact with
> > SIGALRM, as long as the signal is blocked by the calling thread.
> 
> The libc manual says "On GNU systems, it is safe to use ‘sleep’ and
> ‘SIGALRM’ in the same program, because ‘sleep’ does not work by means
> of ‘SIGALRM’." Actually, if you watch it under strace, you can see that
> the sleep(1) call in checkerloop (without strict timing) translates
> into a nanosleep() call. 
> 
> I'm generally with you here, it's better to be clearly on the safe
> side. Anyway, I'd replaced the "usleep" call by "pselect" already, see
> below.
> 
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > ---
> >  libmultipath/io_err_stat.c |  3 ++-
> >  multipathd/main.c          | 27 +++++++++------------------
> >  2 files changed, 11 insertions(+), 19 deletions(-)
> > 
> > diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c
> > index 5b10f03..555af5d 100644
> > --- a/libmultipath/io_err_stat.c
> > +++ b/libmultipath/io_err_stat.c
> > @@ -697,8 +697,9 @@ static void *io_err_stat_loop(void *data)
> >  
> >  	mlockall(MCL_CURRENT | MCL_FUTURE);
> >  	while (1) {
> > +		struct timespec delay = {0, 100000000}; /* .1 sec */
> >  		service_paths();
> > -		usleep(100000);
> > +		nanosleep(&delay, NULL);
> >  	}
> 
> As you probably noticed, my late patch "libmultipath: fix tur checker
> locking" had replaced this usleep call by a pselect() call. pselect()
> doesn't use SIGALRM, either.

Actually, I'm getting a little lost in the unmerged patch queue. Your
method works fine here.
 
> >  
> >  	pthread_cleanup_pop(1);
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 889670c..55b92be 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -1834,7 +1834,6 @@ checkerloop (void *ap)
> >  	struct path *pp;
> >  	int count = 0;
> >  	unsigned int i;
> > -	struct itimerval timer_tick_it;
> >  	struct timespec last_time;
> >  	struct config *conf;
> >  
> > @@ -1852,8 +1851,7 @@ checkerloop (void *ap)
> >  
> >  	while (1) {
> >  		struct timespec diff_time, start_time, end_time;
> > -		int num_paths = 0, ticks = 0, signo, strict_timing,
> > rc = 0;
> > -		sigset_t mask;
> > +		int num_paths = 0, ticks = 0, strict_timing, rc = 0;
> >  
> >  		if (clock_gettime(CLOCK_MONOTONIC, &start_time) !=
> > 0)
> >  			start_time.tv_sec = 0;
> > @@ -1941,25 +1939,18 @@ checkerloop (void *ap)
> >  		if (!strict_timing)
> >  			sleep(1);
> 
> Hm, if the usleep() in the io_error thread was a problem, why wouldn't
> the sleep() here be one, too? We're not holding the vecs lock here.
> Btw there's a couple more sleep() and usleep() calls scattered around
> the code. Anyway, it seems that they all resolve to nanosleep().

I checked all the other usleep calls, and they are all covered by the
vecs lock.  I surprisingly didn't even think to check sleep calls, since
I knew that they were safe.

> >  		else {
> > -			timer_tick_it.it_interval.tv_sec = 0;
> > -			timer_tick_it.it_interval.tv_usec = 0;
> >  			if (diff_time.tv_nsec) {
> > -				timer_tick_it.it_value.tv_sec = 0;
> > -				timer_tick_it.it_value.tv_usec =
> > +				diff_time.tv_sec = 0;
> > +				diff_time.tv_nsec =
> >  				     1000UL * 1000 * 1000 -
> > diff_time.tv_nsec;
> > -			} else {
> > -				timer_tick_it.it_value.tv_sec = 1;
> > -				timer_tick_it.it_value.tv_usec = 0;
> > -			}
> > -			setitimer(ITIMER_REAL, &timer_tick_it,
> > NULL);
> > +			} else
> > +				diff_time.tv_sec = 1;
> >  
> > -			sigemptyset(&mask);
> > -			sigaddset(&mask, SIGALRM);
> >  			condlog(3, "waiting for %lu.%06lu secs",
> > -				timer_tick_it.it_value.tv_sec,
> > -				timer_tick_it.it_value.tv_usec);
> > -			if (sigwait(&mask, &signo) != 0) {
> > -				condlog(3, "sigwait failed with
> > error %d",
> > +				diff_time.tv_sec,
> > +				diff_time.tv_nsec / 1000);
> > +			if (nanosleep(&diff_time, NULL) != 0) {
> > +				condlog(3, "nanosleep failed with
> > error %d",
> >  					errno);
> >  				conf = get_multipath_config();
> >  				conf->strict_timing = 0;
> 
> I'm sure this works, but have you considered achieving the same result
> simply by using create_timer with a different signal than SIGALRM?
> Thinking about it, I'm sure the strict_timing code can be drastically
> simplified by using an interval timer and a realtime signal.

I don't see how it could get much simpler, and all things being equal,
I'd rather that multipathd did less with signals. But if you want to
redo this with a timer, I don't haven any strong feelings against it.

> And we still have the code using SIGALRM in lock_file(), is it your
> intention to leave that as-is and change all other users of SIGALRM?

We call lock_file under the vecs lock, so I really don't want it to
block indefinitely, since that would bring multipathd to a standstill.
However, we need to make sure that multipath and multipathd don't
write to these files at the same time, or they could corrupt them.
If you can come up with a solution that keeps them from corrupting the
files, or discarding the changes that the other one makes, or locking
indefinitely, and doesn't involve signals, I'd be interested to see it.
However, I've never seen a bug with what we currently have, so it's not
something that I feel needs changing.

-Ben

> Regards
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)