STATUS: Approved.
SUGGESTIONS
-----------
1. Do not introduce new condition: use COND_lock_state as before.
2. Do not introduce new flag.
COMMENTS
--------
Looks like a solid piece of work - well done!
DETAILS
-------
Thavamuni.Alagu@stripped wrote:
> #At file:///home/thava/repo/backup/ based on
> revid:sanjay.manwani@stripped
>
> 2899 Thava Alagu 2009-12-01
> Bug#47939 : MyISAM backup driver can wait in unlock() too long
>
> As per the protocol, the backup driver should return from
> unlock() as early as possible. However, the killing of
> locking thread by MyISAM driver from within unlock()
> involves handshakes with locking thread which should
> be eliminated to avoid potential problems.
>
> This is fixed by making kill_locking_thread() not to wait
> for the death of the locking thread.
> The wait for the confirmation of locking thread's death
> happens later at the end.
...
> === modified file 'storage/myisam/myisam_backup_engine.cc'
> --- a/storage/myisam/myisam_backup_engine.cc 2009-10-12 09:08:34 +0000
> +++ b/storage/myisam/myisam_backup_engine.cc 2009-12-01 11:06:35 +0000
> @@ -268,11 +268,14 @@ private:
> THD *lock_thd;
> bool cannot_delete_lock_thd;
> pthread_cond_t COND_lock_state; ///< for communication with locking thread
> + pthread_cond_t COND_lock_death_confirm; ///< To confirm lock thread death.
[1] I concur with Ingo's suggestion to not introduce a new condition here
(see below).
> void kill_locking_thread();
> + void wait_for_locking_thread_death();
> static const size_t bytes_between_sleeps= 10*1024*1024;
> /** After copying bytes_between_sleeps we sleep sleep_time */
> ulong sleep_time;
> size_t bytes_since_last_sleep; ///< how many bytes sent since we last slept
> + bool should_wait_for_lock_thd_death; ///< Remember to wait for lock thread
> death.
[2] I concur with Ingo's suggestion to not use this flag (see below).
> };
>
> /* Needed for VisualAge 6.0 */
...
> @@ -458,9 +463,12 @@ retry:
> cannot_delete_lock_thd= FALSE;
> /* we wake up thread if it was blocked on the bool above */
> pthread_cond_broadcast(&COND_lock_state);
> - /* And we wait for the thread to inform of its death */
> - while (lock_state != LOCK_ERROR)
> - pthread_cond_wait(&COND_lock_state, &THR_LOCK_myisam);
> + /**
> + This is executed during time critical section, hence we won't
> + wait for the confirmation of the death of locking thread here.
> + We indeed remember to wait later.
> + */
> + should_wait_for_lock_thd_death= TRUE;
[2] I concur with Ingo's suggestion to use lock_state instead.
> }
> pthread_mutex_unlock(&THR_LOCK_myisam);
> DBUG_VOID_RETURN;
> @@ -468,6 +476,25 @@ retry:
>
>
> /**
> + Wait for the death of the locking thread.
> +
> + The locking thread is killed using @c kill_locking_thread() method during
> + time sensitive critical section. The confirmation of the death of the
> + locking thread happens little later using this method.
> +*/
> +
> +void Backup::wait_for_locking_thread_death()
> +{
> + /* We wait for the locking thread to inform of its death */
> + pthread_mutex_lock(&THR_LOCK_myisam);
> + while (lock_state != LOCK_ERROR){
> + pthread_cond_wait(&COND_lock_death_confirm, &THR_LOCK_myisam);
[1] I don't completely agree that separate condition makes code more clear.
I guess your thinking is that different events should be signalled by
different conditions. But alternative thinking is that there is one
condition to be signalled each time the state of the locking thread is
changed. That was the original idea with COND_lock_state. We enter waiting
loop if lock_state != LOCK_ERROR and we wait exactly for a change of the
state. So, using COND_lock_state makes sense to me.
> + }
> + pthread_mutex_unlock(&THR_LOCK_myisam);
> +}
> +
> +
> +/**
> This destructor is only called by the class' free().
> It cleans up any leftover the driver could have. It is safe to call it at
> any point. In a normal (no error) situation, the hash freeing is the only
> @@ -478,6 +505,13 @@ retry:
> Backup::~Backup()
> {
> DBUG_ENTER("myisam_backup::Backup::~Backup");
> + kill_locking_thread();
> + /* If we had remembered to wait for the death of locking thread, wait now.*/
> + if (should_wait_for_lock_thd_death)
> + {
> + should_wait_for_lock_thd_death= FALSE;
> + wait_for_locking_thread_death();
> + }
[2] Hmm, this looks strange, because after kill_locking_thread(),
should_wait_for_lock_thd_death should always be true. So, why to bother for
checking this condition? Or, is it for the case that locking thread has died
without being explicitly killed?
[2] Anyway, I disagree that introducing this flag makes it easier for
reading and understanding the code. I think it would be much nicer to simply
have:
kill_locking_thread();
wait_for_locking_thread_death();
with correct understanding that kill_locking_thread() does nothing if thread
has been already killed and wait_for_locking_thread_death() exits
immediately if locking thread is already dead.
> /* If we had already started backup logging, we must dirtily stop it */
> mi_log(MI_LOG_ACTION_CLOSE_INCONSISTENT, MI_LOG_PHYSICAL, NULL, NULL);
> delete image;
Rafal
