* [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices
@ 2017-01-12 5:52 tang.junhui
2017-01-12 5:52 ` [PATCH 01/11] libmultipath: add merge_id in "struct uevent" for uevents merging tang.junhui
` (10 more replies)
0 siblings, 11 replies; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
Hello Christophe, Ben, Hannes, Martin, Bart, and other guys:
This series of patches are modified base on:
[PATCH 00/12] multipath-tools: improve processing efficiency for addition
and deletion of multipath devices
Modifications include follows:
Martin suggested me to use named constants rather than explicit numbers
Thanks to Martin.
And Ben made several good suggestions:
1) Use *_safe list literation in traversing a list while removing elements
2) Do not move uevent_can_discard() to uevents listening thread in avoiding
of socket's buffer fills up
3) Filter path change uevents while path addition uevent having not ever
been processed
4) Remove add and change uevents if they are followed by a remove event,
regardless of whether or not they have a wwid
5) Do not change how the wwid is set, and call domap() on the device if its
wwid is different from its merge_id.
Thanks to Ben too.
The eleven patches in this series are used to improve processing
efficiency for addition and deletion of multipath devices.
This patches are tested pass by ZTE multipath automatic testing syetem.
The modification reduces the system consumption(such as CPU) and shortens
the processing time obviously in scene of massive multipath devices
addition or deletion.
The main processing flow of code is:
1)uevents accumulation in uevents burst scene:
--[pacth]libmultipath: wait one seconds for more uevents in
uevent_listen() in uevents burst situations
2)uevents filtering and merging:
--[pacth]multipathd: merge uevents before proccessing
--[pacth]libmultipath: filter uevents before proccessing
--[patch]multipathd: move uev_discard() to uevent.c and change its name
to uevent_can_discard()
--[patch]multipathd: move calling filter_devnode() from uev_trigger() to
uevent_can_discard()
3)uevents proccessing:
--[pacth]multipathd: add need_do_map to indicate whether need calling
domap() in ev_add_path()
--[pacth]multipathd: add need_do_map to indicate whether need calling
domap() in ev_remove_path()
--[pacth]multipathd: proccess merged uevents
There are some patches providing basic struct members or maros which
would used in above pathes:
--[patch]libmultipath: add merge_id in "struct uevent" for uevents merging
--[patch]libmultipath: add merge_node for "struct uevent" to record
nodes of merged uevents
--[patch]libmultipath: add three list iteration macros
Any comment will be welcome, and it would be appreciated if these
patches would be considered for inclusion in the upstream
multipath-tools.
Thank you all,
Tang Junhui
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 01/11] libmultipath: add merge_id in "struct uevent" for uevents merging
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
2017-01-12 5:52 ` [PATCH 02/11] libmultipath: add merge_node for "struct uevent" to record nodes of merged uevents tang.junhui
` (9 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
merge_id is used for uevents merging, it is expected to be same with wwid,
uevents with the same merge_id will be merged, and if it is same with
wwid, only the last uevent will call domap(). now we only merge uevents
with label ID_SERIAL or ID_UID in uevent, which usually comming from SCSI
or DASD device.
Change-Id: Ie6b076363b3735dc7de10184b27fa799b499af0e
Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
libmultipath/uevent.c | 16 ++++++++++++++++
libmultipath/uevent.h | 1 +
2 files changed, 17 insertions(+)
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 7edcce1..5bde864 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -424,6 +424,22 @@ struct uevent *uevent_from_udev_device(struct udev_device *dev)
uev->devpath = uev->envp[i] + 8;
if (strcmp(name, "ACTION") == 0)
uev->action = uev->envp[i] + 7;
+
+ /*
+ * merge_id used for uevents merging
+ * it is expected to be same with wwid,
+ * uevents with the same merge_id
+ * will be merged, and if it is same with wwid
+ * only the last uevent will call domap()
+ * now we only merge uevents with
+ * label ID_SERIAL or ID_UID, which usually comming
+ * from SCSI or DASD device
+ */
+ if (strcmp(name, "ID_SERIAL") == 0)
+ uev->merge_id = uev->envp[i] + 10;
+ else if (strcmp(name, "ID_UID") == 0)
+ uev->merge_id = uev->envp[i] + 7;
+
i++;
if (i == HOTPLUG_NUM_ENVP - 1)
break;
diff --git a/libmultipath/uevent.h b/libmultipath/uevent.h
index 9d22dcd..3f83bab 100644
--- a/libmultipath/uevent.h
+++ b/libmultipath/uevent.h
@@ -22,6 +22,7 @@ struct uevent {
char *devpath;
char *action;
char *kernel;
+ char *merge_id;
unsigned long seqnum;
char *envp[HOTPLUG_NUM_ENVP];
};
--
2.8.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 02/11] libmultipath: add merge_node for "struct uevent" to record nodes of merged uevents
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
2017-01-12 5:52 ` [PATCH 01/11] libmultipath: add merge_id in "struct uevent" for uevents merging tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
2017-01-12 5:52 ` [PATCH 03/11] libmultipath: add three list iteration macros tang.junhui
` (8 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
Add merged nodes list to store nodes of merged uevents. By Adding
this member, after merging, the list of uevents would be linked like
this:
uevent
---------------------------
|struct list_head node |----->list node of un-merged uevents
---------------------------
|struct list_head merge_node|----->list node of merged uevents, which
---------------------------- moved from the origin un-merged list
|... |
---------------------------
Change-Id: I5fbfc7656ede77e03ca35c855212e2d2d60706b2
Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
libmultipath/uevent.c | 4 +++-
libmultipath/uevent.h | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 5bde864..9b6b1d1 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -72,8 +72,10 @@ struct uevent * alloc_uevent (void)
{
struct uevent *uev = MALLOC(sizeof(struct uevent));
- if (uev)
+ if (uev) {
INIT_LIST_HEAD(&uev->node);
+ INIT_LIST_HEAD(&uev->merge_node);
+ }
return uev;
}
diff --git a/libmultipath/uevent.h b/libmultipath/uevent.h
index 3f83bab..9f65327 100644
--- a/libmultipath/uevent.h
+++ b/libmultipath/uevent.h
@@ -17,6 +17,7 @@ struct udev;
struct uevent {
struct list_head node;
+ struct list_head merge_node;
struct udev_device *udev;
char buffer[HOTPLUG_BUFFER_SIZE + OBJECT_SIZE];
char *devpath;
--
2.8.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 03/11] libmultipath: add three list iteration macros
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
2017-01-12 5:52 ` [PATCH 01/11] libmultipath: add merge_id in "struct uevent" for uevents merging tang.junhui
2017-01-12 5:52 ` [PATCH 02/11] libmultipath: add merge_node for "struct uevent" to record nodes of merged uevents tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
2017-01-12 5:52 ` [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path() tang.junhui
` (7 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
Add three list iteration macros, list_for_each_entry_reverse_safe
is used for safe list iteration, and the other two macros are used
to iterate list forwards or backwards from the given begin node to
the given end node, which would be used in merging uevents.
Change-Id: I8bb53fef9276bb62a5e0f4fdac6455086dc03d9b
Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
libmultipath/list.h | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/libmultipath/list.h b/libmultipath/list.h
index ceaa381..4433167 100644
--- a/libmultipath/list.h
+++ b/libmultipath/list.h
@@ -317,4 +317,45 @@ static inline void list_splice_tail_init(struct list_head *list,
&pos->member != (head); \
pos = n, n = list_entry(n->member.next, typeof(*n), member))
+/**
+ * list_for_each_entry_reverse - iterate backwards over list of given type.
+ * @pos: the type * to use as a loop counter.
+ * @n: another type * to use as temporary storage
+ * @head: the head for your list.
+ * @member: the name of the list_struct within the struct.
+ */
+#define list_for_each_entry_reverse_safe(pos, n, head, member) \
+ for (pos = list_entry((head)->prev, typeof(*pos), member), \
+ n = list_entry(pos->member.prev, typeof(*pos), member); \
+ &pos->member != (head); \
+ pos = n, n = list_entry(n->member.prev, typeof(*n), member))
+
+/**
+ * list_for_some_entry - iterate list of given interval
+ * @pos: the type * to use as a loop counter.
+ * @n: another type * to use as temporary storage
+ * @from: the begin node of the iteration.
+ * @to: the end node of the iteration.
+ * @member: the name of the list_struct within the struct.
+ */
+#define list_for_some_entry_safe(pos, n, from, to, member) \
+ for (pos = list_entry((from)->next, typeof(*pos), member), \
+ n = list_entry(pos->member.next, typeof(*pos), member); \
+ &pos->member != (to); \
+ pos = n, n = list_entry(n->member.next, typeof(*n), member))
+
+/**
+ * list_for_some_entry_reverse - iterate backwards list of given interval
+ * @pos: the type * to use as a loop counter.
+ * @n: another type * to use as temporary storage
+ * @from: the begin node of the iteration.
+ * @to: the end node of the iteration.
+ * @member: the name of the list_struct within the struct.
+ */
+#define list_for_some_entry_reverse_safe(pos, n, from, to, member) \
+ for (pos = list_entry((from)->prev, typeof(*pos), member), \
+ n = list_entry(pos->member.prev, typeof(*pos), member); \
+ &pos->member != (to); \
+ pos = n, n = list_entry(n->member.prev, typeof(*n), member))
+
#endif /* _LIST_H */
--
2.8.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path()
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
` (2 preceding siblings ...)
2017-01-12 5:52 ` [PATCH 03/11] libmultipath: add three list iteration macros tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
2017-01-16 21:38 ` Benjamin Marzinski
2017-01-12 5:52 ` [PATCH 05/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_remove_path() tang.junhui
` (6 subsequent siblings)
10 siblings, 1 reply; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
Usually calling domap() in ev_add_path() is needed, but only last path
need to call domap() in processing for merged uevents to reduce the
count of calling domap() and promote efficiency. So add input parameter
need_do_map to indicate whether need calling domap() in ev_add_path().
Change-Id: I7f33de49a1d89d91180dcb95cd94e4944ae7ff36
Signed-off-by: tang.wenjun <tang.wenjun3@zte.com.cn>
---
multipathd/cli_handlers.c | 3 ++-
multipathd/main.c | 47 ++++++++++++++++++++++++++++++++++++-----------
multipathd/main.h | 2 +-
3 files changed, 39 insertions(+), 13 deletions(-)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index b0eeca6..3a46c09 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -670,7 +670,8 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
pp->checkint = conf->checkint;
}
put_multipath_config(conf);
- return ev_add_path(pp, vecs);
+ return ev_add_path(pp, vecs, 1);
+
blacklisted:
*reply = strdup("blacklisted\n");
*len = strlen(*reply) + 1;
diff --git a/multipathd/main.c b/multipathd/main.c
index adc3258..ebd7de1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -608,7 +608,7 @@ ev_remove_map (char * devname, char * alias, int minor, struct vectors * vecs)
}
static int
-uev_add_path (struct uevent *uev, struct vectors * vecs)
+uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
{
struct path *pp;
int ret = 0, i;
@@ -640,9 +640,18 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
r = pathinfo(pp, conf,
DI_ALL | DI_BLACKLIST);
put_multipath_config(conf);
- if (r == PATHINFO_OK)
- ret = ev_add_path(pp, vecs);
- else if (r == PATHINFO_SKIPPED) {
+ if (r == PATHINFO_OK) {
+ /*
+ * Make sure merging use the correct wwid
+ * Othterwise calling domap()
+ */
+ if (!need_do_map &&
+ uev->merge_id &&
+ strcmp(uev->merge_id, pp->wwid))
+ need_do_map = 1;
+
+ ret = ev_add_path(pp, vecs, need_do_map);
+ } else if (r == PATHINFO_SKIPPED) {
condlog(3, "%s: remove blacklisted path",
uev->kernel);
i = find_slot(vecs->pathvec, (void *)pp);
@@ -681,7 +690,16 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
conf = get_multipath_config();
pp->checkint = conf->checkint;
put_multipath_config(conf);
- ret = ev_add_path(pp, vecs);
+ /*
+ * Make sure merging use the correct wwid
+ * Othterwise calling domap()
+ */
+ if (!need_do_map &&
+ uev->merge_id &&
+ strcmp(uev->merge_id, pp->wwid))
+ need_do_map = 1;
+
+ ret = ev_add_path(pp, vecs, need_do_map);
} else {
condlog(0, "%s: failed to store path info, "
"dropping event",
@@ -699,7 +717,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
* 1: error
*/
int
-ev_add_path (struct path * pp, struct vectors * vecs)
+ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
{
struct multipath * mpp;
char params[PARAMS_SIZE] = {0};
@@ -767,6 +785,13 @@ rescan:
/* persistent reservation check*/
mpath_pr_event_handle(pp);
+ if (!need_do_map)
+ return 0;
+
+ if (!dm_map_present(mpp->alias)) {
+ mpp->action = ACT_CREATE;
+ start_waiter = 1;
+ }
/*
* push the map to the device-mapper
*/
@@ -995,7 +1020,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
}
if (pp->initialized == INIT_REQUESTED_UDEV)
- retval = uev_add_path(uev, vecs);
+ retval = uev_add_path(uev, vecs, 1);
else if (mpp && ro >= 0) {
condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro);
@@ -1150,7 +1175,7 @@ uev_trigger (struct uevent * uev, void * trigger_data)
put_multipath_config(conf);
if (!strncmp(uev->action, "add", 3)) {
- r = uev_add_path(uev, vecs);
+ r = uev_add_path(uev, vecs, 1);
goto out;
}
if (!strncmp(uev->action, "remove", 6)) {
@@ -1570,7 +1595,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
conf = get_multipath_config();
ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
if (ret == PATHINFO_OK) {
- ev_add_path(pp, vecs);
+ ev_add_path(pp, vecs, 1);
pp->tick = 1;
} else if (ret == PATHINFO_SKIPPED) {
put_multipath_config(conf);
@@ -1686,7 +1711,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
}
if (!disable_reinstate && reinstate_path(pp, add_active)) {
condlog(3, "%s: reload map", pp->dev);
- ev_add_path(pp, vecs);
+ ev_add_path(pp, vecs, 1);
pp->tick = 1;
return 0;
}
@@ -1709,7 +1734,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
/* Clear IO errors */
if (reinstate_path(pp, 0)) {
condlog(3, "%s: reload map", pp->dev);
- ev_add_path(pp, vecs);
+ ev_add_path(pp, vecs, 1);
pp->tick = 1;
return 0;
}
diff --git a/multipathd/main.h b/multipathd/main.h
index f72580d..f810d41 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -22,7 +22,7 @@ void exit_daemon(void);
const char * daemon_status(void);
int need_to_delay_reconfig (struct vectors *);
int reconfigure (struct vectors *);
-int ev_add_path (struct path *, struct vectors *);
+int ev_add_path (struct path *, struct vectors *, int);
int ev_remove_path (struct path *, struct vectors *);
int ev_add_map (char *, char *, struct vectors *);
int ev_remove_map (char *, char *, int, struct vectors *);
--
2.8.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 05/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_remove_path()
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
` (3 preceding siblings ...)
2017-01-12 5:52 ` [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path() tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
2017-01-16 18:18 ` Benjamin Marzinski
2017-01-12 5:52 ` [PATCH 06/11] multipathd: move uev_discard() to uevent.c and change its name to uevent_can_discard() tang.junhui
` (5 subsequent siblings)
10 siblings, 1 reply; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
Usually calling domap() in ev_remove_path() is needed, but only last
path need to call domap() in processing for merged uevents to reduce the
count of calling domap() and promote efficiency. So add input parameter
need_do_map to indicate whether need calling domap() in ev_remove_path().
Change-Id: I0a787330a249608396cc3e655465dc820f940539
Signed-off-by: tang.wenjun <tang.wenjun3@zte.com.cn>
---
multipathd/cli_handlers.c | 2 +-
multipathd/main.c | 23 ++++++++++++++++++-----
multipathd/main.h | 2 +-
3 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 3a46c09..302fd02 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -693,7 +693,7 @@ cli_del_path (void * v, char ** reply, int * len, void * data)
condlog(0, "%s: path already removed", param);
return 1;
}
- return ev_remove_path(pp, vecs);
+ return ev_remove_path(pp, vecs, 1);
}
int
diff --git a/multipathd/main.c b/multipathd/main.c
index ebd7de1..718c5e7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -858,7 +858,7 @@ fail:
}
static int
-uev_remove_path (struct uevent *uev, struct vectors * vecs)
+uev_remove_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
{
struct path *pp;
int ret;
@@ -868,8 +868,18 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs)
lock(&vecs->lock);
pthread_testcancel();
pp = find_path_by_dev(vecs->pathvec, uev->kernel);
- if (pp)
- ret = ev_remove_path(pp, vecs);
+ if (pp) {
+ /*
+ * Make sure merging use the correct wwid
+ * Othterwise calling domap()
+ */
+ if (!need_do_map &&
+ uev->merge_id &&
+ strcmp(uev->merge_id, pp->wwid))
+ need_do_map = 1;
+
+ ret = ev_remove_path(pp, vecs, need_do_map);
+ }
lock_cleanup_pop(vecs->lock);
if (!pp) {
/* Not an error; path might have been purged earlier */
@@ -880,7 +890,7 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs)
}
int
-ev_remove_path (struct path *pp, struct vectors * vecs)
+ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
{
struct multipath * mpp;
int i, retval = 0;
@@ -902,6 +912,9 @@ ev_remove_path (struct path *pp, struct vectors * vecs)
if ((i = find_slot(mpp->paths, (void *)pp)) != -1)
vector_del_slot(mpp->paths, i);
+ if(!need_do_map)
+ goto out;
+
/*
* remove the map IFF removing the last path
*/
@@ -1179,7 +1192,7 @@ uev_trigger (struct uevent * uev, void * trigger_data)
goto out;
}
if (!strncmp(uev->action, "remove", 6)) {
- r = uev_remove_path(uev, vecs);
+ r = uev_remove_path(uev, vecs, 1);
goto out;
}
if (!strncmp(uev->action, "change", 6)) {
diff --git a/multipathd/main.h b/multipathd/main.h
index f810d41..094b04f 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -23,7 +23,7 @@ const char * daemon_status(void);
int need_to_delay_reconfig (struct vectors *);
int reconfigure (struct vectors *);
int ev_add_path (struct path *, struct vectors *, int);
-int ev_remove_path (struct path *, struct vectors *);
+int ev_remove_path (struct path *, struct vectors *, int);
int ev_add_map (char *, char *, struct vectors *);
int ev_remove_map (char *, char *, int, struct vectors *);
void sync_map_state (struct multipath *);
--
2.8.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 06/11] multipathd: move uev_discard() to uevent.c and change its name to uevent_can_discard()
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
` (4 preceding siblings ...)
2017-01-12 5:52 ` [PATCH 05/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_remove_path() tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
2017-01-12 5:52 ` [PATCH 07/11] multipathd: move calling filter_devnode() from uev_trigger() " tang.junhui
` (4 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
This patch is mainly done to adjust the code for the preparation of
uevents merging.
Change-Id: Iaac159ffe3930e53c3325d1069c3ed497e440c0c
Signed-off-by: tang.wenjun <tang.wenjun3@zte.com.cn>
---
libmultipath/uevent.c | 40 ++++++++++++++++++++++++++++++++++++++++
multipathd/main.c | 25 -------------------------
2 files changed, 40 insertions(+), 25 deletions(-)
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 9b6b1d1..d6c02a6 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -24,6 +24,7 @@
#include <unistd.h>
#include <stdio.h>
+#include <stdbool.h>
#include <errno.h>
#include <stdlib.h>
#include <stddef.h>
@@ -80,6 +81,44 @@ struct uevent * alloc_uevent (void)
return uev;
}
+bool
+uevent_can_discard(struct uevent *uev)
+{
+ char *tmp;
+ char a[11], b[11];
+
+ /*
+ * keep only block devices, discard partitions
+ */
+ tmp = strstr(uev->devpath, "/block/");
+ if (tmp == NULL){
+ condlog(4, "no /block/ in '%s'", uev->devpath);
+ return true;
+ }
+ if (sscanf(tmp, "/block/%10s", a) != 1 ||
+ sscanf(tmp, "/block/%10[^/]/%10s", a, b) == 2) {
+ condlog(4, "discard event on %s", uev->devpath);
+ return true;
+ }
+
+ return false;
+}
+
+void
+uevent_discard(struct list_head *tmpq)
+{
+ struct uevent *uev, *tmp;
+
+ list_for_each_entry_reverse_safe(uev, tmp, tmpq, node) {
+ if (uevent_can_discard(uev)) {
+ list_del_init(&uev->node);
+ if (uev->udev)
+ udev_device_unref(uev->udev);
+ FREE(uev);
+ }
+ }
+}
+
void
service_uevq(struct list_head *tmpq)
{
@@ -144,6 +183,7 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
pthread_mutex_unlock(uevq_lockp);
if (!my_uev_trigger)
break;
+ uevent_discard(&uevq_tmp);
service_uevq(&uevq_tmp);
}
condlog(3, "Terminating uev service queue");
diff --git a/multipathd/main.c b/multipathd/main.c
index 718c5e7..66d5c3d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1115,28 +1115,6 @@ uxsock_trigger (char * str, char ** reply, int * len, void * trigger_data)
return r;
}
-static int
-uev_discard(char * devpath)
-{
- char *tmp;
- char a[11], b[11];
-
- /*
- * keep only block devices, discard partitions
- */
- tmp = strstr(devpath, "/block/");
- if (tmp == NULL){
- condlog(4, "no /block/ in '%s'", devpath);
- return 1;
- }
- if (sscanf(tmp, "/block/%10s", a) != 1 ||
- sscanf(tmp, "/block/%10[^/]/%10s", a, b) == 2) {
- condlog(4, "discard event on %s", devpath);
- return 1;
- }
- return 0;
-}
-
int
uev_trigger (struct uevent * uev, void * trigger_data)
{
@@ -1146,9 +1124,6 @@ uev_trigger (struct uevent * uev, void * trigger_data)
vecs = (struct vectors *)trigger_data;
- if (uev_discard(uev->devpath))
- return 0;
-
pthread_cleanup_push(config_cleanup, NULL);
pthread_mutex_lock(&config_lock);
if (running_state != DAEMON_IDLE &&
--
2.8.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 07/11] multipathd: move calling filter_devnode() from uev_trigger() to uevent_can_discard()
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
` (5 preceding siblings ...)
2017-01-12 5:52 ` [PATCH 06/11] multipathd: move uev_discard() to uevent.c and change its name to uevent_can_discard() tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
2017-01-12 5:52 ` [PATCH 08/11] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations tang.junhui
` (3 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
Move calling filter_devnode() from uev_trigger() to uevent_can_discard()
since they do the similar work.
Change-Id: I0322443fa551b21aa3211bf1ce3fad7d37aeeab4
Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
libmultipath/uevent.c | 20 ++++++++++++++++++++
multipathd/main.c | 9 ---------
2 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index d6c02a6..b560a50 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -47,6 +47,9 @@
#include "list.h"
#include "uevent.h"
#include "vector.h"
+#include "structs.h"
+#include "config.h"
+#include "blacklist.h"
typedef int (uev_trigger)(struct uevent *, void * trigger_data);
@@ -86,6 +89,7 @@ uevent_can_discard(struct uevent *uev)
{
char *tmp;
char a[11], b[11];
+ struct config * conf;
/*
* keep only block devices, discard partitions
@@ -101,6 +105,22 @@ uevent_can_discard(struct uevent *uev)
return true;
}
+ /*
+ * do not filter dm devices by devnode
+ */
+ if (!strncmp(uev->kernel, "dm-", 3))
+ return false;
+ /*
+ * filter paths devices by devnode
+ */
+ conf = get_multipath_config();
+ if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
+ uev->kernel) > 0) {
+ put_multipath_config(conf);
+ return true;
+ }
+ put_multipath_config(conf);
+
return false;
}
diff --git a/multipathd/main.c b/multipathd/main.c
index 66d5c3d..24116e3 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1120,7 +1120,6 @@ uev_trigger (struct uevent * uev, void * trigger_data)
{
int r = 0;
struct vectors * vecs;
- struct config *conf;
vecs = (struct vectors *)trigger_data;
@@ -1154,14 +1153,6 @@ uev_trigger (struct uevent * uev, void * trigger_data)
/*
* path add/remove event
*/
- conf = get_multipath_config();
- if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
- uev->kernel) > 0) {
- put_multipath_config(conf);
- goto out;
- }
- put_multipath_config(conf);
-
if (!strncmp(uev->action, "add", 3)) {
r = uev_add_path(uev, vecs, 1);
goto out;
--
2.8.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 08/11] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
` (6 preceding siblings ...)
2017-01-12 5:52 ` [PATCH 07/11] multipathd: move calling filter_devnode() from uev_trigger() " tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
2017-01-12 5:52 ` [PATCH 09/11] multipathd: merge uevents before proccessing tang.junhui
` (2 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
The more uevents are merged, the higher efficiency program will performs.
So, do not process uevents after receiving immediately in uevents burst
situations, but continue wait one seconds for more uevents except that
too much uevents (2048) have already been received or too much time
eclipse (30 seconds).
Change-Id: I763d491540e8114a81d12d603281540a81502742
Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
libmultipath/uevent.c | 42 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index b560a50..f231dad 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -39,6 +39,7 @@
#include <linux/netlink.h>
#include <pthread.h>
#include <sys/mman.h>
+#include <sys/time.h>
#include <libudev.h>
#include <errno.h>
@@ -51,6 +52,10 @@
#include "config.h"
#include "blacklist.h"
+#define MAX_ACCUMULATION_COUNT 2048
+#define MAX_ACCUMULATION_TIME 30*1000
+#define MIN_BURST_SPEED 10
+
typedef int (uev_trigger)(struct uevent *, void * trigger_data);
LIST_HEAD(uevq);
@@ -520,11 +525,43 @@ struct uevent *uevent_from_udev_device(struct udev_device *dev)
return uev;
}
+bool uevent_burst(struct timeval *start_time, int events)
+{
+ struct timeval diff_time, end_time;
+ unsigned long speed;
+ unsigned long eclipse_ms;
+
+ if(events > MAX_ACCUMULATION_COUNT) {
+ condlog(2, "burst got %u uevents, too much uevents, stopped", events);
+ return false;
+ }
+
+ gettimeofday(&end_time, NULL);
+ timersub(&end_time, start_time, &diff_time);
+
+ eclipse_ms = diff_time.tv_sec * 1000 + diff_time.tv_usec / 1000;
+
+ if (eclipse_ms == 0)
+ return true;
+
+ if (eclipse_ms > MAX_ACCUMULATION_TIME) {
+ condlog(2, "burst continued %lu ms, too long time, stopped", eclipse_ms);
+ return false;
+ }
+
+ speed = (events * 1000) / eclipse_ms;
+ if (speed > MIN_BURST_SPEED)
+ return true;
+
+ return false;
+}
+
int uevent_listen(struct udev *udev)
{
int err = 2;
struct udev_monitor *monitor = NULL;
int fd, socket_flags, events;
+ struct timeval start_time;
int need_failback = 1;
int timeout = 30;
LIST_HEAD(uevlisten_tmp);
@@ -578,6 +615,7 @@ int uevent_listen(struct udev *udev)
}
events = 0;
+ gettimeofday(&start_time, NULL);
while (1) {
struct uevent *uev;
struct udev_device *dev;
@@ -592,7 +630,8 @@ int uevent_listen(struct udev *udev)
errno = 0;
fdcount = poll(&ev_poll, 1, poll_timeout);
if (fdcount && ev_poll.revents & POLLIN) {
- timeout = 0;
+ timeout = uevent_burst(&start_time, events + 1) ? 1 : 0;
+
dev = udev_monitor_receive_device(monitor);
if (!dev) {
condlog(0, "failed getting udev device");
@@ -625,6 +664,7 @@ int uevent_listen(struct udev *udev)
pthread_mutex_unlock(uevq_lockp);
events = 0;
}
+ gettimeofday(&start_time, NULL);
timeout = 30;
}
need_failback = 0;
--
2.8.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 09/11] multipathd: merge uevents before proccessing
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
` (7 preceding siblings ...)
2017-01-12 5:52 ` [PATCH 08/11] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
2017-01-12 5:52 ` [PATCH 10/11] libmultipath: filter " tang.junhui
2017-01-12 5:52 ` [PATCH 11/11] multipathd: proccess merged uevents tang.junhui
10 siblings, 0 replies; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
These uevents are going to be merged:
1) uevents come from paths and
2) uevents type is same and
3) uevents type is addition or deletion and
4) uevents merge_id is same.
Change-Id: I05ee057391c092aa0c5f989b7a4f9cb550bb4d98
Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
libmultipath/uevent.c | 117 ++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 105 insertions(+), 12 deletions(-)
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index f231dad..7282a51 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -89,6 +89,20 @@ struct uevent * alloc_uevent (void)
return uev;
}
+void
+uevq_cleanup(struct list_head *tmpq)
+{
+ struct uevent *uev, *tmp;
+
+ list_for_each_entry_safe(uev, tmp, tmpq, node) {
+ list_del_init(&uev->node);
+
+ if (uev->udev)
+ udev_device_unref(uev->udev);
+ FREE(uev);
+ }
+}
+
bool
uevent_can_discard(struct uevent *uev)
{
@@ -129,6 +143,62 @@ uevent_can_discard(struct uevent *uev)
return false;
}
+bool
+merge_need_stop(struct uevent *earlier, struct uevent *later)
+{
+ /*
+ * dm uevent do not try to merge with left uevents
+ */
+ if (!strncmp(later->kernel, "dm-", 3))
+ return true;
+
+ /*
+ * we can not make a jugement without merge_id,
+ * so it is sensible to stop merging
+ */
+ if (!earlier->merge_id || !later->merge_id)
+ return true;
+ /*
+ * uevents merging stoped
+ * when we meet an opposite action uevent from the same LUN to AVOID
+ * "add path1 |remove path1 |add path2 |remove path2 |add path3"
+ * to merge as "remove path1, path2" and "add path1, path2, path3"
+ * OR
+ * "remove path1 |add path1 |remove path2 |add path2 |remove path3"
+ * to merge as "add path1, path2" and "remove path1, path2, path3"
+ * SO
+ * when we meet a non-change uevent from the same LUN
+ * with the same merge_id and different action
+ * it would be better to stop merging.
+ */
+ if (!strcmp(earlier->merge_id, later->merge_id) &&
+ strcmp(earlier->action, later->action) &&
+ strcmp(earlier->action, "change") &&
+ strcmp(later->action, "change"))
+ return true;
+
+ return false;
+}
+
+bool
+uevent_can_merge(struct uevent *earlier, struct uevent *later)
+{
+ /* merge paths uevents
+ * whose merge_ids exsit and are same
+ * and actions are same,
+ * and actions are addition or deletion
+ */
+ if (earlier->merge_id && later->merge_id &&
+ !strcmp(earlier->merge_id, later->merge_id) &&
+ !strcmp(earlier->action, later->action) &&
+ strncmp(earlier->action, "change", 6) &&
+ strncmp(earlier->kernel, "dm-", 3)) {
+ return true;
+ }
+
+ return false;
+}
+
void
uevent_discard(struct list_head *tmpq)
{
@@ -145,6 +215,38 @@ uevent_discard(struct list_head *tmpq)
}
void
+uevent_merge(struct uevent *later, struct list_head *tmpq)
+{
+ struct uevent *earlier, *tmp;
+
+ list_for_some_entry_reverse_safe(earlier, tmp, &later->node, tmpq, node) {
+ if (merge_need_stop(earlier, later))
+ break;
+ /*
+ * merge earlier uevents to the later uevent
+ */
+ if (uevent_can_merge(earlier, later)) {
+ condlog(3, "merged uevent: %s-%s-%s with uevent: %s-%s-%s",
+ earlier->action, earlier->kernel, earlier->merge_id,
+ later->action, later->kernel, later->merge_id);
+
+ list_move(&earlier->node, &later->merge_node);
+ }
+ }
+}
+
+void
+merge_uevq(struct list_head *tmpq)
+{
+ struct uevent *later;
+
+ uevent_discard(tmpq);
+ list_for_each_entry_reverse(later, tmpq, node) {
+ uevent_merge(later, tmpq);
+ }
+}
+
+void
service_uevq(struct list_head *tmpq)
{
struct uevent *uev, *tmp;
@@ -155,6 +257,8 @@ service_uevq(struct list_head *tmpq)
if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
condlog(0, "uevent trigger error");
+ uevq_cleanup(&uev->merge_node);
+
if (uev->udev)
udev_device_unref(uev->udev);
FREE(uev);
@@ -169,17 +273,6 @@ static void uevent_cleanup(void *arg)
udev_unref(udev);
}
-void
-uevq_cleanup(struct list_head *tmpq)
-{
- struct uevent *uev, *tmp;
-
- list_for_each_entry_safe(uev, tmp, tmpq, node) {
- list_del_init(&uev->node);
- FREE(uev);
- }
-}
-
/*
* Service the uevent queue.
*/
@@ -208,7 +301,7 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
pthread_mutex_unlock(uevq_lockp);
if (!my_uev_trigger)
break;
- uevent_discard(&uevq_tmp);
+ merge_uevq(&uevq_tmp);
service_uevq(&uevq_tmp);
}
condlog(3, "Terminating uev service queue");
--
2.8.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 10/11] libmultipath: filter uevents before proccessing
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
` (8 preceding siblings ...)
2017-01-12 5:52 ` [PATCH 09/11] multipathd: merge uevents before proccessing tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
2017-01-12 5:52 ` [PATCH 11/11] multipathd: proccess merged uevents tang.junhui
10 siblings, 0 replies; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: tang.junhui <tang.junhui@zte.com.cn>
Before merging uevents, these uevents are going to be filtered:
1) Change or addition uevent of a removed path (it indicate by an
deletion uevent occurred later), and
2) Change uevent if addition uevent of a path has not ever been
proccessed.
Change-Id: If00c2c2e23ea466c1d4643c541ed2d8f9a0c8dea
Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
libmultipath/uevent.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 7282a51..838f128 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -144,6 +144,40 @@ uevent_can_discard(struct uevent *uev)
}
bool
+uevent_can_filter(struct uevent *earlier, struct uevent *later)
+{
+
+ /*
+ * filter earlier uvents if path has removed later. Eg:
+ * "add path1 |chang path1 |add path2 |remove path1"
+ * can filter as:
+ * "add path2 |remove path1"
+ * uevents "add path1" and "chang path1" are filtered out
+ */
+ if (!strcmp(earlier->kernel, later->kernel) &&
+ !strcmp(later->action, "remove") &&
+ strncmp(later->kernel, "dm-", 3)) {
+ return true;
+ }
+
+ /*
+ * filter change uvents if add uevents exist. Eg:
+ * "change path1| add path1 |add path2"
+ * can filter as:
+ * "add path1 |add path2"
+ * uevent "chang path1" is filtered out
+ */
+ if (!strcmp(earlier->kernel, later->kernel) &&
+ !strcmp(earlier->action, "change") &&
+ !strcmp(later->action, "add") &&
+ strncmp(later->kernel, "dm-", 3)) {
+ return true;
+ }
+
+ return false;
+}
+
+bool
merge_need_stop(struct uevent *earlier, struct uevent *later)
{
/*
@@ -215,6 +249,29 @@ uevent_discard(struct list_head *tmpq)
}
void
+uevent_filter(struct uevent *later, struct list_head *tmpq)
+{
+ struct uevent *earlier, *tmp;
+
+ list_for_some_entry_reverse_safe(earlier, tmp, &later->node, tmpq, node) {
+ /*
+ * filter unnessary earlier uevents
+ * by the later uevent
+ */
+ if (uevent_can_filter(earlier, later)) {
+ condlog(2, "uevent: %s-%s has filtered by uevent: %s-%s",
+ earlier->kernel, earlier->action,
+ later->kernel, later->action);
+
+ list_del_init(&earlier->node);
+ if (earlier->udev)
+ udev_device_unref(earlier->udev);
+ FREE(earlier);
+ }
+ }
+}
+
+void
uevent_merge(struct uevent *later, struct list_head *tmpq)
{
struct uevent *earlier, *tmp;
@@ -242,6 +299,7 @@ merge_uevq(struct list_head *tmpq)
uevent_discard(tmpq);
list_for_each_entry_reverse(later, tmpq, node) {
+ uevent_filter(later, tmpq);
uevent_merge(later, tmpq);
}
}
--
2.8.1.windows.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 11/11] multipathd: proccess merged uevents
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
` (9 preceding siblings ...)
2017-01-12 5:52 ` [PATCH 10/11] libmultipath: filter " tang.junhui
@ 2017-01-12 5:52 ` tang.junhui
10 siblings, 0 replies; 20+ messages in thread
From: tang.junhui @ 2017-01-12 5:52 UTC (permalink / raw)
To: christophe.varoqui, bmarzins, hare, mwilck, bart.vanassche
Cc: zhang.kai16, dm-devel, tang.junhui, tang.wenjun3
From: "tang.junhui" <tang.junhui@zte.com.cn>
After filtering and merging, then uevents are processed in
uev_trigger(), firstly, each of merged uevents would be processed one
by one with need_do_map in value of 0. Finally, the node “uev” itself
would be processed with need_do_map in value of 1, which would reload
kernel table, and create or remove the dm device.
Change-Id: Ifb4de0ca36180a8af16729c2f70bc250858877bd
Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
multipathd/main.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 24116e3..5b63577 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1120,6 +1120,7 @@ uev_trigger (struct uevent * uev, void * trigger_data)
{
int r = 0;
struct vectors * vecs;
+ struct uevent *merge_uev, *tmp;
vecs = (struct vectors *)trigger_data;
@@ -1151,20 +1152,21 @@ uev_trigger (struct uevent * uev, void * trigger_data)
}
/*
- * path add/remove event
+ * path add/remove/change event, add/remove maybe merged
*/
- if (!strncmp(uev->action, "add", 3)) {
- r = uev_add_path(uev, vecs, 1);
- goto out;
- }
- if (!strncmp(uev->action, "remove", 6)) {
- r = uev_remove_path(uev, vecs, 1);
- goto out;
- }
- if (!strncmp(uev->action, "change", 6)) {
- r = uev_update_path(uev, vecs);
- goto out;
- }
+ list_for_each_entry_safe(merge_uev, tmp, &uev->merge_node, node) {
+ if (!strncmp(merge_uev->action, "add", 3))
+ r += uev_add_path(merge_uev, vecs, 0);
+ if (!strncmp(merge_uev->action, "remove", 6))
+ r += uev_remove_path(merge_uev, vecs, 0);
+ }
+
+ if (!strncmp(uev->action, "add", 3))
+ r += uev_add_path(uev, vecs, 1);
+ if (!strncmp(uev->action, "remove", 6))
+ r += uev_remove_path(uev, vecs, 1);
+ if (!strncmp(uev->action, "change", 6))
+ r += uev_update_path(uev, vecs);
out:
return r;
--
2.8.1.windows.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 05/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_remove_path()
2017-01-12 5:52 ` [PATCH 05/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_remove_path() tang.junhui
@ 2017-01-16 18:18 ` Benjamin Marzinski
0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Marzinski @ 2017-01-16 18:18 UTC (permalink / raw)
To: tang.junhui; +Cc: tang.wenjun3, zhang.kai16, dm-devel, bart.vanassche, mwilck
On Thu, Jan 12, 2017 at 01:52:21PM +0800, tang.junhui@zte.com.cn wrote:
> From: tang.junhui <tang.junhui@zte.com.cn>
>
> Usually calling domap() in ev_remove_path() is needed, but only last
> path need to call domap() in processing for merged uevents to reduce the
> count of calling domap() and promote efficiency. So add input parameter
> need_do_map to indicate whether need calling domap() in ev_remove_path().
I noticed a bug in this code that I missed before. If you are removing a
merged event, you exit after removing the device from both mpp->paths
and vecs->pathvec and freeing it, but without calling setup_map. This
leaves the multipath device is an incorrect state, and makes if very
likely to access freed memory.
The issue has to do with multipath devices having two lists for paths,
mpp->paths, and mpp->pg->paths. Normally, all of the paths are stored in
mpp->pg->paths, sorted by their pathgroup, and mpp->paths is NULL. When
multipathd needs to modify the paths, it first needs to grab the vecs
lock, and repopulate mpp->paths by calling update_mpp_paths(). Later on,
before releasing the vecs lock, it should call setup_map which will uses
the priority function to update mpp->pg->paths. the prioritizer
functions also free mpp->paths.
With this patch, you exit ev_remove_path before calling setup_map on a
merged event. This means that the mpp->paths is still around, which
isn't a big deal (update_mpp_paths handles that just fine). But the
problem is that mpp->pg->paths still contains a pointer to the now freed
path. Since this was a merged event, you will definitely be calling
ev_remove_path again. When this happens it will call update_mpp_paths,
which will iterate over mpp->pg->paths, and check all the paths to see
if they should be copied to mpp->paths. This will check the now freed
path as well. If that memory get's reused before you call
update_mpp_paths, you will be accessing garbage, with undefined results.
The easiest solution is simply to not return until after you call
setup_map. It should be find to move the need_do_map check till after
the code removes the multipath device if the last path has been removed.
Since this is a merged event, mpp->paths shouldn't be empty (since there
are still more paths to remove). Even if it is, it won't hurt
anything to remove the map before servicing the other remove events,
which are likely spurious in this case.
I realize that setup_map calls a lot of functions that are unnecessary
for merged events, since they will all get rerun before finally calling
domap. It should be safe to simply also remove the path from
mpp->pg->paths, so you aren't accessing freed memory. Like I said
before, I can't see how leaving mpp->paths populated would hurt
anything. But calling setup_map will make sure that nobody needs to
remember that this case gets treated differently, if things change in
the future.
-Ben
>
> Change-Id: I0a787330a249608396cc3e655465dc820f940539
> Signed-off-by: tang.wenjun <tang.wenjun3@zte.com.cn>
> ---
> multipathd/cli_handlers.c | 2 +-
> multipathd/main.c | 23 ++++++++++++++++++-----
> multipathd/main.h | 2 +-
> 3 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 3a46c09..302fd02 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -693,7 +693,7 @@ cli_del_path (void * v, char ** reply, int * len, void * data)
> condlog(0, "%s: path already removed", param);
> return 1;
> }
> - return ev_remove_path(pp, vecs);
> + return ev_remove_path(pp, vecs, 1);
> }
>
> int
> diff --git a/multipathd/main.c b/multipathd/main.c
> index ebd7de1..718c5e7 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -858,7 +858,7 @@ fail:
> }
>
> static int
> -uev_remove_path (struct uevent *uev, struct vectors * vecs)
> +uev_remove_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
> {
> struct path *pp;
> int ret;
> @@ -868,8 +868,18 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs)
> lock(&vecs->lock);
> pthread_testcancel();
> pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> - if (pp)
> - ret = ev_remove_path(pp, vecs);
> + if (pp) {
> + /*
> + * Make sure merging use the correct wwid
> + * Othterwise calling domap()
> + */
> + if (!need_do_map &&
> + uev->merge_id &&
> + strcmp(uev->merge_id, pp->wwid))
> + need_do_map = 1;
> +
> + ret = ev_remove_path(pp, vecs, need_do_map);
> + }
> lock_cleanup_pop(vecs->lock);
> if (!pp) {
> /* Not an error; path might have been purged earlier */
> @@ -880,7 +890,7 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs)
> }
>
> int
> -ev_remove_path (struct path *pp, struct vectors * vecs)
> +ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
> {
> struct multipath * mpp;
> int i, retval = 0;
> @@ -902,6 +912,9 @@ ev_remove_path (struct path *pp, struct vectors * vecs)
> if ((i = find_slot(mpp->paths, (void *)pp)) != -1)
> vector_del_slot(mpp->paths, i);
>
> + if(!need_do_map)
> + goto out;
> +
> /*
> * remove the map IFF removing the last path
> */
> @@ -1179,7 +1192,7 @@ uev_trigger (struct uevent * uev, void * trigger_data)
> goto out;
> }
> if (!strncmp(uev->action, "remove", 6)) {
> - r = uev_remove_path(uev, vecs);
> + r = uev_remove_path(uev, vecs, 1);
> goto out;
> }
> if (!strncmp(uev->action, "change", 6)) {
> diff --git a/multipathd/main.h b/multipathd/main.h
> index f810d41..094b04f 100644
> --- a/multipathd/main.h
> +++ b/multipathd/main.h
> @@ -23,7 +23,7 @@ const char * daemon_status(void);
> int need_to_delay_reconfig (struct vectors *);
> int reconfigure (struct vectors *);
> int ev_add_path (struct path *, struct vectors *, int);
> -int ev_remove_path (struct path *, struct vectors *);
> +int ev_remove_path (struct path *, struct vectors *, int);
> int ev_add_map (char *, char *, struct vectors *);
> int ev_remove_map (char *, char *, int, struct vectors *);
> void sync_map_state (struct multipath *);
> --
> 2.8.1.windows.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path()
2017-01-12 5:52 ` [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path() tang.junhui
@ 2017-01-16 21:38 ` Benjamin Marzinski
2017-01-17 7:28 ` tang.junhui
0 siblings, 1 reply; 20+ messages in thread
From: Benjamin Marzinski @ 2017-01-16 21:38 UTC (permalink / raw)
To: tang.junhui; +Cc: tang.wenjun3, zhang.kai16, dm-devel, bart.vanassche, mwilck
On Thu, Jan 12, 2017 at 01:52:20PM +0800, tang.junhui@zte.com.cn wrote:
> From: tang.junhui <tang.junhui@zte.com.cn>
>
> Usually calling domap() in ev_add_path() is needed, but only last path
> need to call domap() in processing for merged uevents to reduce the
> count of calling domap() and promote efficiency. So add input parameter
> need_do_map to indicate whether need calling domap() in ev_add_path().
With the addition of checking if the merge_id equals the wwid, you are
protected against accidentially merging paths that shouldn't be merged,
which is great. But setting need_do_map for these paths doesn't
completely make sure that if the wwid and merge_id differ, you will
always call domap.
A contrived situation where this fails would look like:
add path1, path2, path3
where merge_id equals the wwid for path1 and path2, but there is a
different wwid for path3. In this case, you would call domap on just
the multipath device for path3, but since path1 and path2 matched the
merge_id, they wouldn't force a call to domap.
A less contrived example would be
add path1, path2, path3, path4
Where these were devices that were actually pointing at two different
LUNs, but all set ID_SERIAL the same. path1 and path2 pointed to one
LUN, while path3 and path4 pointed to another LUN. In this case the
wwid of path1 and path2 matched the merge_id, while the wwid of path3
and path4 was different. In this case you would call domap twice, on
both path3 and path4, but nothing would call domap to create a multipath
device for path1 and path2.
In general, the code to set need_do_map if the wwid and merge_id don't
match only works if either none of the device in a merged set have wwids
that match the merge_id, or if the last device has a wwid that matches
the merge_id. If there are devices with wwids that match the merge_id,
but the last device in the set isn't one of them, then some devices will
not get a multipath device created for them.
Now, I don't know of any device that works like my above example, so
your code will likely work fine for all real-world devices. Also,
fixing this is a pain, as you don't find out until processing the last
path in a set that things went wrong, and then you would have to go back
and run the skipped functions on one of the paths you have already
processed.
The easy way to fix it is to use the other possibility that I mentioned
before, which is to not have the merge_id, and just use the udev
provided wwid, instead of fetching it from pathinfo. Like I mentioned,
if you do this, you want to make sure that you only try to grab the wwid
from the udev events for devices with the correct kernel name: ID_SERIAL
only for "sd.*" devices, and ID_UID only for "dasd.*" devices. I also
think that this should be configurable.
Otherwise, you can either go through the messy work of calling domap
correctly when the last device of a set has a wwid that doesn't match
the merge_id, or we can decide that this won't acutally cause problems
with any known device, and punt fixing it for now. And if it causes
problems with some future devices, we can deal with it then.
-Ben
>
> Change-Id: I7f33de49a1d89d91180dcb95cd94e4944ae7ff36
> Signed-off-by: tang.wenjun <tang.wenjun3@zte.com.cn>
> ---
> multipathd/cli_handlers.c | 3 ++-
> multipathd/main.c | 47 ++++++++++++++++++++++++++++++++++++-----------
> multipathd/main.h | 2 +-
> 3 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index b0eeca6..3a46c09 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -670,7 +670,8 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
> pp->checkint = conf->checkint;
> }
> put_multipath_config(conf);
> - return ev_add_path(pp, vecs);
> + return ev_add_path(pp, vecs, 1);
> +
> blacklisted:
> *reply = strdup("blacklisted\n");
> *len = strlen(*reply) + 1;
> diff --git a/multipathd/main.c b/multipathd/main.c
> index adc3258..ebd7de1 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -608,7 +608,7 @@ ev_remove_map (char * devname, char * alias, int minor, struct vectors * vecs)
> }
>
> static int
> -uev_add_path (struct uevent *uev, struct vectors * vecs)
> +uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
> {
> struct path *pp;
> int ret = 0, i;
> @@ -640,9 +640,18 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
> r = pathinfo(pp, conf,
> DI_ALL | DI_BLACKLIST);
> put_multipath_config(conf);
> - if (r == PATHINFO_OK)
> - ret = ev_add_path(pp, vecs);
> - else if (r == PATHINFO_SKIPPED) {
> + if (r == PATHINFO_OK) {
> + /*
> + * Make sure merging use the correct wwid
> + * Othterwise calling domap()
> + */
> + if (!need_do_map &&
> + uev->merge_id &&
> + strcmp(uev->merge_id, pp->wwid))
> + need_do_map = 1;
> +
> + ret = ev_add_path(pp, vecs, need_do_map);
> + } else if (r == PATHINFO_SKIPPED) {
> condlog(3, "%s: remove blacklisted path",
> uev->kernel);
> i = find_slot(vecs->pathvec, (void *)pp);
> @@ -681,7 +690,16 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
> conf = get_multipath_config();
> pp->checkint = conf->checkint;
> put_multipath_config(conf);
> - ret = ev_add_path(pp, vecs);
> + /*
> + * Make sure merging use the correct wwid
> + * Othterwise calling domap()
> + */
> + if (!need_do_map &&
> + uev->merge_id &&
> + strcmp(uev->merge_id, pp->wwid))
> + need_do_map = 1;
> +
> + ret = ev_add_path(pp, vecs, need_do_map);
> } else {
> condlog(0, "%s: failed to store path info, "
> "dropping event",
> @@ -699,7 +717,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
> * 1: error
> */
> int
> -ev_add_path (struct path * pp, struct vectors * vecs)
> +ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
> {
> struct multipath * mpp;
> char params[PARAMS_SIZE] = {0};
> @@ -767,6 +785,13 @@ rescan:
> /* persistent reservation check*/
> mpath_pr_event_handle(pp);
>
> + if (!need_do_map)
> + return 0;
> +
> + if (!dm_map_present(mpp->alias)) {
> + mpp->action = ACT_CREATE;
> + start_waiter = 1;
> + }
> /*
> * push the map to the device-mapper
> */
> @@ -995,7 +1020,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
> }
>
> if (pp->initialized == INIT_REQUESTED_UDEV)
> - retval = uev_add_path(uev, vecs);
> + retval = uev_add_path(uev, vecs, 1);
> else if (mpp && ro >= 0) {
> condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro);
>
> @@ -1150,7 +1175,7 @@ uev_trigger (struct uevent * uev, void * trigger_data)
> put_multipath_config(conf);
>
> if (!strncmp(uev->action, "add", 3)) {
> - r = uev_add_path(uev, vecs);
> + r = uev_add_path(uev, vecs, 1);
> goto out;
> }
> if (!strncmp(uev->action, "remove", 6)) {
> @@ -1570,7 +1595,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> conf = get_multipath_config();
> ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
> if (ret == PATHINFO_OK) {
> - ev_add_path(pp, vecs);
> + ev_add_path(pp, vecs, 1);
> pp->tick = 1;
> } else if (ret == PATHINFO_SKIPPED) {
> put_multipath_config(conf);
> @@ -1686,7 +1711,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> }
> if (!disable_reinstate && reinstate_path(pp, add_active)) {
> condlog(3, "%s: reload map", pp->dev);
> - ev_add_path(pp, vecs);
> + ev_add_path(pp, vecs, 1);
> pp->tick = 1;
> return 0;
> }
> @@ -1709,7 +1734,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> /* Clear IO errors */
> if (reinstate_path(pp, 0)) {
> condlog(3, "%s: reload map", pp->dev);
> - ev_add_path(pp, vecs);
> + ev_add_path(pp, vecs, 1);
> pp->tick = 1;
> return 0;
> }
> diff --git a/multipathd/main.h b/multipathd/main.h
> index f72580d..f810d41 100644
> --- a/multipathd/main.h
> +++ b/multipathd/main.h
> @@ -22,7 +22,7 @@ void exit_daemon(void);
> const char * daemon_status(void);
> int need_to_delay_reconfig (struct vectors *);
> int reconfigure (struct vectors *);
> -int ev_add_path (struct path *, struct vectors *);
> +int ev_add_path (struct path *, struct vectors *, int);
> int ev_remove_path (struct path *, struct vectors *);
> int ev_add_map (char *, char *, struct vectors *);
> int ev_remove_map (char *, char *, int, struct vectors *);
> --
> 2.8.1.windows.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path()
2017-01-16 21:38 ` Benjamin Marzinski
@ 2017-01-17 7:28 ` tang.junhui
2017-01-17 16:14 ` Benjamin Marzinski
0 siblings, 1 reply; 20+ messages in thread
From: tang.junhui @ 2017-01-17 7:28 UTC (permalink / raw)
To: Benjamin Marzinski
Cc: tang.wenjun3, zhang.kai16, dm-devel, bart.vanassche, mwilck
[-- Attachment #1.1: Type: text/plain, Size: 12562 bytes --]
Hello Ben
Thank you for your patience again.
I'll modify code according to your suggestion as this:
1) add configuration in the defaults section
uid_attrs "sd:ID_SERIAL dasd:ID_UID"
it would override any other configurations if this
filed is configured and matched;
2) In uevent processing thread, we will assign merge_id
according the label in uevents by this configuration;
3) this patch will take back:
[PATCH 12/12] libmultipath: use existing wwid when
wwid has already been existed in uevent
4) if this field is not configured, only do filtering and
no merging works.
Please confirm whether this modification is feasible.
Regards,
Tang Junhui
发件人: "Benjamin Marzinski" <bmarzins@redhat.com>
收件人: tang.junhui@zte.com.cn,
抄送: christophe.varoqui@opensvc.com, hare@suse.de, mwilck@suse.com,
bart.vanassche@sandisk.com, dm-devel@redhat.com, zhang.kai16@zte.com.cn,
tang.wenjun3@zte.com.cn
日期: 2017/01/17 05:38
主题: Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether
need calling domap() in ev_add_path()
On Thu, Jan 12, 2017 at 01:52:20PM +0800, tang.junhui@zte.com.cn wrote:
> From: tang.junhui <tang.junhui@zte.com.cn>
>
> Usually calling domap() in ev_add_path() is needed, but only last path
> need to call domap() in processing for merged uevents to reduce the
> count of calling domap() and promote efficiency. So add input parameter
> need_do_map to indicate whether need calling domap() in ev_add_path().
With the addition of checking if the merge_id equals the wwid, you are
protected against accidentially merging paths that shouldn't be merged,
which is great. But setting need_do_map for these paths doesn't
completely make sure that if the wwid and merge_id differ, you will
always call domap.
A contrived situation where this fails would look like:
add path1, path2, path3
where merge_id equals the wwid for path1 and path2, but there is a
different wwid for path3. In this case, you would call domap on just
the multipath device for path3, but since path1 and path2 matched the
merge_id, they wouldn't force a call to domap.
A less contrived example would be
add path1, path2, path3, path4
Where these were devices that were actually pointing at two different
LUNs, but all set ID_SERIAL the same. path1 and path2 pointed to one
LUN, while path3 and path4 pointed to another LUN. In this case the
wwid of path1 and path2 matched the merge_id, while the wwid of path3
and path4 was different. In this case you would call domap twice, on
both path3 and path4, but nothing would call domap to create a multipath
device for path1 and path2.
In general, the code to set need_do_map if the wwid and merge_id don't
match only works if either none of the device in a merged set have wwids
that match the merge_id, or if the last device has a wwid that matches
the merge_id. If there are devices with wwids that match the merge_id,
but the last device in the set isn't one of them, then some devices will
not get a multipath device created for them.
Now, I don't know of any device that works like my above example, so
your code will likely work fine for all real-world devices. Also,
fixing this is a pain, as you don't find out until processing the last
path in a set that things went wrong, and then you would have to go back
and run the skipped functions on one of the paths you have already
processed.
The easy way to fix it is to use the other possibility that I mentioned
before, which is to not have the merge_id, and just use the udev
provided wwid, instead of fetching it from pathinfo. Like I mentioned,
if you do this, you want to make sure that you only try to grab the wwid
from the udev events for devices with the correct kernel name: ID_SERIAL
only for "sd.*" devices, and ID_UID only for "dasd.*" devices. I also
think that this should be configurable.
Otherwise, you can either go through the messy work of calling domap
correctly when the last device of a set has a wwid that doesn't match
the merge_id, or we can decide that this won't acutally cause problems
with any known device, and punt fixing it for now. And if it causes
problems with some future devices, we can deal with it then.
-Ben
>
> Change-Id: I7f33de49a1d89d91180dcb95cd94e4944ae7ff36
> Signed-off-by: tang.wenjun <tang.wenjun3@zte.com.cn>
> ---
> multipathd/cli_handlers.c | 3 ++-
> multipathd/main.c | 47
++++++++++++++++++++++++++++++++++++-----------
> multipathd/main.h | 2 +-
> 3 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index b0eeca6..3a46c09 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -670,7 +670,8 @@ cli_add_path (void * v, char ** reply, int * len,
void * data)
> pp->checkint = conf->checkint;
> }
> put_multipath_config(conf);
> - return ev_add_path(pp, vecs);
> + return ev_add_path(pp, vecs, 1);
> +
> blacklisted:
> *reply = strdup("blacklisted\n");
> *len = strlen(*reply) + 1;
> diff --git a/multipathd/main.c b/multipathd/main.c
> index adc3258..ebd7de1 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -608,7 +608,7 @@ ev_remove_map (char * devname, char * alias, int
minor, struct vectors * vecs)
> }
>
> static int
> -uev_add_path (struct uevent *uev, struct vectors * vecs)
> +uev_add_path (struct uevent *uev, struct vectors * vecs, int
need_do_map)
> {
> struct path *pp;
> int ret = 0, i;
> @@ -640,9 +640,18 @@ uev_add_path (struct uevent *uev, struct vectors *
vecs)
> r = pathinfo(pp, conf,
> DI_ALL | DI_BLACKLIST);
> put_multipath_config(conf);
> - if (r == PATHINFO_OK)
> - ret =
ev_add_path(pp, vecs);
> - else if (r ==
PATHINFO_SKIPPED) {
> + if (r == PATHINFO_OK) {
> + /*
> + * Make
sure merging use the correct wwid
> + *
Othterwise calling domap()
> + */
> + if
(!need_do_map &&
> + uev->merge_id &&
> + strcmp(uev->merge_id, pp->wwid))
> + need_do_map = 1;
> +
> + ret =
ev_add_path(pp, vecs, need_do_map);
> + } else if (r ==
PATHINFO_SKIPPED) {
> condlog(3, "%s: remove blacklisted path",
> uev->kernel);
> i =
find_slot(vecs->pathvec, (void *)pp);
> @@ -681,7 +690,16 @@ uev_add_path (struct uevent *uev, struct vectors *
vecs)
> conf = get_multipath_config();
> pp->checkint = conf->checkint;
> put_multipath_config(conf);
> - ret = ev_add_path(pp, vecs);
> + /*
> + * Make sure merging use the correct wwid
> + * Othterwise calling domap()
> + */
> + if (!need_do_map &&
> + uev->merge_id &&
> + strcmp(uev->merge_id, pp->wwid))
> + need_do_map = 1;
> +
> + ret = ev_add_path(pp, vecs, need_do_map);
> } else {
> condlog(0, "%s: failed to store path
info, "
> "dropping event",
> @@ -699,7 +717,7 @@ uev_add_path (struct uevent *uev, struct vectors *
vecs)
> * 1: error
> */
> int
> -ev_add_path (struct path * pp, struct vectors * vecs)
> +ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
> {
> struct multipath * mpp;
> char params[PARAMS_SIZE] = {0};
> @@ -767,6 +785,13 @@ rescan:
> /* persistent reservation check*/
> mpath_pr_event_handle(pp);
>
> + if (!need_do_map)
> + return 0;
> +
> + if (!dm_map_present(mpp->alias)) {
> + mpp->action = ACT_CREATE;
> + start_waiter = 1;
> + }
> /*
> * push the map to the device-mapper
> */
> @@ -995,7 +1020,7 @@ uev_update_path (struct uevent *uev, struct vectors
* vecs)
> }
>
> if (pp->initialized ==
INIT_REQUESTED_UDEV)
> - retval =
uev_add_path(uev, vecs);
> + retval =
uev_add_path(uev, vecs, 1);
> else if (mpp && ro >= 0) {
> condlog(2, "%s: update
path write_protect to '%d' (uevent)", uev->kernel, ro);
>
> @@ -1150,7 +1175,7 @@ uev_trigger (struct uevent * uev, void *
trigger_data)
> put_multipath_config(conf);
>
> if (!strncmp(uev->action, "add", 3)) {
> - r = uev_add_path(uev, vecs);
> + r = uev_add_path(uev, vecs, 1);
> goto out;
> }
> if (!strncmp(uev->action, "remove", 6)) {
> @@ -1570,7 +1595,7 @@ check_path (struct vectors * vecs, struct path *
pp, int ticks)
> conf =
get_multipath_config();
> ret = pathinfo(pp, conf,
DI_ALL | DI_BLACKLIST);
> if (ret == PATHINFO_OK) {
> - ev_add_path(pp, vecs);
> + ev_add_path(pp, vecs, 1);
> pp->tick
= 1;
> } else if (ret ==
PATHINFO_SKIPPED) {
> put_multipath_config(conf);
> @@ -1686,7 +1711,7 @@ check_path (struct vectors * vecs, struct path *
pp, int ticks)
> }
> if (!disable_reinstate &&
reinstate_path(pp, add_active)) {
> condlog(3, "%s: reload
map", pp->dev);
> - ev_add_path(pp, vecs);
> + ev_add_path(pp, vecs, 1);
> pp->tick = 1;
> return 0;
> }
> @@ -1709,7 +1734,7 @@ check_path (struct vectors * vecs, struct path *
pp, int ticks)
> /* Clear IO errors */
> if (reinstate_path(pp,
0)) {
> condlog(3, "%s: reload map", pp->dev);
> - ev_add_path(pp, vecs);
> + ev_add_path(pp, vecs, 1);
> pp->tick
= 1;
> return 0;
> }
> diff --git a/multipathd/main.h b/multipathd/main.h
> index f72580d..f810d41 100644
> --- a/multipathd/main.h
> +++ b/multipathd/main.h
> @@ -22,7 +22,7 @@ void exit_daemon(void);
> const char * daemon_status(void);
> int need_to_delay_reconfig (struct vectors *);
> int reconfigure (struct vectors *);
> -int ev_add_path (struct path *, struct vectors *);
> +int ev_add_path (struct path *, struct vectors *, int);
> int ev_remove_path (struct path *, struct vectors *);
> int ev_add_map (char *, char *, struct vectors *);
> int ev_remove_map (char *, char *, int, struct vectors *);
> --
> 2.8.1.windows.1
>
[-- Attachment #1.2: Type: text/html, Size: 27354 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path()
2017-01-17 7:28 ` tang.junhui
@ 2017-01-17 16:14 ` Benjamin Marzinski
2017-04-05 13:26 ` uid_attrs (was Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path()) Martin Wilck
0 siblings, 1 reply; 20+ messages in thread
From: Benjamin Marzinski @ 2017-01-17 16:14 UTC (permalink / raw)
To: tang.junhui; +Cc: tang.wenjun3, zhang.kai16, dm-devel, bart.vanassche, mwilck
On Tue, Jan 17, 2017 at 03:28:06PM +0800, tang.junhui@zte.com.cn wrote:
> Hello Ben
>
> Thank you for your patience again.
>
> I'll modify code according to your suggestion as this:
> 1) add configuration in the defaults section
> uid_attrs "sd:ID_SERIAL dasd:ID_UID"
> it would override any other configurations if this
> filed is configured and matched;
>
> 2) In uevent processing thread, we will assign merge_id
> according the label in uevents by this configuration;
>
> 3) this patch will take back:
> [PATCH 12/12] libmultipath: use existing wwid when
> wwid has already been existed in uevent
>
> 4) if this field is not configured, only do filtering and
> no merging works.
>
> Please confirm whether this modification is feasible.
Yes. This is perfectly reasonable solution. Thanks for doing all the
work on this.
-Ben
>
> Regards,
> Tang Junhui
>
> ������: "Benjamin Marzinski" <bmarzins@redhat.com>
> �ռ���: tang.junhui@zte.com.cn,
> ����: christophe.varoqui@opensvc.com, hare@suse.de,
> mwilck@suse.com, bart.vanassche@sandisk.com, dm-devel@redhat.com,
> zhang.kai16@zte.com.cn, tang.wenjun3@zte.com.cn
> ����: 2017/01/17 05:38
> ����: Re: [PATCH 04/11] multipathd: add need_do_map to indicate
> whether need calling domap() in ev_add_path()
>
> --------------------------------------------------------------------------
>
> On Thu, Jan 12, 2017 at 01:52:20PM +0800, tang.junhui@zte.com.cn wrote:
> > From: tang.junhui <tang.junhui@zte.com.cn>
> >
> > Usually calling domap() in ev_add_path() is needed, but only last path
> > need to call domap() in processing for merged uevents to reduce the
> > count of calling domap() and promote efficiency. So add input parameter
> > need_do_map to indicate whether need calling domap() in ev_add_path().
>
> With the addition of checking if the merge_id equals the wwid, you are
> protected against accidentially merging paths that shouldn't be merged,
> which is great. But setting need_do_map for these paths doesn't
> completely make sure that if the wwid and merge_id differ, you will
> always call domap.
>
> A contrived situation where this fails would look like:
>
> add path1, path2, path3
>
> where merge_id equals the wwid for path1 and path2, but there is a
> different wwid for path3. In this case, you would call domap on just
> the multipath device for path3, but since path1 and path2 matched the
> merge_id, they wouldn't force a call to domap.
>
> A less contrived example would be
>
> add path1, path2, path3, path4
>
> Where these were devices that were actually pointing at two different
> LUNs, but all set ID_SERIAL the same. path1 and path2 pointed to one
> LUN, while path3 and path4 pointed to another LUN. In this case the
> wwid of path1 and path2 matched the merge_id, while the wwid of path3
> and path4 was different. In this case you would call domap twice, on
> both path3 and path4, but nothing would call domap to create a multipath
> device for path1 and path2.
>
> In general, the code to set need_do_map if the wwid and merge_id don't
> match only works if either none of the device in a merged set have wwids
> that match the merge_id, or if the last device has a wwid that matches
> the merge_id. If there are devices with wwids that match the merge_id,
> but the last device in the set isn't one of them, then some devices will
> not get a multipath device created for them.
>
> Now, I don't know of any device that works like my above example, so
> your code will likely work fine for all real-world devices. Also,
> fixing this is a pain, as you don't find out until processing the last
> path in a set that things went wrong, and then you would have to go back
> and run the skipped functions on one of the paths you have already
> processed.
>
> The easy way to fix it is to use the other possibility that I mentioned
> before, which is to not have the merge_id, and just use the udev
> provided wwid, instead of fetching it from pathinfo. Like I mentioned,
> if you do this, you want to make sure that you only try to grab the wwid
> from the udev events for devices with the correct kernel name: ID_SERIAL
> only for "sd.*" devices, and ID_UID only for "dasd.*" devices. I also
> think that this should be configurable.
>
> Otherwise, you can either go through the messy work of calling domap
> correctly when the last device of a set has a wwid that doesn't match
> the merge_id, or we can decide that this won't acutally cause problems
> with any known device, and punt fixing it for now. And if it causes
> problems with some future devices, we can deal with it then.
>
> -Ben
>
> >
> > Change-Id: I7f33de49a1d89d91180dcb95cd94e4944ae7ff36
> > Signed-off-by: tang.wenjun <tang.wenjun3@zte.com.cn>
> > ---
> > multipathd/cli_handlers.c | 3 ++-
> > multipathd/main.c | 47
> ++++++++++++++++++++++++++++++++++++-----------
> > multipathd/main.h | 2 +-
> > 3 files changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index b0eeca6..3a46c09 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -670,7 +670,8 @@ cli_add_path (void * v, char ** reply, int * len,
> void * data)
> > pp->checkint = conf->checkint;
> > }
> > put_multipath_config(conf);
> > - return ev_add_path(pp, vecs);
> > + return ev_add_path(pp, vecs, 1);
> > +
> > blacklisted:
> > *reply = strdup("blacklisted\n");
> > *len = strlen(*reply) + 1;
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index adc3258..ebd7de1 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -608,7 +608,7 @@ ev_remove_map (char * devname, char * alias, int
> minor, struct vectors * vecs)
> > }
> >
> > static int
> > -uev_add_path (struct uevent *uev, struct vectors * vecs)
> > +uev_add_path (struct uevent *uev, struct vectors * vecs, int
> need_do_map)
> > {
> > struct path *pp;
> > int ret = 0, i;
> > @@ -640,9 +640,18 @@ uev_add_path (struct uevent *uev, struct vectors *
> vecs)
> > r = pathinfo(pp,
> conf,
> >
> DI_ALL | DI_BLACKLIST);
> >
> put_multipath_config(conf);
> > - if (r ==
> PATHINFO_OK)
> > - ret
> = ev_add_path(pp, vecs);
> > - else if (r ==
> PATHINFO_SKIPPED) {
> > + if (r ==
> PATHINFO_OK) {
> > + /*
> > + *
> Make sure merging use the correct wwid
> > + *
> Othterwise calling domap()
> > + */
> > + if
> (!need_do_map &&
> > +
> uev->merge_id &&
> > +
> strcmp(uev->merge_id, pp->wwid))
> > +
> need_do_map = 1;
> > +
> > + ret
> = ev_add_path(pp, vecs, need_do_map);
> > + } else if (r ==
> PATHINFO_SKIPPED) {
> >
> condlog(3, "%s: remove blacklisted path",
> >
> uev->kernel);
> > i =
> find_slot(vecs->pathvec, (void *)pp);
> > @@ -681,7 +690,16 @@ uev_add_path (struct uevent *uev, struct vectors *
> vecs)
> > conf = get_multipath_config();
> > pp->checkint = conf->checkint;
> > put_multipath_config(conf);
> > - ret = ev_add_path(pp, vecs);
> > + /*
> > + * Make sure merging use the correct
> wwid
> > + * Othterwise calling domap()
> > + */
> > + if (!need_do_map &&
> > + uev->merge_id &&
> > + strcmp(uev->merge_id, pp->wwid))
> > + need_do_map = 1;
> > +
> > + ret = ev_add_path(pp, vecs,
> need_do_map);
> > } else {
> > condlog(0, "%s: failed to store path
> info, "
> > "dropping event",
> > @@ -699,7 +717,7 @@ uev_add_path (struct uevent *uev, struct vectors *
> vecs)
> > * 1: error
> > */
> > int
> > -ev_add_path (struct path * pp, struct vectors * vecs)
> > +ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
> > {
> > struct multipath * mpp;
> > char params[PARAMS_SIZE] = {0};
> > @@ -767,6 +785,13 @@ rescan:
> > /* persistent reservation check*/
> > mpath_pr_event_handle(pp);
> >
> > + if (!need_do_map)
> > + return 0;
> > +
> > + if (!dm_map_present(mpp->alias)) {
> > + mpp->action = ACT_CREATE;
> > + start_waiter = 1;
> > + }
> > /*
> > * push the map to the device-mapper
> > */
> > @@ -995,7 +1020,7 @@ uev_update_path (struct uevent *uev, struct vectors
> * vecs)
> > }
> >
> > if (pp->initialized ==
> INIT_REQUESTED_UDEV)
> > - retval =
> uev_add_path(uev, vecs);
> > + retval =
> uev_add_path(uev, vecs, 1);
> > else if (mpp && ro >= 0) {
> > condlog(2, "%s:
> update path write_protect to '%d' (uevent)", uev->kernel, ro);
> >
> > @@ -1150,7 +1175,7 @@ uev_trigger (struct uevent * uev, void *
> trigger_data)
> > put_multipath_config(conf);
> >
> > if (!strncmp(uev->action, "add", 3)) {
> > - r = uev_add_path(uev, vecs);
> > + r = uev_add_path(uev, vecs, 1);
> > goto out;
> > }
> > if (!strncmp(uev->action, "remove", 6)) {
> > @@ -1570,7 +1595,7 @@ check_path (struct vectors * vecs, struct path *
> pp, int ticks)
> > conf =
> get_multipath_config();
> > ret = pathinfo(pp,
> conf, DI_ALL | DI_BLACKLIST);
> > if (ret ==
> PATHINFO_OK) {
> > -
> ev_add_path(pp, vecs);
> > +
> ev_add_path(pp, vecs, 1);
> >
> pp->tick = 1;
> > } else if (ret ==
> PATHINFO_SKIPPED) {
> >
> put_multipath_config(conf);
> > @@ -1686,7 +1711,7 @@ check_path (struct vectors * vecs, struct path *
> pp, int ticks)
> > }
> > if (!disable_reinstate &&
> reinstate_path(pp, add_active)) {
> > condlog(3, "%s:
> reload map", pp->dev);
> > - ev_add_path(pp,
> vecs);
> > + ev_add_path(pp,
> vecs, 1);
> > pp->tick = 1;
> > return 0;
> > }
> > @@ -1709,7 +1734,7 @@ check_path (struct vectors * vecs, struct path *
> pp, int ticks)
> > /* Clear IO errors
> */
> > if
> (reinstate_path(pp, 0)) {
> >
> condlog(3, "%s: reload map", pp->dev);
> > -
> ev_add_path(pp, vecs);
> > +
> ev_add_path(pp, vecs, 1);
> >
> pp->tick = 1;
> >
> return 0;
> > }
> > diff --git a/multipathd/main.h b/multipathd/main.h
> > index f72580d..f810d41 100644
> > --- a/multipathd/main.h
> > +++ b/multipathd/main.h
> > @@ -22,7 +22,7 @@ void exit_daemon(void);
> > const char * daemon_status(void);
> > int need_to_delay_reconfig (struct vectors *);
> > int reconfigure (struct vectors *);
> > -int ev_add_path (struct path *, struct vectors *);
> > +int ev_add_path (struct path *, struct vectors *, int);
> > int ev_remove_path (struct path *, struct vectors *);
> > int ev_add_map (char *, char *, struct vectors *);
> > int ev_remove_map (char *, char *, int, struct vectors *);
> > --
> > 2.8.1.windows.1
> >
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* uid_attrs (was Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path())
2017-01-17 16:14 ` Benjamin Marzinski
@ 2017-04-05 13:26 ` Martin Wilck
2017-04-05 18:45 ` Benjamin Marzinski
0 siblings, 1 reply; 20+ messages in thread
From: Martin Wilck @ 2017-04-05 13:26 UTC (permalink / raw)
To: Benjamin Marzinski, tang.junhui
Cc: tang.wenjun3, zhang.kai16, dm-devel, bart.vanassche
Hi Ben & Tang,
On Tue, 2017-01-17 at 10:14 -0600, Benjamin Marzinski wrote:
> On Tue, Jan 17, 2017 at 03:28:06PM +0800, tang.junhui@zte.com.cn
> wrote:
> > Hello Ben
> >
> > Thank you for your patience again.
> >
> > I'll modify code according to your suggestion as this:
> > 1) add configuration in the defaults section
> > uid_attrs "sd:ID_SERIAL dasd:ID_UID"
> > it would override any other configurations if this
> > filed is configured and matched;
> >
> > 2) In uevent processing thread, we will assign merge_id
> > according the label in uevents by this configuration;
> >
> > 3) this patch will take back:
> > [PATCH 12/12] libmultipath: use existing wwid when
> > wwid has already been existed in uevent
> >
> > 4) if this field is not configured, only do filtering and
> > no merging works.
> >
> > Please confirm whether this modification is feasible.
>
> Yes. This is perfectly reasonable solution. Thanks for doing all the
> work on this.
I'm sorry to jump upon this old discussion again. I've just recently
realized that with the introduction of "uid_attrs", the section on
"WWID generation" in multipath.conf(5) needs to be corrected. Looking
into that, I find it unfortunate that this new option was introduced.
It makes an already complex configuration exercise even more confusing
(try to rewrite that man page paragraph in an easily understandable way
and I think you'll know what I mean).
>From the end user point of view, "uid_attrs" seems to simply duplicate
the functionality of "uid_attribute", just in more awkward syntax, and
with higher priority. As a side effect, it enables uevent merging,
which is documented in the man page but unexpected from the name of the
option.
I went through Ben's reasoning from Jan 16th again:
> In general, the code to set need_do_map if the wwid and merge_id
> don't
> match only works if either none of the device in a merged set have
> wwids
> that match the merge_id, or if the last device has a wwid that
> matches
> the merge_id. If there are devices with wwids that match the
> merge_id,
> but the last device in the set isn't one of them, then some devices
> will
> not get a multipath device created for them.
>
[...]
The easy way to fix it is to use the other possibility that I
mentioned
> before, which is to not have the merge_id, and just use the udev
> provided wwid, instead of fetching it from pathinfo. Like I
> mentioned,
> if you do this, you want to make sure that you only try to grab the
> wwid
> from the udev events for devices with the correct kernel name:
> ID_SERIAL
> only for "sd.*" devices, and ID_UID only for "dasd.*" devices. I also
> think that this should be configurable.
>
> Otherwise, you can either go through the messy work of calling domap
> correctly when the last device of a set has a wwid that doesn't match
> the merge_id, or we can decide that this won't acutally cause
> problems
> with any known device, and punt fixing it for now. And if it causes
> problems with some future devices, we can deal with it then.
It appears that Tang chose Ben's proposed "easy way" to fix the problem
described. The key point appears to be to "use the udev provided wwid,
instead of fetching it from pathinfo". But the end result may be
confusing for users.
I didn't react at the time, but in hindsight I'd find it much better if
multipath's standard WWID generation procedure had been used for uevent
merging. I admit I don't fully grok why that's harder to implement than
the solution that got merged, probably because I didn't try yet :-)
Anyway, I wonder if it would still be possible to change this at this
point in time (i.e. after "uid_attrs" was merged).
Cheers,
Martin
PS: And while I'm nitpicking anyway: The description of "uid_attrs" in
the man page talks about "type of device", while what's really matched
against is the kernel devnode name such as /dev/sdX or /dev/dasdY. The
reader is left clueless what to do for devices other than sd or dasd (I
suspect that that's actually unsupported, but the man page doesn't say
anything about that).
I can provide a patches for this (and for "WWID generation") but I'd
like to discuss the main topic of this email first.
--
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: uid_attrs (was Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path())
2017-04-05 13:26 ` uid_attrs (was Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path()) Martin Wilck
@ 2017-04-05 18:45 ` Benjamin Marzinski
2017-04-05 20:16 ` Martin Wilck
0 siblings, 1 reply; 20+ messages in thread
From: Benjamin Marzinski @ 2017-04-05 18:45 UTC (permalink / raw)
To: Martin Wilck
Cc: tang.junhui, tang.wenjun3, zhang.kai16, dm-devel, bart.vanassche
On Wed, Apr 05, 2017 at 03:26:06PM +0200, Martin Wilck wrote:
> Hi Ben & Tang,
>
> On Tue, 2017-01-17 at 10:14 -0600, Benjamin Marzinski wrote:
> > On Tue, Jan 17, 2017 at 03:28:06PM +0800, tang.junhui@zte.com.cn
> > wrote:
> > > Hello Ben
> > >
> > > Thank you for your patience again.
> > >
> > > I'll modify code according to your suggestion as this:
> > > 1) add configuration in the defaults section
> > > uid_attrs "sd:ID_SERIAL dasd:ID_UID"
> > > it would override any other configurations if this
> > > filed is configured and matched;
> > >
> > > 2) In uevent processing thread, we will assign merge_id
> > > according the label in uevents by this configuration;
> > >
> > > 3) this patch will take back:
> > > [PATCH 12/12] libmultipath: use existing wwid when
> > > wwid has already been existed in uevent
> > >
> > > 4) if this field is not configured, only do filtering and
> > > no merging works.
> > >
> > > Please confirm whether this modification is feasible.
> >
> > Yes. This is perfectly reasonable solution. Thanks for doing all the
> > work on this.
>
> I'm sorry to jump upon this old discussion again. I've just recently
> realized that with the introduction of "uid_attrs", the section on
> "WWID generation" in multipath.conf(5) needs to be corrected. Looking
> into that, I find it unfortunate that this new option was introduced.
> It makes an already complex configuration exercise even more confusing
> (try to rewrite that man page paragraph in an easily understandable way
> and I think you'll know what I mean).
>
> >From the end user point of view, "uid_attrs" seems to simply duplicate
> the functionality of "uid_attribute", just in more awkward syntax, and
> with higher priority. As a side effect, it enables uevent merging,
> which is documented in the man page but unexpected from the name of the
> option.
It should probably do a better job of masking "uid_attribute". The
places in the code that currently use "uid_attribute" should all switch
to using "uid_attrs" if it is set and applies to the device. If they
don't, we could be in a situation where "uid_attrs" and "uid_attribute"
are set to give different wwids, and a path's wwid could change based on
whether or not ID_SERIAL was filled in when multipath got the uevent. Of
course, this could only happen due to user error, so it's not that
urgent of an issue. But I completely agree with you that it is totally
non-obvious that the way to stop uevent merging is to unset uid_attrs.
> I went through Ben's reasoning from Jan 16th again:
>
> > In general, the code to set need_do_map if the wwid and merge_id
> > don't
> > match only works if either none of the device in a merged set have
> > wwids
> > that match the merge_id, or if the last device has a wwid that
> > matches
> > the merge_id. If there are devices with wwids that match the
> > merge_id,
> > but the last device in the set isn't one of them, then some devices
> > will
> > not get a multipath device created for them.
> >
> [...]
> The easy way to fix it is to use the other possibility that I
> mentioned
> > before, which is to not have the merge_id, and just use the udev
> > provided wwid, instead of fetching it from pathinfo. Like I
> > mentioned,
> > if you do this, you want to make sure that you only try to grab the
> > wwid
> > from the udev events for devices with the correct kernel name:
> > ID_SERIAL
> > only for "sd.*" devices, and ID_UID only for "dasd.*" devices. I also
> > think that this should be configurable.
> >
> > Otherwise, you can either go through the messy work of calling domap
> > correctly when the last device of a set has a wwid that doesn't match
> > the merge_id, or we can decide that this won't acutally cause
> > problems
> > with any known device, and punt fixing it for now. And if it causes
> > problems with some future devices, we can deal with it then.
>
> It appears that Tang chose Ben's proposed "easy way" to fix the problem
> described. The key point appears to be to "use the udev provided wwid,
> instead of fetching it from pathinfo". But the end result may be
> confusing for users.
>
> I didn't react at the time, but in hindsight I'd find it much better if
> multipath's standard WWID generation procedure had been used for uevent
> merging. I admit I don't fully grok why that's harder to implement than
> the solution that got merged, probably because I didn't try yet :-)
The issue is that you can't use the standard WWID generation code and
choose to merge uevents as early as tang wanted to. So you are left
with, noticing that your uevents shouldn't be merged after the fact, and
trying to untable them.
IIRC, I originally described a solution that involved a larger rewrite
of the multipathd code, where we just processed path events
individually, but never reloaded any devices until we had finished
processing all the queued events, and then went back through the paths
we changed, and made the appropriate map changes all at once. This way
would allow you to process paths just like we always did. But I didn't
feel strongly enough about it to actually write the code myself, when
Tang was already working his idea.
> Anyway, I wonder if it would still be possible to change this at this
> point in time (i.e. after "uid_attrs" was merged).
It's always possible to change things. As far as Red Hat goes, we
haven't pulled in these changes yet, but I'm planning on rebasing Fedora
to upstream shortly. So, it won't cause me any problems if uid_attrs
goes away as long as it does so in the near future. I have no idea if
other distributions have pulled this in. If so, then uid_attrs probably
needs to remain accepted as a valid option by multipath. But since all
correct configurations have it give the same answers as uid_attribute,
we could probably just make the option do nothing.
Of course, all this depends on your solution actually being enough
better than Tang's to justify pulling his version out.
-Ben
> Cheers,
> Martin
>
> PS: And while I'm nitpicking anyway: The description of "uid_attrs" in
> the man page talks about "type of device", while what's really matched
> against is the kernel devnode name such as /dev/sdX or /dev/dasdY. The
> reader is left clueless what to do for devices other than sd or dasd (I
> suspect that that's actually unsupported, but the man page doesn't say
> anything about that).
>
> I can provide a patches for this (and for "WWID generation") but I'd
> like to discuss the main topic of this email first.
>
> --
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: uid_attrs (was Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path())
2017-04-05 18:45 ` Benjamin Marzinski
@ 2017-04-05 20:16 ` Martin Wilck
2017-04-05 23:19 ` Benjamin Marzinski
0 siblings, 1 reply; 20+ messages in thread
From: Martin Wilck @ 2017-04-05 20:16 UTC (permalink / raw)
To: Benjamin Marzinski
Cc: tang.junhui, tang.wenjun3, zhang.kai16, dm-devel, bart.vanassche
On Wed, 2017-04-05 at 13:45 -0500, Benjamin Marzinski wrote:
> On Wed, Apr 05, 2017 at 03:26:06PM +0200, Martin Wilck wrote:
> > >From the end user point of view, "uid_attrs" seems to simply
> duplicate
> > the functionality of "uid_attribute", just in more awkward syntax,
> and
> > with higher priority. As a side effect, it enables uevent merging,
> > which is documented in the man page but unexpected from the name of
> the
> > option.
>
> It should probably do a better job of masking "uid_attribute". The
> places in the code that currently use "uid_attribute" should all
> switch
> to using "uid_attrs" if it is set and applies to the device.
> If they
> don't, we could be in a situation where "uid_attrs" and
> "uid_attribute"
> are set to give different wwids, and a path's wwid could change based
> on
> whether or not ID_SERIAL was filled in when multipath got
I'm not quite sure what you mean. The way it's currently implemented is
in select_getuid(), pp->uid_attribute will be set from uid_attrs if it
exists and matches the device name, and will henceforth be used
everywhere where uid_attribute for this path is referenced.
I don't see how this could lead to inconsistent settings.
Even for RBD devices, uid_attrs would take precedence if users could
uid_attrs="rbd:SOME_VARIABLE". (Btw, why do we hardcode the WWID
generation for RBD rather than writing rbd udev rules and using the
udev attributes created by them)?
> > Anyway, I wonder if it would still be possible to change this at
> > this
> > point in time (i.e. after "uid_attrs" was merged).
>
> It's always possible to change things. As far as Red Hat goes, we
> haven't pulled in these changes yet, but I'm planning on rebasing
> Fedora
> to upstream shortly.
We're planning the same for openSUSE Factory. Currently we've left out
Tang's patch, but the plan is to pull in upstream cleanly.
> So, it won't cause me any problems if uid_attrs
> goes away as long as it does so in the near future. I have no idea if
> other distributions have pulled this in. If so, then uid_attrs
> probably
> needs to remain accepted as a valid option by multipath. But since
> all
> correct configurations have it give the same answers as
> uid_attribute,
> we could probably just make the option do nothing.
>
> Of course, all this depends on your solution actually being enough
> better than Tang's to justify pulling his version out.
Point taken :-) I don't have patches at this time. However, let me make
a suggestion. Does "uid_attrs" really have to be user-configurable? IMO
configuring this is asked too much from 99% of users. I'd suggest to
hard-code the value of uid_attrs and use a boolean configuration option
in the defaults section ("auto_uid_attribute", say) which would control
whether WWID attributes should be configured "the uid_attrs way" or the
"traditional way" (traditional being the fallback if uid_attrs didn't
match, like now). Hard-coding this would have the advantage that we
wouldn't be bound to the "sd:..." syntax. Rather, we could explore more
intelligent ways to autodetect the uid_attribute depending on path
properties which are known early on.
Uevent merging could be controlled by a separate config option which
would depend on the "auto_uid_attribute" option. This would mainly be
done for clarity.
Regards
Martin
--
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: uid_attrs (was Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path())
2017-04-05 20:16 ` Martin Wilck
@ 2017-04-05 23:19 ` Benjamin Marzinski
0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Marzinski @ 2017-04-05 23:19 UTC (permalink / raw)
To: Martin Wilck
Cc: tang.junhui, tang.wenjun3, zhang.kai16, dm-devel, bart.vanassche
On Wed, Apr 05, 2017 at 10:16:33PM +0200, Martin Wilck wrote:
> On Wed, 2017-04-05 at 13:45 -0500, Benjamin Marzinski wrote:
> > On Wed, Apr 05, 2017 at 03:26:06PM +0200, Martin Wilck wrote:
>
> > > >From the end user point of view, "uid_attrs" seems to simply
> > duplicate
> > > the functionality of "uid_attribute", just in more awkward syntax,
> > and
> > > with higher priority. As a side effect, it enables uevent merging,
> > > which is documented in the man page but unexpected from the name of
> > the
> > > option.
> >
> > It should probably do a better job of masking "uid_attribute". The
> > places in the code that currently use "uid_attribute" should all
> > switch
> > to using "uid_attrs" if it is set and applies to the device.
> > If they
> > don't, we could be in a situation where "uid_attrs" and
> > "uid_attribute"
> > are set to give different wwids, and a path's wwid could change based
> > on
> > whether or not ID_SERIAL was filled in when multipath got
>
> I'm not quite sure what you mean. The way it's currently implemented is
> in select_getuid(), pp->uid_attribute will be set from uid_attrs if it
> exists and matches the device name, and will henceforth be used
> everywhere where uid_attribute for this path is referenced.
> I don't see how this could lead to inconsistent settings.
>
> Even for RBD devices, uid_attrs would take precedence if users could
> uid_attrs="rbd:SOME_VARIABLE". (Btw, why do we hardcode the WWID
> generation for RBD rather than writing rbd udev rules and using the
> udev attributes created by them)?
>
That's what I get for responding without double-checking the code. I
forgot that uid_attrs was wired into select_getuid and thought that if
the path got set to INIT_MISSING_UDEV when it was initially discovered,
it would use uid_attribute. My bad.
> > > Anyway, I wonder if it would still be possible to change this at
> > > this
> > > point in time (i.e. after "uid_attrs" was merged).
> >
> > It's always possible to change things. As far as Red Hat goes, we
> > haven't pulled in these changes yet, but I'm planning on rebasing
> > Fedora
> > to upstream shortly.
>
> We're planning the same for openSUSE Factory. Currently we've left out
> Tang's patch, but the plan is to pull in upstream cleanly.
>
> > So, it won't cause me any problems if uid_attrs
> > goes away as long as it does so in the near future. I have no idea if
> > other distributions have pulled this in. If so, then uid_attrs
> > probably
> > needs to remain accepted as a valid option by multipath. But since
> > all
> > correct configurations have it give the same answers as
> > uid_attribute,
> > we could probably just make the option do nothing.
> >
> > Of course, all this depends on your solution actually being enough
> > better than Tang's to justify pulling his version out.
>
> Point taken :-) I don't have patches at this time. However, let me make
> a suggestion. Does "uid_attrs" really have to be user-configurable? IMO
> configuring this is asked too much from 99% of users. I'd suggest to
> hard-code the value of uid_attrs and use a boolean configuration option
> in the defaults section ("auto_uid_attribute", say) which would control
> whether WWID attributes should be configured "the uid_attrs way" or the
> "traditional way" (traditional being the fallback if uid_attrs didn't
> match, like now). Hard-coding this would have the advantage that we
> wouldn't be bound to the "sd:..." syntax. Rather, we could explore more
> intelligent ways to autodetect the uid_attribute depending on path
> properties which are known early on.
The idea behind making it configurable was to allow people to change
this in the same way that they can change uid_attribute, under the
assumption that if users need to be able to change uid_attribute, then
they would equally need to be able to change uid_attrs. Of course, I
don't know of any real use-case for this, and it's pretty hard to defend
adding user-visible complexity to fix a problem that nobody has. So,
sure I'm fine with your suggestion.
-Ben
> Uevent merging could be controlled by a separate config option which
> would depend on the "auto_uid_attribute" option. This would mainly be
> done for clarity.
>
> Regards
> Martin
>
> --
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2017-04-05 23:19 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-12 5:52 [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices tang.junhui
2017-01-12 5:52 ` [PATCH 01/11] libmultipath: add merge_id in "struct uevent" for uevents merging tang.junhui
2017-01-12 5:52 ` [PATCH 02/11] libmultipath: add merge_node for "struct uevent" to record nodes of merged uevents tang.junhui
2017-01-12 5:52 ` [PATCH 03/11] libmultipath: add three list iteration macros tang.junhui
2017-01-12 5:52 ` [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path() tang.junhui
2017-01-16 21:38 ` Benjamin Marzinski
2017-01-17 7:28 ` tang.junhui
2017-01-17 16:14 ` Benjamin Marzinski
2017-04-05 13:26 ` uid_attrs (was Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path()) Martin Wilck
2017-04-05 18:45 ` Benjamin Marzinski
2017-04-05 20:16 ` Martin Wilck
2017-04-05 23:19 ` Benjamin Marzinski
2017-01-12 5:52 ` [PATCH 05/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_remove_path() tang.junhui
2017-01-16 18:18 ` Benjamin Marzinski
2017-01-12 5:52 ` [PATCH 06/11] multipathd: move uev_discard() to uevent.c and change its name to uevent_can_discard() tang.junhui
2017-01-12 5:52 ` [PATCH 07/11] multipathd: move calling filter_devnode() from uev_trigger() " tang.junhui
2017-01-12 5:52 ` [PATCH 08/11] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations tang.junhui
2017-01-12 5:52 ` [PATCH 09/11] multipathd: merge uevents before proccessing tang.junhui
2017-01-12 5:52 ` [PATCH 10/11] libmultipath: filter " tang.junhui
2017-01-12 5:52 ` [PATCH 11/11] multipathd: proccess merged uevents tang.junhui
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.