MDEV-34358 Encryption threads consume CPU when no work available

[Thirunarayanan]

Problem:

1. Encryption threads busy-wait when no work is available. When reaching
fil_system.space_list.end(), fil_crypt_return_iops() is called with
wake=true, causing pthread_cond_broadcast() to wake all threads
unnecessarily, leading to CPU waste.

2. Tablespaces with CLOSING/STOPPING flags (set during DDL operations)
are skipped during iteration. Since DDL completion doesn't wake
encryption threads, these spaces may never be encrypted if threads
sleep indefinitely.

3. For default_encrypt_list iteration, when spaces exist but none are
acquirable, threads need to wake others for
cooperative retry, but this case was not distinguished from
fil_system.space_list.end().

4. IOPS are allocated before searching for tablespaces, wasting resources
during iteration when no I/O occurs.

Solution:
========
1. Implement timed wait with exponential backoff (5s -> 10s -> 20s -> 40s
-> 60s, max 5 attempts) for fil_system.space_list iteration. After
~135 seconds, switch to indefinite wait. This periodically rechecks
for spaces that become available after DDL completes.

2. Use indefinite wait for default_encrypt_list iteration since other
threads will retry and wake when needed.

3. fil_space_t::next(): Add default_encrypt_list flag to distinguish between
the two iteration modes. Wake other threads only
when this flag is true (spaces exist but unacquirable).

4. Move IOPS allocation from before tablespace search to after finding a
   space that needs rotation.

5. Handle wake logic explicitly at call site based on default_encrypt_list flag.

rotate_thread_t changes
====================
- default_encrypt_list (bool): Indicates if default_encrypt_list has unacquirable spaces
- timed_wait_count (uint8_t): Counts consecutive timeouts for exponential backoff
- sleep_timeout_ms (uint16_t): Current timeout in ms (5s -> 60s max)
- wait_for_work(): Implements timed/indefinite wait based on iteration mode
- increase_sleep_timeout(): Doubles timeout up to 60s
- reset_sleep_timeout(): Resets timeout to 5s and clears count

Added debug-only status variables Innodb_encryption_timed_waits
and Innodb_encryption_indefinite_waits to track encryption thread
wait types, enabling verification that threads use timed wait with
exponential backoff instead of busy-waiting when idle.

The counters are incremented in rotate_thread_t::wait_for_work()
and exposed via SHOW STATUS in debug builds only. Also added a
debug sync point 'rotate_only_2_timed_waits' to reduce the timed
wait threshold from 5 to 2 for faster testing of the indefinite
wait transition.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[vuvova]

/gemini review

[gemini-code-assist]

Code Review

This pull request implements an exponential backoff mechanism for InnoDB encryption threads to reduce CPU usage when tablespaces are temporarily unavailable, such as during DDL operations. It introduces a versioning system for encryption settings to trigger iteration restarts and adds debug status variables to track wait behavior. The review feedback highlights several improvement opportunities: addressing dead code wrapped in #if 0, correcting the timeout threshold logic to match the intended behavior described in comments, removing redundant counter resets, and increasing the bit-width of the settings version counter to prevent potential overflow.

[gemini-code-assist]

Using uint16_t for a version counter is risky as it can wrap around relatively quickly (65535 changes). While unlikely to cause an issue in typical operation, using uint32_t or uint64_t is safer and generally has the same performance characteristics for atomic operations on modern architectures.

static Atomic_counter<uint32_t> fil_crypt_settings_version;

[Thirunarayanan]

If user changes innodb_encrypt_tables variable every hour then this variable may ran out in 7.5 years. If we restart the server in the mean time then this variable becomes 0. This datatype is enough for practical usage. I don't think sane people is going to toggle encryption for every hour.

[iMineLink]

While I agree with you Thiru, it's true that Atomic_counter<uint16_t> shows only one result in the new code. Atomic_counter<uint32_t> is more common, and behaves likely similar while being wider. I think it may be considered to use Atomic_counter<uint32_t>. It will maybe cause inefficiency in rotate_thread_t structure due to widening of rotate_thread_t::settings_version. Could you please check in gdb with ptype /o rotate_thread_t if there's any hole/padding that allows widening without size increase?

[dr-m]

I think that we need to consider two paths forward. I see that this pull request is currently targeting the 10.6 branch, to which only minimal changes should be done, given that the branch will soon reach its end of life.

