Skip to content

Commit

Permalink
MDEV-16115 Hang after reducing innodb_encryption_threads
Browse files Browse the repository at this point in the history 
The test encryption.create_or_replace would occasionally fail,
because some fil_space_t::n_pending_ops would never be decremented.

fil_crypt_find_space_to_rotate(): If rotate_thread_t::should_shutdown()
holds due to innodb_encryption_threads having been reduced, do
release the reference.

fil_space_remove_from_keyrotation(), fil_space_next(): Declare the
functions static, simplify a little, and define in the same compilation
unit with the only caller, fil_crypt_find_space_to_rotate().

fil_crypt_key_mutex: Remove (unused).
  • Loading branch information
@dr-m
dr-m committed Aug 10, 2020
1 parent 3e3da16 commit 7f67ef1
Show file tree
Hide file tree
Showing 6 changed files with 222 additions and 374 deletions.
136 changes: 111 additions & 25 deletions storage/innobase/fil/fil0crypt.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,8 @@ Modified Jan Lindström jan.lindstrom@mariadb.com
#include "ha_prototypes.h" // IB_LOG_
#include <my_crypt.h>

/** Mutex for keys */
static ib_mutex_t fil_crypt_key_mutex;

static bool fil_crypt_threads_inited = false;

#ifdef UNIV_PFS_MUTEX
static mysql_pfs_key_t fil_crypt_key_mutex_key;
#endif

/** Is encryption enabled/disabled */
UNIV_INTERN ulong srv_encrypt_tables = 0;

