Re: [PATCH v2 1/7] libmultipath: fix tur checker locking

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Thu, 2018-02-08 at 17:56 -0600, Benjamin Marzinski wrote:
>  static void cleanup_func(void *data)
>  {
> -	int holders;
> +	int running, holders;
>  	struct tur_checker_context *ct = data;
> -	pthread_spin_lock(&ct->hldr_lock);
> -	ct->holders--;
> -	holders = ct->holders;
> -	ct->thread = 0;
> -	pthread_spin_unlock(&ct->hldr_lock);
> +
> +	running = uatomic_xchg(&ct->running, 0);
> +	holders = uatomic_sub_return(&ct->holders, 1);
>  	if (!holders)
>  		cleanup_context(ct);
> +	if (!running)
> +		pause();
>  }

Hello Ben,

Why has the pause() call been added? I think it is safe to call pthread_cancel()
for a non-detached thread that has finished so I don't think that pause() call
is necessary.
 
>  static int tur_running(struct tur_checker_context *ct)
>  {
> -	pthread_t thread;
> -
> -	pthread_spin_lock(&ct->hldr_lock);
> -	thread = ct->thread;
> -	pthread_spin_unlock(&ct->hldr_lock);
> -
> -	return thread != 0;
> +	return (uatomic_read(&ct->running) != 0);
>  }

Is such a one line function really useful? I think the code will be easier to read
if this function is inlined into its callers.

> @@ -418,8 +396,12 @@ int libcheck_check(struct checker * c)
>  		    (tur_status == PATH_PENDING || tur_status == PATH_UNCHECKED)) {
>  			condlog(3, "%s: tur checker still running",
>  				tur_devt(devt, sizeof(devt), ct));
> -			ct->running = 1;
>  			tur_status = PATH_PENDING;
> +		} else {
> +			int running = uatomic_xchg(&ct->running, 0);
> +			if (running)
> +				pthread_cancel(ct->thread);
> +			ct->thread = 0;
>  		}
>  	}

Why has this pthread_cancel() call been added? I think that else clause can only be
reached if ct->running == 0 so I don't think that the pthread_cancel() call will ever
be reached.

Thanks,

Bart.