| Message ID | 1482825809-9528-12-git-send-email-tang.junhui@zte.com.cn (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
On Tue, Dec 27, 2016 at 04:03:28PM +0800, tang.junhui@zte.com.cn wrote: > From: "tang.junhui" <tang.junhui@zte.com.cn> > > After filtering and merging, then uevents are processed in > uev_trigger(), firstly, each of merged uevents would be processed one > by one with need_do_map in value of 0. Finally, the node “uev” itself > would be processed with need_do_map in value of 1, which would reload > kernel table, and create or remove the dm device. 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 <tang.junhui@zte.com.cn> > --- > multipathd/main.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 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
Hello Ben Good idea, I will modify the patch to filter these change uevents. Thanks Tang Junhui 发件人: "Benjamin Marzinski" <bmarzins@redhat.com> 收件人: tang.junhui@zte.com.cn, 抄送: tang.wenjun3@zte.com.cn, zhang.kai16@zte.com.cn, dm-devel@redhat.com, bart.vanassche@sandisk.com, mwilck@suse.com 日期: 2017/01/04 09:10 主题: Re: [dm-devel] [PATCH 11/12] multipathd: proccess merged uevents 发件人: dm-devel-bounces@redhat.com On Tue, Dec 27, 2016 at 04:03:28PM +0800, tang.junhui@zte.com.cn wrote: > From: "tang.junhui" <tang.junhui@zte.com.cn> > > After filtering and merging, then uevents are processed in > uev_trigger(), firstly, each of merged uevents would be processed one > by one with need_do_map in value of 0. Finally, the node “uev” itself > would be processed with need_do_map in value of 1, which would reload > kernel table, and create or remove the dm device. 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 <tang.junhui@zte.com.cn> > --- > multipathd/main.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 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
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;