Re: [dm-devel] [PATCH 05/35] libmultipath: improve cleanup of uevent queues on exit

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Sep 16, 2021 at 09:10:57AM +0200, Martin Wilck wrote:
> On Wed, 2021-09-15 at 17:20 -0500, Benjamin Marzinski wrote:
> > On Fri, Sep 10, 2021 at 01:40:50PM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > uevents listed on merge_node must be cleaned up, too. uevents
> > > cancelled while being serviced and temporary queues, likewise.
> > > The global uevq must be cleaned out in the uevent listener thread,
> > > because it might have added events after the dispatcher thread
> > > had already finished.
> > > 
> > 
> > There's nothing wrong with this, but for the global list, wouldn't it
> > be
> > easier to just wait till after cleanup_child() calls
> > cleanup_threads(),
> > and then call cleanup_global_uevq(). That way you know nothing else
> > is
> > running.
> 
> That would be possible. If I understand your proposal correctly, that
> would require calling cleanup_global_uevq() from cleanup_child(), i.e.
> from the main process. Currently uevq and the functions handling it are
> static and only visible in uevent.c.
> 
> By taking the lock in cleanup_global_uevq() and calling it from the
> listener on cleanup, I can be sure that the dispatcher won't see any
> more uevents if it hasn't terminated yet, and that no no events will be
> added to the queue after cleanup. So I think with this patch I can also
> be certain that uevq is cleaned up for good, without a need to export
> the cleanup function, and without enforcing a certain order of thread
> shutdowns on exit. Do you disagree?

Yeah, that's why I said that there's nothing wrong with the patch. It
just seemed more self-evidently correct to do it after stopping all the
threads. But, regardless

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> 
> Thanks,
> Martin
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel