MDEV-31956 SSD based InnoDB buffer pool extension

[vlad-lesin]

In one of the practical cloud MariaDB setups, a server node accesses its
datadir over the network, but also has a fast local SSD storage for
temporary data. The content of such temporary storage is lost when the
server container is destroyed.

The commit uses this ephemeral fast local storage (SSD) as an extension of
the portion of InnoDB buffer pool (DRAM) that caches persistent data
pages. This cache is separated from the persistent storage of data files
and ib_logfile0 and ignored during backup.

The following system variables were introduced:

innodb_extended_buffer_pool_size - the size of external buffer pool
file, if it equals to 0, external buffer pool will not be used;

innodb_extended_buffer_pool_path - the path to external buffer pool
file.

If innodb_extended_buffer_pool_size is not equal to 0, external buffer
pool file will be (re)created on startup.

Only clean pages will be flushed to external buffer pool file. There is
no need to flush dirty pages, as such pages will become clean after
flushing, and then will be evicted when they reach the tail of LRU list.

The general idea of this commit is to flush clean pages to external
buffer pool file when they are evicted.

A page can be evicted either by transaction thread or by background
thread of page cleaner. In some cases transaction thread is waiting for
page cleaner thread to finish its job. We can't do flushing in external
buffer pool file when transaction threads are waiting for eviction,
that would heart performance. That's why the only case for flushing is
when page cleaner thread evicts pages in background and there are no
waiters. For this purpose buf_pool_t::done_flush_list_waiters_count
variable was introduced, we flush evicted clean pages only if the
variable is zeroed.

Clean pages are evicted in buf_flush_LRU_list_batch() to keep some
amount of pages in buffer pool's free list. That's why we flush every
second page to external buffer pool file, otherwise there could be not
enough amount of pages in free list to let transaction threads to
allocate buffer pool pages without page cleaner waiting. This might be
not a good solution, but this is enough for prototyping.

External buffer pool page is introduced to store information in buffer
pool page hash about the certain page can be read from external buffer
pool file. The first several members of such page must be the same as the
members of internal page. External page frame must be equal to the
certain value to distinguish external page from internal one. External
buffer pages are preallocated on startup in external pages array. We
could get rid of the frame in external page, and check if the page's
address belongs to the array to distinguish external and internal pages.

There are also external pages free and LRU lists. When some internal page
is decided to be flushed in external buffer pool file, a new external
page is allocated either from the head of external free list, or from
the tail of external LRU list. Both lists are protected with
buf_pool.mutex. It makes sense, because a page is removed from internal
LRU list during eviction under buf_pool.mutex.

Then internal page is locked and the allocated external page is attached
to io request for external buffer pool file, and when write request is
completed, the internal page is replaced with external one in page hash,
external page is pushed to the head of external LRU list and internal
page is unlocked. After internal page was removed from external free list,
it was not placed in external LRU, and placed there only
after write completion, so the page can't be used by the other threads
until write is completed.

Page hash chain get element function has additional template parameter,
which notifies the function if external pages must be ignored or not. We
don't ignore external pages in page hash in two cases, when some page is
initialized for read and when one is reinitialized for new page creating.

When an internal page is initialized for read and external page with the
same page id is found in page hash, the internal page is locked,
the external page in replaced with newly initialized internal page in the
page hash chain, the external page is removed from external LRU list and
attached to io request to external buffer pool file. When the io request
is completed, external page is returned to external free list,
internal page is unlocked. So during read external page absents in both
external LRU and free lists and can't be reused.

When an internal page is initialized for new page creating and external
pages with the same page id is found in page hash, we just remove external
page from the page hash chain and external LRU list and push it to the
head of external free list. So the external page can be used for future
flushing.

There is also some magic with watch sentinels. If watch is goint to be
set for the external page with the same page id in page hash, we replace
the external page with sentinel in page hash and attach external
page to the sentinel's frame. When such sentinel with attached external
page should be removed, we replace it with the attached external page in
page hash instead of just removing the sentinel. This idea is not fully
implemented, as the function, which allocates external pages, does not
take into account that page hash can contain sentinels with attached
external pages. And, anyway, this code must be removed if we cherry-pick
the commit to 11.* branch, as change buffer is already removed in those
versions.

The pages are flushed to and read from external buffer pool file with
the same manner as they are flushed to their spaces, i.e. compressed and
encrypted pages stay compressed and encrypted in external buffer pool
file.

• The Jira issue number for this PR is: MDEV-______

Description

TODO: fill description here

Release Notes

TODO: What should the release notes say about this change?
Include any changed system variables, status variables or behaviour. Optionally list any https://mariadb.com/kb/ pages that need changing.

How can this PR be tested?

TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on "Writing good test cases".

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

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.

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.

[CLAassistant]

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.
0 out of 2 committers have signed the CLA.

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

[dr-m]

Before I review this deeper, could you please fix the build and the test innodb.ext_buf_pool on Microsoft Windows?

[dr-m]

Typically, the local SSD is larger than the RAM, and therefore it would be more interesting to test a scenario where the buffer pool is smaller than the extended buffer pool. I understand that our current CI setup is not suitable for any performance testing. But, currently we only cover this functionality in debug-instrumented executables.

Would it be possible to write some sort of a test, or extend an existing one (such as encryption.innochecksum) with a variant that would use the minimal innodb_buffer_pool_size and a larger innodb_extended_buffer_pool_size? Currently, that test is running with innodb_buffer_pool_size=64M.

[vlad-lesin]

The test flushes small amount of pages into extended buffer pool file, that's why there was not any reason to set a big file size. Probably it makes sense to develop separate test, we can set the minimal innodb_buffer_pool_size and a larger innodb_extended_buffer_pool_size, but what exactly are we going to test in that separate test?

[dr-m]

I was thinking that it would be useful to mimic a typical deployment scenario where we expect some pages to be written to the local SSD and read back from there. Sure, mtr tests are limited in size and not good for performance testing. Should there be any race conditions, having a test run on multiple platforms for every push would help catch them over time.

[dr-m]

This is a partial review.

[dr-m]

Here is another incomplete review.

[dr-m]

Could this be const char*?

[vlad-lesin]

No, because the path can be set with system variable.

[dr-m]

Here are some more comments. I’m doing this review in batches so that you can work concurrently on addressing some of these comments. Everything seems to be relatively minor stuff.

[dr-m]

Do we really need to declare the variable space this early? As far as I can tell, it’s only truly needed if !ext_buf().

[vlad-lesin]

In the case when ext_buf() returns true we invoke fil_space_t::get(...), what increases the space's reference counter. We need to decrease it, and we decrease it at the end of IORequest::read_complete().

[dr-m]

Is there any alternative to this, such as holding an exclusive latch on the buf_pool.page_hash slice?

Please add a FIXME comment to implement deferred, batched freeing, similar to the one we do in the normal LRU flushing/eviction in the page cleaner thread.

This would be an obvious performance bottleneck us when there is some write activity to the external buffer pool going on. The IORequest::write_complete() is expected to be called from multiple threads concurrently.

[dr-m]

std::ignore= is the canonical way to discard a return value.

[dr-m]

I see that my_copy() is being outside mysqltest.cc, for example in myisampack and aria_pack. This could fix a bug in those tools. Therefore, it would make sense to file a separate MDEV and PR for this fix, so that this simple bug can be fixed in GA releases.

[dr-m]

I am still yet to review the change to buf0flu.cc, buf0lru.cc, buf0rea.cc.

[dr-m]

This is potentially a very expensive operation, and it is being executed within a single critical section of buf_pool.mutex. Several years ago in 7cffb5f, I tried to refactor code so that we will avoid calculating page_id_t::fold() while holding buf_pool.mutex. Here, we are potentially doing it for every single element of buf_pool.ext_LRU_list. I see that the caller only needs buf_pool.mutex for buf_LRU_stat_inc_io() afterwards. So, there should be no problem to release and reacquire buf_pool.mutex here.

Based on buf_pool_t::unzip() and buf_page_create_low(), the correct lock order is to acquire buf_pool.mutex first and then a latch on a buf_pool.page_hash slice. Hence, it looks like the loop could be unnecessary and we could just wait for the hash_lock on the last element of buf_pool.ext_LRU_list.

There is only a single call to this function, in buf_page_t::flush(). The code would be easier to follow and faster to execute (better locality of reference) if the function was defined inline in the same compilation unit with the caller, that is, buf0flu.cc.

