[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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?



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]