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

[Martin Wilck]

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.

>  
>  	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().

>  		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.

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?

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)