On Thu, 2025-09-25 at 15:38 -0400, Benjamin Marzinski wrote:
> On Wed, Sep 24, 2025 at 08:37:36PM +0200, Martin Wilck wrote:
> > On Tue, 2025-09-23 at 18:12 -0400, Benjamin Marzinski wrote:
> > > -
> > > - num =
> > > resp.prin_descriptor.prin_readresv.additional_length /
> > > 8;
> > > - if (num == 0){
> > > - condlog (2, "%s: Path holding reservation is
> > > released.", mpp->wwid);
> > > - return MPATH_PR_SUCCESS;
> > > - }
> > > - if (!get_be64(mpp->reservation_key) ||
> > > - memcmp(&mpp->reservation_key,
> > > resp.prin_descriptor.prin_readresv.key, 8)) {
> > > + if (!check_holding_reservation(mpp, &res_type)) {
> >
> > The old code didn't call check_prhold() while
> > check_holding_reservation() does.
> >
> > Is it possible that get_prhold() would return a "false negative"
> > and
> > thus we'd wrongly skip releasing the reservation?
>
> At the point where we check this, we've already done the RELEASE and
> gotten a success, but the reservation is still held. This means that
> either an unavailable path on this device is holding it, or another
> device that is using the same key is holding it. These checks are to
> determine whether or not we PREEMPT the key to force-clear the
> reservation. What we are adding here is a requirement that multipathd
> says we are holding the reservation.
>
> I'm not sure how we would get a false negative. When multipathd first
> starts tracking a device (when it starts up, creates a new device, or
> is
> reconfigured) it assumes that there aren't multiple devices holding
> the
> same key. So if it sees that its configured key is holding the
> reservation, it assumes that this device is holding the reservation.
> It
> will only unset the holding state if it notices that the devices key
> is
> not registered, or it is told to drop the reservation. Once it is
> started the only way it will set the holding state on a device is
> when
> libmpathpersist tells it to, but that should always happen when
> libmpathpersist grabs a reservation.
>
> Now, this code is admittedly complex, and there are lots of corner
> cases
> that I might not have thought through which could produce a false
> negative, and it is always possible that libmpathpersist fails to
> communicate with multipathd to set prhold. Also, I don't think we
> really
> need to worry much about another device holding a duplicate key when
> we
> are explicitly asked to release the reservation (that means whoever
> is
> issuing this command also thinks we own the device).
> So I'm fine with backing this change out.
That was not my intention. I just notived the change andwanted to get
confirmation from you that this was well thought-out. Which you have
given in your response.
> I was on the fence about it
> myself. If I do, check_holding_reservation() and
> reservation_key_matches() will still need to be moved earlier in the
> code for the next patch. That patch also makes changes to
> check_holding_reservation(). Do you prefer moving the functions in a
> separate patch from editting them. Or are you fine with doing both in
> the same patch, as long as I call it out.
You can leave it as-is.
Regards
Martin
