Re: [PATCH 1/2] libmultipath: hwhandler auto-detection for ALUA

[Martin Wilck <mwilck@suse.com>]

On Tue, 2018-04-03 at 15:31 -0500, Benjamin Marzinski wrote:
> On Tue, Mar 27, 2018 at 11:50:52PM +0200, Martin Wilck wrote:
> > If the hardware handler isn't explicitly set, infer ALUA support
> > from the pp->tpgs attribute. Likewise, if ALUA is selected, but
> > not supported by the hardware, fall back to no hardware handler.
> 
> Weren't you worried before about temporary ALUA failures? If you had
> a
> temporary failure while configuring a device that you explicitly set
> to
> be ALUA, then this would cause the device to be misconfigured? 

I believe that if TGPS is 0, the device will never be able to support
ALUA. The kernel also looks at the TPGS bits and won't try ALUA if they
are unset. Once the device is configured and actual ALUA RTPG/STPG
calls are performed, they may fail for a variety of temporary reasons -
I wanted to avoid resetting the prio algorithm to "const" for such
cases. That's my understanding, correct me if I'm wrong.

> If the
> hardware handler isn't set, inferring ALUA is fine. But what is the
> case
> where we want to say that a device that is explicitly set to ALUA
> shouldn't actually be ALUA?  It seem like if there is some
> uncertaintly,
> we should just not set the hardware handler, and allow multipath to
> infer it via the pp->tpgs value.
> 
> I'm not strongly against this patch. I just don't see the value in
> overriding an explicit configuration, if we believe that temporary
> failures are possible.

That would be fine if we didn't have any explicit "hardware_handler
alua" settings in the hardcoded hwtable any more, or at least if we're 
positive that those devices where we have "hardware_handler alua"
really support it.

We can also adopt the philosophy of "detect_prio" and "detect_checker",
add an additional config file option "detect_hwhandler", and look at
tpgs only if the latter it set (which would be the default). Like
detect_prio, users could then enforce their config file settings with
"detect_hwhandler no".

I was hoping we could find a simpler approach, without yet another
rarely-used config option.

Btw, at SUSE we solved our problem with the controller at hand by
simply removing "hardware_handler alua" and "prio alua" from the IBM
IPR entry. If the scsi_dh_alua module is loaded early (default on
SUSE), this results in ALUA hwhandler and sysfs prio being used for IPR
controllers that do support ALUA, and no hwhandler / const prio =
PRIO_UNDEF for those that don't. I'm not sure if that simple solution
suits upstream, because upstream doesn't enforce early loading of
device handler modules.

Regards,
Martin


> 
> -Ben
> 
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/propsel.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index 93974a482336..dc24450eb775 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -43,10 +43,13 @@ do {						
> > 			\
> >  		goto out;						
> > \
> >  	}								
> > \
> >  } while(0)
> > +
> > +static char default_origin[] = "(setting: multipath internal)";
> > +
> >  #define do_default(dest, value)					
> > 	\
> >  do {								
> > 	\
> >  	dest = value;						
> > 	\
> > -	origin = "(setting: multipath internal)";			
> > \
> > +	origin = default_origin;					
> > \
> >  } while(0)
> >  
> >  #define mp_set_mpe(var)						
> > 	\
> > @@ -373,16 +376,20 @@ static int get_dh_state(struct path *pp, char
> > *value, size_t value_len)
> >  
> >  int select_hwhandler(struct config *conf, struct multipath *mp)
> >  {
> > -	char *origin;
> > +	const char *origin;
> >  	struct path *pp;
> >  	/* dh_state is no longer than "detached" */
> >  	char handler[12];
> > +	static char alua_name[] = "1 alua";
> > +	static const char tpgs_origin[]= "(setting: autodetected
> > from TPGS)";
> >  	char *dh_state;
> >  	int i;
> > +	bool all_tpgs = true;
> >  
> >  	dh_state = &handler[2];
> >  	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
> >  		vector_foreach_slot(mp->paths, pp, i) {
> > +			all_tpgs = all_tpgs && (pp->tpgs > 0);
> >  			if (get_dh_state(pp, dh_state,
> > sizeof(handler) - 2) > 0
> >  			    && strcmp(dh_state, "detached")) {
> >  				memcpy(handler, "1 ", 2);
> > @@ -397,6 +404,14 @@ int select_hwhandler(struct config *conf,
> > struct multipath *mp)
> >  	mp_set_conf(hwhandler);
> >  	mp_set_default(hwhandler, DEFAULT_HWHANDLER);
> >  out:
> > +	if (all_tpgs && !strcmp(mp->hwhandler, DEFAULT_HWHANDLER)
> > &&
> > +		origin == default_origin) {
> > +		mp->hwhandler = alua_name;
> > +		origin = tpgs_origin;
> > +	} else if (!all_tpgs && !strcmp(mp->hwhandler, alua_name))
> > {
> > +		mp->hwhandler = DEFAULT_HWHANDLER;
> > +		origin = tpgs_origin;
> > +	}
> >  	mp->hwhandler = STRDUP(mp->hwhandler);
> >  	condlog(3, "%s: hardware_handler = \"%s\" %s", mp->alias,
> > mp->hwhandler,
> >  		origin);
> > -- 
> > 2.16.1
> 
> 

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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel