On Thu, 2025-08-28 at 23:21 -0400, Benjamin Marzinski wrote: > On Mon, Aug 25, 2025 at 06:21:59PM +0200, Martin Wilck wrote: > > On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote: > > > Otherwise this request will create a useless prkeys file. > > > > > > Signed-off-by: Benjamin Marzinski <bmarz...@redhat.com> > > > --- > > > libmpathpersist/mpath_persist_int.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/libmpathpersist/mpath_persist_int.c > > > b/libmpathpersist/mpath_persist_int.c > > > index dfdadab6..f901b955 100644 > > > --- a/libmpathpersist/mpath_persist_int.c > > > +++ b/libmpathpersist/mpath_persist_int.c > > > @@ -831,7 +831,8 @@ int do_mpath_persistent_reserve_out(vector > > > curmp, > > > vector pathvec, int fd, > > > break; > > > case MPATH_PROUT_CLEAR_SA: > > > update_prflag(mpp->alias, 0); > > > - update_prkey(mpp->alias, 0); > > > + if (mpp->prkey_source == PRKEY_SOURCE_FILE) > > > + update_prkey(mpp->alias, 0); > > > > Should this condition be checked in update_prkey_flags() directly? > > We could, but it would just end up being a pointless extra check most > of > the time. Aside from the CLEAR command, we only set the prkey when we > are registering a key or reverting the prkey if that registration > failed. When we first set it during register, we need to check > prkey_source early, since there is a bunch of things we can't do if > we > aren't using the prkeys file. And we don't want to revert it unless > we > updated it and have an old value to revert to. In both cases we > already > had to do the necessary check before calling update_prkey(). The > CLEAR > command is the only one where we don't need to check if we are using > the > prkeys file earlier. So, I'd prefer to leave the check outside of > update_prkey_flags() here as well, to avoid the unnecessary extra > work.
Well, the unnecessary work is just a single instruction in this case. I think multipathd has more pressing efficiency issues. Perhaps we could improve our layering. But it isn't important. I'm fine with leaving it as it currently is. Reviewed-by: Martin Wilck <mwi...@suse.com> Martin
