Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

thread sanitizer finds real bugs in fractal tree software #396

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

thread sanitizer finds real bugs in fractal tree software #396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
e082a96
remove test timeout property so that we can use the ctest --timeout p…
prohaska7 Sep 14, 2016
5f357d6
gcc7 raises switch/case fall through issues. annotate valid cases.
prohaska7 Nov 15, 2017
9608927
fix == ?= operator precedence issues identified by gcc7
prohaska7 Nov 15, 2017
6db8c7d
avoid gcc7 warning about truncated snprintf format strings
prohaska7 Nov 15, 2017
6e45858
pfs needs to destroy its key objects when the ft library is uninitial…
prohaska7 Nov 17, 2017
0e65fe9
Merge commit 'e082a9696e14c5f7bfe286962280f85cffed663d' into rm-ctest…
prohaska7 Nov 18, 2017
1ba1dfe
fix directory_lock test, was passing uninitialzed data to comparison …
prohaska7 Nov 19, 2017
d7dbfa6
turn off 2 clang warnings to compile with clang 4.0.1
prohaska7 Nov 19, 2017
1c63ac8
add explicit unsafe operators to race tools so that tsan suppression …
prohaska7 Nov 29, 2017
9071df1
fix data races in portability tests
prohaska7 Nov 29, 2017
525665e
fix data races in util tests
prohaska7 Nov 29, 2017
6bf0a38
fix data race in minicron
prohaska7 Nov 29, 2017
2cd2b35
fix data races in locktree tests
prohaska7 Nov 29, 2017
525177f
mark cachetable status counter updates as multi-thread unsafe
prohaska7 Nov 30, 2017
ab4d26b
mark flusher status counter updates as multi-thread unsafe
prohaska7 Nov 30, 2017
f9d363f
fix data races in ft tests
prohaska7 Nov 30, 2017
23bc4d9
mark unsafe reads in checkpoint status
prohaska7 Dec 1, 2017
0daea59
fix data races in ydb tests
prohaska7 Dec 8, 2017
14e5507
mark locktree read of current lock memory as unsafe
prohaska7 Dec 8, 2017
e9e6c78
add ydb status variable updates as racy by design
prohaska7 Dec 8, 2017
d80bee7
add ydb status variable updates as racy by design
prohaska7 Dec 8, 2017
6ff7045
mark ft read of in memory logical rows as racy by design
prohaska7 Dec 8, 2017
976328e
The thread sanitizer reports thousands of warnings when running the f…
prohaska7 Dec 12, 2017
271ce1b
fix data races on locktree current lock memory by using c++ atomics
prohaska7 Dec 12, 2017
1dee4ce
fix locktree data race on reference count by using c++ atomics
prohaska7 Dec 12, 2017
530a157
fix races on cachetable current size and evictor thread state
prohaska7 Dec 14, 2017
2ddec4b
fix data races in ydb tests
prohaska7 Dec 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmake_modules/TokuSetupCompiler.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ set_cflags_if_supported(
-fno-rtti
-fno-exceptions
-Wno-error=nonnull-compare
-Wno-missing-braces
-Wno-address-of-packed-member
)
## set_cflags_if_supported_named("-Weffc++" -Weffcpp)

Expand Down
7 changes: 4 additions & 3 deletions ft/cachetable/cachetable-internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved.
#include <util/kibbutz.h>
#include <util/nb_mutex.h>
#include <util/partitioned_counter.h>
#include <atomic>

