Re: [PATCH 8/9] libmutipath: continue to use old state on PATH_PENDING

[Martin Wilck <mwilck@suse.de>]

On Fri, 2019-02-22 at 10:58 -0600, Benjamin Marzinski wrote:
> When pathinfo() sets pp->state to PATH_PENDING, it can cause problems
> with path checking.  It should act more like check_path(). When
> check_path() sees a new state of PATH_PENDING, it doesn't update the
> path state at all, so a path's old state is normally never
> PATH_PENDING.
> 
> As and example of the problems of setting a path to PATH_PENDING, If
> check_path() sets a path's state to PATH_UP, then a call to
> pathinfo()
> sets the state to PATH_PENDING, and then another call the
> check_path()
> sets the state to PATH_DOWN, multipathd won't fail the path in the
> kernel. 

I can see what you mean, but I'm unsure how this example would come to
pass in practice (AFAICS, pathinfo(DI_CHECKER) is only called during
path discovery / reconfigure(), or when new paths are detected /
added).

> Also, if a path's state is PATH_PENDING, and nr_active is
> recalculated, that path will count as down, even if the state was
> previously PATH_UP.

Ah, this is how you deal with what I remarked on the second hunk of
patch 6/9. This is smart.

>  If a path already has a state of PATH_WILD or
> PATH_UNCHECKED, changing it to PATH_PENDING won't hurt anything, and
> it
> will help anyone who sees it know what's actually happening. But
> otherwise, pathinfo() should leave the previous state alone.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/discovery.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

My gut feeling is that we should distinguish between the case where
PATH_PENDING is set in path_offline() (sysfs state "quiesce",
"blocked", "pending" etc.), and the case where the async checker is
simply not finished yet. The first case does tell us something about
the path (being temporarily inaccessible), whereas the second basically
just says that we're ignorant.

Anyway, that's future work, so:

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



> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 729bcb9..d3585f9 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1946,8 +1946,11 @@ int pathinfo(struct path *pp, struct config
> *conf, int mask)
>  
>  	if (mask & DI_CHECKER) {
>  		if (path_state == PATH_UP) {
> -			pp->chkrstate = pp->state = get_state(pp, conf,
> 0,
> -							      path_stat
> e);
> +			int newstate = get_state(pp, conf, 0,
> path_state);
> +			if (newstate != PATH_PENDING ||
> +			    pp->state == PATH_UNCHECKED ||
> +			    pp->state == PATH_WILD)
> +				pp->chkrstate = pp->state = newstate;
>  			if (pp->state == PATH_TIMEOUT)
>  				pp->state = PATH_DOWN;
>  			if (pp->state == PATH_UP && !pp->size) {