[dm-devel] [PATCH 4/5] libmultipath: fix (max_)polling_interval setting logic

[Benjamin Marzinski]

On Fri, Nov 15, 2019 at 02:41:52PM +0000, Martin Wilck wrote:
> From: Martin Wilck <mwilck at suse.com>
> 
> checkint should never be larger than max_checkint. The WATCHDOG_USEC
> setting should be honored on "reconfigure", too, not only on startup.
> Pathological values for checkint and integer overflows should be avoided.
> The logic should work reasonably well if both polling_interval and
> max_polling_interval, just one of them, or neither is set.
> 
Reviewed-by: Benjamin Marzinski <bmarzins at redhat.com>
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
>  libmultipath/config.c   | 40 +++++++++++++++++++++++++++++++++++++---
>  libmultipath/config.h   |  1 +
>  libmultipath/defaults.h |  3 +--
>  multipathd/main.c       | 26 ++++++--------------------
>  4 files changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 49723add..0bf7bb40 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -683,6 +683,27 @@ process_config_dir(struct config *conf, char *dir)
>  	pthread_cleanup_pop(1);
>  }
>  
> +static void set_max_checkint_from_watchdog(struct config *conf)
> +{
> +#ifdef USE_SYSTEMD
> +	char *envp = getenv("WATCHDOG_USEC");
> +	unsigned long checkint;
> +
> +	if (envp && sscanf(envp, "%lu", &checkint) == 1) {
> +		/* Value is in microseconds */
> +		checkint /= 1000000;
> +		if (checkint < 1 || checkint > UINT_MAX) {
> +			condlog(1, "invalid value for WatchdogSec: \"%s\"", envp);
> +			return;
> +		}
> +		if (conf->max_checkint == 0 || conf->max_checkint > checkint)
> +			conf->max_checkint = checkint;
> +		condlog(3, "enabling watchdog, interval %ld", checkint);
> +		conf->use_watchdog = true;
> +	}
> +#endif
> +}
> +
>  struct config *
>  load_config (char * file)
>  {
> @@ -703,7 +724,8 @@ load_config (char * file)
>  	conf->multipath_dir = set_default(DEFAULT_MULTIPATHDIR);
>  	conf->attribute_flags = 0;
>  	conf->reassign_maps = DEFAULT_REASSIGN_MAPS;
> -	conf->checkint = DEFAULT_CHECKINT;
> +	conf->checkint = CHECKINT_UNDEF;
> +	conf->use_watchdog = false;
>  	conf->max_checkint = 0;
>  	conf->force_sync = DEFAULT_FORCE_SYNC;
>  	conf->partition_delim = (default_partition_delim != NULL ?
> @@ -754,8 +776,20 @@ load_config (char * file)
>  	/*
>  	 * fill the voids left in the config file
>  	 */
> -	if (conf->max_checkint == 0)
> -		conf->max_checkint = MAX_CHECKINT(conf->checkint);
> +	set_max_checkint_from_watchdog(conf);
> +	if (conf->max_checkint == 0) {
> +		if (conf->checkint == CHECKINT_UNDEF)
> +			conf->checkint = DEFAULT_CHECKINT;
> +		conf->max_checkint = (conf->checkint < UINT_MAX / 4 ?
> +				      conf->checkint * 4 : UINT_MAX);
> +	} else if (conf->checkint == CHECKINT_UNDEF)
> +		conf->checkint = (conf->max_checkint >= 4 ?
> +				  conf->max_checkint / 4 : 1);
> +	else if (conf->checkint > conf->max_checkint)
> +		conf->checkint = conf->max_checkint;
> +	condlog(3, "polling interval: %d, max: %d",
> +		conf->checkint, conf->max_checkint);
> +
>  	if (conf->blist_devnode == NULL) {
>  		conf->blist_devnode = vector_alloc();
>  
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 2ab7b42c..a078947c 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -139,6 +139,7 @@ struct config {
>  	int minio_rq;
>  	unsigned int checkint;
>  	unsigned int max_checkint;
> +	bool use_watchdog;
>  	int pgfailback;
>  	int remove;
>  	int rr_weight;
> diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
> index d7034655..e5ee6afe 100644
> --- a/libmultipath/defaults.h
> +++ b/libmultipath/defaults.h
> @@ -53,9 +53,8 @@
>  /* Enable all foreign libraries by default */
>  #define DEFAULT_ENABLE_FOREIGN ""
>  
> -#define CHECKINT_UNDEF		(~0U)
> +#define CHECKINT_UNDEF		UINT_MAX
>  #define DEFAULT_CHECKINT	5
> -#define MAX_CHECKINT(a)		(a << 2)
>  
>  #define MAX_DEV_LOSS_TMO	UINT_MAX
>  #define DEFAULT_PIDFILE		"/" RUN_DIR "/multipathd.pid"
> diff --git a/multipathd/main.c b/multipathd/main.c
> index c0423602..95426d3d 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -33,10 +33,6 @@
>   */
>  #include "checkers.h"
>  
> -#ifdef USE_SYSTEMD
> -static int use_watchdog;
> -#endif
> -
>  /*
>   * libmultipath
>   */
> @@ -2284,6 +2280,7 @@ checkerloop (void *ap)
>  	struct timespec last_time;
>  	struct config *conf;
>  	int foreign_tick = 0;
> +	bool use_watchdog;
>  
>  	pthread_cleanup_push(rcu_unregister, NULL);
>  	rcu_register_thread();
> @@ -2295,6 +2292,11 @@ checkerloop (void *ap)
>  	get_monotonic_time(&last_time);
>  	last_time.tv_sec -= 1;
>  
> +	/* use_watchdog is set from process environment and never changes */
> +	conf = get_multipath_config();
> +	use_watchdog = conf->use_watchdog;
> +	put_multipath_config(conf);
> +
>  	while (1) {
>  		struct timespec diff_time, start_time, end_time;
>  		int num_paths = 0, strict_timing, rc = 0;
> @@ -2766,7 +2768,6 @@ child (__attribute__((unused)) void *param)
>  	struct multipath * mpp;
>  	int i;
>  #ifdef USE_SYSTEMD
> -	unsigned long checkint;
>  	int startup_done = 0;
>  #endif
>  	int rc;
> @@ -2843,21 +2844,6 @@ child (__attribute__((unused)) void *param)
>  	setscheduler();
>  	set_oom_adj();
>  
> -#ifdef USE_SYSTEMD
> -	envp = getenv("WATCHDOG_USEC");
> -	if (envp && sscanf(envp, "%lu", &checkint) == 1) {
> -		/* Value is in microseconds */
> -		conf->max_checkint = checkint / 1000000;
> -		/* Rescale checkint */
> -		if (conf->checkint > conf->max_checkint)
> -			conf->checkint = conf->max_checkint;
> -		else
> -			conf->checkint = conf->max_checkint / 4;
> -		condlog(3, "enabling watchdog, interval %d max %d",
> -			conf->checkint, conf->max_checkint);
> -		use_watchdog = conf->checkint;
> -	}
> -#endif
>  	/*
>  	 * Startup done, invalidate configuration
>  	 */
> -- 
> 2.24.0