Hi Chuck,
The first race condition I described - I think it is still present (hopefully
I'm wrong so please correct me if this is the case). Anyway, I'll continue to
look at your latest patch as fixing the above issue should be few lines change.
See my comments below.
Chuck Bell wrote:
> Hi Rafal,
>
> Here's a new patch. See comments below concerning other changes.
>
> http://lists.mysql.com/commits/37671
>
> Note that this patch is based on the new mysql-6.0-backup tree so you can
> compile and run it if you desire.
>
> Chuck
>
>> -----Original Message-----
>> From: Rafal Somla [mailto:rsomla@stripped]
>> Sent: Monday, November 12, 2007 8:35 AM
>> To: cbell@stripped
>> Cc: commits@stripped
>> Subject: Re: bk commit into 5.2 tree (cbell:1.2610) BUG#31383
>>
>> Hi Chuck,
>>
>> Writing multi thread code is really tricky. I found two other
>> problems which might happen in this code. The first one is
>> when kill_locking_thread() is called early. Then the folowing
>> race condition might happen:
>>
>> [backup kernel thread]
>> | [table locking thread]
>> | |
>> 1 | set lock_state to LOCK_NOT_STARTED
>> | |
>> 2 kill_locking_thread() |
>> | |
>> 3 | open_and_lock_tables()
>> 4 | set lock_state to LOCK_ACQUIRED
>> 5 | <wait on exit condition>
>> | |
>> 6 ~Backup_thread_driver()
>>
>> When kill_locking_thread() is executed in step 2, lock_state
>> is set to LOCK_NOT_STARTED, thus the function will do
>> nothing. As the result the locking thread will wait on
>> condition but no one is ever going to broadcast it. When the
>> destructor is called, it will wait on COND_driver_wait, since
>> lock_state is now LOCK_ACQUIRED which is different from
>> LOCK_DONE. Thus we have a deadlock... :(
>>
(cut)
>
> Done. Except I use LOCK_SIGNAL in the CS driver and that's why it appears
> outside the if (lock_thd... block. I left your suggestions in but also left
> that statement in as well. It doesn't hurt anything the way I have it. See
> code below:
>
> void Backup_thread_driver::kill_locking_thread()
> {
> DBUG_ENTER("Backup_thread_driver::kill_locking_thread");
> if (lock_state == LOCK_ACQUIRED)
> {
> pthread_mutex_lock(&THR_LOCK_driver);
> if (lock_thd && (lock_state != LOCK_DONE) && (lock_state !=
> LOCK_SIGNAL))
> {
> lock_state= LOCK_SIGNAL;
> pthread_mutex_lock(&lock_thd->LOCK_delete);
> lock_thd->awake(THD::KILL_CONNECTION);
> pthread_mutex_unlock(&lock_thd->LOCK_delete);
> pthread_cond_broadcast(&COND_driver_thread_wait);
> }
> /*
> Set the lock state to LOCK_SIGNAL to tell the CS driver that
> the locks are now freed.
> */
> else if (!lock_thd)
> lock_state= LOCK_SIGNAL;
> pthread_mutex_unlock(&THR_LOCK_driver);
> }
> DBUG_VOID_RETURN;
> }
>
But then, the race condition will still be present. Because of "if (lock_state
== LOCK_ACQUIRED)" the kill_locking_thread() call in step 2 will do nothing and
the same deadlock will happen. The whole point of the change was to remove this
if guard so that kill_locking_thread() *always* changes lock_state to
LOCK_SIGNAL. If it interferes with the use of lock_state in the CS driver then
why not add another constant or use a different state variable (as CS driver is
not using the locking thread).
>
>> The other problem is similar to what we found in the
>> replication thread shutdown code. Here is a possible race
>> condition leading to a crash:
>>
>> [backup kernel thread]
>> | [table locking thread]
>> | |
>> 2 | set lock_state to LOCK_NOT_STARTED
>> 3 | open_and_lock_tables()
>> 4 | set lock_state to LOCK_ACQUIRED
>> 5 | <wait on exit condition>
>> | |
>> 6 kill_locking_thread() |
>> 7 (this sets lock_state to LOCK_SIGNAL)
>> | |
>> 8 | <wakeup on condition>
>> 9 | cleanup (delete thd)
>> | (sets lock_state to LOCK_DONE)
>> | |
>> 10 ~Backup_thread_driver()
>> | |
>> 11 | broadcast COND_driver_wait - blah!
>>
>> The problem is that when destructor is called, lock_state is
>> set to LOCK_DONE and thus the destructor will not wait on the
>> condition but just destroy all mutexes and conditions. But
>> then, in step 11 the locking thread will try to broadcast
>> condition which has been deleted - potential crash.
>>
>> I think an easy fix for this is to switch the order in which
>> COND_driver_wait is broadcasted and THR_LOCK_driver mutex is
>> released at the end of
>> backup_thread_for_locking() function. That is, it should
>> *first* broadcast on the condition and then release the lock.
>>
>
> Done
OK.
>
>> I believe that after fixing these two issues the code will
>> finally be thread safe.
>
> PTL!
>
>> A suggestion, if you care to refactor the code while fixing
>> above problems and agree with the idea. I would move the code
>> which waits for the termination of the locking thread from
>> the destructor to the kill_locking_thread() function.
>> This way, one will know that after calling
>> kill_locking_thread() the locking thread is no longer active.
>> This should make code easier to comprehend. After this
>> change, the destructor should call kill_locking_thread()
>> before destroying the mutexes and conditions.
>
> A good suggestion, however I was trying to minimize possible delays in
> backup operations by delaying the wait to the destructor. Otherwise, the
> backup could stall while waiting for the thread to die. I left it as-is for
> now.
>
But you would call kill_locking_thread() only in the destructor or in cancel()
method(), i.e., in the places where we can't avoid waiting. The only benefit I
see is that now you can kill the locking thread in the last get_data() call
without any waiting. But this is just optimization we could easily live without
(the locking thread will live bit longer) or, we can accept the possible delay
as it will not be very probable - normally the last get_data() call will be
after the locking thread has finished its job and is waiting for termination
signal.
>> Another idea, but this is just for the record and future
>> developement. I wonder if it wouldn't be easier to use
>> pthread_join() to wait for the termination of the locking
>> thread, instead of using all the condition signaling and waits.
>
> Another good suggestion. I have tried this and it works but delays the
> driver. So for the same reason stated above I leave the wait to the
> destructor.
>
For the above reasons I think it is not an issue, but we can leave it now.
>> See also below for some small remarks. I wonder if killing
>> the locking thread should be signalled as an error or be
>> distinguished from real errors (we have similar problem in
>> replication code where termination of a replication thread is
>> reported as an error leading to confusing entries in the
>> error log). But I think there will be no major problems if it
>> is treated as an error.
>
> Hmm...ponderous. I suggest we keep this nugget for when we design the proper
> threading solution for online backup. That being said, look in get_data() in
> be_default.cc:
>
> case LOCK_ERROR: // Something ugly happened in locking
> DBUG_RETURN(ERROR);
>
Yes, I agree that the current logic should work ok. We can just keep it in mind
for the future.
Rafal
