* [dm-devel] [PATCH] multipathd: fix missing persistent reseravtion for active path @ 2021-09-10 12:31 lixiaokeng 2021-09-10 16:17 ` Martin Wilck 0 siblings, 1 reply; 5+ messages in thread From: lixiaokeng @ 2021-09-10 12:31 UTC (permalink / raw) To: Christophe Varoqui, Benjamin Marzinski, Martin Wilck, dm-devel mailing list Cc: linfeilong, liuzhiqiang (I) There are two paths(sucu as sda and adb) for one LUN. The two paths log in, but before the two uevents have been processed (for example there are many uevent), users use multipathd add path /dev/sda to cause mpatha and use mpathpersist -o -I to register prkey for mpatha. The add map uevent is after add path uevent, the the uevent(add sdb) will delay and missing persistent reseravtion check. Here, we add persistent reseravtion check in ev_add_map if mpp->wait_for_udev > 1. Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com> --- multipathd/main.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/multipathd/main.c b/multipathd/main.c index 3aff241d..ef456c34 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -706,6 +706,8 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs) struct multipath * mpp; int delayed_reconfig, reassign_maps; struct config *conf; + struct path *pp; + int i; if (dm_is_mpath(alias) != 1) { condlog(4, "%s: not a multipath map", alias); @@ -721,6 +723,13 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs) if (update_map(mpp, vecs, 0)) /* setup multipathd removed the map */ return 1; + + vector_foreach_slot(mpp->paths, pp, i) { + if ((pp->state == PATH_UP) || (pp->state == PATH_GHOST)) { + /* persistent reseravtion check*/ + mpath_pr_event_handle(pp); + } + } } conf = get_multipath_config(); delayed_reconfig = conf->delayed_reconfig; -- -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [dm-devel] [PATCH] multipathd: fix missing persistent reseravtion for active path 2021-09-10 12:31 [dm-devel] [PATCH] multipathd: fix missing persistent reseravtion for active path lixiaokeng @ 2021-09-10 16:17 ` Martin Wilck 2021-09-11 3:28 ` lixiaokeng 0 siblings, 1 reply; 5+ messages in thread From: Martin Wilck @ 2021-09-10 16:17 UTC (permalink / raw) To: lixiaokeng, Christophe Varoqui, Benjamin Marzinski, dm-devel mailing list Cc: linfeilong, liuzhiqiang (I) Hello lixiaokeng, thanks for your patch. On Fri, 2021-09-10 at 20:31 +0800, lixiaokeng wrote: > There are two paths(sucu as sda and adb) for one LUN. The two > paths log in, but before the two uevents have been processed > (for example there are many uevent), users use multipathd add > path /dev/sda to cause mpatha and use mpathpersist -o -I to > register prkey for mpatha. The add map uevent is after add path > uevent, the the uevent(add sdb) will delay and missing persistent > reseravtion check. > > Here, we add persistent reseravtion check in ev_add_map if > mpp->wait_for_udev > 1. > > Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com> > --- > multipathd/main.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 3aff241d..ef456c34 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -706,6 +706,8 @@ ev_add_map (char * dev, const char * alias, > struct vectors * vecs) > struct multipath * mpp; > int delayed_reconfig, reassign_maps; > struct config *conf; > + struct path *pp; > + int i; > > if (dm_is_mpath(alias) != 1) { > condlog(4, "%s: not a multipath map", alias); > @@ -721,6 +723,13 @@ ev_add_map (char * dev, const char * alias, > struct vectors * vecs) > if (update_map(mpp, vecs, 0)) > /* setup multipathd removed the map > */ > return 1; Should you make this conditional on mpp->prflag, perhaps? After all, sda has already been added, sp prflag should should be set if that was successful. I think this should rather be added to update_map(). Ben? > + > + vector_foreach_slot(mpp->paths, pp, i) { > + if ((pp->state == PATH_UP) || (pp- > >state == PATH_GHOST)) { > + /* persistent reseravtion > check*/ Typo above. > + mpath_pr_event_handle(pp); > + } > + } > } > conf = get_multipath_config(); > delayed_reconfig = conf->delayed_reconfig; Thanks, Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dm-devel] [PATCH] multipathd: fix missing persistent reseravtion for active path 2021-09-10 16:17 ` Martin Wilck @ 2021-09-11 3:28 ` lixiaokeng 2021-09-11 19:13 ` Martin Wilck 0 siblings, 1 reply; 5+ messages in thread From: lixiaokeng @ 2021-09-11 3:28 UTC (permalink / raw) To: Martin Wilck, Christophe Varoqui, Benjamin Marzinski, dm-devel mailing list Cc: linfeilong, liuzhiqiang (I) On 2021/9/11 0:17, Martin Wilck wrote: > Hello lixiaokeng, > > thanks for your patch. > > On Fri, 2021-09-10 at 20:31 +0800, lixiaokeng wrote: >> There are two paths(sucu as sda and adb) for one LUN. The two >> paths log in, but before the two uevents have been processed >> (for example there are many uevent), users use multipathd add >> path /dev/sda to cause mpatha and use mpathpersist -o -I to >> register prkey for mpatha. The add map uevent is after add path >> uevent, the the uevent(add sdb) will delay and missing persistent >> reseravtion check. >> >> Here, we add persistent reseravtion check in ev_add_map if >> mpp->wait_for_udev > 1. >> >> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com> >> --- >> multipathd/main.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/multipathd/main.c b/multipathd/main.c >> index 3aff241d..ef456c34 100644 >> --- a/multipathd/main.c >> +++ b/multipathd/main.c >> @@ -706,6 +706,8 @@ ev_add_map (char * dev, const char * alias, >> struct vectors * vecs) >> struct multipath * mpp; >> int delayed_reconfig, reassign_maps; >> struct config *conf; >> + struct path *pp; >> + int i; >> >> if (dm_is_mpath(alias) != 1) { >> condlog(4, "%s: not a multipath map", alias); >> @@ -721,6 +723,13 @@ ev_add_map (char * dev, const char * alias, >> struct vectors * vecs) >> if (update_map(mpp, vecs, 0)) >> /* setup multipathd removed the map >> */ >> return 1; > > Should you make this conditional on mpp->prflag, perhaps? > After all, sda has already been added, sp prflag should should be set > if that was successful. > Hi Martin: Thanks for your reply. I will add conditional on mpp->prflag. > I think this should rather be added to update_map(). Ben? I have considered putting it after adopt_paths() in update_map(). But I'm not sure which one is better. If you and Ben think adding to update_map() is better, I will move it to there. Here is anotherthing. If two new paths(sdc) for maptha (with sda sdb path) log in. mpathpersist -o -I for mpatha, the sda and sdb will be registered. Before update_prflag() and update_prkey() in do_mpath_persistent_reserve_out, the uevent (add sdc) is finshed, then sdc will missing registering. This is just my theoretical analysis. I'm not sure if this is a real problem. > > >> + >> + vector_foreach_slot(mpp->paths, pp, i) { >> + if ((pp->state == PATH_UP) || (pp- >>> state == PATH_GHOST)) { >> + /* persistent reseravtion >> check*/ > > Typo above. > >> + mpath_pr_event_handle(pp); >> + } >> + } >> } >> conf = get_multipath_config(); >> delayed_reconfig = conf->delayed_reconfig; > > > Thanks, > Martin > > . > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dm-devel] [PATCH] multipathd: fix missing persistent reseravtion for active path 2021-09-11 3:28 ` lixiaokeng @ 2021-09-11 19:13 ` Martin Wilck 2021-09-13 15:23 ` Benjamin Marzinski 0 siblings, 1 reply; 5+ messages in thread From: Martin Wilck @ 2021-09-11 19:13 UTC (permalink / raw) To: lixiaokeng, Christophe Varoqui, Benjamin Marzinski, dm-devel mailing list Cc: linfeilong, liuzhiqiang (I) On Sat, 2021-09-11 at 11:28 +0800, lixiaokeng wrote: > > > On 2021/9/11 0:17, Martin Wilck wrote: > > Hello lixiaokeng, > > > > thanks for your patch. > > > > On Fri, 2021-09-10 at 20:31 +0800, lixiaokeng wrote: > > > There are two paths(sucu as sda and adb) for one LUN. The two > > > paths log in, but before the two uevents have been processed > > > (for example there are many uevent), users use multipathd add > > > path /dev/sda to cause mpatha and use mpathpersist -o -I to > > > register prkey for mpatha. The add map uevent is after add path > > > uevent, the the uevent(add sdb) will delay and missing persistent > > > reseravtion check. > > > > > > Here, we add persistent reseravtion check in ev_add_map if > > > mpp->wait_for_udev > 1. > > > > > > Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com> > > > --- > > > multipathd/main.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/multipathd/main.c b/multipathd/main.c > > > index 3aff241d..ef456c34 100644 > > > --- a/multipathd/main.c > > > +++ b/multipathd/main.c > > > @@ -706,6 +706,8 @@ ev_add_map (char * dev, const char * alias, > > > struct vectors * vecs) > > > struct multipath * mpp; > > > int delayed_reconfig, reassign_maps; > > > struct config *conf; > > > + struct path *pp; > > > + int i; > > > > > > if (dm_is_mpath(alias) != 1) { > > > condlog(4, "%s: not a multipath map", alias); > > > @@ -721,6 +723,13 @@ ev_add_map (char * dev, const char * alias, > > > struct vectors * vecs) > > > if (update_map(mpp, vecs, 0)) > > > /* setup multipathd removed the > > > map > > > */ > > > return 1; > > > > Should you make this conditional on mpp->prflag, perhaps? > > After all, sda has already been added, sp prflag should should be > > set > > if that was successful. > > > Hi Martin: > > Thanks for your reply. I will add conditional on mpp->prflag. > > > I think this should rather be added to update_map(). Ben? > > I have considered putting it after adopt_paths() in update_map(). > But I'm not sure which one is better. If you and Ben think adding > to update_map() is better, I will move it to there. update_map() is called from other places like missing_uev_wait_tick(), where it would also make sense to try and register PR keys. AFAICS this holds for all callers. We may want to set flags on the paths, to track whether a path is already registered, so that we don't have to repeat that operation for already registered paths. > > Here is anotherthing. If two new paths(sdc) for maptha > (with sda sdb path) log in. mpathpersist -o -I for mpatha, the > sda and sdb will be registered. Before update_prflag() and > update_prkey() in do_mpath_persistent_reserve_out, the uevent > (add sdc) is finshed, then sdc will missing registering. > > This is just my theoretical analysis. I'm not sure if this is > a real problem. I think this could happen, yes. It makes me wonder again why we we never try to register keys from multipathd itself, except when paths are added to an already registered map. IMO we could change the logic such that if a registration_key was specified for a given map, multipathd could register that key, even if it was not found already registered on the storage. Multipathd should of course not try to create a reservation. Martin > > > > > > > > + > > > + vector_foreach_slot(mpp->paths, pp, i) { > > > + if ((pp->state == PATH_UP) || > > > (pp- > > > > state == PATH_GHOST)) { > > > + /* persistent reseravtion > > > check*/ > > > > Typo above. > > > > > + mpath_pr_event_handle(pp) > > > ; > > > + } > > > + } > > > } > > > conf = get_multipath_config(); > > > delayed_reconfig = conf->delayed_reconfig; > > > > > > Thanks, > > Martin > > > > . > > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dm-devel] [PATCH] multipathd: fix missing persistent reseravtion for active path 2021-09-11 19:13 ` Martin Wilck @ 2021-09-13 15:23 ` Benjamin Marzinski 0 siblings, 0 replies; 5+ messages in thread From: Benjamin Marzinski @ 2021-09-13 15:23 UTC (permalink / raw) To: Martin Wilck Cc: lixiaokeng, dm-devel mailing list, liuzhiqiang (I), linfeilong On Sat, Sep 11, 2021 at 09:13:27PM +0200, Martin Wilck wrote: > On Sat, 2021-09-11 at 11:28 +0800, lixiaokeng wrote: > > > > > > On 2021/9/11 0:17, Martin Wilck wrote: > > > Hello lixiaokeng, > > > > > > thanks for your patch. > > > > > > On Fri, 2021-09-10 at 20:31 +0800, lixiaokeng wrote: > > > > There are two paths(sucu as sda and adb) for one LUN. The two > > > > paths log in, but before the two uevents have been processed > > > > (for example there are many uevent), users use multipathd add > > > > path /dev/sda to cause mpatha and use mpathpersist -o -I to > > > > register prkey for mpatha. The add map uevent is after add path > > > > uevent, the the uevent(add sdb) will delay and missing persistent > > > > reseravtion check. > > > > > > > > Here, we add persistent reseravtion check in ev_add_map if > > > > mpp->wait_for_udev > 1. > > > > > > > > Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com> > > > > --- > > > > multipathd/main.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/multipathd/main.c b/multipathd/main.c > > > > index 3aff241d..ef456c34 100644 > > > > --- a/multipathd/main.c > > > > +++ b/multipathd/main.c > > > > @@ -706,6 +706,8 @@ ev_add_map (char * dev, const char * alias, > > > > struct vectors * vecs) > > > > struct multipath * mpp; > > > > int delayed_reconfig, reassign_maps; > > > > struct config *conf; > > > > + struct path *pp; > > > > + int i; > > > > > > > > if (dm_is_mpath(alias) != 1) { > > > > condlog(4, "%s: not a multipath map", alias); > > > > @@ -721,6 +723,13 @@ ev_add_map (char * dev, const char * alias, > > > > struct vectors * vecs) > > > > if (update_map(mpp, vecs, 0)) > > > > /* setup multipathd removed the > > > > map > > > > */ > > > > return 1; > > > > > > Should you make this conditional on mpp->prflag, perhaps? > > > After all, sda has already been added, sp prflag should should be > > > set > > > if that was successful. > > > > > Hi Martin: > > > > Thanks for your reply. I will add conditional on mpp->prflag. > > > > > I think this should rather be added to update_map(). Ben? > > > > I have considered putting it after adopt_paths() in update_map(). > > But I'm not sure which one is better. If you and Ben think adding > > to update_map() is better, I will move it to there. > > update_map() is called from other places like missing_uev_wait_tick(), > where it would also make sense to try and register PR keys. AFAICS this > holds for all callers. > > We may want to set flags on the paths, to track whether a path is > already registered, so that we don't have to repeat that operation for > already registered paths. > > > > > Here is anotherthing. If two new paths(sdc) for maptha > > (with sda sdb path) log in. mpathpersist -o -I for mpatha, the > > sda and sdb will be registered. Before update_prflag() and > > update_prkey() in do_mpath_persistent_reserve_out, the uevent > > (add sdc) is finshed, then sdc will missing registering. > > > > This is just my theoretical analysis. I'm not sure if this is > > a real problem. > > I think this could happen, yes. It makes me wonder again why we > we never try to register keys from multipathd itself, except when > paths are added to an already registered map. IMO we could change > the logic such that if a registration_key was specified for a given > map, multipathd could register that key, even if it was not found > already registered on the storage. Multipathd should of course not > try to create a reservation. > Yeah. This is a real problem. I wonder if mpathpersist should try delegating work to multipathd, like multipath does. I don't use persistent reservations enough to know if it would be problematic to have multipathd automatically register paths. It would be different from how persistent reservations work on scsi devices, and the original idea for mpathpersist was to make persistent reservations on a multipath device work just like they do on scsi devices (where devices appear with no registrations, and you must register them). -Ben > Martin > > > > > > > > > > > > > + > > > > + vector_foreach_slot(mpp->paths, pp, i) { > > > > + if ((pp->state == PATH_UP) || > > > > (pp- > > > > > state == PATH_GHOST)) { > > > > + /* persistent reseravtion > > > > check*/ > > > > > > Typo above. > > > > > > > + mpath_pr_event_handle(pp) > > > > ; > > > > + } > > > > + } > > > > } > > > > conf = get_multipath_config(); > > > > delayed_reconfig = conf->delayed_reconfig; > > > > > > > > > Thanks, > > > Martin > > > > > > . > > > > > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-13 15:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-10 12:31 [dm-devel] [PATCH] multipathd: fix missing persistent reseravtion for active path lixiaokeng 2021-09-10 16:17 ` Martin Wilck 2021-09-11 3:28 ` lixiaokeng 2021-09-11 19:13 ` Martin Wilck 2021-09-13 15:23 ` Benjamin Marzinski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.