Re: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

2018-03-06 Thread Martin Wilck
On Mon, 2018-03-05 at 18:46 -0600, Benjamin Marzinski wrote:
> 
> This is prety close to what I had in mind. The only things missing,
> that
> I noticed when I was looking at multipathd's signal handling, is
> dealing
> with SIGUSR2 in io_err_stat.c, which it sounds like you have already
> spotted, and changing the calls to setitimer and usleep outside of
> the
> vecs lock to use nanosleep, so they don't interact with SIGALRM.
> 
> I'll send a patch based on yours, to fix these.

I'm curious to see what you come up with. I've something on my
workbench which seems to work well but needs some cleanup. The idea is
to have a dedicated "alarm dispatcher" thread that deals with kernel
timers exclusively, and forwards expiry events to the threads that have
asked for it. The dispatcher maintains a list of active alarms and sets
a kernel timer to the expiry time of the first alarm in the list.

Cheers,
Martin

-- 
Dr. Martin Wilck , 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: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

2018-03-05 Thread Benjamin Marzinski
On Mon, Mar 05, 2018 at 06:28:05PM +0100, Martin Wilck wrote:
> Hello Bart,
> 
> On Mon, 2018-03-05 at 16:27 +, Bart Van Assche wrote:
> > On Sat, 2018-03-03 at 01:31 +0100, Martin Wilck wrote:
> > > So, there's no reason not to block them, right? Is it expected
> > > behavior
> > > that a user running 'kill -USR2 $(pidof multipathd)' terminates the
> > > process? Why do you think these signals should interrupt ppoll()
> > > although the uxlsnr can't can't handle them? Isn't it sufficient
> > > that
> > > they're handled by the threads that are meant to do that?
> > 
> > Blocking all signals except the ones for which we installed a handler
> > sounds
> > weird to me. I think users expect daemons to process signals instead
> > of
> > blocking all but a specific set of signals. This is a rather
> > philosophical
> > argument and not an argument of which I think that it is strong
> > enough to
> > prevent this patch of being integrated in the upstream multipath-
> > tools
> > repository.
> 
> Thank you. Meanwhile, I found that this patch requires a change in the
> "marginal paths" code. I'll post an official patch as update.
> 
> Regarding the philosophical discussion, may I remind you of your own
> words from 534ec4c?
> 
> The POSIX standard mentions that the only way to guarantee that
> signals are delivered to a specific thread is:
>  * Block all signals before the first pthread_create() call.
>  * Unblock signals from the thread that should receive signals.
> 
> If we unblock signals with a default disposition of "Term" for which we
> have no handler, stray signals caught would terminate multipathd. I
> don't think that's what we want. Of course we must react on
> SIGINT/SIGTERM/SIGHUP in the way the user expects us to, but
> terminating on other signals would be wrong.
> 
> Let's see if others have to say anything about this.

This is prety close to what I had in mind. The only things missing, that
I noticed when I was looking at multipathd's signal handling, is dealing
with SIGUSR2 in io_err_stat.c, which it sounds like you have already
spotted, and changing the calls to setitimer and usleep outside of the
vecs lock to use nanosleep, so they don't interact with SIGALRM.

I'll send a patch based on yours, to fix these.

-Ben

> 
> Martin
> 
> -- 
> Dr. Martin Wilck , 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: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

2018-03-05 Thread Martin Wilck
Hello Bart,

On Mon, 2018-03-05 at 16:27 +, Bart Van Assche wrote:
> On Sat, 2018-03-03 at 01:31 +0100, Martin Wilck wrote:
> > So, there's no reason not to block them, right? Is it expected
> > behavior
> > that a user running 'kill -USR2 $(pidof multipathd)' terminates the
> > process? Why do you think these signals should interrupt ppoll()
> > although the uxlsnr can't can't handle them? Isn't it sufficient
> > that
> > they're handled by the threads that are meant to do that?
> 
> Blocking all signals except the ones for which we installed a handler
> sounds
> weird to me. I think users expect daemons to process signals instead
> of
> blocking all but a specific set of signals. This is a rather
> philosophical
> argument and not an argument of which I think that it is strong
> enough to
> prevent this patch of being integrated in the upstream multipath-
> tools
> repository.

Thank you. Meanwhile, I found that this patch requires a change in the
"marginal paths" code. I'll post an official patch as update.

Regarding the philosophical discussion, may I remind you of your own
words from 534ec4c?

The POSIX standard mentions that the only way to guarantee that
signals are delivered to a specific thread is:
 * Block all signals before the first pthread_create() call.
 * Unblock signals from the thread that should receive signals.

If we unblock signals with a default disposition of "Term" for which we
have no handler, stray signals caught would terminate multipathd. I
don't think that's what we want. Of course we must react on
SIGINT/SIGTERM/SIGHUP in the way the user expects us to, but
terminating on other signals would be wrong.

Let's see if others have to say anything about this.

Martin

-- 
Dr. Martin Wilck , 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: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

