MDEV-12428 SIGSEGV in buf_page_decrypt_after_read() during DDL

Also, some MDEV-11738/MDEV-11581 post-push fixes.

In MariaDB 10.1, there is no fil_space_t::is_being_truncated field,
and the predicates fil_space_t::stop_new_ops and fil_space_t::is_stopping()
are interchangeable. I requested the fil_space_t::is_stopping() to be added
in the review, but some added checks for fil_space_t::stop_new_ops were
not replaced with calls to fil_space_t::is_stopping().

buf_page_decrypt_after_read(): In this low-level I/O operation, we must
look up the tablespace if it exists, even though future I/O operations
have been blocked on it due to a pending DDL operation, such as DROP TABLE
or TRUNCATE TABLE or other table-rebuilding operations (ALTER, OPTIMIZE).
Pass a parameter to fil_space_acquire_low() telling that we are performing
a low-level I/O operation and the fil_space_t::is_stopping() status should
be ignored.

Author: dr-m

storage/innobase/btr/btr0scrub.cc

@@ -129,15 +129,15 @@ btr_scrub_lock_dict_func(ulint space_id, bool lock_to_close_table,
 		* if we don't lock to close a table, we check if space
 		* is closing, and then instead give up
 		*/
-		if (lock_to_close_table == false) {
-			fil_space_t* space = fil_space_acquire(space_id);
-			if (!space || space->stop_new_ops) {
-				if (space) {
-					fil_space_release(space);
-				}
+		if (lock_to_close_table) {
+		} else if (fil_space_t* space = fil_space_acquire(space_id)) {
+			bool stopping = space->is_stopping();
+			fil_space_release(space);
+			if (stopping) {
 				return false;
 			}
-			fil_space_release(space);
+		} else {
+			return false;
 		}
 		os_thread_sleep(250000);
 
@@ -197,18 +197,15 @@ btr_scrub_table_close_for_thread(
 		return;
 	}
 
-	fil_space_t* space = fil_space_acquire(scrub_data->space);
-
-	/* If tablespace is not marked as stopping perform
-	the actual close. */
-	if (space && !space->is_stopping()) {
-		mutex_enter(&dict_sys->mutex);
-		/* perform the actual closing */
-		btr_scrub_table_close(scrub_data->current_table);
-		mutex_exit(&dict_sys->mutex);
-	}
-
-	if (space) {
+	if (fil_space_t* space = fil_space_acquire(scrub_data->space)) {
+		/* If tablespace is not marked as stopping perform
+		the actual close. */
+		if (!space->is_stopping()) {
+			mutex_enter(&dict_sys->mutex);
+			/* perform the actual closing */
+			btr_scrub_table_close(scrub_data->current_table);
+			mutex_exit(&dict_sys->mutex);
+		}
 		fil_space_release(space);
 	}
 

storage/innobase/buf/buf0buf.cc

@@ -6250,13 +6250,12 @@ buf_page_decrypt_after_read(
 		return (true);
 	}
 
-	fil_space_t* space = fil_space_acquire(bpage->space);
-	fil_space_crypt_t* crypt_data = space->crypt_data;
+	fil_space_t* space = fil_space_acquire(bpage->space, true);
 
 	/* Page is encrypted if encryption information is found from
 	tablespace and page contains used key_version. This is true
 	also for pages first compressed and then encrypted. */
-	if (!crypt_data) {
+	if (!space || !space->crypt_data) {
 		key_version = 0;
 	}
 
@@ -6340,6 +6339,8 @@ buf_page_decrypt_after_read(
 		}
 	}
 
-	fil_space_release(space);
+	if (space != NULL) {
+		fil_space_release(space);
+	}
 	return (success);
 }

storage/innobase/fil/fil0fil.cc

@@ -6325,16 +6325,12 @@ fil_flush(
 {
 	mutex_enter(&fil_system->mutex);
 
-	fil_space_t*	space = fil_space_get_by_id(space_id);
-
-	if (!space || space->stop_new_ops) {
-		mutex_exit(&fil_system->mutex);
-
-		return;
+	if (fil_space_t* space = fil_space_get_by_id(space_id)) {
+		if (!space->is_stopping()) {
+			fil_flush_low(space);
+		}
 	}
 
-	fil_flush_low(space);
-
 	mutex_exit(&fil_system->mutex);
 }
 
@@ -6374,8 +6370,7 @@ fil_flush_file_spaces(
 	     space;
 	     space = UT_LIST_GET_NEXT(unflushed_spaces, space)) {
 
-		if (space->purpose == purpose && !space->stop_new_ops) {
-
+		if (space->purpose == purpose && !space->is_stopping()) {
 			space_ids[n_space_ids++] = space->id;
 		}
 	}
@@ -7276,12 +7271,13 @@ Used by background threads that do not necessarily hold proper locks
 for concurrency control.
 @param[in]	id	tablespace ID
 @param[in]	silent	whether to silently ignore missing tablespaces
-@return the tablespace, or NULL if missing or being deleted */
+@param[in]	for_io	whether to look up the tablespace while performing I/O
+			(possibly executing TRUNCATE)
+@return	the tablespace
+@retval	NULL if missing or being deleted or truncated */
 inline
 fil_space_t*
-fil_space_acquire_low(
-	ulint	id,
-	bool	silent)
+fil_space_acquire_low(ulint id, bool silent, bool for_io = false)
 {
 	fil_space_t*	space;
 
@@ -7294,7 +7290,7 @@ fil_space_acquire_low(
 			ib_logf(IB_LOG_LEVEL_WARN, "Trying to access missing"
 				" tablespace " ULINTPF ".", id);
 		}
-	} else if (space->stop_new_ops) {
+	} else if (!for_io && space->is_stopping()) {
 		space = NULL;
 	} else {
 		space->n_pending_ops++;
@@ -7309,31 +7305,32 @@ fil_space_acquire_low(
 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 missing or being deleted */
+@param[in]	for_io	whether to look up the tablespace while performing I/O
+			(possibly executing TRUNCATE)
+@return	the tablespace
+@retval	NULL	if missing or being deleted or truncated */
 fil_space_t*
-fil_space_acquire(
-	ulint	id)
+fil_space_acquire(ulint id, bool for_io)
 {
-	return(fil_space_acquire_low(id, false));
+	return(fil_space_acquire_low(id, false, for_io));
 }
 
 /** Acquire a tablespace that may not exist.
 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 missing or being deleted */
+@return	the tablespace
+@retval	NULL if missing or being deleted */
 fil_space_t*
-fil_space_acquire_silent(
-	ulint	id)
+fil_space_acquire_silent(ulint id)
 {
 	return(fil_space_acquire_low(id, true));
 }
 
 /** Release a tablespace acquired with fil_space_acquire().
 @param[in,out]	space	tablespace to release  */
 void
-fil_space_release(
-	fil_space_t*	space)
+fil_space_release(fil_space_t* space)
 {
 	mutex_enter(&fil_system->mutex);
 	ut_ad(space->magic_n == FIL_SPACE_MAGIC_N);
@@ -7351,8 +7348,7 @@ If NULL, use the first fil_space_t on fil_system->space_list.
 @return pointer to the next fil_space_t.
 @retval NULL if this was the last*/
 fil_space_t*
-fil_space_next(
-	fil_space_t*	prev_space)
+fil_space_next(fil_space_t* prev_space)
 {
 	fil_space_t*		space=prev_space;
 
@@ -7375,8 +7371,8 @@ fil_space_next(
 		fil_ibd_create(), or dropped, or !tablespace. */
 		while (space != NULL
 			&& (UT_LIST_GET_LEN(space->chain) == 0
-				|| space->stop_new_ops
-				|| space->purpose != FIL_TABLESPACE)) {
+			    || space->is_stopping()
+			    || space->purpose != FIL_TABLESPACE)) {
 			space = UT_LIST_GET_NEXT(space_list, space);
 		}
 

storage/innobase/include/fil0fil.h

@@ -645,27 +645,28 @@ fil_write_flushed_lsn_to_data_files(
 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 missing or being deleted */
+@param[in]	for_io	whether to look up the tablespace while performing I/O
+			(possibly executing TRUNCATE)
+@return	the tablespace
+@retval	NULL if missing or being deleted or truncated */
 fil_space_t*
-fil_space_acquire(
-	ulint	id)
+fil_space_acquire(ulint id, bool for_io = false)
 	MY_ATTRIBUTE((warn_unused_result));
 
 /** Acquire a tablespace that may not exist.
 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 missing or being deleted */
+@return	the tablespace
+@retval	NULL if missing or being deleted */
 fil_space_t*
-fil_space_acquire_silent(
-	ulint	id)
+fil_space_acquire_silent(ulint id)
 	MY_ATTRIBUTE((warn_unused_result));
 
 /** Release a tablespace acquired with fil_space_acquire().
 @param[in,out]	space	tablespace to release  */
 void
-fil_space_release(
-	fil_space_t*	space);
+fil_space_release(fil_space_t* space);
 
 /** Return the next fil_space_t.
 Once started, the caller must keep calling this until it returns NULL.

storage/xtradb/btr/btr0scrub.cc

@@ -129,15 +129,15 @@ btr_scrub_lock_dict_func(ulint space_id, bool lock_to_close_table,
 		* if we don't lock to close a table, we check if space
 		* is closing, and then instead give up
 		*/
-		if (lock_to_close_table == false) {
-			fil_space_t* space = fil_space_acquire(space_id);
-			if (!space || space->stop_new_ops) {
-				if (space) {
-					fil_space_release(space);
-				}
+		if (lock_to_close_table) {
+		} else if (fil_space_t* space = fil_space_acquire(space_id)) {
+			bool stopping = space->is_stopping();
+			fil_space_release(space);
+			if (stopping) {
 				return false;
 			}
-			fil_space_release(space);
+		} else {
+			return false;
 		}
 		os_thread_sleep(250000);
 
@@ -197,18 +197,15 @@ btr_scrub_table_close_for_thread(
 		return;
 	}
 
-	fil_space_t* space = fil_space_acquire(scrub_data->space);
-
-	/* If tablespace is not marked as stopping perform
-	the actual close. */
-	if (space && !space->is_stopping()) {
-		mutex_enter(&dict_sys->mutex);
-		/* perform the actual closing */
-		btr_scrub_table_close(scrub_data->current_table);
-		mutex_exit(&dict_sys->mutex);
-	}
-
-	if (space) {
+	if (fil_space_t* space = fil_space_acquire(scrub_data->space)) {
+		/* If tablespace is not marked as stopping perform
+		the actual close. */
+		if (!space->is_stopping()) {
+			mutex_enter(&dict_sys->mutex);
+			/* perform the actual closing */
+			btr_scrub_table_close(scrub_data->current_table);
+			mutex_exit(&dict_sys->mutex);
+		}
 		fil_space_release(space);
 	}
 

storage/xtradb/buf/buf0buf.cc

@@ -6410,14 +6410,12 @@ buf_page_decrypt_after_read(
 		return (true);
 	}
 
-	fil_space_t* space = fil_space_acquire(bpage->space);
-
-	fil_space_crypt_t* crypt_data = space->crypt_data;
+	fil_space_t* space = fil_space_acquire(bpage->space, true);
 
 	/* Page is encrypted if encryption information is found from
 	tablespace and page contains used key_version. This is true
 	also for pages first compressed and then encrypted. */
-	if (!crypt_data) {
+	if (!space || !space->crypt_data) {
 		key_version = 0;
 	}
 
@@ -6501,6 +6499,8 @@ buf_page_decrypt_after_read(
 		}
 	}
 
-	fil_space_release(space);
+	if (space != NULL) {
+		fil_space_release(space);
+	}
 	return (success);
 }