[dm-devel] [PATCH 1/3] multipathd: fix waiter thread cancelling

[Martin Wilck]

On Mon, 2018-03-19 at 11:25 -0500, Benjamin Marzinski wrote:
> On Mon, Mar 19, 2018 at 01:59:24PM +0100, Martin Wilck wrote:
> > On Fri, 2018-03-16 at 16:31 -0500, Benjamin Marzinski wrote:
> > > diff --git a/multipathd/waiter.c b/multipathd/waiter.c
> > > index cb9708b..e894294 100644
> > > --- a/multipathd/waiter.c
> > > +++ b/multipathd/waiter.c
> > > @@ -23,6 +23,7 @@
> > >  #include "waiter.h"
> > >  
> > >  pthread_attr_t waiter_attr;
> > > +struct mutex_lock waiter_lock = { .mutex =
> > > PTHREAD_MUTEX_INITIALIZER
> > > };
> > >  
> > >  static struct event_thread *alloc_waiter (void)
> > >  {
> > > @@ -59,8 +60,12 @@ void stop_waiter_thread (struct multipath
> > > *mpp,
> > > struct vectors *vecs)
> > >  		mpp->waiter);
> > >  	thread = mpp->waiter;
> > >  	mpp->waiter = (pthread_t)0;
> > > -	pthread_cancel(thread);
> > > +	pthread_cleanup_push(cleanup_lock, &waiter_lock);
> > > +	lock(&waiter_lock);
> > > +	pthread_testcancel();
> > 
> > What's the purpose of this pthread_testcancel() call?
> > 
> 
> It's not necessary. I feel like in general, after you get done
> waiting
> on a lock, it's good form to see if you've been cancelled while you
> were
> waiting. However, it's really unlikely that a thread will be waiting
> here long, and this pthread_cancel isn't protecting it form accessing
> any freed data. If you think it's likely to confuse people, I have no
> problem with removing it.

My thinking was: this thread is just about to cancel another thread. If
it's cancelled itself just before doing that, the other one won't be
cancelled. I find it hard to imagine conditions where that would be the
right thing. So if at all, I'd call pthread_testcancel() after
releasing the lock. But probably it's best just to remove it here.

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)