Bug#19149177 Remove space_id lookups

Bug#16831138 69276: fil_system->mutex contention limits block read rate
with fast storage (partial fix)

The function calls fil_inc_pending_ops() and fil_decr_pending_ops() form a
logical pair.

If the first call succeeds, the fil_space_t object cannot be deleted until
after the second call has been issued. So, we could safely cache the pointer
here, and avoid looking up the tablespace again.

We replace the two functions with fil_space_acquire() and fil_space_release().

We will also start using these functions in the following code:
buf_load(): restoring a buffer pool dump
fsp_get_available_space_in_free_extents(): used in ha_innobase::info()
lock_rec_block_validate(): debug code

rb#5919 approved by Yasufumi Kinoshita and Jimmy Yang

Author: Marko Makela

mysql-test/suite/innodb/t/innodb_buffer_pool_load.test

@@ -27,6 +27,7 @@ WHERE table_name LIKE '%ib_bp_test%';
 
 # Here we end up with 16382 rows in the table
 -- disable_query_log
+call mtr.add_suppression("InnoDB: Trying to do an operation on a dropped tablespace");
 INSERT INTO ib_bp_test (b, c) VALUES (REPEAT('b', 64), REPEAT('c', 256));
 INSERT INTO ib_bp_test (b, c) VALUES (REPEAT('B', 64), REPEAT('C', 256));
 let $i=12;

storage/innobase/buf/buf0dump.cc

@@ -540,31 +540,34 @@ buf_load()
 	ulint		last_check_time = 0;
 	ulint		last_activity_cnt = 0;
 
-	/* Avoid calling the expensive fil_space_get_page_size() for each
+	/* Avoid calling the expensive fil_space_acquire() for each
 	page within the same tablespace. dump[] is sorted by (space, page),
 	so all pages from a given tablespace are consecutive. */
 	ulint		cur_space_id = BUF_DUMP_SPACE(dump[0]);
