On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote:
> This patchset builds on top of my previous libmpathpersist patchset,
> ("Improve mpathpersist's unavailable path handling") and add various
> fixes and cleanups to multipath's persistent reservation handling.
>
> The issues handled by this patchset are:
> - Handling removal of keys from paths that were down when the
> multipath
> device was unregistered.
> - Changing how conflicts on key registration are handled. Instead of
> the current rollback method (which was broken anyways),
> libmpathpersist now retrys with REGISTER AND IGNORE, as long as the
> REGISTER command completed successfully down some of the paths.
> - Changing when the reservation key is set to fix corner cases on
> failure and registration while paths are coming up.
> - Retrying on conflicts in mpath_prout_common to fix corner cases
> when
> a path is coming up while a doing a reserve, preempt or clear.
> - Allowing registrations to succeed when there are retryable errors,
> if the paths are actually down.
> - Fixing the reservation key validation code
>
> I realize that a number of these fixes are dealing with races between
> libmpathperist and multipathd, which points to shortcomings with the
> current design of libmpathpersist. The code could likely be simpler
> and
> more robust if limpathpersist was a thin wrapper around commands to
> multipathd, which handled issuing the actual persistent reservation
> commands. However, we have customers that need persistent
> reservations
> working well on currently released versions of the mutipath-tools,
> and
> redesigning how the libmpathpersist library works is a much larger
> task,
> and even harder to backport. I'd like to know if people think that
> would
> be a worthwhile task for later, however.
Difficult to say. It seems to me that you have eliminated most obvious
races. But yes, it would be cleaner to have a single instance
(multipathd) deal with PRs.
It is awkward that libmpathpersist contains code that sends commands to
multipathd, while multipathd itself links with libmpathpersist. This
begs for a separation of libraries.
Also, I think there are still too many instances of condlog(0, ...) in
libmpathpersist. But then, there are too many in our code in general,
so we can clean this up later.
>
> Benjamin Marzinski (14):
> limpathpersist: remove update_map_pr code for NULL pp
> libmpathpersist: move update_map_pr to multipathd
> multipathd: clean up update_map_pr and mpath_pr_event_handle
> libmpathpersist: clean up duplicate function declarations
> multipathd: wrap setting and unsetting prflag
> multipathd: unregister PR key when path is restored if necessary
> libmpathpersist: Fix-up reservation_key checking
> libmpathpersist: change how reservation conflicts are handled
> libmpathpersist: Clear prkey in multipathd before unregistering
> libmpathpersist: only clear the key if we are using the prkeys file
> libmpathpersist: Restore old reservation key on failure
> libmpatpersist: update reservation key before checking paths
> libmpathpersist: retry on conflicts in mpath_prout_common
> libmpathpersist: Don't always fail registrations for retryable
> errors
>
> libmpathpersist/libmpathpersist.version | 1 -
> libmpathpersist/mpath_persist_int.c | 363 +++++++++++++---------
> --
> libmpathpersist/mpath_persist_int.h | 2 -
> libmpathpersist/mpath_pr_ioctl.c | 12 +-
> libmultipath/structs.h | 2 +
> mpathpersist/main.c | 2 -
> multipathd/cli_handlers.c | 11 +-
> multipathd/main.c | 132 +++++++--
> multipathd/main.h | 2 +
> 9 files changed, 311 insertions(+), 216 deletions(-)
For the series, except for the patches I replied to:
Reviewed-by: Martin Wilck <[email protected]>
As for the previous series, I have applied out clang-format formatting
and fixed a few typos, and pushed the result to my tip branch [1].
Regards
Martin
