MDEV-36234: Add innodb_linux_aio #3976
Conversation
|
|
grooverdan
force-pushed
the
MDEV-36234
branch
3 times, most recently
from
5157b6e to
6ac340d
Compare
There was a problem hiding this comment.
Copilot reviewed 14 out of 21 changed files in this pull request and generated 2 comments.
Files not reviewed (7)
- debian/autobake-deb.sh: Language not supported
- debian/rules: Language not supported
- mysql-test/mariadb-test-run.pl: Language not supported
- mysql-test/suite/sys_vars/r/innodb_linux_aio_basic.result: Language not supported
- mysql-test/suite/sys_vars/t/innodb_linux_aio_basic.test: Language not supported
- mysql-test/suite/sys_vars/t/sysvars_innodb.test: Language not supported
- tpool/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (3)
tpool/tpool_generic.cc:220
- Please verify that the removal of m_group_enqueued is intentional; if it was tracking any necessary metrics, ensure these metrics are either no longer needed or are correctly handled elsewhere.
unsigned long long m_group_enqueued;
storage/innobase/handler/ha_innodb.cc:19516
- There is a spelling mistake in the documentation: 'implementaiton' should be corrected to 'implementation'.
Specifies which InnoDB AIO implementaiton should used. Possible value are "auto" ...
tpool/aio_linux.cc:120
- [nitpick] Double-check that using reinterpret_cast here is safe and that event.obj is always of type aiocb*. This cast assumes a specific memory layout; consider adding a static_assert or runtime check if feasible.
aiocb *iocb= reinterpret_cast<aiocb*>(event.obj);
tpool/tpool_generic.cc
Outdated
| @@ -300,12 +299,12 @@ class thread_pool_generic : public thread_pool | |||
| void wait_begin() override; | |||
| void wait_end() override; | |||
| void submit_task(task *task) override; | |||
| aio *create_native_aio(int max_io) override | |||
| aio *create_native_aio(int max_io, aio_implementation implementation) override | |||
| { | |||
| #ifdef _WIN32 | |||
| return create_win_aio(this, max_io); | |||
There was a problem hiding this comment.
Ensure that all callers of create_native_aio have been updated to pass the appropriate aio_implementation argument, to maintain consistency with the new function signature.
| return create_win_aio(this, max_io); | |
| return create_win_aio(this, max_io, implementation); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
/home/buildbot/amd64-debian-11-msan-clang-16/build/tpool/tpool_generic.cc:307:12: error: no matching function for call to 'create_linux_aio'
return create_linux_aio(this, max_io, implementation);
^~~~~~~~~~~~~~~~
/home/buildbot/amd64-debian-11-msan-clang-16/build/tpool/tpool_generic.cc:45:8: note: candidate function not viable: requires 2 arguments, but 3 were provided
aio *create_linux_aio(thread_pool *, int) { return nullptr; };
There was a problem hiding this comment.
per MDEV I did an update on bb-10.6-MDEV-36234-pkgtest (which is actually 10.11, opps) that fixes this difference. Is probably enough for testing (resizing has a fault if i remember) though I need to take @vaintroub's feedback and refactor the handling into tpool.
There was a problem hiding this comment.
Fixed in 322b6a4. I could reproduce this by the following:
cmake -DCMAKE_DISABLE_FIND_PACKAGE_{URING,LIBAIO}=ON …There was a problem hiding this comment.
The Copilot warning is actually about -Wunused-parameter on Windows.
storage/innobase/os/os0file.cc
Outdated
| max_events, | ||
| tpool::OS_AIO); | ||
| if (!ret && !is_linux_native_aio_supported()) | ||
| { |
There was a problem hiding this comment.
[nitpick] Clarify the fallback behavior when native AIO is unsupported. An inline comment explaining why ret is set to 1 in this case would improve maintainability.
| { | |
| { | |
| int // Fallback: Set ret to 1 to indicate that native AIO is unsupported, | |
| // and we should proceed with non-native AIO methods. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
In fact, this additional check should better be implemented inside tpool. I’m not sure if the check is actually relevant. @vaintroub, what do you think?
There was a problem hiding this comment.
According to strace -p $(pgrep mariadbd) in my environment, libaio on tmpfs will work just fine; I did see io_submit() calls for page writes on InnoDB shutdown.
What was the is_linux_native_aio_supported() originally about? It had been refactored in mysql/mysql-server@98909ce in 2015 and originally added in mysql/mysql-server@0da310b in 2012 to address MySQL Bug #58421 InnoDB temporary table crash when tmpdir on Linux tmpfs. The system that is reported there is CentOS 5.5, and it is about temporary tables, which actually were handled mostly as persistent tables back then. I believe that this check got useless in MySQL WL#6560: InnoDB: separate tablespace for innodb-temp-tables. Temporary tables would be written to the innodb_temp_data_file_path (default file name ibtmp1), no longer into any /tmp/#sql-something.ibd file. The error was that io_submit() would return EINVAL when writing to a file on tmpfs.
In https://stackoverflow.com/questions/34572559/asynchronous-io-io-submit-latency-in-ubuntu-linux/46377629 the accepted answer from 2017 suggests that libaio would always work (after io_setup() succeeded), but the requests could silently be converted to synchronous ones when O_DIRECT is not being used. This does not seem to entirely hold for CentOS 5, but CentOS 5, which was originally released in 2007, reached its end-of-life in March 2017. I find it reasonable to expect that any currently maintained GNU/Linux distributions, even including the recently-EOLed RHEL 7, would just work. In the worst case, someone would have to specify innodb_use_native_aio=OFF in order to be able to start up InnoDB when their data directory or the innodb_temp_data_file_path are in tmpfs. Both would seem to be rather insane choices in a production environment.
I hoped that we could test RHEL 7 on our CI system, which runs the mtr tests on /dev/shm (tmpfs), but it turns out that https://buildbot.mariadb.org/#/builders/222 reports a way too new kernel version (that of the Docker host system):
Linux 2930783f10e5 6.8.0-59-generic #61-Ubuntu SMP PREEMPT_DYNAMIC Fri Apr 11 23:16:11 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
In any case, I will remove is_linux_native_aio_supported(). It would be useful to test that on an actual RHEL 7 kernel to be on the safe side. After all, the problem was claimed to affect "old kernels" already in 2012, and RHEL 7 was released in 2014.
storage/innobase/os/os0file.cc
Outdated
| srv_use_native_aio= false; | ||
| ret= srv_thread_pool->configure_aio(false, max_events); | ||
| } | ||
| switch (srv_linux_aio_method) |
There was a problem hiding this comment.
Suggestion : I think most of this if linux fallback logic (uring to libaio) could be handled inside tpool::configure_aio itself, you got the OS_DEFAULT for it, right? Will be less clutter here and you can put OS_DEFAULT to use
There was a problem hiding this comment.
sql_print_warning() is not being used in tpool. What we can do is to remove some #ifdef LINUX_NATIVE_AIO checks that I believe are redundant by now.
dr-m
force-pushed
the
MDEV-36234
branch
2 times, most recently
from
322b6a4 to
1e03323
Compare
dr-m
changed the title
There was a problem hiding this comment.
With Native AIO only the mariabackup tests failed:
$ mysql-test/mtr --strace --client-strace mariabackup.slave_provision_nolock
Logging: /home/dan/repos/mariadb-server-10.11/mysql-test/mariadb-test-run.pl --strace --client-strace mariabackup.slave_provision_nolock
VS config:
vardir: /home/dan/repos/build-mariadb-server-10.11/mysql-test/var
Checking leftover processes...
Removing old var directory...
Creating var directory '/home/dan/repos/build-mariadb-server-10.11/mysql-test/var'...
Checking supported features...
MariaDB Version 10.11.12-MariaDB
- SSL connections supported
- binaries built with wsrep patch
Collecting tests...
Installing system database...
==============================================================================
TEST RESULT TIME (ms) or COMMENT
--------------------------------------------------------------------------
worker[01] Using MTR_BUILD_THREAD 300, with reserved ports 19000..19029
mariabackup.slave_provision_nolock [ fail ]
Test ended at 2025-05-21 16:43:58
CURRENT_TEST: mariabackup.slave_provision_nolock
Warning: /home/dan/repos/build-mariadb-server-10.11/client//mariadb-test: unknown option '--loose-debug-gdb'
Warning: /home/dan/repos/build-mariadb-server-10.11/client//mariadb-test: unknown option '--loose-skip-stack-trace'
[00] 2025-05-21 16:38:48 Connecting to MariaDB server host: localhost, user: root, password: set, port: 19000, socket: /home/dan/repos/build-mariadb-server-10.11/mysql-test/var/tmp/mysqld.1.sock
[00] 2025-05-21 16:38:48 Using server version 10.11.12-MariaDB-log
....
[00] 2025-05-21 16:43:51 Finished backing up non-InnoDB tables and files
[00] 2025-05-21 16:43:51 Waiting for log copy thread to read lsn 282202708
[00] 2025-05-21 16:43:52 Retrying read of log at LSN=48736
[00] 2025-05-21 16:43:53 Retrying read of log at LSN=48736
[00] 2025-05-21 16:43:54 Retrying read of log at LSN=48736
[00] 2025-05-21 16:43:55 Retrying read of log at LSN=48736
[00] 2025-05-21 16:43:56 Was only able to copy log from 48736 to 48736, not 282202708; try increasing innodb_log_file_size
mariabackup: Stopping log copying thread[00] 2025-05-21 16:43:56 Retrying read of log at LSN=48736
.
mysqltest: At line 100: exec of '/home/dan/repos/build-mariadb-server-10.11/extra/mariabackup/mariabackup --defaults-file=/home/dan/repos/build-mariadb-server-10.11/mysql-test/var/my.cnf --backup --target-dir=/home/dan/repos/build-mariadb-server-10.11/mysql-test/var/tmp/backup --no-lock' failed, error: 256, status: 1, errno: 11
Output from before failure:
*** Doing backup...
The result from queries just before the failure was:
call mtr.add_suppression("Can't init tc log");
call mtr.add_suppression("Aborting");
# restart
RESET MASTER;
CREATE TABLE t1(a varchar(60) PRIMARY KEY, b VARCHAR(60)) ENGINE INNODB;
INSERT INTO t1 VALUES(1, NULL);
CREATE TABLE t2 (val INT) ENGINE=InnoDB;
INSERT INTO t2 VALUES (0);
connect con1,localhost,root,,;
*** Start a background load...
CALL gen_load();
connection default;
*** Doing backup...
- saving
Once io_uring is linked it functions correctly - on and off --mem (ext4 alternative).
I find myself on a old machine - 6.4.12-200.fc38.x86_64 - will test again after kernel upgrade (because fc38 is definitely EOL)
$ firejail --noprofile '--seccomp=io_uring_setup:ENOSYS,!io_setup' sql/mariadbd --no-defaults --bootstrap --datadir=$(mktemp -d)
Seccomp list in: io_uring_setup:ENOSYS,!io_setup, check list: @default-keep, prelist: io_uring_setup:ENOSYS,unknown,
Parent pid 655069, child pid 655070
Seccomp list in: io_uring_setup:ENOSYS,!io_setup, check list: @default-keep, prelist: io_uring_setup:ENOSYS,unknown,
Child process initialized in 18.72 ms
2025-05-21 17:49:15 0 [Note] Starting MariaDB 10.11.12-MariaDB source revision 1e033232260abae44089544877e1fc4462c8219c server_uid tluFI/7BR1nZc5vueEzXfY8fsIE= as process 4
2025-05-21 17:49:15 0 [Note] InnoDB: The first data file './ibdata1' did not exist. A new tablespace will be created!
2025-05-21 17:49:15 0 [Note] InnoDB: Compressed tables use zlib 1.2.13
2025-05-21 17:49:15 0 [Note] InnoDB: Number of transaction pools: 1
2025-05-21 17:49:15 0 [Note] InnoDB: Using crc32 + pclmulqdq instructions
2025-05-21 17:49:15 0 [Warning] mariadbd: io_uring_queue_init() failed with ENOSYS: check seccomp filters, and the kernel version (newer than 5.1 required)
2025-05-21 17:49:15 0 [Warning] InnoDB: io_uring failed: falling back to libaio
io_getevents returned -1
250521 17:49:15 [ERROR] sql/mariadbd got signal 6 ;
Bit of an eccentric case that io_getevents failed.
include/my_time.h
Outdated
| @@ -230,7 +230,6 @@ static inline longlong sec_part_unshift(longlong second_part, uint digits) | |||
| /* Date/time rounding and truncation functions */ | |||
| static inline long my_time_fraction_remainder(long nr, uint decimals) | |||
| { | |||
| DBUG_ASSERT(decimals <= TIME_SECOND_PART_DIGITS); | |||
| return nr % (long) log_10_int[TIME_SECOND_PART_DIGITS - decimals]; | |||
There was a problem hiding this comment.
minor: not sure why this is included.
There was a problem hiding this comment.
That change was included accidentally. Removing the assertion removes the following compilation warning for me:
FAILED: sql/CMakeFiles/sql.dir/field.cc.o
/usr/bin/c++ -DHAVE_CONFIG_H -DHAVE_EVENT_SCHEDULER -DHAVE_POOL_OF_THREADS -DMYSQL_SERVER -D_FILE_OFFSET_BITS=64 -I/mariadb/10.11/wsrep-lib/include -I/mariadb/10.11/wsrep-lib/wsrep-API/v26 -I/dev/shm/10.11/include -I/mariadb/10.11/include/providers -I/mariadb/10.11/include -I/mariadb/10.11/sql -I/dev/shm/10.11/sql -I/mariadb/10.11/tpool -O2 -march=native -pie -fPIC -fstack-protector --param=ssp-buffer-size=4 -fPIC -g -DPROTECT_STATEMENT_MEMROOT -DENABLED_DEBUG_SYNC -D_GLIBCXX_DEBUG -D_GLIBCXX_ASSERTIONS -ggdb3 -DSAFE_MUTEX -DTRASH_FREED_MEMORY -Wall -Wenum-compare -Wenum-conversion -Wextra -Wmissing-braces -Wno-format-truncation -Wno-init-self -Wno-nonnull-compare -Wno-unused-parameter -Wnon-virtual-dtor -Woverloaded-virtual -Wsuggest-override -Wvla -Wwrite-strings -Werror -fno-operator-names -std=gnu++11 -DHAVE_OPENSSL -DOPENSSL_API_COMPAT=0x10100000L -MD -MT sql/CMakeFiles/sql.dir/field.cc.o -MF sql/CMakeFiles/sql.dir/field.cc.o.d -o sql/CMakeFiles/sql.dir/field.cc.o -c /mariadb/10.11/sql/field.cc
In file included from /mariadb/10.11/sql/structs.h:26,
from /mariadb/10.11/sql/handler.h:34,
from /mariadb/10.11/sql/log.h:20,
from /mariadb/10.11/sql/sql_class.h:28,
from /mariadb/10.11/sql/procedure.h:31,
from /mariadb/10.11/sql/sql_select.h:31,
from /mariadb/10.11/sql/field.cc:32:
In function 'long int my_time_fraction_remainder(long int, uint)',
inlined from 'long int Timestamp::fraction_remainder(uint) const' at /mariadb/10.11/sql/sql_type.h:2815:38,
inlined from 'Timestamp& Timestamp::round(uint, time_round_mode_t, int*)' at /mariadb/10.11/sql/sql_type.h:2831:7,
inlined from 'virtual int Field_timestamp::store_timestamp_dec(const timeval&, uint)' at /mariadb/10.11/sql/field.cc:5411:42:
/mariadb/10.11/include/my_time.h:234:67: error: array subscript 4294901767 is above array bounds of 'ulonglong [20]' {aka 'long long unsigned int [20]'} [-Werror=array-bounds=]
234 | return nr % (long) log_10_int[TIME_SECOND_PART_DIGITS - decimals];
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
/mariadb/10.11/include/my_time.h: In member function 'virtual int Field_timestamp::store_timestamp_dec(const timeval&, uint)':
/mariadb/10.11/include/my_time.h:30:38: note: while referencing 'log_10_int'
30 | extern MYSQL_PLUGIN_IMPORT ulonglong log_10_int[20];
| ^~~~~~~~~~
I’m not sure if it is related, but there is a relationship between |
I should of explained this more - the On retesting the mariabackup, with a firejail disable (below) and with libaio natively as option (validated by server error logs) both where passing mariabackup tests. No idea what was failing yesterday. On a uring enabled build you can disable libaio as follows to test. |
tpool/tpool.h
Outdated
| @@ -256,6 +277,10 @@ class thread_pool | |||
| { | |||
| m_aio.reset(); | |||
| } | |||
| const char *get_implementation() const | |||
There was a problem hiding this comment.
please call it get_aio_implementation. It is not threadpool implementation
| @@ -244,7 +260,12 @@ class thread_pool | |||
| assert(m_aio); | |||
| if (use_native_aio) | |||
| { | |||
| auto new_aio = create_native_aio(max_io); | |||
| const aio_implementation impl= | |||
| #ifdef LINUX_NATIVE_AIO | |||
There was a problem hiding this comment.
Can this ifdef be put inside create_native_aio? So that LINUX_NATIVE_AIO would not have to be defined by the API caller, and can be an internal detail of the tpool.
There was a problem hiding this comment.
I can’t see how it could be moved inside create_native_aio(). Here the intention is to replace m_aio with an object where some attributes differ, but the type of the implementation is preserved. The function create_native_aio() may also be invoked for constructing the very first thread_pool::m_aio. This is the cleanest solution that I was able to come up with. Can you post a tested patch that corresponds to what you are suggesting?
There was a problem hiding this comment.
Ok, will write and post, but this gonna need some time (since I'd need to spend time in an environment I'm not most productive in)
storage/innobase/os/os0file.cc
Outdated
| switch (srv_linux_aio_method) { | ||
| case SRV_LINUX_AIO_AUTO: | ||
| case SRV_LINUX_AIO_IO_URING: | ||
| #ifdef HAVE_URING |
There was a problem hiding this comment.
I'd prefer HAVE_URING and HAVE_LINUX_NATIVE_AIO to be contained inside tpool and not used outside of it.
my_printf_error with ME_WARNING inside tpool is used by uring already, though I think fprintf is less dependencies. if you need some reporting
There was a problem hiding this comment.
Only for Linux, we fall back to innodb_use_native_aio=OFF when a native interface cannot be configured. I think this is a bad practice, and therefore I wouldn’t want to introduce such logic for other platforms. I think that it is unavoidable to have the references here. But we can move the message output from srv_start() to this function.
There was a problem hiding this comment.
Only on Linux, innodb_use_native_aio=ON can fail in the first place. You do not need to apply logic for other OSes at all, even if introduce it, it won't change anything . However, as you already introduced aio_implementation, for the sake Linux-only, and put into tpool API, is there anything fundamental that will change if you put it into appropriate place.
| @@ -378,6 +378,10 @@ extern const char *innodb_checksum_algorithm_names[]; | |||
| extern TYPELIB innodb_checksum_algorithm_typelib; | |||
| extern const char *innodb_flush_method_names[]; | |||
| extern TYPELIB innodb_flush_method_typelib; | |||
| #ifdef __linux__ | |||
| extern const char *innodb_linux_aio_names[]; | |||
| extern TYPELIB innodb_linux_aio_typelib; | |||
There was a problem hiding this comment.
Do we want backup to choose AIO implementation? Does really which AIO it uses, is it performance relevant? Can't we just use default one? Or even simulated one?
There was a problem hiding this comment.
It could be relevant for mariadb-backup --prepare. I’ll try to test a MDEV-29911 or MDEV-34062 like scenario to assess the impact.
There was a problem hiding this comment.
I think that we should give users the option to choose the implementation also for mariadb-backup --prepare. I believe that it can be useful for some troubleshooting, and I do can’t foresee any risks that would be related to offering this choice.
This controls which linux implementation to use for innodb_use_native_aio=ON. innodb_linux_aio=auto is equivalent to innodb_linux_aio=io_uring when it is available, and falling back to innodb_linux_aio=aio when not. Debian packaging is no longer aio exclusive or uring, so for those older Debian or Ubuntu releases, its a remove_uring directive. For more recent releases, add mandatory liburing for consistent packaging. WITH_LIBAIO is now an independent option from WITH_URING. is_linux_native_aio_supported(): Remove. This had originally been added in mysql/mysql-server@0da310b in 2012 to fix an issue where io_submit() on CentOS 5.5 would return EINVAL for a /tmp/#sql*.ibd file associated with CREATE TEMPORARY TABLE. But, starting with commit 2e814d4 InnoDB temporary tables will be written to innodb_temp_data_file_path. The 2012 commit said that the error could occur on "old kernels". Any GNU/Linux distribution that we currently support should be based on a newer Linux kernel; for example, Red Hat Enterprise Linux 7 was released in 2014. This is joint work with Daniel Black and Vladislav Vaintroub.
Description
The parameter
innodb_linux_aiocontrols which Linux implementation to use forinnodb_use_native_aio=ON.innodb_linux_aio=autois equivalent toinnodb_linux_aio=io_uringwhen it is available, and falling back toinnodb_linux_aio=aiowhen not.Debian packaging is no longer aio exclusive or uring, so for those older Debian or Ubuntu releases, its a
remove_uringdirective. For more recent releases, add mandatory liburing for consistent packaging.WITH_LIBAIOis now an independent option fromWITH_URING.is_linux_native_aio_supported(): Remove. This had originally been added in mysql/mysql-server@0da310b in 2012 to fix an issue whereio_submit()on CentOS 5.5 would returnEINVALfor a/tmp/#sql*.ibdfile associated withCREATE TEMPORARY TABLE. But, starting with 2e814d4 InnoDB temporary tables will be written toinnodb_temp_data_file_path. The 2012 commit said that the error could occur on "old kernels". Any GNU/Linux distribution that we currently support should be based on a newer Linux kernel; for example, Red Hat Enterprise Linux 7 was released in 2014.This is joint work with @grooverdan and @vaintroub.
Release Notes
The parameter
innodb_linux_aiocontrols which Linux implementation to use forinnodb_use_native_aio=ON.innodb_linux_aio=autois equivalent toinnodb_linux_aio=io_uringwhen it is available, and falling back toinnodb_linux_aio=aiowhen not.How can this PR be tested?
This is rather well covered on https://buildbot.mariadb.org. The MemorySanitizer builders will disable both libaio and liburing. Unfortunately, our RHEL 7 builders seem to run on a mismatching Linux kernels, either 4.18 from RHEL 8 or 6.x from Debian.
Basing the PR against the correct MariaDB version
mainbranch.We are basing this on 10.11 instead of 10.6, with the assumption that there is no
liburingcapable GNU/Linux distribution that would by default ship MariaDB Server 10.6.PR quality check