[PATCH] (DRAFT!) Fix a deadlock in CLVMD (possibly related to BZ 561226).

[PATCH] (DRAFT!) Fix a deadlock in CLVMD (possibly related to BZ 561226).

[Petr Rockai]

Hi,

the signalling code (pthread_cond_signal/pthread_cond_wait) in the
pre_and_post_thread in clvmd is using the wait mutex (see man
pthread_cond_wait) incorrectly, and this can cause clvmd to completely
deadlock when the timing is right.

This was showing up on nevrast in buildbot. Possibly triggered by an
independent bug in my vgextend --restoremissing code that caused a
double unlock of orphans, which would *normally* be fairly harmless, but
it tripped this race. Was not happening on other machines...

Anyway, Milan says that this could be related to BZ 561226 where clvmd
runs into deadlocks as well. It may or may not be the same, but it is
quite likely that someone somewhere would run into the bug that this
patch is fixing as well.

NB., I only made very modest effort at checking if localsock.mutex was
not being used for anything else. Whoever ends up reviewing the code,
please make sure this is so! If the localsock.mutex is being used for
anything else, the patch could actually introduce some new bad
behaviours. In that case, just creating a new mutex in the same
structure just for the pre_and_post_thread would be necessary. But you
get the idea of the fix, anyway. (It is also recommended that the
reviewer spends a while reading pthread_cond_wait manpage, especially
the section about the mutex pointer, and also with a paper and pencil
sketching on how things could go wrong if the mutex is used incorrectly,
as in this case.)

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: clvmd.diff
Type: text/x-diff
Size: 1794 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20101018/0f9bdb4d/attachment.bin>

[PATCH] (DRAFT!) Fix a deadlock in CLVMD (possibly related to BZ 561226).

[Petr Rockai]

Petr Rockai <prockai@redhat.com> writes:
> (It is also recommended that the reviewer spends a while reading
> pthread_cond_wait manpage, especially the section about the mutex
> pointer, and also with a paper and pencil sketching on how things
> could go wrong if the mutex is used incorrectly, as in this case.)

I can try to illustrate the problem in ASCII art... There is a single
mutex around (L for Lock, U for Unlock), a signal (S) and a wait (W). C
for pthread_create. Time flows from left to right, each arrow is a
thread.

So first the "naive" scenario, with no mutex (PPT = pre_and_post_thread,
MCT = main clvmd thread). I will also use X, for a moment when MCT
actually waits for something to happen that PPT was supposed to do.


MCT -----C ------S--X-----S----X----------------------S------XXXXXXXXX
         |                everything OK up to this --> <-- point...
PPT       -----WWW-----WWWW------------------------------WWWWWWWWWWWWW

Ok, so pthread API actually does not let you use W/S like that. It goes
out of its way to tell you that you need a mutex to protect the W so
that the above cannot happen. *But* if you get creative and just lock
around the W's and S's, this happens:

MCT ----C-----LSU----X-----LSU----X------------LSU------XXXXXXX
        |
PPT      ---LWWWU-------LWWWWU-----------------------LWWWWWWWWW

Ooops. Nothing changed (the above is what actually was done by clvmd
before the patch). So let's do it differently, holding L locked *all*
the time in PPT, unless we are actually in W (this is something that the
pthread API does itself, see the man page).

MCT ----C-----LSU------X---LSU---X-----LLLLLLLSU----X----
        |                             (and they live happily ever after)
PPT     L---WWWWW---------WWWW----------------W----------

So W actually ensures that L is unlocked *atomically* together with
entering the wait. That means that unless PPT is actually waiting, it
cannot be signalled by MCT. So if MCT happens to signal it too soon (it
wasn't waiting yet), it MCT will be blocked on the mutex (L), until PPT
is actually ready to do something.

That's the story. You obviously need monotype-capable reader to make any
sense of it. : - )

Yours,
   Petr.

[PATCH] (DRAFT!) Fix a deadlock in CLVMD (possibly related to BZ 561226).

[Milan Broz]

On 10/18/2010 07:17 PM, Petr Rockai wrote:
> the signalling code (pthread_cond_signal/pthread_cond_wait) in the
> pre_and_post_thread in clvmd is using the wait mutex (see man
> pthread_cond_wait) incorrectly, and this can cause clvmd to completely
> deadlock when the timing is right.

well, nobody even tried to answer, despite this quite important problem...

So, I see these important items:

1) always rely on implicit unlock in pthread_cond_wait()

2) some of the code now runs with client->bits.localsock.mutex locked,
mainly the do_pre_command() and do_post_command() + socket writes

3) the common code to signal is _only_ this pattern

	pthread_mutex_lock(&client->bits.localsock.mutex);
	client->bits.localsock.state = <STATE>;
	pthread_cond_signal(&client->bits.localsock.cond);
	pthread_mutex_unlock(&client->bits.localsock.mutex);

this is called from request_timed_out() and read_from_local_sock()
and it wakes up thrad always waiting in this code pattern

	pthread_cond_wait(&client->bits.localsock.cond,
			  &client->bits.localsock.mutex);

I didn't found any other uses for this lock.

So the thread is signaled only when in cond_wait now (there is no
other place where signaling thread can take lock now).

This seems as expected fix.

The post/pre routines should not have problems with running with
localsock.mutex taken, same for socket writes - hope I am right:-)

Did I miss or misinterpret anything?

Not extra deep analysis, but for me it seems like correct fix,
I think we should commit it and run some tests...

Milan

p.s.
nitpicking

	DEBUGLOG("in sub thread: client = %p\n", client);
	+ pthread_mutex_lock(&client->bits.localsock.mutex);

	/* Don't start until the LVM thread is ready */
	pthread_mutex_lock(&lvm_start_mutex);
	pthread_mutex_unlock(&lvm_start_mutex);
	DEBUGLOG("Sub thread ready for work.\n");


Can we move that localsock.muext lock after the lvm_start_mutex exercise?
(Which I do not like either - isn't cond wait better for that?)

[PATCH] (DRAFT!) Fix a deadlock in CLVMD (possibly related to BZ 561226).

[Petr Rockai]

Milan Broz <mbroz@redhat.com> writes:

> p.s.
> nitpicking
>
> 	DEBUGLOG("in sub thread: client = %p\n", client);
> 	+ pthread_mutex_lock(&client->bits.localsock.mutex);
>
> 	/* Don't start until the LVM thread is ready */
> 	pthread_mutex_lock(&lvm_start_mutex);
> 	pthread_mutex_unlock(&lvm_start_mutex);
> 	DEBUGLOG("Sub thread ready for work.\n");
>
>
> Can we move that localsock.muext lock after the lvm_start_mutex exercise?
> (Which I do not like either - isn't cond wait better for that?)

I would be actually wary about that -- I suspect that going the other
way around could introduce a (very slight) race condition. Not that it
would ever happen in practice, but still. On the other hand, I didn't
try too hard to think about it, so I might be wrong.

Yours,
   Petr.

[PATCH] (DRAFT!) Fix a deadlock in CLVMD (possibly related to BZ 561226).

[Milan Broz]

On 10/20/2010 03:03 PM, Petr Rockai wrote:
> I would be actually wary about that -- I suspect that going the other
> way around could introduce a (very slight) race condition. Not that it
> would ever happen in practice, but still. On the other hand, I didn't
> try too hard to think about it, so I might be wrong.

ok, ack, please commit it.
... and wait what happens with tests.

m.