multipath: change default configuration parameters

PATCH In review Benjamin Marzinski 2 participants 4 messages Jan 10, 2013 – Jan 14, 2013 Device mapper

Share & tools

Benjamin Marzinski

author maintainer

2013-01-10 20:05 UTC #0 lore raw

PATCH v2 1/1 multipath: change default configuration parameters

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".

Signed-off-by Benjamin Marzinski <[email protected]>

---
 libmultipath/config.c   |    1 
 libmultipath/defaults.h |    2 -
 libmultipath/hwtable.c  |   65 ------------------------------------------------
 3 files changed, 2 insertions(+), 66 deletions(-)

libmultipath/config.c +1 -0

Index: 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

libmultipath/hwtable.c +0 ~~-65~~

455 lines not on this page · raw message

libmultipath/defaults.h +1 -1

Index: 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"

Hannes Reinecke

2013-01-11 07:00 UTC #1 lore raw

Quoted 5 lines · Benjamin Marzinski

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

Quoted 12 lines

Signed-off-by Benjamin Marzinski <[email protected]>
---
  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
===================================================================

libmultipath/config.c load_config (char * file) · line 522

Quoted 7 lines

libmultipath/config.c +1 -0

@@ -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.

libmultipath/hwtable.c static struct hwentry default_hw[] = { · line 33

Quoted 12 lines

Index: multipath-tools-120821/libmultipath/hwtable.c
===================================================================

libmultipath/hwtable.c +0 -1

@@ -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 ...

Quoted 6 lines

Index: multipath-tools-120821/libmultipath/defaults.h
===================================================================

libmultipath/defaults.h line 7

Quoted 8 lines

libmultipath/defaults.h +1 -1

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

Signature · 4 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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)

Benjamin Marzinski

author maintainer

2013-01-11 17:01 UTC #2 lore raw

Quoted 10 lines · Hannes Reinecke

Quoted 5 lines · Benjamin Marzinski
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 ...

I suppose. However, each config change is easily isolatable by editting multipath.conf.

Quoted 17 lines

Quoted 17 lines
Signed-off-by Benjamin Marzinski <[email protected]>
---
  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
===================================================================
libmultipath/config.c +1 -0
@@ -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;
 

Quoted 4 lines