[dr-m]

Shouldn’t we assign ext_buf_pages_array= nullptr; to be in line with how we release buf_pool.memory and buf_pool.memory_unaligned?

[dr-m]

We would be checking for bpage!=nullptr twice after this change. I would suggest the following:

  if (!bpage)
    /* not found */;
  else if (buf_pool.is_page_extrenal(*bpage))
  {
    // … (indent with 2 extra spaces instead of 4)
  }
  else
  {

that is, replacing the if (bpage) with else.

[dr-m]

I think I only have buf0flu.cc left.

[dr-m]

The added line is indented insufficiently, and the comment needs to be expanded, presumably as follows:

@param ext_buf_page external buffer page to replace bpage in buf_pool.page_hash

[dr-m]

There are only 2 callers of this function. We could remove the default parameter and explicitly pass the nullptr in buf_pool_t::corrupted_evict().

[dr-m]

It’s customary to align the description of each @param. The preceding lines should be edited accordingly, or the parameter name could be shortened to ext_bpage.

[dr-m]

This is the only call site where we suppress the call to buf_page_t::set_corrupted_id(). Can you please add a comment that explains why that is necessary?

[dr-m]

I would omit the constructor comment as well as the member function. I don’t think that replacing init_page_result.in_ext_buffer_pool() with init_page_result.ext_buf_page or (say) init_page_result.ext_read would reduce readability.

The data members need to be documented with Doxygen compatible comments:

struct page_init_result
{
  /** initialized page descriptor and uninitialized frame */
  buf_page_t *page;
  /** external buffer pool page if the page can be read from ext_bp */
  ext_buf_page_t *ext_read;
};

[dr-m]

We are checking for init_page_result.ext_buf_page twice. Maybe it’s acceptable.

Thinking aloud: Could we free the block before reading it? As far as I can tell, the call buf_pool.ext_page_offset() inside fil_system.ext_bp_io() is not dereferencing the object. The answer is no, because if we freed the block first, then it could be reallocated for something else and the underlying file block could be overwritten before we get a chance to read the desired contents.

Also thinking aloud: Could we avoid the buf_pool.mutex here, and avoid having the buf_pool.ext_free list as well? We would simply identify freed blocks with an atomic acquire/release version of id_.is_corrupted() and id_.set_corrupted(). In that way, freeing blocks would be fast, but allocation would have to sweep the entire extended buffer pool until an empty slot is found. There could perhaps be a "cursor" to quickly locate the next available empty slot. Maybe you could add a FIXME or TODO comment about this somewhere, for example next to the definition of buf_pool.ext_free? This would also address my earlier comment about the new scalability bottleneck in IORequest::read_complete().

[dr-m]

What if we did not replace IORequest::node with a union, but just added another field ext_buf_page_t *const ext_buf_page? In that way, we could avoid this change altogether.

[dr-m]

Can we write else if (!fil_system.ext_buf_pool_enabled()) in order to skip one condition inside a critical section of buf_pool.mutex?

If we didn’t need the else branch inside the critical section (and I think we could fix that, see another comment), we could write

  buf_pool.stat.n_pages_read+= !ext_buf_page;

to replace the condition with arithmetics, optimizing for the likely code path that no external buffer pool is being used.

[dr-m]

As noted elsewhere, if we removed buf_pool.ext_free, this could be done outside the critical section of buf_pool.mutex. But, since this is an unlikely code path (the extended buffer pool was recently disabled due to an I/O error), I think that we can live with this.

[dr-m]

You are right, the page_zip_des_t::clear() is redundant here, and the entire macro declaration could be removed. This is because ut_zalloc_nokey() is already zero-initializing the entire bpage. The bpage->zip.clear() would zero-initialize everything except the buffer-fix field. Can you push this code removal as a separate pull request against 10.11?

[dr-m]

I completed my first round of review. I would suggest that you reply to each comment that you intend to fully address, and mark those comments as resolved.

There are some "nice to have" comments that can be addressed later. I would request to leave them as unresolved, so that we can easily find them when working on improving the performance.

Many of my comments are rather trivial to address and do not prevent this from being approved. After all, this has been already subjected to stress testing and a number of bugs have already been fixed.

[dr-m]

I think that it would be clearer to add a switch (type) around all this. This would also make the assertion about type==TEMPORARY stricter:

  lsn_t om ut_d(= oldest_modification());
  switch (type) {
  case PERSISTENT:
    ut_ad(om > 2);
    om= 1;
    goto clear_modification;
  case TEMPORARY:
    ut_ad(om == 2);
    om= 0;
  clear_modification:
    /* We use release memory order to guarantee that callers of
    oldest_modification_acquire() will observe the block as
    being detached from buf_pool.flush_list, after reading the value 0. */
    oldest_modification_.store(om, std::memory_order_release);
    break;
  case EXT_BUF:
    ut_ad(om == 0);
    break;
  }

[dr-m]

Instead of replacing node with a union and repurposing a bit of bpage we could leave those data members alone and simply add an ext_buf_page that would normally be nullptr. The sizeof(IORequest) is only 32 bytes on 64-bit systems, including 32 bits of padding around type. It should be no problem at all to let it grow by a single pointer.

[vlad-lesin]

Yes, the size of IORequest  is 32 bytes, as well as MAX_AIO_USERDATA_LEN, i.e. aiocb::m_userdata buffer size where the IORequest  object should be fit in. We can, of course, increase MAX_AIO_USERDATA_LEN by one more sizeof(void*)  as well as write_slots and read_slots arrays size. But it will cost a memcpy.

[dr-m]

It would be good to document this assumption of the values 0 and 1 here:

static_assert(buf_page_t::PERSISTENT == 0, "");
static_assert(buf_page_t::TEMPORARY == 1, "");

[dr-m]

Can we use a normal non-atomic counter here, and increment it inside bpage->write_complete(), inside the critical section of buf_pool.mutex? I think it should be more efficient. Also, if we had an IORequest::node always present, we could simplify the debug injection and just refer to node->space in the debug injection.

I think that we’d better assign bpage->oldest_modification() to a local variable and pass it to buf_page_t::write_complete(), instead of passing type. It’s an atomic field, therefore we should load it only once. Inside that function we should assert that the value still matches, and then identify the type of the block (0=external buffer pool, 1=impossible, 2=temporary tablespace, >2=write to persistent file) based on that.

[dr-m]

Can we assert that space->is_temporary() will never be written to_ext_buf?

[dr-m]

If we omitted this condition, we could guarantee that IORequest::node (see my other comments) is valid until the write has been completed. I don’t think that we should attempt to optimize for the unlikely case that the tablespace is being dropped while a write to the external buffer pool is in progress. It’s better to keep it simple.

[dr-m]

We should just check if (ext_page) and let the variable to_ext_buf die earlier. The ext_page will also be passed as a parameter to the subsequent function call.

[dr-m]

In our formatting convention, there is no line break before a { that begins a switch block.

[dr-m]

It looks like the code is being deindented by one level here for no valid reason. We’re basically just adding a label right before the if, as well as a condition flush_to_ebp || to skip the state check.

It would be nice to have a comment that explains why we would skip the state check here. It seems to me that it could lead to a failure of the subsequent statement ut_ad(!bpage->is_io_fixed()). state >= buf_page_t::READ_FIX holds exactly when the block is read or write fixed.

I could understand if we allowed a block to be concurrently written both to the persistent data file and to the extended buffer pool. But, I don’t think that buf_page_t::write_complete() is prepared to deal with such "recursion". Furthermore, I understood that the design principle is that only clean blocks (which have already been written back to the persistent data file) may be written to the extended buffer pool.

Based on the following subsequent code:

    default:
      /* bpage->oldest_modification() could be changed from 0 to not 0 while
      bpage was unlocked, in this case we just flush the page to its space */
      flush_to_ebp= false;

I believe that we may have a real race condition here. I don’t quite understand that comment. I would suggest replacing the phrase "was unlocked" (whose literal interpretation could be "during a call to bpage->lock.u_unlock()") with something that identifies a time frame, i.e., "between the time the block had been unlocked in … and our bpage->lock.u_lock_try(true) above". Anyway, I understood that a race condition was found during testing. Maybe it is related to this one.

[dr-m]

What if we always held a space reference, also for flush_to_ebp? It is true that we are not writing to space->chain but to the external buffer pool file, but some things would be simpler if we can assume that the tablespace won’t disappear while the write is in progress.