MDEV-36234: Dual stack libaio/liburing #3976

dr-m · 2025-04-15T09:17:39Z

The Jira issue number for this PR is: MDEV-36234

Description

FIXME: Introduce a parameter for explicitly specifying which one to use: innodb_linux_aio=libaio/liburing/auto

FIXME: In srv_start(), indicate which implementation was chosen

Release Notes

TBD

How can this PR be tested?

TBD

Basing the PR against the correct MariaDB version

This is a new feature or a refactoring, and the PR is based against the main branch.
This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

We may want to base this on 10.11 instead of 10.6, if there is no liburing capable GNU/Linux distribution that would by default ship 10.6.

PR quality check

I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

FIXME: Introduce a parameter for explicitly specifying which one to use: innodb_linux_aio=libaio/liburing/auto FIXME: In srv_start(), indicate which implementation was chosen

CLAassistant · 2025-04-15T09:17:46Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ vaintroub
✅ grooverdan
❌ dr-m
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Debian packaging is no longer aio exclusive or uring, so for those older versions, its a remove_uring directive. Add manditory liburing for consistent packaging, if workers need to be updated that can be handled. WITH_LIBAIO is now an independent option from WITH_URING.

Add abstraction to return implementation name from threadpool.

Remove version check on the kernel as it now corresponds to a working RHEL9 kernel and the problem was only there in pre-release kernels that shouldn't have been used in production. This reverts commit 3dc0d88.

...when using io_uring on a potentially affected kernel" Remove version check on the kernel as it now corresponds to a working RHEL9 kernel and the problem was only there in pre-release kernels that shouldn't have been used in production. This reverts commit 1193a79.

…on name

Noted in MDEV-36542 that it wasn't used and the compile warning -Wunused-private-field is legitimate. Review: Vladislav Vaintroub

As noted by Jens Axobe, errno isn't set, it is returned by the io_uring_queue_init function.

This controls which linux implementation to use under the assumption that innodb_native_aio is true. "auto" select liburing first, if fails, tries libaio with a filesystem test, and if fails, falls back to no-async. "aio" starts is per auto, but skips liburing step.

The examination of the AIO method needs to occur after the os_aio_init has been completed.

vaintroub

Fixed Windows compilation. Other than that, and minor commit on unnecessary attribute(unused), I do not have any objections

vaintroub · 2025-04-16T08:43:04Z

tpool/aio_linux.cc

+#ifdef HAVE_URING
+aio *create_libaio(thread_pool *pool, int max_io)
+#else
+aio *create_linux_aio(thread_pool *pool, int max_io, aio_implementation implementation __attribute__((unused)))


Drop the implementation __attribute__((unused)) from this line. Just aio_implementation. It's cleaner

It is C++, if you do not have to name parameter, if you do not use them.

Copilot

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);

Copilot · 2025-04-16T08:58:57Z

tpool/tpool_generic.cc

@@ -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);


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.

Suggested change

return create_win_aio(this, max_io);

return create_win_aio(this, max_io, implementation);

Copilot · 2025-04-16T08:58:57Z

storage/innobase/os/os0file.cc

+                                          max_events,
+                                          tpool::OS_AIO);
+      if (!ret && !is_linux_native_aio_supported())
+      {


[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.

Suggested change

{

{

int // Fallback: Set ret to 1 to indicate that native AIO is unsupported,

// and we should proceed with non-native AIO methods.

vaintroub · 2025-04-16T09:25:55Z

storage/innobase/os/os0file.cc

-    srv_use_native_aio= false;
-    ret= srv_thread_pool->configure_aio(false, max_events);
-  }
+    switch (srv_linux_aio_method)


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

WIP MDEV-36234: Dual stack libaio/liburing

3968746

FIXME: Introduce a parameter for explicitly specifying which one to use: innodb_linux_aio=libaio/liburing/auto FIXME: In srv_start(), indicate which implementation was chosen

dr-m added the MariaDB Corporation label Apr 15, 2025

dr-m requested a review from vaintroub April 15, 2025 09:17

dr-m self-assigned this Apr 15, 2025

grooverdan force-pushed the MDEV-36234 branch 2 times, most recently from b1e9ea3 to 5157b6e Compare April 16, 2025 05:51

grooverdan added 8 commits April 16, 2025 17:09

MDEV-36234: Dual stack libaio/liburing - return implementation name

aa0d1bc

Add abstraction to return implementation name from threadpool.

MDEV-36182: Revert "MDEV-26674 workaround for mariadb-backup"

46c403a

Remove version check on the kernel as it now corresponds to a working RHEL9 kernel and the problem was only there in pre-release kernels that shouldn't have been used in production. This reverts commit 3dc0d88.

MDEV-34234: Dualstack aio/io_uring - mariabackup display implementati…

e1f8406

…on name

tpool: remove m_group_enqueued (unused)

ee0f8aa

Noted in MDEV-36542 that it wasn't used and the compile warning -Wunused-private-field is legitimate. Review: Vladislav Vaintroub

MDEV-36234: dualstack aio/io_uring - document EPERM error

7ed1f02

As noted by Jens Axobe, errno isn't set, it is returned by the io_uring_queue_init function.

MDEV-36234: dual stack aio/io_uring on MariaDB-backup

6ac340d

The examination of the AIO method needs to occur after the os_aio_init has been completed.

grooverdan force-pushed the MDEV-36234 branch from 5157b6e to 6ac340d Compare April 16, 2025 07:10

Fix compilation

847f834

vaintroub approved these changes Apr 16, 2025

View reviewed changes

vaintroub requested a review from Copilot April 16, 2025 08:58

Copilot AI reviewed Apr 16, 2025

View reviewed changes

vaintroub reviewed Apr 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDEV-36234: Dual stack libaio/liburing #3976

MDEV-36234: Dual stack libaio/liburing #3976

dr-m commented Apr 15, 2025

CLAassistant commented Apr 15, 2025 •

edited

Loading

vaintroub left a comment

vaintroub Apr 16, 2025

Copilot AI left a comment

Copilot AI Apr 16, 2025

Copilot AI Apr 16, 2025

vaintroub Apr 16, 2025

	return create_win_aio(this, max_io);
	return create_win_aio(this, max_io, implementation);

-      {
+      {
+  int 	// Fallback: Set ret to 1 to indicate that native AIO is unsupported,
+	// and we should proceed with non-native AIO methods.

MDEV-36234: Dual stack libaio/liburing #3976

Are you sure you want to change the base?

MDEV-36234: Dual stack libaio/liburing #3976

Conversation

dr-m commented Apr 15, 2025

Description

Release Notes

How can this PR be tested?

Basing the PR against the correct MariaDB version

PR quality check

CLAassistant commented Apr 15, 2025 • edited Loading

vaintroub left a comment

Choose a reason for hiding this comment

vaintroub Apr 16, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Apr 16, 2025

Choose a reason for hiding this comment

Copilot AI Apr 16, 2025

Choose a reason for hiding this comment

vaintroub Apr 16, 2025

Choose a reason for hiding this comment

CLAassistant commented Apr 15, 2025 •

edited

Loading