Hi Ben,
On Tue, 2026-01-20 at 23:23 -0500, Benjamin Marzinski wrote:
> If a path in a mulitpath device is offline while multipathd is
> reconfigured, it will get added to the updated multipath device, just
> like it was in the old multipath device. However the device will
> still
> be in the INIT_NEW state because it can't get initilized while
> offline.
> This is different than the INIT_PARTIAL state because the path was
> discovered in path_discovery(). INIT_PARTIAL is for paths that
> multipathd did not discover in path_discovery() or receive a uevent
> for,
> but are part of a multipath device that was added, and which should
> receive a uevent shortly. There is no reason to expect a uevent for
> these offline paths.
>
> When the path comes back online, multipathd will run the checker and
> prioritizer on it. The only two pathinfo checks that won't happen
> are the DI_WWID and DI_IOCTL ones. Modify pathinfo() to make sure
> that if DI_IOCTL was skipped for offline paths, it gets called the
> next time pathinfo() is called after the fd can be opened. Also,
> make sure that when one of these offline paths becomes usable, its
> WWID is rechecked. With those changes, all the DI_ALL checks will
> be accounted for, and the path can be marked INIT_OK.
>
> Signed-off-by: Benjamin Marzinski <[email protected]>
Great observation, thanks!
> ---
> libmultipath/discovery.c | 2 ++
> multipathd/main.c | 52 +++++++++++++++++++++++++++++++++++---
> --
> 2 files changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 0c5e5ca6..0efb8213 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -2585,6 +2585,8 @@ blank:
> * Recoverable error, for example faulty or offline path
> */
> pp->chkrstate = pp->state = PATH_DOWN;
> + if (mask & DI_IOCTL && pp->ioctl_info ==
> IOCTL_INFO_NOT_REQUESTED)
> + pp->ioctl_info = IOCTL_INFO_SKIPPED;
> if (pp->initialized == INIT_NEW || pp->initialized ==
> INIT_FAILED)
> memset(pp->wwid, 0, WWID_SIZE);
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 61e0ea34..2140e432 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2572,6 +2572,26 @@ static int sync_mpp(struct vectors *vecs,
> struct multipath *mpp, unsigned int ti
> return do_sync_mpp(vecs, mpp);
> }
>
> +/*
> + * pp->wwid should never be empty when this function is called, but
> if it
> + * is, this function can set it.
> + */
> +static bool new_path_wwid_changed(struct path *pp, int state)
> +{
> + char wwid[WWID_SIZE];
> +
> + strcpy(wwid, pp->wwid);
> + if (get_uid(pp, state, pp->udev, 1) != 0) {
> + strcpy(pp->wwid, wwid);
> + return false;
> + }
> + if (strlen(wwid) && strcmp(wwid, pp->wwid) != 0) {
> + strcpy(pp->wwid, wwid);
> + return true;
> + }
It feels a bit odd to use strcpy() and strcmp() here. I am fine with
doing that if the arguments are string constants, where it's
immediately obvious while are obviously properly null-terminated, but
this isn't the case here. I'd prefer using strlcpy() and strncmp(),
even if we are both convinced that it's ok without the length check.
> + return false;
> +}
> +
> static int
> update_path_state (struct vectors * vecs, struct path * pp)
> {
> @@ -2601,14 +2621,34 @@ update_path_state (struct vectors * vecs,
> struct path * pp)
> return CHECK_PATH_SKIPPED;
> }
>
> - if (pp->recheck_wwid == RECHECK_WWID_ON &&
> + if ((pp->recheck_wwid == RECHECK_WWID_ON || pp->initialized
> == INIT_NEW) &&
The readbility of this code would improve if you move the INIT_NEW vs.
RECHECK_WWID_ON test into an inner conditional, like this:
if ((newstate == PATH_UP || newstate == PATH_GHOST) &&
((pp->state != PATH_UP && pp->state != PATH_GHOST)) ||
pp->dmstate == PSTATE_FAILED) {
bool wwid_changed;
if (pp->initialized == INIT_NEW) {
...
} else ... ;
if (wwid_changed) { ... };
}
Martin
