Re: [PATCH v2 07/12] multipathd: ignore failed wwid recheck

[Benjamin Marzinski <bmarzins@redhat.com>]

On Fri, Mar 15, 2019 at 12:48:49PM +0100, Martin Wilck wrote:
> On Fri, 2019-03-08 at 17:12 -0600, Benjamin Marzinski wrote:
> > If disable_changed_wwids is set, when multipathd gets a change event
> > on
> > a path, it verifies that the wwid hasn't changed in
> > uev_update_path().
> > If get_uid() failed, uev_update_path treated this as a wwid change to
> > 0.
> > This could cause paths to suddenly be dropped due to an issue with
> > getting the wwid.  Even if get_uid() failed because the path was
> > down,
> > it no change uevent happend when it later became active, multipathd
> > would continue to ignore the path. Also, scsi_uid_fallback() clears
> > the
> > failure return if it doesn't attempt to fallback, causing get_uid()
> > to return success, when it actually failed.
> > 
> > Multipathd should neither set nor clear wwid_changed if get_uid()
> > returned failure. Also, scsi_uid_fallback() should retain the old
> > return
> > value if it doesn't attempt to fallback.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/discovery.c | 6 +++---
> >  multipathd/main.c        | 6 ++++--
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index 729bcb9..b08cb2d 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1755,9 +1755,9 @@ get_vpd_uid(struct path * pp)
> >  }
> >  
> >  static ssize_t scsi_uid_fallback(struct path *pp, int path_state,
> > -			     const char **origin)
> > +			     const char **origin, ssize_t old_len)
> >  {
> > -	ssize_t len = 0;
> > +	ssize_t len = old_len;
> >  	int retrigger;
> >  	struct config *conf;
> 
> Please don't call this variable "old_len" but "errcode" or the like. If
> this is called, "old_len" is always negative (indicating a previous
> error) or 0 (indicating previous attempts returned an empty WWID, which
> is also likely an error).

Sure.
 
> Otherwise, ACK. But you revert most of this later in 12/12; I think the
> two should be merged (except for the scsi_uid_fallback part, maybe).

See my reply to your "New approach at handling changed WWIDs" patches.
Something like this patch is still my preferred solution, for reasons I
explain there.

-Ben

> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
>