There is also the hang MDEV-37946, which I expect we should be hitting when testing this extensively enough. I think that the hang needs to be fixed as part of a minimal fix; it should be fairly easy.

For newer branches, I would like to see a more comprehensive solution, which I am outlining below.

I’d like to see some testing of this subsystem. I have some doubts of the efficacy of the current multi-threaded implementation. We currently have multiple threads that can increase the size of buf_pool.flush_list, It could be the case that multiple threads are concurrently calling buf_flush_list_space(), which could repeatedly invalidate buf_pool.flush_hp.

As a starting point, I think that we have to test if there is any benefit of having multiple "encryption threads" which perform dummy writes to data pages. The name "encryption thread" is misleading, because the actual encryption will take place in the single buf_flush_page_cleaner() thread, in the function buf_page_encrypt(). Yes, this CPU intensive operation will be executed in a single thread!

How can it possibly be helpful to have multiple "producers" (fil_crypt_thread) that are dirtying the buffer pool, when we have a single "consumer" (buf_flush_page_cleaner) that is doing some very CPU intensive work (buf_page_encrypt() and computing CRC-32C)?

Could we implement tighter coupling between the current "encryption" (page-dirtying) and the buf_flush_page_cleaner()? That thread is already regularly scanning potentially the entire buffer pool. So, why not implement an "encryption step" that would process a number of pages that were missed by buf_flush_list_holding_mutex() and buf_flush_LRU()? Such a design could trivially follow the innodb_io_capacity budget and scale back the key rotation activity when there are bursts of application workload (some threads are blocked in buf_flush_wait() or buf_LRU_get_free_block()).

When a clean page that we wish to re-encrypt is in the buffer pool, the buf_flush_page_cleaner() thread can simply write a dummy log record and move the page to the buf_pool.flush_list.

When a page will need to be read into the buffer pool, the current implementation is invoking synchronous buf_page_read() via buf_page_get_gen(). It would be much better to submit asynchronous reads, implementing the logic with a future implementation of MDEV-11378 in mind. The read completion callback would have to detect that this is a special read for encryption, and it would write the dummy record so that the page will be re-encrypted.

The relevant parts of fil_space_rotate_state_t and key_state_t may have to be made part of fil_space_t or fil_space_crypt_t.

A final part of this would be to repurpose the parameter innodb_encrypt_threads to refer to worker tasks that the buf_flush_page_cleaner() would employ to encrypt pages or compute CRC-32C. The function buf_page_t::flush() would be split. Only initial part would be invoked by the buf_flush_page_cleaner() thread; the rest would be invoked by worker tasks, which would process a number of buf_page_t* from their queue.

[Thirunarayanan]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an exponential backoff mechanism for InnoDB encryption threads to optimize CPU usage and a versioning system to manage configuration changes, including updates to status variable tests. The review feedback identifies several improvements: using 32-bit integers for timeout and version counters to prevent overflow and wrap-around, correcting the wait logic for the default encryption list, resetting timeout states when work is found, and addressing a busy-wait loop in IOPS allocation.

[gemini-code-assist]

The variable sleep_timeout_ms should be changed to uint32_t to avoid potential overflow issues during calculations in increase_sleep_timeout(). When sleep_timeout_ms is 40,000, sleep_timeout_ms * 2 results in 80,000, which exceeds the uint16_t range and causes incorrect behavior when passed to std::min<uint16_t>.

  uint32_t sleep_timeout_ms= 5000;

[iMineLink]

The writes are bounded, no need to widen IMHO.

[gemini-code-assist]

As noted previously, sleep_timeout_ms * 2 can exceed 65,535. If sleep_timeout_ms is changed to uint32_t, the explicit template parameter for std::min can be removed or updated to ensure correct comparison.

	sleep_timeout_ms = std::min(sleep_timeout_ms * 2, 60000U);

[iMineLink]

Please, address this: you can widen sleep_timeout_ms to 32-bit before doing * 2 to avoid 16-bit overflow, then clamp it to the max of 60 seconds.

[iMineLink]

Or, change to sleep_timeout_s and avoid the /1000 in set_timespec(abstime, sleep_timeout_ms / 1000);.

[gemini-code-assist]

Using uint16_t for a version counter is risky in a long-running database server as it can wrap around relatively quickly (65,535 changes). While the logic only checks for inequality, using uint32_t is safer and more standard for versioning to avoid any collision during a single iteration.

