| Message ID | 20200924134054.14632-8-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:38PM +0200, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > Also removing the comment about dlog() and dm_write_log(). > dlog() can cope with get_multipath_config() returning NULL, > and dm_write_log() hasn't accessed the configuration for a while. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > multipathd/main.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 8b9df55..4d5b40b 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2892,6 +2892,16 @@ set_oom_adj (void) > condlog(0, "couldn't adjust oom score"); > } > > +static void cleanup_conf(void) { > + struct config *conf; > + > + conf = rcu_dereference(multipath_conf); > + if (!conf) > + return; > + rcu_assign_pointer(multipath_conf, NULL); > + call_rcu(&conf->rcu, rcu_free_config); > +} > + > static void cleanup_maps(struct vectors *vecs) > { > int queue_without_daemon, i; > @@ -3194,17 +3204,11 @@ child (__attribute__((unused)) void *param) > > condlog(2, "--------shut down-------"); > > - if (logsink == 1) > + if (logsink == 1) { > + logsink = 0; > log_thread_stop(); It seems like log_thread_stop() could just do something like pthread_t log_thr_save = log_thr; log_thr = (pthread_t)0; at the start, and then you would continue to get syslog logging, even when the log thread stopped. It's racy, but all the other threads (except the log_thread, obviously) should be stopped. Or am I not understanding the purpose of doing this? > - > - /* > - * Freeing config must be done after condlog() and dm_lib_exit(), > - * because logging functions like dlog() and dm_write_log() > - * reference the config. > - */ > - conf = rcu_dereference(multipath_conf); > - rcu_assign_pointer(multipath_conf, NULL); > - call_rcu(&conf->rcu, rcu_free_config); > + } > + cleanup_conf(); > #ifdef _DEBUG_ > dbg_free_final(NULL); > #endif > -- > 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 13:03 -0500, Benjamin Marzinski wrote: > > > - if (logsink == 1) > > + if (logsink == 1) { > > + logsink = 0; > > log_thread_stop(); > > It seems like log_thread_stop() could just do something like > > pthread_t log_thr_save = log_thr; > log_thr = (pthread_t)0; > > at the start, and then you would continue to get syslog logging, even > when the log thread stopped. It's racy, but all the other threads > (except the log_thread, obviously) should be stopped. Or am I not > understanding the purpose of doing this? I guess we could do this, yes (although I think (pthread_t)0 should be avoided, but that's a different issue. Let me have another look. Regards, Martin -- 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 8b9df55..4d5b40b 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2892,6 +2892,16 @@ set_oom_adj (void) condlog(0, "couldn't adjust oom score"); } +static void cleanup_conf(void) { + struct config *conf; + + conf = rcu_dereference(multipath_conf); + if (!conf) + return; + rcu_assign_pointer(multipath_conf, NULL); + call_rcu(&conf->rcu, rcu_free_config); +} + static void cleanup_maps(struct vectors *vecs) { int queue_without_daemon, i; @@ -3194,17 +3204,11 @@ child (__attribute__((unused)) void *param) condlog(2, "--------shut down-------"); - if (logsink == 1) + if (logsink == 1) { + logsink = 0; log_thread_stop(); - - /* - * Freeing config must be done after condlog() and dm_lib_exit(), - * because logging functions like dlog() and dm_write_log() - * reference the config. - */ - conf = rcu_dereference(multipath_conf); - rcu_assign_pointer(multipath_conf, NULL); - call_rcu(&conf->rcu, rcu_free_config); + } + cleanup_conf(); #ifdef _DEBUG_ dbg_free_final(NULL); #endif