Fix Bug #54538 - use of exclusive innodb dictionary lock limits performance.

This patch doesn't get rid of the need to acquire the dict_sys->mutex but
reduces the need to keep the mutex locked for the duration of the query
to fsp_get_available_space_in_free_extents() from ha_innobase::info().
 
This is a port of revno:3548 from the builtin to the plugin.

rb://501 Approved by Jimmy Yang and Marko Makela.

5 files changed, 134 insertions, 34 deletions

diff --git a/storage/innodb_plugin/fil/fil0fil.c b/storage/innodb_plugin/fil/fil0fil.c
index 796fe921a7e..0f774dcaaa1 100644
--- a/storage/innodb_plugin/fil/fil0fil.c
+++ b/storage/innodb_plugin/fil/fil0fil.c
@@ -329,14 +329,15 @@ fil_get_space_id_for_table(
 /*******************************************************************//**
 Frees a space object from the tablespace memory cache. Closes the files in
 the chain but does not delete them. There must not be any pending i/o's or
-flushes on the files. */
+flushes on the files.
+@return TRUE on success */
 static
 ibool
 fil_space_free(
 /*===========*/
-				/* out: TRUE if success */
-	ulint		id,	/* in: space id */
-	ibool		own_mutex);/* in: TRUE if own system->mutex */
+	ulint		id,		/* in: space id */
+	ibool		x_latched);	/* in: TRUE if caller has space->latch
+					in X mode */
 /********************************************************************//**
 Reads data from a space to a buffer. Remember that the possible incomplete
 blocks at the end of file are ignored: they are not taken into account when
@@ -1123,6 +1124,7 @@ try_again:
 	space = fil_space_get_by_name(name);
 
 	if (UNIV_LIKELY_NULL(space)) {
+		ibool	success;
 		ulint	namesake_id;
 
 		ut_print_timestamp(stderr);
@@ -1161,9 +1163,10 @@ try_again:
 
 		namesake_id = space->id;
 
-		mutex_exit(&fil_system->mutex);
+		success = fil_space_free(namesake_id, FALSE);
+		ut_a(success);
 
-		fil_space_free(namesake_id, FALSE);
+		mutex_exit(&fil_system->mutex);
 
 		goto try_again;
 	}
@@ -1314,15 +1317,14 @@ fil_space_free(
 /*===========*/
 					/* out: TRUE if success */
 	ulint		id,		/* in: space id */
-	ibool		own_mutex)	/* in: TRUE if own system->mutex */
+	ibool		x_latched)	/* in: TRUE if caller has space->latch
+					in X mode */
 {
 	fil_space_t*	space;
 	fil_space_t*	namespace;
 	fil_node_t*	fil_node;
 
-	if (!own_mutex) {
-		mutex_enter(&fil_system->mutex);
-	}
+	ut_ad(mutex_own(&fil_system->mutex));
 
 	space = fil_space_get_by_id(id);
 
@@ -1333,8 +1335,6 @@ fil_space_free(
 			" from the cache but\n"
 			"InnoDB: it is not there.\n", (ulong) id);
 
-		mutex_exit(&fil_system->mutex);
-
 		return(FALSE);
 	}
 