Expand Down Expand Up @@ -133,9 +126,6 @@ UNIV_INTERN
void
fil_space_crypt_init()
{
mutex_create(fil_crypt_key_mutex_key,
&fil_crypt_key_mutex, SYNC_NO_ORDER_CHECK);

fil_crypt_throttle_sleep_event = os_event_create();

mutex_create(fil_crypt_stat_mutex_key,
Expand All @@ -152,7 +142,6 @@ fil_space_crypt_cleanup()
{
os_event_free(fil_crypt_throttle_sleep_event);
fil_crypt_throttle_sleep_event = NULL;
mutex_free(&fil_crypt_key_mutex);
mutex_free(&crypt_stat_mutex);
}

Expand Down Expand Up @@ -1451,6 +1440,109 @@ fil_crypt_return_iops(
fil_crypt_update_total_stat(state);
}

/** Remove space from key rotation list if there are no pending operations. */
static void fil_space_remove_from_keyrotation(fil_space_t *space)
{
ut_ad(mutex_own(&fil_system->mutex));

if (space->n_pending_ops == 0 && space->is_in_rotation_list)
{
space->is_in_rotation_list= false;
ut_a(UT_LIST_GET_LEN(fil_system->rotation_list) > 0);
UT_LIST_REMOVE(rotation_list, fil_system->rotation_list, space);
}
}

/** Return the next tablespace from key rotation list.
@param space previous tablespace (NULL to start from the beginning)
@return pointer to the next tablespace (with n_pending_ops incremented)
@retval NULL if this was the last */
static fil_space_t *fil_space_keyrotate_next(fil_space_t *space)
{
ut_ad(mutex_own(&fil_system->mutex));

if (UT_LIST_GET_LEN(fil_system->rotation_list) == 0)
{
if (space)
{
space->n_pending_ops--;
fil_space_remove_from_keyrotation(space);
}

return NULL;
}

if (!space)
{
space= UT_LIST_GET_FIRST(fil_system->rotation_list);
/* We can trust that space is not NULL because we
checked list length above */
}
else
{
space->n_pending_ops--;
fil_space_t *old = space;
space= UT_LIST_GET_NEXT(rotation_list, space);
fil_space_remove_from_keyrotation(old);
}

/* Skip spaces that are being created by fil_ibd_create(),
or dropped. Note that rotation_list contains only
space->purpose == FIL_TABLESPACE. */
while (space && (!UT_LIST_GET_LEN(space->chain) || space->is_stopping()))
{
fil_space_t *old = space;
space= UT_LIST_GET_NEXT(rotation_list, space);
fil_space_remove_from_keyrotation(old);
}

if (space)
space->n_pending_ops++;

return space;
}

/** Return the next tablespace.
@param space previous tablespace (NULL to start from the beginning)
@return pointer to the next tablespace (with n_pending_ops incremented)
@retval NULL if this was the last */
static fil_space_t *fil_space_next(fil_space_t *space)
{
mutex_enter(&fil_system->mutex);
ut_ad(!space || space->n_pending_ops);

if (!srv_fil_crypt_rotate_key_age)
space= fil_space_keyrotate_next(space);
else if (!space)
{
space= UT_LIST_GET_FIRST(fil_system->space_list);
/* We can trust that space is not NULL because at least the
system tablespace is always present and loaded first. */
space->n_pending_ops++;
}
else
{
ut_ad(space->n_pending_ops > 0);
/* Move on to the next fil_space_t */
space->n_pending_ops--;
space= UT_LIST_GET_NEXT(space_list, space);

/* Skip abnormal tablespaces or those that are being created by
fil_ibd_create(), or being dropped. */
while (space &&
(UT_LIST_GET_LEN(space->chain) == 0 ||
space->is_stopping() || space->purpose != FIL_TABLESPACE))
space= UT_LIST_GET_NEXT(space_list, space);

if (space)
space->n_pending_ops++;
}

mutex_exit(&fil_system->mutex);

return space;
}

/***********************************************************************
Search for a space needing rotation
@param[in,out] key_state Key state
Expand Down Expand Up @@ -1485,14 +1577,7 @@ fil_crypt_find_space_to_rotate(
state->space = NULL;
}

/* If key rotation is enabled (default) we iterate all tablespaces.
If key rotation is not enabled we iterate only the tablespaces
added to keyrotation list. */
if (srv_fil_crypt_rotate_key_age) {
state->space = fil_space_next(state->space);
} else {
state->space = fil_space_keyrotate_next(state->space);
}
state->space = fil_space_next(state->space);

while (!state->should_shutdown() && state->space) {
fil_crypt_read_crypt_data(state->space);
Expand All @@ -1505,14 +1590,15 @@ fil_crypt_find_space_to_rotate(
return true;
}

if (srv_fil_crypt_rotate_key_age) {
state->space = fil_space_next(state->space);
} else {
state->space = fil_space_keyrotate_next(state->space);
}
state->space = fil_space_next(state->space);
}

if (state->space) {
fil_space_release(state->space);
state->space = NULL;
}

/* if we didn't find any space return iops */
/* no work to do; release our allocation of I/O capacity */
fil_crypt_return_iops(state);

return false;
Expand Down
134 changes: 0 additions & 134 deletions storage/innobase/fil/fil0fil.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6476,137 +6476,3 @@ fil_space_release_for_io(fil_space_t* space)
space->n_pending_ios--;
mutex_exit(&fil_system->mutex);
}

/** Return the next fil_space_t.
Once started, the caller must keep calling this until it returns NULL.
fil_space_acquire() and fil_space_release() are invoked here which
blocks a concurrent operation from dropping the tablespace.
@param[in] prev_space Pointer to the previous fil_space_t.
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*/
UNIV_INTERN
fil_space_t*
fil_space_next(fil_space_t* prev_space)
{
fil_space_t* space=prev_space;

mutex_enter(&fil_system->mutex);

if (prev_space == NULL) {
space = UT_LIST_GET_FIRST(fil_system->space_list);

/* We can trust that space is not NULL because at least the
system tablespace is always present and loaded first. */
space->n_pending_ops++;
} else {
ut_ad(space->n_pending_ops > 0);

/* Move on to the next fil_space_t */
space->n_pending_ops--;
space = UT_LIST_GET_NEXT(space_list, space);

/* Skip spaces that are being created by
fil_ibd_create(), or dropped, or !tablespace. */
while (space != NULL
&& (UT_LIST_GET_LEN(space->chain) == 0
|| space->is_stopping()
|| space->purpose != FIL_TABLESPACE)) {
space = UT_LIST_GET_NEXT(space_list, space);
}

if (space != NULL) {
space->n_pending_ops++;
}
}

mutex_exit(&fil_system->mutex);

return(space);
}

/**
Remove space from key rotation list if there are no more
pending operations.
@param[in] space Tablespace */
static
void
fil_space_remove_from_keyrotation(
fil_space_t* space)
{
ut_ad(mutex_own(&fil_system->mutex));
ut_ad(space);

if (space->n_pending_ops == 0 && space->is_in_rotation_list) {
space->is_in_rotation_list = false;
ut_a(UT_LIST_GET_LEN(fil_system->rotation_list) > 0);
UT_LIST_REMOVE(rotation_list, fil_system->rotation_list, space);
}
}


/** Return the next fil_space_t from key rotation list.
Once started, the caller must keep calling this until it returns NULL.
fil_space_acquire() and fil_space_release() are invoked here which
blocks a concurrent operation from dropping the tablespace.
@param[in] prev_space Pointer to the previous fil_space_t.
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*/
UNIV_INTERN
fil_space_t*
fil_space_keyrotate_next(
fil_space_t* prev_space)
{
fil_space_t* space = prev_space;
fil_space_t* old = NULL;

mutex_enter(&fil_system->mutex);

if (UT_LIST_GET_LEN(fil_system->rotation_list) == 0) {
if (space) {
ut_ad(space->n_pending_ops > 0);
space->n_pending_ops--;
fil_space_remove_from_keyrotation(space);
}
mutex_exit(&fil_system->mutex);
return(NULL);
}

if (prev_space == NULL) {
space = UT_LIST_GET_FIRST(fil_system->rotation_list);

/* We can trust that space is not NULL because we
checked list length above */
} else {
ut_ad(space->n_pending_ops > 0);

/* Move on to the next fil_space_t */
space->n_pending_ops--;

old = space;
space = UT_LIST_GET_NEXT(rotation_list, space);

fil_space_remove_from_keyrotation(old);
}

/* Skip spaces that are being created by fil_ibd_create(),
or dropped. Note that rotation_list contains only
space->purpose == FIL_TABLESPACE. */
while (space != NULL
&& (UT_LIST_GET_LEN(space->chain) == 0
|| space->is_stopping())) {

old = space;
space = UT_LIST_GET_NEXT(rotation_list, space);
fil_space_remove_from_keyrotation(old);
}

if (space != NULL) {
space->n_pending_ops++;
}

mutex_exit(&fil_system->mutex);

return(space);
}
28 changes: 0 additions & 28 deletions storage/innobase/include/fil0fil.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -699,34 +699,6 @@ UNIV_INTERN
void
fil_space_release_for_io(fil_space_t* space);

/** Return the next fil_space_t.
Once started, the caller must keep calling this until it returns NULL.
fil_space_acquire() and fil_space_release() are invoked here which
blocks a concurrent operation from dropping the tablespace.
@param[in,out] prev_space Pointer to the previous fil_space_t.
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 */
UNIV_INTERN
fil_space_t*
fil_space_next(
fil_space_t* prev_space)
MY_ATTRIBUTE((warn_unused_result));

/** Return the next fil_space_t from key rotation list.
Once started, the caller must keep calling this until it returns NULL.
fil_space_acquire() and fil_space_release() are invoked here which
blocks a concurrent operation from dropping the tablespace.
@param[in,out] prev_space Pointer to the previous fil_space_t.
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*/
UNIV_INTERN
fil_space_t*
fil_space_keyrotate_next(
fil_space_t* prev_space)
MY_ATTRIBUTE((warn_unused_result));

/** Wrapper with reference-counting for a fil_space_t. */
class FilSpace
{
Expand Down
Loading

0 comments on commit 7f67ef1

Please sign in to comment.