On Thu, 2025-08-28 at 23:37 -0400, Benjamin Marzinski wrote: > On Mon, Aug 25, 2025 at 06:36:18PM +0200, Martin Wilck wrote: > > On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote: > > > There is a race condition when changing reservation keys where a > > > failed > > > path could come back online after libmpathpersist checks the > > > paths, > > > but > > > before it updates the reservation key. In this case, the path > > > would > > > come > > > up and get reregistered with the old key by multipathd, and > > > libmpathpersist would not update its key, because the path was > > > down > > > when it checked. > > > > > > To fix this, check the paths after updating the key, so any path > > > that > > > comes up after getting checked will use the updated key. > > > > In do_mpath_persistent_reserve_out(), you call update_prkey_flags() > > before checking for MPATH_PR_SYNTAX_ERROR. Perhaps you should check > > the > > key parameters first? > > We could, but the way the code currently is, if you called > update_prkey_flags() earlier, you can't fail those > MPATH_PR_SYNTAX_ERROR > checks. > > You can only call update_prkey_flags() if rq_servact is > MPATH_PROUT_REG_IGN_SA or MPATH_PROUT_REG_SA, so we only need to look > at > the True branch of the outermost if statement in those > MPATH_PR_SYNTAX_ERROR checks. We also cannot fail those checks if > unregistering is true, so we only care about the case were > paramp->sa_key is non-zero. And when we call update_prkey_flags(), we > also copy the value of paramp->sa_key to mpp->reservation_key, so it > too > must be non-zero. This means that can't fail the checks since they > check if mpp->reservation_key is zero or not equal to paramp->sa_key. > > But it doesn't hury anything to move the update_prkey_flags() call to > after those checks either. So I'm fine with either solving this by > adding an explanation of why we don't need to worry about failing > these > checks if we updated the key to the comments above the checks, or by > just moving the update_prkey_flags() call to after them. Do you have > a > preference?
I gather that the conditional would become more complicated when we move the code. A comment with an explanation will do the job. Martin