@@ -1369,8 +1369,8 @@ fil_space_free(
 
 	ut_a(0 == UT_LIST_GET_LEN(space->chain));
 
-	if (!own_mutex) {
-		mutex_exit(&fil_system->mutex);
+	if (x_latched) {
+		rw_lock_x_unlock(&space->latch);
 	}
 
 	rw_lock_free(&(space->latch));
@@ -1615,25 +1615,27 @@ fil_close_all_files(void)
 /*=====================*/
 {
 	fil_space_t*	space;
-	fil_node_t*	node;
 
 	mutex_enter(&fil_system->mutex);
 
 	space = UT_LIST_GET_FIRST(fil_system->space_list);
 
 	while (space != NULL) {
+		fil_node_t*	node;
 		fil_space_t*	prev_space = space;
 
-		node = UT_LIST_GET_FIRST(space->chain);
+		for (node = UT_LIST_GET_FIRST(space->chain);
+		     node != NULL;
+		     node = UT_LIST_GET_NEXT(chain, node)) {
 
-		while (node != NULL) {
 			if (node->open) {
 				fil_node_close_file(node, fil_system);
 			}
-			node = UT_LIST_GET_NEXT(chain, node);
 		}
+
 		space = UT_LIST_GET_NEXT(space_list, space);
-		fil_space_free(prev_space->id, TRUE);
+
+		fil_space_free(prev_space->id, FALSE);
 	}
 
 	mutex_exit(&fil_system->mutex);
@@ -2253,6 +2255,19 @@ try_again:
 	path = mem_strdup(space->name);
 
 	mutex_exit(&fil_system->mutex);
+
+	/* Important: We rely on the data dictionary mutex to ensure
+	that a race is not possible here. It should serialize the tablespace
+	drop/free. We acquire an X latch only to avoid a race condition
+	when accessing the tablespace instance via:
+
+	  fsp_get_available_space_in_free_extents().
+
+	There our main motivation is to reduce the contention on the
+	dictionary mutex. */
+
+	rw_lock_x_lock(&space->latch);
+
 #ifndef UNIV_HOTBACKUP
 	/* Invalidate in the buffer pool all pages belonging to the
 	tablespace. Since we have set space->is_being_deleted = TRUE, readahead
@@ -2265,7 +2280,11 @@ try_again:
 #endif
 	/* printf("Deleting tablespace %s id %lu\n", space->name, id); */
 
-	success = fil_space_free(id, FALSE);
+	mutex_enter(&fil_system->mutex);
+
+	success = fil_space_free(id, TRUE);
+
+	mutex_exit(&fil_system->mutex);
 
 	if (success) {
 		success = os_file_delete(path);
@@ -2273,6 +2292,8 @@ try_again:
 		if (!success) {
 			success = os_file_delete_if_exists(path);
 		}
+	} else {
+		rw_lock_x_unlock(&space->latch);
 	}
 
 	if (success) {
@@ -2300,6 +2321,31 @@ try_again:
 	return(FALSE);
 }
 
+/*******************************************************************//**
+Returns TRUE if a single-table tablespace is being deleted.
+@return TRUE if being deleted */
+UNIV_INTERN
+ibool
+fil_tablespace_is_being_deleted(
+/*============================*/
+	ulint		id)	/*!< in: space id */
+{
+	fil_space_t*	space;
+	ibool		is_being_deleted;
+
+	mutex_enter(&fil_system->mutex);
+
+	space = fil_space_get_by_id(id);
+
+	ut_a(space != NULL);
+
+	is_being_deleted = space->is_being_deleted;
+
+	mutex_exit(&fil_system->mutex);
+
+	return(is_being_deleted);
+}
+
 #ifndef UNIV_HOTBACKUP
 /*******************************************************************//**
 Discards a single-table tablespace. The tablespace must be cached in the
@@ -4763,7 +4809,7 @@ fil_page_get_type(
 	return(mach_read_from_2(page + FIL_PAGE_TYPE));
 }
 
-/********************************************************************
+/****************************************************************//**
 Initializes the tablespace memory cache. */
 UNIV_INTERN
 void
diff --git a/storage/innodb_plugin/fsp/fsp0fsp.c b/storage/innodb_plugin/fsp/fsp0fsp.c
index 2bae8481d20..f600e96bf88 100644
--- a/storage/innodb_plugin/fsp/fsp0fsp.c
+++ b/storage/innodb_plugin/fsp/fsp0fsp.c
@@ -3102,13 +3102,63 @@ fsp_get_available_space_in_free_extents(
 
 	ut_ad(!mutex_own(&kernel_mutex));
 
+	/* The convoluted mutex acquire is to overcome latching order
+	issues: The problem is that the fil_mutex is at a lower level
+	than the tablespace latch and the buffer pool mutex. We have to
+	first prevent any operations on the file system by acquiring the
+	dictionary mutex. Then acquire the tablespace latch to obey the
+	latching order and then release the dictionary mutex. That way we
+	ensure that the tablespace instance can't be freed while we are
+	examining its contents (see fil_space_free()).
+
+	However, there is one further complication, we release the fil_mutex
+	when we need to invalidate the the pages in the buffer pool and we
+	reacquire the fil_mutex when deleting and freeing the tablespace
+	instance in fil0fil.c. Here we need to account for that situation
+	too. */
+
+	mutex_enter(&dict_sys->mutex);
+
+	/* At this stage there is no guarantee that the tablespace even
+	exists in the cache. */
+
+	if (fil_tablespace_deleted_or_being_deleted_in_mem(space, -1)) {
+
+		mutex_exit(&dict_sys->mutex);
+
+		return(ULLINT_UNDEFINED);
+	}
+
 	mtr_start(&mtr);
 
 	latch = fil_space_get_latch(space, &flags);
+
+	/* This should ensure that the tablespace instance can't be freed
+	by another thread. However, the tablespace pages can still be freed
+	from the buffer pool. We need to check for that again. */
+
 	zip_size = dict_table_flags_to_zip_size(flags);
 
 	mtr_x_lock(latch, &mtr);
 
+	mutex_exit(&dict_sys->mutex);
+
+	/* At this point it is possible for the tablespace to be deleted and
+	its pages removed from the buffer pool. We need to check for that
+	situation. However, the tablespace instance can't be deleted because
+	our latching above should ensure that. */
+
+	if (fil_tablespace_is_being_deleted(space)) {
+
+		mtr_commit(&mtr);
+
+		return(ULLINT_UNDEFINED);
+	}
+
+	/* From here on even if the user has dropped the tablespace, the
+	pages _must_ still exist in the buffer pool and the tablespace
+	instance _must_ be in the file system hash table. */
+
 	space_header = fsp_get_space_header(space, zip_size, &mtr);
 
 	size = mtr_read_ulint(space_header + FSP_SIZE, MLOG_4BYTES, &mtr);
diff --git a/storage/innodb_plugin/handler/ha_innodb.cc b/storage/innodb_plugin/handler/ha_innodb.cc
index ff2f46256e6..ef323f7b87c 100644
--- a/storage/innodb_plugin/handler/ha_innodb.cc
+++ b/storage/innodb_plugin/handler/ha_innodb.cc
@@ -7637,19 +7637,12 @@ ha_innobase::info_low(
 			innodb_crash_recovery is set to a high value. */
 			stats.delete_length = 0;
 		} else {
-			/* lock the data dictionary to avoid races with
-			ibd_file_missing and tablespace_discarded */
-			row_mysql_lock_data_dictionary(prebuilt->trx);
+			ullint	avail_space;
 
-			/* ib_table->space must be an existent tablespace */
-			if (!ib_table->ibd_file_missing
-			    && !ib_table->tablespace_discarded) {
-
-				stats.delete_length =
-					fsp_get_available_space_in_free_extents(
-						ib_table->space) * 1024;
-			} else {
+			avail_space = fsp_get_available_space_in_free_extents(
+				ib_table->space);
 
+			if (avail_space == ULLINT_UNDEFINED) {
 				THD*	thd;
 
 				thd = ha_thd();
@@ -7666,9 +7659,9 @@ ha_innobase::info_low(
 					ib_table->name);
 
 				stats.delete_length = 0;
+			} else {
+				stats.delete_length = avail_space * 1024;
 			}
-
-			row_mysql_unlock_data_dictionary(prebuilt->trx);
 		}
 
 		stats.check_time = 0;
diff --git a/storage/innodb_plugin/include/fil0fil.h b/storage/innodb_plugin/include/fil0fil.h
index c894875b352..f6a19646292 100644
--- a/storage/innodb_plugin/include/fil0fil.h
+++ b/storage/innodb_plugin/include/fil0fil.h
@@ -716,6 +716,14 @@ fil_page_get_type(
 /*==============*/
 	const byte*	page);	/*!< in: file page */
 
+/*******************************************************************//**
+Returns TRUE if a single-table tablespace is being deleted.
+@return TRUE if being deleted */
+UNIV_INTERN
+ibool
+fil_tablespace_is_being_deleted(
+/*============================*/
+	ulint		id);	/*!< in: space id */
 
 typedef	struct fil_space_struct	fil_space_t;
 
diff --git a/storage/innodb_plugin/include/univ.i b/storage/innodb_plugin/include/univ.i
index c1ed56684c9..cab3af5297e 100644
--- a/storage/innodb_plugin/include/univ.i
+++ b/storage/innodb_plugin/include/univ.i
@@ -360,6 +360,9 @@ typedef unsigned long long int	ullint;
 /* Maximum value for ib_uint64_t */
 #define IB_ULONGLONG_MAX	((ib_uint64_t) (~0ULL))
 
+/* THe 'undefined' value for ullint */
+#define ULLINT_UNDEFINED        ((ullint)(-1))
+
 /* This 'ibool' type is used within Innobase. Remember that different included
 headers may define 'bool' differently. Do not assume that 'bool' is a ulint! */
 #define ibool			ulint