//////////////////////////////////////////////////////////////////////////////
//
Expand Down Expand Up @@ -498,12 +499,12 @@ class evictor {

pair_list* m_pl;
cachefile_list* m_cf_list;
int64_t m_size_current; // the sum of the sizes of the pairs in the cachetable
std::atomic_int64_t m_size_current; // the sum of the sizes of the pairs in the cachetable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes 'class evictor' not a POD and triggers static assertion via 'ENSURE_POD(evictor);' in cachetable.cc around line 3611

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, all of these atomic_type_t specializations are incorrect as they do not exist in all supported back versions of glib, must use std::atomic instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so for backward compatibility, use std::atomic in place of std::atomic_int?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, these all need to be the older style specializations std::atomic<uint32_t>, etc... I easily changed those but the bigger issue is that by adding this to evictor (and possibly other class/structs) introduces non trivial layout and construction/destruction/copy semantics, which then causes failure of std::is_pod and triggers the static_assertion in ENSURE_POD. I do not recall offhand what the need for evictor and others to be POD was and what it would take to eliminate it.

Alternately, since the evictor is really just a singleton, a hack could be to just to move this to global scope, but that IMHO is going the wrong direction in the greater scheme to properly "C++-ize" the FT library.

int64_t m_size_cloned_data; // stores amount of cloned data we have, only used for engine status
// changes to these two values are protected
// by ev_thread_lock
int64_t m_size_reserved; // How much memory is reserved (e.g., by the loader)
int64_t m_size_evicting; // the sum of the sizes of the pairs being written
std::atomic_int64_t m_size_evicting; // the sum of the sizes of the pairs being written

// these are constants
int64_t m_low_size_watermark; // target max size of cachetable that eviction thread aims for
Expand Down Expand Up @@ -531,7 +532,7 @@ class evictor {
// in init, set to false during destroy
bool m_run_thread;
// bool that states if the eviction thread is currently running
bool m_ev_thread_is_running;
std::atomic_bool m_ev_thread_is_running;
// period which the eviction thread sleeps
uint32_t m_period_in_seconds;
// condition variable on which client threads wait on when sleeping
Expand Down
29 changes: 13 additions & 16 deletions ft/cachetable/cachetable.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ static void cachetable_free_pair(PAIR p) {
void *write_extraargs = p->write_extraargs;
PAIR_ATTR old_attr = p->attr;

cachetable_evictions++;
toku_unsafe_inc(&cachetable_evictions);
PAIR_ATTR new_attr = p->attr;
// Note that flush_callback is called with write_me false, so the only purpose of this
// call is to tell the ft layer to evict the node (keep_me is false).
Expand Down Expand Up @@ -1715,8 +1715,8 @@ int toku_cachetable_get_and_pin_with_dep_pairs (
// The pair being fetched will be marked as pending if a checkpoint happens during the
// fetch because begin_checkpoint will mark as pending any pair that is locked even if it is clean.
cachetable_fetch_pair(ct, cachefile, p, fetch_callback, read_extraargs, true);
cachetable_miss++;
cachetable_misstime += get_tnow() - t0;
toku_unsafe_inc(&cachetable_miss);
toku_unsafe_add(&cachetable_misstime, get_tnow() - t0);

// If the lock_type requested was a PL_READ, we downgrade to PL_READ,
// but if the request was for a PL_WRITE_CHEAP, we don't bother
Expand Down Expand Up @@ -2063,8 +2063,8 @@ int toku_cachetable_get_and_pin_nonblocking(
// no list lock is held
uint64_t t0 = get_tnow();
cachetable_fetch_pair(ct, cf, p, fetch_callback, read_extraargs, false);
cachetable_miss++;
cachetable_misstime += get_tnow() - t0;
toku_unsafe_inc(&cachetable_miss);
toku_unsafe_add(&cachetable_misstime, get_tnow() - t0);

if (ct->ev.should_client_thread_sleep()) {
ct->ev.wait_for_cache_pressure_to_subside();
Expand Down Expand Up @@ -2206,7 +2206,7 @@ int toku_cachefile_prefetch(CACHEFILE cf, CACHEKEY key, uint32_t fullhash,
p = ct->list.find_pair(cf, key, fullhash);
// if not found then create a pair and fetch it
if (p == NULL) {
cachetable_prefetches++;
toku_unsafe_inc(&cachetable_prefetches);
ct->list.pair_unlock_by_fullhash(fullhash);
ct->list.write_list_lock();
ct->list.pair_lock_by_fullhash(fullhash);
Expand Down Expand Up @@ -2764,7 +2764,7 @@ void toku_cachetable_begin_checkpoint (CHECKPOINTER cp, TOKULOGGER UU(logger)) {


// This is used by the cachetable_race test.
static volatile int toku_checkpointing_user_data_status = 0;
static std::atomic_int toku_checkpointing_user_data_status = {0};
static void toku_cachetable_set_checkpointing_user_data_status (int v) {
toku_checkpointing_user_data_status = v;
}
Expand Down Expand Up @@ -3076,7 +3076,7 @@ int cleaner::run_cleaner(void) {
int r;
uint32_t num_iterations = this->get_iterations();
for (uint32_t i = 0; i < num_iterations; ++i) {
cleaner_executions++;
toku_unsafe_inc(&cleaner_executions);
m_pl->read_list_lock();
PAIR best_pair = NULL;
int n_seen = 0;
Expand Down Expand Up @@ -3779,15 +3779,15 @@ void evictor::change_pair_attr(PAIR_ATTR old_attr, PAIR_ATTR new_attr) {
// the size of the cachetable.
//
void evictor::add_to_size_current(long size) {
(void) toku_sync_fetch_and_add(&m_size_current, size);
m_size_current += size; // atomic add fetch
}

//
// Subtracts the given size from the evictor's current
// approximation of the cachetable size.
//
void evictor::remove_from_size_current(long size) {
(void) toku_sync_fetch_and_sub(&m_size_current, size);
m_size_current -= size; // atomic sub fetch
}

//
Expand All @@ -3812,14 +3812,11 @@ void evictor::remove_cloned_data_size(long size) {
uint64_t evictor::reserve_memory(double fraction, uint64_t upper_bound) {
toku_mutex_lock(&m_ev_thread_lock);
uint64_t reserved_memory = fraction * (m_low_size_watermark - m_size_reserved);
if (0) { // debug
fprintf(stderr, "%s %" PRIu64 " %" PRIu64 "\n", __PRETTY_FUNCTION__, reserved_memory, upper_bound);
}
if (upper_bound > 0 && reserved_memory > upper_bound) {
reserved_memory = upper_bound;
}
m_size_reserved += reserved_memory;
(void) toku_sync_fetch_and_add(&m_size_current, reserved_memory);
m_size_current += reserved_memory; // atomic add fetch
this->signal_eviction_thread_locked();
toku_mutex_unlock(&m_ev_thread_lock);

Expand All @@ -3833,7 +3830,7 @@ uint64_t evictor::reserve_memory(double fraction, uint64_t upper_bound) {
// TODO: (Zardosht) comment this function
//
void evictor::release_reserved_memory(uint64_t reserved_memory){
(void) toku_sync_fetch_and_sub(&m_size_current, reserved_memory);
m_size_current -= reserved_memory; // atomic sub fetch
toku_mutex_lock(&m_ev_thread_lock);
m_size_reserved -= reserved_memory;
// signal the eviction thread in order to possibly wake up sleeping clients
Expand Down Expand Up @@ -3992,7 +3989,7 @@ bool evictor::run_eviction_on_pair(PAIR curr_in_clock) {
// extract and use these values so that we don't risk them changing
// out from underneath us in calculations below.
n_in_table = m_pl->m_n_in_table;
size_current = m_size_current;
size_current = unsafe_read_size_current();

// now that we have the pair mutex we care about, we can
// release the read list lock and reacquire it at the end of the function
Expand Down
4 changes: 2 additions & 2 deletions ft/cachetable/checkpoint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ checkpoint_safe_checkpoint_unlock(void) {

void
toku_multi_operation_client_lock(void) {
if (locked_mo)
if (toku_unsafe_fetch(locked_mo))
(void) toku_sync_fetch_and_add(&CP_STATUS_VAL(CP_CLIENT_WAIT_ON_MO), 1);
toku_pthread_rwlock_rdlock(&multi_operation_lock);
}
Expand All @@ -215,7 +215,7 @@ void toku_low_priority_multi_operation_client_unlock(void) {

void
toku_checkpoint_safe_client_lock(void) {
if (locked_cs)
if (toku_unsafe_fetch(locked_cs))
(void) toku_sync_fetch_and_add(&CP_STATUS_VAL(CP_CLIENT_WAIT_ON_CS), 1);
toku_mutex_lock(&checkpoint_safe_mutex);
checkpoint_safe_lock.read_lock();
Expand Down
38 changes: 19 additions & 19 deletions ft/ft-flusher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,22 +99,22 @@ find_heaviest_child(FTNODE node)

static void
update_flush_status(FTNODE child, int cascades) {
FL_STATUS_VAL(FT_FLUSHER_FLUSH_TOTAL)++;
FL_STATUS_INC(FT_FLUSHER_FLUSH_TOTAL);
if (cascades > 0) {
FL_STATUS_VAL(FT_FLUSHER_FLUSH_CASCADES)++;
FL_STATUS_INC(FT_FLUSHER_FLUSH_CASCADES);
switch (cascades) {
case 1:
FL_STATUS_VAL(FT_FLUSHER_FLUSH_CASCADES_1)++; break;
FL_STATUS_INC(FT_FLUSHER_FLUSH_CASCADES_1); break;
case 2:
FL_STATUS_VAL(FT_FLUSHER_FLUSH_CASCADES_2)++; break;
FL_STATUS_INC(FT_FLUSHER_FLUSH_CASCADES_2); break;
case 3:
FL_STATUS_VAL(FT_FLUSHER_FLUSH_CASCADES_3)++; break;
FL_STATUS_INC(FT_FLUSHER_FLUSH_CASCADES_3); break;
case 4:
FL_STATUS_VAL(FT_FLUSHER_FLUSH_CASCADES_4)++; break;
FL_STATUS_INC(FT_FLUSHER_FLUSH_CASCADES_4); break;
case 5:
FL_STATUS_VAL(FT_FLUSHER_FLUSH_CASCADES_5)++; break;
FL_STATUS_INC(FT_FLUSHER_FLUSH_CASCADES_5); break;
default:
FL_STATUS_VAL(FT_FLUSHER_FLUSH_CASCADES_GT_5)++; break;
FL_STATUS_INC(FT_FLUSHER_FLUSH_CASCADES_GT_5); break;
}
}
bool flush_needs_io = false;
Expand All @@ -124,9 +124,9 @@ update_flush_status(FTNODE child, int cascades) {
}
}
if (flush_needs_io) {
FL_STATUS_VAL(FT_FLUSHER_FLUSH_NEEDED_IO)++;
FL_STATUS_INC(FT_FLUSHER_FLUSH_NEEDED_IO);
} else {
FL_STATUS_VAL(FT_FLUSHER_FLUSH_IN_MEMORY)++;
FL_STATUS_INC(FT_FLUSHER_FLUSH_IN_MEMORY);
}
}

Expand Down Expand Up @@ -685,7 +685,7 @@ ftleaf_split(
{

paranoid_invariant(node->height == 0);
FL_STATUS_VAL(FT_FLUSHER_SPLIT_LEAF)++;
FL_STATUS_INC(FT_FLUSHER_SPLIT_LEAF);
if (node->n_children) {
// First move all the accumulated stat64info deltas into the first basement.
// After the split, either both nodes or neither node will be included in the next checkpoint.
Expand Down Expand Up @@ -862,7 +862,7 @@ ft_nonleaf_split(
FTNODE* dependent_nodes)
{
//VERIFY_NODE(t,node);
FL_STATUS_VAL(FT_FLUSHER_SPLIT_NONLEAF)++;
FL_STATUS_INC(FT_FLUSHER_SPLIT_NONLEAF);
toku_ftnode_assert_fully_in_memory(node);
int old_n_children = node->n_children;
int n_children_in_a = old_n_children/2;
Expand Down Expand Up @@ -1018,7 +1018,7 @@ flush_this_child(
static void
merge_leaf_nodes(FTNODE a, FTNODE b)
{
FL_STATUS_VAL(FT_FLUSHER_MERGE_LEAF)++;
FL_STATUS_INC(FT_FLUSHER_MERGE_LEAF);
toku_ftnode_assert_fully_in_memory(a);
toku_ftnode_assert_fully_in_memory(b);
paranoid_invariant(a->height == 0);
Expand Down Expand Up @@ -1094,7 +1094,7 @@ static void balance_leaf_nodes(
// If b is bigger then move stuff from b to a until b is the smaller.
// If a is bigger then move stuff from a to b until a is the smaller.
{
FL_STATUS_VAL(FT_FLUSHER_BALANCE_LEAF)++;
FL_STATUS_INC(FT_FLUSHER_BALANCE_LEAF);
// first merge all the data into a
merge_leaf_nodes(a,b);
// now split them
Expand Down Expand Up @@ -1173,7 +1173,7 @@ maybe_merge_pinned_nonleaf_nodes(
*did_rebalance = false;
toku_init_dbt(splitk);

FL_STATUS_VAL(FT_FLUSHER_MERGE_NONLEAF)++;
FL_STATUS_INC(FT_FLUSHER_MERGE_NONLEAF);
}

static void
Expand Down Expand Up @@ -1627,16 +1627,16 @@ update_cleaner_status(
FTNODE node,
int childnum)
{
FL_STATUS_VAL(FT_FLUSHER_CLEANER_TOTAL_NODES)++;
FL_STATUS_INC(FT_FLUSHER_CLEANER_TOTAL_NODES);
if (node->height == 1) {
FL_STATUS_VAL(FT_FLUSHER_CLEANER_H1_NODES)++;
FL_STATUS_INC(FT_FLUSHER_CLEANER_H1_NODES);
} else {
FL_STATUS_VAL(FT_FLUSHER_CLEANER_HGT1_NODES)++;
FL_STATUS_INC(FT_FLUSHER_CLEANER_HGT1_NODES);
}

unsigned int nbytesinbuf = toku_bnc_nbytesinbuf(BNC(node, childnum));
if (nbytesinbuf == 0) {
FL_STATUS_VAL(FT_FLUSHER_CLEANER_EMPTY_NODES)++;
FL_STATUS_INC(FT_FLUSHER_CLEANER_EMPTY_NODES);
} else {
if (nbytesinbuf > FL_STATUS_VAL(FT_FLUSHER_CLEANER_MAX_BUFFER_SIZE)) {
FL_STATUS_VAL(FT_FLUSHER_CLEANER_MAX_BUFFER_SIZE) = nbytesinbuf;
Expand Down
91 changes: 89 additions & 2 deletions ft/ft-ops.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4880,6 +4880,94 @@ static void toku_pfs_keys_init(const char *toku_instr_group_name) {
toku_instr_probe_1 = new toku_instr_probe(*fti_probe_1_key);
}

static void toku_pfs_keys_destroy(void) {
delete kibbutz_mutex_key;
delete minicron_p_mutex_key;
delete queue_result_mutex_key;
delete tpool_lock_mutex_key;
delete workset_lock_mutex_key;
delete bjm_jobs_lock_mutex_key;
delete log_internal_lock_mutex_key;
delete cachetable_ev_thread_lock_mutex_key;
delete cachetable_disk_nb_mutex_key;
delete safe_file_size_lock_mutex_key;
delete cachetable_m_mutex_key;
delete checkpoint_safe_mutex_key;
delete ft_ref_lock_mutex_key;
delete ft_open_close_lock_mutex_key;
delete loader_error_mutex_key;
delete bfs_mutex_key;
delete loader_bl_mutex_key;
delete loader_fi_lock_mutex_key;
delete loader_out_mutex_key;
delete result_output_condition_lock_mutex_key;
delete block_table_mutex_key;
delete rollback_log_node_cache_mutex_key;
delete txn_lock_mutex_key;
delete txn_state_lock_mutex_key;
delete txn_child_manager_mutex_key;
delete txn_manager_lock_mutex_key;
delete treenode_mutex_key;
delete locktree_request_info_mutex_key;
delete locktree_request_info_retry_mutex_key;
delete manager_mutex_key;
delete manager_escalation_mutex_key;
delete db_txn_struct_i_txn_mutex_key;
delete manager_escalator_mutex_key;
delete indexer_i_indexer_lock_mutex_key;
delete indexer_i_indexer_estimate_lock_mutex_key;

delete tokudb_file_data_key;
delete tokudb_file_load_key;
delete tokudb_file_tmp_key;
delete tokudb_file_log_key;

delete fti_probe_1_key;

delete extractor_thread_key;
delete fractal_thread_key;
delete io_thread_key;
delete eviction_thread_key;
delete kibbutz_thread_key;
delete minicron_thread_key;
delete tp_internal_thread_key;

delete result_state_cond_key;
delete bjm_jobs_wait_key;
delete cachetable_p_refcount_wait_key;
delete cachetable_m_flow_control_cond_key;
delete cachetable_m_ev_thread_cond_key;
delete bfs_cond_key;
delete result_output_condition_key;
delete manager_m_escalator_done_key;
delete lock_request_m_wait_cond_key;
delete queue_result_cond_key;
delete ws_worker_wait_key;
delete rwlock_wait_read_key;
delete rwlock_wait_write_key;
delete rwlock_cond_key;
delete tp_thread_wait_key;
delete tp_pool_wait_free_key;
delete frwlock_m_wait_read_key;
delete kibbutz_k_cond_key;
delete minicron_p_condvar_key;
delete locktree_request_info_retry_cv_key;

delete multi_operation_lock_key;
delete low_priority_multi_operation_lock_key;
delete cachetable_m_list_lock_key;
delete cachetable_m_pending_lock_expensive_key;
delete cachetable_m_pending_lock_cheap_key;
delete cachetable_m_lock_key;
delete result_i_open_dbs_rwlock_key;
delete checkpoint_safe_rwlock_key;
delete cachetable_value_key;
delete safe_file_size_lock_rwlock_key;

delete cachetable_disk_nb_rwlock_key;
delete toku_instr_probe_1;
}

int toku_ft_layer_init(void) {
int r = 0;

Expand Down Expand Up @@ -4916,8 +5004,7 @@ void toku_ft_layer_destroy(void) {
toku_status_destroy();
partitioned_counters_destroy();
toku_scoped_malloc_destroy();

delete toku_instr_probe_1;
toku_pfs_keys_destroy();

// Portability must be cleaned up last
toku_portability_destroy();
Expand Down
8 changes: 7 additions & 1 deletion ft/ft-status.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,13 @@ extern FT_FLUSHER_STATUS_S fl_status;

#define FL_STATUS_VAL(x) fl_status.status[FT_FLUSHER_STATUS_S::x].value.num


#define FL_STATUS_INC(x) FL_STATUS_VAL(x)++
#if defined(__has_feature)
#if __has_feature(thread_sanitizer)
#undef FL_STATUS_INC
#define FL_STATUS_INC(x) toku_unsafe_inc(&FL_STATUS_VAL(x))
#endif
#endif

//
// Hot Flusher
Expand Down
Loading