[dm-devel] [PATCH 46/54] libmultipath: path_discover(): always set DI_BLACKLIST

[Martin Wilck]

On Fri, 2020-07-17 at 17:21 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:36:15PM +0200, mwilck at suse.com wrote:
> > From: Martin Wilck <mwilck at suse.com>
> > 
> > Since 65e1845 ("multipath: call store_pathinfo with DI_BLACKLIST"),
> > we
> > use DI_BLACKLIST for new paths. There's no reason why we shouldn't
> > do the
> > same with paths which are (unexpectedly) already in pathvec. As
> > argued
> > for 65e1845, this might save some unnecessary work for paths which
> > are
> > blacklisted anyway.
> 
> It seems to me like either we should assume that if we added the path
> to
> pathvec, it's valid, or we should check, and if it's blacklisted,
> remove
> it. Just leaving it on pathvec without all of the pathinfo work done
> doesn't make much sense to me. Am I missing something here?

TL;DR: let's skip this patch for now.

path_discover() is only called from path_discovery(). path_discovery()
is called from 

 1 configure() (multipathd), with an empty pathvec
 2 configure() (multipath), with a pathvec possibly pre-populated by
get_refwwid()
 3 check_valid_path(), with pathvec pre-populated with the path to be
checked

For 1) my patch obviously makes no difference. In case 2) get_refwwid()
sets DI_BLACKLIST anyway (except for CMD_REMOVE_WWID, in which case
path_discovery() is not called). In case 3), DI_BLACKLIST
has already been used by is_path_valid() (the only exception being the
"is already multipathed" case, in which path_discovery() is not
called).

So, this patch makes no difference in practice - all callers make sure
that the pathvec is pre-populated only with non-blacklisted paths.

The reason I wrote this patch was because I wanted to use consistent
flags in path_discover() - it was non-obvious to me why we treated
existing and new paths differently. The side effect was a very small
speed-up. 

But your argument above is valid. 

To me it would come as a surprise if path_discovery() removed paths;
that's the kind of side effect I'd like to minimize in our code. Thus,
path_discover() should rely on the caller wrt the pre-populated paths,
and not use DI_BLACKLIST on them.

For now, I'll replace this patch with a comment explaining why we use
different flags in the two pathinfo() calls. 

In the future, we may want to change path_discovery() such that it only
works on an empty pathvec. IMO that would be cleaner, and require only
minimal changes to callers.

Thanks,
Martin


> -Ben
>  
> > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > ---
> >  libmultipath/discovery.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index 5f4ebf0..caabfef 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -137,7 +137,7 @@ path_discover (vector pathvec, struct config *
> > conf,
> >  				      udevice, flag | DI_BLACKLIST,
> >  				      NULL);
> >  	else
> > -		return pathinfo(pp, conf, flag);
> > +		return pathinfo(pp, conf, flag | DI_BLACKLIST);
> >  }
> >  
> >  static void cleanup_udev_enumerate_ptr(void *arg)
> > -- 
> > 2.26.2