Re: [PATCH 2/4] multipathd: finish initalization of paths added while offline

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Jan 21, 2026 at 01:41:35PM +0100, Martin Wilck wrote:
> 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 <bmarzins@redhat.com>
> 
> 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.

I'm fine with making that change. I assume that we can skip the
strnlen(), since we don't use that anywhere, and if we switch the
initial strcpy() to a strlcpy() it's pretty obvious that wwid must be
null terminated.
 
> > +	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:

Good point.

-Ben
 
> 
> 	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