[PATCH] multipathd: use nanosleep for sleeping

[PATCH] multipathd: use nanosleep for sleeping

[Benjamin Marzinski]

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.

Signed-off-by: Benjamin Marzinski <bmarzins@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);
 	}
 
 	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);
 		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;
-- 
2.7.4

Re: [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@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@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

Re: [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@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@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

Re: [PATCH] multipathd: use nanosleep for sleeping

[Martin Wilck]

On Tue, 2018-03-06 at 19:21 -0600, Benjamin Marzinski wrote:
> On Tue, Mar 06, 2018 at 09:37:13PM +0100, Martin Wilck wrote:
> > 
> > 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.

I just sent it with the subject "[RFC PATCH] multipathd: strict_timing
without signals". Please have a look. The patch actually does two
different things - a) convert the current setitimer-based code to a
custom timer that works without SIGALRM (without any explicit signal,
actually), and b) use the it_interval field to wakeup automatically at
fixed intervals, rather than re-calculate the wait time from tick to
tick. 

Whether or not this is simpler than the current code (or your approach,
for that matter) is up to personal judgement. My patch has a positive
line count, but it requires less code inside the checkerloop itself.

Cheers,
Martin

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