libmultipath/config.c load_config (char * file) · line 522
Quoted 2 lines
libmultipath/config.c +0 -0
  	/*
  	 * preload default hwtable
Please use #define DEFAULT_FAST_IO_FAIL here.

fine.

Quoted 9 lines

Quoted 9 lines
Index: multipath-tools-120821/libmultipath/hwtable.c
===================================================================
libmultipath/hwtable.c +0 -1
@@ -28,7 +28,6 @@ static struct hwentry default_hw[] = {
  		.product       = "Compellent Vol",
  		.features      = DEFAULT_FEATURES,
  		.hwhandler     = DEFAULT_HWHANDLER,
-		.selector      = DEFAULT_SELECTOR,

Quoted 8 lines

libmultipath/hwtable.c static struct hwentry default_hw[] = { · line 33
Quoted 3 lines
libmultipath/hwtable.c +0 -0
  		.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 ...

Not all of them, I think. For instance, for many devices, we know that we don't want a hardware handler. Someone could set the hardware handler in their defaults section to deal with all their devices that don't already have a device specific config built in. In this case, we want the devices with a built-in config to not use that new hwhandler setting. I think a good question would be, "will changing this option make the device not work correctly?" If so, then it should be explicitly set in the device config. If not, then it can inherit it from default.

So, DEFAULT_HWHANDLER, DEFAULT_CHECKER, and DEFAULT_PRIO, must be stay explicitly set. DEFAULT_FEATURES is kind of questionable but I think it should stay as well. DEFAULT_MINIO and DEFAULT_MINIO_RQ are safe to inherit from the defaults section. Does that sound reasonable?

Quoted 16 lines

Quoted 6 lines
Index: multipath-tools-120821/libmultipath/defaults.h
===================================================================
libmultipath/defaults.h line 7
Quoted 8 lines
libmultipath/defaults.h +1 -1
@@ -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.

Quoted 1 line

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

I've tested it with the admittedly non-exhaustive supply of hardware I have available. In my experience, service-time or queue-length are both pefectly good choices. But on some loads to some arrays, they do outperform round-robin, and I've never seen them noticeably worse.

When people ask for performance tuning tips and we have them try out the dynamic load balancing selectors, I've never heard a report that they underperform round-robin. Have you seen different results? If there's a good reason to think it might cause problems, I'm fine with leaving round-robin as the default. But I think we should try to get the defaults to what we believe will provide the best performance on most devices.

-Ben

Quoted 13 lines

Cheers,

Hannes

Signature · 9 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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)

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Hannes Reinecke

2013-01-14 15:50 UTC #3 lore raw

Quoted 96 lines · Benjamin Marzinski

Quoted 10 lines · Hannes Reinecke
Quoted 5 lines · Benjamin Marzinski
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 ...
I suppose. However, each config change is easily isolatable by editting multipath.conf.
Quoted 17 lines
Quoted 17 lines
Signed-off-by Benjamin Marzinski <[email protected]>
---
   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
===================================================================
libmultipath/config.c +1 -0
@@ -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;
 
Quoted 4 lines
libmultipath/config.c load_config (char * file) · line 522
Quoted 2 lines
libmultipath/config.c +0 -0
   	/*
   	 * preload default hwtable
Please use #define DEFAULT_FAST_IO_FAIL here.
fine.
Quoted 9 lines
Quoted 9 lines
Index: multipath-tools-120821/libmultipath/hwtable.c
===================================================================
libmultipath/hwtable.c +0 -1
@@ -28,7 +28,6 @@ static struct hwentry default_hw[] = {
   		.product       = "Compellent Vol",
   		.features      = DEFAULT_FEATURES,
   		.hwhandler     = DEFAULT_HWHANDLER,
-		.selector      = DEFAULT_SELECTOR,
Quoted 8 lines
libmultipath/hwtable.c static struct hwentry default_hw[] = { · line 33
Quoted 3 lines
libmultipath/hwtable.c +0 -0
   		.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 ...
Not all of them, I think. For instance, for many devices, we know that we don't want a hardware handler. Someone could set the hardware handler in their defaults section to deal with all their devices that don't already have a device specific config built in. In this case, we want the devices with a built-in config to not use that new hwhandler setting. I think a good question would be, "will changing this option make the device not work correctly?" If so, then it should be explicitly set in the device config. If not, then it can inherit it from default.
So, DEFAULT_HWHANDLER, DEFAULT_CHECKER, and DEFAULT_PRIO, must be stay explicitly set. DEFAULT_FEATURES is kind of questionable but I think it should stay as well. DEFAULT_MINIO and DEFAULT_MINIO_RQ are safe to inherit from the defaults section. Does that sound reasonable?
Quoted 16 lines
Quoted 6 lines
Index: multipath-tools-120821/libmultipath/defaults.h
===================================================================
libmultipath/defaults.h line 7
Quoted 8 lines
libmultipath/defaults.h +1 -1
@@ -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.
Quoted 1 line
And you _have_ tested 'service-time' thoroughly, now have you?
I've tested it with the admittedly non-exhaustive supply of hardware I have available. In my experience, service-time or queue-length are both pefectly good choices. But on some loads to some arrays, they do outperform round-robin, and I've never seen them noticeably worse.

Quoted 8 lines

When people ask for performance tuning tips and we have them try out the dynamic load balancing selectors, I've never heard a report that they underperform round-robin. Have you seen different results? If there's a good reason to think it might cause problems, I'm fine with leaving round-robin as the default. But I think we should try to get the defaults to what we believe will provide the best performance on most devices.

Actually, I'm not worried about performance, but rather stability.
multipath-tools took _ages_ to stabilize, and we're still somewhat
on the shaky side. So I'm rather loath to make new features the default without having tested them thoroughly.

But if you say so ...

Cheers,

Hannes

Signature · 4 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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)