[dm-devel] [PATCH 5/5] add prflag to path

[Benjamin Marzinski]

On Tue, Nov 16, 2021 at 10:01:15PM +0800, lixiaokeng wrote:
> The update_map will frequently be called and there will be
> unnecessary checks of reseravtion. We add prflag to path
> to avoid this.
> 
> The pp->state changes from others to up or ghost, the
> mpath_pr_event_handle should be called. The
> mpath_pr_event_handle in ev_add_path may not be called,
> so set pp->prkey PRKEY_NO when path is removed.

This patch kind of confuses me.  You only check pp->prkey before calling
mpath_pr_event_handle() in update_map(). I get from your commit message
that you are doing this to keep from frequent, unnecessary calls. But
isn't update_map() only called when a multipath device is first created,
or when multipathd stops waiting for something that it noticed during
device creation? I don't see how this can be frequently called on a
multipath device. What am I missing?

-Ben

> 
> Fix: 4db4fa
> Signed-off-by: Lixiaokeng <lixiaokeng>
> ---
>  libmpathpersist/mpath_persist.c |  2 +-
>  libmultipath/structs.c          |  1 +
>  libmultipath/structs.h          | 12 ++++++++++++
>  multipathd/cli_handlers.c       | 15 ++++++++++-----
>  multipathd/main.c               |  5 +++--
>  5 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
> index 803a2a28..f88a2e89 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> @@ -924,7 +924,7 @@ int update_map_pr(struct multipath *mpp)
> 
>  	if (isFound)
>  	{
> -		mpp->prflag = 1;
> +		mpp->prflag = PRFLAG_OK;
>  		condlog(2, "%s: prflag flag set.", mpp->alias );
>  	}
> 
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index e8cacb4b..82dbd565 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -122,6 +122,7 @@ uninitialize_path(struct path *pp)
>  	pp->dmstate = PSTATE_UNDEF;
>  	pp->uid_attribute = NULL;
>  	pp->getuid = NULL;
> +	pp->prflag = PRFLAG_NO;
> 
>  	if (checker_selected(&pp->checker))
>  		checker_put(&pp->checker);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 399540e7..5b77218b 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -249,6 +249,17 @@ enum recheck_wwid_states {
>  	RECHECK_WWID_ON = YNU_YES,
>  };
> 
> +/*
> + * PRFLAG_NO for path, it means reservation should be checked.
> + * PRFLAG_NO for multipath, it means mpp has no prkey.
> + * PRFLAG_OK for path, it means reservation has been checked.
> + * PRFLAG_OK for multipath, it means mpp has prkey.
> + */
> +enum prflag_states {
> +	PRFLAG_NO = 0,
> +	PRFLAG_OK = 1,
> +};
> +
>  struct vpd_vendor_page {
>  	int pg;
>  	const char *name;
> @@ -327,6 +338,7 @@ struct path {
>  	/* configlet pointers */
>  	vector hwe;
>  	struct gen_path generic_path;
> +	int prflag;
>  };
> 
>  typedef int (pgpolicyfn) (struct multipath *, vector);
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 6d3a0ae2..8662fad7 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1341,7 +1341,7 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
>  		return 1;
> 
>  	if (!mpp->prflag) {
> -		mpp->prflag = 1;
> +		mpp->prflag = PRFLAG_OK;
>  		condlog(2, "%s: prflag set", param);
>  	}
> 
> @@ -1352,9 +1352,11 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
>  int
>  cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
>  {
> -	struct multipath * mpp;
> -	struct vectors * vecs = (struct vectors *)data;
> -	char * param = get_keyparam(v, MAP);
> +	int i;
> +	struct multipath *mpp;
> +	struct path *pp;
> +	struct vectors *vecs = (struct vectors *)data;
> +	char *param = get_keyparam(v, MAP);
> 
>  	param = convert_dev(param, 0);
>  	get_path_layout(vecs->pathvec, 0);
> @@ -1364,7 +1366,10 @@ cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
>  		return 1;
> 
>  	if (mpp->prflag) {
> -		mpp->prflag = 0;
> +		mpp->prflag = PRFLAG_NO;
> +		vector_foreach_slot(mpp->paths, pp, i) {
> +			pp->prflag = PRFLAG_NO;
> +		}
>  		condlog(2, "%s: prflag unset", param);
>  	}
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 82ab3ed1..6ef6495b 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -506,7 +506,7 @@ retry:
> 
>  	if (mpp->prflag) {
>  		vector_foreach_slot(mpp->paths, pp, i) {
> -			if ((pp->state == PATH_UP)  || (pp->state == PATH_GHOST)) {
> +			if (!pp->prflag && ((pp->state == PATH_UP) || (pp->state == PATH_GHOST))) {
>  				/* persistent reseravtion check*/
>  				mpath_pr_event_handle(pp);
>  			}
> @@ -3570,7 +3570,8 @@ void *  mpath_pr_event_handler_fn (void * pathp )
>  	{
>  		condlog(0,"%s: Reservation registration failed. Error: %d", pp->dev, ret);
>  	}
> -	mpp->prflag = 1;
> +	mpp->prflag = PRFLAG_OK;
> +	pp->prflag = PRFLAG_OK;
> 
>  	free(param);
>  out:
> --