| Message ID | 20181010200506.15796-8-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | various multipath-tools patches | expand |
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. 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. -Ben > } > /* Start new TUR checker */ > pthread_mutex_lock(&ct->lock); > -- > 2.19.0 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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
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; } /* Start new TUR checker */ pthread_mutex_lock(&ct->lock);
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(-)