Skip to content

MDEV-15111 - fix simple InnoDB data race #582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kevgs wants to merge 5 commits into from
Closed

MDEV-15111 - fix simple InnoDB data race #582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
52fd74b
fix data races
kevgs Jan 31, 2018
5cb0f4f
move code to a proper place
kevgs Mar 14, 2018
e8696e5
make srv_running accesses atomical, remove unneeded atomic accesses f…
kevgs Mar 19, 2018
bd4fada
move code to mutex protected place
kevgs Mar 19, 2018
86b4a7b
unlock mutex
kevgs Mar 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions storage/innobase/handler/ha_innodb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ is_partition(

/** Signal to shut down InnoDB (NULL if shutdown was signaled, or if
running in innodb_read_only mode, srv_read_only_mode) */
volatile st_my_thread_var *srv_running;
st_my_thread_var *srv_running;
/** Service thread that waits for the server shutdown and stops purge threads.
Purge workers have THDs that are needed to calculate virtual columns.
This THDs must be destroyed rather early in the server shutdown sequence.
Expand All @@ -332,12 +332,16 @@ thd_destructor_proxy(void *)
myvar->current_cond = &thd_destructor_cond;

mysql_mutex_lock(&thd_destructor_mutex);
srv_running = myvar;
my_atomic_storeptr_explicit(&srv_running, myvar,
MY_MEMORY_ORDER_RELAXED);

@svoj svoj Mar 7, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of alright, but FWICS it is just a half of the fix. Though probably it is acceptable.

A few lines below there's second assignment srv_running = NULL. As well as two loads in srv0start.cc and srv0srv.cc that are most probably affected by data race as well.

@kevgs kevgs Mar 19, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made all accesses atomical.

/* wait until the server wakes the THD to abort and die */
while (!srv_running->abort)
while (!my_atomic_loadptr_explicit(&srv_running,
MY_MEMORY_ORDER_RELAXED)
->abort)

@svoj svoj Mar 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted this atomic load: no other thread can update srv_running.

mysql_cond_wait(&thd_destructor_cond, &thd_destructor_mutex);
mysql_mutex_unlock(&thd_destructor_mutex);
srv_running = NULL;
my_atomic_storeptr_explicit(&srv_running, NULL,
MY_MEMORY_ORDER_RELAXED);

while (srv_fast_shutdown == 0 &&
(trx_sys_any_active_transactions() ||
Expand Down Expand Up @@ -4333,7 +4337,8 @@ innobase_init(
mysql_thread_create(thd_destructor_thread_key,
&thd_destructor_thread,
NULL, thd_destructor_proxy, NULL);
while (!srv_running)
while (!my_atomic_loadptr_explicit(&srv_running,
MY_MEMORY_ORDER_RELAXED))
os_thread_sleep(20);
}

Expand Down Expand Up @@ -4427,10 +4432,12 @@ innobase_end(handlerton*, ha_panic_function)
hash_table_free(innobase_open_tables);
innobase_open_tables = NULL;

if (!abort_loop && srv_running) {
st_my_thread_var* running = my_atomic_loadptr_explicit(
&srv_running, MY_MEMORY_ORDER_RELAXED);
if (!abort_loop && running) {
// may be UNINSTALL PLUGIN statement
srv_running->abort = 1;
mysql_cond_broadcast(srv_running->current_cond);
running->abort = 1;
mysql_cond_broadcast(running->current_cond);
}

if (!srv_read_only_mode) {
Expand Down Expand Up @@ -17764,7 +17771,9 @@ fast_shutdown_validate(

uint new_val = *reinterpret_cast<uint*>(save);

if (srv_fast_shutdown && !new_val && !srv_running) {
if (srv_fast_shutdown && !new_val
&& !my_atomic_loadptr_explicit(&srv_running,
MY_MEMORY_ORDER_RELAXED)) {
return(1);
}

Expand Down
3 changes: 2 additions & 1 deletion storage/innobase/include/ib0mutex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,8 @@ struct TTASEventMutex {
int32 state() const
UNIV_NOTHROW
{
return(m_lock_word);
return(my_atomic_load32_explicit(&m_lock_word,
MY_MEMORY_ORDER_RELAXED));

@svoj svoj Mar 7, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, thanks!

}

/** The event that the mutex will wait in sync0arr.cc
Expand Down
2 changes: 1 addition & 1 deletion storage/innobase/include/srv0srv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ extern uint srv_fast_shutdown; /*!< If this is 1, do not do a

/** Signal to shut down InnoDB (NULL if shutdown was signaled, or if
running in innodb_read_only mode, srv_read_only_mode) */
extern volatile st_my_thread_var *srv_running;
extern st_my_thread_var *srv_running;

extern ibool srv_innodb_status;

Expand Down
23 changes: 14 additions & 9 deletions storage/innobase/srv/srv0srv.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1170,7 +1170,16 @@ srv_refresh_innodb_monitor_stats(void)
{
mutex_enter(&srv_innodb_monitor_mutex);

srv_last_monitor_time = time(NULL);
time_t current_time = time(NULL);

if (difftime(current_time, srv_last_monitor_time) > 60) {

@svoj svoj Mar 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I inverted this condition, so that original meaning retained.

/* We referesh InnoDB Monitor values so that averages are
printed from at most 60 last seconds */

@svoj svoj Mar 19, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to unlock mutex here, right?

@kevgs kevgs Mar 19, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Sorry once more.

@svoj svoj Mar 19, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will go through this patch once again soon. So far looks good.

mutex_exit(&srv_innodb_monitor_mutex);
return;
}

srv_last_monitor_time = current_time;

os_aio_refresh_stats();

Expand Down Expand Up @@ -1792,6 +1801,8 @@ DECLARE_THREAD(srv_monitor_thread)(void*)
}
}

srv_refresh_innodb_monitor_stats();

if (srv_shutdown_state != SRV_SHUTDOWN_NONE) {
goto exit_func;
}
Expand Down Expand Up @@ -1863,13 +1874,6 @@ DECLARE_THREAD(srv_error_monitor_thread)(void*)
old_lsn = new_lsn;
}

if (difftime(time(NULL), srv_last_monitor_time) > 60) {
/* We referesh InnoDB Monitor values so that averages are
printed from at most 60 last seconds */

srv_refresh_innodb_monitor_stats();
}

/* Update the statistics collected for deciding LRU
eviction policy. */
buf_LRU_stat_update();
Expand Down Expand Up @@ -2938,7 +2942,8 @@ srv_purge_wakeup()

srv_release_threads(SRV_WORKER, n_workers);
}
} while (!srv_running
} while (!my_atomic_loadptr_explicit(&srv_running,
MY_MEMORY_ORDER_RELAXED)
&& (srv_sys.n_threads_active[SRV_WORKER]
|| srv_sys.n_threads_active[SRV_PURGE]));
}
Expand Down
3 changes: 2 additions & 1 deletion storage/innobase/srv/srv0start.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2840,7 +2840,8 @@ srv_shutdown_bg_undo_sources()
void
innodb_shutdown()
{
ut_ad(!srv_running);
ut_ad(!my_atomic_loadptr_explicit(&srv_running,
MY_MEMORY_ORDER_RELAXED));
ut_ad(!srv_undo_sources);

switch (srv_operation) {
Expand Down