2018-03-05 Thread Bart Van Assche
On Sat, 2018-03-03 at 01:31 +0100, Martin Wilck wrote:
> So, there's no reason not to block them, right? Is it expected behavior
> that a user running 'kill -USR2 $(pidof multipathd)' terminates the
> process? Why do you think these signals should interrupt ppoll()
> although the uxlsnr can't can't handle them? Isn't it sufficient that
> they're handled by the threads that are meant to do that?

Blocking all signals except the ones for which we installed a handler sounds
weird to me. I think users expect daemons to process signals instead of
blocking all but a specific set of signals. This is a rather philosophical
argument and not an argument of which I think that it is strong enough to
prevent this patch of being integrated in the upstream multipath-tools
repository.

Bart.



--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


Re: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

2018-03-02 Thread Martin Wilck
On Fri, 2018-03-02 at 23:27 +, Bart Van Assche wrote:
> On Sat, 2018-03-03 at 00:16 +0100, Martin Wilck wrote:
> > On Fri, 2018-03-02 at 22:23 +, Bart Van Assche wrote:
> > > On Fri, 2018-03-02 at 23:15 +0100, Martin Wilck wrote:
> > > > On Fri, 2018-03-02 at 21:35 +, Bart Van Assche wrote:
> > > > > This change looks more complicated to me than necessary. Have
> > > > > you
> > > > > considered
> > > > > to pass an empty signal set as the fourth ppoll() argument?
> > > > 
> > > > An empty set would mean that no signal is blocked during
> > > > ppoll().
> > > > Therefore e.g. SIGALRM would terminate multipathd if it arrives
> > > > during the ppoll (no handler set, and default action is
> > > > "Term").
> > > 
> > > Have you considered to only block SIGALRM while ppoll() is in
> > > progress?
> > 
> > Why should we? The same reasoning applies to other signals such as
> > e.g.
> > SIGUSR2. We need to block all signals except those that we can
> > handle.
> 
> I see two reasons:
> - All signals that are delivered to the multipathd process except
> SIGALRM
>   should interrupt ppoll(). SIGPIPE is delivered to the thread that
> caused
>   that signal to be raised. SIGUSR2 is sent to a specific thread.
> Hence
>   neither SIGPIPE nor SIGUSR2 will ever be delivered to the thread
> that
>   calls ppoll().

So, there's no reason not to block them, right? Is it expected behavior
that a user running 'kill -USR2 $(pidof multipathd)' terminates the
process? Why do you think these signals should interrupt ppoll()
although the uxlsnr can't can't handle them? Isn't it sufficient that
they're handled by the threads that are meant to do that?

IMO the idea of ppoll() is to specify a set of signals the calling
thread is interested in, and that's precisely the set of signals
handle_signals() can deal with.

I'm rather neutral about SIGPIPE, we can add a sigdelset(, SIGPIPE)
if you insist (multipathd will die anyway), but clearing SIGUSR2 in the
uxlsnr would be wrong IMO, as it's dedicated to the waiter thread.

> - Asking ppoll() to block all signals is weird because some signals,
> e.g.
>   SIGKILL and SIGSTOP, cannot be blocked.

And the kernel makes sure that doesn't happen. So I can add
sigdelset(, SIGKILL) bit it's kind of pointless.

Martin

> 
> Bart.

-- 
Dr. Martin Wilck , 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: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

2018-03-02 Thread Bart Van Assche
On Sat, 2018-03-03 at 00:16 +0100, Martin Wilck wrote:
> On Fri, 2018-03-02 at 22:23 +, Bart Van Assche wrote:
> > On Fri, 2018-03-02 at 23:15 +0100, Martin Wilck wrote:
> > > On Fri, 2018-03-02 at 21:35 +, Bart Van Assche wrote:
> > > > This change looks more complicated to me than necessary. Have you
> > > > considered
> > > > to pass an empty signal set as the fourth ppoll() argument?
> > > 
> > > An empty set would mean that no signal is blocked during ppoll().
> > > Therefore e.g. SIGALRM would terminate multipathd if it arrives
> > > during the ppoll (no handler set, and default action is "Term").
> > 
> > Have you considered to only block SIGALRM while ppoll() is in
> > progress?
> 
> Why should we? The same reasoning applies to other signals such as e.g.
> SIGUSR2. We need to block all signals except those that we can handle.

I see two reasons:
- All signals that are delivered to the multipathd process except SIGALRM
  should interrupt ppoll(). SIGPIPE is delivered to the thread that caused
  that signal to be raised. SIGUSR2 is sent to a specific thread. Hence
  neither SIGPIPE nor SIGUSR2 will ever be delivered to the thread that
  calls ppoll().
- Asking ppoll() to block all signals is weird because some signals, e.g.
  SIGKILL and SIGSTOP, cannot be blocked.

Bart.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


Re: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

