On 02/13/2018 04:42 AM, Benjamin Marzinski wrote:
> Commit 6e2423fd fixed a bug where the tur checker could cancel a
> detached thread after it had exitted. However in fixing this, the new
> code grabbed a mutex (to call condlog) while holding a spin_lock.  To
> deal with this, I've done away with the holder spin_lock completely, and
> replaced it with two atomic variables, based on a suggestion by Martin
> Wilck.
> 
> The holder variable works exactly like before.  When the checker is
> initialized, it is set to 1. When a thread is created it is incremented.
> When either the thread or the checker are done with the context, they
> atomically decrement the holder variable and check its value. If it
> is 0, they free the context. If it is 1, they never touch the context
> again.
> 
> The other variable has changed. First, ct->running and ct->thread have
> switched uses. ct->thread is now only ever accessed by the checker,
> never the thread.  If it is non-NULL, a thread has started up, but
> hasn't been dealt with by the checker yet. It is also obviously used
> by the checker to cancel the thread.
> 
> ct->running is now an atomic variable.  When the thread is started
> it is set to 1. When the checker wants to kill a thread, it atomically
> sets the value to 0 and reads the previous value.  If it was 1,
> the checker cancels the thread. If it was 0, the nothing needs to be
> done.  After the checker has dealt with the thread, it sets ct->thread
> to NULL.
> 
> Right before the thread finishes and pops the cleanup handler, it
> atomically sets the value of ct->running to 0 and reads the previous
> value. If it was 1, the thread just pops the cleanup handler and exits.
> If it was 0, then the checker is trying to cancel the thread, and so the
> thread calls pause(), which is a cancellation point.
> 
> Cc: Martin Wilck <mwi...@suse.com>
> Cc: Bart Van Assche <bart.vanass...@wdc.com>
> Signed-off-by: Benjamin Marzinski <bmarz...@redhat.com>
> ---
>  libmultipath/checkers/tur.c | 107 
> ++++++++++++++++++--------------------------
>  1 file changed, 43 insertions(+), 64 deletions(-)
> 
[ .. ]
> @@ -391,19 +368,17 @@ int libcheck_check(struct checker * c)
>               ct->state = PATH_UNCHECKED;
>               ct->fd = c->fd;
>               ct->timeout = c->timeout;
> -             pthread_spin_lock(&ct->hldr_lock);
> -             ct->holders++;
> -             pthread_spin_unlock(&ct->hldr_lock);
> +             uatomic_add(&ct->holders, 1);
> +             uatomic_set(&ct->running, 1);
>               tur_set_async_timeout(c);
>               setup_thread_attr(&attr, 32 * 1024, 1);
>               r = pthread_create(&ct->thread, &attr, tur_thread, ct);
>               pthread_attr_destroy(&attr);
>               if (r) {
> -                     pthread_spin_lock(&ct->hldr_lock);
> -                     ct->holders--;
> -                     pthread_spin_unlock(&ct->hldr_lock);
> -                     pthread_mutex_unlock(&ct->lock);
> +                     uatomic_sub(&ct->holders, 1);
> +                     uatomic_set(&ct->running, 0);
>                       ct->thread = 0;
> +                     pthread_mutex_unlock(&ct->lock);
>                       condlog(3, "%s: failed to start tur thread, using"
>                               " sync mode", tur_devt(devt, sizeof(devt), ct));
>                       return tur_check(c->fd, c->timeout,
I would rather set 'ct->running' from within the thread; otherwise there
is a chance that pthread_create() returns without error, but the thread
itself hasn't been run yet.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Teamlead Storage & Networking
h...@suse.de                                   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

Reply via email to