| Message ID | 20200924134054.14632-20-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | libmultipath: improve cleanup on exit | expand |
On Thu, Sep 24, 2020 at 03:40:50PM +0200, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmpathpersist/mpath_persist.c | 2 -- > libmultipath/config.c | 4 ++++ > libmultipath/libmultipath.version | 5 +---- > multipathd/main.c | 3 --- > 4 files changed, 5 insertions(+), 9 deletions(-) > > diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c > index 873b419..af56a95 100644 > --- a/libmpathpersist/mpath_persist.c > +++ b/libmpathpersist/mpath_persist.c > @@ -78,8 +78,6 @@ mpath_lib_init (void) > > static void libmpathpersist_cleanup(void) > { > - cleanup_prio(); > - cleanup_checkers(); > libmultipath_exit(); > dm_lib_exit(); > } > diff --git a/libmultipath/config.c b/libmultipath/config.c > index 8097838..f115ac2 100644 > --- a/libmultipath/config.c > +++ b/libmultipath/config.c > @@ -26,6 +26,7 @@ > #include "devmapper.h" > #include "mpath_cmd.h" > #include "propsel.h" > +#include "foreign.h" > > /* > * We don't support re-initialization after > @@ -65,6 +66,9 @@ int libmultipath_init(void) > static void _libmultipath_exit(void) > { > libmultipath_exit_called = true; > + cleanup_foreign(); I don't really feel too strongly about this, but it seems to me that there is a difference between the checkers and prioritizers, which it seems like most users of libmultipath would want, and the foreign code, which doesn't seem that way. libmpathpersist, for instance, will use the checkers and prioritizers, but not the foreign code. On the other hand, if the caller isn't using the foreign code, then grabbing the lock and checking the foreign pointer shouldn't take much time. -Ben > + cleanup_checkers(); > + cleanup_prio(); > libmp_dm_exit(); > udev_unref(udev); > } > diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version > index 9abdb22..80f1950 100644 > --- a/libmultipath/libmultipath.version > +++ b/libmultipath/libmultipath.version > @@ -1,4 +1,4 @@ > -LIBMULTIPATH_0.8.4.5 { > +LIBMULTIPATH_0.8.4.6 { > global: > /* symbols referenced by multipath and multipathd */ > add_foreign; > @@ -18,10 +18,7 @@ global: > checker_name; > checker_state_name; > check_foreign; > - cleanup_checkers; > - cleanup_foreign; > cleanup_lock; > - cleanup_prio; > close_fd; > coalesce_paths; > convert_dev; > diff --git a/multipathd/main.c b/multipathd/main.c > index e7b479a..efed56e 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -3027,9 +3027,6 @@ static void cleanup_child(void) > { > cleanup_threads(); > cleanup_vecs(); > - cleanup_foreign(); > - cleanup_checkers(); > - cleanup_prio(); > if (poll_dmevents) > cleanup_dmevent_waiter(); > > -- > 2.28.0 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 2020-09-28 at 15:26 -0500, Benjamin Marzinski wrote: > On Thu, Sep 24, 2020 at 03:40:50PM +0200, mwilck@suse.com wrote: > > > > /* > > * We don't support re-initialization after > > @@ -65,6 +66,9 @@ int libmultipath_init(void) > > static void _libmultipath_exit(void) > > { > > libmultipath_exit_called = true; > > + cleanup_foreign(); > > I don't really feel too strongly about this, but it seems to me that > there is a difference between the checkers and prioritizers, which > it seems like most users of libmultipath would want, and the foreign > code, which doesn't seem that way. libmpathpersist, for instance, > will use the checkers and prioritizers, but not the foreign code. > On the other hand, if the caller isn't using the foreign code, > then grabbing the lock and checking the foreign pointer shouldn't > take much time. It would just be a few cycles. I want callers to have to worry about cleanup as little as possible. All else is error-prone IMO, and although I agree that the foreign functions are less important than checkers and prio, I thought it made sense to treat all our "plug-ins" the same way. Ideally I'd like to do checker/prio/foreign initialization completely lazily too, in the sense that callers don't need to worry about calling init_checkers() etc., either. But this series had to stop at some point. Either way, it's not a big issue, so please tell me if you feel strongly enough about it to ask me to revert the change. Regards, Martin -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, Sep 29, 2020 at 11:31:06AM +0200, Martin Wilck wrote: > On Mon, 2020-09-28 at 15:26 -0500, Benjamin Marzinski wrote: > > On Thu, Sep 24, 2020 at 03:40:50PM +0200, mwilck@suse.com wrote: > > > > > > /* > > > * We don't support re-initialization after > > > @@ -65,6 +66,9 @@ int libmultipath_init(void) > > > static void _libmultipath_exit(void) > > > { > > > libmultipath_exit_called = true; > > > + cleanup_foreign(); > > > > I don't really feel too strongly about this, but it seems to me that > > there is a difference between the checkers and prioritizers, which > > it seems like most users of libmultipath would want, and the foreign > > code, which doesn't seem that way. libmpathpersist, for instance, > > will use the checkers and prioritizers, but not the foreign code. > > On the other hand, if the caller isn't using the foreign code, > > then grabbing the lock and checking the foreign pointer shouldn't > > take much time. > > It would just be a few cycles. I want callers to have to worry about > cleanup as little as possible. All else is error-prone IMO, and > although I agree that the foreign functions are less important than > checkers and prio, I thought it made sense to treat all our "plug-ins" > the same way. > > Ideally I'd like to do checker/prio/foreign initialization completely > lazily too, in the sense that callers don't need to worry about calling > init_checkers() etc., either. But this series had to stop at some > point. > > Either way, it's not a big issue, so please tell me if you feel > strongly enough about it to ask me to revert the change. I already ACKed this bug in my reply to: [PATCH 00/23] libmultipath: improve cleanup on exit https://www.redhat.com/archives/dm-devel/2020-September/msg00635.html Sorry for the confusion. -Ben > Regards, > Martin > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c index 873b419..af56a95 100644 --- a/libmpathpersist/mpath_persist.c +++ b/libmpathpersist/mpath_persist.c @@ -78,8 +78,6 @@ mpath_lib_init (void) static void libmpathpersist_cleanup(void) { - cleanup_prio(); - cleanup_checkers(); libmultipath_exit(); dm_lib_exit(); } diff --git a/libmultipath/config.c b/libmultipath/config.c index 8097838..f115ac2 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -26,6 +26,7 @@ #include "devmapper.h" #include "mpath_cmd.h" #include "propsel.h" +#include "foreign.h" /* * We don't support re-initialization after @@ -65,6 +66,9 @@ int libmultipath_init(void) static void _libmultipath_exit(void) { libmultipath_exit_called = true; + cleanup_foreign(); + cleanup_checkers(); + cleanup_prio(); libmp_dm_exit(); udev_unref(udev); } diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version index 9abdb22..80f1950 100644 --- a/libmultipath/libmultipath.version +++ b/libmultipath/libmultipath.version @@ -1,4 +1,4 @@ -LIBMULTIPATH_0.8.4.5 { +LIBMULTIPATH_0.8.4.6 { global: /* symbols referenced by multipath and multipathd */ add_foreign; @@ -18,10 +18,7 @@ global: checker_name; checker_state_name; check_foreign; - cleanup_checkers; - cleanup_foreign; cleanup_lock; - cleanup_prio; close_fd; coalesce_paths; convert_dev; diff --git a/multipathd/main.c b/multipathd/main.c index e7b479a..efed56e 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -3027,9 +3027,6 @@ static void cleanup_child(void) { cleanup_threads(); cleanup_vecs(); - cleanup_foreign(); - cleanup_checkers(); - cleanup_prio(); if (poll_dmevents) cleanup_dmevent_waiter();