Re: [PATCH 10/15] libmpathpersist: fail the release if all threads fail

[Benjamin Marzinski <bmarzins@redhat.com>]

On Sun, Aug 24, 2025 at 05:33:07PM +0200, Martin Wilck wrote:
> On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> > If none of the threads succeeds in issuing the release, simply return
> > failure, instead of trying the workaround.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index 7fb08b2e..ad5a4ee7 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> > @@ -475,15 +475,21 @@ static int mpath_prout_rel(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  		}
> >  	}
> >  
> > +	rc = MPATH_PR_DMMP_ERROR;
> 
> I find the relationship between "status" and "rc" a bit confusing in
> this patch. Can we use something like "bool all_failed" instead?

Sure.

-Ben

> 
> Martin
> 
> 
> >  	for (i = 0; i < count; i++){
> >  		/*  check thread status here and return the status
> > */
> >  
> > -		if (thread[i].param.status ==
> > MPATH_PR_RESERV_CONFLICT)
> > +		if (thread[i].param.status == MPATH_PR_SUCCESS)
> > +			rc = MPATH_PR_SUCCESS;
> > +		else if (thread[i].param.status ==
> > MPATH_PR_RESERV_CONFLICT)
> >  			status = MPATH_PR_RESERV_CONFLICT;
> > -		else if (status == MPATH_PR_SUCCESS
> > -				&& thread[i].param.status !=
> > MPATH_PR_RESERV_CONFLICT)
> > +		else if (status == MPATH_PR_SUCCESS)
> >  			status = thread[i].param.status;
> >  	}
> > +	if (rc != MPATH_PR_SUCCESS) {
> > +		condlog(0, "%s: all threads failed to release
> > reservation.", mpp->wwid);
> > +		return status;
> > +	}
> >  
> >  	status = mpath_prin_activepath (mpp, MPATH_PRIN_RRES_SA,
> > &resp, noisy);
> >  	if (status != MPATH_PR_SUCCESS){