[dm-devel] [PATCH 17/32] libmultipath: fix tur checker double locking

[Bart Van Assche]

On Mon, 2018-08-06 at 17:55 -0500, Benjamin Marzinski wrote:
> I'm confused here. If the data is only read by multiple threads, I don't
> see why we need to synchronize it. Unless I'm missing something, my
> patch does make sure that the only time ct->devt is written is when
> there is only one thread running that has access to ct.
> 
> -       if (fstat(c->fd, &sb) == 0) {
> ...
> +       if (uatomic_read(&ct->holders) == 1 && ct->devt[0] == 0 &&
> +           fstat(c->fd, &sb) == 0) {
> 
> holders will always be 2 when a tur checker thread is running (well, the
> thread will decrement it when it finishes, but it will not touch ct
> after that action). Since only the thread calling libcheck_init can
> start the tur checker thread using this context, if there is no thread
> running at the start of libcheck_check(), then no thread can be started
> until libcheck_check() starts it.
> 
> Ideally, ct->devt would be set in libcheck_init(), but we don't have the
> information there, so setting it once, on the first time we call
> libcheck_check() seems like a reasonable, and safe, answer.  I certainly
> prefer it to adding recusive locking where it isn't needed.

Replacing a locking scheme into an approach based on atomic variables is
something I don't like because it makes the code harder to analyze for
correctness. Have you considered to keep the locking and fix the bug that
you described in the commit message?

Thanks,

Bart.