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.

4 files changed, 81 insertions, 16 deletions

diff --git a/extra/mariabackup/xtrabackup.cc b/extra/mariabackup/xtrabackup.cc
index 522072a05fd..c57c2685c94 100644
--- a/extra/mariabackup/xtrabackup.cc
+++ b/extra/mariabackup/xtrabackup.cc
@@ -3361,7 +3361,9 @@ static void xb_load_single_table_tablespace(const char *dirname,
 	if (err == DB_SUCCESS && file->space_id() != SRV_TMP_SPACE_ID) {
 		space = fil_space_t::create(
 			name, file->space_id(), file->flags(),
-			FIL_TYPE_TABLESPACE, NULL/* TODO: crypt_data */);
+			FIL_TYPE_TABLESPACE, NULL/* TODO: crypt_data */,
+			FIL_ENCRYPTION_DEFAULT,
+			file->handle() != OS_FILE_CLOSED);
 
 		ut_a(space != NULL);
 		space->add(file->filepath(),
@@ -5242,7 +5244,8 @@ exit:
 	ut_ad(fil_space_t::physical_size(flags) == info.page_size);
 
 	if (fil_space_t::create(dest_space_name, info.space_id, flags,
-				FIL_TYPE_TABLESPACE, 0)) {
+				FIL_TYPE_TABLESPACE, 0, FIL_ENCRYPTION_DEFAULT,
+				true)) {
 		*success = xb_space_create_file(real_name, info.space_id,
 						flags, &file);
 	} else {
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<pfs_os_file_t> 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<dev_t> 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);