Re: [PATCH 06/12] multipathd: use nanosleep for strict timing

[Martin Wilck <mwilck@suse.com>]

On Wed, 2018-03-14 at 12:46 -0500, 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.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 6ba6131..ce914ab 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> 

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

Nitpick: the only realistic error code for nanosleep is EINTR, in which
case we IMO don't need the log message, because just means one of the
expected signals arrived.

As stated earlier, I'd prefer a kernel interval timer for
strict_timing. I'm unsure why it hasn't been done that way in the first
place. Anyway, that can be discussed later, therefore:

Reviewed-by: Martin Wilck <mwilck@suse.com> 

... with the nit above.

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