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@xxxxxxxx> > > --- > > 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@xxxxxxxx>, 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel