Re: [PATCH 1/3] libmultipath: fix handling of pp->pgindex

[Benjamin Marzinski <bmarzins@redhat.com>]

On Mon, Nov 25, 2024 at 03:32:22PM +0100, Martin Wilck wrote:
> pp->pgindex is set in disassemble_map() when a map is parsed.
> There are various possiblities for this index to become invalid.
> pp->pgindex is only used in enable_group() and followover_should_fallback(),
> and both callers take no action if it is 0, which is the right
> thing to do if we don't know the path's pathgroup.
> 
> Make sure pp->pgindex is reset to 0 in various places:
> - when it's orphaned,
> - before (re)grouping paths,
> - when we detect a bad mpp assignment in update_pathvec_from_dm().
> 
> The hunk in group_paths is mostly redundant with the hunk in free_pgvec(), but
> because we're looping over pg->paths in the former and over pg->pgp in
> the latter, I think it's better too play safe.

I'm not sure this will always fix issue #105. Perhaps I'm overlooking
the connection in the code and this just needs a more explicit
explanation to aid clueless reviewers, but here's what I don't get.

As far as I can see, the only change here that would effect the reported
issue is the change to update_pathvec_from_dm(). While I totally agree
that if you have a path that appears to be two multipath devices, you
can't trust pp->pgindex, I believe there's also another problem.

Say you have a multipath device with two path groups (pgp1 and pgp2)
each with one path (pp1 in pgp1 and pp2 in pgp2). In this case,
pp1->pgindex == 1 and pp2->pgindex == 2. If update_pathvec_from_dm()
discovers that pp1 is part of another multipath device, and removes it,
that will mean that pgp1 is now empty, so update_pathvec_from_dm() will
also remove that (at the delete_pg label).  But pp2->pgindex will still
be set to 2 even though there's only one path group, so it will now
point off the end of the pgp list. Right?

At any rate, updating pgindex seems finicky and perhaps we should just
drop it. It's not that much work to scan the path groups for the path in
enable_group() and since we're already reading through the path groups
in followover_should_failback(), we can just refactor the code a little
bit to avoid needing pgindex at all.

-Ben
 
> Fixes: 99db1bd ("[multipathd] re-enable disabled PG when at least one path is up")
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/pgpolicies.c  |  6 ++++++
>  libmultipath/structs.c     | 12 +++++++++++-
>  libmultipath/structs_vec.c |  8 ++++++++
>  3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
> index edc3c61..23ef2bd 100644
> --- a/libmultipath/pgpolicies.c
> +++ b/libmultipath/pgpolicies.c
> @@ -127,6 +127,8 @@ fail:
>  int group_paths(struct multipath *mp, int marginal_pathgroups)
>  {
>  	vector normal, marginal;
> +	struct path *pp;
> +	int i;
>  
>  	if (!mp->pg)
>  		mp->pg = vector_alloc();
> @@ -138,6 +140,10 @@ int group_paths(struct multipath *mp, int marginal_pathgroups)
>  	if (!mp->pgpolicyfn)
>  		goto fail;
>  
> +	/* Reset pgindex, we're going to invalidate it */
> +	vector_foreach_slot(mp->paths, pp, i)
> +		pp->pgindex = 0;
> +
>  	if (!marginal_pathgroups ||
>  	    split_marginal_paths(mp->paths, &normal, &marginal) != 0) {
>  		if (mp->pgpolicyfn(mp, mp->paths) != 0)
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 61c8f32..4851725 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -239,8 +239,18 @@ free_pgvec (vector pgvec, enum free_path_mode free_paths)
>  	if (!pgvec)
>  		return;
>  
> -	vector_foreach_slot(pgvec, pgp, i)
> +	vector_foreach_slot(pgvec, pgp, i) {
> +
> +		/* paths are going to be re-grouped, reset pgindex */
> +		if (free_paths != FREE_PATHS) {
> +			struct path *pp;
> +			int j;
> +
> +			vector_foreach_slot(pgp->paths, pp, j)
> +				pp->pgindex = 0;
> +		}
>  		free_pathgroup(pgp, free_paths);
> +	}
>  
>  	vector_free(pgvec);
>  }
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index d22056c..64c5ad5 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -139,6 +139,13 @@ static bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
>  				must_reload = true;
>  				dm_fail_path(mpp->alias, pp->dev_t);
>  				vector_del_slot(pgp->paths, j--);
> +				/*
> +				 * pp->pgindex has been set in disassemble_map(),
> +				 * which has probably been called just before for
> +				 * mpp. So he pgindex relates to mpp and may be
> +				 * wrong for pp->mpp. Invalidate it.
> +				 */
> +				pp->pgindex = 0;
>  				continue;
>  			}
>  			pp->mpp = mpp;
> @@ -354,6 +361,7 @@ void orphan_path(struct path *pp, const char *reason)
>  {
>  	condlog(3, "%s: orphan path, %s", pp->dev, reason);
>  	pp->mpp = NULL;
> +	pp->pgindex = 0;
>  	uninitialize_path(pp);
>  }
>  
> -- 
> 2.47.0