[dm-devel] [PATCH] multipath-tools: improve processing efficiency for addition and deletion of multipath devices
From: "tang.junhui" This patch used to improve processing efficiency for addition and deletion of multipath devices. This patch is tested pass by ZTE multipath automatic testing system. 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) add uid_attrs configuration in the defaults section: It is configured udev attribute which providing a unique path identifier for corresponding type of path devices. If this field is configured and matched with type of device, it would override any other methods providing for device unique identifier in config file, and it would activate merging uevents according to the identifier to promote effiecncy in processing uevents. Tt has no default value, so defaultly only uevents filtering works, and uevents merging does not works, if users want to identify path by udev attribute and to activate merging uevents for SCSI and DAS device, they can set it's value as: "sd:ID_SERIAL dasd:ID_UID" 2) uevents accumulation in uevents burst scene: wait one seconds for more uevents in uevent_listen() in uevents burst situations 3) uevents preparing, filtering and merging: discard unuse uevents and fetch path idendifier from uevents; filter uevents; merge uevents. 4) uevents proccessing: proccess the merged uevents in uev->merge_node list without calling domap(); proccess the last uevents uev with calling domap(). Signed-off-by: tang.junhui --- libmultipath/config.c | 3 + libmultipath/config.h | 1 + libmultipath/dict.c| 3 + libmultipath/discovery.c | 5 +- libmultipath/discovery.h | 2 +- libmultipath/list.h| 41 ++ libmultipath/propsel.c | 7 + libmultipath/uevent.c | 320 +++-- libmultipath/uevent.h | 2 + libmultipath/util.c| 42 ++ libmultipath/util.h| 1 + multipath/multipath.conf.5 | 18 +++ multipathd/cli_handlers.c | 4 +- multipathd/main.c | 93 + multipathd/main.h | 4 +- 15 files changed, 471 insertions(+), 75 deletions(-) diff --git a/libmultipath/config.c b/libmultipath/config.c index 9d3f3e1..bb6619b 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -493,6 +493,9 @@ free_config (struct config * conf) if (conf->uid_attribute) FREE(conf->uid_attribute); + if (conf->uid_attrs) + FREE(conf->uid_attrs); + if (conf->getuid) FREE(conf->getuid); diff --git a/libmultipath/config.h b/libmultipath/config.h index 9a90745..4f1d596 100644 --- a/libmultipath/config.h +++ b/libmultipath/config.h @@ -164,6 +164,7 @@ struct config { char * multipath_dir; char * selector; + char * uid_attrs; char * uid_attribute; char * getuid; char * features; diff --git a/libmultipath/dict.c b/libmultipath/dict.c index bababdb..82066f6 100644 --- a/libmultipath/dict.c +++ b/libmultipath/dict.c @@ -249,6 +249,8 @@ declare_ovr_snprint(selector, print_str) declare_mp_handler(selector, set_str) declare_mp_snprint(selector, print_str) +declare_def_handler(uid_attrs, set_str) +declare_def_snprint(uid_attrs, print_str) declare_def_handler(uid_attribute, set_str) declare_def_snprint_defstr(uid_attribute, print_str, DEFAULT_UID_ATTRIBUTE) declare_ovr_handler(uid_attribute, set_str) @@ -1396,6 +1398,7 @@ init_keywords(vector keywords) install_keyword("multipath_dir", &def_multipath_dir_handler, &snprint_def_multipath_dir); install_keyword("path_selector", &def_selector_handler, &snprint_def_selector); install_keyword("path_grouping_policy", &def_pgpolicy_handler, &snprint_def_pgpolicy); + install_keyword("uid_attrs", &def_uid_attrs_handler, &snprint_def_uid_attrs); install_keyword("uid_attribute", &def_uid_attribute_handler, &snprint_def_uid_attribute); install_keyword("getuid_callout", &def_getuid_handler, &snprint_def_getuid); install_keyword("prio", &def_prio_name_handler, &snprint_def_prio_name); diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index 4e99845..7398040 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -34,7 +34,7 @@ int alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, - int flag, struct path **pp_ptr) + char *wwid, int flag, struct path **pp_ptr) { int err = PATHINFO_FAILED; struct path * pp; @@ -52,6 +52,9 @@ alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, if (!pp) return PATHINFO_FAILED; + if(wwid) + strncpy(pp->wwid, wwid, sizeof(pp->wwid)); + if (safe_sprintf(pp->dev, "%s", devname)) { condlog(0, "
[dm-devel] [PATCH] multipath-tools: improve processing efficiency for addition and deletion of multipath devices
From: "tang.junhui" Change-Id: I3f81a55fff389f991f915927000b281d7e263cc5 Signed-off-by: tang.junhui This patch used to improve processing efficiency for addition and deletion of multipath devices. This patch is tested pass by ZTE multipath automatic testing system. 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) add uid_attrs configuration in the defaults section: It is configured udev attribute which providing a unique path identifier for corresponding type of path devices. If this field is configured and matched with type of device, it would override any other methods providing for device unique identifier in config file, and it would activate merging uevents according to the identifier to promote effiecncy in processing uevents. Tt has no default value, so defaultly only uevents filtering works, and uevents merging does not works, if users want to identify path by udev attribute and to activate merging uevents for SCSI and DAS device, they can set it's value as: "sd:ID_SERIAL dasd:ID_UID" 2) uevents accumulation in uevents burst scene: wait one seconds for more uevents in uevent_listen() in uevents burst situations 3) uevents preparing, filtering and merging: discard unuse uevents and fetch path idendifier from uevents; filter uevents; merge uevents. 4) uevents proccessing: proccess the merged uevents in uev->merge_node list without calling domap(); proccess the last uevents uev with calling domap(). --- libmultipath/config.c | 3 + libmultipath/config.h | 1 + libmultipath/dict.c| 3 + libmultipath/discovery.c | 5 +- libmultipath/discovery.h | 2 +- libmultipath/list.h| 41 ++ libmultipath/propsel.c | 7 + libmultipath/uevent.c | 320 +++-- libmultipath/uevent.h | 2 + libmultipath/util.c| 42 ++ libmultipath/util.h| 1 + multipath/multipath.conf.5 | 18 +++ multipathd/cli_handlers.c | 4 +- multipathd/main.c | 93 + multipathd/main.h | 4 +- 15 files changed, 471 insertions(+), 75 deletions(-) diff --git a/libmultipath/config.c b/libmultipath/config.c index 15ddbd8..765e91d 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -488,6 +488,9 @@ free_config (struct config * conf) if (conf->uid_attribute) FREE(conf->uid_attribute); + if (conf->uid_attrs) + FREE(conf->uid_attrs); + if (conf->getuid) FREE(conf->getuid); diff --git a/libmultipath/config.h b/libmultipath/config.h index 9670020..ab85930 100644 --- a/libmultipath/config.h +++ b/libmultipath/config.h @@ -153,6 +153,7 @@ struct config { char * multipath_dir; char * selector; + char * uid_attrs; char * uid_attribute; char * getuid; char * features; diff --git a/libmultipath/dict.c b/libmultipath/dict.c index dc21846..0a531d1 100644 --- a/libmultipath/dict.c +++ b/libmultipath/dict.c @@ -249,6 +249,8 @@ declare_ovr_snprint(selector, print_str) declare_mp_handler(selector, set_str) declare_mp_snprint(selector, print_str) +declare_def_handler(uid_attrs, set_str) +declare_def_snprint(uid_attrs, print_str) declare_def_handler(uid_attribute, set_str) declare_def_snprint_defstr(uid_attribute, print_str, DEFAULT_UID_ATTRIBUTE) declare_ovr_handler(uid_attribute, set_str) @@ -1367,6 +1369,7 @@ init_keywords(vector keywords) install_keyword("multipath_dir", &def_multipath_dir_handler, &snprint_def_multipath_dir); install_keyword("path_selector", &def_selector_handler, &snprint_def_selector); install_keyword("path_grouping_policy", &def_pgpolicy_handler, &snprint_def_pgpolicy); + install_keyword("uid_attrs", &def_uid_attrs_handler, &snprint_def_uid_attrs); install_keyword("uid_attribute", &def_uid_attribute_handler, &snprint_def_uid_attribute); install_keyword("getuid_callout", &def_getuid_handler, &snprint_def_getuid); install_keyword("prio", &def_prio_name_handler, &snprint_def_prio_name); diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index d1aec31..14904f2 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -33,7 +33,7 @@ int alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, - int flag, struct path **pp_ptr) + char *wwid, int flag, struct path **pp_ptr) { int err = PATHINFO_FAILED; struct path * pp; @@ -51,6 +51,9 @@ alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, if (!pp) return PATHINFO_FAILED; + if(wwid) + strncpy(pp->wwid, wwid, sizeof(pp->wwid)); + if (safe_sprintf(pp
[dm-devel] [PATCH] multipath-tools: improve processing efficiency for addition and deletion of multipath devices
From: "tang.junhui" Change-Id: I3f81a55fff389f991f915927000b281d7e263cc5 Signed-off-by: tang.junhui This patch used to improve processing efficiency for addition and deletion of multipath devices. This patch is tested pass by ZTE multipath automatic testing system. 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) add uid_attrs configuration in the defaults section: It is configured udev attribute which providing a unique path identifier for corresponding type of path devices. If this field is configured and matched with type of device, it would override any other methods providing for device unique identifier in config file, and it would activate merging uevents according to the identifier to promote effiecncy in processing uevents. Tt has no default value, so defaultly only uevents filtering works, and uevents merging does not works, if users want to identify path by udev attribute and to activate merging uevents for SCSI and DAS device, they can set it's value as: "sd:ID_SERIAL dasd:ID_UID" 2) uevents accumulation in uevents burst scene: wait one seconds for more uevents in uevent_listen() in uevents burst situations 3) uevents preparing, filtering and merging: discard unuse uevents and fetch path idendifier from uevents; filter uevents; merge uevents. 4) uevents proccessing: proccess the merged uevents in uev->merge_node list without calling domap(); proccess the last uevents uev with calling domap(). 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 --- libmultipath/config.c | 3 + libmultipath/config.h | 1 + libmultipath/dict.c| 3 + libmultipath/discovery.c | 5 +- libmultipath/discovery.h | 2 +- libmultipath/list.h| 41 ++ libmultipath/propsel.c | 7 + libmultipath/uevent.c | 319 +++-- libmultipath/uevent.h | 2 + libmultipath/util.c| 40 ++ libmultipath/util.h| 1 + multipath/multipath.conf.5 | 18 +++ multipathd/cli_handlers.c | 4 +- multipathd/main.c | 93 + multipathd/main.h | 4 +- 15 files changed, 468 insertions(+), 75 deletions(-) diff --git a/libmultipath/config.c b/libmultipath/config.c index 15ddbd8..765e91d 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -488,6 +488,9 @@ free_config (struct config * conf) if (conf->uid_attribute) FREE(conf->uid_attribute); + if (conf->uid_attrs) + FREE(conf->uid_attrs); + if (conf->getuid) FREE(conf->getuid); diff --git a/libmultipath/config.h b/libmultipath/config.h index 9670020..ab85930 100644 --- a/libmultipath/config.h +++ b/libmultipath/config.h @@ -153,6 +153,7 @@ struct config { char * multipath_dir; char * selector; + char * uid_attrs; char * uid_attribute; char * getuid; char * features; diff --git a/libmultipath/dict.c b/libmultipath/dict.c index dc21846..0a531d1 100644 --- a/libmultipath/dict.c +++ b/libmultipath/dict.c @@ -249,6 +249,8 @@ declare_ovr_snprint(selector, print_str) declare_mp_handler(selector, set_str) declare_mp_snprint(selector, print_str) +declare_def_handler(uid_attrs, set_str) +declare_def_snprint(uid_attrs, print_str) declare_def_handler(uid_attribute, set_str) declare_def_snprint_defstr(uid_attribute, print_str, DEFAULT_UID_ATTRIBUTE) declare_ovr_handler(uid_attribute, set_str) @@ -1367,6 +1369,7 @@ init_keywords(vector keywords) install_keyword("multipath_dir", &def_multipath_dir_handler, &snprint_def_multipath_dir); install_keyword("path_selector", &def_selector_handler, &snprint_def_selector); install_keyword("path_grouping_policy", &def_pgpolicy_handler, &snprint_def_pgpolicy); + install_keyword("uid_attrs", &def_uid_attrs_handler, &snprint_def_uid_attrs); install_keyword("uid_attribute", &def_uid_attribute_handler, &snprint_def_uid_attribute); install_keyword("getuid_callout", &def_getuid_handler, &snprint_def_getuid); install_keyword("prio", &def_prio_name_handler, &snprint_def_prio_name); diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index d1aec31..14904f2 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -33,7 +33,7 @@ int alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, - int flag, struct path **pp_ptr) + char *wwid, int flag
Re: [dm-devel] [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path()
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" 收件人: tang.jun...@zte.com.cn, 抄送: christophe.varo...@opensvc.com, h...@suse.de, mwi...@suse.com, bart.vanass...@sandisk.com, dm-devel@redhat.com, zhang.ka...@zte.com.cn, tang.wenj...@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.jun...@zte.com.cn wrote: > From: tang.junhui > > 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 > --- > 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; >
Re: [dm-devel] [LSF/MM TOPIC][LSF/MM ATTEND] multipath redesign
Hello Hannes: As a mulitpath developer, I find that multipath is getting bigger and harder to maintain now, and I'm really looking forward to this change, and I hope to be able to devote myself to this change too. I am very interested in any news of the multipath redesign and hope to see results soon. I can't imagine what the new multipath looks like, but I suggest some bad places for the current multipath we should avoid: 1) coarse grained lock; 2) vectors; 3) waiter thread; 4) high coupling; 5) too many configurations; I really hope we can make a clean, efficient and easy to use multipath. Thank you Tang Junhui 发件人: Hannes Reinecke 收件人: Mike Snitzer , 抄送: "linux-bl...@vger.kernel.org" , "lsf...@lists.linux-foundation.org" , device-mapper development , "linux-s...@vger.kernel.org" 日期: 2017/01/14 01:52 主题: Re: [dm-devel] [LSF/MM TOPIC][LSF/MM ATTEND] multipath redesign 发件人: dm-devel-boun...@redhat.com On 01/13/2017 05:07 PM, Mike Snitzer wrote: > On Fri, Jan 13 2017 at 10:56am -0500, > Hannes Reinecke wrote: > >> On 01/12/2017 06:29 PM, Benjamin Marzinski wrote: [ .. ] >>> While I've daydreamed of rewriting the multipath tools multiple times, >>> and having nothing aginst you doing it in concept, I would be happier >>> knowing that it won't simply mean that there are two sets of tools, that >>> both need to be supported to deal with all customer configurations. >>> >> Sure. I feel the pain of supporting multipath-tools all too strongly. >> Having two tools for the same thing is always a pain, and I would like >> to avoid this if at all possible. > > I welcome your work. Should help us focus on what fat needs to be > trimmed from both multipath-tools and kernel. > > Might be a good time to branch multipath-tools and get very aggressive > with trimming stuff that is outdated. > > Things like the event stuff, using select interface, that Andy Grover is > working on (and Mikulas is taking a stab at finishing/optimizing) is > something that might help... but your approach described in this thread > may prove better. > > Point is, everything should be on the table for revitalizing multipath > userspace (and kernel) to meet new requirements (e.g. NVMEoF, etc). > > And yes, I'd prefer to ultimately see these advances land in terms of DM > multipath advances but we'll take it as it comes. I'm fully on board with that. And it would be good if Ben Marzinski would be present, too; he might have some insights which both of us might lack (like the ominous dm-event interface into multipathd where we both struggle to figure out what it's for ...) Looking forward to that discussion. And promising to have some results by then. Cheers, Hannes -- Dr. Hannes ReineckezSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 00/11] multipath-tools: improve processing efficiency for addition and deletion of multipath devices
From: tang.junhui 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 03/11] libmultipath: add three list iteration macros
From: tang.junhui 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 --- 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 01/11] libmultipath: add merge_id in "struct uevent" for uevents merging
From: tang.junhui 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 --- 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path()
From: tang.junhui 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 --- 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,
[dm-devel] [PATCH 06/11] multipathd: move uev_discard() to uevent.c and change its name to uevent_can_discard()
From: tang.junhui This patch is mainly done to adjust the code for the preparation of uevents merging. Change-Id: Iaac159ffe3930e53c3325d1069c3ed497e440c0c Signed-off-by: tang.wenjun --- 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 #include +#include #include #include #include @@ -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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 08/11] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations
From: tang.junhui 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 --- 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 #include #include +#include #include #include @@ -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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 09/11] multipathd: merge uevents before proccessing
From: tang.junhui 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 --- 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_
[dm-devel] [PATCH 10/11] libmultipath: filter uevents before proccessing
From: tang.junhui 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 --- 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 07/11] multipathd: move calling filter_devnode() from uev_trigger() to uevent_can_discard()
From: tang.junhui 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 --- 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 02/11] libmultipath: add merge_node for "struct uevent" to record nodes of merged uevents
From: tang.junhui 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 --- 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 05/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_remove_path()
From: tang.junhui 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 --- 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 11/11] multipathd: proccess merged uevents
From: "tang.junhui" 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 --- 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
Re: [dm-devel] [PATCH 01/12] libmultipath: add wwid for "struct uevent" to record wwid of uevent
Hello Ben, OK, I will modify code to call domap() on the device if its wwid is different from its merge_id. I'll resend a serials of patch later, Thank you for your patience. Regards, Tang Junhui 发件人: "Benjamin Marzinski" 收件人: tang.jun...@zte.com.cn, 抄送: tang.wenj...@zte.com.cn, zhang.ka...@zte.com.cn, dm-devel-boun...@redhat.com, dm-devel@redhat.com, bart.vanass...@sandisk.com, mwi...@suse.com 日期: 2017/01/07 00:11 主题: Re: [dm-devel] [PATCH 01/12] libmultipath: add wwid for "struct uevent" to record wwid of uevent 发件人: dm-devel-boun...@redhat.com On Fri, Jan 06, 2017 at 08:59:27AM +0800, tang.jun...@zte.com.cn wrote: >Hello Ben, > >>>The premise is that, devices whose "merge_id" is the same have >the >>>same "wwid". I think this premise can be established. > >>So, if you have device where the merge_id doesn't equal the wwid, you >>call domap on that device, as it it wasn't part of a merged set? > >Maybe I did not say clearly. "merge_id" is only used for uevents merging, >and >it has nothing to do with "wwid". I only primse devices whose "merge_id" >are >same have same "wwid" (Sorry, maybe I had a misleading statement in >previous email). >"merge_id" and "wwid" can be same or not same. The merged uevents are >still >processed as bellow patch I send previous: >[dm-devel] [PATCH 11/12] multipathd: proccess merged uevents Let me back up. Say you have a batch of merged uevents "add path1, path2, path3" where path1 and path2 are on path3's merge_node list. This means that when you call uev_add_path() for path1 and path2, you will call it with need_do_map set to 0, and no multipath device will be created. The multipath device will be created when you call uev_add_path() for path3, with need_no_map set to 1. Now lets say that when you call pathinfo on path2, you get a different wwid than your merge_id. path1 and path3 get the same wwid as their merge_id. In this situation you would call domap on path1 and path3, but nothing would create the multipath device for path2. Like I said, this is unlikely to happen. But it could. Imagine that there was some device that presented itself like a scsi device, but ID_SERIAL could have the same value for devices that should not be multipathed together. To figure out which devices actually should be multipathed together, you need to call pathinfo and get the correct wwid. In this case you would find that you had incorrectly batched devices with different wwids together. To solve this, you would need to make sure to call domap on the device if its wwid ended up being different from its merge_id. The issue that my last email pointed out was that if the main uevent, path3 in the above example, was the only one to have a different wwid, then you would need to make sure to call domap on the other devices as well. Otherwise you would create a multipath device for path3, but not path1 and path2. If you don't handle these cases, you will only work correctly on the (admittedly vastly more common) case where even if the wwid and merge_id don't match, the wwid is still the same for all the batched devices. But if you are already assuming that the merge_id will correctly group the devices, then what is the purpose in fetching the wwid at all? It may be different from the merge_id, but it will still group the devices the same way. There's nothing special about the wwid value itself. It is just a tool to correctly group the devices. So, I would say that you should either not grab the wwid at all if you got it from the uevent, or do grab the wwid, but be prepared to deal with the case where not all of the devices that you have batched having the same wwid. Does that make more sense? -Ben > >Regards, >Tang Junhui > >发件人: "Benjamin Marzinski" >收件人: tang.jun...@zte.com.cn, >抄送:mwi...@suse.com, bart.vanass...@sandisk.com, >dm-devel@redhat.com, dm-devel-boun...@redhat.com, tang.wenj...@zte.com.cn, >zhang.ka...@zte.com.cn >日期: 2017/01/06 01:37 >主题:Re: Re: [dm-devel] [PATCH 01/12] libmultipath: add wwid for >"struct uevent" to record wwid of uevent > > -- > >On Thu, Jan 05, 2017 at 11:10:43AM +0800, tang.jun...@zte.com.cn wrote: >>Hello Ben, >> >>I know what you concern about. Maybe we can change the "wwid" member >>in "struct uevent" to another name such as "merge_id" to only >>identify which uevents can merge toge
Re: [dm-devel] [PATCH 01/12] libmultipath: add wwid for "struct uevent" to record wwid of uevent
Hello Ben, >>The premise is that, devices whose "merge_id" is the same have the >>same "wwid". I think this premise can be established. >So, if you have device where the merge_id doesn't equal the wwid, you >call domap on that device, as it it wasn't part of a merged set? Maybe I did not say clearly. "merge_id" is only used for uevents merging, and it has nothing to do with "wwid". I only primse devices whose "merge_id" are same have same "wwid" (Sorry, maybe I had a misleading statement in previous email). "merge_id" and "wwid" can be same or not same. The merged uevents are still processed as bellow patch I send previous: [dm-devel] [PATCH 11/12] multipathd: proccess merged uevents Regards, Tang Junhui 发件人: "Benjamin Marzinski" 收件人: tang.jun...@zte.com.cn, 抄送: mwi...@suse.com, bart.vanass...@sandisk.com, dm-devel@redhat.com, dm-devel-boun...@redhat.com, tang.wenj...@zte.com.cn, zhang.ka...@zte.com.cn 日期: 2017/01/06 01:37 主题: Re: Re: [dm-devel] [PATCH 01/12] libmultipath: add wwid for "struct uevent" to record wwid of uevent On Thu, Jan 05, 2017 at 11:10:43AM +0800, tang.jun...@zte.com.cn wrote: >Hello Ben, > >I know what you concern about. Maybe we can change the "wwid" member >in "struct uevent" to another name such as "merge_id" to only >identify which uevents can merge together. The value of "merge_id" can >set by "ID_SERIAL" or "ID_UID" in uevent. If we get the "merge_id" >from uevents, we can merge and process it as merging way, otherwise >we process it as old way (processed one by one). >And we revert this patch: >"[dm-devel] [PATCH 12/12] libmultipath: use existing wwid when wwid >has already been existed in uevent" >thus wwid of each path will get by methods of configuration as old >ways. > >The premise is that, devices whose "merge_id" is the same have the >same "wwid". I think this premise can be established. So, if you have device where the merge_id doesn't equal the wwid, you call domap on that device, as it it wasn't part of a merged set? That would work, but you would have to be careful to deal with the case where the last uevent in the set, the one that would be used to call domap for the whole merged set, suddenly gets processed all on it's own, because it had a different wwid from the rest of the set. In that case You would need to make sure that something called domap on the other members of the merged set. Now, I admit that this is a very unlikely thing to happen. I thought about proposing that we revert patch 12, but it does add complexity if you find that only some of your merged devices change wwid. Also, if all of them change to the same new wwid, you just skipped merging when you should be able to. But I'm not against reverting it, as long as we handle everything o.k. Reverting it would mean that we don't have to worry about removing a capability that multipath previously had. There's just a trade-off between supporting a capability (that we don't know if anyone uses), and maintaining less complex code. As long as it is possible to configure what uevent variable you check for by device type, I don't know of a situation where this will cause real world problems. It's possible that some users are changing uid_attribute for only certain scsi-based arrays, but I don't know of any cases. If somebody reading this knows of any cases, please speak up. But I would like to have the variable you check be configurable so that it's flexible with regards to new device types or the possiblity of differing uevent variable names either between distributions or over time. Does that sound reasonable? -Ben > >how do you think about the above proposal? > >>The other option would be to not actually merge the uevents, but >simply >>run through the filtered but unmerged list of uevents, and skip the >>domap stuff but remember the maps that need pushing to device-mapper. >>Once you are done processing all the uevents, except for updating the >>maps in device-mapper, you go back and update all the maps that need >>updating. There's more code refactoring in this approach, but it >keeps >>the uid being set in pathinfo, where you have all the information >>necessary to set it using uid_attribute, getuid, or specialized code >>like rbd uses. >I think it is ok, but a little complex. and if we can get rid of the >"wwid" issue, we do not need to do so. > >
Re: [dm-devel] [PATCH 01/12] libmultipath: add wwid for "struct uevent" to record wwid of uevent
Hello Ben, I know what you concern about. Maybe we can change the "wwid" member in "struct uevent" to another name such as "merge_id" to only identify which uevents can merge together. The value of "merge_id" can set by "ID_SERIAL" or "ID_UID" in uevent. If we get the "merge_id" from uevents, we can merge and process it as merging way, otherwise we process it as old way (processed one by one). And we revert this patch: "[dm-devel] [PATCH 12/12] libmultipath: use existing wwid when wwid has already been existed in uevent" thus wwid of each path will get by methods of configuration as old ways. The premise is that, devices whose "merge_id" is the same have the same "wwid". I think this premise can be established. how do you think about the above proposal? >The other option would be to not actually merge the uevents, but simply >run through the filtered but unmerged list of uevents, and skip the >domap stuff but remember the maps that need pushing to device-mapper. >Once you are done processing all the uevents, except for updating the >maps in device-mapper, you go back and update all the maps that need >updating. There's more code refactoring in this approach, but it keeps >the uid being set in pathinfo, where you have all the information >necessary to set it using uid_attribute, getuid, or specialized code >like rbd uses. I think it is ok, but a little complex. and if we can get rid of the "wwid" issue, we do not need to do so. Regards Tang Junhui 发件人: "Benjamin Marzinski" 收件人: tang.jun...@zte.com.cn, 抄送: tang.wenj...@zte.com.cn, zhang.ka...@zte.com.cn, dm-devel@redhat.com, bart.vanass...@sandisk.com, mwi...@suse.com 日期: 2017/01/05 02:23 主题: Re: [dm-devel] [PATCH 01/12] libmultipath: add wwid for "struct uevent" to record wwid of uevent 发件人: dm-devel-boun...@redhat.com On Wed, Jan 04, 2017 at 02:56:46PM +0800, tang.jun...@zte.com.cn wrote: >Hello Ben, > >> Right now, multipath users are allowed configure devices to set the wwid >> based on any udev environment variable (or even use a callout, although >> this is deprecated). With this patch, that breaks. >Does WWID obtained by different methods change? If it changes, we would >better to modify code to keep it no change. Yes. users can pick any udev environment variable (and currently, any callout program that they write) to use as the wwid. >> If the udev sets >> ID_SERIAL for a device, that is its wwid, right? >Yes > >> Do you know if rbd >> devices have ID_SERIAL set? >WWID has different label in uevents for different devices, I only test for >SCSI devices. Where is that check? I only see a check for whether ID_SERIAL exists. If it exists on things that are not scsi devices, you will set the wwid for these devices with it as well, even if it doesn't work for them. >Now we do not support rbd divice for uevents merging, these >device process as old way, it has no harm in logic. If we need to merge >rbd uevents for these devices, we can add code to get wwid from uevents >and it can be supported easily. Look at get_rbd_uid(), and how it's called. rbd devices don't even use the getuid callout or uid_attribute. They use special code called by get_uid. Now you could add explicit checks so we only check ID_SERIAL for scsi devices, ID_UID for dasd devices, and never set the wwid otherwise. You could even make the attribute we check configurable by device type with an option like uid_attrs "sd:ID_SERIAL dasd:ID_UID" in the defaults section, that would set up mappings between device types and uevent attributes to check for the uid. But this would be on per device types, not per storage array, like it currently is. uid_attribute and getuid attribute would only ever be used for device types that weren't in the uid_attrs list. The other option would be to not actually merge the uevents, but simply run through the filtered but unmerged list of uevents, and skip the domap stuff but remember the maps that need pushing to device-mapper. Once you are done processing all the uevents, except for updating the maps in device-mapper, you go back and update all the maps that need updating. There's more code refactoring in this approach, but it keeps the uid being set in pathinfo, where you have all the information necessary to set it using uid_attribute, getuid, or specialized code like rbd uses. As long as we make sure that we are only checking specific uevent attributes for specific device types, I'm o.k. with your way, but we are losing flexibility that multipath has always had in regards to setting the wwid. I want to point that out so
Re: [dm-devel] [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations
Hello Ben, I know the issue of socket's buffer fills up, but I think uevent_can_discard() totally process in memory, it is light-weight and low cpu consumption, and it reduce the iteration count in merging, the test result is also good in massive multipath devices scene. But if you still stick to it, I will revert it to uevents processing thread, and call it in uevent_dispatch() before calling merge_uevq(), actually, I'm going to do that. Regards Tang Junhui 发件人: "Benjamin Marzinski" 收件人: tang.jun...@zte.com.cn, 抄送: tang.wenj...@zte.com.cn, zhang.ka...@zte.com.cn, dm-devel@redhat.com, bart.vanass...@sandisk.com, mwi...@suse.com 日期: 2017/01/04 06:38 主题: Re: [dm-devel] [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations 发件人: dm-devel-boun...@redhat.com On Tue, Dec 27, 2016 at 04:03:25PM +0800, tang.jun...@zte.com.cn wrote: > From: tang.junhui > > 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 1 seconds for more uevents except that too > much uevents (2048) have already been received or too much time eclipse > (60 seconds). The issue I have with doing this in uevent_listen is that we seperated receiving the uevents from servicing the uevents specificially to make sure what we received them as fast as possible. The udev monitor code is all based on a NETLINK socket. If our socket's receive buffer fills up, there is no flow control. Events just start getting dropped, which does cut down on processing time, but not in a way we would like. This issue applies to a lesser extent to you previous two patches. I don't really see the benefit of making sure that we drop the uevents as soon as possible. As long as we don't process them, that should be good enough, right? Now, maybe you put a lot of thought into your timeouts, and feel confident that we will start processing well before the receive buffer fills up. But if so, some comments on that would be reassuring, because from the commit message, they seem fairly arbitrary to me. -Ben > > Change-Id: I763d491540e8114a81d12d603281540a81502742 > Signed-off-by: tang.junhui > --- > libmultipath/uevent.c | 35 +-- > 1 file changed, 33 insertions(+), 2 deletions(-) > > diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c > index cc10d65..b0b05e9 100644 > --- a/libmultipath/uevent.c > +++ b/libmultipath/uevent.c > @@ -39,6 +39,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -490,11 +491,37 @@ 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; > + > + 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; > + /* max wait 60s */ > + if (eclipse_ms > 60*1000) { > + condlog(1, "burst continued =%lu ms, stoped", eclipse_ms); > + return false; > + } > + > + speed = (events * 1000) / eclipse_ms; > + if (speed > 10) > + 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); > @@ -548,6 +575,7 @@ int uevent_listen(struct udev *udev) >} > >events = 0; > + gettimeofday(&start_time, NULL); >while (1) { >struct uevent *uev; >struct udev_device *dev; > @@ -562,7 +590,8 @@ int uevent_listen(struct udev *udev) >errno = 0; >fdcount = poll(&ev_poll, 1, poll_timeout); >if (fdcount && ev_poll.revents & POLLIN) { > - timeout = 0; > +
Re: [dm-devel] [PATCH 01/12] libmultipath: add wwid for "struct uevent" to record wwid of uevent
Hello Ben, > Right now, multipath users are allowed configure devices to set the wwid > based on any udev environment variable (or even use a callout, although > this is deprecated). With this patch, that breaks. Does WWID obtained by different methods change? If it changes, we would better to modify code to keep it no change. > If the udev sets > ID_SERIAL for a device, that is its wwid, right? Yes > Do you know if rbd > devices have ID_SERIAL set? WWID has different label in uevents for different devices, I only test for SCSI devices. Now we do not support rbd divice for uevents merging, these device process as old way, it has no harm in logic. If we need to merge rbd uevents for these devices, we can add code to get wwid from uevents and it can be supported easily. Regards Tang Junhui 发件人: "Benjamin Marzinski" 收件人: tang.jun...@zte.com.cn, 抄送: christophe.varo...@opensvc.com, h...@suse.de, mwi...@suse.com, bart.vanass...@sandisk.com, dm-devel@redhat.com, zhang.ka...@zte.com.cn, tang.wenj...@zte.com.cn 日期: 2017/01/04 06:03 主题: Re: [PATCH 01/12] libmultipath: add wwid for "struct uevent" to record wwid of uevent On Tue, Dec 27, 2016 at 04:03:18PM +0800, tang.jun...@zte.com.cn wrote: > From: tang.junhui > > Add "char *wwid" to point WWID of uevent. This member identifies > the LUN ID which the path belongs to, and it is used for merging > uevents. WWID possibly did not exist in uevent yet, so ->wwid > would be NULL, those uevents would not be merged, but be proccessed > as old way. Right now, multipath users are allowed configure devices to set the wwid based on any udev environment variable (or even use a callout, although this is deprecated). With this patch, that breaks. If the udev sets ID_SERIAL for a device, that is its wwid, right? Do you know if rbd devices have ID_SERIAL set? If so, this change will break them. Even if this change doesn't break any devices in their default configurations, we would need to disallow changing how the wwid is set for this patch to be safe. -Ben > > Change-Id: Ie6b076363b3735dc7de10184b27fa799b499af0e > Signed-off-by: tang.junhui > --- > libmultipath/uevent.c | 2 ++ > libmultipath/uevent.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c > index 7edcce1..ef1bafe 100644 > --- a/libmultipath/uevent.c > +++ b/libmultipath/uevent.c > @@ -424,6 +424,8 @@ 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; > + if (strcmp(name, "ID_SERIAL") == 0) > + uev->wwid = uev->envp[i] + 10; >i++; >if (i == HOTPLUG_NUM_ENVP - 1) >break; > diff --git a/libmultipath/uevent.h b/libmultipath/uevent.h > index 9d22dcd..7bfccef 100644 > --- a/libmultipath/uevent.h > +++ b/libmultipath/uevent.h > @@ -22,6 +22,7 @@ struct uevent { >char *devpath; >char *action; >char *kernel; > + char *wwid; >unsigned long seqnum; >char *envp[HOTPLUG_NUM_ENVP]; > }; > -- > 2.8.1.windows.1 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 09/12] multipathd: merge uevents before proccessing
Hello Ben, Yes, a *_safe list traversal method can meet the needs, I will modify it and simplify the codes. Thanks, Tang Junhui 发件人: "Benjamin Marzinski" 收件人: tang.jun...@zte.com.cn, 抄送: tang.wenj...@zte.com.cn, zhang.ka...@zte.com.cn, dm-devel@redhat.com, bart.vanass...@sandisk.com, mwi...@suse.com 日期: 2017/01/04 08:39 主题: Re: [dm-devel] [PATCH 09/12] multipathd: merge uevents before proccessing 发件人: dm-devel-boun...@redhat.com On Tue, Dec 27, 2016 at 04:03:26PM +0800, tang.jun...@zte.com.cn wrote: > From: tang.junhui > > 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 wwid is same. This is just a nit, and I might be missing something subtle here, but it seems like instead of adding list_for_some_entry_reverse, and then breaking the abstraction to manually get previous entries, you could have just added list_for_some_entry_reverse_safe in your earlier patch, and hid the work of traversing a list while removing elements behind the well understood abstraction of a *_safe list traversal method. -Ben > > Change-Id: I05ee057391c092aa0c5f989b7a4f9cb550bb4d98 > Signed-off-by: tang.junhui > --- > libmultipath/uevent.c | 125 +- > 1 file changed, 114 insertions(+), 11 deletions(-) > > diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c > index b0b05e9..114068c 100644 > --- a/libmultipath/uevent.c > +++ b/libmultipath/uevent.c > @@ -85,6 +85,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(char *devpath, char *kernel) > { > @@ -125,6 +139,103 @@ uevent_can_discard(char *devpath, char *kernel) >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 wwid, > + * so it is sensible to stop merging > + */ > + if (!earlier->wwid || !later->wwid) > + 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 wwid and different action > + * it would be better to stop merging. > + */ > + if (!strcmp(earlier->wwid, later->wwid) && > + 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 wwids exsit and are same > + * and actions are same, > + * and actions are addition or deletion > + */ > + if (earlier->wwid && later->wwid && > + !strcmp(earlier->wwid, later->wwid) && > + !strcmp(earlier->action, later->action) && > + strncmp(earlier->action, "change", 6) && > + strncmp(earlier->kernel, "dm-", 3)) { > +
Re: [dm-devel] [PATCH 10/12] libmultipath: filter uevents before proccessing
Hello Ben, I add wwid judgment for safe. I think twice, and as you say, it is safe enough without this judgment, I will delete these wwid judgment. Tanks Tang Junhui 发件人: "Benjamin Marzinski" 收件人: tang.jun...@zte.com.cn, 抄送: tang.wenj...@zte.com.cn, zhang.ka...@zte.com.cn, dm-devel@redhat.com, bart.vanass...@sandisk.com, mwi...@suse.com 日期: 2017/01/04 09:28 主题: Re: [dm-devel] [PATCH 10/12] libmultipath: filter uevents before proccessing 发件人: dm-devel-boun...@redhat.com On Tue, Dec 27, 2016 at 04:03:27PM +0800, tang.jun...@zte.com.cn wrote: > From: tang.junhui > > Before merging uevents, these uevents are going to be filtered: > Change or addition uevent of a removed path (it indicate by an > deletion uevent occurred later). > I think it's safe to remove add and change uevents if they are followed by a remove event, regardless of whether or not they have a wwid, as long as the kernel name is the same. We only get the remove event when the device is gone. Processing the add and change events will never get us anything in these cases, because there is no device to act on. -Ben > Change-Id: If00c2c2e23ea466c1d4643c541ed2d8f9a0c8dea > Signed-off-by: tang.junhui > --- > libmultipath/uevent.c | 55 +++ > 1 file changed, 55 insertions(+) > > diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c > index 114068c..424f272 100644 > --- a/libmultipath/uevent.c > +++ b/libmultipath/uevent.c > @@ -140,6 +140,28 @@ uevent_can_discard(char *devpath, char *kernel) > } > > bool > +uevent_can_filter(struct uevent *earlier, struct uevent *later) > +{ > + > + /* > + * filter earlier uvents if path has removed later, eg: > + * "add path3 |chang path3 |add path2 |remove path3" > + * can filter as: > + * "add path2 |remove path3" > + * uevent "add path3" and "chang path3" are filtered out > + */ > + if (earlier->wwid && later->wwid && > + !strcmp(earlier->wwid, later->wwid) && > + strncmp(later->kernel, "dm-", 3) && > + !strcmp(later->action, "remove") && > + !strcmp(earlier->kernel, later->kernel)) { > + return true; > + } > + > + return false; > +} > + > +bool > merge_need_stop(struct uevent *earlier, struct uevent *later) > { >/* > @@ -196,6 +218,38 @@ uevent_can_merge(struct uevent *earlier, struct uevent *later) > } > > void > +uevent_filter(struct uevent *later, struct list_head *tmpq) > +{ > + struct uevent *earlier, *temp; > + /* > + * compare the uevent with earlier uevents > + */ > + list_for_some_entry_reverse(earlier, &later->node, tmpq, node) { > +next_earlier_node: > + /* > + * filter unnessary earlier uevents by the later uevent > + */ > + if (uevent_can_filter(earlier, later)) { > + condlog(2, "uevent: %s-%s-%s has removed by uevent: %s-%s-%s, filtered", > + earlier->action, earlier->kernel, earlier->wwid, > + later->action, later->kernel, later->wwid); > + > + temp = earlier; > + earlier = list_entry(earlier->node.prev, typeof(struct uevent), node); > + list_del_init(&temp->node); > + if (temp->udev) > + udev_device_unref(temp->udev); > + FREE(temp); > + > + if (earlier == list_entry(tmpq, typeof(struct uevent), node)) > + break; > + else > + goto next_earlier_node; > + } > + } > +} > + > +void > uevent_merge(struct uevent *later, struct list_head *tmpq) > { >struct uevent *earlier, *temp; > @@ -232,6 +286,7 @@ merge_uevq(struct list_head *tmpq) >struct uevent *later; > >list_for_each_entry_reverse(later, tmpq, node) { > + uevent_filter(later, tmpq); >uevent_merge(later, tmpq); >} > } > -- > 2.8.1.windows.1 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 11/12] multipathd: proccess merged uevents
Hello Ben Good idea, I will modify the patch to filter these change uevents. Thanks Tang Junhui 发件人: "Benjamin Marzinski" 收件人: tang.jun...@zte.com.cn, 抄送: tang.wenj...@zte.com.cn, zhang.ka...@zte.com.cn, dm-devel@redhat.com, bart.vanass...@sandisk.com, mwi...@suse.com 日期: 2017/01/04 09:10 主题: Re: [dm-devel] [PATCH 11/12] multipathd: proccess merged uevents 发件人: dm-devel-boun...@redhat.com On Tue, Dec 27, 2016 at 04:03:28PM +0800, tang.jun...@zte.com.cn wrote: > From: "tang.junhui" > > 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. Wait, doesn't this mean that you would change "add path1 | change path1 | add path2" to "change path1 | add path1, path2" It doesn't make sense to process a change event before an add event. Looking at uev_update_path, the three things it currently does all can be safely ignored if you don't process the add event until after you receive the change event. They are: disable path on changed wwid: the multipath device hasn't been created yet, so it will simply be created with the new wwid. That's fine. attempt to get the pathinfo again if you failed when adding it: Either you don't have the necessary udev information in the add event, in which case that uevent won't get merged in the first place and they will remain in order, or you do have the information, and there's no point in processing the change event in that case. change the Read-Only state of the device: since both events (the change and the add event) have already happened, when you look at the device for the add event, you will already get the current read-only state. So, as far as I can tell, if you have change events in your queue as well as an add event for the same device, you might as well just filter out the change events, since processing it immediately after the add event will never actually change anything, and processing it beforehand makes no sense. -Ben > > Change-Id: Ifb4de0ca36180a8af16729c2f70bc250858877bd > Signed-off-by: tang.junhui > --- > multipathd/main.c | 28 +++- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 15a6175..0b18f6c 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -1092,6 +1092,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; > > @@ -1123,20 +1124,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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] multipathd: fix SIGUSR2 handling
From: "tang.junhui" SIGUSR2 is not blocked by default, it would cause bellow conflict in removing of the last path: - uevent processing thread | waiter thread -|--- uev_remove_path()| waiteventloop() lock(&vecs->lock) | ev_remove_path() | dm_queue_if_no_path()|> dm_task_run() flush_map() | | remove_map_and_stop_waiter() | -> pthread_cleanup_push(cleanup_lock, &waiter->vecs->lock); _remove_map()| lock(&waiter->vecs->lock)//wait for the locker stop_waiter_thread() | pthread_cancel(thread) | pthread_kill(thread,SIGUSR2)-|--> sigusr2 (int sig) | condlog() | fprintf() //it has test cancel point | cleanup_lock() //thread cancel, and pop, which unlock the | locker of uevent processing thread -- Since SIGUSR2 is only needed when waiter thread running in dm_task_run(), and it has already been unblocked before dm_task_run(), so this patch block SIGUSR2 for other times. Change-Id: I8c46292dc954415764ebbe054498b4f7ea53c1c6 Signed-off-by: tang.junhui --- multipathd/main.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/multipathd/main.c b/multipathd/main.c index adc3258..fe69abd 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2181,6 +2181,12 @@ sigusr2 (int sig) static void signal_init(void) { + sigset_t set; + + sigemptyset(&set); + sigaddset(&set, SIGUSR2); + pthread_sigmask(SIG_SETMASK, &set, NULL); + signal_set(SIGHUP, sighup); signal_set(SIGUSR1, sigusr1); signal_set(SIGUSR2, sigusr2); -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations
From: tang.junhui 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 --- libmultipath/uevent.c | 42 +- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c index cc10d65..0345e6e 100644 --- a/libmultipath/uevent.c +++ b/libmultipath/uevent.c @@ -39,6 +39,7 @@ #include #include #include +#include #include #include @@ -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); @@ -490,11 +495,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); @@ -548,6 +585,7 @@ int uevent_listen(struct udev *udev) } events = 0; + gettimeofday(&start_time, NULL); while (1) { struct uevent *uev; struct udev_device *dev; @@ -562,7 +600,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"); @@ -600,6 +639,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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations
Hi Martin, Thank you for your respond, I will modify it and resend it later. Regards, Tang 发件人: Martin Wilck 收件人: tang.jun...@zte.com.cn, christophe.varo...@opensvc.com, h...@suse.de, bmarz...@redhat.com, bart.vanass...@sandisk.com, 抄送: dm-devel@redhat.com, zhang.ka...@zte.com.cn, tang.wenj...@zte.com.cn 日期: 2016/12/29 04:26 主题: Re: [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations On Tue, 2016-12-27 at 16:03 +0800, tang.jun...@zte.com.cn wrote: > From: tang.junhui > > 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 1 seconds for more uevents except that > too > much uevents (2048) have already been received or too much time > eclipse > (60 seconds). Hi Tang, thanks for this impressive patch set. While I haven't had time to read through the more complex parts yet, one small nitpick on this patch: please use named constants rather than explicit numbers in the code. Regards, Martin > > Change-Id: I763d491540e8114a81d12d603281540a81502742 > Signed-off-by: tang.junhui > --- > libmultipath/uevent.c | 35 +-- > 1 file changed, 33 insertions(+), 2 deletions(-) > > diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c > index cc10d65..b0b05e9 100644 > --- a/libmultipath/uevent.c > +++ b/libmultipath/uevent.c > @@ -39,6 +39,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -490,11 +491,37 @@ 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; > + > + 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; > + /* max wait 60s */ > + if (eclipse_ms > 60*1000) { > + condlog(1, "burst continued =%lu ms, stoped", > eclipse_ms); > + return false; > + } > + > + speed = (events * 1000) / eclipse_ms; > + if (speed > 10) > + 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); > @@ -548,6 +575,7 @@ int uevent_listen(struct udev *udev) >} > >events = 0; > + gettimeofday(&start_time, NULL); >while (1) { >struct uevent *uev; >struct udev_device *dev; > @@ -562,7 +590,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"); > @@ -578,7 +607,8 @@ int uevent_listen(struct udev *udev) >} >list_add_tail(&uev->node, &uevlisten_tmp); >events++; > - continue; > + if(events < 2048) > + continue; >} >if (fdcount < 0) { >if (errno == EINTR) > @@ -600,6 +630,7 @@ int uevent_listen(struct udev *udev) > pthread_mutex_unlock(uevq_lockp); >events = 0; >} > + gettimeofday(&start_time, NULL); >timeout = 30; >} >need_failback = 0; -- Dr. Martin Wilck , Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Gr
[dm-devel] [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations
From: tang.junhui 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 1 seconds for more uevents except that too much uevents (2048) have already been received or too much time eclipse (60 seconds). Change-Id: I763d491540e8114a81d12d603281540a81502742 Signed-off-by: tang.junhui --- libmultipath/uevent.c | 35 +-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c index cc10d65..b0b05e9 100644 --- a/libmultipath/uevent.c +++ b/libmultipath/uevent.c @@ -39,6 +39,7 @@ #include #include #include +#include #include #include @@ -490,11 +491,37 @@ 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; + + 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; + /* max wait 60s */ + if (eclipse_ms > 60*1000) { + condlog(1, "burst continued =%lu ms, stoped", eclipse_ms); + return false; + } + + speed = (events * 1000) / eclipse_ms; + if (speed > 10) + 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); @@ -548,6 +575,7 @@ int uevent_listen(struct udev *udev) } events = 0; + gettimeofday(&start_time, NULL); while (1) { struct uevent *uev; struct udev_device *dev; @@ -562,7 +590,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"); @@ -578,7 +607,8 @@ int uevent_listen(struct udev *udev) } list_add_tail(&uev->node, &uevlisten_tmp); events++; - continue; + if(events < 2048) + continue; } if (fdcount < 0) { if (errno == EINTR) @@ -600,6 +630,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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 04/12] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path()
From: tang.junhui 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 --- multipathd/cli_handlers.c | 3 ++- multipathd/main.c | 25 - multipathd/main.h | 2 +- 3 files changed, 19 insertions(+), 11 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..34b1b64 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; @@ -641,7 +641,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs) DI_ALL | DI_BLACKLIST); put_multipath_config(conf); if (r == PATHINFO_OK) - ret = ev_add_path(pp, vecs); + ret = ev_add_path(pp, vecs, need_do_map); else if (r == PATHINFO_SKIPPED) { condlog(3, "%s: remove blacklisted path", uev->kernel); @@ -681,7 +681,7 @@ 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); + ret = ev_add_path(pp, vecs, need_do_map); } else { condlog(0, "%s: failed to store path info, " "dropping event", @@ -699,7 +699,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 +767,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 +1002,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 +1157,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 +1577,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 +1693,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 +1716,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks
[dm-devel] [PATCH 00/12] multipath-tools: improve processing efficiency for addition and deletion of multipath devices
From: tang.junhui Hello Christophe, Ben, Hannes, Martin, Bart, and other guys: The twelve patches in this series are used to improve processing efficiency for addition and deletion of multipath devices. These 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 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 wwid for "struct uevent" to record wwid of uevent --[patch]libmultipath: add merge_node for "struct uevent" to record nodes of merged uevents --[patch]libmultipath: add two list iteration macros There are also some independent optimization pathes for further improvement: --[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() --[patch]libmultipath: use existing wwid when wwid has already been existed in uevent 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 11/12] multipathd: proccess merged uevents
From: "tang.junhui" 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 --- multipathd/main.c | 28 +++- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/multipathd/main.c b/multipathd/main.c index 15a6175..0b18f6c 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -1092,6 +1092,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; @@ -1123,20 +1124,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
[dm-devel] [PATCH 09/12] multipathd: merge uevents before proccessing
From: tang.junhui 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 wwid is same. Change-Id: I05ee057391c092aa0c5f989b7a4f9cb550bb4d98 Signed-off-by: tang.junhui --- libmultipath/uevent.c | 125 +- 1 file changed, 114 insertions(+), 11 deletions(-) diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c index b0b05e9..114068c 100644 --- a/libmultipath/uevent.c +++ b/libmultipath/uevent.c @@ -85,6 +85,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(char *devpath, char *kernel) { @@ -125,6 +139,103 @@ uevent_can_discard(char *devpath, char *kernel) 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 wwid, +* so it is sensible to stop merging +*/ + if (!earlier->wwid || !later->wwid) + 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 wwid and different action +* it would be better to stop merging. +*/ + if (!strcmp(earlier->wwid, later->wwid) && + 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 wwids exsit and are same +* and actions are same, +* and actions are addition or deletion +*/ + if (earlier->wwid && later->wwid && + !strcmp(earlier->wwid, later->wwid) && + !strcmp(earlier->action, later->action) && + strncmp(earlier->action, "change", 6) && + strncmp(earlier->kernel, "dm-", 3)) { + return true; + } + + return false; +} + +void +uevent_merge(struct uevent *later, struct list_head *tmpq) +{ + struct uevent *earlier, *temp; + /* +* compare the uevent with earlier uevents +*/ + list_for_some_entry_reverse(earlier, &later->node, tmpq, node) { +next_earlier_node: + if (merge_need_stop(earlier, later)) + break; + /* +* try to 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->wwid, + later->action, later->kernel, later->wwid); + temp = earlier; + + earlier = list_entry(earlier->node.prev, typeof(struct uevent), node); + list_move(&temp->node, &later->merge_node); + + if (earlier == list_entry(tmpq, typeof(struct uevent), node)) + break; + else + goto next_earlier_node; + } + } +} + +void +merge_uevq(struct list_head *tmpq) +{ + struct uevent *later; + + list_for_each_entry_reverse(later, tmpq, node) { + uevent_merge(later, tmpq); + } +} + void service_uevq(struct list_head *tmpq) { @@ -136,6 +247,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); @@ -150,17 +263,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
[dm-devel] [PATCH 02/12] libmultipath: add merge_node for "struct uevent" to record nodes of merged uevents
From: tang.junhui 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 --- 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 ef1bafe..181b3b8 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 7bfccef..61a4207 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 12/12] libmultipath: use existing wwid when wwid has already been existed in uevent
From: tang.junhui Use existing wwid when wwid has already been existed in uevent, it avoids to get wwid again in pathinfo(), and reduces the count of calling getuid(), which would promote processing efficiency. Change-Id: Ia0c7273d33a220e5b415ec42d6b72660018cf4d9 Signed-off-by: tang.junhui --- libmultipath/discovery.c | 9 ++--- libmultipath/discovery.h | 4 +++- multipathd/main.c| 4 ++-- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index d1aec31..4985e03 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -32,7 +32,7 @@ #include "defaults.h" int -alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, +alloc_path_with_pathinfo (struct config *conf, struct uevent *uev, int flag, struct path **pp_ptr) { int err = PATHINFO_FAILED; @@ -42,7 +42,7 @@ alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, if (pp_ptr) *pp_ptr = NULL; - devname = udev_device_get_sysname(udevice); + devname = udev_device_get_sysname(uev->udev); if (!devname) return PATHINFO_FAILED; @@ -51,10 +51,13 @@ alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, if (!pp) return PATHINFO_FAILED; + if(uev->wwid) + strncpy(pp->wwid, uev->wwid, sizeof(pp->wwid)); + if (safe_sprintf(pp->dev, "%s", devname)) { condlog(0, "pp->dev too small"); } else { - pp->udev = udev_device_ref(udevice); + pp->udev = udev_device_ref(uev->udev); err = pathinfo(pp, conf, flag | DI_BLACKLIST); } diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h index 3039268..df7de0f 100644 --- a/libmultipath/discovery.h +++ b/libmultipath/discovery.h @@ -1,6 +1,8 @@ #ifndef DISCOVERY_H #define DISCOVERY_H +#include "uevent.h" + #define SYSFS_PATH_SIZE 255 #define INQUIRY_CMDLEN 6 #define INQUIRY_CMD 0x12 @@ -36,7 +38,7 @@ int do_tur (char *); int path_offline (struct path *); int get_state (struct path * pp, struct config * conf, int daemon); int pathinfo (struct path * pp, struct config * conf, int mask); -int alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, +int alloc_path_with_pathinfo (struct config *conf, struct uevent *uev, int flag, struct path **pp_ptr); int store_pathinfo (vector pathvec, struct config *conf, struct udev_device *udevice, int flag, diff --git a/multipathd/main.c b/multipathd/main.c index 0b18f6c..7a1cd09 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -664,7 +664,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map) * get path vital state */ conf = get_multipath_config(); - ret = alloc_path_with_pathinfo(conf, uev->udev, + ret = alloc_path_with_pathinfo(conf, uev, DI_ALL, &pp); put_multipath_config(conf); if (!pp) { @@ -1026,7 +1026,7 @@ out: int flag = DI_SYSFS | DI_WWID; conf = get_multipath_config(); - retval = alloc_path_with_pathinfo(conf, uev->udev, flag, NULL); + retval = alloc_path_with_pathinfo(conf, uev, flag, NULL); put_multipath_config(conf); if (retval == PATHINFO_SKIPPED) { -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 07/12] multipathd: move calling filter_devnode() from uev_trigger() to uevent_can_discard()
From: tang.junhui 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 --- libmultipath/uevent.c | 25 +++-- multipathd/main.c | 9 - 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c index ac49cac..cc10d65 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); @@ -82,10 +85,11 @@ struct uevent * alloc_uevent (void) } bool -uevent_can_discard(char *devpath) +uevent_can_discard(char *devpath, char *kernel) { char *tmp; char a[11], b[11]; + struct config * conf; /* * keep only block devices, discard partitions @@ -100,6 +104,23 @@ uevent_can_discard(char *devpath) condlog(4, "discard event on %s", devpath); return true; } + + /* +* do not filter dm devices by devnode +*/ + if (!strncmp(kernel, "dm-", 3)) + return false; + /* +* filter paths devices by devnode +*/ + conf = get_multipath_config(); + if (filter_devnode(conf->blist_devnode, conf->elist_devnode, + kernel) > 0) { + put_multipath_config(conf); + return true; + } + put_multipath_config(conf); + return false; } @@ -550,7 +571,7 @@ int uevent_listen(struct udev *udev) uev = uevent_from_udev_device(dev); if (!uev) continue; - if (uevent_can_discard(uev->devpath)) { + if (uevent_can_discard(uev->devpath, uev->kernel)) { udev_device_unref(uev->udev); FREE(uev); continue; diff --git a/multipathd/main.c b/multipathd/main.c index 51f7b60..15a6175 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -1092,7 +1092,6 @@ uev_trigger (struct uevent * uev, void * trigger_data) { int r = 0; struct vectors * vecs; - struct config *conf; vecs = (struct vectors *)trigger_data; @@ -1126,14 +1125,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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 06/12] multipathd: move uev_discard() to uevent.c and change its name to uevent_can_discard()
From: tang.junhui Move uev_discard() form uevent processing thread to uevent listening thread to discard unnecessary uevents as soon as possible. Change-Id: Iaac159ffe3930e53c3325d1069c3ed497e440c0c Signed-off-by: tang.wenjun --- libmultipath/uevent.c | 28 multipathd/main.c | 25 - 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c index 181b3b8..ac49cac 100644 --- a/libmultipath/uevent.c +++ b/libmultipath/uevent.c @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -80,6 +81,28 @@ struct uevent * alloc_uevent (void) return uev; } +bool +uevent_can_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 true; + } + if (sscanf(tmp, "/block/%10s", a) != 1 || + sscanf(tmp, "/block/%10[^/]/%10s", a, b) == 2) { + condlog(4, "discard event on %s", devpath); + return true; + } + return false; +} + void service_uevq(struct list_head *tmpq) { @@ -527,6 +550,11 @@ int uevent_listen(struct udev *udev) uev = uevent_from_udev_device(dev); if (!uev) continue; + if (uevent_can_discard(uev->devpath)) { +udev_device_unref(uev->udev); +FREE(uev); +continue; + } list_add_tail(&uev->node, &uevlisten_tmp); events++; continue; diff --git a/multipathd/main.c b/multipathd/main.c index 68c8e17..51f7b60 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -1087,28 +1087,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) { @@ -1118,9 +1096,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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 05/12] multipathd: add need_do_map to indicate whether need calling domap() in ev_remove_path()
From: tang.junhui 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 --- multipathd/cli_handlers.c | 2 +- multipathd/main.c | 11 +++ multipathd/main.h | 2 +- 3 files changed, 9 insertions(+), 6 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 34b1b64..68c8e17 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -840,7 +840,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; @@ -851,7 +851,7 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs) pthread_testcancel(); pp = find_path_by_dev(vecs->pathvec, uev->kernel); if (pp) - ret = ev_remove_path(pp, vecs); + 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 */ @@ -862,7 +862,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; @@ -884,6 +884,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 */ @@ -1161,7 +1164,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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 10/12] libmultipath: filter uevents before proccessing
From: tang.junhui Before merging uevents, these uevents are going to be filtered: Change or addition uevent of a removed path (it indicate by an deletion uevent occurred later). Change-Id: If00c2c2e23ea466c1d4643c541ed2d8f9a0c8dea Signed-off-by: tang.junhui --- libmultipath/uevent.c | 55 +++ 1 file changed, 55 insertions(+) diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c index 114068c..424f272 100644 --- a/libmultipath/uevent.c +++ b/libmultipath/uevent.c @@ -140,6 +140,28 @@ uevent_can_discard(char *devpath, char *kernel) } bool +uevent_can_filter(struct uevent *earlier, struct uevent *later) +{ + + /* +* filter earlier uvents if path has removed later, eg: +* "add path3 |chang path3 |add path2 |remove path3" +* can filter as: +* "add path2 |remove path3" +* uevent "add path3" and "chang path3" are filtered out +*/ + if (earlier->wwid && later->wwid && + !strcmp(earlier->wwid, later->wwid) && + strncmp(later->kernel, "dm-", 3) && + !strcmp(later->action, "remove") && + !strcmp(earlier->kernel, later->kernel)) { + return true; + } + + return false; +} + +bool merge_need_stop(struct uevent *earlier, struct uevent *later) { /* @@ -196,6 +218,38 @@ uevent_can_merge(struct uevent *earlier, struct uevent *later) } void +uevent_filter(struct uevent *later, struct list_head *tmpq) +{ + struct uevent *earlier, *temp; + /* +* compare the uevent with earlier uevents +*/ + list_for_some_entry_reverse(earlier, &later->node, tmpq, node) { +next_earlier_node: + /* +* filter unnessary earlier uevents by the later uevent +*/ + if (uevent_can_filter(earlier, later)) { + condlog(2, "uevent: %s-%s-%s has removed by uevent: %s-%s-%s, filtered", + earlier->action, earlier->kernel, earlier->wwid, + later->action, later->kernel, later->wwid); + + temp = earlier; + earlier = list_entry(earlier->node.prev, typeof(struct uevent), node); + list_del_init(&temp->node); + if (temp->udev) + udev_device_unref(temp->udev); + FREE(temp); + + if (earlier == list_entry(tmpq, typeof(struct uevent), node)) + break; + else + goto next_earlier_node; + } + } +} + +void uevent_merge(struct uevent *later, struct list_head *tmpq) { struct uevent *earlier, *temp; @@ -232,6 +286,7 @@ merge_uevq(struct list_head *tmpq) struct uevent *later; list_for_each_entry_reverse(later, tmpq, node) { + uevent_filter(later, tmpq); uevent_merge(later, tmpq); } } -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 03/12] libmultipath: add two list iteration macros
From: tang.junhui Add two list iteration macros, they 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 --- libmultipath/list.h | 24 1 file changed, 24 insertions(+) diff --git a/libmultipath/list.h b/libmultipath/list.h index ceaa381..e728ff0 100644 --- a/libmultipath/list.h +++ b/libmultipath/list.h @@ -294,6 +294,18 @@ static inline void list_splice_tail_init(struct list_head *list, pos = list_entry(pos->member.next, typeof(*pos), member)) /** + * list_for_some_entry - iterate list of given interval + * @pos: the type * to use as a loop counter. + * @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(pos, from, to, member) \ + for (pos = list_entry((from)->next, typeof(*pos), member); \ +&pos->member != (to); \ +pos = list_entry(pos->member.next, typeof(*pos), member)) + +/** * list_for_each_entry_reverse - iterate backwards over list of given type. * @pos: the type * to use as a loop counter. * @head: the head for your list. @@ -305,6 +317,18 @@ static inline void list_splice_tail_init(struct list_head *list, pos = list_entry(pos->member.prev, typeof(*pos), member)) /** + * list_for_some_entry_reverse - iterate backwards list of given interval + * @pos: the type * to use as a loop counter. + * @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(pos, from, to, member) \ + for (pos = list_entry((from)->prev, typeof(*pos), member); \ +&pos->member != (to); \ +pos = list_entry(pos->member.prev, typeof(*pos), member)) + +/** * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry * @pos: the type * to use as a loop counter. * @n: another type * to use as temporary storage -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 01/12] libmultipath: add wwid for "struct uevent" to record wwid of uevent
From: tang.junhui Add "char *wwid" to point WWID of uevent. This member identifies the LUN ID which the path belongs to, and it is used for merging uevents. WWID possibly did not exist in uevent yet, so ->wwid would be NULL, those uevents would not be merged, but be proccessed as old way. Change-Id: Ie6b076363b3735dc7de10184b27fa799b499af0e Signed-off-by: tang.junhui --- libmultipath/uevent.c | 2 ++ libmultipath/uevent.h | 1 + 2 files changed, 3 insertions(+) diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c index 7edcce1..ef1bafe 100644 --- a/libmultipath/uevent.c +++ b/libmultipath/uevent.c @@ -424,6 +424,8 @@ 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; + if (strcmp(name, "ID_SERIAL") == 0) + uev->wwid = uev->envp[i] + 10; i++; if (i == HOTPLUG_NUM_ENVP - 1) break; diff --git a/libmultipath/uevent.h b/libmultipath/uevent.h index 9d22dcd..7bfccef 100644 --- a/libmultipath/uevent.h +++ b/libmultipath/uevent.h @@ -22,6 +22,7 @@ struct uevent { char *devpath; char *action; char *kernel; + char *wwid; unsigned long seqnum; char *envp[HOTPLUG_NUM_ENVP]; }; -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices
Hello Ben, Hannes I'm sorry for the late reply. > You can't just get the wwid with no work (look at all work uev_add_path > does, specifically alloc_path_with_pathinfo). Now you could reorder > this, but there isn't much point, since it is doing useful things, like > checking if this is a spurious uevent, and necessary things, like > figuring out the device type and using that that the configuration to > figure out HOW to get the wwid. IMO, WWID can be geted from uevent, since it has a ID_SERIAL filed in uevent body. > It seems like what you want to do is to > call uev_add_path multiple times, but defer most of the work that > ev_add_path does (creating or updating the multipath device), until > you've processed all that paths. Not exactly, the input parameter "struct uevent *uev" is a batch of merged uevents, so uev_add_path() is called once to process the merged uevent. > split off uev_add_path() and > ev_add_path(). > Then uev_add_path could generate a list of fully-formed paths which > ev_add_path() would process. > IE generalize coalesce_paths() to work on a passed-in list rather than > the normal vecs->pathvec. Hannes, I think my thoughts are close to your idea now, In uev_add_path(), we get all information of the merged paths, and then call ev_add_path() to create or update the multipath device. Maybe the different between us is "processing all the same type uevents(add, etc.) in ev_add_path()" or "processing all the same type uevents(add, etc.) which came from the same LUN in ev_add_path()". I think your idea can also reach the goal, and reduce the DM reload times. So we will try to code as your idea, and in list_merger_uevents(&uevq_tmp) only merger the same type(add, etc.) uevents, add stop mergering when another type uevents are occured. Thanks all, Tang 发件人: Hannes Reinecke 收件人: Benjamin Marzinski , tang.jun...@zte.com.cn, 抄送: dm-devel@redhat.com, zhang.ka...@zte.com.cn, Martin Wilck , Bart Van Assche 日期: 2016/11/28 23:46 主题: Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices 发件人: dm-devel-boun...@redhat.com On 11/28/2016 04:25 PM, Benjamin Marzinski wrote: > On Mon, Nov 28, 2016 at 10:19:15AM +0800, tang.jun...@zte.com.cn wrote: >>Hello Christophe, Ben, Hannes, Martin, Bart, >>I am a member of host-side software development team of ZXUSP storage >>project >>in ZTE Corporation. Facing the market demand, our team decides to write >>code to >>promote multipath efficiency next month. The whole idea is in the mail >>below.We >>hope to participate in and make progress with the open source community, >>so any >>suggestion and comment would be welcome. > > Like I mentioned before, I think this is a good idea in general, but the > devil is in the details here. > >> >>Thanks, >>Tang >> >> -- >> -- >>1.Problem >>In these scenarios, multipath processing efficiency is low: >>1) Many paths exist in each multipath device, >>2) Devices addition or deletion during iSCSI login/logout or FC link >>up/down. > > > >>4.Proposal >>Other than processing uevents one by one, uevents which coming from the >>same LUN devices can be mergered to one, and then uevent processing >>thread only needs to process it once, and it only produces one DM addition >>uevent which could reduce system resource consumption. >> >>The example in Chapter 2 is continued to use to explain the proposal: >>1) Multipath receives block device addition uevents from udev: >>UDEV [89068.806214] add >> /devices/platform/host3/session44/target3:0:0/3:0:0:0/block/sdc (block) >>UDEV [89068.909457] add >> /devices/platform/host3/session44/target3:0:0/3:0:0:2/block/sdg (block) >>UDEV [89068.944956] add >> /devices/platform/host3/session44/target3:0:0/3:0:0:1/block/sde (block) >>UDEV [89068.959215] add >> /devices/platform/host5/session46/target5:0:0/5:0:0:0/block/sdh (block) >>UDEV [89068.978558] add >> /devices/platform/host5/session46/target5:0:0/5:0:0:2/block/sdk (block) >>UDEV [89069.004217] add >> /devices/platform/host5/session46/target5:0:0/5:0:0:1/block/sdj (block) >>UDEV [89069.486361] add >> /devices/platform/host4/session45/target4:0:0/4:0:0:1/block/sdf (block) >>UDEV [89069.495194] add >> /devices/platform/host4/session45/target4:0:0/4:0:0:0/block/sdd (block) >>UDEV [89069.511628] add >> /devices/platform/host4/session45/target4:0:0/4:0:0:2/block/sdi (block) >>UDEV [89069.716292] add >> /devices/platform/host6/session47/target6:0:0/6:0:0:0/block/sdl (block) >>UDEV [8906
Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices
Hello Zdenek I think you suggestion is constructive, some unnecessary operations in udev rules waster too much system resoure(CPU etc.) It is another way to promote multipath efficiency, could you put forward more specific amendment? Regards, Tang 发件人: Zdenek Kabelac 收件人: tang.jun...@zte.com.cn, Benjamin Marzinski , 抄送: zhang.ka...@zte.com.cn, dm-devel@redhat.com, Martin Wilck , Bart Van Assche 日期: 2016/11/29 17:22 主题: Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices 发件人: dm-devel-boun...@redhat.com Dne 23.11.2016 v 02:08 tang.jun...@zte.com.cn napsal(a): > So, now we have at least 4 ways to improve mutliapth efficiency: > 1) Filtering uevents; > 2) Merger uevents; > 3) Using separate locks for mpvec and pathvec; > 4) Get rid of the gazillion waiter threads. > > This is exciting, but how do we achieve this blueprint? > Can we set up some working groups and develop it in parallel > to implement each improvement since most of them are independent? > I'm missing here the analysis of impact on performance. Do we have 'perf' traces and other reports where is the CPU wasted ? While surely solving all issues make sense - there are some tasks which have major impact on speed - while others user will most not notice at all. Regards Zdenek -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 2/2] dm-mpath: Remove useless retain_attached_hw_handler parameter
Hello Mike: Thanks for your respond. > 1) in parse_path() you need to always initialize q the condition of q initialization is if (m->retain_attached_hw_handler)” comparing with previous condition if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags) || m->hw_handler_name) it does not increase the possibility of q initialization. > 2) setting m->hw_handler_name to attached_handler_name needs to still > happen regardless of whether m->hw_handler_name was previously set The patch does not change the logic above, does it? > 3) the "retain_attached_hw_handler" feature should still be allowed on > the command line, no sense to break multipath-tools Yes, actually I am still preparing two other patches now, one patch modifies parse_features() to ignore unsupported parameter and do not return failure. Another patch modifies multipath to remove this parameter also. > In addition, your patch actually breaks the ability to cope with �CEBUSY > from the call to scsi_dh_attach(). Meaning we wouldn't properly fall > back to retaining the attached hw handler. scsi_dh_attach() return �CEBUSY only if that device has attached a different handle, after previous processing, the handle would be same, so it is impossible to return �CEBUSY in scsi_dh_attach(), we do not need to process it. Regards, Tang 发件人: Mike Snitzer 收件人: tang.jun...@zte.com.cn, 抄送: zhang.ka...@zte.com.cn, dm-devel@redhat.com, a...@redhat.com 日期: 2016/11/29 05:52 主题: Re: [dm-devel] [PATCH 2/2] dm-mpath: Remove useless retain_attached_hw_handler parameter 发件人: dm-devel-boun...@redhat.com On Thu, Nov 24 2016 at 2:11am -0500, tang.jun...@zte.com.cn wrote: > From: "tang.junhui" > > Hardware handle would be retained no matter parameter > retain_attached_hw_handler is set or not in the logic > of current code. So remove this useless parameter. Right, that wasn't always the case. Previously (before commit ) dm-mpath would first detach the attached handler. I'm not completely opposed to removing the code that checks MPATHF_RETAIN_ATTACHED_HW_HANDLER in parse_path() but your proposed patch is broken in 2 ways: 1) in parse_path() you need to always initialize q 2) setting m->hw_handler_name to attached_handler_name needs to still happen regardless of whether m->hw_handler_name was previously set 3) the "retain_attached_hw_handler" feature should still be allowed on the command line, no sense to break multipath-tools But honestly, I'm not seeing any reason to not just leave the existing code. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices
Hello Christophe, Ben, Hannes, Martin, Bart, I am a member of host-side software development team of ZXUSP storage project in ZTE Corporation. Facing the market demand, our team decides to write code to promote multipath efficiency next month. The whole idea is in the mail below.We hope to participate in and make progress with the open source community, so any suggestion and comment would be welcome. Thanks, Tang -- -- 1. Problem In these scenarios, multipath processing efficiency is low: 1) Many paths exist in each multipath device, 2) Devices addition or deletion during iSCSI login/logout or FC link up/down. 2. Reasons Multipath process uevents one by one, and each one also produce a new dm addition change or deletion uevent to increased system resource consumption, actually most of these uevents have no sense at all. E.g. login procedure of 4 iSCSI sessions with 3 LUNs: 1) Multipath processes these uevents one by one: UDEV [89068.806214] add /devices/platform/host3/session44/target3:0:0/3:0:0:0/block/sdc (block) UDEV [89068.909457] add /devices/platform/host3/session44/target3:0:0/3:0:0:2/block/sdg (block) UDEV [89068.944956] add /devices/platform/host3/session44/target3:0:0/3:0:0:1/block/sde (block) UDEV [89068.959215] add /devices/platform/host5/session46/target5:0:0/5:0:0:0/block/sdh (block) UDEV [89068.978558] add /devices/platform/host5/session46/target5:0:0/5:0:0:2/block/sdk (block) UDEV [89069.004217] add /devices/platform/host5/session46/target5:0:0/5:0:0:1/block/sdj (block) UDEV [89069.486361] add /devices/platform/host4/session45/target4:0:0/4:0:0:1/block/sdf (block) UDEV [89069.495194] add /devices/platform/host4/session45/target4:0:0/4:0:0:0/block/sdd (block) UDEV [89069.511628] add /devices/platform/host4/session45/target4:0:0/4:0:0:2/block/sdi (block) UDEV [89069.716292] add /devices/platform/host6/session47/target6:0:0/6:0:0:0/block/sdl (block) UDEV [89069.748456] add /devices/platform/host6/session47/target6:0:0/6:0:0:1/block/sdm (block) UDEV [89069.789662] add /devices/platform/host6/session47/target6:0:0/6:0:0:2/block/sdn (block) 2) Multipath also produce DM uvents by step 1), which would be processed by udev and other process who listening kernel: KERNEL[89068.899614] add /devices/virtual/block/dm-2 (block) KERNEL[89068.902477] change /devices/virtual/block/dm-2 (block) KERNEL[89068.955364] add /devices/virtual/block/dm-3 (block) KERNEL[89068.960663] change /devices/virtual/block/dm-3 (block) KERNEL[89069.018903] add /devices/virtual/block/dm-4 (block) KERNEL[89069.042102] change /devices/virtual/block/dm-4 (block) KERNEL[89069.297252] change /devices/virtual/block/dm-2 (block) KERNEL[89069.346718] change /devices/virtual/block/dm-4 (block) KERNEL[89069.388361] change /devices/virtual/block/dm-3 (block) KERNEL[89069.548270] change /devices/virtual/block/dm-4 (block) KERNEL[89069.607306] change /devices/virtual/block/dm-2 (block) KERNEL[89070.118067] change /devices/virtual/block/dm-3 (block) KERNEL[89070.136256] change /devices/virtual/block/dm-2 (block) KERNEL[89070.157222] change /devices/virtual/block/dm-4 (block) KERNEL[89070.216269] change /devices/virtual/block/dm-3 (block) 3) After processing by udev in step 2), udev also transfers these uevents to multipath: UDEV [89068.926428] add /devices/virtual/block/dm-2 (block) UDEV [89069.007511] add /devices/virtual/block/dm-3 (block) UDEV [89069.098054] add /devices/virtual/block/dm-4 (block) UDEV [89069.291184] change /devices/virtual/block/dm-2 (block) UDEV [89069.320632] change /devices/virtual/block/dm-4 (block) UDEV [89069.381434] change /devices/virtual/block/dm-3 (block) UDEV [89069.637666] change /devices/virtual/block/dm-2 (block) UDEV [89069.682303] change /devices/virtual/block/dm-4 (block) UDEV [89069.860877] change /devices/virtual/block/dm-2 (block) UDEV [89069.904735] change /devices/virtual/block/dm-4 (block) UDEV [89070.327167] change /devices/virtual/block/dm-2 (block) UDEV [89070.371114] change /devices/virtual/block/dm-4 (block) UDEV [89070.434592] change /devices/virtual/block/dm-3 (block) UDEV [89070.572072] change /devices/virtual/block/dm-3 (block) UDEV [89070.703181] change /devices/virtual/block/dm-3 (block) 4) Multipath processes uevents above. The efficiency of processing uevents one by one is low, and it produces too many uevents, which further reducing the processing efficiency. The problem is similar in the logout procedure of iSCSI sessions. 3. Negative effect Multipath processes so slowly that it is not satisfied to some applications, For example, Openstack is often timeout in waiting for the c
[dm-devel] [PATCH 1/2] dm-mpath: Enable hw_handler_params to take effect if hw_handler is the same between new and old
From: "tang.junhui" If the hardware handle which a device has already attached is the same with m->hw_handler_name, m->hw_handler_params should not be ignored, but be enabled to take effect. Signed-off-by: tang.junhui --- drivers/md/dm-mpath.c | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index 0caab4b..b1aba63 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -849,19 +849,23 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps retain: attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL); if (attached_handler_name) { + /* clear any hw_handler_params associated with +* m->hw_handler_params if a different handler has +* already been attached. +*/ + if(!m->hw_handler_name || strcmp(attached_handler_name, m->hw_handler_name)) + { + kfree(m->hw_handler_params); + m->hw_handler_params = NULL; + } /* * Reset hw_handler_name to match the attached handler -* and clear any hw_handler_params associated with the -* ignored handler. * * NB. This modifies the table line to show the actual * handler instead of the original table passed in. */ kfree(m->hw_handler_name); m->hw_handler_name = attached_handler_name; - - kfree(m->hw_handler_params); - m->hw_handler_params = NULL; } } -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 2/2] dm-mpath: Remove useless retain_attached_hw_handler parameter
From: "tang.junhui" Hardware handle would be retained no matter parameter retain_attached_hw_handler is set or not in the logic of current code. So remove this useless parameter. Signed-off-by: tang.junhui --- drivers/md/dm-mpath.c | 34 +- 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index b1aba63..fe6a529 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -129,10 +129,9 @@ static void process_queued_bios(struct work_struct *work); #define MPATHF_QUEUE_IO 0 /* Must we queue all I/O? */ #define MPATHF_QUEUE_IF_NO_PATH 1 /* Queue I/O if last path fails? */ #define MPATHF_SAVED_QUEUE_IF_NO_PATH 2/* Saved state during suspension */ -#define MPATHF_RETAIN_ATTACHED_HW_HANDLER 3/* If there's already a hw_handler present, don't change it. */ -#define MPATHF_PG_INIT_DISABLED 4 /* pg_init is not currently allowed */ -#define MPATHF_PG_INIT_REQUIRED 5 /* pg_init needs calling? */ -#define MPATHF_PG_INIT_DELAY_RETRY 6 /* Delay pg_init retry? */ +#define MPATHF_PG_INIT_DISABLED 3 /* pg_init is not currently allowed */ +#define MPATHF_PG_INIT_REQUIRED 4 /* pg_init needs calling? */ +#define MPATHF_PG_INIT_DELAY_RETRY 5 /* Delay pg_init retry? */ /*--- * Allocation routines @@ -240,11 +239,6 @@ static int alloc_multipath_stage2(struct dm_target *ti, struct multipath *m) } else if (m->queue_mode == DM_TYPE_BIO_BASED) { INIT_WORK(&m->process_queued_bios, process_queued_bios); - /* -* bio-based doesn't support any direct scsi_dh management; -* it just discovers if a scsi_dh is attached. -*/ - set_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags); } dm_table_set_type(ti->table, m->queue_mode); @@ -842,11 +836,8 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps goto bad; } - if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags) || m->hw_handler_name) + if (m->hw_handler_name) { q = bdev_get_queue(p->path.dev->bdev); - - if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags)) { -retain: attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL); if (attached_handler_name) { /* clear any hw_handler_params associated with @@ -871,13 +862,6 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps if (m->hw_handler_name) { r = scsi_dh_attach(q, m->hw_handler_name); - if (r == -EBUSY) { - char b[BDEVNAME_SIZE]; - - printk(KERN_INFO "dm-mpath: retaining handler on device %s\n", - bdevname(p->path.dev->bdev, b)); - goto retain; - } if (r < 0) { ti->error = "error attaching hardware handler"; dm_put_device(ti, p->path.dev); @@ -1040,7 +1024,7 @@ static int parse_features(struct dm_arg_set *as, struct multipath *m) const char *arg_name; static struct dm_arg _args[] = { - {0, 8, "invalid number of feature args"}, + {0, 7, "invalid number of feature args"}, {1, 50, "pg_init_retries must be between 1 and 50"}, {0, 6, "pg_init_delay_msecs must be between 0 and 6"}, }; @@ -1061,11 +1045,6 @@ static int parse_features(struct dm_arg_set *as, struct multipath *m) continue; } - if (!strcasecmp(arg_name, "retain_attached_hw_handler")) { - set_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags); - continue; - } - if (!strcasecmp(arg_name, "pg_init_retries") && (argc >= 1)) { r = dm_read_arg(_args + 1, as, &m->pg_init_retries, &ti->error); @@ -1745,7 +1724,6 @@ static void multipath_status(struct dm_target *ti, status_type_t type, DMEMIT("%u ", test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags) + (m->pg_init_retries > 0) * 2 + (m->pg_init_delay_msecs != DM_PG_INIT_DELAY_DEFAULT) * 2 + - test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags) + (m->queue_mode != DM_TYPE_REQUEST_BASED) * 2); if (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) @@ -1754,8 +1732,6 @@ static void multipath_status(struct dm_target *ti, status_type_t type, DMEMIT("pg_init_retries %u ", m->pg_init_retries);
Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices
So, now we have at least 4 ways to improve mutliapth efficiency: 1) Filtering uevents; 2) Merger uevents; 3) Using separate locks for mpvec and pathvec; 4) Get rid of the gazillion waiter threads. This is exciting, but how do we achieve this blueprint? Can we set up some working groups and develop it in parallel to implement each improvement since most of them are independent? 发件人: "Benjamin Marzinski" 收件人: Martin Wilck , 抄送: dm-devel@redhat.com, tang.jun...@zte.com.cn 日期: 2016/11/19 06:33 主题: Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices 发件人: dm-devel-boun...@redhat.com My $.02 I'm not sure that we want to wait for a predetermined time to grab the uevents. If they are coming in slowly, multipathd is more responsive by processing them immediately, and if they are coming in a storm, they will naturally pile up faster than we deal with them. I also don't really like the idea of waiting when we get an event to see if another one comes along, for the same reasons. That being said, I wouldn't NACK an config option (turned off by default) that set a minimum wait time between uevent dispatches, because I might be wrong here. However, It sees like what uevent_dispatch() dispatch already does is the right thing, which is to pull all oustanding uevents off the uevent queue into a new list. The issue is that we simply need to work on processing the whole list at a time. Martin's filtering definitely has a place here. He is correct that if we get a delete uevent for a device, we don't need to process any of the earlier ones. We do need to look at both the add and change uevents individually. For instance, one of the change uevents out of a group could be for changing the read-only status of a device. If we just took the most recent, we would miss that information. The good news is that we don't need to actually call uev_add_path and uev_update_path once for each of these events. All we need to do is read through the uev environment variables for all of them to find the information we need (like change in ro status), and then call the function once with that information. Hannes pointed out that for adding paths we don't need to do any locking until we want to add the path to the pathvec. We could grab all the outstanding add events from the list that gets passed to service_uevq, and collect the pathinfo for all the paths, add them all into the pathvec, and then create or update the multipath devices. delete uevents could work similarly, where we find all the paths for a multipath device in our list, remove them all, and reload the device once. I'm not sure how much a separate thread for doing the multipath work would buy us, however. It's true that we could have a seperate lock for the mpvec, so we didn't need to hold the pathvec lock while updating the mpvec, but actually updating the mpvec only takes a moment. The time consuming part is building the multipath device and passing it to the kernel. For this, we do need the the paths locked. Otherwise what would keep a path from getting removed while the multipath device was using it to set get set up. If working with multipath devices and proccessing path uevents is happening at the same time, this is a very real possibility. But even if we keep one thread processing the uevents, simply avoiding calling uev_add_path and uev_update_path for repeated add and change events, and batching multiple add and delete events together could provide a real speedup, IMHO. Now, the holy grail of multipathd efficiency would be to totally rework multipathd's locking into something sensible. We could have locks for the vectors that only got held when you were actually traversing or manipulating the vectors, coupled with reference counts on the individual paths and multipaths, so that they didn't get removed while they were in use, and probably locks on the individual paths and multipaths for just the sections that really needed to be protected. The problem is that this would take a huge amount of code auditting to get mostly right, and then a while to flush out all the missed corner cases. At least I think it would. But I dismissed decoupling the config structure from the vecs lock as too painful, and clearly that was possible. At any rate, I'd rather get rid of the gazillion waiter threads first. -Ben On Thu, Nov 17, 2016 at 11:48:32AM +0100, Martin Wilck wrote: > Hi Tang, > > > As to process several uevents for the same physical devices, I think > > the opinions > > different between us is "FILTER" or "MERGER". Personally, I think > > Merger is more > > accuracy, for example, we receive 4 paths addition uevent messages > > from the same > > physical devices: > > 1)uevent add sdb > > 2)uevent add sdc > > 3)uevent add sdd > > 4)uevent add sde > > > > We cannot just filter the 1)2)3) uevent messages but only process the > > 4)uevent message, > > which would cause losing paths of
Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices
Hi Martin, 〉But once you start merging, you'd rather be prepared for 〉several events for the same phys device, too. We can base on such a threshold that there is no repeat uevents from the same sd device, otherwise, we pause doing merger, and kick uevent processing thread to process the merged uevents. Regards, Tang 发件人: Martin Wilck 收件人: tang.jun...@zte.com.cn, 抄送: dm-devel@redhat.com 日期: 2016/11/18 16:38 主题: Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices 发件人: dm-devel-boun...@redhat.com On Fri, 2016-11-18 at 16:24 +0800, tang.jun...@zte.com.cn wrote: > Hi Martin, > > In your case, my action is: > 1) merger uevents 1) 2) to one uevent "add sda sdb", and process them > togother This will fail because sdb is non-existent at the time you try - no? > Though the processing efficiency in such scenario is lower than > yours, but it is simple and reliable, > more importantly, Martin, you still focus on such special scene, > which I concerned is like this: [...] I understand what you're concerned with. I just think we need to do both. I agree that many events for many different devices are more likely. But once you start merging, you'd rather be prepared for several events for the same phys device, too. Martin -- Dr. Martin Wilck , 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices
Hi Martin, > This will fail because sdb is non-existent at the time you try - no? Non-existent paths in merger uevents do not affect the entire algorithm. It should be considered at the time of implementation. Your suggestion is meaningful, but I think we need to focus on the existed problem, resolve it, and then do more work to make it better in other areas. is not it? Regards,Tang 发件人: Martin Wilck 收件人: tang.jun...@zte.com.cn, 抄送: dm-devel@redhat.com 日期: 2016/11/18 16:31 主题: Re: Re: Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices On Fri, 2016-11-18 at 16:24 +0800, tang.jun...@zte.com.cn wrote: > Hi Martin, > > In your case, my action is: > 1) merger uevents 1) 2) to one uevent "add sda sdb", and process them > togother This will fail because sdb is non-existent at the time you try - no? > Though the processing efficiency in such scenario is lower than > yours, but it is simple and reliable, > more importantly, Martin, you still focus on such special scene, > which I concerned is like this: [...] I understand what you're concerned with. I just think we need to do both. I agree that many events for many different devices are more likely. But once you start merging, you'd rather be prepared for several events for the same phys device, too. Martin -- Dr. Martin Wilck , 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
Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices
Hi Martin, In your case, my action is: 1) merger uevents 1) 2) to one uevent "add sda sdb", and process them togother 2) process uevent "del sda" 3) process uevent "add sdc" 4) process uevent "del sdb" 5) process uevent "add sda" Though the processing efficiency in such scenario is lower than yours, but it is simple and reliable, more importantly, Martin, you still focus on such special scene, which I concerned is like this: UDEV [19172.014482] add /devices/platform/host3/session17/target3:0:0/3:0:0:0/block/sdc (block) UDEV [19172.249389] add /devices/platform/host4/session18/target4:0:0/4:0:0:0/block/sdd (block) UDEV [19172.343791] add /devices/platform/host3/session17/target3:0:0/3:0:0:2/block/sdf (block) UDEV [19172.364496] add /devices/platform/host5/session19/target5:0:0/5:0:0:0/block/sdh (block) UDEV [19172.523794] add /devices/platform/host4/session18/target4:0:0/4:0:0:2/block/sdi (block) UDEV [19172.621333] add /devices/platform/host3/session17/target3:0:0/3:0:0:4/block/sdn (block) UDEV [19172.699473] add /devices/platform/host4/session18/target4:0:0/4:0:0:1/block/sdg (block) UDEV [19172.704605] add /devices/platform/host3/session17/target3:0:0/3:0:0:1/block/sde (block) UDEV [19172.709687] add /devices/platform/host3/session17/target3:0:0/3:0:0:3/block/sdj (block) UDEV [19172.714919] add /devices/platform/host4/session18/target4:0:0/4:0:0:3/block/sdl (block) UDEV [19172.728891] add /devices/platform/host4/session18/target4:0:0/4:0:0:4/block/sdo (block) UDEV [19172.872156] add /devices/platform/host3/session17/target3:0:0/3:0:0:6/block/sdt (block) UDEV [19172.915542] add /devices/platform/host4/session18/target4:0:0/4:0:0:6/block/sdu (block) UDEV [19173.086935] add /devices/platform/host6/session20/target6:0:0/6:0:0:0/block/sdw (block) UDEV [19173.108278] add /devices/platform/host6/session20/target6:0:0/6:0:0:1/block/sdz (block) UDEV [19173.195153] add /devices/platform/host4/session18/target4:0:0/4:0:0:5/block/sdr (block) UDEV [19173.228397] add /devices/platform/host3/session17/target3:0:0/3:0:0:5/block/sdq (block) UDEV [19173.363632] add /devices/platform/host5/session19/target5:0:0/5:0:0:2/block/sdm (block) UDEV [19173.386560] add /devices/platform/host3/session17/target3:0:0/3:0:0:7/block/sdx (block) UDEV [19173.394515] add /devices/platform/host4/session18/target4:0:0/4:0:0:7/block/sdy (block) UDEV [19173.410152] add /devices/platform/host5/session19/target5:0:0/5:0:0:1/block/sdk (block) UDEV [19173.474286] add /devices/platform/host6/session20/target6:0:0/6:0:0:2/block/sdab (block) UDEV [19173.508438] add /devices/platform/host5/session19/target5:0:0/5:0:0:3/block/sdp (block) UDEV [19173.713146] add /devices/platform/host5/session19/target5:0:0/5:0:0:4/block/sds (block) UDEV [19173.782065] add /devices/platform/host5/session19/target5:0:0/5:0:0:5/block/sdv (block) UDEV [19173.787447] add /devices/platform/host5/session19/target5:0:0/5:0:0:6/block/sdaa (block) UDEV [19173.803218] add /devices/platform/host6/session20/target6:0:0/6:0:0:3/block/sdad (block) UDEV [19173.849411] add /devices/platform/host5/session19/target5:0:0/5:0:0:7/block/sdac (block) UDEV [19173.918301] add /devices/platform/host6/session20/target6:0:0/6:0:0:5/block/sdaf (block) UDEV [19173.941005] add /devices/platform/host6/session20/target6:0:0/6:0:0:4/block/sdae (block) UDEV [19173.987206] add /devices/platform/host6/session20/target6:0:0/6:0:0:7/block/sdah (block) UDEV [19173.992431] add /devices/platform/host6/session20/target6:0:0/6:0:0:6/block/sdag (block) Or like this: UDEV [20712.402631] remove /devices/platform/host3/session17/target3:0:0/3:0:0:0/block/sdc (block) UDEV [20712.427716] remove /devices/platform/host6/session20/target6:0:0/6:0:0:0/block/sdw (block) UDEV [20712.459854] remove /devices/platform/host4/session18/target4:0:0/4:0:0:0/block/sdd (block) UDEV [20712.471124] remove /devices/platform/host5/session19/target5:0:0/5:0:0:0/block/sdh (block) UDEV [20712.492190] remove /devices/platform/host6/session20/target6:0:0/6:0:0:1/block/sdz (block) UDEV [20712.495245] remove /devices/platform/host4/session18/target4:0:0/4:0:0:1/block/sdg (block) UDEV [20712.512007] remove /devices/platform/host3/session17/target3:0:0/3:0:0:1/block/sde (block) UDEV [20712.522986] remove /devices/platform/host5/session19/target5:0:0/5:0:0:1/block/sdk (block) UDEV [20712.528676] remove /devices/platform/host6/session20/target6:0:0/6:0:0:2/block/sdab (block) UDEV [20712.529891] remove /devices/platform/host5/session19/target5:0:0/5:0:0:2/block/sdm (block) UDEV [20712.536178] remove /devices/platform/host4/session18/target4:0:0/4:0:0:2/block/sdi (block) UDEV [20712.545444] remove /devices/platform/host4/session18/target4:0:0/4:0:0:3/block/sdl (block) UDEV [20712.548006] remove /devices/platform/host3/session17/target3:0:0/3:0:0:3/block/sdj (block) UDEV [20712.551935] remove /devices/platform/host5/session19/target5:0:0/5:0:0:3/block/sdp
Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices
Hi Martin, For your opinion: > My "filtering" idea was meant for cases where several events > for the same device are queued, e.g > 1) add sda > 2) change sda > 3) delete sda >Is it always sufficient to look only at the last event in such a case? I do not agree with you. The reasons are as follows: 1) It’s risky to filter uevents like that, a SCSI device has been generated, May be its life time is very short, but we cannot turn a blind eye on it, the system or applications may need multipath device of the SCSI device. 2) This scenario you mentioned rarely happens, we are more concerned about the more common situation like addition or deletion devices when iSCSI login/logout or FC link up/down with many iSCSI or FC links in each LUN. At this situation we may receive many uevents from different paths of the same LUN device, we want merge these uevents to one and process them together. Regards, Tang 发件人: Martin Wilck 收件人: tang.jun...@zte.com.cn, 抄送: dm-devel@redhat.com 日期: 2016/11/17 18:57 主题: Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices 发件人: dm-devel-boun...@redhat.com Hi Tang, > As to process several uevents for the same physical devices, I think > the opinions > different between us is "FILTER" or "MERGER". Personally, I think > Merger is more > accuracy, for example, we receive 4 paths addition uevent messages > from the same > physical devices: > 1)uevent add sdb > 2)uevent add sdc > 3)uevent add sdd > 4)uevent add sde > > We cannot just filter the 1)2)3) uevent messages but only process the > 4)uevent message, > which would cause losing paths of this multipath devices. Of course. My "filtering" idea was meant for cases where several events for the same device are queued, e.g. 1) add sda 2) change sda 3) delete sda Is it always sufficient to look only at the last event in such a case? I think so, but I'm not 100% certain. Regards Martin -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices
Hi Martin, Nice to see you, May you success in your team and our open source community. You have put forward a good proposal to queued more uevent messages by kicking uevent processing thread in a predefined time intervals. It is also a good idea to have such intervals configuration. As to process several uevents for the same physical devices, I think the opinions different between us is "FILTER" or "MERGER". Personally, I think Merger is more accuracy, for example, we receive 4 paths addition uevent messages from the same physical devices: 1)uevent add sdb 2)uevent add sdc 3)uevent add sdd 4)uevent add sde We cannot just filter the 1)2)3) uevent messages but only process the 4)uevent message, which would cause losing paths of this multipath devices. In my opionion, we should MERGE the four uevent messages to one like: 1)uevent add sdb sdb sdd sde And then uevent processing thread only needs to process one uevent message, and it only produce one DM addition uevent messages(VS. now: one DM addition uevent message and 3 DM change uevent messages) which really reduce system consumption (for example: avoid udev to process every DM uevent messages DM kernel produced). Regards,Tang 发件人: Martin Wilck 收件人: dm-devel@redhat.com, 日期: 2016/11/16 17:59 主题: Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices 发件人: dm-devel-boun...@redhat.com Hi Tang, On Wed, 2016-11-16 at 16:45 +0800, tang.jun...@zte.com.cn wrote: > I think we can merger most of uevent messages and reduce most of > unnecessary DM change > uevent messages during creation/deletion of multipath devices by this > way. > The method you mentioned I think that it is a little complex, and it > not reduce the DM > addition/change/deletion uevent messages which consumed system high > resource. Let me quickly introduce myself, I'm a new member of Hannes' team at SUSE and new on this ML. Apart from Hannes' proposal for more fine-grained locking, I can see the following options: a) instead of kicking the uevent processing thread whenever anything is queued, kick it in predefined time intervals (1 second, say). The uevent processor would then accumulate all changes received since it had been kicked last, and apply DM changes only when necessary. This may need to be a config option because it could obviously lead to delayed device setup in some configurations. b) the logic of a) could be improved by the uevent listener detecting "event storms" and waiting for them to end before kicking the processing thread. The details of the heuristics for "storms" would need to be worked out, of course. c) sometimes several uevents for the same physical path occur in quick succession. Normally it should be sufficient to filter these such that only the last event is processed. Regards, Martin -- Dr. Martin Wilck , 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] Improve processing efficiency for addition and deletion of multipath devices
Hello Hannes: Since the received uevent messages store in the queue, and the speed uevent messages received is much faster than the speed uevent messages processed, so, we can merge these queued uevent message first, and then process it in the next step. Of course, some paths ’ uevent messages of multipath device may not be received yet, but we do not need to wait for it, since we can deal with the left paths in the original way when we received uevent messages of these paths later. I think we can merger most of uevent messages and reduce most of unnecessary DM change uevent messages during creation/deletion of multipath devices by this way. The method you mentioned I think that it is a little complex, and it not reduce the DM addition/change/deletion uevent messages which consumed system high resource. Sincerely Tang On 11/16/2016 02:46 AM, tang.jun...@zte.com.cn wrote: > In these scenarios, multipath processing efficiency is very low: > 1) There are many paths in multipath devices, > 2) Add/delete devices when iSCSI login/logout or FC link up/down. > > Multipath process so slowly that it is not satisfied some applications, > For example, openstack is often timeout in waiting for the creation of > multipath devices. > > The reason of the low processing efficiency I think is that multipath > process uevent message one by one, and each one also produce a new dm > addition/change/deletion uevent message to increased system resource > consumption, actually most of these uevent message have no sense at all. > > So, can we find out a way to reduce these uevent messages to promote > multipath processing efficiency? Personally, I think we can merge > these uevent messages before processing. For example, during the > 4 iSCSI session login procedures, we can wait for a moment until > the addition uevent messages of 4 paths are all arrived, and then we can > merge these uevent messages to one and deal with it at once. The way to > deal with deletion uevent messages is the same. > > How do you think about? Any points of view are welcome. The problem is that we don't know beforehand on how many uevents we should be waiting for. And even if we do there still would be a chance of one or several of these uevents failing to setup the device, so we would be waiting forever here. The one possible way out would be to modify the way we're handling events internally. Event processing really are several steps: 1) Getting information about the attached device (pathinfo() and friends) 2) Store the information in our pathvec 3) Identify and update the mpp structure with the new pathvecs Currently, we're processing each step for every uevent. As we have only a single lock protecting both, pathvec and mppvec, we have to take the lock prior to setup 2 and release it after step 3. So while we could receive events in parallel, we can only process them one-by-one, with every event having to re-do step 3. The idea would be to split off single lock into a pathvec lock and an mppvec lock, and create a separate thread for updating mppvec. Then event processing can be streamlined by having the uevent thread adding the new device to the pathvec and notifying the mppvec thread. This thread could then check if an pathvec update is in progress, and only start once this pathvec handling has finished. With this we would avoid having to issue several similar mppvec updates, and the entire handling would be far smoother. Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] Improve processing efficiency for addition and deletion of multipath devices
In these scenarios, multipath processing efficiency is very low: 1) There are many paths in multipath devices, 2) Add/delete devices when iSCSI login/logout or FC link up/down. Multipath process so slowly that it is not satisfied some applications, For example, openstack is often timeout in waiting for the creation of multipath devices. The reason of the low processing efficiency I think is that multipath process uevent message one by one, and each one also produce a new dm addition/change/deletion uevent message to increased system resource consumption, actually most of these uevent message have no sense at all. So, can we find out a way to reduce these uevent messages to promote multipath processing efficiency? Personally, I think we can merge these uevent messages before processing. For example, during the 4 iSCSI session login procedures, we can wait for a moment until the addition uevent messages of 4 paths are all arrived, and then we can merge these uevent messages to one and deal with it at once. The way to deal with deletion uevent messages is the same. How do you think about? Any points of view are welcome.-- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] dm mpath: add check for count of groups to avoid wild pointer access
From: "tang.junhui" pg is not assigned to a group address when count of multipath groups is zero in bypass_pg_num(), then it is used in bypass_pg(), which may cause wild pointer access. Signed-off-by: tang.junhui --- drivers/md/dm-mpath.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index e477af8..ab4d716 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -1394,7 +1394,7 @@ static int bypass_pg_num(struct multipath *m, const char *pgstr, bool bypassed) char dummy; if (!pgstr || (sscanf(pgstr, "%u%c", &pgnum, &dummy) != 1) || !pgnum || - (pgnum > m->nr_priority_groups)) { + !m->nr_priority_groups || (pgnum > m->nr_priority_groups)) { DMWARN("invalid PG number supplied to bypass_pg"); return -EINVAL; } -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] 答复: Re: dm mpath: add check for count of groups to avoid wild pointer access
Hello Mike, I'm sorry to send you the wrong patch, I'll send a new patch to you later. Thanks Tang 发件人: Mike Snitzer 收件人: tang.jun...@zte.com.cn, 抄送: zhang.ka...@zte.com.cn, dm-devel@redhat.com, a...@redhat.com 日期: 2016/11/03 23:33 主题: Re: [dm-devel] dm mpath: add check for count of groups to avoid wild pointer access 发件人: dm-devel-boun...@redhat.com On Thu, Nov 03 2016 at 6:49am -0400, tang.jun...@zte.com.cn wrote: > From: "tang.junhui" > > pg is not assigned to a group address when count of multipath groups > is zero in bypass_pg_num(), then it is used in bypass_pg(), which may > cause wild pointer access. > > Signed-off-by: tang.junhui > --- > drivers/md/dm-mpath.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index d376dc8..8c1359c 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -1084,7 +1084,7 @@ static int switch_pg_num(struct multipath *m, const char *pgstr) >char dummy; > >if (!pgstr || (sscanf(pgstr, "%u%c", &pgnum, &dummy) != 1) || !pgnum || > - (pgnum > m->nr_priority_groups)) { > + !m->nr_priority_groups || (pgnum > m->nr_priority_groups)) { >DMWARN("invalid PG number supplied to switch_pg_num"); >return -EINVAL; >} > -- > 2.8.1.windows.1 > > You mention bypass_pg_num() going on to hit a NULL/"wild" pointer. Not immediately seeing the relation between switch_pg_num() and bypass_pg_num(). But shouldn't bypass_pg_num() have improved bounds checking (and/or NULL pointer checks) too? Maybe your patch was applied with an offset and it modified switch_pg_num() when you really meant to modify bypass_pg_num()? -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] dm mpath: remove unusable kmultipathd work queue
Hello Mike, I used the master branch, the code seems a little older, It's true that the newer branches such as dm-4.10 have reused kmultipathd. Thanks, Tang 发件人: Mike Snitzer 收件人: tang.jun...@zte.com.cn, 抄送: zhang.ka...@zte.com.cn, dm-devel@redhat.com, a...@redhat.com 日期: 2016/11/03 22:57 主题: Re: [dm-devel] dm mpath: remove unusable kmultipathd work queue 发件人: dm-devel-boun...@redhat.com On Thu, Nov 03 2016 at 6:29am -0400, tang.jun...@zte.com.cn wrote: > From: "tang.junhui" > > kmultipathd work queue is used to process queued IOs for multipath > target when IOs need to retry, but now these IOs have been requeued > to block queue of DM device, hence this work queue is not useful > anymore, and this patch deletes it. > > Signed-off-by: tang.junhui Nack, you must be looking at older code. bio-based DM multipath makes use of kmultipathd. When suggesting changes please always make sure you're working with the latest upstream Linux code. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] dm mpath: add check for count of groups to avoid wild pointer access
From: "tang.junhui" pg is not assigned to a group address when count of multipath groups is zero in bypass_pg_num(), then it is used in bypass_pg(), which may cause wild pointer access. Signed-off-by: tang.junhui --- drivers/md/dm-mpath.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index d376dc8..8c1359c 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -1084,7 +1084,7 @@ static int switch_pg_num(struct multipath *m, const char *pgstr) char dummy; if (!pgstr || (sscanf(pgstr, "%u%c", &pgnum, &dummy) != 1) || !pgnum || - (pgnum > m->nr_priority_groups)) { + !m->nr_priority_groups || (pgnum > m->nr_priority_groups)) { DMWARN("invalid PG number supplied to switch_pg_num"); return -EINVAL; } -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] dm mpath: remove unusable kmultipathd work queue
From: "tang.junhui" kmultipathd work queue is used to process queued IOs for multipath target when IOs need to retry, but now these IOs have been requeued to block queue of DM device, hence this work queue is not useful anymore, and this patch deletes it. Signed-off-by: tang.junhui --- drivers/md/dm-mpath.c | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index d376dc8..8f687b9 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -117,7 +117,7 @@ typedef int (*action_fn) (struct pgpath *pgpath); static struct kmem_cache *_mpio_cache; -static struct workqueue_struct *kmultipathd, *kmpath_handlerd; +static struct workqueue_struct *kmpath_handlerd; static void trigger_event(struct work_struct *work); static void activate_path(struct work_struct *work); static int __pgpath_busy(struct pgpath *pgpath); @@ -935,7 +935,6 @@ static void flush_multipath_work(struct multipath *m) flush_workqueue(kmpath_handlerd); multipath_wait_for_pg_init_completion(m); - flush_workqueue(kmultipathd); flush_work(&m->trigger_event); spin_lock_irqsave(&m->lock, flags); @@ -1737,13 +1736,6 @@ static int __init dm_multipath_init(void) goto bad_register_target; } - kmultipathd = alloc_workqueue("kmpathd", WQ_MEM_RECLAIM, 0); - if (!kmultipathd) { - DMERR("failed to create workqueue kmpathd"); - r = -ENOMEM; - goto bad_alloc_kmultipathd; - } - /* * A separate workqueue is used to handle the device handlers * to avoid overloading existing workqueue. Overloading the @@ -1765,8 +1757,6 @@ static int __init dm_multipath_init(void) return 0; bad_alloc_kmpath_handlerd: - destroy_workqueue(kmultipathd); -bad_alloc_kmultipathd: dm_unregister_target(&multipath_target); bad_register_target: kmem_cache_destroy(_mpio_cache); @@ -1777,7 +1767,6 @@ bad_register_target: static void __exit dm_multipath_exit(void) { destroy_workqueue(kmpath_handlerd); - destroy_workqueue(kmultipathd); dm_unregister_target(&multipath_target); kmem_cache_destroy(_mpio_cache); -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] mpathpersistent: segment faulty occured in mpath_persistent_reserve_in()
Hello Hannes, Since this issue is introduced by RCU, can you have a review for this patch? Thanks, Tang 发件人: tang.we...@zte.com.cn 收件人: christophe varoqui , 抄送: zhang.ka...@zte.com.cn, dm-devel@redhat.com, tang.jun...@zte.com.cn, tang.we...@zte.com.cn 日期: 2016/10/27 17:08 主题: [dm-devel] [PATCH] mpathpersistent: segment faulty occured in mpath_persistent_reserve_in() 发件人: dm-devel-boun...@redhat.com From: 10111224 Segment faulty occured when executing "mpathpersist -i -k /dev/mapper/mpath1" command.The reason is that an uninitialized global variable conf is used in mpath_persistent_reserve_in(). The same problem also exists in mpath_persistent_reserve_out(). Signed-off-by: tang.wenji --- libmpathpersist/mpath_persist.c | 21 +++-- libmpathpersist/mpathpr.h | 4 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c index 7501651..582d4ef 100644 --- a/libmpathpersist/mpath_persist.c +++ b/libmpathpersist/mpath_persist.c @@ -78,6 +78,7 @@ updatepaths (struct multipath * mpp) int i, j; struct pathgroup * pgp; struct path * pp; +struct config *conf; if (!mpp->pg) return 0; @@ -97,16 +98,24 @@ updatepaths (struct multipath * mpp) continue; } pp->mpp = mpp; +conf = get_multipath_config(); pathinfo(pp, conf, DI_ALL); + put_multipath_config(conf); continue; } pp->mpp = mpp; if (pp->state == PATH_UNCHECKED || - pp->state == PATH_WILD) + pp->state == PATH_WILD){ +conf = get_multipath_config(); pathinfo(pp, conf, DI_CHECKER); + put_multipath_config(conf); +} -if (pp->priority == PRIO_UNDEF) +if (pp->priority == PRIO_UNDEF){ +conf = get_multipath_config(); pathinfo(pp, conf, DI_PRIO); + put_multipath_config(conf); +} } } return 0; @@ -159,8 +168,11 @@ int mpath_persistent_reserve_in (int fd, int rq_servact, int map_present; int major, minor; int ret; +struct config *conf; +conf = get_multipath_config(); conf->verbosity = verbose; +put_multipath_config( conf); if (fstat( fd, &info) != 0){ condlog(0, "stat error %d", fd); @@ -252,8 +264,11 @@ int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope, int j; unsigned char *keyp; uint64_t prkey; +struct config *conf; +conf = get_multipath_config(); conf->verbosity = verbose; +put_multipath_config(conf); if (fstat( fd, &info) != 0){ condlog(0, "stat error fd=%d", fd); @@ -320,7 +335,9 @@ int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope, goto out1; } +conf = get_multipath_config(); select_reservation_key(conf, mpp); +put_multipath_config(conf); switch(rq_servact) { diff --git a/libmpathpersist/mpathpr.h b/libmpathpersist/mpathpr.h index cd58201..e6c2ded 100644 --- a/libmpathpersist/mpathpr.h +++ b/libmpathpersist/mpathpr.h @@ -25,10 +25,6 @@ struct threadinfo { struct prout_param param; }; - -struct config * conf; - - int prin_do_scsi_ioctl(char * dev, int rq_servact, struct prin_resp * resp, int noisy); int prout_do_scsi_ioctl( char * dev, int rq_servact, int rq_scope, unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy); -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] hwe_regmatch: match error
Hello Christophe, This issue affects the accuracy of regular matching, and the patch bellow had been reviewed by Hannes, Can you merge it to Opensvc? Thanks Tang On 10/14/2016 04:03 AM, huang.we...@zte.com.cn wrote: > From: "wei.huang" > > Problem: > when we configure a device like vendor, product, revision all null in multipath.conf, hwe_regmatch always return 0. > > Reasons: > \!hwe2->vendor, \!hwe2->product and \!hwe2->revision are all true. > > Signed-off-by: wei.huang > --- > libmultipath/config.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libmultipath/config.c b/libmultipath/config.c > index a48b8af..d99cd75 100644 > --- a/libmultipath/config.c > +++ b/libmultipath/config.c > @@ -80,7 +80,8 @@ hwe_regmatch (struct hwentry *hwe1, struct hwentry *hwe2) >regcomp(&rre, hwe1->revision, REG_EXTENDED|REG_NOSUB)) >goto out_pre; > > - if ((!hwe1->vendor || !hwe2->vendor || > + if ((hwe2->vendor || hwe2->product || hwe2->revision) && > + (!hwe1->vendor || !hwe2->vendor || > !regexec(&vre, hwe2->vendor, 0, NULL, 0)) && >(!hwe1->product || !hwe2->product || > !regexec(&pre, hwe2->product, 0, NULL, 0)) && > Good point. Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] md/dm-mpath: Add NULL pointer check in parse_hw_handler()
From: "tang.junhui" m->hw_handler_name value should be checked in parse_hw_handler() after calling kstrdup() to avoid access of NULL pointer. Signed-off-by: tang.junhui --- drivers/md/dm-mpath.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index d376dc8..77428fb 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -732,6 +732,9 @@ static int parse_hw_handler(struct dm_arg_set *as, struct multipath *m) return 0; m->hw_handler_name = kstrdup(dm_shift_arg(as), GFP_KERNEL); + if (!m->hw_handler_name) + return -ENOMEM; + if (!try_then_request_module(scsi_dh_handler_exist(m->hw_handler_name), "scsi_dh_%s", m->hw_handler_name)) { ti->error = "unknown hardware handler type"; -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] md/dm-table: tgt->type should be putted in dm_table_add_target()
Hello, Mike Thanks for your quick respond. This problem causes failure in "rmmod dm-spp" command, (dm-spp is a tagert type for our product) since the reference count is not zero due to the failure of DM table loads. I don't intend to cc stable, I just add the "Cc: sta...@vger.kernel.org" after Signed-off-by in commit message as Bart suggested it to me, then I send email by "git send-email" command, and it cc to stable automatically, I think maybe my git is not configured well, I'll research it later. (BTW, I like your name, Mike, Thank you for your accompanying with us in our English class for so many years.) thanks, Tang 发件人: Mike Snitzer 收件人: tang.jun...@zte.com.cn, 抄送: zhang.ka...@zte.com.cn, dm-devel@redhat.com, sta...@vger.kernel.org, a...@redhat.com 日期: 2016/10/21 10:24 主题: Re: [dm-devel] md/dm-table: tgt->type should be putted in dm_table_add_target() 发件人: dm-devel-boun...@redhat.com On Thu, Oct 20 2016 at 9:35pm -0400, tang.jun...@zte.com.cn wrote: > From: "tang.junhui" > > tgt->type should be putted in dm_table_add_target() > when the target do not statisfy the needs of target type, > otherwise it would cause the module reference count > of this target type leakage. > > Signed-off-by: tang.junhui > Cc: sta...@vger.kernel.org This issue, missing dm_put_target_type, dates back to 2011. See these 3 commits: $ git log --oneline 3791e2fc0^..36a0456fb 36a0456 dm table: add immutable feature cc6cbe1 dm table: add always writeable feature 3791e2f dm table: add singleton feature So are you having problems with failed DM table loads (due to the errors below) resulting in the inability to unload a DM module? (BTW, you don't need to cc stable when initially proposing a patch for stable. The stable@ maintainers will automatically pull such stable@ changes in once they land in Linus' tree.) Mike > --- > drivers/md/dm-table.c | 24 > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index 6554d91..4f56c38 100644 > --- a/drivers/md/dm-table.c > +++ b/drivers/md/dm-table.c > @@ -698,30 +698,30 @@ int dm_table_add_target(struct dm_table *t, const char *type, > >if (dm_target_needs_singleton(tgt->type)) { >if (t->num_targets) { > - DMERR("%s: target type %s must appear alone in table", > - dm_device_name(t->md), type); > - return -EINVAL; > + tgt->error = "target type must appear alone in table"; > + r = -EINVAL; > + goto bad; >} >t->singleton = 1; >} > >if (dm_target_always_writeable(tgt->type) && !(t->mode & FMODE_WRITE)) { > - DMERR("%s: target type %s may not be included in read-only tables", > -dm_device_name(t->md), type); > - return -EINVAL; > + tgt->error = "target type may not be included in read-only tables"; > + r = -EINVAL; > + goto bad; >} > >if (t->immutable_target_type) { >if (t->immutable_target_type != tgt->type) { > - DMERR("%s: immutable target type %s cannot be mixed with other target types", > - dm_device_name(t->md), t->immutable_target_type->name); > - return -EINVAL; > + tgt->error = "immutable target type cannot be mixed with other target types"; > + r = -EINVAL; > + goto bad; >} >} else if (dm_target_is_immutable(tgt->type)) { >if (t->num_targets) { > - DMERR("%s: immutable target type %s cannot be mixed with other target types", > - dm_device_name(t->md), tgt->type->name); > - return -EINVAL; > + tgt->error = "immutable target type cannot be mixed with other target types"; > + r = -EINVAL; > + goto bad; >} >t->immutable_target_type = tgt->type; >} > -- > 2.8.1.windows.1 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.re
[dm-devel] [PATCH] md/dm-table: tgt->type should be putted in dm_table_add_target()
From: "tang.junhui" tgt->type should be putted in dm_table_add_target() when the target do not statisfy the needs of target type, otherwise it would cause the module reference count of this target type leakage. Signed-off-by: tang.junhui Cc: sta...@vger.kernel.org --- drivers/md/dm-table.c | 24 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 6554d91..4f56c38 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -698,30 +698,30 @@ int dm_table_add_target(struct dm_table *t, const char *type, if (dm_target_needs_singleton(tgt->type)) { if (t->num_targets) { - DMERR("%s: target type %s must appear alone in table", - dm_device_name(t->md), type); - return -EINVAL; + tgt->error = "target type must appear alone in table"; + r = -EINVAL; + goto bad; } t->singleton = 1; } if (dm_target_always_writeable(tgt->type) && !(t->mode & FMODE_WRITE)) { - DMERR("%s: target type %s may not be included in read-only tables", - dm_device_name(t->md), type); - return -EINVAL; + tgt->error = "target type may not be included in read-only tables"; + r = -EINVAL; + goto bad; } if (t->immutable_target_type) { if (t->immutable_target_type != tgt->type) { - DMERR("%s: immutable target type %s cannot be mixed with other target types", - dm_device_name(t->md), t->immutable_target_type->name); - return -EINVAL; + tgt->error = "immutable target type cannot be mixed with other target types"; + r = -EINVAL; + goto bad; } } else if (dm_target_is_immutable(tgt->type)) { if (t->num_targets) { - DMERR("%s: immutable target type %s cannot be mixed with other target types", - dm_device_name(t->md), tgt->type->name); - return -EINVAL; + tgt->error = "immutable target type cannot be mixed with other target types"; + r = -EINVAL; + goto bad; } t->immutable_target_type = tgt->type; } -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 2/3] access vecs memory outside of locking range in check_path()
From: "tang.junhui" there are vecs->mpvec memory accesses outside of locking range in check_path(), the judgments is not necessary since the they has existed in vector_foreach_slot(), so delete them. Signed-off-by: tang.junhui --- multipathd/main.c | 45 ++--- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/multipathd/main.c b/multipathd/main.c index b6eb696..e369a79 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -1804,30 +1804,29 @@ checkerloop (void *ap) condlog(4, "timeout waiting for DAEMON_IDLE"); continue; } - if (vecs->pathvec) { - pthread_cleanup_push(cleanup_lock, &vecs->lock); - lock(&vecs->lock); - pthread_testcancel(); - vector_foreach_slot (vecs->pathvec, pp, i) { - rc = check_path(vecs, pp, ticks); - if (rc < 0) { - vector_del_slot(vecs->pathvec, i); - free_path(pp); - i--; - } else - num_paths += rc; - } - lock_cleanup_pop(vecs->lock); - } - if (vecs->mpvec) { - pthread_cleanup_push(cleanup_lock, &vecs->lock); - lock(&vecs->lock); - pthread_testcancel(); - defered_failback_tick(vecs->mpvec); - retry_count_tick(vecs->mpvec); - missing_uev_wait_tick(vecs); - lock_cleanup_pop(vecs->lock); + + pthread_cleanup_push(cleanup_lock, &vecs->lock); + lock(&vecs->lock); + pthread_testcancel(); + vector_foreach_slot (vecs->pathvec, pp, i) { + rc = check_path(vecs, pp, ticks); + if (rc < 0) { + vector_del_slot(vecs->pathvec, i); + free_path(pp); + i--; + } else + num_paths += rc; } + lock_cleanup_pop(vecs->lock); + + pthread_cleanup_push(cleanup_lock, &vecs->lock); + lock(&vecs->lock); + pthread_testcancel(); + defered_failback_tick(vecs->mpvec); + retry_count_tick(vecs->mpvec); + missing_uev_wait_tick(vecs); + lock_cleanup_pop(vecs->lock); + if (count) count--; else { -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 3/3] Treat PATH_TIMEOUT as PATH_DOWN
From: "tang.junhui" Treat PATH_TIMEOUT as PATH_DOWN since it is basically the same as PATH_DOWN, except with a different state name. So change it to PATH_DOWN immediately if it is PATH_TIMEOUT according to the method in pathinfo(). After the modification all PATH_TIMEOUTs are equal to PATH_DOWN since the rest of pp->state is assigned all in pathinfo(). Signed-off-by: tang.junhui --- multipathd/main.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/multipathd/main.c b/multipathd/main.c index e369a79..f715eb5 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -1528,6 +1528,10 @@ check_path (struct vectors * vecs, struct path * pp, int ticks) put_multipath_config(conf); return 1; } + + if (newstate == PATH_TIMEOUT) + newstate = PATH_DOWN; + if (!pp->mpp) { if (!strlen(pp->wwid) && pp->initialized != INIT_MISSING_UDEV && (newstate == PATH_UP || newstate == PATH_GHOST)) { @@ -1601,7 +1605,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks) pp->checkint = conf->checkint; put_multipath_config(conf); - if (newstate == PATH_DOWN || newstate == PATH_SHAKY || newstate == PATH_TIMEOUT) { + if (newstate == PATH_DOWN || newstate == PATH_SHAKY) { /* * proactively fail path in the DM */ -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 1/3] segment faulty occured in dm_get_status()
From: "tang.junhui" Signed-off-by: tang.junhui segment faulty occured in dm_get_status(), the call trace is: -- (gdb) bt 0 0x00338ec328a5 in raise () from /lib64/libc.so.6 1 0x00338ec3400d in abort () from /lib64/libc.so.6 2 0x0040596d in sigsegv () 3 4 0x00338ec480ac in vfprintf () from /lib64/libc.so.6 5 0x00338ec6f9d2 in vsnprintf () from /lib64/libc.so.6 6 0x00338ec4f4a3 in snprintf () from /lib64/libc.so.6 7 0x7f43fe66bbb9 in dm_get_status (name=0x7f43f0001eb0 "spathe", outstatus=) at devmapper.c:521 8 0x7f43fe68c058 in update_multipath_status (mpp=0x7f43f000ad60, pathvec=) at structs_vec.c:465 9 update_multipath_strings (mpp=0x7f43f000ad60, pathvec=) at structs_vec.c:495 10 0x00409056 in check_path () 11 0x00409c7e in checkerloop () 12 0x003b27207851 in start_thread () from /lib64/libpthread.so.0 13 0x00338ece890d in clone () from /lib64/libc.so.6 --- we debuged the code, and found that targets information storing in the list dmt->head which fetching from kernel by executing dm_task_run() is null. --- rbx0x7f43c4000d60 139929027874144 (gdb) p *(struct dm_task *)0x7f43c4000d60 $3 = {type = 10, dev_name = 0x7f43c4000d40 "spathe", head = 0x0, tail = 0x0, read_only = 0, event_nr = 0, major = -1, minor = -1, allow_default_major_fallback = 1, uid = 0, gid = 0, mode = 384, read_ahead = 4294967295, read_ahead_flags = 0, dmi = { v4 = 0x7f43c40011e0, v1 = 0x7f43c40011e0}, newname = 0x0, message = 0x0, geometry = 0x0, sector = 0, no_flush = 0, no_open_count = 1, skip_lockfs = 0, query_inactive_table = 0, suppress_identical_reload = 0, existing_table_size = 0, cookie_set = 0, uuid = 0x0} --- since status is not initioned to null, so after calling dm_get_next_target(dmt, next, &start, &length,&target_type, &status),status becaming wild pointer, which lead snprintf() into the situation of segment faulty. --- libmultipath/devmapper.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c index 5eb1713..89aa5da 100644 --- a/libmultipath/devmapper.c +++ b/libmultipath/devmapper.c @@ -534,8 +534,8 @@ dm_get_status(char * name, char * outstatus) int r = 1; struct dm_task *dmt; uint64_t start, length; - char *target_type; - char *status; + char *target_type = NULL; + char *status = NULL; if (!(dmt = dm_task_create(DM_DEVICE_STATUS))) return 1; @@ -551,6 +551,10 @@ dm_get_status(char * name, char * outstatus) /* Fetch 1st target */ dm_get_next_target(dmt, NULL, &start, &length, &target_type, &status); + if (!status) { + condlog(2, "get null status."); + goto out; + } if (snprintf(outstatus, PARAMS_SIZE, "%s", status) <= PARAMS_SIZE) r = 0; -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] 答复: [RFC] unreleased lock after handler failure
Hi, Michael Wang This patch looks well for me, cleanup_lock() should be called without affected by the return value of h->fn(). Regards, Tang 发件人: Michael Wang 收件人: christophe.varo...@opensvc.com, dm-devel@redhat.com, 日期: 2016/10/12 16:11 主题: [dm-devel] [RFC] unreleased lock after handler failure 发件人: dm-devel-boun...@redhat.com Hi, folks While we are testing with the latest multipathd, we encounter issue that once 'del map' failed, all the following operation will show 'error -1 receiving packet' whatever how long the timeout has been set (we have applied the latest fix which make sure the poll will last for 10 seconds). After some debugging we found that inside parse_cmd(), the pthread_cleanup_pop() rely on '!r' as indicator for locked or not, while this will be overwritten if the handler return failed (1 in our case), and then the unlock will be missed. After applied below fix the problem got solved, although the del map failed, the other operation can still works. Could you please help to review whether this is really the problem to be fixed like that? Regards, Michael Wang diff --git a/multipathd/cli.c b/multipathd/cli.c index e8a9384..50161be 100644 --- a/multipathd/cli.c +++ b/multipathd/cli.c @@ -481,6 +481,7 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout ) tmo.tv_sec = 0; } if (h->locked) { + int locked = 0; struct vectors * vecs = (struct vectors *)data; pthread_cleanup_push(cleanup_lock, &vecs->lock); @@ -491,10 +492,11 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout ) r = 0; } if (r == 0) { + locked = 1; pthread_testcancel(); r = h->fn(cmdvec, reply, len, data); } - pthread_cleanup_pop(!r); + pthread_cleanup_pop(locked); } else r = h->fn(cmdvec, reply, len, data); free_keys(cmdvec); -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] 答复: Re: [PATCH] libmultipath: fix multipath -q command logic
Hello Hannes, check_daemon() now is only used to determine whether to set/remove queue_if_no_path feature for mapped devices, which you said maybe another issue, we are looking forward for your patch. Thanks, Tang 发件人: Hannes Reinecke 收件人: tang.jun...@zte.com.cn, bmarz...@redhat.com, 抄送: Bart Van Assche , device-mapper development , zhang.ka...@zte.com.cn 日期: 2016/10/11 18:42 主题: Re: [dm-devel] [PATCH] libmultipath: fix multipath -q command logic 发件人: dm-devel-boun...@redhat.com On 10/11/2016 11:17 AM, tang.jun...@zte.com.cn wrote: > Hello Hannes, Ben, > Could you have a review for this patch, any comment will be highly > appreciated. > > Thanks, > Tang > > > > > 发件人: Christophe Varoqui > 收件人: tang.jun...@zte.com.cn, > 抄送:Bart Van Assche , device-mapper > development , zhang.ka...@zte.com.cn > 日期: 2016/10/11 14:59 > 主题:Re: [dm-devel] [PATCH] libmultipath: fix multipath -q > command logic > 发件人:dm-devel-boun...@redhat.com > > > > > Hannes, Ben, > > are you ok with the solution to these two issues. > Seems sane to me. > This actually is only part of the story. The whole idea of issuing 'multipath' is to check if a given path _should_ be multipathed (as this is typically called from an udev event). But as it's called from an udev event we cannot rely on the multipath daemon to be started; we might just handle an event which came in before multipathd got started from systemd. So checking for the PID file is not enough, we need to check if the daemon will be started eventually. And in fact checking the PID file or calling mpath_connect() is equivalent, with the added advantage the mpath_connect() will start the daemon _if enabled by systemd_. So this patch doesn't help much, as it doesn't solve the main problem of figuring out if multipathd _should_ be started. I've done a patch for checking the '.wants' directories from systemd, but this obviously will only work if the OS is systemd-based. And it's not really perfect, as there are corner-cases where just checking for the .wants directory is not enough. Cheers, Hannes -- Dr. Hannes ReineckezSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] libmultipath: fix multipath -q command logic
Hello Ben, In multipath manual document, multipath -q command is described as: "-q allow device tables with queue_if_no_path when multiathd is not running". However the command does not take effect when we testing it. About use-case, In my opinion, sometimes we need to stop multipath service temporarily (for example: multipath version upgrade), but we do not want IO interrupt during those times even path down existed, so we can execute “multiath -q” to keep IO queue until we starting service after version upgrade. Cheers, Tang 发件人: "Benjamin Marzinski" 收件人: Hannes Reinecke , 抄送: Bart Van Assche , device-mapper development , tang.jun...@zte.com.cn, zhang.ka...@zte.com.cn 日期: 2016/10/12 10:53 主题: Re: [dm-devel] [PATCH] libmultipath: fix multipath -q command logic 发件人: dm-devel-boun...@redhat.com On Tue, Oct 11, 2016 at 12:33:43PM +0200, Hannes Reinecke wrote: > On 10/11/2016 11:17 AM, tang.jun...@zte.com.cn wrote: > >Hello Hannes, Ben, > >Could you have a review for this patch, any comment will be highly > >appreciated. > > > >Thanks, > >Tang > > > > > > > > > >发件人: Christophe Varoqui > >收件人: tang.jun...@zte.com.cn, > >抄送:Bart Van Assche , device-mapper > >development , zhang.ka...@zte.com.cn > >日期: 2016/10/11 14:59 > >主题:Re: [dm-devel] [PATCH] libmultipath: fix multipath -q > >command logic > >发件人:dm-devel-boun...@redhat.com > > > > > > > > > >Hannes, Ben, > > > >are you ok with the solution to these two issues. > >Seems sane to me. > > > This actually is only part of the story. > > The whole idea of issuing 'multipath' is to check if a given path _should_ > be multipathed (as this is typically called from an udev event). > But as it's called from an udev event we cannot rely on the multipath daemon > to be started; we might just handle an event which came in before multipathd > got started from systemd. > So checking for the PID file is not enough, we need to check if the daemon > will be started eventually. > And in fact checking the PID file or calling mpath_connect() is equivalent, > with the added advantage the mpath_connect() will start the daemon _if > enabled by systemd_. > So this patch doesn't help much, as it doesn't solve the main problem of > figuring out if multipathd _should_ be started. > > I've done a patch for checking the '.wants' directories from systemd, but > this obviously will only work if the OS is systemd-based. > And it's not really perfect, as there are corner-cases where just checking > for the .wants directory is not enough. I think you are dealing with a different issue. the "-q" option for multipath just overrides the default behaviour of not enabling queue_if_no_path when creating a multipath device is multipathd isn't running. For this, we don't care if multipathd will start running in the future, because if/when multipathd does start up, it will set the queue_if_no_path flag itself. We only care about now, when we know it's not running. Really, I doubt that anyone ever uses the -q option. So your change in how multipath checks if multipathd is running is fine by me. However, you also changed what the "-q" option does. Previously, it just kept multipath from disabling "queue_if_no_path" on devices that were configured to have it, when multipathd wasn't running. With your code, doesn't it now make all devices set queue_if_no_path if multipathd isn't running, including devices that weren't configured to set it even when multipathd is running? Is there a reason for this? In general, setting "queue_if_no_path" when multiapthd isn't running is a bad idea, since the paths will never come back, and nothing will ever disable the queueing. That's why I said that I doubt anybody uses the option. Forcing all devices to set "queue_if_no_path", even if they weren't configured to, seems even less useful. Or is there actually a use-case that I'm missing here? -Ben > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke zSeries & Storage > h...@suse.de +49 911 74053 688 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipathd: fail path when path check timeout
Hello, Ben Thanks for your review, This problem is found by our automatic testing, I will spend more time to correct other similar bugs in the next patch。 Cheers, Tang 发件人: "Benjamin Marzinski" 收件人: tang.jun...@zte.com.cn, 抄送: dm-devel@redhat.com, zhang.ka...@zte.com.cn, bart.vanass...@sandisk.com 日期: 2016/10/12 11:02 主题: Re: [dm-devel] [PATCH] multipathd: fail path when path check timeout 发件人: dm-devel-boun...@redhat.com On Tue, Oct 11, 2016 at 02:50:00PM +0800, tang.jun...@zte.com.cn wrote: >Please have a review for this patch, any comment will be highly >appreciated. This is clearly correct. I suspect that there will be other places where we need to also check for PATH_TIMEOUT, since it is basically the same as PATH_DOWN, except with a different state name. For instance, we only log error messages on repeated checks for (newstate == PATH_DOWN), and we should probably do that for PATH_TIMEOUT as well. There are probably more instances outside of check_path. -Ben > >��: tang.jun...@zte.com.cn >�ռ���: christophe varoqui , >:dm-devel@redhat.com, zhang.ka...@zte.com.cn, "tang.junhui" > >: 2016/08/10 16:11 >:[PATCH] multipathd: fail path when path check timeout > > -- > >From: "tang.junhui" > >path should be failed when path status is PATH_TIMEOUT after check, >otherwise, the valid number of paths in the map would be increased when >the path status is PATH_UP after the next turn check, which would cause >the valid number of paths exceeding the total number of paths in the map. > >Signed-off-by: tang.junhui >--- >multipathd/main.c | 2 +- >1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/multipathd/main.c b/multipathd/main.c >index f5e9a01..01f1e58 100644 >--- a/multipathd/main.c >+++ b/multipathd/main.c >@@ -1585,7 +1585,7 @@ check_path (struct vectors * vecs, struct path * pp, >int ticks) > pp->checkint = conf->checkint; > put_multipath_config(conf); > >- if (newstate == PATH_DOWN || newstate >== PATH_SHAKY) { >+ if (newstate == PATH_DOWN || newstate >== PATH_SHAKY || newstate == PATH_TIMEOUT) { > /* >* proactively fail >path in the DM >*/ >-- >2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 00/15] Fifteen multipath-tools patches
Hello Bart, I also think that we do not need init_path_check_interval(), since pp->checkint is initialized in configure() too. Cheers, Tang 发件人: Bart Van Assche 收件人: , , 抄送: bart.vanass...@sandisk.com, dm-devel@redhat.com 日期: 2016/10/11 04:41 主题: Re: [dm-devel] [PATCH 00/15] Fifteen multipath-tools patches 发件人: dm-devel-boun...@redhat.com On 10/07/2016 07:05 PM, tang.jun...@zte.com.cn wrote: > "0007-multipathd-Fix-a-data-race.patch" > > This patch solved the data race problem, > > but the assignment for paths check interval has no effect, > > since there is no path in vecs when calling > init_path_check_interval(vecs) in child(), > > I think it is better to call > init_path_check_interval(vecs) at reconfigure() or configure() > > after the paths has created in vecs . Hello Tang, If init_path_check_interval() doesn't have any effect then I propose to remove that function. pp->checkint is namely already initialized in cli_add_path(). Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] libmultipath: fix multipath -q command logic
Hello Hannes, Ben, Could you have a review for this patch, any comment will be highly appreciated. Thanks, Tang 发件人: Christophe Varoqui 收件人: tang.jun...@zte.com.cn, 抄送: Bart Van Assche , device-mapper development , zhang.ka...@zte.com.cn 日期: 2016/10/11 14:59 主题: Re: [dm-devel] [PATCH] libmultipath: fix multipath -q command logic 发件人: dm-devel-boun...@redhat.com Hannes, Ben, are you ok with the solution to these two issues. Seems sane to me. Thanks, Christophe On Tue, Oct 11, 2016 at 8:46 AM, wrote: Please have a review for this patch, any comment will be highly appreciated. 发件人: tang.jun...@zte.com.cn 收件人: christophe varoqui , 抄送:dm-devel@redhat.com, zhang.ka...@zte.com.cn, "tang.junhui" < tang.jun...@zte.com.cn> 日期: 2016/08/16 19:33 主题:[PATCH] libmultipath: fix multipath -q command logic From: "tang.junhui" multipath judged whether multipathd service in running by check_daemon() when executing mutipath commands, check_daemon() try to connect to the multipathd service and execute "show dameon" command. The expected result is that the command will be failed when the service is not running, however, mpath_connect() will activate the multipathd service when the service is not running, so check_daemon() always return with true. Another problem is that multipath command with -q parameter is not processed in coalesce_paths(). This patch fix for those two problems. Signed-off-by: tang.junhui --- libmultipath/configure.c | 85 +++- 1 file changed, 48 insertions(+), 37 deletions(-) diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 707e6be..d8a17a6 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -715,36 +715,36 @@ deadmap (struct multipath * mpp) return 1; /* dead */ } -int check_daemon(void) +static int +check_daemon(void) { int fd; - char *reply; - int ret = 0; - unsigned int timeout; - struct config *conf; - - fd = mpath_connect(); - if (fd == -1) - return 0; + struct flock lock; - if (send_packet(fd, "show daemon") != 0) - goto out; - conf = get_multipath_config(); - timeout = conf->uxsock_timeout; - put_multipath_config(conf); - if (recv_packet(fd, &reply, timeout) != 0) - goto out; - - if (strstr(reply, "shutdown")) - goto out_free; - - ret = 1; + fd = open(DEFAULT_PIDFILE, O_RDONLY); + if (fd < 0) { + if (errno == ENOENT) + return 0; + if (errno == EMFILE) + condlog(0, "failed to open file, increase max_fds at %s", DEFAULT_CONFIGFILE); + else + condlog(0, "can not open pidfile %s: %s", DEFAULT_PIDFILE, strerror(errno)); + return -1; + } -out_free: - FREE(reply); -out: - mpath_disconnect(fd); - return ret; + lock.l_type = F_WRLCK; + lock.l_start = 0; + lock.l_whence = SEEK_SET; + lock.l_len = 0; + if (fcntl(fd, F_GETLK, &lock) < 0) { + condlog(0, "can not get file locker, %s: %s", DEFAULT_PIDFILE, strerror(errno)); + close(fd); + return -1; + } + close(fd); + if (lock.l_type == F_UNLCK) + return 0; + return 1; } extern int @@ -873,17 +873,28 @@ coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid, int force_r if (r == DOMAP_DRY) continue; - conf = get_multipath_config(); - allow_queueing = conf->allow_queueing; - put_multipath_config(conf); - if (!is_daemon && !allow_queueing && !check_daemon()) { - if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF && - mpp->no_path_retry != NO_PATH_RETRY_FAIL) - condlog(3, "%s: multipathd not running, unset " -
Re: [dm-devel] [PATCH] multipathd: fail path when path check timeout
Please have a review for this patch, any comment will be highly appreciated. 发件人: tang.jun...@zte.com.cn 收件人: christophe varoqui , 抄送: dm-devel@redhat.com, zhang.ka...@zte.com.cn, "tang.junhui" 日期: 2016/08/10 16:11 主题: [PATCH] multipathd: fail path when path check timeout From: "tang.junhui" path should be failed when path status is PATH_TIMEOUT after check, otherwise, the valid number of paths in the map would be increased when the path status is PATH_UP after the next turn check, which would cause the valid number of paths exceeding the total number of paths in the map. Signed-off-by: tang.junhui --- multipathd/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/multipathd/main.c b/multipathd/main.c index f5e9a01..01f1e58 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -1585,7 +1585,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks) pp->checkint = conf->checkint; put_multipath_config(conf); -if (newstate == PATH_DOWN || newstate == PATH_SHAKY) { +if (newstate == PATH_DOWN || newstate == PATH_SHAKY || newstate == PATH_TIMEOUT) { /* * proactively fail path in the DM */ -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] libmultipath: fix multipath -q command logic
Please have a review for this patch, any comment will be highly appreciated. 发件人: tang.jun...@zte.com.cn 收件人: christophe varoqui , 抄送: dm-devel@redhat.com, zhang.ka...@zte.com.cn, "tang.junhui" 日期: 2016/08/16 19:33 主题: [PATCH] libmultipath: fix multipath -q command logic From: "tang.junhui" multipath judged whether multipathd service in running by check_daemon() when executing mutipath commands, check_daemon() try to connect to the multipathd service and execute "show dameon" command. The expected result is that the command will be failed when the service is not running, however, mpath_connect() will activate the multipathd service when the service is not running, so check_daemon() always return with true. Another problem is that multipath command with -q parameter is not processed in coalesce_paths(). This patch fix for those two problems. Signed-off-by: tang.junhui --- libmultipath/configure.c | 85 +++- 1 file changed, 48 insertions(+), 37 deletions(-) diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 707e6be..d8a17a6 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -715,36 +715,36 @@ deadmap (struct multipath * mpp) return 1; /* dead */ } -int check_daemon(void) +static int +check_daemon(void) { int fd; -char *reply; -int ret = 0; -unsigned int timeout; -struct config *conf; - -fd = mpath_connect(); -if (fd == -1) -return 0; +struct flock lock; -if (send_packet(fd, "show daemon") != 0) -goto out; -conf = get_multipath_config(); -timeout = conf->uxsock_timeout; -put_multipath_config(conf); -if (recv_packet(fd, &reply, timeout) != 0) -goto out; - -if (strstr(reply, "shutdown")) -goto out_free; - -ret = 1; +fd = open(DEFAULT_PIDFILE, O_RDONLY); +if (fd < 0) { +if (errno == ENOENT) +return 0; +if (errno == EMFILE) +condlog(0, "failed to open file, increase max_fds at %s", DEFAULT_CONFIGFILE); +else +condlog(0, "can not open pidfile %s: %s", DEFAULT_PIDFILE, strerror(errno)); +return -1; +} -out_free: -FREE(reply); -out: -mpath_disconnect(fd); -return ret; +lock.l_type = F_WRLCK; +lock.l_start = 0; +lock.l_whence = SEEK_SET; +lock.l_len = 0; +if (fcntl(fd, F_GETLK, &lock) < 0) { +condlog(0, "can not get file locker, %s: %s", DEFAULT_PIDFILE, strerror(errno)); +close(fd); +return -1; +} +close(fd); +if (lock.l_type == F_UNLCK) +return 0; +return 1; } extern int @@ -873,17 +873,28 @@ coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid, int force_r if (r == DOMAP_DRY) continue; -conf = get_multipath_config(); -allow_queueing = conf->allow_queueing; -put_multipath_config(conf); -if (!is_daemon && !allow_queueing && !check_daemon()) { -if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF && -mpp->no_path_retry != NO_PATH_RETRY_FAIL) - condlog(3, "%s: multipathd not running, unset " - "queue_if_no_path feature", mpp->alias); -if (!dm_queue_if_no_path(mpp->alias, 0)) - remove_feature(&mpp->features, - "queue_if_no_path"); +/* run as multipath command and the service is not running */ +if (!is_daemon && !check_daemon()) { +conf = get_multipath_config(); +allow_queueing = conf->allow_queueing; + put_multipath_config(conf); +/* no -q choice */ +if (!allow_queueing) { +if
Re: [dm-devel] [PATCH] multipathd: uxsock_timeout should be assigned by conf->uxsock_timeout
Please have a review for this patch, any comment will be highly appreciated. 发件人: tang.jun...@zte.com.cn 收件人: christophe varoqui , 抄送: zhang.ka...@zte.com.cn, dm-devel@redhat.com, "tang.junhui" 日期: 2016/09/13 18:17 主题: [dm-devel] [PATCH] multipathd: uxsock_timeout should be assigned by conf->uxsock_timeout 发件人: dm-devel-boun...@redhat.com From: "tang.junhui" uxsock_timeout should be assigned by conf->uxsock_timeout before using it in uxclnt() as a CLI client timeout value, otherwise its default value is 0, and the CLI client timeout value is 0 + 100(ms), so the CLI client will be timeout very quickly. Signed-off-by: tang.junhui --- multipathd/main.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/multipathd/main.c b/multipathd/main.c index 96ef01f..a08f1a5 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2549,6 +2549,7 @@ main (int argc, char *argv[]) exit(1); if (verbosity) conf->verbosity = verbosity; +uxsock_timeout = conf->uxsock_timeout; uxclnt(optarg, uxsock_timeout + 100); exit(0); case 'B': @@ -2573,6 +2574,7 @@ main (int argc, char *argv[]) exit(1); if (verbosity) conf->verbosity = verbosity; +uxsock_timeout = conf->uxsock_timeout; memset(cmd, 0x0, CMDSIZE); while (optind < argc) { if (strchr(argv[optind], ' ')) -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] mpathpersist: memset length is wrong
Please have a review for this patch, any comment will be highly appreciated. 发件人: tang.jun...@zte.com.cn 收件人: christophe varoqui , chau...@redhat.com, Vijay , Benjamin Marzinski , 抄送: zhang.ka...@zte.com.cn, dm-devel@redhat.com, "tang.junhui" 日期: 2016/09/21 16:54 主题: [dm-devel] [PATCH] mpathpersist: memset length is wrong 发件人: dm-devel-boun...@redhat.com From: "tang.junhui" variable transportids is cleared by memset() with wrong length MPATH_MX_TIDS, the length should be MPATH_MX_TIDS*sizeof(struct transportid). Signed-off-by: tang.junhui --- mpathpersist/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mpathpersist/main.c b/mpathpersist/main.c index a55865f..8e8cc35 100644 --- a/mpathpersist/main.c +++ b/mpathpersist/main.c @@ -105,7 +105,7 @@ int main (int argc, char * argv[]) udev = udev_new(); conf = mpath_lib_init(udev); -memset(transportids,0,MPATH_MX_TIDS); +memset(transportids, 0, MPATH_MX_TIDS * sizeof(struct transportid)); multipath_conf = conf; while (1) -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 07/15] multipathd: Fix a data race
Hello Bart, This patch solved the data race problem, but the assignment for paths check interval has no effect, since there is no path in vecs when calling init_path_check_interval(vecs) in child(), I think it is better to call init_path_check_interval(vecs) at reconfigure() or configure() after the paths has created in vecs . Thanks, Tang 发件人: Bart Van Assche 收件人: Christophe Varoqui , 抄送: device-mapper development 日期: 2016/10/05 01:45 主题: [dm-devel] [PATCH 07/15] multipathd: Fix a data race 发件人: dm-devel-boun...@redhat.com Avoid that the path check interval initialization loop races with other code that accesses the path vectors by executing that code on the context of the main thread instead of the checker loop. This patch avoids that DRD reports the following: Conflicting store by thread 1 at 0x07f3f1f8 size 8 at 0x40B50C: reconfigure (main.c:2014) by 0x40C2EC: child (main.c:2371) by 0x40CDA1: main (main.c:2609) Address 0x7f3f1f8 is at offset 40 from 0x7f3f1d0. Allocation context: at 0x4C32995: calloc (in /usr/lib64/valgrind/vgpreload_drd-amd64-linux.so) by 0x5DDAC36: zalloc (memory.c:34) by 0x40B61A: init_vecs (main.c:2043) by 0x40BF01: child (main.c:2295) by 0x40CDA1: main (main.c:2609) Signed-off-by: Bart Van Assche --- multipathd/main.c | 24 +++- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/multipathd/main.c b/multipathd/main.c index 3030e85..cdfafe8 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -1742,6 +1742,19 @@ check_path (struct vectors * vecs, struct path * pp, int ticks) return 1; } +static void init_path_check_interval(struct vectors *vecs) +{ +struct config *conf; +struct path *pp; +unsigned int i; + +vector_foreach_slot (vecs->pathvec, pp, i) { +conf = get_multipath_config(); +pp->checkint = conf->checkint; +put_multipath_config(conf); +} +} + static void * checkerloop (void *ap) { @@ -1759,15 +1772,6 @@ checkerloop (void *ap) vecs = (struct vectors *)ap; condlog(2, "path checkers start up"); -/* - * init the path check interval - */ -vector_foreach_slot (vecs->pathvec, pp, i) { -conf = get_multipath_config(); -pp->checkint = conf->checkint; -put_multipath_config(conf); -} - /* Tweak start time for initial path check */ if (clock_gettime(CLOCK_MONOTONIC, &last_time) != 0) last_time.tv_sec = 0; @@ -2327,6 +2331,8 @@ child (void * param) */ post_config_state(DAEMON_CONFIGURE); +init_path_check_interval(vecs); + /* * Start uevent listener early to catch events */ -- 2.10.0 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] mpathpersist: memset length is wrong
From: "tang.junhui" variable transportids is cleared by memset() with wrong length MPATH_MX_TIDS, the length should be MPATH_MX_TIDS*sizeof(struct transportid). Signed-off-by: tang.junhui --- mpathpersist/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mpathpersist/main.c b/mpathpersist/main.c index a55865f..8e8cc35 100644 --- a/mpathpersist/main.c +++ b/mpathpersist/main.c @@ -105,7 +105,7 @@ int main (int argc, char * argv[]) udev = udev_new(); conf = mpath_lib_init(udev); - memset(transportids,0,MPATH_MX_TIDS); + memset(transportids, 0, MPATH_MX_TIDS * sizeof(struct transportid)); multipath_conf = conf; while (1) -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] multipathd: uxsock_timeout should be assigned by conf->uxsock_timeout
From: "tang.junhui" uxsock_timeout should be assigned by conf->uxsock_timeout before using it in uxclnt() as a CLI client timeout value, otherwise its default value is 0, and the CLI client timeout value is 0 + 100(ms), so the CLI client will be timeout very quickly. Signed-off-by: tang.junhui --- multipathd/main.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/multipathd/main.c b/multipathd/main.c index 96ef01f..a08f1a5 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2549,6 +2549,7 @@ main (int argc, char *argv[]) exit(1); if (verbosity) conf->verbosity = verbosity; + uxsock_timeout = conf->uxsock_timeout; uxclnt(optarg, uxsock_timeout + 100); exit(0); case 'B': @@ -2573,6 +2574,7 @@ main (int argc, char *argv[]) exit(1); if (verbosity) conf->verbosity = verbosity; + uxsock_timeout = conf->uxsock_timeout; memset(cmd, 0x0, CMDSIZE); while (optind < argc) { if (strchr(argv[optind], ' ')) -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] kpartx: partitions of mapped devices cannot be created in redhat OS
From: "tang.junhui" Environment variables such as DM_TABLE_STATE are used in kpartx.rules which exported by "dmsetup export" in previous udev rules in SuSE OS, however, there is no such command "dmsetup export" in redhat OS, so these environment variables are not initialized and partitions cannot be created. This patch replace "dmsetup export" with "dmsetup info" to get the status of mapped device, which can work well both in SuSE and redhat OS. Signed-off-by: tang.junhui --- kpartx/kpartx.rules | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules index 1713f3c..8c8b836 100644 --- a/kpartx/kpartx.rules +++ b/kpartx/kpartx.rules @@ -7,8 +7,12 @@ KERNEL!="dm-*", GOTO="kpartx_end" ACTION=="remove", GOTO="kpartx_end" -ENV{DM_TABLE_STATE}!="LIVE", GOTO="kpartx_end" -ENV{DM_DEPS}=="0", GOTO="kpartx_end" +ENV{DMSETUP_SBIN_PATH}="/sbin" +TEST!="$env{DMSETUP_SBIN_PATH}/dmsetup", ENV{DMSETUP_SBIN_PATH}="/usr/sbin" +IMPORT{program}="$env{DMSETUP_SBIN_PATH}/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o tables_loaded,device_count" + +ENV{DM_TABLES_LOADED}!="Live", GOTO="kpartx_end" +ENV{DM_DEVICE_COUNT}=="0", GOTO="kpartx_end" ENV{DM_UUID}=="?*", IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}" @@ -37,7 +41,7 @@ ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", \ # Create dm tables for partitions ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", GOTO="kpartx_end" ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end" -ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \ +ENV{DM_SUSPENDED}!="Suspended", ENV{DM_UUID}=="mpath-*", \ RUN+="/sbin/kpartx -u -p -part /dev/$name" LABEL="kpartx_end" -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] kpartx: partitions of mapped devices cannot be created in redhat OS
From: "tang.junhui" Environment variables such as DM_TABLE_STATE are used in kpartx.rules which exported by "dmsetup export" in previous udev rules in SuSE OS, however, there is no such command "dmsetup export" in redhat OS, so these environment variables are not initialized and partitions cannot be created. This patch replace "dmsetup export" with "dmsetup info" to get the status of mapped device, which can work well both in SuSE and redhat OS. Signed-off-by: tang.junhui --- kpartx/kpartx.rules | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules index 1713f3c..8c8b836 100644 --- a/kpartx/kpartx.rules +++ b/kpartx/kpartx.rules @@ -7,8 +7,12 @@ KERNEL!="dm-*", GOTO="kpartx_end" ACTION=="remove", GOTO="kpartx_end" -ENV{DM_TABLE_STATE}!="LIVE", GOTO="kpartx_end" -ENV{DM_DEPS}=="0", GOTO="kpartx_end" +ENV{DMSETUP_SBIN_PATH}="/sbin" +TEST!="$env{DMSETUP_SBIN_PATH}/dmsetup", ENV{DMSETUP_SBIN_PATH}="/usr/sbin" +IMPORT{program}="$env{DMSETUP_SBIN_PATH}/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o tables_loaded,device_count" + +ENV{DM_TABLES_LOADED}!="Live", GOTO="kpartx_end" +ENV{DM_DEVICE_COUNT}=="0", GOTO="kpartx_end" ENV{DM_UUID}=="?*", IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}" @@ -37,7 +41,7 @@ ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", \ # Create dm tables for partitions ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", GOTO="kpartx_end" ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end" -ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \ +ENV{DM_SUSPENDED}!="Suspended", ENV{DM_UUID}=="mpath-*", \ RUN+="/sbin/kpartx -u -p -part /dev/$name" LABEL="kpartx_end" -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] libmultipath: fix multipath -q command logic
From: "tang.junhui" multipath judged whether multipathd service in running by check_daemon() when executing mutipath commands, check_daemon() try to connect to the multipathd service and execute "show dameon" command. The expected result is that the command will be failed when the service is not running, however, mpath_connect() will activate the multipathd service when the service is not running, so check_daemon() always return with true. Another problem is that multipath command with -q parameter is not processed in coalesce_paths(). This patch fix for those two problems. Signed-off-by: tang.junhui --- libmultipath/configure.c | 85 +++- 1 file changed, 48 insertions(+), 37 deletions(-) diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 707e6be..d8a17a6 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -715,36 +715,36 @@ deadmap (struct multipath * mpp) return 1; /* dead */ } -int check_daemon(void) +static int +check_daemon(void) { int fd; - char *reply; - int ret = 0; - unsigned int timeout; - struct config *conf; - - fd = mpath_connect(); - if (fd == -1) - return 0; + struct flock lock; - if (send_packet(fd, "show daemon") != 0) - goto out; - conf = get_multipath_config(); - timeout = conf->uxsock_timeout; - put_multipath_config(conf); - if (recv_packet(fd, &reply, timeout) != 0) - goto out; - - if (strstr(reply, "shutdown")) - goto out_free; - - ret = 1; + fd = open(DEFAULT_PIDFILE, O_RDONLY); + if (fd < 0) { + if (errno == ENOENT) + return 0; + if (errno == EMFILE) + condlog(0, "failed to open file, increase max_fds at %s", DEFAULT_CONFIGFILE); + else + condlog(0, "can not open pidfile %s: %s", DEFAULT_PIDFILE, strerror(errno)); + return -1; + } -out_free: - FREE(reply); -out: - mpath_disconnect(fd); - return ret; + lock.l_type = F_WRLCK; + lock.l_start = 0; + lock.l_whence = SEEK_SET; + lock.l_len = 0; + if (fcntl(fd, F_GETLK, &lock) < 0) { + condlog(0, "can not get file locker, %s: %s", DEFAULT_PIDFILE, strerror(errno)); + close(fd); + return -1; + } + close(fd); + if (lock.l_type == F_UNLCK) + return 0; + return 1; } extern int @@ -873,17 +873,28 @@ coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid, int force_r if (r == DOMAP_DRY) continue; - conf = get_multipath_config(); - allow_queueing = conf->allow_queueing; - put_multipath_config(conf); - if (!is_daemon && !allow_queueing && !check_daemon()) { - if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF && - mpp->no_path_retry != NO_PATH_RETRY_FAIL) - condlog(3, "%s: multipathd not running, unset " - "queue_if_no_path feature", mpp->alias); - if (!dm_queue_if_no_path(mpp->alias, 0)) - remove_feature(&mpp->features, - "queue_if_no_path"); + /* run as multipath command and the service is not running */ + if (!is_daemon && !check_daemon()) { + conf = get_multipath_config(); + allow_queueing = conf->allow_queueing; + put_multipath_config(conf); + /* no -q choice */ + if (!allow_queueing) { + if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF && + mpp->no_path_retry != NO_PATH_RETRY_FAIL) + condlog(3, "%s: multipathd not running, unset " + "queue_if_no_path feature", mpp->alias); + if (!dm_queue_if_no_path(mpp->alias, 0)) + remove_feature(&mpp->features, + "queue_if_no_path"); + } else { /* with -q choice */ + if (mpp->no_path_retry == NO_PATH_RETRY_UNDEF || + mpp->no_path_retry == NO_PATH_RETRY_FAIL) + condlog(3, "%s: multipathd not running, set " + "queue_if_no_path feature", mpp->alias); + if (!dm_queue_if_no_path(mpp->alias, 1)) + add_feature(&mpp->features, "q
[dm-devel] [PATCH] libmultipath: fix memory API logic error
From: "tang.junhui" Memroy API use mem_allocated to record the total size of used memory, however, it's wrong to use size(p) as the length of freed memory in xfree(), and memory may also be allocated by STRDUP() or REALLOC(), which is not calculated into mem_allocated, so the total size of used memory is not correctly. For these reasons, we removed these incorrectly code to keep the code clean. Signed-off-by: tang.junhui --- libmpathpersist/mpath_updatepr.c | 1 - libmultipath/memory.c| 31 --- libmultipath/memory.h| 13 ++--- 3 files changed, 2 insertions(+), 43 deletions(-) diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c index 9ff4b30..5af2e03 100644 --- a/libmpathpersist/mpath_updatepr.c +++ b/libmpathpersist/mpath_updatepr.c @@ -16,7 +16,6 @@ #include "uxsock.h" #include "memory.h" -unsigned long mem_allocated;/* Total memory used in Bytes */ int update_prflag(char * arg1, char * arg2, int noisy) { diff --git a/libmultipath/memory.c b/libmultipath/memory.c index 1366f45..5441e6a 100644 --- a/libmultipath/memory.c +++ b/libmultipath/memory.c @@ -22,37 +22,6 @@ #include "memory.h" -/* Global var */ -unsigned long mem_allocated; /* Total memory used in Bytes */ - -void * -xalloc(unsigned long size) -{ - void *mem; - if ((mem = malloc(size))) - mem_allocated += size; - return mem; -} - -void * -zalloc(unsigned long size) -{ - void *mem; - if ((mem = malloc(size))) { - memset(mem, 0, size); - mem_allocated += size; - } - return mem; -} - -void -xfree(void *p) -{ - mem_allocated -= sizeof (p); - free(p); - p = NULL; -} - /* * Memory management. in debug mode, * help finding eventual memory leak. diff --git a/libmultipath/memory.h b/libmultipath/memory.h index 8573f6f..29a75ed 100644 --- a/libmultipath/memory.h +++ b/libmultipath/memory.h @@ -29,15 +29,6 @@ #include #include -/* extern types */ -extern unsigned long mem_allocated; -extern void *xalloc(unsigned long size); -extern void *zalloc(unsigned long size); -extern void xfree(void *p); - -/* Global alloc macro */ -#define ALLOC(n) (xalloc(n)) - /* Local defines */ #ifdef _DEBUG_ @@ -63,8 +54,8 @@ extern void dbg_free_final(char *); #else -#define MALLOC(n)(zalloc(n)) -#define FREE(p) (xfree(p)) +#define MALLOC(n)(calloc(1,(n))) +#define FREE(p) do { free(p); p = NULL; } while(0) #define REALLOC(p,n) (realloc((p),(n))) #define STRDUP(n)(strdup(n)) -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] 答复: Re: [PATCH] libmultipath: fix memory API logic error
Hi, Johannes, Your advice is nice, thank you I will send the modified patch later. 发件人: Johannes Thumshirn 收件人: tang.jun...@zte.com.cn, 抄送: christophe varoqui , zhang.ka...@zte.com.cn, dm-devel@redhat.com 日期: 2016/08/15 21:59 主题: Re: [dm-devel] [PATCH] libmultipath: fix memory API logic error On Mon, Aug 15, 2016 at 06:59:09PM +0800, tang.jun...@zte.com.cn wrote: > From: "tang.junhui" > > Memroy API use mem_allocated to record the total size of used memory, > however, it's wrong to use size(p) as the length of freed memory in xfree(), > and memory may also be allocated by STRDUP() or REALLOC(), which is > not calculated into mem_allocated, so the total size of used memory is > not correctly. For these reasons, we removed these incorrectly code to keep > the code clean. > > Signed-off-by: tang.junhui > --- > libmpathpersist/mpath_updatepr.c | 1 - > libmultipath/memory.c| 31 --- > libmultipath/memory.h| 13 ++--- > 3 files changed, 2 insertions(+), 43 deletions(-) > > diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c > index 9ff4b30..5af2e03 100644 > --- a/libmpathpersist/mpath_updatepr.c > +++ b/libmpathpersist/mpath_updatepr.c > @@ -16,7 +16,6 @@ > #include "uxsock.h" > #include "memory.h" > > -unsigned long mem_allocated;/* Total memory used in Bytes */ > > int update_prflag(char * arg1, char * arg2, int noisy) > { > diff --git a/libmultipath/memory.c b/libmultipath/memory.c > index 1366f45..5441e6a 100644 > --- a/libmultipath/memory.c > +++ b/libmultipath/memory.c > @@ -22,37 +22,6 @@ > [...] > - > -void > -xfree(void *p) > -{ > - mem_allocated -= sizeof (p); > - free(p); > - p = NULL; > -} > - > /* [...] > -#define MALLOC(n)(zalloc(n)) > -#define FREE(p) (xfree(p)) > +#define MALLOC(n)(calloc(1,(n))) > +#define FREE(p) (free(p)) > #define REALLOC(p,n) (realloc((p),(n))) > #define STRDUP(n)(strdup(n)) One minor nit, the original xfree() has set the freed pointer to 0, so maybe a #define FREE(p) do { free(p); p = NULL } while(0) would be advisable. I personally like setting free'd pointers to NULL, as a NULL pointer access is a more immediate feedback than some rather nasty use-after-free. Just my $0.2 Byte, Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] libmultipath: fix memory API logic error
From: "tang.junhui" Memroy API use mem_allocated to record the total size of used memory, however, it's wrong to use size(p) as the length of freed memory in xfree(), and memory may also be allocated by STRDUP() or REALLOC(), which is not calculated into mem_allocated, so the total size of used memory is not correctly. For these reasons, we removed these incorrectly code to keep the code clean. Signed-off-by: tang.junhui --- libmpathpersist/mpath_updatepr.c | 1 - libmultipath/memory.c| 31 --- libmultipath/memory.h| 13 ++--- 3 files changed, 2 insertions(+), 43 deletions(-) diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c index 9ff4b30..5af2e03 100644 --- a/libmpathpersist/mpath_updatepr.c +++ b/libmpathpersist/mpath_updatepr.c @@ -16,7 +16,6 @@ #include "uxsock.h" #include "memory.h" -unsigned long mem_allocated;/* Total memory used in Bytes */ int update_prflag(char * arg1, char * arg2, int noisy) { diff --git a/libmultipath/memory.c b/libmultipath/memory.c index 1366f45..5441e6a 100644 --- a/libmultipath/memory.c +++ b/libmultipath/memory.c @@ -22,37 +22,6 @@ #include "memory.h" -/* Global var */ -unsigned long mem_allocated; /* Total memory used in Bytes */ - -void * -xalloc(unsigned long size) -{ - void *mem; - if ((mem = malloc(size))) - mem_allocated += size; - return mem; -} - -void * -zalloc(unsigned long size) -{ - void *mem; - if ((mem = malloc(size))) { - memset(mem, 0, size); - mem_allocated += size; - } - return mem; -} - -void -xfree(void *p) -{ - mem_allocated -= sizeof (p); - free(p); - p = NULL; -} - /* * Memory management. in debug mode, * help finding eventual memory leak. diff --git a/libmultipath/memory.h b/libmultipath/memory.h index 8573f6f..99937b2 100644 --- a/libmultipath/memory.h +++ b/libmultipath/memory.h @@ -29,15 +29,6 @@ #include #include -/* extern types */ -extern unsigned long mem_allocated; -extern void *xalloc(unsigned long size); -extern void *zalloc(unsigned long size); -extern void xfree(void *p); - -/* Global alloc macro */ -#define ALLOC(n) (xalloc(n)) - /* Local defines */ #ifdef _DEBUG_ @@ -63,8 +54,8 @@ extern void dbg_free_final(char *); #else -#define MALLOC(n)(zalloc(n)) -#define FREE(p) (xfree(p)) +#define MALLOC(n)(calloc(1,(n))) +#define FREE(p) (free(p)) #define REALLOC(p,n) (realloc((p),(n))) #define STRDUP(n)(strdup(n)) -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] libmultipath: retry to get path serial number by get_serial() after failure by get_vpd_sysfs()
From: "tang.junhui" Some operating systems have no such file vpd_pg80 in sys directory such as /sys/devices/platform/host23/session21/target23:0:0/23:0:0:33/, so path serial number would be failed to get by get_vpd_sysfs(), we need retry to get it by get_serial to avoid empty serial of the path. Signed-off-by: tang.junhui --- libmultipath/discovery.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index 0974c92..bb3116d 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -1435,10 +1435,14 @@ scsi_ioctl_pathinfo (struct path * pp, int mask) if (!attr_path || pp->sg_id.host_no == -1) return 0; - if (get_vpd_sysfs(parent, 0x80, pp->serial, SERIAL_SIZE) > 0) - condlog(3, "%s: serial = %s", - pp->dev, pp->serial); + if (get_vpd_sysfs(parent, 0x80, pp->serial, SERIAL_SIZE) <= 0) { + if (get_serial(pp->serial, SERIAL_SIZE, pp->fd)) { + condlog(2, "%s: fail to get serial", pp->dev); + return 0; + } + } + condlog(3, "%s: serial = %s", pp->dev, pp->serial); return 0; } -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] multipathd: fix some small bugs for cli socket
From: "tang.junhui" 1) mpath_recv_reply_len() should return error when the length which received by read_all() is wrong; 2) The buffer which *reply point to maybe null when mpath_recv_reply() return with success, so the caller needs to determine whether it is null, and then to use it. Signed-off-by: tang.junhui --- libmpathcmd/mpath_cmd.c | 2 +- multipathd/uxclnt.c | 3 +++ multipathd/uxlsnr.c | 4 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/libmpathcmd/mpath_cmd.c b/libmpathcmd/mpath_cmd.c index 2290ecb..c058479 100644 --- a/libmpathcmd/mpath_cmd.c +++ b/libmpathcmd/mpath_cmd.c @@ -112,7 +112,7 @@ ssize_t mpath_recv_reply_len(int fd, unsigned int timeout) return ret; if (ret != sizeof(len)) { errno = EIO; - return ret; + return -1; } return len; } diff --git a/multipathd/uxclnt.c b/multipathd/uxclnt.c index 62ff6f4..c5c32ea 100644 --- a/multipathd/uxclnt.c +++ b/multipathd/uxclnt.c @@ -28,6 +28,9 @@ static void print_reply(char *s) { + if (!s) + return; + if (isatty(1)) { printf("%s", s); return; diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c index fa29b2a..7a9faf3 100644 --- a/multipathd/uxlsnr.c +++ b/multipathd/uxlsnr.c @@ -243,6 +243,10 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data) dead_client(c); continue; } + if (!inbuf) { + condlog(4, "recv_packet get null request"); + continue; + } condlog(4, "cli[%d]: Got request [%s]", i, inbuf); uxsock_trigger(inbuf, &reply, &rlen, -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] multipathd: fix memory allocation logic error for polls in uxsock_listen()
From: "tang.junhui" logic error exists in memory allocation for polls in uxsock_listen(), even if the allocated memory size meet the needs, it is still to realloc memory, which is not up to expectations. Signed-off-by: tang.junhui --- multipathd/uxlsnr.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c index f114e59..fa29b2a 100644 --- a/multipathd/uxlsnr.c +++ b/multipathd/uxlsnr.c @@ -145,7 +145,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data) pthread_cleanup_push(uxsock_cleanup, NULL); condlog(3, "uxsock: startup listener"); - polls = (struct pollfd *)MALLOC(MIN_POLLS + 1); + polls = (struct pollfd *)MALLOC((MIN_POLLS + 1) * sizeof(struct pollfd)); if (!polls) { condlog(0, "uxsock: failed to allocate poll fds"); return NULL; @@ -167,9 +167,11 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data) } if (num_clients != old_clients) { struct pollfd *new; - if (num_clients < MIN_POLLS) { + if (num_clients <= MIN_POLLS && old_clients > MIN_POLLS) { new = REALLOC(polls, (1 + MIN_POLLS) * sizeof(struct pollfd)); + } else if (num_clients <= MIN_POLLS && old_clients <= MIN_POLLS) { + new = polls; } else { new = REALLOC(polls, (1+num_clients) * sizeof(struct pollfd)); @@ -181,7 +183,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data) pthread_yield(); continue; } - num_clients = old_clients; + old_clients = num_clients; polls = new; } polls[0].fd = ux_sock; -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] multipathd: fail path when path check timeout
From: "tang.junhui" path should be failed when path status is PATH_TIMEOUT after check, otherwise, the valid number of paths in the map would be increased when the path status is PATH_UP after the next turn check, which would cause the valid number of paths exceeding the total number of paths in the map. Signed-off-by: tang.junhui --- multipathd/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/multipathd/main.c b/multipathd/main.c index f5e9a01..01f1e58 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -1585,7 +1585,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks) pp->checkint = conf->checkint; put_multipath_config(conf); - if (newstate == PATH_DOWN || newstate == PATH_SHAKY) { + if (newstate == PATH_DOWN || newstate == PATH_SHAKY || newstate == PATH_TIMEOUT) { /* * proactively fail path in the DM */ -- 2.8.1.windows.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] multipathd: fix memory leak in reconfigure()
From: "tang.junhui" Problem: Memory leak exists in reconfigure() when multipathd command reconfigure is executed. Reasons: * The following judgment condition is not satisfied when there is no path, free_pathvec() is not called to free the vector of vecs->pathvec: if (VECTOR_SIZE(vecs->pathvec)) free_pathvec(vecs->pathvec, FREE_PATHS); * Then the vecs->pathvec is set to NULL, so the vector memory which vecs->pathvec pointed to is leaked: vecs->pathvec = NULL; Signed-off-by: tang.junhui --- multipathd/main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/multipathd/main.c b/multipathd/main.c index c129298..d9731cd 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -1997,9 +1997,7 @@ reconfigure (struct vectors * vecs) if (VECTOR_SIZE(vecs->mpvec)) remove_maps_and_stop_waiters(vecs); - if (VECTOR_SIZE(vecs->pathvec)) - free_pathvec(vecs->pathvec, FREE_PATHS); - + free_pathvec(vecs->pathvec, FREE_PATHS); vecs->pathvec = NULL; /* Re-read any timezone changes */ -- 2.8.1.windows.1 ZTE Information Security Notice: The information contained in this mail (and any attachment transmitted herewith) is privileged and confidential and is intended for the exclusive use of the addressee(s). If you are not an intended recipient, any disclosure, reproduction, distribution or other dissemination or use of the information contained is strictly prohibited. If you have received this mail in error, please delete it and notify us immediately. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] libmultipath: fix memory leak in reconfigure
Problem: * Memory leak exists in reconfigure function when multipathd command "reconfigure" is executed. Reasons: * The following judgement condition is not satisfied when there is no path , free_pathvec is not called to free the vector of vecs->pathvec: if (VECTOR_SIZE(vecs->pathvec)) free_pathvec(vecs->pathvec, FREE_PATHS); * Then the vecs->pathvec is set to NULL, so the vector memory which vecs->pathvec pointed to is leaked: vecs->pathvec = NULL; Please review the solutions to the problem in the attachment: 唐军辉 Tang Junhui 主机侧软件开发 Host-side Software Development 无线研究院 / 虚拟化长沙开发部 NIV Changsha Development Dept/Wireless Product R&D Institute/Wireless Product Operation 湖南长沙岳麓区尖山路中电软件园8楼 410205 8/F,CEC Software Park,NO.39, Jianshan Road, Lugu High-tech Industrial Center, Yuelu District, Changsha, P.R. China T: 0731 x M: +86 15616251617 E: tangjun...@zte.com.cn www.zte.com.cn ZTE Information Security Notice: The information contained in this mail (and any attachment transmitted herewith) is privileged and confidential and is intended for the exclusive use of the addressee(s). If you are not an intended recipient, any disclosure, reproduction, distribution or other dissemination or use of the information contained is strictly prohibited. If you have received this mail in error, please delete it and notify us immediately. [dm-devel] [PATCH] libmultipath fix memory leak in reconfigure function.patch Description: Binary data -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel