| Message ID | 1520299256-27243-1-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
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
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
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
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;
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(-)