* [PATCH 01/12] Fix memory issues in path reordering failure code paths
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
@ 2014-06-30 5:13 ` Benjamin Marzinski
2014-07-01 18:45 ` Christophe Varoqui
2014-06-30 5:13 ` [PATCH 02/12] Return the correct size buffer in set_value() Benjamin Marzinski
` (10 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:13 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
There were some possible NULL pointer dereferences and multiple frees
in the failure code paths of the path reordering code. This patch
cleans them up.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/configure.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 3ed6b55..6ad7a80 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -160,8 +160,16 @@ int order_paths_in_pg_by_alt_adapters(struct pathgroup *pgp, vector adapters,
while (total_paths > 0) {
agp = VECTOR_SLOT(adapters, next_adapter_index);
+ if (!agp) {
+ condlog(0, "can't get adapter group %d", next_adapter_index);
+ return 1;
+ }
hgp = VECTOR_SLOT(agp->host_groups, agp->next_host_index);
+ if (!hgp) {
+ condlog(0, "can't get host group %d of adapter group %d", next_adapter_index, agp->next_host_index);
+ return 1;
+ }
if (!hgp->num_paths) {
agp->next_host_index++;
@@ -223,8 +231,8 @@ int rr_optimize_path_order(struct pathgroup *pgp)
/* group paths in path group by host adapters
*/
if (group_by_host_adapter(pgp, adapters)) {
+ /* already freed adapters */
condlog(3, "Failed to group paths by adapters");
- free_adaptergroup(adapters);
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 01/12] Fix memory issues in path reordering failure code paths
2014-06-30 5:13 ` [PATCH 01/12] Fix memory issues in path reordering failure code paths Benjamin Marzinski
@ 2014-07-01 18:45 ` Christophe Varoqui
0 siblings, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-01 18:45 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 1776 bytes --]
Applied.
Thanks.
On Mon, Jun 30, 2014 at 7:13 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> There were some possible NULL pointer dereferences and multiple frees
> in the failure code paths of the path reordering code. This patch
> cleans them up.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/configure.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 3ed6b55..6ad7a80 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -160,8 +160,16 @@ int order_paths_in_pg_by_alt_adapters(struct
> pathgroup *pgp, vector adapters,
>
> while (total_paths > 0) {
> agp = VECTOR_SLOT(adapters, next_adapter_index);
> + if (!agp) {
> + condlog(0, "can't get adapter group %d",
> next_adapter_index);
> + return 1;
> + }
>
> hgp = VECTOR_SLOT(agp->host_groups, agp->next_host_index);
> + if (!hgp) {
> + condlog(0, "can't get host group %d of adapter
> group %d", next_adapter_index, agp->next_host_index);
> + return 1;
> + }
>
> if (!hgp->num_paths) {
> agp->next_host_index++;
> @@ -223,8 +231,8 @@ int rr_optimize_path_order(struct pathgroup *pgp)
> /* group paths in path group by host adapters
> */
> if (group_by_host_adapter(pgp, adapters)) {
> + /* already freed adapters */
> condlog(3, "Failed to group paths by adapters");
> - free_adaptergroup(adapters);
> return 0;
> }
>
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 2543 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 02/12] Return the correct size buffer in set_value()
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
2014-06-30 5:13 ` [PATCH 01/12] Fix memory issues in path reordering failure code paths Benjamin Marzinski
@ 2014-06-30 5:13 ` Benjamin Marzinski
2014-07-01 18:47 ` Christophe Varoqui
2014-06-30 5:13 ` [PATCH 03/12] enable gcc format-security check Benjamin Marzinski
` (9 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:13 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
When multipath was mallocing the buffer in set_value, it was using
sizeof(char *), instead of sizeof(char), so it was allocating a buffer
big enough for an array of pointers instead of characters.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/parser.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 526c45b..0d4c870 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -408,11 +408,11 @@ set_value(vector strvec)
len += strlen(str);
if (!alloc)
alloc =
- (char *) MALLOC(sizeof (char *) *
+ (char *) MALLOC(sizeof (char) *
(len + 1));
else {
alloc =
- REALLOC(alloc, sizeof (char *) * (len + 1));
+ REALLOC(alloc, sizeof (char) * (len + 1));
tmp = VECTOR_SLOT(strvec, i-1);
if (alloc && *str != '"' && *tmp != '"')
strncat(alloc, " ", 1);
@@ -422,7 +422,7 @@ set_value(vector strvec)
strncat(alloc, str, strlen(str));
}
} else {
- alloc = MALLOC(sizeof (char *) * (size + 1));
+ alloc = MALLOC(sizeof (char) * (size + 1));
if (alloc)
memcpy(alloc, str, size);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 02/12] Return the correct size buffer in set_value()
2014-06-30 5:13 ` [PATCH 02/12] Return the correct size buffer in set_value() Benjamin Marzinski
@ 2014-07-01 18:47 ` Christophe Varoqui
0 siblings, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-01 18:47 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 1837 bytes --]
Applied.
Thanks.
On Mon, Jun 30, 2014 at 7:13 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> When multipath was mallocing the buffer in set_value, it was using
> sizeof(char *), instead of sizeof(char), so it was allocating a buffer
> big enough for an array of pointers instead of characters.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/parser.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libmultipath/parser.c b/libmultipath/parser.c
> index 526c45b..0d4c870 100644
> --- a/libmultipath/parser.c
> +++ b/libmultipath/parser.c
> @@ -408,11 +408,11 @@ set_value(vector strvec)
> len += strlen(str);
> if (!alloc)
> alloc =
> - (char *) MALLOC(sizeof (char *) *
> + (char *) MALLOC(sizeof (char) *
> (len + 1));
> else {
> alloc =
> - REALLOC(alloc, sizeof (char *) * (len
> + 1));
> + REALLOC(alloc, sizeof (char) * (len +
> 1));
> tmp = VECTOR_SLOT(strvec, i-1);
> if (alloc && *str != '"' && *tmp != '"')
> strncat(alloc, " ", 1);
> @@ -422,7 +422,7 @@ set_value(vector strvec)
> strncat(alloc, str, strlen(str));
> }
> } else {
> - alloc = MALLOC(sizeof (char *) * (size + 1));
> + alloc = MALLOC(sizeof (char) * (size + 1));
> if (alloc)
> memcpy(alloc, str, size);
> }
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 2706 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 03/12] enable gcc format-security check
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
2014-06-30 5:13 ` [PATCH 01/12] Fix memory issues in path reordering failure code paths Benjamin Marzinski
2014-06-30 5:13 ` [PATCH 02/12] Return the correct size buffer in set_value() Benjamin Marzinski
@ 2014-06-30 5:13 ` Benjamin Marzinski
2014-07-01 18:48 ` Christophe Varoqui
2014-07-04 6:18 ` Hannes Reinecke
2014-06-30 5:13 ` [PATCH 04/12] change conf->dry_run to conf->cmd Benjamin Marzinski
` (8 subsequent siblings)
11 siblings, 2 replies; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:13 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
-Wformat-security warns about format-strigs that represent possible
security problems. This is check is now enabled for fedora builds, and it
seems like a reasonable thing to always be checking.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
Makefile.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile.inc b/Makefile.inc
index 0669d32..1486721 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -46,7 +46,7 @@ GZIP = gzip -9 -c
INSTALL_PROGRAM = install
ifndef RPM_OPT_FLAGS
- RPM_OPT_FLAGS = -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
+ RPM_OPT_FLAGS = -O2 -g -pipe -Wformat-security -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4
endif
OPTFLAGS = $(RPM_OPT_FLAGS) -Wunused -Wstrict-prototypes
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 03/12] enable gcc format-security check
2014-06-30 5:13 ` [PATCH 03/12] enable gcc format-security check Benjamin Marzinski
@ 2014-07-01 18:48 ` Christophe Varoqui
2014-07-04 6:18 ` Hannes Reinecke
1 sibling, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-01 18:48 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 1037 bytes --]
Applied.
Thanks.
On Mon, Jun 30, 2014 at 7:13 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> -Wformat-security warns about format-strigs that represent possible
> security problems. This is check is now enabled for fedora builds, and it
> seems like a reasonable thing to always be checking.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> Makefile.inc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile.inc b/Makefile.inc
> index 0669d32..1486721 100644
> --- a/Makefile.inc
> +++ b/Makefile.inc
> @@ -46,7 +46,7 @@ GZIP = gzip -9 -c
> INSTALL_PROGRAM = install
>
> ifndef RPM_OPT_FLAGS
> - RPM_OPT_FLAGS = -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2
> -fexceptions -fstack-protector --param=ssp-buffer-size=4
> + RPM_OPT_FLAGS = -O2 -g -pipe -Wformat-security -Wall
> -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong
> --param=ssp-buffer-size=4
> endif
>
> OPTFLAGS = $(RPM_OPT_FLAGS) -Wunused -Wstrict-prototypes
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 1549 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 03/12] enable gcc format-security check
2014-06-30 5:13 ` [PATCH 03/12] enable gcc format-security check Benjamin Marzinski
2014-07-01 18:48 ` Christophe Varoqui
@ 2014-07-04 6:18 ` Hannes Reinecke
1 sibling, 0 replies; 31+ messages in thread
From: Hannes Reinecke @ 2014-07-04 6:18 UTC (permalink / raw)
To: dm-devel
On 06/30/2014 07:13 AM, Benjamin Marzinski wrote:
> -Wformat-security warns about format-strigs that represent possible
> security problems. This is check is now enabled for fedora builds, and it
> seems like a reasonable thing to always be checking.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> Makefile.inc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile.inc b/Makefile.inc
> index 0669d32..1486721 100644
> --- a/Makefile.inc
> +++ b/Makefile.inc
> @@ -46,7 +46,7 @@ GZIP = gzip -9 -c
> INSTALL_PROGRAM = install
>
> ifndef RPM_OPT_FLAGS
> - RPM_OPT_FLAGS = -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
> + RPM_OPT_FLAGS = -O2 -g -pipe -Wformat-security -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4
> endif
>
> OPTFLAGS = $(RPM_OPT_FLAGS) -Wunused -Wstrict-prototypes
>
'-fstack-protector-strong' is not recognized on any of my gcc
versions supplied by SUSE. Can we please revert it to the original
'-fstack-protector'?
'-Wformat-security' is okay, though.
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)
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 04/12] change conf->dry_run to conf->cmd
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
` (2 preceding siblings ...)
2014-06-30 5:13 ` [PATCH 03/12] enable gcc format-security check Benjamin Marzinski
@ 2014-06-30 5:13 ` Benjamin Marzinski
2014-07-01 18:56 ` Christophe Varoqui
2014-06-30 5:13 ` [PATCH 05/12] allow users to add wwids to /etc/multipath/wwids with -a Benjamin Marzinski
` (7 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:13 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
conf->dry_run is getting used for many different things besides running
multipath in dry_run mode, and the name and values were getting
increasingly confusing. This patch switches the variable from
conf->dry_run to conf->cmd, and folds in conf->list. It also changes
the value to a enum.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/config.h | 13 +++++++++++--
libmultipath/configure.c | 2 +-
libmultipath/discovery.c | 6 ++++--
multipath/main.c | 49 ++++++++++++++++++++++++------------------------
4 files changed, 41 insertions(+), 29 deletions(-)
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 445525b..eb23820 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -22,6 +22,16 @@ enum devtypes {
DEV_DEVMAP
};
+enum mpath_cmds {
+ CMD_CREATE,
+ CMD_DRY_RUN,
+ CMD_LIST_SHORT,
+ CMD_LIST_LONG,
+ CMD_VALID_PATH,
+ CMD_REMOVE_WWID,
+ CMD_RESET_WWIDS,
+};
+
struct hwentry {
char * vendor;
char * product;
@@ -78,8 +88,7 @@ struct mpentry {
struct config {
int verbosity;
- int dry_run;
- int list;
+ enum mpath_cmds cmd;
int pgpolicy_flag;
int pgpolicy;
enum devtypes dev_type;
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 6ad7a80..107517b 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -577,7 +577,7 @@ domap (struct multipath * mpp, char * params)
/*
* last chance to quit before touching the devmaps
*/
- if (conf->dry_run && mpp->action != ACT_NOTHING) {
+ if (conf->cmd == CMD_DRY_RUN && mpp->action != ACT_NOTHING) {
print_multipath_topology(mpp, conf->verbosity);
return DOMAP_DRY;
}
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 786e1de..b62a59c 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -56,7 +56,8 @@ store_pathinfo (vector pathvec, vector hwtable, struct udev_device *udevice,
}
pp->udev = udev_device_ref(udevice);
err = pathinfo(pp, hwtable,
- (conf->dry_run == 3)? flag : (flag | DI_BLACKLIST));
+ (conf->cmd == CMD_REMOVE_WWID)? flag :
+ (flag | DI_BLACKLIST));
if (err)
goto out;
@@ -1127,7 +1128,8 @@ get_uid (struct path * pp)
value = udev_device_get_property_value(pp->udev,
pp->uid_attribute);
- if ((!value || strlen(value) == 0) && conf->dry_run == 2)
+ if ((!value || strlen(value) == 0) &&
+ conf->cmd == CMD_VALID_PATH)
value = getenv(pp->uid_attribute);
if (value && strlen(value)) {
size_t len = WWID_SIZE;
diff --git a/multipath/main.c b/multipath/main.c
index 64c8fc5..54b2a74 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -207,18 +207,19 @@ get_dm_mpvec (vector curmp, vector pathvec, char * refwwid)
* If not in "fast list mode", we need to fetch information
* about them
*/
- if (conf->list != 1)
+ if (conf->cmd != CMD_LIST_SHORT)
update_paths(mpp);
- if (conf->list > 1)
+ if (conf->cmd == CMD_LIST_LONG)
mpp->bestpg = select_path_group(mpp);
disassemble_status(status, mpp);
- if (conf->list)
+ if (conf->cmd == CMD_LIST_SHORT ||
+ conf->cmd == CMD_LIST_LONG)
print_multipath_topology(mpp, conf->verbosity);
- if (!conf->dry_run)
+ if (conf->cmd == CMD_CREATE)
reinstate_paths(mpp);
}
return 0;
@@ -260,10 +261,11 @@ configure (void)
/*
* if we have a blacklisted device parameter, exit early
*/
- if (dev && conf->dev_type == DEV_DEVNODE && conf->dry_run != 3 &&
+ if (dev && conf->dev_type == DEV_DEVNODE &&
+ conf->cmd != CMD_REMOVE_WWID &&
(filter_devnode(conf->blist_devnode,
conf->elist_devnode, dev) > 0)) {
- if (conf->dry_run == 2)
+ if (conf->cmd == CMD_VALID_PATH)
printf("%s is not a valid multipath device path\n",
conf->dev);
goto out;
@@ -276,13 +278,13 @@ configure (void)
int failed = get_refwwid(conf->dev, conf->dev_type, pathvec,
&refwwid);
if (!refwwid) {
- if (failed == 2 && conf->dry_run == 2)
+ if (failed == 2 && conf->cmd == CMD_VALID_PATH)
printf("%s is not a valid multipath device path\n", conf->dev);
else
condlog(3, "scope is nul");
goto out;
}
- if (conf->dry_run == 3) {
+ if (conf->cmd == CMD_REMOVE_WWID) {
r = remove_wwid(refwwid);
if (r == 0)
printf("wwid '%s' removed\n", refwwid);
@@ -294,7 +296,7 @@ configure (void)
goto out;
}
condlog(3, "scope limited to %s", refwwid);
- if (conf->dry_run == 2) {
+ if (conf->cmd == CMD_VALID_PATH) {
if (check_wwids_file(refwwid, 0) == 0){
printf("%s is a valid multipath device path\n", conf->dev);
r = 0;
@@ -311,10 +313,10 @@ configure (void)
if (conf->dev)
di_flag = DI_WWID;
- if (conf->list > 1)
+ if (conf->cmd == CMD_LIST_LONG)
/* extended path info '-ll' */
di_flag |= DI_SYSFS | DI_CHECKER;
- else if (conf->list)
+ else if (conf->cmd == CMD_LIST_SHORT)
/* minimum path info '-l' */
di_flag |= DI_SYSFS;
else
@@ -334,7 +336,7 @@ configure (void)
filter_pathvec(pathvec, refwwid);
- if (conf->list) {
+ if (conf->cmd != CMD_CREATE && conf->cmd != CMD_DRY_RUN) {
r = 0;
goto out;
}
@@ -456,11 +458,11 @@ main (int argc, char *argv[])
conf->allow_queueing = 1;
break;
case 'c':
- conf->dry_run = 2;
+ conf->cmd = CMD_VALID_PATH;
break;
case 'd':
- if (!conf->dry_run)
- conf->dry_run = 1;
+ if (conf->cmd == CMD_CREATE)
+ conf->cmd = CMD_DRY_RUN;
break;
case 'f':
conf->remove = FLUSH_ONE;
@@ -469,11 +471,10 @@ main (int argc, char *argv[])
conf->remove = FLUSH_ALL;
break;
case 'l':
- conf->list = 1;
- conf->dry_run = 1;
-
if (optarg && !strncmp(optarg, "l", 1))
- conf->list++;
+ conf->cmd = CMD_LIST_LONG;
+ else
+ conf->cmd = CMD_LIST_SHORT;
break;
case 'M':
@@ -499,10 +500,10 @@ main (int argc, char *argv[])
usage(argv[0]);
exit(0);
case 'w':
- conf->dry_run = 3;
+ conf->cmd = CMD_REMOVE_WWID;
break;
case 'W':
- conf->dry_run = 4;
+ conf->cmd = CMD_RESET_WWIDS;
break;
case ':':
fprintf(stderr, "Missing option argument\n");
@@ -558,16 +559,16 @@ main (int argc, char *argv[])
}
dm_init();
- if (conf->dry_run == 2 &&
+ if (conf->cmd == CMD_VALID_PATH &&
(!conf->dev || conf->dev_type == DEV_DEVMAP)) {
condlog(0, "the -c option requires a path to check");
goto out;
}
- if (conf->dry_run == 3 && !conf->dev) {
+ if (conf->cmd == CMD_REMOVE_WWID && !conf->dev) {
condlog(0, "the -w option requires a device");
goto out;
}
- if (conf->dry_run == 4) {
+ if (conf->cmd == CMD_RESET_WWIDS) {
struct multipath * mpp;
int i;
vector curmp;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 04/12] change conf->dry_run to conf->cmd
2014-06-30 5:13 ` [PATCH 04/12] change conf->dry_run to conf->cmd Benjamin Marzinski
@ 2014-07-01 18:56 ` Christophe Varoqui
0 siblings, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-01 18:56 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 9571 bytes --]
Applied.
Nice readability improvement indeed.
Thanks.
On Mon, Jun 30, 2014 at 7:13 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> conf->dry_run is getting used for many different things besides running
> multipath in dry_run mode, and the name and values were getting
> increasingly confusing. This patch switches the variable from
> conf->dry_run to conf->cmd, and folds in conf->list. It also changes
> the value to a enum.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/config.h | 13 +++++++++++--
> libmultipath/configure.c | 2 +-
> libmultipath/discovery.c | 6 ++++--
> multipath/main.c | 49
> ++++++++++++++++++++++++------------------------
> 4 files changed, 41 insertions(+), 29 deletions(-)
>
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 445525b..eb23820 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -22,6 +22,16 @@ enum devtypes {
> DEV_DEVMAP
> };
>
> +enum mpath_cmds {
> + CMD_CREATE,
> + CMD_DRY_RUN,
> + CMD_LIST_SHORT,
> + CMD_LIST_LONG,
> + CMD_VALID_PATH,
> + CMD_REMOVE_WWID,
> + CMD_RESET_WWIDS,
> +};
> +
> struct hwentry {
> char * vendor;
> char * product;
> @@ -78,8 +88,7 @@ struct mpentry {
>
> struct config {
> int verbosity;
> - int dry_run;
> - int list;
> + enum mpath_cmds cmd;
> int pgpolicy_flag;
> int pgpolicy;
> enum devtypes dev_type;
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 6ad7a80..107517b 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -577,7 +577,7 @@ domap (struct multipath * mpp, char * params)
> /*
> * last chance to quit before touching the devmaps
> */
> - if (conf->dry_run && mpp->action != ACT_NOTHING) {
> + if (conf->cmd == CMD_DRY_RUN && mpp->action != ACT_NOTHING) {
> print_multipath_topology(mpp, conf->verbosity);
> return DOMAP_DRY;
> }
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 786e1de..b62a59c 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -56,7 +56,8 @@ store_pathinfo (vector pathvec, vector hwtable, struct
> udev_device *udevice,
> }
> pp->udev = udev_device_ref(udevice);
> err = pathinfo(pp, hwtable,
> - (conf->dry_run == 3)? flag : (flag | DI_BLACKLIST));
> + (conf->cmd == CMD_REMOVE_WWID)? flag :
> + (flag |
> DI_BLACKLIST));
> if (err)
> goto out;
>
> @@ -1127,7 +1128,8 @@ get_uid (struct path * pp)
>
> value = udev_device_get_property_value(pp->udev,
> pp->uid_attribute);
> - if ((!value || strlen(value) == 0) && conf->dry_run == 2)
> + if ((!value || strlen(value) == 0) &&
> + conf->cmd == CMD_VALID_PATH)
> value = getenv(pp->uid_attribute);
> if (value && strlen(value)) {
> size_t len = WWID_SIZE;
> diff --git a/multipath/main.c b/multipath/main.c
> index 64c8fc5..54b2a74 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -207,18 +207,19 @@ get_dm_mpvec (vector curmp, vector pathvec, char *
> refwwid)
> * If not in "fast list mode", we need to fetch information
> * about them
> */
> - if (conf->list != 1)
> + if (conf->cmd != CMD_LIST_SHORT)
> update_paths(mpp);
>
> - if (conf->list > 1)
> + if (conf->cmd == CMD_LIST_LONG)
> mpp->bestpg = select_path_group(mpp);
>
> disassemble_status(status, mpp);
>
> - if (conf->list)
> + if (conf->cmd == CMD_LIST_SHORT ||
> + conf->cmd == CMD_LIST_LONG)
> print_multipath_topology(mpp, conf->verbosity);
>
> - if (!conf->dry_run)
> + if (conf->cmd == CMD_CREATE)
> reinstate_paths(mpp);
> }
> return 0;
> @@ -260,10 +261,11 @@ configure (void)
> /*
> * if we have a blacklisted device parameter, exit early
> */
> - if (dev && conf->dev_type == DEV_DEVNODE && conf->dry_run != 3 &&
> + if (dev && conf->dev_type == DEV_DEVNODE &&
> + conf->cmd != CMD_REMOVE_WWID &&
> (filter_devnode(conf->blist_devnode,
> conf->elist_devnode, dev) > 0)) {
> - if (conf->dry_run == 2)
> + if (conf->cmd == CMD_VALID_PATH)
> printf("%s is not a valid multipath device path\n",
> conf->dev);
> goto out;
> @@ -276,13 +278,13 @@ configure (void)
> int failed = get_refwwid(conf->dev, conf->dev_type,
> pathvec,
> &refwwid);
> if (!refwwid) {
> - if (failed == 2 && conf->dry_run == 2)
> + if (failed == 2 && conf->cmd == CMD_VALID_PATH)
> printf("%s is not a valid multipath device
> path\n", conf->dev);
> else
> condlog(3, "scope is nul");
> goto out;
> }
> - if (conf->dry_run == 3) {
> + if (conf->cmd == CMD_REMOVE_WWID) {
> r = remove_wwid(refwwid);
> if (r == 0)
> printf("wwid '%s' removed\n", refwwid);
> @@ -294,7 +296,7 @@ configure (void)
> goto out;
> }
> condlog(3, "scope limited to %s", refwwid);
> - if (conf->dry_run == 2) {
> + if (conf->cmd == CMD_VALID_PATH) {
> if (check_wwids_file(refwwid, 0) == 0){
> printf("%s is a valid multipath device
> path\n", conf->dev);
> r = 0;
> @@ -311,10 +313,10 @@ configure (void)
> if (conf->dev)
> di_flag = DI_WWID;
>
> - if (conf->list > 1)
> + if (conf->cmd == CMD_LIST_LONG)
> /* extended path info '-ll' */
> di_flag |= DI_SYSFS | DI_CHECKER;
> - else if (conf->list)
> + else if (conf->cmd == CMD_LIST_SHORT)
> /* minimum path info '-l' */
> di_flag |= DI_SYSFS;
> else
> @@ -334,7 +336,7 @@ configure (void)
>
> filter_pathvec(pathvec, refwwid);
>
> - if (conf->list) {
> + if (conf->cmd != CMD_CREATE && conf->cmd != CMD_DRY_RUN) {
> r = 0;
> goto out;
> }
> @@ -456,11 +458,11 @@ main (int argc, char *argv[])
> conf->allow_queueing = 1;
> break;
> case 'c':
> - conf->dry_run = 2;
> + conf->cmd = CMD_VALID_PATH;
> break;
> case 'd':
> - if (!conf->dry_run)
> - conf->dry_run = 1;
> + if (conf->cmd == CMD_CREATE)
> + conf->cmd = CMD_DRY_RUN;
> break;
> case 'f':
> conf->remove = FLUSH_ONE;
> @@ -469,11 +471,10 @@ main (int argc, char *argv[])
> conf->remove = FLUSH_ALL;
> break;
> case 'l':
> - conf->list = 1;
> - conf->dry_run = 1;
> -
> if (optarg && !strncmp(optarg, "l", 1))
> - conf->list++;
> + conf->cmd = CMD_LIST_LONG;
> + else
> + conf->cmd = CMD_LIST_SHORT;
>
> break;
> case 'M':
> @@ -499,10 +500,10 @@ main (int argc, char *argv[])
> usage(argv[0]);
> exit(0);
> case 'w':
> - conf->dry_run = 3;
> + conf->cmd = CMD_REMOVE_WWID;
> break;
> case 'W':
> - conf->dry_run = 4;
> + conf->cmd = CMD_RESET_WWIDS;
> break;
> case ':':
> fprintf(stderr, "Missing option argument\n");
> @@ -558,16 +559,16 @@ main (int argc, char *argv[])
> }
> dm_init();
>
> - if (conf->dry_run == 2 &&
> + if (conf->cmd == CMD_VALID_PATH &&
> (!conf->dev || conf->dev_type == DEV_DEVMAP)) {
> condlog(0, "the -c option requires a path to check");
> goto out;
> }
> - if (conf->dry_run == 3 && !conf->dev) {
> + if (conf->cmd == CMD_REMOVE_WWID && !conf->dev) {
> condlog(0, "the -w option requires a device");
> goto out;
> }
> - if (conf->dry_run == 4) {
> + if (conf->cmd == CMD_RESET_WWIDS) {
> struct multipath * mpp;
> int i;
> vector curmp;
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 12737 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 05/12] allow users to add wwids to /etc/multipath/wwids with -a
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
` (3 preceding siblings ...)
2014-06-30 5:13 ` [PATCH 04/12] change conf->dry_run to conf->cmd Benjamin Marzinski
@ 2014-06-30 5:13 ` Benjamin Marzinski
2014-07-01 18:58 ` Christophe Varoqui
2014-06-30 5:13 ` [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A Benjamin Marzinski
` (6 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:13 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
The patch adds a "-a" option to multipath, that allows it to add wwids
to the /etc/multipath wwids file.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/config.h | 1 +
multipath/main.c | 17 +++++++++++++++--
multipath/multipath.8 | 5 ++++-
3 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/libmultipath/config.h b/libmultipath/config.h
index eb23820..ac7c58e 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -30,6 +30,7 @@ enum mpath_cmds {
CMD_VALID_PATH,
CMD_REMOVE_WWID,
CMD_RESET_WWIDS,
+ CMD_ADD_WWID,
};
struct hwentry {
diff --git a/multipath/main.c b/multipath/main.c
index 54b2a74..157475e 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -84,7 +84,7 @@ usage (char * progname)
{
fprintf (stderr, VERSION_STRING);
fprintf (stderr, "Usage:\n");
- fprintf (stderr, " %s [-c|-w|-W] [-d] [-r] [-v lvl] [-p pol] [-b fil] [-q] [dev]\n", progname);
+ fprintf (stderr, " %s [-a|-c|-w|-W] [-d] [-r] [-v lvl] [-p pol] [-b fil] [-q] [dev]\n", progname);
fprintf (stderr, " %s -l|-ll|-f [-v lvl] [-b fil] [dev]\n", progname);
fprintf (stderr, " %s -F [-v lvl]\n", progname);
fprintf (stderr, " %s -t\n", progname);
@@ -97,6 +97,7 @@ usage (char * progname)
" -ll show multipath topology (maximum info)\n" \
" -f flush a multipath device map\n" \
" -F flush all multipath device maps\n" \
+ " -a add a device wwid to the wwids file\n" \
" -c check if a device should be a path in a multipath device\n" \
" -q allow queue_if_no_path when multipathd is not running\n"\
" -d dry run, do not create or update devmaps\n" \
@@ -295,6 +296,15 @@ configure (void)
}
goto out;
}
+ if (conf->cmd == CMD_ADD_WWID) {
+ r = remember_wwid(refwwid);
+ if (r == 0)
+ printf("wwid '%s' added\n", refwwid);
+ else
+ printf("failed adding '%s' to wwids file\n",
+ refwwid);
+ goto out;
+ }
condlog(3, "scope limited to %s", refwwid);
if (conf->cmd == CMD_VALID_PATH) {
if (check_wwids_file(refwwid, 0) == 0){
@@ -435,7 +445,7 @@ main (int argc, char *argv[])
if (load_config(DEFAULT_CONFIGFILE, udev))
exit(1);
- while ((arg = getopt(argc, argv, ":dchl::FfM:v:p:b:BrtqwW")) != EOF ) {
+ while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BrtqwW")) != EOF ) {
switch(arg) {
case 1: printf("optarg : %s\n",optarg);
break;
@@ -505,6 +515,9 @@ main (int argc, char *argv[])
case 'W':
conf->cmd = CMD_RESET_WWIDS;
break;
+ case 'a':
+ conf->cmd = CMD_ADD_WWID;
+ break;
case ':':
fprintf(stderr, "Missing option argument\n");
usage(argv[0]);
diff --git a/multipath/multipath.8 b/multipath/multipath.8
index a2262ac..b6479b1 100644
--- a/multipath/multipath.8
+++ b/multipath/multipath.8
@@ -8,7 +8,7 @@ multipath \- Device mapper target autoconfig
.RB [\| \-b\ \c
.IR bindings_file \|]
.RB [\| \-d \|]
-.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-q | \|-r | \-w | \-W \|]
+.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-q | \|-r | \-a | \-w | \-W \|]
.RB [\| \-p\ \c
.BR failover | multibus | group_by_serial | group_by_prio | group_by_node_name \|]
.RB [\| device \|]
@@ -68,6 +68,9 @@ check if a block device should be a path in a multipath device
.B \-q
allow device tables with queue_if_no_path when multipathd is not running
.TP
+.B \-a
+add the wwid for the specified device to the wwids file
+.TP
.B \-w
remove the wwid for the specified device from the wwids file
.TP
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 05/12] allow users to add wwids to /etc/multipath/wwids with -a
2014-06-30 5:13 ` [PATCH 05/12] allow users to add wwids to /etc/multipath/wwids with -a Benjamin Marzinski
@ 2014-07-01 18:58 ` Christophe Varoqui
0 siblings, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-01 18:58 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 4559 bytes --]
Applied.
Thanks.
On Mon, Jun 30, 2014 at 7:13 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> The patch adds a "-a" option to multipath, that allows it to add wwids
> to the /etc/multipath wwids file.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/config.h | 1 +
> multipath/main.c | 17 +++++++++++++++--
> multipath/multipath.8 | 5 ++++-
> 3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index eb23820..ac7c58e 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -30,6 +30,7 @@ enum mpath_cmds {
> CMD_VALID_PATH,
> CMD_REMOVE_WWID,
> CMD_RESET_WWIDS,
> + CMD_ADD_WWID,
> };
>
> struct hwentry {
> diff --git a/multipath/main.c b/multipath/main.c
> index 54b2a74..157475e 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -84,7 +84,7 @@ usage (char * progname)
> {
> fprintf (stderr, VERSION_STRING);
> fprintf (stderr, "Usage:\n");
> - fprintf (stderr, " %s [-c|-w|-W] [-d] [-r] [-v lvl] [-p pol] [-b
> fil] [-q] [dev]\n", progname);
> + fprintf (stderr, " %s [-a|-c|-w|-W] [-d] [-r] [-v lvl] [-p pol]
> [-b fil] [-q] [dev]\n", progname);
> fprintf (stderr, " %s -l|-ll|-f [-v lvl] [-b fil] [dev]\n",
> progname);
> fprintf (stderr, " %s -F [-v lvl]\n", progname);
> fprintf (stderr, " %s -t\n", progname);
> @@ -97,6 +97,7 @@ usage (char * progname)
> " -ll show multipath topology (maximum info)\n" \
> " -f flush a multipath device map\n" \
> " -F flush all multipath device maps\n" \
> + " -a add a device wwid to the wwids file\n" \
> " -c check if a device should be a path in a
> multipath device\n" \
> " -q allow queue_if_no_path when multipathd is not
> running\n"\
> " -d dry run, do not create or update devmaps\n" \
> @@ -295,6 +296,15 @@ configure (void)
> }
> goto out;
> }
> + if (conf->cmd == CMD_ADD_WWID) {
> + r = remember_wwid(refwwid);
> + if (r == 0)
> + printf("wwid '%s' added\n", refwwid);
> + else
> + printf("failed adding '%s' to wwids
> file\n",
> + refwwid);
> + goto out;
> + }
> condlog(3, "scope limited to %s", refwwid);
> if (conf->cmd == CMD_VALID_PATH) {
> if (check_wwids_file(refwwid, 0) == 0){
> @@ -435,7 +445,7 @@ main (int argc, char *argv[])
> if (load_config(DEFAULT_CONFIGFILE, udev))
> exit(1);
>
> - while ((arg = getopt(argc, argv, ":dchl::FfM:v:p:b:BrtqwW")) !=
> EOF ) {
> + while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BrtqwW")) !=
> EOF ) {
> switch(arg) {
> case 1: printf("optarg : %s\n",optarg);
> break;
> @@ -505,6 +515,9 @@ main (int argc, char *argv[])
> case 'W':
> conf->cmd = CMD_RESET_WWIDS;
> break;
> + case 'a':
> + conf->cmd = CMD_ADD_WWID;
> + break;
> case ':':
> fprintf(stderr, "Missing option argument\n");
> usage(argv[0]);
> diff --git a/multipath/multipath.8 b/multipath/multipath.8
> index a2262ac..b6479b1 100644
> --- a/multipath/multipath.8
> +++ b/multipath/multipath.8
> @@ -8,7 +8,7 @@ multipath \- Device mapper target autoconfig
> .RB [\| \-b\ \c
> .IR bindings_file \|]
> .RB [\| \-d \|]
> -.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-q | \|-r | \-w
> | \-W \|]
> +.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-q | \|-r | \-a
> | \-w | \-W \|]
> .RB [\| \-p\ \c
> .BR failover | multibus | group_by_serial | group_by_prio |
> group_by_node_name \|]
> .RB [\| device \|]
> @@ -68,6 +68,9 @@ check if a block device should be a path in a multipath
> device
> .B \-q
> allow device tables with queue_if_no_path when multipathd is not running
> .TP
> +.B \-a
> +add the wwid for the specified device to the wwids file
> +.TP
> .B \-w
> remove the wwid for the specified device from the wwids file
> .TP
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 5991 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
` (4 preceding siblings ...)
2014-06-30 5:13 ` [PATCH 05/12] allow users to add wwids to /etc/multipath/wwids with -a Benjamin Marzinski
@ 2014-06-30 5:13 ` Benjamin Marzinski
2014-07-01 19:22 ` Christophe Varoqui
2014-06-30 5:13 ` [PATCH 07/12] Make multipathd orphan paths that were removed externally Benjamin Marzinski
` (5 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:13 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
This patch adds another option to multipath, "-A", which reads
/proc/cmdline for mpath.wwid=<WWID> options, and adds any wwids it finds to
/etc/multipath/wwids. While this isn't usually important during normal
operation, since these wwids should already be added, it can be helpful
during installation, to make sure that multipath can claim devices as its
own, before LVM or something else makes use of them. The patch also execs
"/sbin/multipath -A" before running multipathd in multipathd.service
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/wwids.c | 44 +++++++++++++++++++++++++++++++++++++++++++
libmultipath/wwids.h | 1 +
multipath/main.c | 10 ++++++++--
multipath/multipath.8 | 5 ++++-
multipathd/multipathd.service | 1 +
5 files changed, 58 insertions(+), 3 deletions(-)
diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index eca1799..1dc00bb 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -274,3 +274,47 @@ remember_wwid(char *wwid)
condlog(4, "wwid %s already in wwids file", wwid);
return 0;
}
+
+int remember_cmdline_wwid(void)
+{
+ FILE *f = NULL;
+ char buf[LINE_MAX], *next, *ptr;
+ int ret = 0;
+
+ f = fopen("/proc/cmdline", "re");
+ if (!f) {
+ condlog(0, "can't open /proc/cmdline : %s", strerror(errno));
+ return -1;
+ }
+
+ if (!fgets(buf, sizeof(buf), f)) {
+ if (ferror(f))
+ condlog(0, "read of /proc/cmdline failed : %s",
+ strerror(errno));
+ else
+ condlog(0, "couldn't read /proc/cmdline");
+ fclose(f);
+ return -1;
+ }
+ fclose(f);
+ next = buf;
+ while((ptr = strstr(next, "mpath.wwid="))) {
+ ptr += 11;
+ next = strpbrk(ptr, " \t\n");
+ if (next) {
+ *next = '\0';
+ next++;
+ }
+ if (strlen(ptr)) {
+ if (remember_wwid(ptr) != 0)
+ ret = -1;
+ }
+ else {
+ condlog(0, "empty mpath.wwid kernel command line option");
+ ret = -1;
+ }
+ if (!next)
+ break;
+ }
+ return ret;
+}
diff --git a/libmultipath/wwids.h b/libmultipath/wwids.h
index f3b21fa..2fbd419 100644
--- a/libmultipath/wwids.h
+++ b/libmultipath/wwids.h
@@ -16,5 +16,6 @@ int remember_wwid(char *wwid);
int check_wwids_file(char *wwid, int write_wwid);
int remove_wwid(char *wwid);
int replace_wwids(vector mp);
+int remember_cmdline_wwid(void);
#endif /* _WWIDS_H */
diff --git a/multipath/main.c b/multipath/main.c
index 157475e..ed93f66 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -84,7 +84,7 @@ usage (char * progname)
{
fprintf (stderr, VERSION_STRING);
fprintf (stderr, "Usage:\n");
- fprintf (stderr, " %s [-a|-c|-w|-W] [-d] [-r] [-v lvl] [-p pol] [-b fil] [-q] [dev]\n", progname);
+ fprintf (stderr, " %s [-a|-A|-c|-w|-W] [-d] [-r] [-v lvl] [-p pol] [-b fil] [-q] [dev]\n", progname);
fprintf (stderr, " %s -l|-ll|-f [-v lvl] [-b fil] [dev]\n", progname);
fprintf (stderr, " %s -F [-v lvl]\n", progname);
fprintf (stderr, " %s -t\n", progname);
@@ -98,6 +98,8 @@ usage (char * progname)
" -f flush a multipath device map\n" \
" -F flush all multipath device maps\n" \
" -a add a device wwid to the wwids file\n" \
+ " -A add devices from kernel command line mpath.wwids\n"
+ " parameters to wwids file\n" \
" -c check if a device should be a path in a multipath device\n" \
" -q allow queue_if_no_path when multipathd is not running\n"\
" -d dry run, do not create or update devmaps\n" \
@@ -445,7 +447,7 @@ main (int argc, char *argv[])
if (load_config(DEFAULT_CONFIGFILE, udev))
exit(1);
- while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BrtqwW")) != EOF ) {
+ while ((arg = getopt(argc, argv, ":aAdchl::FfM:v:p:b:BrtqwW")) != EOF ) {
switch(arg) {
case 1: printf("optarg : %s\n",optarg);
break;
@@ -518,6 +520,10 @@ main (int argc, char *argv[])
case 'a':
conf->cmd = CMD_ADD_WWID;
break;
+ case 'A':
+ if (remember_cmdline_wwid() != 0)
+ exit(1);
+ exit(0);
case ':':
fprintf(stderr, "Missing option argument\n");
usage(argv[0]);
diff --git a/multipath/multipath.8 b/multipath/multipath.8
index b6479b1..f6b30c7 100644
--- a/multipath/multipath.8
+++ b/multipath/multipath.8
@@ -8,7 +8,7 @@ multipath \- Device mapper target autoconfig
.RB [\| \-b\ \c
.IR bindings_file \|]
.RB [\| \-d \|]
-.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-q | \|-r | \-a | \-w | \-W \|]
+.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-q | \|-r | \-a | \-A | \-w | \-W \|]
.RB [\| \-p\ \c
.BR failover | multibus | group_by_serial | group_by_prio | group_by_node_name \|]
.RB [\| device \|]
@@ -71,6 +71,9 @@ allow device tables with queue_if_no_path when multipathd is not running
.B \-a
add the wwid for the specified device to the wwids file
.TP
+.B \-A
+add wwids from any kernel command line mpath.wwid parameters to the wwids file
+.TP
.B \-w
remove the wwid for the specified device from the wwids file
.TP
diff --git a/multipathd/multipathd.service b/multipathd/multipathd.service
index be3ba3f..8bb48b5 100644
--- a/multipathd/multipathd.service
+++ b/multipathd/multipathd.service
@@ -10,6 +10,7 @@ Type=notify
NotifyAccess=main
LimitCORE=infinity
ExecStartPre=/sbin/modprobe dm-multipath
+ExecStartPre=-/sbin/multipath -A
ExecStart=/sbin/multipathd -d -s
ExecReload=/sbin/multipathd reconfigure
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A
2014-06-30 5:13 ` [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A Benjamin Marzinski
@ 2014-07-01 19:22 ` Christophe Varoqui
2014-07-02 6:03 ` Hannes Reinecke
0 siblings, 1 reply; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-01 19:22 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 6838 bytes --]
Hannes,
would you Ack this one, or do you have some other idea for this in your
tree ?
Best regards,
Christophe Varoqui
www.opensvc.com
On Mon, Jun 30, 2014 at 7:13 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> This patch adds another option to multipath, "-A", which reads
> /proc/cmdline for mpath.wwid=<WWID> options, and adds any wwids it finds to
> /etc/multipath/wwids. While this isn't usually important during normal
> operation, since these wwids should already be added, it can be helpful
> during installation, to make sure that multipath can claim devices as its
> own, before LVM or something else makes use of them. The patch also execs
> "/sbin/multipath -A" before running multipathd in multipathd.service
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/wwids.c | 44
> +++++++++++++++++++++++++++++++++++++++++++
> libmultipath/wwids.h | 1 +
> multipath/main.c | 10 ++++++++--
> multipath/multipath.8 | 5 ++++-
> multipathd/multipathd.service | 1 +
> 5 files changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
> index eca1799..1dc00bb 100644
> --- a/libmultipath/wwids.c
> +++ b/libmultipath/wwids.c
> @@ -274,3 +274,47 @@ remember_wwid(char *wwid)
> condlog(4, "wwid %s already in wwids file", wwid);
> return 0;
> }
> +
> +int remember_cmdline_wwid(void)
> +{
> + FILE *f = NULL;
> + char buf[LINE_MAX], *next, *ptr;
> + int ret = 0;
> +
> + f = fopen("/proc/cmdline", "re");
> + if (!f) {
> + condlog(0, "can't open /proc/cmdline : %s",
> strerror(errno));
> + return -1;
> + }
> +
> + if (!fgets(buf, sizeof(buf), f)) {
> + if (ferror(f))
> + condlog(0, "read of /proc/cmdline failed : %s",
> + strerror(errno));
> + else
> + condlog(0, "couldn't read /proc/cmdline");
> + fclose(f);
> + return -1;
> + }
> + fclose(f);
> + next = buf;
> + while((ptr = strstr(next, "mpath.wwid="))) {
> + ptr += 11;
> + next = strpbrk(ptr, " \t\n");
> + if (next) {
> + *next = '\0';
> + next++;
> + }
> + if (strlen(ptr)) {
> + if (remember_wwid(ptr) != 0)
> + ret = -1;
> + }
> + else {
> + condlog(0, "empty mpath.wwid kernel command line
> option");
> + ret = -1;
> + }
> + if (!next)
> + break;
> + }
> + return ret;
> +}
> diff --git a/libmultipath/wwids.h b/libmultipath/wwids.h
> index f3b21fa..2fbd419 100644
> --- a/libmultipath/wwids.h
> +++ b/libmultipath/wwids.h
> @@ -16,5 +16,6 @@ int remember_wwid(char *wwid);
> int check_wwids_file(char *wwid, int write_wwid);
> int remove_wwid(char *wwid);
> int replace_wwids(vector mp);
> +int remember_cmdline_wwid(void);
>
> #endif /* _WWIDS_H */
> diff --git a/multipath/main.c b/multipath/main.c
> index 157475e..ed93f66 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -84,7 +84,7 @@ usage (char * progname)
> {
> fprintf (stderr, VERSION_STRING);
> fprintf (stderr, "Usage:\n");
> - fprintf (stderr, " %s [-a|-c|-w|-W] [-d] [-r] [-v lvl] [-p pol]
> [-b fil] [-q] [dev]\n", progname);
> + fprintf (stderr, " %s [-a|-A|-c|-w|-W] [-d] [-r] [-v lvl] [-p
> pol] [-b fil] [-q] [dev]\n", progname);
> fprintf (stderr, " %s -l|-ll|-f [-v lvl] [-b fil] [dev]\n",
> progname);
> fprintf (stderr, " %s -F [-v lvl]\n", progname);
> fprintf (stderr, " %s -t\n", progname);
> @@ -98,6 +98,8 @@ usage (char * progname)
> " -f flush a multipath device map\n" \
> " -F flush all multipath device maps\n" \
> " -a add a device wwid to the wwids file\n" \
> + " -A add devices from kernel command line
> mpath.wwids\n"
> + " parameters to wwids file\n" \
> " -c check if a device should be a path in a
> multipath device\n" \
> " -q allow queue_if_no_path when multipathd is not
> running\n"\
> " -d dry run, do not create or update devmaps\n" \
> @@ -445,7 +447,7 @@ main (int argc, char *argv[])
> if (load_config(DEFAULT_CONFIGFILE, udev))
> exit(1);
>
> - while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BrtqwW")) !=
> EOF ) {
> + while ((arg = getopt(argc, argv, ":aAdchl::FfM:v:p:b:BrtqwW")) !=
> EOF ) {
> switch(arg) {
> case 1: printf("optarg : %s\n",optarg);
> break;
> @@ -518,6 +520,10 @@ main (int argc, char *argv[])
> case 'a':
> conf->cmd = CMD_ADD_WWID;
> break;
> + case 'A':
> + if (remember_cmdline_wwid() != 0)
> + exit(1);
> + exit(0);
> case ':':
> fprintf(stderr, "Missing option argument\n");
> usage(argv[0]);
> diff --git a/multipath/multipath.8 b/multipath/multipath.8
> index b6479b1..f6b30c7 100644
> --- a/multipath/multipath.8
> +++ b/multipath/multipath.8
> @@ -8,7 +8,7 @@ multipath \- Device mapper target autoconfig
> .RB [\| \-b\ \c
> .IR bindings_file \|]
> .RB [\| \-d \|]
> -.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-q | \|-r | \-a
> | \-w | \-W \|]
> +.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-q | \|-r | \-a
> | \-A | \-w | \-W \|]
> .RB [\| \-p\ \c
> .BR failover | multibus | group_by_serial | group_by_prio |
> group_by_node_name \|]
> .RB [\| device \|]
> @@ -71,6 +71,9 @@ allow device tables with queue_if_no_path when
> multipathd is not running
> .B \-a
> add the wwid for the specified device to the wwids file
> .TP
> +.B \-A
> +add wwids from any kernel command line mpath.wwid parameters to the wwids
> file
> +.TP
> .B \-w
> remove the wwid for the specified device from the wwids file
> .TP
> diff --git a/multipathd/multipathd.service b/multipathd/multipathd.service
> index be3ba3f..8bb48b5 100644
> --- a/multipathd/multipathd.service
> +++ b/multipathd/multipathd.service
> @@ -10,6 +10,7 @@ Type=notify
> NotifyAccess=main
> LimitCORE=infinity
> ExecStartPre=/sbin/modprobe dm-multipath
> +ExecStartPre=-/sbin/multipath -A
> ExecStart=/sbin/multipathd -d -s
> ExecReload=/sbin/multipathd reconfigure
>
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 8750 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A
2014-07-01 19:22 ` Christophe Varoqui
@ 2014-07-02 6:03 ` Hannes Reinecke
2014-07-02 19:48 ` Benjamin Marzinski
0 siblings, 1 reply; 31+ messages in thread
From: Hannes Reinecke @ 2014-07-02 6:03 UTC (permalink / raw)
To: dm-devel
On 07/01/2014 09:22 PM, Christophe Varoqui wrote:
> Hannes,
>
> would you Ack this one, or do you have some other idea for this in
> your tree ?
>
Sigh. The whole multipath / systemd / dracut integration
is _a mess_.
The main problem is that RH and SUSE treat multipath handling
differently.
(From what I can see. I've still a hard time to understand how
multipath booting works with RH. So there might be errors.)
RH is taking a restrictive approach, ie it'll allow only configured
multipath devices during boot. IE it'll accept only devices present
in '/etc/multipath/wwids' for booting. So when coming across a new
wwid multipath won't be setup there, so of course they'll need an
additional parameter for that.
SUSE, OTOH, is taking the permissive approach. When multipath is
included in dracut it'll try to generate multipath devices for _all_
existing devices; the wwid file is not really required here.
And, consequently, the '-A' parameter isn't required, too.
While this is nice and proper, both approaches have issues:
- From what I've seen RH is building a 'generic' initrd, and
configures them via the kernel or dracut commandline.
Which makes it a bit hard for multipathing as the wwid
most certainly cannot be part of /etc/multipath/wwids.
But I guess this is what should be fixed by this patch.
- SUSE is building a 'per-host' initrd, ie it'll generate
an initrd for that specific installation.
So there isn't actually a _need_ for the permissive approach,
as chances are it'll never come across anything else
_but_ the configured device.
Plus I haven't really evaluated whether the permissive
approach actually works properly, ie that multipath will
try to create device-mapper devices for unknown wwids.
But back to the patch.
I must say I'm not really in favour of this.
Implementing kernel commandline parsing in the _daemon_ is just
downright evil.
It would be _far_ more sensible to have it implemented in dracut
as a commandline hook, which just adds the wwid from the kernel
commandline to /etc/multipath/wwids.
That's a simple shell script with no magic involved.
Then the wwids would be in place when multipathd is started and
everything will work.
Cheers,
Hannes
P.S.: And yes, I do have some patches queued, too ...
--
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)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A
2014-07-02 6:03 ` Hannes Reinecke
@ 2014-07-02 19:48 ` Benjamin Marzinski
2014-07-03 11:05 ` Hannes Reinecke
0 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-07-02 19:48 UTC (permalink / raw)
To: device-mapper development
On Wed, Jul 02, 2014 at 08:03:38AM +0200, Hannes Reinecke wrote:
> On 07/01/2014 09:22 PM, Christophe Varoqui wrote:
> >Hannes,
> >
> >would you Ack this one, or do you have some other idea for this in
> >your tree ?
> >
> Sigh. The whole multipath / systemd / dracut integration
> is _a mess_.
> The main problem is that RH and SUSE treat multipath handling differently.
> (From what I can see. I've still a hard time to understand how
> multipath booting works with RH. So there might be errors.)
>
> RH is taking a restrictive approach, ie it'll allow only configured
> multipath devices during boot. IE it'll accept only devices present
> in '/etc/multipath/wwids' for booting. So when coming across a new
> wwid multipath won't be setup there, so of course they'll need an
> additional parameter for that.
That's not strictly true. multipathd will happily create a multipath
device on top of any valid scsi devices it finds, but unless those
devices are in /etc/multipath/wwids, other components, like lvm won't
know to leave them alone. This actually isn't an issue during the
initramfs boot stages because lvm doesn't do autoactivation there.
So, if the device appears in the initramfs portion of boot, we're great.
The specific issue that prompted this goes like this:
- The iscsi initiator is not setup to run in the initramfs on install
- Storage is added to the system that makes up a working LV
- Once the machine boots up, and is past the initramfs, the iscsi
initiator starts up and discovers the devices.
- Multipath races with lvmetad and loses
- Now you have a LV built on top of a single path device, instead of
being multipathed (The LV is on top of a partition of the path
device, so reassign_maps doesn't work on it)
If you tell the redhat installer that you want to use multipath, this
causes problems, since it can't disassemble the an arbitrary stack of
virtual devices. Eventually, we'll fix that issue, and this won't
matter anymore, because it will just be able to disassemble the virtual
device stack, and rerun multipath, to make everything autoassemble in
the correct order.
> SUSE, OTOH, is taking the permissive approach. When multipath is included in
> dracut it'll try to generate multipath devices for _all_ existing devices;
> the wwid file is not really required here.
> And, consequently, the '-A' parameter isn't required, too.
>
> While this is nice and proper, both approaches have issues:
> - From what I've seen RH is building a 'generic' initrd, and
> configures them via the kernel or dracut commandline.
> Which makes it a bit hard for multipathing as the wwid
> most certainly cannot be part of /etc/multipath/wwids.
> But I guess this is what should be fixed by this patch.
> - SUSE is building a 'per-host' initrd, ie it'll generate
> an initrd for that specific installation.
> So there isn't actually a _need_ for the permissive approach,
> as chances are it'll never come across anything else
> _but_ the configured device.
> Plus I haven't really evaluated whether the permissive
> approach actually works properly, ie that multipath will
> try to create device-mapper devices for unknown wwids.
>
> But back to the patch.
> I must say I'm not really in favour of this.
> Implementing kernel commandline parsing in the _daemon_ is just downright
> evil.
> It would be _far_ more sensible to have it implemented in dracut
> as a commandline hook, which just adds the wwid from the kernel commandline
> to /etc/multipath/wwids.
> That's a simple shell script with no magic involved.
> Then the wwids would be in place when multipathd is started and everything
> will work.
Like I metioned above, we don't have a problem in the initramfs, so
adding functionality to dracut wouldn't help. If the path devices get
discovered there, everything just works. It's what happens when
multipath is racing against lvm autoactivation in late boot that's the
problem. This problem should go away when we can automatically
deactivate an arbitrary stack of virtual devices, so I won't be
horribly sad if I have to maintain this patch outside of upstream until
then. I just thought that people might find it a valuable feature.
Also, I'd like to point out that the daemon doesn't parse the cmdline.
You can call multipath to do it. I agree that having this happen
everytime the daemon starts up by including it in multipathd.service
is kind of excessive, seeing as it's only really helpful when run
right after root pivots in the boot process. But I don't see what's
"just downright evil" about letting a command parse /proc/cmdline.
-Ben
> Cheers,
>
> Hannes
> P.S.: And yes, I do have some patches queued, too ...
> --
> 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)
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A
2014-07-02 19:48 ` Benjamin Marzinski
@ 2014-07-03 11:05 ` Hannes Reinecke
2014-07-03 19:25 ` Benjamin Marzinski
0 siblings, 1 reply; 31+ messages in thread
From: Hannes Reinecke @ 2014-07-03 11:05 UTC (permalink / raw)
To: dm-devel
On 07/02/2014 09:48 PM, Benjamin Marzinski wrote:
> On Wed, Jul 02, 2014 at 08:03:38AM +0200, Hannes Reinecke wrote:
>> On 07/01/2014 09:22 PM, Christophe Varoqui wrote:
>>> Hannes,
>>>
>>> would you Ack this one, or do you have some other idea for this in
>>> your tree ?
>>>
>> Sigh. The whole multipath / systemd / dracut integration
>> is _a mess_.
>> The main problem is that RH and SUSE treat multipath handling differently.
>> (From what I can see. I've still a hard time to understand how
>> multipath booting works with RH. So there might be errors.)
>>
>> RH is taking a restrictive approach, ie it'll allow only configured
>> multipath devices during boot. IE it'll accept only devices present
>> in '/etc/multipath/wwids' for booting. So when coming across a new
>> wwid multipath won't be setup there, so of course they'll need an
>> additional parameter for that.
>
> That's not strictly true. multipathd will happily create a multipath
> device on top of any valid scsi devices it finds, but unless those
> devices are in /etc/multipath/wwids, other components, like lvm won't
> know to leave them alone. This actually isn't an issue during the
> initramfs boot stages because lvm doesn't do autoactivation there.
>
> So, if the device appears in the initramfs portion of boot, we're great.
>
> The specific issue that prompted this goes like this:
>
> - The iscsi initiator is not setup to run in the initramfs on install
> - Storage is added to the system that makes up a working LV
> - Once the machine boots up, and is past the initramfs, the iscsi
> initiator starts up and discovers the devices.
> - Multipath races with lvmetad and loses
> - Now you have a LV built on top of a single path device, instead of
> being multipathed (The LV is on top of a partition of the path
> device, so reassign_maps doesn't work on it)
>
> If you tell the redhat installer that you want to use multipath, this
> causes problems, since it can't disassemble the an arbitrary stack of
> virtual devices. Eventually, we'll fix that issue, and this won't
> matter anymore, because it will just be able to disassemble the virtual
> device stack, and rerun multipath, to make everything autoassemble in
> the correct order.
>
Hmm. Similar to what I've seen here when booting with multipath
enabled and an empty '/etc/multipath/wwids' file.
We're having an udev rule calling 'multipath -u' to check if the
device is eligible for multipathing. If so it'll set the various
udev properties to keep LVM and others from working with that device.
But as 'multipath -u' is be checking /etc/multipath/wwids it will
always report 'not a multipath device'.
So I would be perfectly happy with 'multipath -u' _not_ checking
/etc/multipath/wwids (or have a switch for doing so).
Anyway. There is actually a slight inconsistency with the above
approach:
Multipath is _not_ setup for autoconfiguration; from my
understanding this was exactly why /etc/multipath/wwids was
introduced in the first place.
LVM, on the other hand, is trying to do autoconfiguration.
Why? I would set either both to autoconfiguration or none.
If I want something different I need to configure the system.
Can you clarify what the intended usage for /etc/multipath/wwids is?
I was under the impression that it's been introduced to keep
multipath from accepting unconfigured devices ...
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)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A
2014-07-03 11:05 ` Hannes Reinecke
@ 2014-07-03 19:25 ` Benjamin Marzinski
2014-07-04 6:24 ` Hannes Reinecke
0 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-07-03 19:25 UTC (permalink / raw)
To: device-mapper development
On Thu, Jul 03, 2014 at 01:05:37PM +0200, Hannes Reinecke wrote:
> On 07/02/2014 09:48 PM, Benjamin Marzinski wrote:
> >On Wed, Jul 02, 2014 at 08:03:38AM +0200, Hannes Reinecke wrote:
> >>On 07/01/2014 09:22 PM, Christophe Varoqui wrote:
> >>>Hannes,
> >>>
> >>>would you Ack this one, or do you have some other idea for this in
> >>>your tree ?
> >>>
> >>Sigh. The whole multipath / systemd / dracut integration
> >>is _a mess_.
> >>The main problem is that RH and SUSE treat multipath handling differently.
> >>(From what I can see. I've still a hard time to understand how
> >> multipath booting works with RH. So there might be errors.)
> >>
> >>RH is taking a restrictive approach, ie it'll allow only configured
> >>multipath devices during boot. IE it'll accept only devices present
> >>in '/etc/multipath/wwids' for booting. So when coming across a new
> >>wwid multipath won't be setup there, so of course they'll need an
> >>additional parameter for that.
> >
> >That's not strictly true. multipathd will happily create a multipath
> >device on top of any valid scsi devices it finds, but unless those
> >devices are in /etc/multipath/wwids, other components, like lvm won't
> >know to leave them alone. This actually isn't an issue during the
> >initramfs boot stages because lvm doesn't do autoactivation there.
> >
> >So, if the device appears in the initramfs portion of boot, we're great.
> >
> >The specific issue that prompted this goes like this:
> >
> >- The iscsi initiator is not setup to run in the initramfs on install
> >- Storage is added to the system that makes up a working LV
> >- Once the machine boots up, and is past the initramfs, the iscsi
> >initiator starts up and discovers the devices.
> >- Multipath races with lvmetad and loses
> >- Now you have a LV built on top of a single path device, instead of
> > being multipathed (The LV is on top of a partition of the path
> > device, so reassign_maps doesn't work on it)
> >
> >If you tell the redhat installer that you want to use multipath, this
> >causes problems, since it can't disassemble the an arbitrary stack of
> >virtual devices. Eventually, we'll fix that issue, and this won't
> >matter anymore, because it will just be able to disassemble the virtual
> >device stack, and rerun multipath, to make everything autoassemble in
> >the correct order.
> >
> Hmm. Similar to what I've seen here when booting with multipath enabled and
> an empty '/etc/multipath/wwids' file.
>
> We're having an udev rule calling 'multipath -u' to check if the device is
> eligible for multipathing. If so it'll set the various udev properties to
> keep LVM and others from working with that device.
>
> But as 'multipath -u' is be checking /etc/multipath/wwids it will always
> report 'not a multipath device'.
> So I would be perfectly happy with 'multipath -u' _not_ checking
> /etc/multipath/wwids (or have a switch for doing so).
'multipath -u'? Do you mean 'multipath -c'? I do worry that not
checking the wwids file could break things were some device appears
multipathable, but will never successfully get created for reasons
outside of multipath's knowledge. The wwids file makes sure that this
device is multipathable, because it HAS been multipathed before.
That being said, I have no objections to a switch to avoid the wwid file
check.
> Anyway. There is actually a slight inconsistency with the above approach:
> Multipath is _not_ setup for autoconfiguration; from my understanding this
> was exactly why /etc/multipath/wwids was introduced in the first place.
> LVM, on the other hand, is trying to do autoconfiguration.
>
> Why? I would set either both to autoconfiguration or none.
> If I want something different I need to configure the system.
Well, multipath is only not set up to do autoconfiguration because you
keep objecting to my find_multipaths patch, which makes multipath only
run on devices that have more than one path. With that, you can usually
leave the blacklists alone, and you will only get the devices that you
want.
> Can you clarify what the intended usage for /etc/multipath/wwids is?
> I was under the impression that it's been introduced to keep
> multipath from accepting unconfigured devices ...
Like I mentioned above, it was added to avoid the situations where
multipath isn't blacklisted on a device, but is unable to set up on it.
We use this to so that 'multipath -c' doesn't claim a device and keep
lvm or md from using it when it shouldn't.
-Ben
> 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)
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A
2014-07-03 19:25 ` Benjamin Marzinski
@ 2014-07-04 6:24 ` Hannes Reinecke
2014-07-04 7:12 ` Christophe Varoqui
0 siblings, 1 reply; 31+ messages in thread
From: Hannes Reinecke @ 2014-07-04 6:24 UTC (permalink / raw)
To: dm-devel
On 07/03/2014 09:25 PM, Benjamin Marzinski wrote:
> On Thu, Jul 03, 2014 at 01:05:37PM +0200, Hannes Reinecke wrote:
>> On 07/02/2014 09:48 PM, Benjamin Marzinski wrote:
>>> On Wed, Jul 02, 2014 at 08:03:38AM +0200, Hannes Reinecke wrote:
>>>> On 07/01/2014 09:22 PM, Christophe Varoqui wrote:
>>>>> Hannes,
>>>>>
>>>>> would you Ack this one, or do you have some other idea for this in
>>>>> your tree ?
>>>>>
>>>> Sigh. The whole multipath / systemd / dracut integration
>>>> is _a mess_.
>>>> The main problem is that RH and SUSE treat multipath handling differently.
>>>> (From what I can see. I've still a hard time to understand how
>>>> multipath booting works with RH. So there might be errors.)
>>>>
>>>> RH is taking a restrictive approach, ie it'll allow only configured
>>>> multipath devices during boot. IE it'll accept only devices present
>>>> in '/etc/multipath/wwids' for booting. So when coming across a new
>>>> wwid multipath won't be setup there, so of course they'll need an
>>>> additional parameter for that.
>>>
>>> That's not strictly true. multipathd will happily create a multipath
>>> device on top of any valid scsi devices it finds, but unless those
>>> devices are in /etc/multipath/wwids, other components, like lvm won't
>>> know to leave them alone. This actually isn't an issue during the
>>> initramfs boot stages because lvm doesn't do autoactivation there.
>>>
>>> So, if the device appears in the initramfs portion of boot, we're great.
>>>
>>> The specific issue that prompted this goes like this:
>>>
>>> - The iscsi initiator is not setup to run in the initramfs on install
>>> - Storage is added to the system that makes up a working LV
>>> - Once the machine boots up, and is past the initramfs, the iscsi
>>> initiator starts up and discovers the devices.
>>> - Multipath races with lvmetad and loses
>>> - Now you have a LV built on top of a single path device, instead of
>>> being multipathed (The LV is on top of a partition of the path
>>> device, so reassign_maps doesn't work on it)
>>>
>>> If you tell the redhat installer that you want to use multipath, this
>>> causes problems, since it can't disassemble the an arbitrary stack of
>>> virtual devices. Eventually, we'll fix that issue, and this won't
>>> matter anymore, because it will just be able to disassemble the virtual
>>> device stack, and rerun multipath, to make everything autoassemble in
>>> the correct order.
>>>
>> Hmm. Similar to what I've seen here when booting with multipath enabled and
>> an empty '/etc/multipath/wwids' file.
>>
>> We're having an udev rule calling 'multipath -u' to check if the device is
>> eligible for multipathing. If so it'll set the various udev properties to
>> keep LVM and others from working with that device.
>>
>> But as 'multipath -u' is be checking /etc/multipath/wwids it will always
>> report 'not a multipath device'.
>> So I would be perfectly happy with 'multipath -u' _not_ checking
>> /etc/multipath/wwids (or have a switch for doing so).
>
> 'multipath -u'? Do you mean 'multipath -c'? I do worry that not
> checking the wwids file could break things were some device appears
> multipathable, but will never successfully get created for reasons
> outside of multipath's knowledge. The wwids file makes sure that this
> device is multipathable, because it HAS been multipathed before.
>
> That being said, I have no objections to a switch to avoid the wwid file
> check.
>
Okay, I've send a patch.
>> Anyway. There is actually a slight inconsistency with the above approach:
>> Multipath is _not_ setup for autoconfiguration; from my understanding this
>> was exactly why /etc/multipath/wwids was introduced in the first place.
>> LVM, on the other hand, is trying to do autoconfiguration.
>>
>> Why? I would set either both to autoconfiguration or none.
>> If I want something different I need to configure the system.
>
> Well, multipath is only not set up to do autoconfiguration because you
> keep objecting to my find_multipaths patch, which makes multipath only
> run on devices that have more than one path. With that, you can usually
> leave the blacklists alone, and you will only get the devices that you
> want.
>
Oh, I don't have any objections to that, provided it's configurable
via the config file.
Care to send a patch for that?
>
>> Can you clarify what the intended usage for /etc/multipath/wwids is?
>> I was under the impression that it's been introduced to keep
>> multipath from accepting unconfigured devices ...
>
> Like I mentioned above, it was added to avoid the situations where
> multipath isn't blacklisted on a device, but is unable to set up on it.
> We use this to so that 'multipath -c' doesn't claim a device and keep
> lvm or md from using it when it shouldn't.
>
Ah, I've been luckier than you, then.
I keep telling folks that multipath will grab all devices, and the
only way to modify this is blacklisting devices via /etc/multipath.conf.
So any system where the above happens is per definition
misconfigured :-)
Christophe, what happened to the patches you've said to have
applied? I haven't seen them showing up in the git repository ...
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)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A
2014-07-04 6:24 ` Hannes Reinecke
@ 2014-07-04 7:12 ` Christophe Varoqui
0 siblings, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-04 7:12 UTC (permalink / raw)
To: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 6023 bytes --]
Still tanked in my local git tree.
I'll push them online late this afternoon.
On Fri, Jul 4, 2014 at 8:24 AM, Hannes Reinecke <hare@suse.de> wrote:
> On 07/03/2014 09:25 PM, Benjamin Marzinski wrote:
>
>> On Thu, Jul 03, 2014 at 01:05:37PM +0200, Hannes Reinecke wrote:
>>
>>> On 07/02/2014 09:48 PM, Benjamin Marzinski wrote:
>>>
>>>> On Wed, Jul 02, 2014 at 08:03:38AM +0200, Hannes Reinecke wrote:
>>>>
>>>>> On 07/01/2014 09:22 PM, Christophe Varoqui wrote:
>>>>>
>>>>>> Hannes,
>>>>>>
>>>>>> would you Ack this one, or do you have some other idea for this in
>>>>>> your tree ?
>>>>>>
>>>>>> Sigh. The whole multipath / systemd / dracut integration
>>>>> is _a mess_.
>>>>> The main problem is that RH and SUSE treat multipath handling
>>>>> differently.
>>>>> (From what I can see. I've still a hard time to understand how
>>>>> multipath booting works with RH. So there might be errors.)
>>>>>
>>>>> RH is taking a restrictive approach, ie it'll allow only configured
>>>>> multipath devices during boot. IE it'll accept only devices present
>>>>> in '/etc/multipath/wwids' for booting. So when coming across a new
>>>>> wwid multipath won't be setup there, so of course they'll need an
>>>>> additional parameter for that.
>>>>>
>>>>
>>>> That's not strictly true. multipathd will happily create a multipath
>>>> device on top of any valid scsi devices it finds, but unless those
>>>> devices are in /etc/multipath/wwids, other components, like lvm won't
>>>> know to leave them alone. This actually isn't an issue during the
>>>> initramfs boot stages because lvm doesn't do autoactivation there.
>>>>
>>>> So, if the device appears in the initramfs portion of boot, we're great.
>>>>
>>>> The specific issue that prompted this goes like this:
>>>>
>>>> - The iscsi initiator is not setup to run in the initramfs on install
>>>> - Storage is added to the system that makes up a working LV
>>>> - Once the machine boots up, and is past the initramfs, the iscsi
>>>> initiator starts up and discovers the devices.
>>>> - Multipath races with lvmetad and loses
>>>> - Now you have a LV built on top of a single path device, instead of
>>>> being multipathed (The LV is on top of a partition of the path
>>>> device, so reassign_maps doesn't work on it)
>>>>
>>>> If you tell the redhat installer that you want to use multipath, this
>>>> causes problems, since it can't disassemble the an arbitrary stack of
>>>> virtual devices. Eventually, we'll fix that issue, and this won't
>>>> matter anymore, because it will just be able to disassemble the virtual
>>>> device stack, and rerun multipath, to make everything autoassemble in
>>>> the correct order.
>>>>
>>>> Hmm. Similar to what I've seen here when booting with multipath
>>> enabled and
>>> an empty '/etc/multipath/wwids' file.
>>>
>>> We're having an udev rule calling 'multipath -u' to check if the device
>>> is
>>> eligible for multipathing. If so it'll set the various udev properties to
>>> keep LVM and others from working with that device.
>>>
>>> But as 'multipath -u' is be checking /etc/multipath/wwids it will always
>>> report 'not a multipath device'.
>>> So I would be perfectly happy with 'multipath -u' _not_ checking
>>> /etc/multipath/wwids (or have a switch for doing so).
>>>
>>
>> 'multipath -u'? Do you mean 'multipath -c'? I do worry that not
>> checking the wwids file could break things were some device appears
>> multipathable, but will never successfully get created for reasons
>> outside of multipath's knowledge. The wwids file makes sure that this
>> device is multipathable, because it HAS been multipathed before.
>>
>> That being said, I have no objections to a switch to avoid the wwid file
>> check.
>>
>> Okay, I've send a patch.
>
>
> Anyway. There is actually a slight inconsistency with the above approach:
>>> Multipath is _not_ setup for autoconfiguration; from my understanding
>>> this
>>> was exactly why /etc/multipath/wwids was introduced in the first place.
>>> LVM, on the other hand, is trying to do autoconfiguration.
>>>
>>> Why? I would set either both to autoconfiguration or none.
>>> If I want something different I need to configure the system.
>>>
>>
>> Well, multipath is only not set up to do autoconfiguration because you
>> keep objecting to my find_multipaths patch, which makes multipath only
>> run on devices that have more than one path. With that, you can usually
>> leave the blacklists alone, and you will only get the devices that you
>> want.
>>
>> Oh, I don't have any objections to that, provided it's configurable
> via the config file.
> Care to send a patch for that?
>
>
>
>> Can you clarify what the intended usage for /etc/multipath/wwids is?
>>> I was under the impression that it's been introduced to keep
>>> multipath from accepting unconfigured devices ...
>>>
>>
>> Like I mentioned above, it was added to avoid the situations where
>> multipath isn't blacklisted on a device, but is unable to set up on it.
>> We use this to so that 'multipath -c' doesn't claim a device and keep
>> lvm or md from using it when it shouldn't.
>>
>> Ah, I've been luckier than you, then.
> I keep telling folks that multipath will grab all devices, and the only
> way to modify this is blacklisting devices via /etc/multipath.conf.
> So any system where the above happens is per definition misconfigured :-)
>
> Christophe, what happened to the patches you've said to have applied? I
> haven't seen them showing up in the git repository ...
>
>
> 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)
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>
[-- Attachment #1.2: Type: text/html, Size: 7979 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 07/12] Make multipathd orphan paths that were removed externally
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
` (5 preceding siblings ...)
2014-06-30 5:13 ` [PATCH 06/12] Make multipath add wwids from kernel cmdline mpath.wwids with -A Benjamin Marzinski
@ 2014-06-30 5:13 ` Benjamin Marzinski
2014-07-24 8:41 ` Christophe Varoqui
2014-06-30 5:13 ` [PATCH 08/12] Add missing interactive commands to multipathd man page Benjamin Marzinski
` (4 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:13 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
Multipathd was only orphaning paths that it removed, not ones that were
removed by the multipath command. This could cause problems if a path
was failed but not removed, and "multipath -r" was run. multipath would
remove the path, and when multipathd updated itself, it would remove
that path from the multipath device's path list, but not orphan it.
When the path became active again, multipathd crashed trying to adjust
the pathgroups of the multipath device it had previously belonged to.
This patch makes sure that whenever multipathd updates the multipath device
table, it first makes sure the mpp->paths is uptodate. Once it has
finished updating the device table, it orphans any paths in mpp->paths
that are no longer part of the multipath device.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/structs_vec.c | 31 +++++++++++++++++++++++++++----
multipathd/main.c | 4 ++++
2 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 8cdbe3d..23f5bbb 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -281,12 +281,38 @@ update_multipath_status (struct multipath *mpp)
return 0;
}
+void sync_paths(struct multipath *mpp, vector pathvec)
+{
+ struct path *pp;
+ struct pathgroup *pgp;
+ int found, i, j;
+
+ vector_foreach_slot (mpp->paths, pp, i) {
+ found = 0;
+ vector_foreach_slot(mpp->pg, pgp, j) {
+ if (find_slot(pgp->paths, (void *)pp) != -1) {
+ found = 1;
+ break;
+ }
+ }
+ if (!found) {
+ condlog(3, "%s dropped path %s", mpp->alias, pp->dev);
+ vector_del_slot(mpp->paths, i--);
+ orphan_path(pp, "path removed externally");
+ }
+ }
+ update_mpp_paths(mpp, pathvec);
+ vector_foreach_slot (mpp->paths, pp, i)
+ pp->mpp = mpp;
+}
+
extern int
update_multipath_strings (struct multipath *mpp, vector pathvec)
{
if (!mpp)
return 1;
+ update_mpp_paths(mpp, pathvec);
condlog(4, "%s: %s", mpp->alias, __FUNCTION__);
free_multipath_attributes(mpp);
@@ -295,6 +321,7 @@ update_multipath_strings (struct multipath *mpp, vector pathvec)
if (update_multipath_table(mpp, pathvec))
return 1;
+ sync_paths(mpp, pathvec);
if (update_multipath_status(mpp))
return 1;
@@ -508,13 +535,9 @@ int update_multipath (struct vectors *vecs, char *mapname, int reset)
return 2;
}
- free_pgvec(mpp->pg, KEEP_PATHS);
- mpp->pg = NULL;
-
if (__setup_multipath(vecs, mpp, reset))
return 1; /* mpp freed in setup_multipath */
- adopt_paths(vecs->pathvec, mpp, 0);
/*
* compare checkers states with DM states
*/
diff --git a/multipathd/main.c b/multipathd/main.c
index 56d00d3..337bfe9 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1170,6 +1170,10 @@ check_path (struct vectors * vecs, struct path * pp)
pp->dev);
pp->dmstate = PSTATE_UNDEF;
}
+ /* if update_multipath_strings orphaned the path, quit early */
+ if (!pp->mpp)
+ return 0;
+
pp->chkrstate = newstate;
if (newstate != pp->state) {
int oldstate = pp->state;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 07/12] Make multipathd orphan paths that were removed externally
2014-06-30 5:13 ` [PATCH 07/12] Make multipathd orphan paths that were removed externally Benjamin Marzinski
@ 2014-07-24 8:41 ` Christophe Varoqui
0 siblings, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-24 8:41 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 3884 bytes --]
Applied,
Thanks.
On Mon, Jun 30, 2014 at 7:13 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> Multipathd was only orphaning paths that it removed, not ones that were
> removed by the multipath command. This could cause problems if a path
> was failed but not removed, and "multipath -r" was run. multipath would
> remove the path, and when multipathd updated itself, it would remove
> that path from the multipath device's path list, but not orphan it.
> When the path became active again, multipathd crashed trying to adjust
> the pathgroups of the multipath device it had previously belonged to.
>
> This patch makes sure that whenever multipathd updates the multipath device
> table, it first makes sure the mpp->paths is uptodate. Once it has
> finished updating the device table, it orphans any paths in mpp->paths
> that are no longer part of the multipath device.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/structs_vec.c | 31 +++++++++++++++++++++++++++----
> multipathd/main.c | 4 ++++
> 2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 8cdbe3d..23f5bbb 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -281,12 +281,38 @@ update_multipath_status (struct multipath *mpp)
> return 0;
> }
>
> +void sync_paths(struct multipath *mpp, vector pathvec)
> +{
> + struct path *pp;
> + struct pathgroup *pgp;
> + int found, i, j;
> +
> + vector_foreach_slot (mpp->paths, pp, i) {
> + found = 0;
> + vector_foreach_slot(mpp->pg, pgp, j) {
> + if (find_slot(pgp->paths, (void *)pp) != -1) {
> + found = 1;
> + break;
> + }
> + }
> + if (!found) {
> + condlog(3, "%s dropped path %s", mpp->alias,
> pp->dev);
> + vector_del_slot(mpp->paths, i--);
> + orphan_path(pp, "path removed externally");
> + }
> + }
> + update_mpp_paths(mpp, pathvec);
> + vector_foreach_slot (mpp->paths, pp, i)
> + pp->mpp = mpp;
> +}
> +
> extern int
> update_multipath_strings (struct multipath *mpp, vector pathvec)
> {
> if (!mpp)
> return 1;
>
> + update_mpp_paths(mpp, pathvec);
> condlog(4, "%s: %s", mpp->alias, __FUNCTION__);
>
> free_multipath_attributes(mpp);
> @@ -295,6 +321,7 @@ update_multipath_strings (struct multipath *mpp,
> vector pathvec)
>
> if (update_multipath_table(mpp, pathvec))
> return 1;
> + sync_paths(mpp, pathvec);
>
> if (update_multipath_status(mpp))
> return 1;
> @@ -508,13 +535,9 @@ int update_multipath (struct vectors *vecs, char
> *mapname, int reset)
> return 2;
> }
>
> - free_pgvec(mpp->pg, KEEP_PATHS);
> - mpp->pg = NULL;
> -
> if (__setup_multipath(vecs, mpp, reset))
> return 1; /* mpp freed in setup_multipath */
>
> - adopt_paths(vecs->pathvec, mpp, 0);
> /*
> * compare checkers states with DM states
> */
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 56d00d3..337bfe9 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1170,6 +1170,10 @@ check_path (struct vectors * vecs, struct path * pp)
> pp->dev);
> pp->dmstate = PSTATE_UNDEF;
> }
> + /* if update_multipath_strings orphaned the path, quit early */
> + if (!pp->mpp)
> + return 0;
> +
> pp->chkrstate = newstate;
> if (newstate != pp->state) {
> int oldstate = pp->state;
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 5029 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 08/12] Add missing interactive commands to multipathd man page
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
` (6 preceding siblings ...)
2014-06-30 5:13 ` [PATCH 07/12] Make multipathd orphan paths that were removed externally Benjamin Marzinski
@ 2014-06-30 5:13 ` Benjamin Marzinski
2014-06-30 5:14 ` [PATCH 09/12] orphan paths on failed add Benjamin Marzinski
` (3 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:13 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
This patch adds the missing options from the multipathd.8 man page. It
also fixes a typo in a log message from the alua prioritizer.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/prioritizers/alua.c | 4 ++--
multipathd/multipathd.8 | 29 +++++++++++++++++++++++++++++
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/libmultipath/prioritizers/alua.c b/libmultipath/prioritizers/alua.c
index 4165ec6..b967ec7 100644
--- a/libmultipath/prioritizers/alua.c
+++ b/libmultipath/prioritizers/alua.c
@@ -119,10 +119,10 @@ int getprio (struct path * pp, char * args)
condlog(0, "%s: couldn't get target port group", pp->dev);
break;
case ALUA_PRIO_GETAAS_FAILED:
- condlog(0, "%s: couln't get asymmetric access state", pp->dev);
+ condlog(0, "%s: couldn't get asymmetric access state", pp->dev);
break;
case ALUA_PRIO_TPGS_FAILED:
- condlog(3, "%s: couln't get supported alua states", pp->dev);
+ condlog(3, "%s: couldn't get supported alua states", pp->dev);
break;
}
}
diff --git a/multipathd/multipathd.8 b/multipathd/multipathd.8
index 6da9d2b..358141d 100644
--- a/multipathd/multipathd.8
+++ b/multipathd/multipathd.8
@@ -80,6 +80,9 @@ Show all available block devices by name including the information if they are b
.B list|show status
Show the number of path checkers in each possible state, the number of monitored paths, and whether multipathd is currently handling a uevent.
.TP
+.B list|show daemon
+Show the current state of the multipathd daemon
+.TP
.B add path $path
Add a path to the list of monitored paths. $path is as listed in /sys/block (e.g. sda).
.TP
@@ -107,6 +110,13 @@ Sets map $map into suspend state.
.B resume map|multipath $map
Resumes map $map from suspend state.
.TP
+.B reset map|multipath $map
+Reassign existing device-mapper table(s) use use the multipath device, instead
+of its path devices.
+.TP
+.B reload map|multipath $map
+Reload a multipath device.
+.TP
.B fail path $path
Sets path $path into failed state.
.TP
@@ -125,8 +135,27 @@ Disable queuing on multipathed map $map
.B restorequeueing map|multipath $map
Restore queuing on multipahted map $map
.TP
+.B forcequeueing daemon
+Forces multipathd into queue_without_daemon mode, so that no_path_retry queueing
+will not be disabled when the daemon stops
+.TP
+.B restorequeueing daemon
+Restores configured queue_without_daemon mode
+.TP
+.B map|multipath $map setprstatus
+Enable persistent reservation management on $map
+.TP
+.B map|multipath $map unsetprstatus
+Disable persistent reservation management on $map
+.TP
+.B map|multipath $map getprstatus
+Get the current persistent reservation management status of $map
+.TP
.B quit|exit
End interactive session.
+.TP
+.B shutdown
+Stop multipathd.
.SH "SYSTEMD INTEGRATION"
When compiled with systemd support two systemd service files are
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 09/12] orphan paths on failed add
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
` (7 preceding siblings ...)
2014-06-30 5:13 ` [PATCH 08/12] Add missing interactive commands to multipathd man page Benjamin Marzinski
@ 2014-06-30 5:14 ` Benjamin Marzinski
2014-07-24 8:45 ` Christophe Varoqui
2014-06-30 5:14 ` [PATCH 10/12] Improve multipath.conf syntax checking Benjamin Marzinski
` (2 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:14 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
When multipathd tries to add a path but fails doing the table reload, it
wasn't orphaning the path. This can cause problems later if multipathd
tries to switch the pathgroup of this path which isn't actually part of
the multipath device.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
multipathd/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 337bfe9..3afed62 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -561,7 +561,7 @@ rescan:
return 0;
}
else
- return 1;
+ goto fail;
fail_map:
remove_map(mpp, vecs, 1);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 09/12] orphan paths on failed add
2014-06-30 5:14 ` [PATCH 09/12] orphan paths on failed add Benjamin Marzinski
@ 2014-07-24 8:45 ` Christophe Varoqui
0 siblings, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-24 8:45 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 841 bytes --]
Applied,
Thanks
On Mon, Jun 30, 2014 at 7:14 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> When multipathd tries to add a path but fails doing the table reload, it
> wasn't orphaning the path. This can cause problems later if multipathd
> tries to switch the pathgroup of this path which isn't actually part of
> the multipath device.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> multipathd/main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 337bfe9..3afed62 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -561,7 +561,7 @@ rescan:
> return 0;
> }
> else
> - return 1;
> + goto fail;
>
> fail_map:
> remove_map(mpp, vecs, 1);
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 1380 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 10/12] Improve multipath.conf syntax checking
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
` (8 preceding siblings ...)
2014-06-30 5:14 ` [PATCH 09/12] orphan paths on failed add Benjamin Marzinski
@ 2014-06-30 5:14 ` Benjamin Marzinski
2014-07-24 8:47 ` Christophe Varoqui
2014-06-30 5:14 ` [PATCH 11/12] make prioritizers use checker_timeout, if set Benjamin Marzinski
2014-06-30 5:14 ` [PATCH 12/12] Add multipath.conf force_sync option Benjamin Marzinski
11 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:14 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
multipath's parser for multipath.conf is neither very verbose or forgiving
about errors. This patch improves both. multipath will now warn about
obviously missing quotes and curly braces, but will still do the right thing.
It will also give more verbose error messages including a line number when
it can't parse a line.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/parser.c | 152 +++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 125 insertions(+), 27 deletions(-)
diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 0d4c870..accff62 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -395,36 +395,57 @@ set_value(vector strvec)
char *alloc = NULL;
char *tmp;
- if (!str)
+ if (!str) {
+ condlog(0, "option '%s' missing value",
+ (char *)VECTOR_SLOT(strvec, 0));
return NULL;
-
+ }
size = strlen(str);
- if (size == 0)
+ if (size == 0) {
+ condlog(0, "option '%s' has empty value",
+ (char *)VECTOR_SLOT(strvec, 0));
return NULL;
-
- if (*str == '"') {
- for (i = 2; i < VECTOR_SIZE(strvec); i++) {
- str = VECTOR_SLOT(strvec, i);
- len += strlen(str);
- if (!alloc)
- alloc =
- (char *) MALLOC(sizeof (char) *
- (len + 1));
- else {
- alloc =
- REALLOC(alloc, sizeof (char) * (len + 1));
- tmp = VECTOR_SLOT(strvec, i-1);
- if (alloc && *str != '"' && *tmp != '"')
- strncat(alloc, " ", 1);
- }
-
- if (alloc && i != VECTOR_SIZE(strvec)-1)
- strncat(alloc, str, strlen(str));
- }
- } else {
+ }
+ if (*str != '"') {
alloc = MALLOC(sizeof (char) * (size + 1));
if (alloc)
memcpy(alloc, str, size);
+ else
+ condlog(0, "can't allocate memeory for option '%s'",
+ (char *)VECTOR_SLOT(strvec, 0));
+ return alloc;
+ }
+ /* Even empty quotes counts as a value (An empty string) */
+ alloc = (char *) MALLOC(sizeof (char));
+ if (!alloc) {
+ condlog(0, "can't allocate memeory for option '%s'",
+ (char *)VECTOR_SLOT(strvec, 0));
+ return NULL;
+ }
+ for (i = 2; i < VECTOR_SIZE(strvec); i++) {
+ str = VECTOR_SLOT(strvec, i);
+ if (!str) {
+ free(alloc);
+ condlog(0, "parse error for option '%s'",
+ (char *)VECTOR_SLOT(strvec, 0));
+ return NULL;
+ }
+ if (*str == '"')
+ break;
+ tmp = alloc;
+ /* The first +1 is for the NULL byte. The rest are for the
+ * spaces between words */
+ len += strlen(str) + 1;
+ alloc = REALLOC(alloc, sizeof (char) * len);
+ if (!alloc) {
+ FREE(tmp);
+ condlog(0, "can't allocate memeory for option '%s'",
+ (char *)VECTOR_SLOT(strvec, 0));
+ return NULL;
+ }
+ if (*alloc != '\0')
+ strncat(alloc, " ", 1);
+ strncat(alloc, str, strlen(str));
}
return alloc;
}
@@ -465,6 +486,74 @@ void free_uniques(vector uniques)
}
int
+is_sublevel_keyword(char *str)
+{
+ return (strcmp(str, "defaults") == 0 || strcmp(str, "blacklist") == 0 ||
+ strcmp(str, "blacklist_exceptions") == 0 ||
+ strcmp(str, "devices") == 0 || strcmp(str, "devices") == 0 ||
+ strcmp(str, "device") == 0 || strcmp(str, "multipaths") == 0 ||
+ strcmp(str, "multipath") == 0);
+}
+
+int
+validate_config_strvec(vector strvec)
+{
+ char *str;
+ int i;
+
+ str = VECTOR_SLOT(strvec, 0);
+ if (str == NULL) {
+ condlog(0, "can't parse option on line %d of config file",
+ line_nr);
+ return -1;
+ }
+ if (*str == '}') {
+ if (VECTOR_SIZE(strvec) > 1)
+ condlog(0, "ignoring extra data starting with '%s' on line %d of config file", (char *)VECTOR_SLOT(strvec, 1), line_nr);
+ return 0;
+ }
+ if (*str == '{') {
+ condlog(0, "invalid keyword '%s' on line %d of config file", str, line_nr);
+ return -1;
+ }
+ if (is_sublevel_keyword(str)) {
+ str = VECTOR_SLOT(strvec, 1);
+ if (str == NULL)
+ condlog(0, "missing '{' on line %d of config file", line_nr);
+ else if (*str != '{')
+ condlog(0, "expecting '{' on line %d of config file. found '%s'", line_nr, str);
+ else if (VECTOR_SIZE(strvec) > 2)
+ condlog(0, "ignoring extra data starting with '%s' on line %d of config file", (char *)VECTOR_SLOT(strvec, 2), line_nr);
+ return 0;
+ }
+ str = VECTOR_SLOT(strvec, 1);
+ if (str == NULL) {
+ condlog(0, "missing value for option '%s' on line %d of config file", (char *)VECTOR_SLOT(strvec, 0), line_nr);
+ return -1;
+ }
+ if (*str != '"') {
+ if (VECTOR_SIZE(strvec) > 2)
+ condlog(0, "ignoring extra data starting with '%s' on line %d of config file", (char *)VECTOR_SLOT(strvec, 2), line_nr);
+ return 0;
+ }
+ for (i = 2; i < VECTOR_SIZE(strvec); i++) {
+ str = VECTOR_SLOT(strvec, i);
+ if (str == NULL) {
+ condlog(0, "can't parse value on line %d of config file", line_nr);
+ return -1;
+ }
+ if (*str == '"') {
+ if (VECTOR_SIZE(strvec) > i + 1)
+ condlog(0, "ignoring extra data starting with '%s' on line %d of config file", (char *)VECTOR_SLOT(strvec, (i + 1)), line_nr);
+ return 0;
+ }
+ }
+ condlog(0, "missing closing quotes on line %d of config file",
+ line_nr);
+ return 0;
+}
+
+int
process_stream(vector keywords)
{
int i;
@@ -494,11 +583,20 @@ process_stream(vector keywords)
if (!strvec)
continue;
+ if (validate_config_strvec(strvec) != 0) {
+ free_strvec(strvec);
+ continue;
+ }
+
str = VECTOR_SLOT(strvec, 0);
- if (!strcmp(str, EOB) && kw_level > 0) {
- free_strvec(strvec);
- break;
+ if (!strcmp(str, EOB)) {
+ if (kw_level > 0) {
+ free_strvec(strvec);
+ break;
+ }
+ condlog(0, "unmatched '%s' at line %d of config file",
+ EOB, line_nr);
}
for (i = 0; i < VECTOR_SIZE(keywords); i++) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 10/12] Improve multipath.conf syntax checking
2014-06-30 5:14 ` [PATCH 10/12] Improve multipath.conf syntax checking Benjamin Marzinski
@ 2014-07-24 8:47 ` Christophe Varoqui
0 siblings, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-24 8:47 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 8265 bytes --]
Applied,
Thanks.
On Mon, Jun 30, 2014 at 7:14 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> multipath's parser for multipath.conf is neither very verbose or forgiving
> about errors. This patch improves both. multipath will now warn about
> obviously missing quotes and curly braces, but will still do the right
> thing.
> It will also give more verbose error messages including a line number when
> it can't parse a line.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/parser.c | 152
> +++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 125 insertions(+), 27 deletions(-)
>
> diff --git a/libmultipath/parser.c b/libmultipath/parser.c
> index 0d4c870..accff62 100644
> --- a/libmultipath/parser.c
> +++ b/libmultipath/parser.c
> @@ -395,36 +395,57 @@ set_value(vector strvec)
> char *alloc = NULL;
> char *tmp;
>
> - if (!str)
> + if (!str) {
> + condlog(0, "option '%s' missing value",
> + (char *)VECTOR_SLOT(strvec, 0));
> return NULL;
> -
> + }
> size = strlen(str);
> - if (size == 0)
> + if (size == 0) {
> + condlog(0, "option '%s' has empty value",
> + (char *)VECTOR_SLOT(strvec, 0));
> return NULL;
> -
> - if (*str == '"') {
> - for (i = 2; i < VECTOR_SIZE(strvec); i++) {
> - str = VECTOR_SLOT(strvec, i);
> - len += strlen(str);
> - if (!alloc)
> - alloc =
> - (char *) MALLOC(sizeof (char) *
> - (len + 1));
> - else {
> - alloc =
> - REALLOC(alloc, sizeof (char) * (len +
> 1));
> - tmp = VECTOR_SLOT(strvec, i-1);
> - if (alloc && *str != '"' && *tmp != '"')
> - strncat(alloc, " ", 1);
> - }
> -
> - if (alloc && i != VECTOR_SIZE(strvec)-1)
> - strncat(alloc, str, strlen(str));
> - }
> - } else {
> + }
> + if (*str != '"') {
> alloc = MALLOC(sizeof (char) * (size + 1));
> if (alloc)
> memcpy(alloc, str, size);
> + else
> + condlog(0, "can't allocate memeory for option
> '%s'",
> + (char *)VECTOR_SLOT(strvec, 0));
> + return alloc;
> + }
> + /* Even empty quotes counts as a value (An empty string) */
> + alloc = (char *) MALLOC(sizeof (char));
> + if (!alloc) {
> + condlog(0, "can't allocate memeory for option '%s'",
> + (char *)VECTOR_SLOT(strvec, 0));
> + return NULL;
> + }
> + for (i = 2; i < VECTOR_SIZE(strvec); i++) {
> + str = VECTOR_SLOT(strvec, i);
> + if (!str) {
> + free(alloc);
> + condlog(0, "parse error for option '%s'",
> + (char *)VECTOR_SLOT(strvec, 0));
> + return NULL;
> + }
> + if (*str == '"')
> + break;
> + tmp = alloc;
> + /* The first +1 is for the NULL byte. The rest are for the
> + * spaces between words */
> + len += strlen(str) + 1;
> + alloc = REALLOC(alloc, sizeof (char) * len);
> + if (!alloc) {
> + FREE(tmp);
> + condlog(0, "can't allocate memeory for option
> '%s'",
> + (char *)VECTOR_SLOT(strvec, 0));
> + return NULL;
> + }
> + if (*alloc != '\0')
> + strncat(alloc, " ", 1);
> + strncat(alloc, str, strlen(str));
> }
> return alloc;
> }
> @@ -465,6 +486,74 @@ void free_uniques(vector uniques)
> }
>
> int
> +is_sublevel_keyword(char *str)
> +{
> + return (strcmp(str, "defaults") == 0 || strcmp(str, "blacklist")
> == 0 ||
> + strcmp(str, "blacklist_exceptions") == 0 ||
> + strcmp(str, "devices") == 0 || strcmp(str, "devices") == 0
> ||
> + strcmp(str, "device") == 0 || strcmp(str, "multipaths") ==
> 0 ||
> + strcmp(str, "multipath") == 0);
> +}
> +
> +int
> +validate_config_strvec(vector strvec)
> +{
> + char *str;
> + int i;
> +
> + str = VECTOR_SLOT(strvec, 0);
> + if (str == NULL) {
> + condlog(0, "can't parse option on line %d of config file",
> + line_nr);
> + return -1;
> + }
> + if (*str == '}') {
> + if (VECTOR_SIZE(strvec) > 1)
> + condlog(0, "ignoring extra data starting with '%s'
> on line %d of config file", (char *)VECTOR_SLOT(strvec, 1), line_nr);
> + return 0;
> + }
> + if (*str == '{') {
> + condlog(0, "invalid keyword '%s' on line %d of config
> file", str, line_nr);
> + return -1;
> + }
> + if (is_sublevel_keyword(str)) {
> + str = VECTOR_SLOT(strvec, 1);
> + if (str == NULL)
> + condlog(0, "missing '{' on line %d of config
> file", line_nr);
> + else if (*str != '{')
> + condlog(0, "expecting '{' on line %d of config
> file. found '%s'", line_nr, str);
> + else if (VECTOR_SIZE(strvec) > 2)
> + condlog(0, "ignoring extra data starting with '%s'
> on line %d of config file", (char *)VECTOR_SLOT(strvec, 2), line_nr);
> + return 0;
> + }
> + str = VECTOR_SLOT(strvec, 1);
> + if (str == NULL) {
> + condlog(0, "missing value for option '%s' on line %d of
> config file", (char *)VECTOR_SLOT(strvec, 0), line_nr);
> + return -1;
> + }
> + if (*str != '"') {
> + if (VECTOR_SIZE(strvec) > 2)
> + condlog(0, "ignoring extra data starting with '%s'
> on line %d of config file", (char *)VECTOR_SLOT(strvec, 2), line_nr);
> + return 0;
> + }
> + for (i = 2; i < VECTOR_SIZE(strvec); i++) {
> + str = VECTOR_SLOT(strvec, i);
> + if (str == NULL) {
> + condlog(0, "can't parse value on line %d of config
> file", line_nr);
> + return -1;
> + }
> + if (*str == '"') {
> + if (VECTOR_SIZE(strvec) > i + 1)
> + condlog(0, "ignoring extra data starting
> with '%s' on line %d of config file", (char *)VECTOR_SLOT(strvec, (i + 1)),
> line_nr);
> + return 0;
> + }
> + }
> + condlog(0, "missing closing quotes on line %d of config file",
> + line_nr);
> + return 0;
> +}
> +
> +int
> process_stream(vector keywords)
> {
> int i;
> @@ -494,11 +583,20 @@ process_stream(vector keywords)
> if (!strvec)
> continue;
>
> + if (validate_config_strvec(strvec) != 0) {
> + free_strvec(strvec);
> + continue;
> + }
> +
> str = VECTOR_SLOT(strvec, 0);
>
> - if (!strcmp(str, EOB) && kw_level > 0) {
> - free_strvec(strvec);
> - break;
> + if (!strcmp(str, EOB)) {
> + if (kw_level > 0) {
> + free_strvec(strvec);
> + break;
> + }
> + condlog(0, "unmatched '%s' at line %d of config
> file",
> + EOB, line_nr);
> }
>
> for (i = 0; i < VECTOR_SIZE(keywords); i++) {
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 11083 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 11/12] make prioritizers use checker_timeout, if set
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
` (9 preceding siblings ...)
2014-06-30 5:14 ` [PATCH 10/12] Improve multipath.conf syntax checking Benjamin Marzinski
@ 2014-06-30 5:14 ` Benjamin Marzinski
2014-07-24 8:49 ` Christophe Varoqui
2014-06-30 5:14 ` [PATCH 12/12] Add multipath.conf force_sync option Benjamin Marzinski
11 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:14 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
The multipath prioritizers can get stuck issuing scsi commands that
don't return quickly, just like the checkers. So if checker_timeout
is set, the prioritizers should should it for their cmd timeouts as
well.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/prio.c | 7 +++++++
libmultipath/prio.h | 1 +
libmultipath/prioritizers/alua_rtpg.c | 5 +++--
libmultipath/prioritizers/emc.c | 2 +-
libmultipath/prioritizers/hds.c | 2 +-
libmultipath/prioritizers/hp_sw.c | 2 +-
libmultipath/prioritizers/ontap.c | 4 ++--
libmultipath/prioritizers/rdac.c | 2 +-
multipath.conf.annotated | 5 +++--
multipath/multipath.conf.5 | 4 ++--
10 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/libmultipath/prio.c b/libmultipath/prio.c
index 05a8cf1..6ee0b9c 100644
--- a/libmultipath/prio.c
+++ b/libmultipath/prio.c
@@ -10,6 +10,13 @@
static LIST_HEAD(prioritizers);
+unsigned int get_prio_timeout(unsigned int default_timeout)
+{
+ if (conf->checker_timeout)
+ return conf->checker_timeout * 1000;
+ return default_timeout;
+}
+
int init_prio (void)
{
if (!add_prio(DEFAULT_PRIO))
diff --git a/libmultipath/prio.h b/libmultipath/prio.h
index 4eeb216..495688f 100644
--- a/libmultipath/prio.h
+++ b/libmultipath/prio.h
@@ -51,6 +51,7 @@ struct prio {
int (*getprio)(struct path *, char *);
};
+unsigned int get_prio_timeout(unsigned int default_timeout);
int init_prio (void);
void cleanup_prio (void);
struct prio * add_prio (char *);
diff --git a/libmultipath/prioritizers/alua_rtpg.c b/libmultipath/prioritizers/alua_rtpg.c
index 981ba06..6d04fc1 100644
--- a/libmultipath/prioritizers/alua_rtpg.c
+++ b/libmultipath/prioritizers/alua_rtpg.c
@@ -21,6 +21,7 @@
#define __user
#include <scsi/sg.h>
+#include "../prio.h"
#include "alua_rtpg.h"
#define SENSE_BUFF_LEN 32
@@ -134,7 +135,7 @@ do_inquiry(int fd, int evpd, unsigned int codepage, void *resp, int resplen)
hdr.dxfer_len = resplen;
hdr.sbp = sense;
hdr.mx_sb_len = sizeof(sense);
- hdr.timeout = DEF_TIMEOUT;
+ hdr.timeout = get_prio_timeout(DEF_TIMEOUT);
if (ioctl(fd, SG_IO, &hdr) < 0) {
PRINT_DEBUG("do_inquiry: IOCTL failed!\n");
@@ -253,7 +254,7 @@ do_rtpg(int fd, void* resp, long resplen)
hdr.dxfer_len = resplen;
hdr.mx_sb_len = sizeof(sense);
hdr.sbp = sense;
- hdr.timeout = DEF_TIMEOUT;
+ hdr.timeout = get_prio_timeout(DEF_TIMEOUT);
if (ioctl(fd, SG_IO, &hdr) < 0)
return -RTPG_RTPG_FAILED;
diff --git a/libmultipath/prioritizers/emc.c b/libmultipath/prioritizers/emc.c
index 91b3d90..e49809c 100644
--- a/libmultipath/prioritizers/emc.c
+++ b/libmultipath/prioritizers/emc.c
@@ -31,7 +31,7 @@ int emc_clariion_prio(const char *dev, int fd)
io_hdr.dxferp = sense_buffer;
io_hdr.cmdp = inqCmdBlk;
io_hdr.sbp = sb;
- io_hdr.timeout = 60000;
+ io_hdr.timeout = get_prio_timeout(60000);
io_hdr.pack_id = 0;
if (ioctl(fd, SG_IO, &io_hdr) < 0) {
pp_emc_log(0, "sending query command failed");
diff --git a/libmultipath/prioritizers/hds.c b/libmultipath/prioritizers/hds.c
index f748707..8043b5b 100644
--- a/libmultipath/prioritizers/hds.c
+++ b/libmultipath/prioritizers/hds.c
@@ -114,7 +114,7 @@ int hds_modular_prio (const char *dev, int fd)
io_hdr.dxferp = inqBuff;
io_hdr.cmdp = inqCmdBlk;
io_hdr.sbp = sense_buffer;
- io_hdr.timeout = 2000; /* TimeOut = 2 seconds */
+ io_hdr.timeout = get_prio_timeout(2000); /* TimeOut = 2 seconds */
if (ioctl (fd, SG_IO, &io_hdr) < 0) {
pp_hds_log(0, "SG_IO error");
diff --git a/libmultipath/prioritizers/hp_sw.c b/libmultipath/prioritizers/hp_sw.c
index c24baad..4950cf7 100644
--- a/libmultipath/prioritizers/hp_sw.c
+++ b/libmultipath/prioritizers/hp_sw.c
@@ -46,7 +46,7 @@ int hp_sw_prio(const char *dev, int fd)
io_hdr.dxfer_direction = SG_DXFER_NONE;
io_hdr.cmdp = turCmdBlk;
io_hdr.sbp = sb;
- io_hdr.timeout = 60000;
+ io_hdr.timeout = get_prio_timeout(60000);
io_hdr.pack_id = 0;
retry:
if (ioctl(fd, SG_IO, &io_hdr) < 0) {
diff --git a/libmultipath/prioritizers/ontap.c b/libmultipath/prioritizers/ontap.c
index 026d45d..5e82a17 100644
--- a/libmultipath/prioritizers/ontap.c
+++ b/libmultipath/prioritizers/ontap.c
@@ -89,7 +89,7 @@ static int send_gva(const char *dev, int fd, unsigned char pg,
io_hdr.dxferp = results;
io_hdr.cmdp = cdb;
io_hdr.sbp = sb;
- io_hdr.timeout = SG_TIMEOUT;
+ io_hdr.timeout = get_prio_timeout(SG_TIMEOUT);
io_hdr.pack_id = 0;
if (ioctl(fd, SG_IO, &io_hdr) < 0) {
pp_ontap_log(0, "SG_IO ioctl failed, errno=%d", errno);
@@ -141,7 +141,7 @@ static int get_proxy(const char *dev, int fd)
io_hdr.dxferp = results;
io_hdr.cmdp = cdb;
io_hdr.sbp = sb;
- io_hdr.timeout = SG_TIMEOUT;
+ io_hdr.timeout = get_prio_timeout(SG_TIMEOUT);
io_hdr.pack_id = 0;
if (ioctl(fd, SG_IO, &io_hdr) < 0) {
pp_ontap_log(0, "ioctl sending inquiry command failed, "
diff --git a/libmultipath/prioritizers/rdac.c b/libmultipath/prioritizers/rdac.c
index 2bf1443..a210055 100644
--- a/libmultipath/prioritizers/rdac.c
+++ b/libmultipath/prioritizers/rdac.c
@@ -31,7 +31,7 @@ int rdac_prio(const char *dev, int fd)
io_hdr.dxferp = sense_buffer;
io_hdr.cmdp = inqCmdBlk;
io_hdr.sbp = sb;
- io_hdr.timeout = 60000;
+ io_hdr.timeout = get_prio_timeout(60000);
io_hdr.pack_id = 0;
if (ioctl(fd, SG_IO, &io_hdr) < 0) {
pp_rdac_log(0, "sending inquiry command failed");
diff --git a/multipath.conf.annotated b/multipath.conf.annotated
index 235e130..f158746 100644
--- a/multipath.conf.annotated
+++ b/multipath.conf.annotated
@@ -243,8 +243,9 @@
# #
# # name : checker_timeout
# # scope : multipath & multipathd
-# # desc : The timeout to use for path checkers that issue scsi
-# # commands with an explicit timeout, in seconds.
+# # desc : The timeout to use for path checkers and prioritizers
+# # that issue scsi commands with an explicit timeout, in
+# # seconds.
# # values : n > 0
# # default : taken from /sys/block/sd<x>/device/timeout
# checker_timeout 60
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 1a904e9..195e663 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -330,8 +330,8 @@ maximum number of open fds is taken from the calling process. It is usually
if that number is greated than 1024.
.TP
.B checker_timeout
-Specify the timeout to user for path checkers that issue scsi commands with an
-explicit timeout, in seconds; default taken from
+Specify the timeout to use for path checkers and prioritizers that issue scsi
+commands with an explicit timeout, in seconds; default taken from
.I /sys/block/sd<x>/device/timeout
.TP
.B fast_io_fail_tmo
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 11/12] make prioritizers use checker_timeout, if set
2014-06-30 5:14 ` [PATCH 11/12] make prioritizers use checker_timeout, if set Benjamin Marzinski
@ 2014-07-24 8:49 ` Christophe Varoqui
0 siblings, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-24 8:49 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 7857 bytes --]
Applied,
Thanks.
On Mon, Jun 30, 2014 at 7:14 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> The multipath prioritizers can get stuck issuing scsi commands that
> don't return quickly, just like the checkers. So if checker_timeout
> is set, the prioritizers should should it for their cmd timeouts as
> well.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/prio.c | 7 +++++++
> libmultipath/prio.h | 1 +
> libmultipath/prioritizers/alua_rtpg.c | 5 +++--
> libmultipath/prioritizers/emc.c | 2 +-
> libmultipath/prioritizers/hds.c | 2 +-
> libmultipath/prioritizers/hp_sw.c | 2 +-
> libmultipath/prioritizers/ontap.c | 4 ++--
> libmultipath/prioritizers/rdac.c | 2 +-
> multipath.conf.annotated | 5 +++--
> multipath/multipath.conf.5 | 4 ++--
> 10 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/libmultipath/prio.c b/libmultipath/prio.c
> index 05a8cf1..6ee0b9c 100644
> --- a/libmultipath/prio.c
> +++ b/libmultipath/prio.c
> @@ -10,6 +10,13 @@
>
> static LIST_HEAD(prioritizers);
>
> +unsigned int get_prio_timeout(unsigned int default_timeout)
> +{
> + if (conf->checker_timeout)
> + return conf->checker_timeout * 1000;
> + return default_timeout;
> +}
> +
> int init_prio (void)
> {
> if (!add_prio(DEFAULT_PRIO))
> diff --git a/libmultipath/prio.h b/libmultipath/prio.h
> index 4eeb216..495688f 100644
> --- a/libmultipath/prio.h
> +++ b/libmultipath/prio.h
> @@ -51,6 +51,7 @@ struct prio {
> int (*getprio)(struct path *, char *);
> };
>
> +unsigned int get_prio_timeout(unsigned int default_timeout);
> int init_prio (void);
> void cleanup_prio (void);
> struct prio * add_prio (char *);
> diff --git a/libmultipath/prioritizers/alua_rtpg.c
> b/libmultipath/prioritizers/alua_rtpg.c
> index 981ba06..6d04fc1 100644
> --- a/libmultipath/prioritizers/alua_rtpg.c
> +++ b/libmultipath/prioritizers/alua_rtpg.c
> @@ -21,6 +21,7 @@
> #define __user
> #include <scsi/sg.h>
>
> +#include "../prio.h"
> #include "alua_rtpg.h"
>
> #define SENSE_BUFF_LEN 32
> @@ -134,7 +135,7 @@ do_inquiry(int fd, int evpd, unsigned int codepage,
> void *resp, int resplen)
> hdr.dxfer_len = resplen;
> hdr.sbp = sense;
> hdr.mx_sb_len = sizeof(sense);
> - hdr.timeout = DEF_TIMEOUT;
> + hdr.timeout = get_prio_timeout(DEF_TIMEOUT);
>
> if (ioctl(fd, SG_IO, &hdr) < 0) {
> PRINT_DEBUG("do_inquiry: IOCTL failed!\n");
> @@ -253,7 +254,7 @@ do_rtpg(int fd, void* resp, long resplen)
> hdr.dxfer_len = resplen;
> hdr.mx_sb_len = sizeof(sense);
> hdr.sbp = sense;
> - hdr.timeout = DEF_TIMEOUT;
> + hdr.timeout = get_prio_timeout(DEF_TIMEOUT);
>
> if (ioctl(fd, SG_IO, &hdr) < 0)
> return -RTPG_RTPG_FAILED;
> diff --git a/libmultipath/prioritizers/emc.c
> b/libmultipath/prioritizers/emc.c
> index 91b3d90..e49809c 100644
> --- a/libmultipath/prioritizers/emc.c
> +++ b/libmultipath/prioritizers/emc.c
> @@ -31,7 +31,7 @@ int emc_clariion_prio(const char *dev, int fd)
> io_hdr.dxferp = sense_buffer;
> io_hdr.cmdp = inqCmdBlk;
> io_hdr.sbp = sb;
> - io_hdr.timeout = 60000;
> + io_hdr.timeout = get_prio_timeout(60000);
> io_hdr.pack_id = 0;
> if (ioctl(fd, SG_IO, &io_hdr) < 0) {
> pp_emc_log(0, "sending query command failed");
> diff --git a/libmultipath/prioritizers/hds.c
> b/libmultipath/prioritizers/hds.c
> index f748707..8043b5b 100644
> --- a/libmultipath/prioritizers/hds.c
> +++ b/libmultipath/prioritizers/hds.c
> @@ -114,7 +114,7 @@ int hds_modular_prio (const char *dev, int fd)
> io_hdr.dxferp = inqBuff;
> io_hdr.cmdp = inqCmdBlk;
> io_hdr.sbp = sense_buffer;
> - io_hdr.timeout = 2000; /* TimeOut = 2 seconds */
> + io_hdr.timeout = get_prio_timeout(2000); /* TimeOut = 2 seconds */
>
> if (ioctl (fd, SG_IO, &io_hdr) < 0) {
> pp_hds_log(0, "SG_IO error");
> diff --git a/libmultipath/prioritizers/hp_sw.c
> b/libmultipath/prioritizers/hp_sw.c
> index c24baad..4950cf7 100644
> --- a/libmultipath/prioritizers/hp_sw.c
> +++ b/libmultipath/prioritizers/hp_sw.c
> @@ -46,7 +46,7 @@ int hp_sw_prio(const char *dev, int fd)
> io_hdr.dxfer_direction = SG_DXFER_NONE;
> io_hdr.cmdp = turCmdBlk;
> io_hdr.sbp = sb;
> - io_hdr.timeout = 60000;
> + io_hdr.timeout = get_prio_timeout(60000);
> io_hdr.pack_id = 0;
> retry:
> if (ioctl(fd, SG_IO, &io_hdr) < 0) {
> diff --git a/libmultipath/prioritizers/ontap.c
> b/libmultipath/prioritizers/ontap.c
> index 026d45d..5e82a17 100644
> --- a/libmultipath/prioritizers/ontap.c
> +++ b/libmultipath/prioritizers/ontap.c
> @@ -89,7 +89,7 @@ static int send_gva(const char *dev, int fd, unsigned
> char pg,
> io_hdr.dxferp = results;
> io_hdr.cmdp = cdb;
> io_hdr.sbp = sb;
> - io_hdr.timeout = SG_TIMEOUT;
> + io_hdr.timeout = get_prio_timeout(SG_TIMEOUT);
> io_hdr.pack_id = 0;
> if (ioctl(fd, SG_IO, &io_hdr) < 0) {
> pp_ontap_log(0, "SG_IO ioctl failed, errno=%d", errno);
> @@ -141,7 +141,7 @@ static int get_proxy(const char *dev, int fd)
> io_hdr.dxferp = results;
> io_hdr.cmdp = cdb;
> io_hdr.sbp = sb;
> - io_hdr.timeout = SG_TIMEOUT;
> + io_hdr.timeout = get_prio_timeout(SG_TIMEOUT);
> io_hdr.pack_id = 0;
> if (ioctl(fd, SG_IO, &io_hdr) < 0) {
> pp_ontap_log(0, "ioctl sending inquiry command failed, "
> diff --git a/libmultipath/prioritizers/rdac.c
> b/libmultipath/prioritizers/rdac.c
> index 2bf1443..a210055 100644
> --- a/libmultipath/prioritizers/rdac.c
> +++ b/libmultipath/prioritizers/rdac.c
> @@ -31,7 +31,7 @@ int rdac_prio(const char *dev, int fd)
> io_hdr.dxferp = sense_buffer;
> io_hdr.cmdp = inqCmdBlk;
> io_hdr.sbp = sb;
> - io_hdr.timeout = 60000;
> + io_hdr.timeout = get_prio_timeout(60000);
> io_hdr.pack_id = 0;
> if (ioctl(fd, SG_IO, &io_hdr) < 0) {
> pp_rdac_log(0, "sending inquiry command failed");
> diff --git a/multipath.conf.annotated b/multipath.conf.annotated
> index 235e130..f158746 100644
> --- a/multipath.conf.annotated
> +++ b/multipath.conf.annotated
> @@ -243,8 +243,9 @@
> # #
> # # name : checker_timeout
> # # scope : multipath & multipathd
> -# # desc : The timeout to use for path checkers that issue scsi
> -# # commands with an explicit timeout, in seconds.
> +# # desc : The timeout to use for path checkers and prioritizers
> +# # that issue scsi commands with an explicit timeout, in
> +# # seconds.
> # # values : n > 0
> # # default : taken from /sys/block/sd<x>/device/timeout
> # checker_timeout 60
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 1a904e9..195e663 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -330,8 +330,8 @@ maximum number of open fds is taken from the calling
> process. It is usually
> if that number is greated than 1024.
> .TP
> .B checker_timeout
> -Specify the timeout to user for path checkers that issue scsi commands
> with an
> -explicit timeout, in seconds; default taken from
> +Specify the timeout to use for path checkers and prioritizers that issue
> scsi
> +commands with an explicit timeout, in seconds; default taken from
> .I /sys/block/sd<x>/device/timeout
> .TP
> .B fast_io_fail_tmo
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 9459 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 12/12] Add multipath.conf force_sync option
2014-06-30 5:13 [PATCH 00/12] miscellaneous multipath patches Benjamin Marzinski
` (10 preceding siblings ...)
2014-06-30 5:14 ` [PATCH 11/12] make prioritizers use checker_timeout, if set Benjamin Marzinski
@ 2014-06-30 5:14 ` Benjamin Marzinski
2014-07-24 8:51 ` Christophe Varoqui
11 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2014-06-30 5:14 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
Normally multipathd runs the path checkers asynchronously. However if there
are a lot of paths, this can cause large CPU spikes (for instance, in
cases where they are all competing for the Big Kernel Lock). In these
situations, overall machine performance is better if multipath doesn't have
hundreds or even thousands of path checkers running at the same time. This
patch lets users turn off the asynchronous mode of these checks if they
see this problem.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/config.c | 1 +
libmultipath/config.h | 1 +
libmultipath/dict.c | 33 +++++++++++++++++++++++++++++++++
libmultipath/discovery.c | 8 ++++++--
multipath.conf.annotated | 11 +++++++++++
multipath/multipath.conf.5 | 9 +++++++++
6 files changed, 61 insertions(+), 2 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index e13c307..39963b4 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -561,6 +561,7 @@ load_config (char * file, struct udev *udev)
conf->fast_io_fail = DEFAULT_FAST_IO_FAIL;
conf->retain_hwhandler = DEFAULT_RETAIN_HWHANDLER;
conf->detect_prio = DEFAULT_DETECT_PRIO;
+ conf->force_sync = 0;
/*
* preload default hwtable
diff --git a/libmultipath/config.h b/libmultipath/config.h
index ac7c58e..1a23e4b 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -124,6 +124,7 @@ struct config {
int reassign_maps;
int retain_hwhandler;
int detect_prio;
+ int force_sync;
unsigned int version[3];
char * dev;
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 91d9b83..7de7a67 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -685,6 +685,29 @@ def_detect_prio_handler(vector strvec)
return 0;
}
+static int
+def_force_sync_handler(vector strvec)
+{
+ char * buff;
+
+ buff = set_value(strvec);
+
+ if (!buff)
+ return 1;
+
+ if ((strlen(buff) == 2 && !strcmp(buff, "no")) ||
+ (strlen(buff) == 1 && !strcmp(buff, "0")))
+ conf->force_sync = 0;
+ else if ((strlen(buff) == 3 && !strcmp(buff, "yes")) ||
+ (strlen(buff) == 1 && !strcmp(buff, "1")))
+ conf->force_sync = 1;
+ else
+ conf->force_sync = 0;
+
+ FREE(buff);
+ return 0;
+}
+
/*
* blacklist block handlers
*/
@@ -2783,6 +2806,15 @@ snprint_def_detect_prio(char * buff, int len, void * data)
}
static int
+snprint_def_force_sync(char * buff, int len, void * data)
+{
+ if (conf->force_sync)
+ return snprintf(buff, len, "yes");
+ else
+ return snprintf(buff, len, "no");
+}
+
+static int
snprint_ble_simple (char * buff, int len, void * data)
{
struct blentry * ble = (struct blentry *)data;
@@ -2849,6 +2881,7 @@ init_keywords(void)
install_keyword("reservation_key", &def_reservation_key_handler, &snprint_def_reservation_key);
install_keyword("retain_attached_hw_handler", &def_retain_hwhandler_handler, &snprint_def_retain_hwhandler_handler);
install_keyword("detect_prio", &def_detect_prio_handler, &snprint_def_detect_prio);
+ install_keyword("force_sync", &def_force_sync_handler, &snprint_def_force_sync);
__deprecated install_keyword("default_selector", &def_selector_handler, NULL);
__deprecated install_keyword("default_path_grouping_policy", &def_pgpolicy_handler, NULL);
__deprecated install_keyword("default_uid_attribute", &def_uid_attribute_handler, NULL);
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index b62a59c..a5f5a20 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1053,8 +1053,12 @@ get_state (struct path * pp, int daemon)
}
}
checker_clear_message(c);
- if (daemon)
- checker_set_async(c);
+ if (daemon) {
+ if (conf->force_sync == 0)
+ checker_set_async(c);
+ else
+ checker_set_sync(c);
+ }
if (!conf->checker_timeout &&
sysfs_get_timeout(pp, &(c->timeout)) <= 0)
c->timeout = DEF_TIMEOUT;
diff --git a/multipath.conf.annotated b/multipath.conf.annotated
index f158746..0af1d4c 100644
--- a/multipath.conf.annotated
+++ b/multipath.conf.annotated
@@ -270,6 +270,7 @@
# # default : determined by the OS
# dev_loss_tmo 600
#
+# #
# # name : bindings_file
# # scope : multipath
# # desc : The location of the bindings file that is used with
@@ -278,6 +279,7 @@
# # default : "/var/lib/multipath/bindings"
# bindings_file "/etc/multipath/bindings"
#
+# #
# # name : wwids_file
# # scope : multipath
# # desc : The location of the wwids file multipath uses to
@@ -286,6 +288,7 @@
# # default : "/var/lib/multipath/wwids"
# wwids_file "/etc/multipath/wwids"
#
+# #
# # name : reservation_key
# # scope : multipath
# # desc : Service action reservation key used by mpathpersist.
@@ -293,6 +296,14 @@
# # default : (null)
# reservation_key "mpathkey"
#
+# #
+# # name : force_sync
+# # scope : multipathd
+# # desc : If set to yes, multipath will run all of the checkers in
+# # sync mode, even if the checker has an async mode.
+# # values : yes|no
+# # default : no
+# force_sync yes
#}
#
##
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 195e663..cadb34d 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -409,6 +409,15 @@ will automatically use the
.I alua
prioritizer. If not, the prioritizer will be selected as usual. Default is
.I no
+.TP
+.B force_sync
+If set to
+.I yes
+, multipathd will call the path checkers in sync mode only. This means that
+only one checker will run at a time. This is useful in the case where many
+multipathd checkers running in parallel causes significant CPU pressure. The
+Default is
+.I no
.
.SH "blacklist section"
The
--
1.8.3.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 12/12] Add multipath.conf force_sync option
2014-06-30 5:14 ` [PATCH 12/12] Add multipath.conf force_sync option Benjamin Marzinski
@ 2014-07-24 8:51 ` Christophe Varoqui
0 siblings, 0 replies; 31+ messages in thread
From: Christophe Varoqui @ 2014-07-24 8:51 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: device-mapper development
[-- Attachment #1.1: Type: text/plain, Size: 6717 bytes --]
Applied,
Thanks.
On Mon, Jun 30, 2014 at 7:14 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:
> Normally multipathd runs the path checkers asynchronously. However if there
> are a lot of paths, this can cause large CPU spikes (for instance, in
> cases where they are all competing for the Big Kernel Lock). In these
> situations, overall machine performance is better if multipath doesn't have
> hundreds or even thousands of path checkers running at the same time. This
> patch lets users turn off the asynchronous mode of these checks if they
> see this problem.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/config.c | 1 +
> libmultipath/config.h | 1 +
> libmultipath/dict.c | 33 +++++++++++++++++++++++++++++++++
> libmultipath/discovery.c | 8 ++++++--
> multipath.conf.annotated | 11 +++++++++++
> multipath/multipath.conf.5 | 9 +++++++++
> 6 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index e13c307..39963b4 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -561,6 +561,7 @@ load_config (char * file, struct udev *udev)
> conf->fast_io_fail = DEFAULT_FAST_IO_FAIL;
> conf->retain_hwhandler = DEFAULT_RETAIN_HWHANDLER;
> conf->detect_prio = DEFAULT_DETECT_PRIO;
> + conf->force_sync = 0;
>
> /*
> * preload default hwtable
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index ac7c58e..1a23e4b 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -124,6 +124,7 @@ struct config {
> int reassign_maps;
> int retain_hwhandler;
> int detect_prio;
> + int force_sync;
> unsigned int version[3];
>
> char * dev;
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index 91d9b83..7de7a67 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -685,6 +685,29 @@ def_detect_prio_handler(vector strvec)
> return 0;
> }
>
> +static int
> +def_force_sync_handler(vector strvec)
> +{
> + char * buff;
> +
> + buff = set_value(strvec);
> +
> + if (!buff)
> + return 1;
> +
> + if ((strlen(buff) == 2 && !strcmp(buff, "no")) ||
> + (strlen(buff) == 1 && !strcmp(buff, "0")))
> + conf->force_sync = 0;
> + else if ((strlen(buff) == 3 && !strcmp(buff, "yes")) ||
> + (strlen(buff) == 1 && !strcmp(buff, "1")))
> + conf->force_sync = 1;
> + else
> + conf->force_sync = 0;
> +
> + FREE(buff);
> + return 0;
> +}
> +
> /*
> * blacklist block handlers
> */
> @@ -2783,6 +2806,15 @@ snprint_def_detect_prio(char * buff, int len, void
> * data)
> }
>
> static int
> +snprint_def_force_sync(char * buff, int len, void * data)
> +{
> + if (conf->force_sync)
> + return snprintf(buff, len, "yes");
> + else
> + return snprintf(buff, len, "no");
> +}
> +
> +static int
> snprint_ble_simple (char * buff, int len, void * data)
> {
> struct blentry * ble = (struct blentry *)data;
> @@ -2849,6 +2881,7 @@ init_keywords(void)
> install_keyword("reservation_key", &def_reservation_key_handler,
> &snprint_def_reservation_key);
> install_keyword("retain_attached_hw_handler",
> &def_retain_hwhandler_handler, &snprint_def_retain_hwhandler_handler);
> install_keyword("detect_prio", &def_detect_prio_handler,
> &snprint_def_detect_prio);
> + install_keyword("force_sync", &def_force_sync_handler,
> &snprint_def_force_sync);
> __deprecated install_keyword("default_selector",
> &def_selector_handler, NULL);
> __deprecated install_keyword("default_path_grouping_policy",
> &def_pgpolicy_handler, NULL);
> __deprecated install_keyword("default_uid_attribute",
> &def_uid_attribute_handler, NULL);
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index b62a59c..a5f5a20 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1053,8 +1053,12 @@ get_state (struct path * pp, int daemon)
> }
> }
> checker_clear_message(c);
> - if (daemon)
> - checker_set_async(c);
> + if (daemon) {
> + if (conf->force_sync == 0)
> + checker_set_async(c);
> + else
> + checker_set_sync(c);
> + }
> if (!conf->checker_timeout &&
> sysfs_get_timeout(pp, &(c->timeout)) <= 0)
> c->timeout = DEF_TIMEOUT;
> diff --git a/multipath.conf.annotated b/multipath.conf.annotated
> index f158746..0af1d4c 100644
> --- a/multipath.conf.annotated
> +++ b/multipath.conf.annotated
> @@ -270,6 +270,7 @@
> # # default : determined by the OS
> # dev_loss_tmo 600
> #
> +# #
> # # name : bindings_file
> # # scope : multipath
> # # desc : The location of the bindings file that is used with
> @@ -278,6 +279,7 @@
> # # default : "/var/lib/multipath/bindings"
> # bindings_file "/etc/multipath/bindings"
> #
> +# #
> # # name : wwids_file
> # # scope : multipath
> # # desc : The location of the wwids file multipath uses to
> @@ -286,6 +288,7 @@
> # # default : "/var/lib/multipath/wwids"
> # wwids_file "/etc/multipath/wwids"
> #
> +# #
> # # name : reservation_key
> # # scope : multipath
> # # desc : Service action reservation key used by mpathpersist.
> @@ -293,6 +296,14 @@
> # # default : (null)
> # reservation_key "mpathkey"
> #
> +# #
> +# # name : force_sync
> +# # scope : multipathd
> +# # desc : If set to yes, multipath will run all of the checkers
> in
> +# # sync mode, even if the checker has an async mode.
> +# # values : yes|no
> +# # default : no
> +# force_sync yes
> #}
> #
> ##
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 195e663..cadb34d 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -409,6 +409,15 @@ will automatically use the
> .I alua
> prioritizer. If not, the prioritizer will be selected as usual. Default is
> .I no
> +.TP
> +.B force_sync
> +If set to
> +.I yes
> +, multipathd will call the path checkers in sync mode only. This means
> that
> +only one checker will run at a time. This is useful in the case where
> many
> +multipathd checkers running in parallel causes significant CPU pressure.
> The
> +Default is
> +.I no
> .
> .SH "blacklist section"
> The
> --
> 1.8.3.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 8413 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread