Re: [PATCH v2 11/20] libmultipath: don't try to set up failed wwids again

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Mar 19, 2018 at 04:01:46PM +0100, Martin Wilck wrote:
> Once setting up a map has failed, don't try to set it up again.
> This applies to "multipathd reconfigure" and even multipathd restart,
> too. That's deliberate - if a WWID is marked as failed, we don't wont
> to bother with it again, unless the admin explicitly tells us so.
> Specifically, the exceptions are:
> 
>  1) multipathd add map $MAP
>  2) multipathd add path $PATH
>  3) multipath $PATH
> 
> In these cases, addmap() will eventually be called again, and the failed
> flag will be set according to it's return status. Unless the reason for
> the previous failure has been fixed, it may well be "failed" again.
> 
> Inofficially, it's also possible to manually remove a failed marker under
> /dev/shm/multipath/failed_wwids and run "multipathd reconfigure".

The code looks fine, but I wonder why this is necessary.  You already
posted patches that let multipathd issue uevent to claim a path device
after if it was not previously claimed. I admit that you don't generally
see multipathd succeed in creating a device after failing to, but it's
easy to imagine situations where it could.  For instance, if a path
device appears and then disappears soon after, multipathd would fail to
create the device because when the path device finally reappeared for
good.

I see that this will keep multipathd from needlessly retrying in-use
devices whenever a path comes or goes, but I don't know of any harm that
has ever caused, and I can see this causing harm. Am I missing something
here?

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
>  libmultipath/configure.c  |  5 +++--
>  libmultipath/configure.h  |  3 ++-
>  libmultipath/wwids.c      |  7 ++++++-
>  libmultipath/wwids.h      |  3 ++-
>  multipath/main.c          | 12 +++++++++---
>  multipathd/cli_handlers.c |  5 +++--
>  multipathd/main.c         | 13 +++++++------
>  multipathd/main.h         |  8 ++++++++
>  8 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 88e6687849f8..98b80337d899 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -981,7 +981,7 @@ out:
>   * reloaded in DM if there's a difference. This is useful during startup.
>   */
>  int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
> -		    int force_reload, enum mpath_cmds cmd)
> +		    int force_reload, bool retry_failed, enum mpath_cmds cmd)
>  {
>  	int r = 1;
>  	int k, i;
> @@ -1032,7 +1032,8 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  			continue;
>  
>  		/* If find_multipaths was selected check if the path is valid */
> -		if (!refwwid && !should_multipath(pp1, pathvec, curmp)) {
> +		if (!refwwid && !should_multipath(pp1, pathvec, curmp,
> +						  retry_failed)) {
>  			orphan_path(pp1, "only one path");
>  			continue;
>  		}
> diff --git a/libmultipath/configure.h b/libmultipath/configure.h
> index 8b56d33a1d0b..7e175c890d25 100644
> --- a/libmultipath/configure.h
> +++ b/libmultipath/configure.h
> @@ -32,7 +32,8 @@ int setup_map (struct multipath * mpp, char * params, int params_size,
>  	       struct vectors *vecs );
>  int domap (struct multipath * mpp, char * params, int is_daemon);
>  int reinstate_paths (struct multipath *mpp);
> -int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid, int force_reload, enum mpath_cmds cmd);
> +int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid,
> +		    int force_reload, bool retry_failed, enum mpath_cmds cmd);
>  int get_refwwid (enum mpath_cmds cmd, char * dev, enum devtypes dev_type,
>  		 vector pathvec, char **wwid);
>  int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon);
> diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
> index e0fdcb34dbc6..288a9ad50c73 100644
> --- a/libmultipath/wwids.c
> +++ b/libmultipath/wwids.c
> @@ -4,6 +4,7 @@
>  #include <string.h>
>  #include <limits.h>
>  #include <stdio.h>
> +#include <stdbool.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  
> @@ -273,12 +274,16 @@ out:
>  }
>  
>  int
> -should_multipath(struct path *pp1, vector pathvec, vector mpvec)
> +should_multipath(struct path *pp1, vector pathvec, vector mpvec,
> +		 bool retry_failed)
>  {
>  	int i, ignore_new;
>  	struct path *pp2;
>  	struct config *conf;
>  
> +	if (!retry_failed && is_failed_wwid(pp1->wwid))
> +		return 0;
> +
>  	conf = get_multipath_config();
>  	ignore_new = ignore_new_devs(conf);
>  	if (!find_multipaths_on(conf) && !ignore_new) {
> diff --git a/libmultipath/wwids.h b/libmultipath/wwids.h
> index d00e1f58137c..911e8da720c5 100644
> --- a/libmultipath/wwids.h
> +++ b/libmultipath/wwids.h
> @@ -12,7 +12,8 @@
>  "#\n" \
>  "# Valid WWIDs:\n"
>  
> -int should_multipath(struct path *pp, vector pathvec, vector mpvec);
> +int should_multipath(struct path *pp, vector pathvec, vector mpvec,
> +		     bool retry_failed);
>  int remember_wwid(char *wwid);
>  int check_wwids_file(char *wwid, int write_wwid);
>  int remove_wwid(char *wwid);
> diff --git a/multipath/main.c b/multipath/main.c
> index 3944fd504de2..566404e56ef4 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -441,8 +441,12 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		 * Paths listed in the wwids file are always considered valid.
>  		 */
>  		if (cmd == CMD_VALID_PATH) {
> -			if ((!find_multipaths_on(conf) && ignore_wwids(conf)) ||
> -			    check_wwids_file(refwwid, 0) == 0)
> +			if (is_failed_wwid(refwwid)) {
> +				r = 1;
> +				goto print_valid;
> +			} else if ((!find_multipaths_on(conf) &&
> +				  ignore_wwids(conf)) ||
> +				 check_wwids_file(refwwid, 0) == 0)
>  				r = 0;
>  			if (r == 0 ||
>  			    !find_multipaths_on(conf) || !ignore_wwids(conf)) {
> @@ -504,9 +508,11 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  
>  	/*
>  	 * core logic entry point
> +	 * If refwwid != NULL, user has given a device to multipath,
> +	 * so retry even if this ID has failed before.
>  	 */
>  	r = coalesce_paths(&vecs, NULL, refwwid,
> -			   conf->force_reload, cmd);
> +			   conf->force_reload, refwwid != NULL, cmd);
>  
>  out:
>  	if (refwwid)
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 60ec48b9904a..202438795ee5 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -750,7 +750,7 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
>  		pp->checkint = conf->checkint;
>  	}
>  	put_multipath_config(conf);
> -	return ev_add_path(pp, vecs, 1);
> +	return ev_add_path(pp, vecs, ADD_PATH_DOMAP_FORCE);
>  blacklisted:
>  	*reply = strdup("blacklisted\n");
>  	*len = strlen(*reply) + 1;
> @@ -813,7 +813,8 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
>  					 vecs->pathvec, &refwwid);
>  			if (refwwid) {
>  				if (coalesce_paths(vecs, NULL, refwwid,
> -						   FORCE_RELOAD_NONE, CMD_NONE))
> +						   FORCE_RELOAD_NONE, true,
> +						   CMD_NONE))
>  					condlog(2, "%s: coalesce_paths failed",
>  									param);
>  				dm_lib_release();
> diff --git a/multipathd/main.c b/multipathd/main.c
> index ea8c413f28c6..8fd7d2b75cba 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -933,7 +933,8 @@ rescan:
>  		mpp->action = ACT_RELOAD;
>  		extract_hwe_from_path(mpp);
>  	} else {
> -		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec)) {
> +		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec,
> +				      need_do_map == ADD_PATH_DOMAP_FORCE)) {
>  			orphan_path(pp, "only one path");
>  			return 0;
>  		}
> @@ -953,7 +954,7 @@ rescan:
>  	/* persistent reservation check*/
>  	mpath_pr_event_handle(pp);
>  
> -	if (!need_do_map)
> +	if (need_do_map == ADD_PATH_DOMAP_NO)
>  		return 0;
>  
>  	if (!dm_map_present(mpp->alias)) {
> @@ -1863,7 +1864,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  			conf = get_multipath_config();
>  			ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
>  			if (ret == PATHINFO_OK) {
> -				ev_add_path(pp, vecs, 1);
> +				ev_add_path(pp, vecs, ADD_PATH_DOMAP_YES);
>  				pp->tick = 1;
>  			} else if (ret == PATHINFO_SKIPPED) {
>  				put_multipath_config(conf);
> @@ -1989,7 +1990,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  		}
>  		if (!disable_reinstate && reinstate_path(pp, add_active)) {
>  			condlog(3, "%s: reload map", pp->dev);
> -			ev_add_path(pp, vecs, 1);
> +			ev_add_path(pp, vecs, ADD_PATH_DOMAP_YES);
>  			pp->tick = 1;
>  			return 0;
>  		}
> @@ -2012,7 +2013,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  			/* Clear IO errors */
>  			if (reinstate_path(pp, 0)) {
>  				condlog(3, "%s: reload map", pp->dev);
> -				ev_add_path(pp, vecs, 1);
> +				ev_add_path(pp, vecs, ADD_PATH_DOMAP_YES);
>  				pp->tick = 1;
>  				return 0;
>  			}
> @@ -2288,7 +2289,7 @@ configure (struct vectors * vecs)
>  	 * superfluous ACT_RELOAD ioctls. Later calls are done
>  	 * with FORCE_RELOAD_YES.
>  	 */
> -	ret = coalesce_paths(vecs, mpvec, NULL, force_reload, CMD_NONE);
> +	ret = coalesce_paths(vecs, mpvec, NULL, force_reload, false, CMD_NONE);
>  	if (force_reload == FORCE_RELOAD_WEAK)
>  		force_reload = FORCE_RELOAD_YES;
>  	if (ret) {
> diff --git a/multipathd/main.h b/multipathd/main.h
> index af395589ff93..a92650cd958b 100644
> --- a/multipathd/main.h
> +++ b/multipathd/main.h
> @@ -22,6 +22,14 @@ void exit_daemon(void);
>  const char * daemon_status(void);
>  int need_to_delay_reconfig (struct vectors *);
>  int reconfigure (struct vectors *);
> +/*
> + * 3rd argument of ev_add_path()
> + */
> +enum {
> +	ADD_PATH_DOMAP_NO = 0,	  /* don't call domap */
> +	ADD_PATH_DOMAP_YES = 1,	  /* call domap, don't retry failed */
> +	ADD_PATH_DOMAP_FORCE = 2, /* call domap, retry previously failed */
> +};
>  int ev_add_path (struct path *, struct vectors *, int);
>  int ev_remove_path (struct path *, struct vectors *, int);
>  int ev_add_map (char *, const char *, struct vectors *);
> -- 
> 2.16.1

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



[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux