Re: [PATCH v2] multipath: change default configuration parameters

[Hannes Reinecke <hare@suse.de>]

On 01/10/2013 09:05 PM, Benjamin Marzinski wrote:
> This patch makes multipath devices default to setting fast_io_fail,
> switches the default selector to service-time and removes the
> round-robin setting of the builtin device configurations. The goal is
> to give multipath devices better performance "out of the box".
>
Couldn't you split it off in three patches?
(Bad) experience tells me one always run into problems when
munching together unrelated pieces ...

> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>   libmultipath/config.c   |    1
>   libmultipath/defaults.h |    2 -
>   libmultipath/hwtable.c  |   65 ------------------------------------------------
>   3 files changed, 2 insertions(+), 66 deletions(-)
>
> Index: multipath-tools-120821/libmultipath/config.c
> ===================================================================
> --- multipath-tools-120821.orig/libmultipath/config.c
> +++ multipath-tools-120821/libmultipath/config.c
> @@ -516,6 +516,7 @@ load_config (char * file)
>   	conf->reassign_maps = DEFAULT_REASSIGN_MAPS;
>   	conf->checkint = DEFAULT_CHECKINT;
>   	conf->max_checkint = MAX_CHECKINT(conf->checkint);
> +	conf->fast_io_fail = 5;
>
>   	/*
>   	 * preload default hwtable

Please use #define DEFAULT_FAST_IO_FAIL here.

> Index: multipath-tools-120821/libmultipath/hwtable.c
> ===================================================================
> --- multipath-tools-120821.orig/libmultipath/hwtable.c
> +++ multipath-tools-120821/libmultipath/hwtable.c
> @@ -28,7 +28,6 @@ static struct hwentry default_hw[] = {
>   		.product       = "Compellent Vol",
>   		.features      = DEFAULT_FEATURES,
>   		.hwhandler     = DEFAULT_HWHANDLER,
> -		.selector      = DEFAULT_SELECTOR,
>   		.pgpolicy      = MULTIBUS,
>   		.pgfailback    = -FAILBACK_IMMEDIATE,
>   		.rr_weight     = RR_WEIGHT_NONE,

Can't we get rid of _all_ 'DEFAULT_XXX' settings in the
hardware table? They _should_ be set to default during
init anyway, shouldn't they?
And that would shorten the hardware table by quite a lot ...


> Index: multipath-tools-120821/libmultipath/defaults.h
> ===================================================================
> --- multipath-tools-120821.orig/libmultipath/defaults.h
> +++ multipath-tools-120821/libmultipath/defaults.h
> @@ -1,7 +1,7 @@
>   #define DEFAULT_UID_ATTRIBUTE	"ID_SERIAL"
>   #define DEFAULT_UDEVDIR		"/dev"
>   #define DEFAULT_MULTIPATHDIR	"/" LIB_STRING "/multipath"
> -#define DEFAULT_SELECTOR	"round-robin 0"
> +#define DEFAULT_SELECTOR	"service-time 0"
>   #define DEFAULT_ALIAS_PREFIX	"mpath"
>   #define DEFAULT_FEATURES	"0"
>   #define DEFAULT_HWHANDLER	"0"
>
Hmm.

And you _have_ tested 'service-time' thoroughly, now have you?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)