Re: [Maria-developers] Deadlock with STOP SLAVE and LOCK_active_mi
Hi!
Any suggestions for how this is supposed to work? Or is it just broken by design, but saved because normally slave threads do not need to access SHOW STATUS or system variables?
<cut>
Hm. So I went through all of the code that references LOCK_active_mi to try and understand this. It seems there are two main uses:
1. As you say, to serialise admin commands. I think the list is: end_slave() (server shutdown); CHANGE MASTER; START/STOP SLAVE; START/STOP ALL SLAVES; RESET SLAVE; FLUSH SLAVE.
2. To protect access to master_info_index, eg. to prevent a Master_info from disappearing while it is accessed.
When reading about this, I think it would be better to have a counter in Master_info if someone is using it and only delete if if the counter is zero. Would be trivial to add an increment of the counter in get_master_info(). The counter could either be protected by the LOCK_active_mi or one of the mutexes in Master_info like data_lock.
There is a comment in rpl_rli.h that LOCK_active_mi protects Relay_log_info::inited, but I did not understand it - maybe it is wrong?
Setting inited is indirectly protected by LOCK_active_mi, as init_all_master_info() which calls init_master_info(), is protected by LOCK_active_mi. Looking at the code, I didn't however see any need to protect inited as this is an internal flag that is always 1 after start. It's main usage is to avoid some re-initialization of relay logs on retries.
So the idea would be to make a new lock for (1), and keep LOCK_active_mi for (2).
Actually, using a lock for (1) seems a bad idea - STOP SLAVE can take a long time to run, and a mutex makes it unkillable. A condition variable might be better (to make STOP SLAVE killable). But I guess that is a separate issue, it would not affect the possibility of deadlocks.
Agree that a condition variable would be better, but not critical as one has already a problem if stop_slave doesn't work.
Both LOCK_active_mi and the new LOCK_serialize_replication_admin_commands must by themselves prevent master_info_index from having elements added or removed. This is needed for start_all_slaves() and stop_all_slaves() to work correctly, I think.
Why both? Isn't it enough to first take LOCK_serialize_replication_admin_commands and then take LOCK_active_mi if one needs to access master_info_index ?
Something will need to be done for remove_master_info(). Currently, it also tries to stop slave threads (from within free_key_master_info()) while holding LOCK_active_mi. It seems that threads should be stopped before attempting to remove their mi from master_info_index, probably?
To do that we would need to: - Add a flag in Master_info that it's not usable anymore and change this flag only under LOCK_active_mi. get_master_info() could wait (on a condition) if this flag is set. - Add a counter that Master_info is in use. - Add a function to release Master_info. - Call terminate_slave_threads() outside of free_key_master_info()
Actually, there would still be the deadlock problem after introducing the new lock, because it is possible for a slave thread to run CHANGE MASTER! (and I think START SLAVE / RESET SLAVE as well). But that is probably a bug, does not seem good that this is possible. I guess these need a check to throw an ER_LOCK_OR_ACTIVE_TRANSACTION error, like STOP SLAVE.
Wouldn't the new lock LOCK_serialize_replication_admin_commands fix that?
I think I also found another lock problem while reading the code, for MASTER_POS_WAIT() og SHOW BINLOG EVENTS. They both grab a Master_info under LOCK_active_mi, but then proceed to access it after releasing the lock. I do not see anything that prevents the Master_info to disappear under their feet if RESET SLAVE ALL is run in another thread? But again, that's a separate issue, I suppose. Probably there are some other tricky deadlock issues lurking here.
I checked the function mysql_show_binlog_events() but could not find any access to Master_info after mysql_mutex_unlock(&LOCK_active_mi) was released.
I don't know. It seems splitting into a new LOCK_serialize_replication_admin_commands could solve the deadlock issue, maybe, if the other problems mentioned above are addressed. It seems very hard to convince oneself that this would not introduce new problems somewhere, the locking in replication is really complex and feels very fragile :-/ It doesn't really feel like something one would want to do in 10.1 (certainly not 10.0), but maybe 10.2?
It's kind of a very edge-case problem (and seems to have been there for a _long_ time). Still, hanging the mysqld process hard, requiring kill -9, is also not nice...
Adding two new flags, one if master_info is in use and one if it's unusable, shouldn't be that hard to make reasonable safe in 10.1 I am now back in Finland and working. Feel free to call me to discuss this any time. Regards, Monty
Michael Widenius <michael.widenius@gmail.com> writes:
2. To protect access to master_info_index, eg. to prevent a Master_info from disappearing while it is accessed.
When reading about this, I think it would be better to have a counter in Master_info if someone is using it and only delete if if the counter is zero. Would be trivial to add an increment of the counter in get_master_info().
The counter could either be protected by the LOCK_active_mi or one of the mutexes in Master_info like data_lock.
Sounds reasonable. Will you do a patch? I guess this is code from the multisource feature, so you are the one familiar with it (before multi-source, the Master_info could not be created and deleted dynamically, could it?)
Both LOCK_active_mi and the new LOCK_serialize_replication_admin_commands must by themselves prevent master_info_index from having elements added or removed. This is needed for start_all_slaves() and stop_all_slaves() to work correctly, I think.
Why both? Isn't it enough to first take LOCK_serialize_replication_admin_commands and then take LOCK_active_mi if one needs to access master_info_index ?
In stop_all_slaves(), there is a loop over the master_info_hash, within which slaves are stopped one at a time. So LOCK_active_mi cannot be held across the loop because of the deadlock discussed in this thread. So it seems that LOCK_serialize_replication_admin_commands must prevent elements being added or removed, to protect this loop. But LOCK_active_mi also needs to protect elements from going away, so that they can be safely accessed in the many different places in the code that do this. Of course there are a number of different ways this could be solved - just pointing out that it is something that need to be handled one way or the other.
Actually, there would still be the deadlock problem after introducing the new lock, because it is possible for a slave thread to run CHANGE MASTER! (and I think START SLAVE / RESET SLAVE as well). But that is probably a bug, does not seem good that this is possible. I guess these need a check to throw an ER_LOCK_OR_ACTIVE_TRANSACTION error, like STOP SLAVE.
Wouldn't the new lock LOCK_serialize_replication_admin_commands fix that?
How? It's exactly the same deadlock, no matter what we call the lock. STOP SLAVE holds lock, waits for sql thread to stop. SQL thread requests lock (eg. in CHANGE MASTER), deadlocking. But really, there is no reason a slave thread should be allowed to run replication adminitrative commands, it should just be an error so that the slave threads will never try to take LOCK_serialize_replication_admin_commands. Maybe that is what you meant.
I checked the function mysql_show_binlog_events() but could not find any access to Master_info after mysql_mutex_unlock(&LOCK_active_mi) was released.
Hm, no, it doesn't seem so, I guess I misread the code. So it is only MASTER_POS_WAIT() that has the problem, then. - Kristian.
Hi! On Mon, Apr 25, 2016 at 1:18 PM, Kristian Nielsen <knielsen@knielsen-hq.org> wrote:
Michael Widenius <michael.widenius@gmail.com> writes:
2. To protect access to master_info_index, eg. to prevent a Master_info from disappearing while it is accessed.
When reading about this, I think it would be better to have a counter in Master_info if someone is using it and only delete if if the counter is zero. Would be trivial to add an increment of the counter in get_master_info().
The counter could either be protected by the LOCK_active_mi or one of the mutexes in Master_info like data_lock.
Sounds reasonable. Will you do a patch? I guess this is code from the multisource feature, so you are the one familiar with it (before multi-source, the Master_info could not be created and deleted dynamically, could it?)
I can start by doing a patch for protecting get_master_info. Will do that tomorrow. Regards, Monty
participants (2)
-
Kristian Nielsen
-
Michael Widenius