From 78084fa747f373f2b404c3cb543d19f439100d9e Mon Sep 17 00:00:00 2001 From: Eugene Kosov Date: Wed, 8 Sep 2021 16:45:11 +0600 Subject: [PATCH] TSAN: data race on vptr (ctor/dtor vs virtual call) Read of size 8 at 0x7fecf2e75fc8 by thread T2 (mutexes: write M1318): #0 tpool::thread_pool_generic::submit_task(tpool::task*) /tpool/tpool_generic.cc:823:9 (mariadbd+0x25fd2d2) #1 (anonymous namespace)::aio_uring::thread_routine((anonymous namespace)::aio_uring*) /tpool/aio_liburing.cc:173:20 (mariadbd+0x260b21b) #2 void std::__invoke_impl(std::__invoke_other, void (*&&)((anonymous namespace)::aio_uring*), (anonymous namespace)::aio_uring*&&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/invoke.h:61:14 (mariadbd+0x260c62a) #3 std::__invoke_result::type std::__invoke(void (*&&)((anonymous namespace)::aio_uring*), (anonymous namespace)::aio_uring*&&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/invoke.h:96:14 (mariadbd+0x260c4ba) #4 void std::thread::_Invoker >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/std_thread.h:253:13 (mariadbd+0x260c442) #5 std::thread::_Invoker >::operator()() /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/std_thread.h:260:11 (mariadbd+0x260c3c5) #6 std::thread::_State_impl > >::_M_run() /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/std_thread.h:211:13 (mariadbd+0x260c189) #7 (libstdc++.so.6+0xd230f) Previous write of size 8 at 0x7fecf2e75fc8 by main thread: #0 tpool::task::task(void (*)(void*), void*, tpool::task_group*) /tpool/task.cc:40:46 (mariadbd+0x260a138) #1 tpool::aiocb::aiocb() /tpool/tpool.h:147:13 (mariadbd+0x2355943) #2 void std::_Construct(tpool::aiocb*) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_construct.h:109:38 (mariadbd+0x2355845) #3 tpool::aiocb* std::__uninitialized_default_n_1::__uninit_default_n(tpool::aiocb*, unsigned long) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_uninitialized.h:579:3 (mariadbd+0x235576c) #4 tpool::aiocb* std::__uninitialized_default_n(tpool::aiocb*, unsigned long) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_uninitialized.h:638:14 (mariadbd+0x23556e9) #5 tpool::aiocb* std::__uninitialized_default_n_a(tpool::aiocb*, unsigned long, std::allocator&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_uninitialized.h:704:14 (mariadbd+0x2355641) #6 std::vector >::_M_default_initialize(unsigned long) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:1606:4 (mariadbd+0x2354f3d) #7 std::vector >::vector(unsigned long, std::allocator const&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:512:9 (mariadbd+0x2354a19) #8 tpool::cache::cache(unsigned long, tpool::cache_notification_mode) /tpool/tpool_structs.h:73:20 (mariadbd+0x2354784) #9 io_slots::io_slots(int, int) /storage/innobase/os/os0file.cc:93:3 (mariadbd+0x235343b) #10 os_aio_init() /storage/innobase/os/os0file.cc:3780:22 (mariadbd+0x234ebce) #11 srv_start(bool) /storage/innobase/srv/srv0start.cc:1190:6 (mariadbd+0x256720c) #12 innodb_init(void*) /storage/innobase/handler/ha_innodb.cc:4188:8 (mariadbd+0x1ed3bda) #13 ha_initialize_handlerton(st_plugin_int*) /sql/handler.cc:659:31 (mariadbd+0xf7be06) #14 plugin_initialize(st_mem_root*, st_plugin_int*, int*, char**, bool) /sql/sql_plugin.cc:1463:9 (mariadbd+0x160fa1b) #15 plugin_init(int*, char**, int) /sql/sql_plugin.cc:1756:15 (mariadbd+0x160f07f) #16 init_server_components() /sql/mysqld.cc:5043:7 (mariadbd+0xd70fb2) #17 mysqld_main(int, char**) /sql/mysqld.cc:5655:7 (mariadbd+0xd6a9d7) #18 main /sql/main.cc:34:10 (mariadbd+0xd65d18) I think the report is incorrect: it's not possible to have such a race condition. I've checked it by reading the code and putting assertions. Namely, no aio I/O is possible before the end of os_aio_init(). Most probably it's some bug in TSAN. But the patch fixes around 5 related reports and this is a step toward TSAN usefullness. Currently it reports too much noise. std::unique_ptr is a step toward https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r11-avoid-calling-new-and-delete-explicitly There is no std::make_unique() in C++11, however. --- storage/innobase/os/os0file.cc | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/storage/innobase/os/os0file.cc b/storage/innobase/os/os0file.cc index 1df40b0525f..76212f9b9b0 100644 --- a/storage/innobase/os/os0file.cc +++ b/storage/innobase/os/os0file.cc @@ -80,6 +80,7 @@ Created 10/21/1995 Heikki Tuuri #include #include +#include /* Per-IO operation environment*/ class io_slots @@ -133,8 +134,8 @@ public: } }; -static io_slots *read_slots; -static io_slots *write_slots; +static std::unique_ptr read_slots; +static std::unique_ptr write_slots; /** Number of retries for partial I/O's */ constexpr ulint NUM_RETRIES_ON_PARTIAL_IO = 10; @@ -3744,6 +3745,10 @@ int os_aio_init() int max_read_events= int(srv_n_read_io_threads * OS_AIO_N_PENDING_IOS_PER_THREAD); int max_events= max_read_events + max_write_events; + + read_slots.reset(new io_slots(max_read_events, srv_n_read_io_threads)); + write_slots.reset(new io_slots(max_write_events, srv_n_write_io_threads)); + int ret; #if LINUX_NATIVE_AIO if (srv_use_native_aio && !is_linux_native_aio_supported()) @@ -3774,11 +3779,12 @@ disable: } #endif - if (!ret) + if (ret) { - read_slots= new io_slots(max_read_events, srv_n_read_io_threads); - write_slots= new io_slots(max_write_events, srv_n_write_io_threads); + read_slots.reset(); + write_slots.reset(); } + return ret; } @@ -3786,10 +3792,8 @@ disable: void os_aio_free() { srv_thread_pool->disable_aio(); - delete read_slots; - delete write_slots; - read_slots= nullptr; - write_slots= nullptr; + read_slots.reset(); + write_slots.reset(); } /** Wait until there are no pending asynchronous writes. */ @@ -3879,7 +3883,7 @@ func_exit: } compile_time_assert(sizeof(IORequest) <= tpool::MAX_AIO_USERDATA_LEN); - io_slots* slots= type.is_read() ? read_slots : write_slots; + io_slots* slots= type.is_read() ? read_slots.get() : write_slots.get(); tpool::aiocb* cb = slots->acquire(); cb->m_buffer = buf;