From 7d6b3d40085562d6f9f110f4ba921cf061548844 Mon Sep 17 00:00:00 2001 From: Vlad Lesin Date: Mon, 6 Mar 2023 19:09:13 +0300 Subject: MDEV-30775 Performance regression in fil_space_t::try_to_close() introduced in MDEV-23855 fil_node_open_file_low() tries to close files from the top of fil_system.space_list if the number of opened files is exceeded. It invokes fil_space_t::try_to_close(), which iterates the list searching for the first opened space. Then it just closes the space, leaving it in the same position in fil_system.space_list. On heavy files opening, like during 'SHOW TABLE STATUS ...' execution, if the number of opened files limit is reached, fil_space_t::try_to_close() iterates more and more closed spaces before reaching any opened space for each fil_node_open_file_low() call. What causes performance regression if the number of spaces is big enough. The fix is to keep opened spaces at the top of fil_system.space_list, and move closed files at the end of the list. For this purpose fil_space_t::space_list_last_opened pointer is introduced. It points to the last inserted opened space in fil_space_t::space_list. When space is opened, it's inserted to the position just after the pointer points to in fil_space_t::space_list to preserve the logic, inroduced in MDEV-23855. Any closed space is added to the end of fil_space_t::space_list. As opened spaces are located at the top of fil_space_t::space_list, fil_space_t::try_to_close() finds opened space faster. There can be the case when opened and closed spaces are mixed in fil_space_t::space_list if fil_system.freeze_space_list was set during fil_node_open_file_low() execution. But this should not cause any error, as fil_space_t::try_to_close() still iterates spaces in the list. There is no need in any test case for the fix, as it does not change any functionality, but just fixes performance regression. --- storage/innobase/fil/fil0fil.cc | 33 ++++++++++++++++------- storage/innobase/include/fil0fil.h | 54 +++++++++++++++++++++++++++++++++++--- storage/innobase/srv/srv0start.cc | 3 ++- 3 files changed, 76 insertions(+), 14 deletions(-) (limited to 'storage') diff --git a/storage/innobase/fil/fil0fil.cc b/storage/innobase/fil/fil0fil.cc index 45786e39696..9b6afbeb793 100644 --- a/storage/innobase/fil/fil0fil.cc +++ b/storage/innobase/fil/fil0fil.cc @@ -119,6 +119,9 @@ bool fil_space_t::try_to_close(bool print_info) } node->close(); + + fil_system.move_closed_last_to_space_list(node->space); + return true; } @@ -409,13 +412,7 @@ static bool fil_node_open_file_low(fil_node_t *node) ut_ad(node->is_open()); - if (UNIV_LIKELY(!fil_system.freeze_space_list)) - { - /* Move the file last in fil_system.space_list, so that - fil_space_t::try_to_close() should close it as a last resort. */ - UT_LIST_REMOVE(fil_system.space_list, node->space); - UT_LIST_ADD_LAST(fil_system.space_list, node->space); - } + fil_system.move_opened_last_to_space_list(node->space); fil_system.n_open++; return true; @@ -809,6 +806,8 @@ std::vector fil_system_t::detach(fil_space_t *space, space->is_in_default_encrypt= false; default_encrypt_tables.remove(*space); } + if (space_list_last_opened == space) + space_list_last_opened = UT_LIST_GET_PREV(space_list, space); UT_LIST_REMOVE(space_list, space); if (space == sys_space) sys_space= nullptr; @@ -933,12 +932,14 @@ fil_space_free( @param purpose tablespace purpose @param crypt_data encryption information @param mode encryption mode +@param opened true if space files are opened @return pointer to created tablespace, to be filled in with add() @retval nullptr on failure (such as when the same tablespace exists) */ fil_space_t *fil_space_t::create(const char *name, ulint id, ulint flags, fil_type_t purpose, fil_space_crypt_t *crypt_data, - fil_encryption_t mode) + fil_encryption_t mode, + bool opened) { fil_space_t* space; @@ -1004,7 +1005,10 @@ fil_space_t *fil_space_t::create(const char *name, ulint id, ulint flags, HASH_INSERT(fil_space_t, hash, &fil_system.spaces, id, space); - UT_LIST_ADD_LAST(fil_system.space_list, space); + if (opened) + fil_system.add_opened_last_to_space_list(space); + else + UT_LIST_ADD_LAST(fil_system.space_list, space); switch (id) { case 0: @@ -1334,6 +1338,15 @@ void fil_system_t::close() #endif /* __linux__ */ } +void fil_system_t::add_opened_last_to_space_list(fil_space_t *space) +{ + if (UNIV_LIKELY(space_list_last_opened != nullptr)) + UT_LIST_INSERT_AFTER(space_list, space_list_last_opened, space); + else + UT_LIST_ADD_FIRST(space_list, space); + space_list_last_opened= space; +} + /** Extend all open data files to the recovered size */ ATTRIBUTE_COLD void fil_system_t::extend_to_recv_size() { @@ -2412,7 +2425,7 @@ err_exit: if (fil_space_t* space = fil_space_t::create(name, space_id, flags, FIL_TYPE_TABLESPACE, - crypt_data, mode)) { + crypt_data, mode, true)) { space->punch_hole = punch_hole; fil_node_t* node = space->add(path, file, size, false, true); mtr_t mtr; diff --git a/storage/innobase/include/fil0fil.h b/storage/innobase/include/fil0fil.h index af941e359f8..b124f5c6358 100644 --- a/storage/innobase/include/fil0fil.h +++ b/storage/innobase/include/fil0fil.h @@ -906,11 +906,13 @@ public: @param purpose tablespace purpose @param crypt_data encryption information @param mode encryption mode + @param opened true if space files are opened @return pointer to created tablespace, to be filled in with add() @retval nullptr on failure (such as when the same tablespace exists) */ static fil_space_t *create(const char *name, ulint id, ulint flags, fil_type_t purpose, fil_space_crypt_t *crypt_data, - fil_encryption_t mode= FIL_ENCRYPTION_DEFAULT); + fil_encryption_t mode= FIL_ENCRYPTION_DEFAULT, + bool opened= false); MY_ATTRIBUTE((warn_unused_result)) /** Acquire a tablespace reference. @@ -1361,6 +1363,11 @@ struct fil_system_t { private: bool m_initialised; + + /** Points to the last opened space in space_list. Protected with + fil_system.mutex. */ + fil_space_t *space_list_last_opened= nullptr; + #ifdef __linux__ /** available block devices that reside on non-rotational storage */ std::vector ssd; @@ -1405,8 +1412,11 @@ public: /** nonzero if fil_node_open_file_low() should avoid moving the tablespace to the end of space_list, for FIFO policy of try_to_close() */ ulint freeze_space_list; - UT_LIST_BASE_NODE_T(fil_space_t) space_list; - /*!< list of all file spaces */ + + /** List of all file spaces, opened spaces should be at the top of the list + to optimize try_to_close() execution. Protected with fil_system.mutex. */ + UT_LIST_BASE_NODE_T(fil_space_t) space_list; + UT_LIST_BASE_NODE_T(fil_space_t) named_spaces; /*!< list of all file spaces for which a FILE_MODIFY @@ -1422,6 +1432,44 @@ public: has issued a warning about potential space_id reuse */ + /** Add the file to the end of opened spaces list in + fil_system.space_list, so that fil_space_t::try_to_close() should close + it as a last resort. + @param space space to add */ + void add_opened_last_to_space_list(fil_space_t *space); + + /** Move the file to the end of opened spaces list in + fil_system.space_list, so that fil_space_t::try_to_close() should close + it as a last resort. + @param space space to move */ + void move_opened_last_to_space_list(fil_space_t *space) + { + /* In the case when several files of the same space are added in a + row, there is no need to remove and add a space to the same position + in space_list. It can be for system or temporary tablespaces. */ + if (freeze_space_list || space_list_last_opened == space) + return; + + UT_LIST_REMOVE(space_list, space); + + add_opened_last_to_space_list(space); + } + + /** Move closed file last in fil_system.space_list, so that + fil_space_t::try_to_close() iterates opened files first in FIFO order, + i.e. first opened, first closed. + @param space space to move */ + void move_closed_last_to_space_list(fil_space_t *space) + { + if (UNIV_UNLIKELY(freeze_space_list)) + return; + + if (space_list_last_opened == space) + space_list_last_opened= UT_LIST_GET_PREV(space_list, space); + UT_LIST_REMOVE(space_list, space); + UT_LIST_ADD_LAST(space_list, space); + } + /** Return the next tablespace from default_encrypt_tables list. @param space previous tablespace (nullptr to start from the start) @param recheck whether the removal condition needs to be rechecked after diff --git a/storage/innobase/srv/srv0start.cc b/storage/innobase/srv/srv0start.cc index f56f2846872..cc2bb699fd9 100644 --- a/storage/innobase/srv/srv0start.cc +++ b/storage/innobase/srv/srv0start.cc @@ -563,7 +563,8 @@ err_exit: fil_set_max_space_id_if_bigger(space_id); fil_space_t *space= fil_space_t::create(undo_name, space_id, fsp_flags, - FIL_TYPE_TABLESPACE, NULL); + FIL_TYPE_TABLESPACE, NULL, + FIL_ENCRYPTION_DEFAULT, true); ut_a(fil_validate()); ut_a(space); -- cgit v1.2.1 From 1495f057c8a3aa5783de0993f6f95aae3e352f7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Thu, 16 Mar 2023 13:39:23 +0200 Subject: MDEV-30860 Race condition between buffer pool flush and log file deletion in mariadb-backup --prepare srv_start(): If we are going to close the log file in mariadb-backup --prepare, call buf_flush_sync() before calling recv_sys.debug_free() to ensure that the log file will not be accessed. This fixes a rather rare failure in the test mariabackup.innodb_force_recovery where buf_flush_page_cleaner() would invoke log_checkpoint_low() because !recv_recovery_is_on() would hold due to the fact that recv_sys.debug_free() had already been called. Then, the log write for the checkpoint would fail because srv_start() had invoked log_sys.log.close_file(). --- storage/innobase/srv/srv0start.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'storage') diff --git a/storage/innobase/srv/srv0start.cc b/storage/innobase/srv/srv0start.cc index cc2bb699fd9..44fca2c81a5 100644 --- a/storage/innobase/srv/srv0start.cc +++ b/storage/innobase/srv/srv0start.cc @@ -922,9 +922,7 @@ static lsn_t srv_prepare_to_delete_redo_log_file(bool old_exists) { DBUG_ENTER("srv_prepare_to_delete_redo_log_file"); - /* Disable checkpoints in the page cleaner. */ - ut_ad(!recv_sys.recovery_on); - recv_sys.recovery_on= true; + ut_ad(recv_sys.recovery_on); /* Clean the buffer pool. */ buf_flush_sync(); @@ -1606,10 +1604,10 @@ file_checked: } } - recv_sys.debug_free(); - if (srv_operation == SRV_OPERATION_RESTORE || srv_operation == SRV_OPERATION_RESTORE_EXPORT) { + buf_flush_sync(); + recv_sys.debug_free(); /* After applying the redo log from SRV_OPERATION_BACKUP, flush the changes to the data files and truncate or delete the log. @@ -1701,6 +1699,8 @@ file_checked: return(srv_init_abort(err)); } } + + recv_sys.debug_free(); } ut_ad(err == DB_SUCCESS); -- cgit v1.2.1