static Atomic_counter<uint32_t> fil_crypt_settings_version;

[gemini-code-assist]

Update this to uint32_t to match the suggested change for the global fil_crypt_settings_version.

  uint32_t settings_version;

[gemini-code-assist]

There is a discrepancy between the code and the intended design described in the PR and the comments (lines 1131-1133). The code currently performs a timed wait when space == .end(), regardless of whether it's iterating the default_encrypt_list. To follow the stated design of using an indefinite wait for the default list (where other threads are expected to wake this one), the condition should check the default_encrypt_list flag.

		if (space == fil_system.space_list.end() && !default_encrypt_list) {

[Thirunarayanan]

Are you kidding me, right?
Please check the callers for wait_for_work()

                                if (thr.default_encrypt_list) {
                                        pthread_cond_broadcast(
                                                &fil_crypt_threads_cond);
                                }
                
                                goto wait_for_work;
                        }

[gemini-code-assist]

When a tablespace is successfully found for rotation, the sleep timeout and timed wait count should be reset. This ensures that the exponential backoff starts fresh the next time the thread becomes idle, rather than potentially jumping straight to an indefinite wait due to stale state from previous idle periods.

			/* we found a space to rotate */
			thr.reset_sleep_timeout();

[gemini-code-assist]

This loop for IOPS allocation is a busy-wait that consumes 100% CPU if fil_crypt_alloc_iops returns false (e.g., when the global encryption IOPS limit is reached). While this pattern existed in the previous version, it remains a performance bottleneck. Consider adding a short sleep or yielding the thread if IOPS are unavailable.

[iMineLink]

Leaved few comments, one about potential stats missing update, one about iteration restarts on heavy IOPS usage, few about comments improvements, and the most important one about a missing mtr test to "smoke-test" the new behavior measurable with the INNODB_ENCRYPTION_INDEFINITE_WAITS and INNODB_ENCRYPTION_TIMED_WAITS variables. I also commented previous Gemini review comments, please check, I think only the overflow on 16-bit timeout variable is important.

[iMineLink]

Requesting a few touchups, the only critical one (please review if necessary) is in fil_crypt_find_space_to_rotate(). Thanks.

[iMineLink]

Thanks for addressing the previous review comments.
I found a minor stale comment and a potential case in which "polling" behavior would be still observed: the safest change is to throttle it with a sleep I guess, please take a look.

[iMineLink]

There is a failure in encryption.create_or_replace which needs attention and some reasoning needed around the indefinite wait in wait_for_work() now that polling is removed. Thank you.

[iMineLink]

Thanks for the changes. I did some testing and everything was OK:

• 3x ./build/Debug/mysql-test/mtr --mem encryption.create_or_replace{,,,,,,,,,} --parallel=auto --repeat=100
• ./build/Debug/mysql-test/mtr --mem --parallel=auto

I have one final comment (even though no failure was seen in mtr), please check.
I think this is a pretty thorough rework of the encryption subsystem, and will need dedicated testing too.

[iMineLink]

Thanks for addressing my last comment.
I did another round of stress testing with ./build/Debug/mysql-test/mtr --mem encryption.create_or_replace{,,,,,,,,,} --parallel=auto --repeat=100 and it passed.
The changes now look good to me, but they will need dedicated testing before merge because of the wide impact on encryption.

[dr-m]

I’d find the name before_first more descriptive. I don’t insist that it be changed as part of this.

[dr-m]

warning C4805: '|': unsafe mix of type 'ulint' and type 'bool' in operation

I think we need ulint{} around the second operand.

[dr-m]

Instead of implementing my suggestion, you added another conditional branch. I tested my suggestion on https://godbolt.org (using the built-in type unsigned instead of a type alias ulint):

unsigned t(bool a, unsigned b) {
    return !(unsigned{a} | b);
}
unsigned w(bool a, unsigned b) {
    return !(a | b);
}
unsigned bloat(bool a, unsigned b) {
    return !a || !b;
}

The first variant does not result in a warning; the second one does:

example.cpp
<source>(5): warning C4805: '|': unsafe mix of type 'bool' and type 'unsigned int' in operation

The third variant results in a conditional branch being emitted.

[dr-m]

OK to push after eliminating the conditional branch.