Re: [PATCH 7/8] libmultipath: handle TUR threads that can't be cancelled

[Martin Wilck <mwilck@suse.com>]

On Fri, 2018-10-12 at 17:11 -0500,  Benjamin Marzinski  wrote:
> On Wed, Oct 10, 2018 at 10:05:05PM +0200, Martin Wilck wrote:
> > When the tur checker code determines that a hanging TUR thread
> > couldn't be cancelled, rather than simply returning, reallocate
> > the checker context and start a new thread. This will leak some
> > memory if the hanging thread never wakes up again, but well, in
> > that highly unlikely case we're leaking threads anyway.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/checkers/tur.c | 24 +++++++++++++++++++++---
> >  1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libmultipath/checkers/tur.c
> > b/libmultipath/checkers/tur.c
> > index a986a244..9ecca5bd 100644
> > --- a/libmultipath/checkers/tur.c
> > +++ b/libmultipath/checkers/tur.c
> > @@ -349,11 +349,29 @@ int libcheck_check(struct checker * c)
> >  		}
> >  	} else {
> >  		if (uatomic_read(&ct->holders) > 1) {
> > -			/* The thread has been cancelled but hasn't
> > -			 * quit. exit with timeout. */
> > +			int holders;
> > +
> > +			/*
> > +			 * The thread has been cancelled but hasn't
> > quit.
> > +			 * We have to prevent it from interfering with
> > the new
> > +			 * thread. We create a new context and leave
> > the old
> > +			 * one with the stale thread, hoping it will
> > clean up
> > +			 * eventually.
> > +			 */
> >  			condlog(3, "%d:%d : tur thread not responding",
> >  				major(ct->devt), minor(ct->devt));
> > -			return PATH_TIMEOUT;
> > +
> > +			/* libcheck_init will replace c->context */
> > +			libcheck_init(c);
> > +
> > +			holders = uatomic_sub_return(&ct->holders, 1);
> > +			if (!holders)
> > +				/* It did terminate, eventually */
> > +				cleanup_context(ct);
> > +
> > +			ct = c->context;
> > +			if (ct == NULL)
> > +				return PATH_UNCHECKED;
> 
> libcheck_init can fail to allocate the checker_context, and return 1.
> If it does so, it won't reset c->context.  In this case, you will
> unhold
> the context but keep using it.  Instead we should fail right after
> the
> call to libcheck_init if it returns 1.

OK, I'll resubmit with this change.

> Also, returning PATH_UNCHECKED triggers code in check_path that
> assumes
> there is a problem with the path. It recalls pathinfo(), and does not
> do
> all the work that would happen on PATH_DOWN. PATH_TIMEOUT works like
> PATH_DOWN and PATH_SHAKY, which seems like the right thing to do
> here.

I am unsure about that. if libcheck_init() fails, we are out of memory.
This is a pretty hopeless situation anyway, but it does not indicate
(by itself) that the path is down. Proactively failing the path in DM
is dangerous; it might remove the last working path and cause file
systems to go read-only. We should only do that if we're positive that
the path is bad. Are we positive that a stalled TUR thread can only
happen if a path is broken? Maybe the thread is stalled because system
is thrashing?

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