-	bool		found;
-	page_size_t	page_size(fil_space_get_page_size(
-					cur_space_id, &found));
+	fil_space_t*	space = fil_space_acquire(cur_space_id);
+	page_size_t	page_size(space ? space->flags : 0);
 
 	for (i = 0; i < dump_n && !SHUTTING_DOWN(); i++) {
 
 		/* space_id for this iteration of the loop */
 		const ulint	this_space_id = BUF_DUMP_SPACE(dump[i]);
 
 		if (this_space_id != cur_space_id) {
-			cur_space_id = this_space_id;
+			if (space) {
+				fil_space_release(space);
+			}
 
-			const page_size_t	cur_page_size(
-				fil_space_get_page_size(cur_space_id, &found));
+			cur_space_id = this_space_id;
+			space = fil_space_acquire(cur_space_id);
 
-			if (found) {
+			if (space) {
+				const page_size_t	cur_page_size(
+					space->flags);
 				page_size.copy_from(cur_page_size);
 			}
 		}
 
-		if (!found) {
+		if (!space) {
 			continue;
 		}
 
@@ -583,6 +586,9 @@ buf_load()
 		}
 
 		if (buf_load_abort_flag) {
+			if (space) {
+				fil_space_release(space);
+			}
 			buf_load_abort_flag = FALSE;
 			ut_free(dump);
 			buf_load_status(
@@ -595,6 +601,10 @@ buf_load()
 			&last_check_time, &last_activity_cnt, i);
 	}
 
+	if (space) {
+		fil_space_release(space);
+	}
+
 	ut_free(dump);
 
 	ut_sprintf_timestamp(now);

storage/innobase/buf/buf0rea.cc

@@ -278,22 +278,23 @@ buf_read_ahead_random(
 		return(0);
 	}
 
-	/* Remember the tablespace version before we ask te tablespace size
-	below: if DISCARD + IMPORT changes the actual .ibd file meanwhile, we
-	do not try to read outside the bounds of the tablespace! */
-
-	tablespace_version = fil_space_get_version(page_id.space());
-
 	low  = (page_id.page_no() / buf_read_ahead_random_area)
 		* buf_read_ahead_random_area;
 
 	high = (page_id.page_no() / buf_read_ahead_random_area + 1)
 		* buf_read_ahead_random_area;
 
-	const ulint	space_size = fil_space_get_size(page_id.space());
-
-	if (high > space_size) {
-		high = space_size;
+	/* Remember the tablespace version before we ask the tablespace size
+	below: if DISCARD + IMPORT changes the actual .ibd file meanwhile, we
+	do not try to read outside the bounds of the tablespace! */
+	if (fil_space_t* space = fil_space_acquire(page_id.space())) {
+		tablespace_version = space->tablespace_version;
+		if (high > space->size) {
+			high = space->size;
+		}
+		fil_space_release(space);
+	} else {
+		return(0);
 	}
 
 	buf_pool_mutex_enter(buf_pool);
@@ -542,18 +543,24 @@ buf_read_ahead_linear(
 	/* Remember the tablespace version before we ask te tablespace size
 	below: if DISCARD + IMPORT changes the actual .ibd file meanwhile, we
 	do not try to read outside the bounds of the tablespace! */
+	ulint	space_size;
 
-	tablespace_version = fil_space_get_version(page_id.space());
-
-	buf_pool_mutex_enter(buf_pool);
-
-	if (high > fil_space_get_size(page_id.space())) {
-		buf_pool_mutex_exit(buf_pool);
-		/* The area is not whole, return */
+	if (fil_space_t* space = fil_space_acquire(page_id.space())) {
+		tablespace_version = space->tablespace_version;
+		space_size = space->size;
+		fil_space_release(space);
 
+		if (high > space->size) {
+			/* The area is not whole */
+			return(0);
+		}
+	} else {
 		return(0);
 	}
 
+
+	buf_pool_mutex_enter(buf_pool);
+
 	if (buf_pool->n_pend_reads
 	    > buf_pool->curr_size / BUF_READ_AHEAD_PEND_LIMIT) {
 		buf_pool_mutex_exit(buf_pool);
@@ -677,7 +684,7 @@ buf_read_ahead_linear(
 		return(0);
 	}
 
-	if (high > fil_space_get_size(page_id.space())) {
+	if (high > space_size) {
 		/* The area is not whole, return */
 
 		return(0);
@@ -818,39 +825,40 @@ buf_read_ibuf_merge_pages(
 }
 
 /** Issues read requests for pages which recovery wants to read in.
-@param[in]	sync		TRUE if the caller wants this function to wait
+@param[in]	sync		true if the caller wants this function to wait
 for the highest address page to get read in, before this function returns
-@param[in]	space		space id
+@param[in]	space_id	tablespace id
 @param[in]	page_nos	array of page numbers to read, with the
 highest page number the last in the array
 @param[in]	n_stored	number of page numbers in the array */
+
 void
 buf_read_recv_pages(
-	ibool		sync,
-	ulint		space,
+	bool		sync,
+	ulint		space_id,
 	const ulint*	page_nos,
 	ulint		n_stored)
 {
 	int64_t			tablespace_version;
 	ulint			count;
 	dberr_t			err;
 	ulint			i;
-	bool			found;
-	const page_size_t	page_size(fil_space_get_page_size(space,
-								  &found));
-
-	if (!found) {
-		/* It is a single table tablespace and the .ibd file is
-		missing: do nothing */
+	ulint			space_flags;
 
+	if (fil_space_t* space = fil_space_acquire(space_id)) {
+		tablespace_version = space->tablespace_version;
+		space_flags = space->flags;
+		fil_space_release(space);
+	} else {
+		/* The tablespace is missing: do nothing */
 		return;
 	}
 
-	tablespace_version = fil_space_get_version(space);
+	const page_size_t	page_size(space_flags);
 
 	for (i = 0; i < n_stored; i++) {
 		buf_pool_t*		buf_pool;
-		const page_id_t	cur_page_id(space, page_nos[i]);
+		const page_id_t	cur_page_id(space_id, page_nos[i]);
 
 		count = 0;
 

storage/innobase/fil/fil0fil.cc

@@ -53,7 +53,6 @@ Created 10/25/1995 Heikki Tuuri
 #else /* !UNIV_HOTBACKUP */
 # include "srv0srv.h"
 #endif /* !UNIV_HOTBACKUP */
-#include "fsp0file.h"
 #include "fsp0sysspace.h"
 #include "ut0new.h"
 
@@ -1844,65 +1843,48 @@ fil_write_flushed_lsn(
 	return(err);
 }
 
-/*================ SINGLE-TABLE TABLESPACES ==========================*/
-
 #ifndef UNIV_HOTBACKUP
-/*******************************************************************//**
-Increments the count of pending operation, if space is not being deleted
-or truncated.
-@return	true if being deleted/truncated, and operations should be skipped */
-bool
-fil_inc_pending_ops(
-/*================*/
-	ulint	id)	/*!< in: space id */
+/** Acquire a tablespace when it could be dropped concurrently.
+Used by background threads that do not necessarily hold proper locks
+for concurrency control.
+@param[in]	id	tablespace ID
+@return the tablespace, or NULL if deleted or being deleted */
+
+fil_space_t*
+fil_space_acquire(
+	ulint	id)
 {
 	fil_space_t*	space;
-	bool skip_inc_pending_ops = true;
 
 	mutex_enter(&fil_system->mutex);
 
 	space = fil_space_get_by_id(id);
 
 	if (space == NULL) {
-
 		ib::error() << "Trying to do an operation on a dropped"
 			" tablespace. Space ID: " << id;
+	} else if (space->stop_new_ops || space->is_being_truncated) {
+		space = NULL;
 	} else {
-
-		skip_inc_pending_ops = (space->stop_new_ops
-					|| space->is_being_truncated);
-		if (!skip_inc_pending_ops) {
-			space->n_pending_ops++;
-		}
+		space->n_pending_ops++;
 	}
 
 	mutex_exit(&fil_system->mutex);
 
-	return(skip_inc_pending_ops);
+	return(space);
 }
 
-/*******************************************************************//**
-Decrements the count of pending operations. */
+/** Release a tablespace acquired with fil_space_acquire().
+@param[in,out]	space	tablespace to release  */
+
 void
-fil_decr_pending_ops(
-/*=================*/
-	ulint	id)	/*!< in: space id */
+fil_space_release(
+	fil_space_t*	space)
 {
-	fil_space_t*	space;
-
 	mutex_enter(&fil_system->mutex);
-
-	space = fil_space_get_by_id(id);
-
-	if (space == NULL) {
-		ib::error() << "Decrementing pending operation"
-			" of a dropped tablespace " << id;
-	}
-
-	if (space != NULL) {
-		space->n_pending_ops--;
-	}
-
+	ut_ad(space->magic_n == FIL_SPACE_MAGIC_N);
+	ut_ad(space->n_pending_ops > 0);
+	space->n_pending_ops--;
 	mutex_exit(&fil_system->mutex);
 }
 #endif /* !UNIV_HOTBACKUP */
@@ -2403,25 +2385,27 @@ enum fil_operation_t {
 	FIL_OPERATION_TRUNCATE	/*!< truncate a single-table tablespace */
 };
 
-/*******************************************************************//**
-Check for change buffer merges.
-@return 0 if no merges else count + 1. */
+/** Check for pending operations.
+@param[in]	space	tablespace
+@param[in]	count	number of attempts so far
+@return 0 if no operations else count + 1. */
 static
 ulint
-fil_ibuf_check_pending_ops(
-/*=======================*/
-	fil_space_t*	space,	/*!< in/out: Tablespace to check */
-	ulint		count)	/*!< in: number of attempts so far */
+fil_check_pending_ops(
+	fil_space_t*	space,
+	ulint		count)
 {
 	ut_ad(mutex_own(&fil_system->mutex));
 
-	if (space != 0 && space->n_pending_ops != 0) {
+	const ulint	n_pending_ops = space ? space->n_pending_ops : 0;
+
+	if (n_pending_ops) {
 
 		if (count > 5000) {
 			ib::warn() << "Trying to close/delete/truncate"
 				" tablespace '" << space->name
-				<< "' but there are " << space->n_pending_ops
-				<< " pending change buffer merges on it.";
+				<< "' but there are " << n_pending_ops
+				<< " pending operations on it.";
 		}
 
 		return(count + 1);
@@ -2506,14 +2490,14 @@ fil_check_pending_operations(
 	}
 	mutex_exit(&fil_system->mutex);
 
-	/* Check for pending change buffer merges. */
+	/* Check for pending operations. */
 
 	do {
 		mutex_enter(&fil_system->mutex);
 
 		sp = fil_space_get_by_id(id);
 
-		count = fil_ibuf_check_pending_ops(sp, count);
+		count = fil_check_pending_ops(sp, count);
 
 		mutex_exit(&fil_system->mutex);
 
@@ -6198,8 +6182,8 @@ fil_names_write(
 	while fil_check_pending_operations() is waiting for operations
 	to quiesce. This is not a problem, because
 	ibuf_merge_or_delete_for_page() would call
-	fil_inc_pending_ops() before mtr_start() and
-	fil_decr_pending_ops() after mtr_commit(). This is why
+	fil_space_acquire() before mtr_start() and
+	fil_space_release() after mtr_commit(). This is why
 	n_pending_ops should not be zero if stop_new_ops is set. */
 	ut_ad(!space->stop_new_ops
 	      || space->is_being_truncated /* TRUNCATE sets stop_new_ops */