[dm-devel] [PATCH v2] multipath-tools: handle exit signal immediately

[Chongyun Wu]

Hi Martin,

My patch have tested but your patch seems to be more gracefully. May be 
there are some small issues if I am not wrong.Thanks.

On 2018/1/30 22:18, Martin Wilck wrote:
> multipathd shouldn't try to service any more client connections
> when it receives an exit signal. Moreover, ppoll() can return
> success even if signals occured. So check for reconfigure or
> log_reset signals after handling pending client requests.
> 
> Based on an analysis by Chongyun Wu.
> 
> Reported-by: Chongyun Wu <wu.chongyun at h3c.com>
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
>   multipathd/main.c   | 6 ++++--
>   multipathd/main.h   | 2 +-
>   multipathd/uxlsnr.c | 7 +++++--
>   3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 27cf234623d0..8e96f5dd2d7f 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2184,12 +2184,15 @@ signal_set(int signo, void (*func) (int))
>   }
>   
>   void
> -handle_signals(void)
> +handle_signals(bool nonfatal)
>   {
>   	if (exit_sig) {
>   		condlog(2, "exit (signal)");
> +		exit_sig = 0;

If we received a exit signal should we just escape all below commands 
process to avoid the race condition between thread cancellation and 
commands processing? so maybe flag exit_sig keep to 1 here?

>   		exit_daemon();
>   	}
> +	if (!nonfatal)
> +		return;
>   	if (reconfig_sig) {
>   		condlog(2, "reconfigure (signal)");
>   		set_config_state(DAEMON_CONFIGURE);
> @@ -2200,7 +2203,6 @@ handle_signals(void)
>   		log_reset("multipathd");
>   		pthread_mutex_unlock(&logq_lock);
>   	}
> -	exit_sig = 0;
>   	reconfig_sig = 0;
>   	log_reset_sig = 0;
>   }
> diff --git a/multipathd/main.h b/multipathd/main.h
> index ededdaba32fe..e8140feaf291 100644
> --- a/multipathd/main.h
> +++ b/multipathd/main.h
> @@ -38,6 +38,6 @@ int mpath_pr_event_handle(struct path *pp);
>   void * mpath_pr_event_handler_fn (void * );
>   int update_map_pr(struct multipath *mpp);
>   void * mpath_pr_event_handler_fn (void * pathp );
> -void handle_signals(void);
> +void handle_signals(bool);
>   
>   #endif /* MAIN_H */
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index 98ac25a68c43..dc116cf2515b 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -221,9 +221,10 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data)
>   		/* most of our life is spent in this call */
>   		poll_count = ppoll(polls, i, &sleep_time, &mask);
>   
> +		handle_signals(false);
Is it better to add and check the return value from handle_signals()? If 
return true means this a exit signal should run continue to escape all 
below processing.
If not doing that, we may need to add pthread_cleanup_push(cleanup_lock, 
&client_lock) before pthread_mutex_lock(&client_lock) in below command 
processing to avid dead lock which happen when commands processing code 
just have the lock and cancellation works to call uxsock_cleanup hook to 
get the lock and the commands processing code have no chance to release 
the lock.

>   		if (poll_count == -1) {
>   			if (errno == EINTR) {
> -				handle_signals();
> +				handle_signals(true);
>   				continue;
>   			}
>   
> @@ -233,7 +234,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data)
>   		}
>   
>   		if (poll_count == 0) {
> -			handle_signals();
> +			handle_signals(true);
>   			continue;
>   		}
>   
> @@ -292,6 +293,8 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data)
>   				FREE(inbuf);
>   			}
>   		}
> +		/* see if we got a non-fatal signal */
> +		handle_signals(true);
above code maybe not needed. Because it's safe to make sure no commands 
be processed then to handle nonefatal signal, like code "if(poll_count 
==0){...}" do. If here to call hanle_signals(true) again, may facing 
race condition between signals processing and command processing. That 
maybe not so safe. And the handle_signals have been called above. Is 
there a special reason for this which I missed?

>   
>   		/* see if we got a new client */
>   		if (polls[0].revents & POLLIN) {
> 
Chongyun Wu