2018-03-02 Thread Martin Wilck
On Fri, 2018-03-02 at 22:23 +, Bart Van Assche wrote:
> On Fri, 2018-03-02 at 23:15 +0100, Martin Wilck wrote:
> > On Fri, 2018-03-02 at 21:35 +, Bart Van Assche wrote:
> > > This change looks more complicated to me than necessary. Have you
> > > considered
> > > to pass an empty signal set as the fourth ppoll() argument?
> > 
> > An empty set would mean that no signal is blocked during ppoll().
> > Therefore e.g. SIGALRM would terminate multipathd if it arrives
> > during the ppoll (no handler set, and default action is "Term").
> 
> Have you considered to only block SIGALRM while ppoll() is in
> progress?

Why should we? The same reasoning applies to other signals such as e.g.
SIGUSR2. We need to block all signals except those that we can handle.

Regards,
Martin

-- 
Dr. Martin Wilck , 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: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

2018-03-02 Thread Bart Van Assche
On Fri, 2018-03-02 at 23:15 +0100, Martin Wilck wrote:
> On Fri, 2018-03-02 at 21:35 +, Bart Van Assche wrote:
> > This change looks more complicated to me than necessary. Have you
> > considered
> > to pass an empty signal set as the fourth ppoll() argument?
> 
> An empty set would mean that no signal is blocked during ppoll().
> Therefore e.g. SIGALRM would terminate multipathd if it arrives
> during the ppoll (no handler set, and default action is "Term").

Have you considered to only block SIGALRM while ppoll() is in progress?

Thanks,

Bart.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


Re: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

2018-03-02 Thread Martin Wilck
On Fri, 2018-03-02 at 21:35 +, Bart Van Assche wrote:
> On Fri, 2018-03-02 at 22:18 +0100, Martin Wilck wrote:
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 61739ac6ea59..85ee9b713d75 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2270,10 +2270,13 @@ signal_init(void)
> >  {
> > sigset_t set;
> >  
> > -   sigemptyset();
> > -   sigaddset(, SIGUSR2);
> > +   /* block all signals */
> > +   sigfillset();
> > +   /* SIGPIPE occurs if logging fails */
> > +   sigdelset(, SIGPIPE);
> > pthread_sigmask(SIG_SETMASK, , NULL);
> 
> The modified code was introduced by commit 90dd424afa65 ("multipathd:
> fix SIGUSR2 handling"). This change looks fine to me.
> 
> > diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> > index 98ac25a68c43..a2ca36ba1653 100644
> > --- a/multipathd/uxlsnr.c
> > +++ b/multipathd/uxlsnr.c
> > @@ -170,11 +170,11 @@ void * uxsock_listen(uxsock_trigger_fn
> > uxsock_trigger, void * trigger_data)
> > condlog(0, "uxsock: failed to allocate poll fds");
> > return NULL;
> > }
> > -   sigemptyset();
> > -   sigaddset(, SIGINT);
> > -   sigaddset(, SIGTERM);
> > -   sigaddset(, SIGHUP);
> > -   sigaddset(, SIGUSR1);
> > +   sigfillset();
> > +   sigdelset(, SIGINT);
> > +   sigdelset(, SIGTERM);
> > +   sigdelset(, SIGHUP);
> > +   sigdelset(, SIGUSR1);
> > while (1) {
> > struct client *c, *tmp;
> > int i, poll_count, num_clients;
> 
> This change looks more complicated to me than necessary. Have you
> considered
> to pass an empty signal set as the fourth ppoll() argument?

An empty set would mean that no signal is blocked during ppoll().
Therefore e.g. SIGALRM would terminate multipathd if it arrives
during the ppoll (no handler set, and default action is "Term").

Regards,
Martin

-- 
Dr. Martin Wilck , 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: [dm-devel] [PATCH] multipathd: fix inverted signal blocking logic

2018-03-02 Thread Bart Van Assche
On Fri, 2018-03-02 at 22:18 +0100, Martin Wilck wrote:
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 61739ac6ea59..85ee9b713d75 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2270,10 +2270,13 @@ signal_init(void)
>  {
>   sigset_t set;
>  
> - sigemptyset();
> - sigaddset(, SIGUSR2);
> + /* block all signals */
> + sigfillset();
> + /* SIGPIPE occurs if logging fails */
> + sigdelset(, SIGPIPE);
>   pthread_sigmask(SIG_SETMASK, , NULL);

The modified code was introduced by commit 90dd424afa65 ("multipathd:
fix SIGUSR2 handling"). This change looks fine to me.

> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index 98ac25a68c43..a2ca36ba1653 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -170,11 +170,11 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, 
> void * trigger_data)
>   condlog(0, "uxsock: failed to allocate poll fds");
>   return NULL;
>   }
> - sigemptyset();
> - sigaddset(, SIGINT);
> - sigaddset(, SIGTERM);
> - sigaddset(, SIGHUP);
> - sigaddset(, SIGUSR1);
> + sigfillset();
> + sigdelset(, SIGINT);
> + sigdelset(, SIGTERM);
> + sigdelset(, SIGHUP);
> + sigdelset(, SIGUSR1);
>   while (1) {
>   struct client *c, *tmp;
>   int i, poll_count, num_clients;

This change looks more complicated to me than necessary. Have you considered
to pass an empty signal set as the fourth ppoll() argument?

Thanks,

Bart.



--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel