| Message ID | 20180220132658.22295-20-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
On Tue, Feb 20, 2018 at 02:26:57PM +0100, Martin Wilck wrote: > Call into the foreign library code when paths are discovered, uevents > are received, and in the checker loop. Furthermore, use the foreign > code to print information in the "multipathd show paths", "multipathd > show maps", and "multipathd show topology" client commands. > > We don't support foreign data in the individual "show map" and "show path" > commands, and neither in the "json" commands. The former is a deliberate > decision, the latter could be added if desired. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/discovery.c | 4 +++- > multipathd/cli_handlers.c | 39 ++++++++++++++++++++++++++++++++++----- > multipathd/main.c | 43 ++++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 77 insertions(+), 9 deletions(-) > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > index 645224c1029c..45a4d8378893 100644 > --- a/libmultipath/discovery.c > +++ b/libmultipath/discovery.c > @@ -31,6 +31,7 @@ > #include "prio.h" > #include "defaults.h" > #include "prioritizers/alua_rtpg.h" > +#include "foreign.h" > > int > alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, > @@ -1909,7 +1910,8 @@ int pathinfo(struct path *pp, struct config *conf, int mask) > * limited by DI_BLACKLIST and occurs before this debug > * message with the mask value. > */ > - if (pp->udev && filter_property(conf, pp->udev) > 0) > + if (pp->udev && (is_claimed_by_foreign(pp->udev) || > + filter_property(conf, pp->udev) > 0)) > return PATHINFO_SKIPPED; > > if (filter_devnode(conf->blist_devnode, > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c > index 78f2a12bc2f8..c0ae54aae841 100644 > --- a/multipathd/cli_handlers.c > +++ b/multipathd/cli_handlers.c > @@ -27,6 +27,7 @@ > #include "main.h" > #include "cli.h" > #include "uevent.h" > +#include "foreign.h" > > int > show_paths (char ** r, int * len, struct vectors * vecs, char * style, > @@ -35,11 +36,13 @@ show_paths (char ** r, int * len, struct vectors * vecs, char * style, > int i; > struct path * pp; > char * c; > - char * reply; > + char * reply, * header; > unsigned int maxlen = INITIAL_REPLY_LEN; > int again = 1; > > get_path_layout(vecs->pathvec, 1); > + foreign_path_layout(); > + > reply = MALLOC(maxlen); > > while (again) { > @@ -48,18 +51,29 @@ show_paths (char ** r, int * len, struct vectors * vecs, char * style, > > c = reply; > > - if (pretty && VECTOR_SIZE(vecs->pathvec) > 0) > + if (pretty) > c += snprint_path_header(c, reply + maxlen - c, > style); > + header = c; > > vector_foreach_slot(vecs->pathvec, pp, i) > c += snprint_path(c, reply + maxlen - c, > style, pp, pretty); > > + c += snprint_foreign_paths(c, reply + maxlen - c, > + style, pretty); > + > again = ((c - reply) == (maxlen - 1)); > > REALLOC_REPLY(reply, again, maxlen); > } > + > + if (pretty && c == header) { > + /* No output - clear header */ > + *reply = '\0'; > + c = reply; > + } > + > *r = reply; > *len = (int)(c - reply + 1); > return 0; > @@ -134,6 +148,8 @@ show_maps_topology (char ** r, int * len, struct vectors * vecs) > int again = 1; > > get_path_layout(vecs->pathvec, 0); > + foreign_path_layout(); > + > reply = MALLOC(maxlen); > > while (again) { > @@ -150,11 +166,13 @@ show_maps_topology (char ** r, int * len, struct vectors * vecs) > c += snprint_multipath_topology(c, reply + maxlen - c, > mpp, 2); > } > + c += snprint_foreign_topology(c, reply + maxlen - c, 2); > > again = ((c - reply) == (maxlen - 1)); > > REALLOC_REPLY(reply, again, maxlen); > } > + > *r = reply; > *len = (int)(c - reply + 1); > return 0; > @@ -499,12 +517,14 @@ show_maps (char ** r, int *len, struct vectors * vecs, char * style, > { > int i; > struct multipath * mpp; > - char * c; > + char * c, *header; > char * reply; > unsigned int maxlen = INITIAL_REPLY_LEN; > int again = 1; > > get_multipath_layout(vecs->mpvec, 1); > + foreign_multipath_layout(); > + > reply = MALLOC(maxlen); > > while (again) { > @@ -512,9 +532,10 @@ show_maps (char ** r, int *len, struct vectors * vecs, char * style, > return 1; > > c = reply; > - if (pretty && VECTOR_SIZE(vecs->mpvec) > 0) > + if (pretty) > c += snprint_multipath_header(c, reply + maxlen - c, > style); > + header = c; > > vector_foreach_slot(vecs->mpvec, mpp, i) { > if (update_multipath(vecs, mpp->alias, 0)) { > @@ -523,12 +544,20 @@ show_maps (char ** r, int *len, struct vectors * vecs, char * style, > } > c += snprint_multipath(c, reply + maxlen - c, > style, mpp, pretty); > - } > > + } > + c += snprint_foreign_multipaths(c, reply + maxlen - c, > + style, pretty); > again = ((c - reply) == (maxlen - 1)); > > REALLOC_REPLY(reply, again, maxlen); > } > + > + if (pretty && c == header) { > + /* No output - clear header */ > + *reply = '\0'; > + c = reply; > + } > *r = reply; > *len = (int)(c - reply + 1); > return 0; > diff --git a/multipathd/main.c b/multipathd/main.c > index b900bb3ec2e3..e1d98861a841 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -84,6 +84,7 @@ static int use_watchdog; > #include "waiter.h" > #include "io_err_stat.h" > #include "wwids.h" > +#include "foreign.h" > #include "../third-party/valgrind/drd.h" > > #define FILE_NAME_SIZE 256 > @@ -699,6 +700,15 @@ rescan: > mpp->action = ACT_RELOAD; > extract_hwe_from_path(mpp); > } else { > + switch (add_foreign(pp->udev)) { There's a rather convoluted issue here. This function will eventually call _find_slaves() in the nvme code, which calls glob(), which isn't strictly multithreading safe for a number of reasons. The only one that effects multipathd is its use of SIGALRM. Now assuming signal processing worked correctly in multipathd (it doesn't. commit 534ec4c broke it. I'm planning on fixing that shortly) there would still be a problem, because of the strict_timing option. strict_timing calls setitimer(), which sets an alarm in checkerloop, without any locking. strict_timing also messes with any call to lock_file(). lock_file() and _find_slaves() will never interfere with eachother, since both are only called with the vecs lock held (I think. See below). strict_timing probably needs to get reimplemented using nanosleep() or some other non-signal method, so that there aren't two threads setting alarms and waiting for SIGARLM at the same time. > + case FOREIGN_CLAIMED: > + case FOREIGN_OK: > + orphan_path(pp, "claimed by foreign library"); > + return 0; > + default: > + break; Is there a purpose to this break? > + } > + > if (!should_multipath(pp, vecs->pathvec)) { > orphan_path(pp, "only one path"); > return 0; > @@ -798,6 +808,8 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs, int need_do_map) > int ret; > > condlog(2, "%s: remove path (uevent)", uev->kernel); > + delete_foreign(uev->udev); > + > pthread_cleanup_push(cleanup_lock, &vecs->lock); > lock(&vecs->lock); > pthread_testcancel(); > @@ -917,12 +929,27 @@ fail: > static int > uev_update_path (struct uevent *uev, struct vectors * vecs) > { > - int ro, retval = 0; > + int ro, retval = 0, rc; > struct path * pp; > struct config *conf; > int disable_changed_wwids; > int needs_reinit = 0; > > + switch ((rc = change_foreign(uev->udev))) { > + case FOREIGN_OK: > + /* known foreign path, ignore event */ > + return 0; > + case FOREIGN_IGNORED: > + break; > + case FOREIGN_ERR: > + condlog(3, "%s: error in change_foreign", __func__); > + break; > + default: > + condlog(1, "%s: return code %d of change_forein is unsupported", > + __func__, rc); > + break; > + } > + > conf = get_multipath_config(); > disable_changed_wwids = conf->disable_changed_wwids; > put_multipath_config(conf); > @@ -1122,8 +1149,13 @@ uev_trigger (struct uevent * uev, void * trigger_data) > * are not fully initialised then. > */ > if (!strncmp(uev->kernel, "dm-", 3)) { > - if (!uevent_is_mpath(uev)) > + if (!uevent_is_mpath(uev)) { > + if (!strncmp(uev->action, "change", 6)) > + (void)add_foreign(uev->udev); > + else if (!strncmp(uev->action, "remove", 6)) > + (void)delete_foreign(uev->udev); > goto out; > + } I'm confused. Should foreign maps ever have uev->kernel set to "dm-"? If this is correct, these calls probably need to get called with the vecs lock held, otherwise they can interfere with calls to lock_file(). -Ben > if (!strncmp(uev->action, "change", 6)) { > r = uev_add_map(uev, vecs); > > @@ -1932,7 +1964,7 @@ checkerloop (void *ap) > diff_time.tv_sec); > } > } > - > + check_foreign(); > post_config_state(DAEMON_IDLE); > conf = get_multipath_config(); > strict_timing = conf->strict_timing; > @@ -2121,6 +2153,7 @@ reconfigure (struct vectors * vecs) > > free_pathvec(vecs->pathvec, FREE_PATHS); > vecs->pathvec = NULL; > + delete_all_foreign(); > > /* Re-read any timezone changes */ > tzset(); > @@ -2372,6 +2405,9 @@ child (void * param) > condlog(0, "failed to initialize prioritizers"); > goto failed; > } > + /* Failing this is non-fatal */ > + > + init_foreign(conf->multipath_dir); > > setlogmask(LOG_UPTO(conf->verbosity + 3)); > > @@ -2529,6 +2565,7 @@ child (void * param) > FREE(vecs); > vecs = NULL; > > + cleanup_foreign(); > cleanup_checkers(); > cleanup_prio(); > > -- > 2.16.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, 2018-02-28 at 23:13 -0600, Benjamin Marzinski wrote: > On Tue, Feb 20, 2018 at 02:26:57PM +0100, Martin Wilck wrote: > > Call into the foreign library code when paths are discovered, > > uevents > > are received, and in the checker loop. Furthermore, use the foreign > > code to print information in the "multipathd show paths", > > "multipathd > > show maps", and "multipathd show topology" client commands. > > > > We don't support foreign data in the individual "show map" and > > "show path" > > commands, and neither in the "json" commands. The former is a > > deliberate > > decision, the latter could be added if desired. > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > --- > > libmultipath/discovery.c | 4 +++- > > multipathd/cli_handlers.c | 39 ++++++++++++++++++++++++++++++++++- > > ---- > > multipathd/main.c | 43 > > ++++++++++++++++++++++++++++++++++++++++--- > > 3 files changed, 77 insertions(+), 9 deletions(-) > > > > diff --git a/multipathd/main.c b/multipathd/main.c > > index b900bb3ec2e3..e1d98861a841 100644 > > --- a/multipathd/main.c > > +++ b/multipathd/main.c > > @@ -84,6 +84,7 @@ static int use_watchdog; > > #include "waiter.h" > > #include "io_err_stat.h" > > #include "wwids.h" > > +#include "foreign.h" > > #include "../third-party/valgrind/drd.h" > > > > #define FILE_NAME_SIZE 256 > > @@ -699,6 +700,15 @@ rescan: > > mpp->action = ACT_RELOAD; > > extract_hwe_from_path(mpp); > > } else { > > + switch (add_foreign(pp->udev)) { > > There's a rather convoluted issue here. This function will eventually > call _find_slaves() in the nvme code, which calls glob(), which isn't > strictly multithreading safe for a number of reasons. The only one > that > effects multipathd is its use of SIGALRM. Now assuming signal > processing > worked correctly in multipathd (it doesn't. commit 534ec4c broke it. > I'm > planning on fixing that shortly) there would still be a problem, > because > of the strict_timing option. strict_timing calls setitimer(), which > sets an alarm in checkerloop, without any locking. strict_timing > also > messes with any call to lock_file(). lock_file() and _find_slaves() > will > never interfere with eachother, since both are only called with the > vecs > lock held (I think. See below). strict_timing probably needs to get > reimplemented using nanosleep() or some other non-signal method, so > that > there aren't two threads setting alarms and waiting for SIGARLM at > the > same time. Thanks for spotting this. It was lazyness on my side to use glob(). I will reimplement this using safer, more elementary functions. > > > + case FOREIGN_CLAIMED: > > + case FOREIGN_OK: > > + orphan_path(pp, "claimed by foreign > > library"); > > + return 0; > > + default: > > + break; > > Is there a purpose to this break? I thought this was common style. You find it the kernel's CodingStyle document, too. I have no issues with removing it if you insist. > > conf = get_multipath_config(); > > disable_changed_wwids = conf->disable_changed_wwids; > > put_multipath_config(conf); > > @@ -1122,8 +1149,13 @@ uev_trigger (struct uevent * uev, void * > > trigger_data) > > * are not fully initialised then. > > */ > > if (!strncmp(uev->kernel, "dm-", 3)) { > > - if (!uevent_is_mpath(uev)) > > + if (!uevent_is_mpath(uev)) { > > + if (!strncmp(uev->action, "change", 6)) > > + (void)add_foreign(uev->udev); > > + else if (!strncmp(uev->action, "remove", > > 6)) > > + (void)delete_foreign(uev->udev); > > goto out; > > + } > > I'm confused. Should foreign maps ever have uev->kernel set to "dm-"? Although I don't see a likely candidate, I didn't want to exclude it in general. > If this is correct, these calls probably need to get called with the > vecs lock held, otherwise they can interfere with calls to > lock_file(). Please explain. These devices are ignored by multipath-tools, because !uevent_is_mpath(uev), so they won't be part of any of their data structures. And lock_file() is only about fcntl, what does it have to do with this code? Regards, Martin
On Fri, Mar 02, 2018 at 06:04:29PM +0100, Martin Wilck wrote: > On Wed, 2018-02-28 at 23:13 -0600, Benjamin Marzinski wrote: > > On Tue, Feb 20, 2018 at 02:26:57PM +0100, Martin Wilck wrote: > > > Call into the foreign library code when paths are discovered, > > > uevents > > > are received, and in the checker loop. Furthermore, use the foreign > > > code to print information in the "multipathd show paths", > > > "multipathd > > > show maps", and "multipathd show topology" client commands. > > > > > > We don't support foreign data in the individual "show map" and > > > "show path" > > > commands, and neither in the "json" commands. The former is a > > > deliberate > > > decision, the latter could be added if desired. > > > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > > --- > > > libmultipath/discovery.c | 4 +++- > > > multipathd/cli_handlers.c | 39 ++++++++++++++++++++++++++++++++++- > > > ---- > > > multipathd/main.c | 43 > > > ++++++++++++++++++++++++++++++++++++++++--- > > > 3 files changed, 77 insertions(+), 9 deletions(-) > > > > > > diff --git a/multipathd/main.c b/multipathd/main.c > > > index b900bb3ec2e3..e1d98861a841 100644 > > > --- a/multipathd/main.c > > > +++ b/multipathd/main.c > > > @@ -84,6 +84,7 @@ static int use_watchdog; > > > #include "waiter.h" > > > #include "io_err_stat.h" > > > #include "wwids.h" > > > +#include "foreign.h" > > > #include "../third-party/valgrind/drd.h" > > > > > > #define FILE_NAME_SIZE 256 > > > @@ -699,6 +700,15 @@ rescan: > > > mpp->action = ACT_RELOAD; > > > extract_hwe_from_path(mpp); > > > } else { > > > + switch (add_foreign(pp->udev)) { > > > > There's a rather convoluted issue here. This function will eventually > > call _find_slaves() in the nvme code, which calls glob(), which isn't > > strictly multithreading safe for a number of reasons. The only one > > that > > effects multipathd is its use of SIGALRM. Now assuming signal > > processing > > worked correctly in multipathd (it doesn't. commit 534ec4c broke it. > > I'm > > planning on fixing that shortly) there would still be a problem, > > because > > of the strict_timing option. strict_timing calls setitimer(), which > > sets an alarm in checkerloop, without any locking. strict_timing > > also > > messes with any call to lock_file(). lock_file() and _find_slaves() > > will > > never interfere with eachother, since both are only called with the > > vecs > > lock held (I think. See below). strict_timing probably needs to get > > reimplemented using nanosleep() or some other non-signal method, so > > that > > there aren't two threads setting alarms and waiting for SIGARLM at > > the > > same time. > > Thanks for spotting this. It was lazyness on my side to use glob(). I > will reimplement this using safer, more elementary functions. Sure. But lock_file() already needs this fixed to work correctly, and once it is, I don't see a problem calling glob() here. > > > > > + case FOREIGN_CLAIMED: > > > + case FOREIGN_OK: > > > + orphan_path(pp, "claimed by foreign > > > library"); > > > + return 0; > > > + default: > > > + break; > > > > Is there a purpose to this break? > > I thought this was common style. You find it the kernel's CodingStyle > document, too. I have no issues with removing it if you insist. I didn't realize that. I'm fine with keeping it. It's not confusing. I just thought it was an accident. > > > conf = get_multipath_config(); > > > disable_changed_wwids = conf->disable_changed_wwids; > > > put_multipath_config(conf); > > > @@ -1122,8 +1149,13 @@ uev_trigger (struct uevent * uev, void * > > > trigger_data) > > > * are not fully initialised then. > > > */ > > > if (!strncmp(uev->kernel, "dm-", 3)) { > > > - if (!uevent_is_mpath(uev)) > > > + if (!uevent_is_mpath(uev)) { > > > + if (!strncmp(uev->action, "change", 6)) > > > + (void)add_foreign(uev->udev); > > > + else if (!strncmp(uev->action, "remove", > > > 6)) > > > + (void)delete_foreign(uev->udev); > > > goto out; > > > + } > > > > I'm confused. Should foreign maps ever have uev->kernel set to "dm-"? > > Although I don't see a likely candidate, I didn't want to exclude it in > general. > > > If this is correct, these calls probably need to get called with the > > vecs lock held, otherwise they can interfere with calls to > > lock_file(). > > Please explain. These devices are ignored by multipath-tools, because > !uevent_is_mpath(uev), so they won't be part of any of their data > structures. And lock_file() is only about fcntl, what does it have to > do with this code? add_foreign will eventually call glob(). Like I said above, as long as all the functions that use SIGALRM are restricted from being called at the same time, they can't mess with each other's timeout, and everything is fine. But if you call add_foreign here without holding the vecs lock, then there would be nothing to keep it and lock_file() from being called at the same time. This is yet another example of the headache involved in removing the vecs lock. It is protecting a lot of stuff that's really easy to overlook (I'm not saying that's a good thing.. Just that it's a thing). -Ben > Regards, > Martin > > -- > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, 2018-02-28 at 23:13 -0600, Benjamin Marzinski wrote: > > effects multipathd is its use of SIGALRM. Now assuming signal > processing > worked correctly in multipathd (it doesn't. commit 534ec4c broke it. > I'm > planning on fixing that shortly) Details about this breakage, and your idea how to fix it, would be highly appreciated. Martin
On Fri, Mar 02, 2018 at 08:19:39PM +0100, Martin Wilck wrote: > On Wed, 2018-02-28 at 23:13 -0600, Benjamin Marzinski wrote: > > > > effects multipathd is its use of SIGALRM. Now assuming signal > > processing > > worked correctly in multipathd (it doesn't. commit 534ec4c broke it. > > I'm > > planning on fixing that shortly) > > Details about this breakage, and your idea how to fix it, would be > highly appreciated. I plan on fixing the issue using exactly the method that commit 534ec4c says it is using. :) I cleaned up the signal handling years ago with commit 5ee9f71, which blocked all the then-used signals for all threads except uxlsnr, which is who calls handle_signals. Other threads unblocked signals only as necessary. I'm not sure what issue caused Bart to write 534ec4c. Perhaps an earlier commit actually broke the signal handling, and this was just the commit that git blame pointed to. But in any case, after commit 534ec4c, we are only blocking SIGUSR2 on all threads, which is the only signal we don't need to block. This means that, for instance, we have no idea which thread a SIGALRM is going to, because any of them could receive it. It is safe to unblock SIGUSR2 on multiple threads at once, since (I hope) the only things that send it are stop_io_err_stat_thread() and stop_waiter_thread(), and both of those send it via pthread_kill, which will make sure it goes to the proper thread. -Ben > > Martin > > -- > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, 2018-02-28 at 23:13 -0600, Benjamin Marzinski wrote: > Now assuming signal processing worked correctly in multipathd (it doesn't. > commit 534ec4c broke it. I'm planning on fixing that shortly) there would > still be a problem, because of the strict_timing option. Although I'm not convinced that there is anything wrong with commit 534ec4c: if you want to change signal handling in multipathd please retest multipathd with Valgrind. Although Valgrind's signal handling is POSIX compliant it handles signal delivery different than the Linux kernel. Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index 645224c1029c..45a4d8378893 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -31,6 +31,7 @@ #include "prio.h" #include "defaults.h" #include "prioritizers/alua_rtpg.h" +#include "foreign.h" int alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice, @@ -1909,7 +1910,8 @@ int pathinfo(struct path *pp, struct config *conf, int mask) * limited by DI_BLACKLIST and occurs before this debug * message with the mask value. */ - if (pp->udev && filter_property(conf, pp->udev) > 0) + if (pp->udev && (is_claimed_by_foreign(pp->udev) || + filter_property(conf, pp->udev) > 0)) return PATHINFO_SKIPPED; if (filter_devnode(conf->blist_devnode, diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c index 78f2a12bc2f8..c0ae54aae841 100644 --- a/multipathd/cli_handlers.c +++ b/multipathd/cli_handlers.c @@ -27,6 +27,7 @@ #include "main.h" #include "cli.h" #include "uevent.h" +#include "foreign.h" int show_paths (char ** r, int * len, struct vectors * vecs, char * style, @@ -35,11 +36,13 @@ show_paths (char ** r, int * len, struct vectors * vecs, char * style, int i; struct path * pp; char * c; - char * reply; + char * reply, * header; unsigned int maxlen = INITIAL_REPLY_LEN; int again = 1; get_path_layout(vecs->pathvec, 1); + foreign_path_layout(); + reply = MALLOC(maxlen); while (again) { @@ -48,18 +51,29 @@ show_paths (char ** r, int * len, struct vectors * vecs, char * style, c = reply; - if (pretty && VECTOR_SIZE(vecs->pathvec) > 0) + if (pretty) c += snprint_path_header(c, reply + maxlen - c, style); + header = c; vector_foreach_slot(vecs->pathvec, pp, i) c += snprint_path(c, reply + maxlen - c, style, pp, pretty); + c += snprint_foreign_paths(c, reply + maxlen - c, + style, pretty); + again = ((c - reply) == (maxlen - 1)); REALLOC_REPLY(reply, again, maxlen); } + + if (pretty && c == header) { + /* No output - clear header */ + *reply = '\0'; + c = reply; + } + *r = reply; *len = (int)(c - reply + 1); return 0; @@ -134,6 +148,8 @@ show_maps_topology (char ** r, int * len, struct vectors * vecs) int again = 1; get_path_layout(vecs->pathvec, 0); + foreign_path_layout(); + reply = MALLOC(maxlen); while (again) { @@ -150,11 +166,13 @@ show_maps_topology (char ** r, int * len, struct vectors * vecs) c += snprint_multipath_topology(c, reply + maxlen - c, mpp, 2); } + c += snprint_foreign_topology(c, reply + maxlen - c, 2); again = ((c - reply) == (maxlen - 1)); REALLOC_REPLY(reply, again, maxlen); } + *r = reply; *len = (int)(c - reply + 1); return 0; @@ -499,12 +517,14 @@ show_maps (char ** r, int *len, struct vectors * vecs, char * style, { int i; struct multipath * mpp; - char * c; + char * c, *header; char * reply; unsigned int maxlen = INITIAL_REPLY_LEN; int again = 1; get_multipath_layout(vecs->mpvec, 1); + foreign_multipath_layout(); + reply = MALLOC(maxlen); while (again) { @@ -512,9 +532,10 @@ show_maps (char ** r, int *len, struct vectors * vecs, char * style, return 1; c = reply; - if (pretty && VECTOR_SIZE(vecs->mpvec) > 0) + if (pretty) c += snprint_multipath_header(c, reply + maxlen - c, style); + header = c; vector_foreach_slot(vecs->mpvec, mpp, i) { if (update_multipath(vecs, mpp->alias, 0)) { @@ -523,12 +544,20 @@ show_maps (char ** r, int *len, struct vectors * vecs, char * style, } c += snprint_multipath(c, reply + maxlen - c, style, mpp, pretty); - } + } + c += snprint_foreign_multipaths(c, reply + maxlen - c, + style, pretty); again = ((c - reply) == (maxlen - 1)); REALLOC_REPLY(reply, again, maxlen); } + + if (pretty && c == header) { + /* No output - clear header */ + *reply = '\0'; + c = reply; + } *r = reply; *len = (int)(c - reply + 1); return 0; diff --git a/multipathd/main.c b/multipathd/main.c index b900bb3ec2e3..e1d98861a841 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -84,6 +84,7 @@ static int use_watchdog; #include "waiter.h" #include "io_err_stat.h" #include "wwids.h" +#include "foreign.h" #include "../third-party/valgrind/drd.h" #define FILE_NAME_SIZE 256 @@ -699,6 +700,15 @@ rescan: mpp->action = ACT_RELOAD; extract_hwe_from_path(mpp); } else { + switch (add_foreign(pp->udev)) { + case FOREIGN_CLAIMED: + case FOREIGN_OK: + orphan_path(pp, "claimed by foreign library"); + return 0; + default: + break; + } + if (!should_multipath(pp, vecs->pathvec)) { orphan_path(pp, "only one path"); return 0; @@ -798,6 +808,8 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs, int need_do_map) int ret; condlog(2, "%s: remove path (uevent)", uev->kernel); + delete_foreign(uev->udev); + pthread_cleanup_push(cleanup_lock, &vecs->lock); lock(&vecs->lock); pthread_testcancel(); @@ -917,12 +929,27 @@ fail: static int uev_update_path (struct uevent *uev, struct vectors * vecs) { - int ro, retval = 0; + int ro, retval = 0, rc; struct path * pp; struct config *conf; int disable_changed_wwids; int needs_reinit = 0; + switch ((rc = change_foreign(uev->udev))) { + case FOREIGN_OK: + /* known foreign path, ignore event */ + return 0; + case FOREIGN_IGNORED: + break; + case FOREIGN_ERR: + condlog(3, "%s: error in change_foreign", __func__); + break; + default: + condlog(1, "%s: return code %d of change_forein is unsupported", + __func__, rc); + break; + } + conf = get_multipath_config(); disable_changed_wwids = conf->disable_changed_wwids; put_multipath_config(conf); @@ -1122,8 +1149,13 @@ uev_trigger (struct uevent * uev, void * trigger_data) * are not fully initialised then. */ if (!strncmp(uev->kernel, "dm-", 3)) { - if (!uevent_is_mpath(uev)) + if (!uevent_is_mpath(uev)) { + if (!strncmp(uev->action, "change", 6)) + (void)add_foreign(uev->udev); + else if (!strncmp(uev->action, "remove", 6)) + (void)delete_foreign(uev->udev); goto out; + } if (!strncmp(uev->action, "change", 6)) { r = uev_add_map(uev, vecs); @@ -1932,7 +1964,7 @@ checkerloop (void *ap) diff_time.tv_sec); } } - + check_foreign(); post_config_state(DAEMON_IDLE); conf = get_multipath_config(); strict_timing = conf->strict_timing; @@ -2121,6 +2153,7 @@ reconfigure (struct vectors * vecs) free_pathvec(vecs->pathvec, FREE_PATHS); vecs->pathvec = NULL; + delete_all_foreign(); /* Re-read any timezone changes */ tzset(); @@ -2372,6 +2405,9 @@ child (void * param) condlog(0, "failed to initialize prioritizers"); goto failed; } + /* Failing this is non-fatal */ + + init_foreign(conf->multipath_dir); setlogmask(LOG_UPTO(conf->verbosity + 3)); @@ -2529,6 +2565,7 @@ child (void * param) FREE(vecs); vecs = NULL; + cleanup_foreign(); cleanup_checkers(); cleanup_prio();
Call into the foreign library code when paths are discovered, uevents are received, and in the checker loop. Furthermore, use the foreign code to print information in the "multipathd show paths", "multipathd show maps", and "multipathd show topology" client commands. We don't support foreign data in the individual "show map" and "show path" commands, and neither in the "json" commands. The former is a deliberate decision, the latter could be added if desired. Signed-off-by: Martin Wilck <mwilck@suse.com> --- libmultipath/discovery.c | 4 +++- multipathd/cli_handlers.c | 39 ++++++++++++++++++++++++++++++++++----- multipathd/main.c | 43 ++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 77 insertions(+), 9 deletions(-)