Fix a data race for cfd->log_number_ #6249
Conversation
|
Will update with tests. |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| if (new_descriptor_log) { | ||
| // if we are writing out new snapshot make sure to persist max column | ||
| // family. | ||
| if (column_family_set_->GetMaxColumnFamily() > 0) { | ||
| first_writer.edit_list.front()->SetMaxColumnFamily( | ||
| column_family_set_->GetMaxColumnFamily()); | ||
| } | ||
| for (const auto* cfd : *column_family_set_) { |
There was a problem hiding this comment.
Can you please add a comment here or at the local variable definition describing why we are saving the state here for use later? (Saved while holding mutex to avoid race condition.)
| if (new_descriptor_log) { | ||
| // if we are writing out new snapshot make sure to persist max column | ||
| // family. | ||
| if (column_family_set_->GetMaxColumnFamily() > 0) { | ||
| first_writer.edit_list.front()->SetMaxColumnFamily( | ||
| column_family_set_->GetMaxColumnFamily()); | ||
| } | ||
| for (const auto* cfd : *column_family_set_) { | ||
| assert(curr_state.find(cfd->GetID()) == curr_state.end()); | ||
| curr_state[cfd->GetID()] = {cfd->GetLogNumber()}; |
There was a problem hiding this comment.
If you're going to create a struct/class rather than storing the single field (naked), I suggest the logic for populating the struct go into the struct itself, as I would expect it always to come from the same kind of source.
Or maybe put that logic in a SaveMutableStateForManifest function. Suggestion.
There was a problem hiding this comment.
OK, I put the logic in struct's constructor.
Summary: A thread calling LogAndApply may release db mutex when calling WriteCurrentStateToManifest() which reads cfd->log_number_. Another thread can call SwitchMemtable() and writes to cfd->log_number_. Solution is to cache the cfd->log_number_ before releasing mutex in LogAndApply. Test Plan (on devserver): ``` $COMPILE_WITH_TSAN=1 $./db_stress --acquire_snapshot_one_in=10000 --avoid_unnecessary_blocking_io=1 --block_size=16384 --bloom_bits=16 --bottommost_compression_type=zstd --cache_index_and_filter_blocks=1 --cache_size=1048576 --checkpoint_one_in=1000000 --checksum_type=kxxHash --clear_column_family_one_in=0 --compact_files_one_in=1000000 --compact_range_one_in=1000000 --compaction_ttl=0 --compression_max_dict_bytes=16384 --compression_type=zstd --compression_zstd_max_train_bytes=0 --continuous_verification_interval=0 --db=/dev/shm/rocksdb/rocksdb_crashtest_blackbox --db_write_buffer_size=1048576 --delpercent=5 --delrangepercent=0 --destroy_db_initially=0 --enable_pipelined_write=0 --flush_one_in=1000000 --format_version=5 --get_live_files_and_wal_files_one_in=1000000 --index_block_restart_interval=5 --index_type=0 --log2_keys_per_lock=22 --long_running_snapshots=0 --max_background_compactions=20 --max_bytes_for_level_base=10485760 --max_key=1000000 --max_manifest_file_size=16384 --max_write_batch_group_size_bytes=16 --max_write_buffer_number=3 --memtablerep=skip_list --mmap_read=0 --nooverwritepercent=1 --open_files=500000 --ops_per_thread=100000000 --partition_filters=0 --pause_background_one_in=1000000 --periodic_compaction_seconds=0 --prefixpercent=5 --progress_reports=0 --readpercent=45 --recycle_log_file_num=0 --reopen=20 --set_options_one_in=10000 --snapshot_hold_ops=100000 --subcompactions=2 --sync=1 --target_file_size_base=2097152 --target_file_size_multiplier=2 --test_batches_snapshots=1 --use_direct_io_for_flush_and_compaction=0 --use_direct_reads=0 --use_full_merge_v1=0 --use_merge=0 --use_multiget=1 --verify_checksum=1 --verify_checksum_one_in=1000000 --verify_db_one_in=100000 --write_buffer_size=4194304 --write_dbid_to_manifest=1 --writepercent=35 ```
riversand963
force-pushed
the
fix-race-cfd-lognumber
branch
from
e6bece9
to
f5aaf74
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 merged this pull request in 1aaa145. |
Summary:
A thread calling LogAndApply may release db mutex when calling
WriteCurrentStateToManifest() which reads cfd->log_number_. Another thread can
call SwitchMemtable() and writes to cfd->log_number_.
Solution is to cache the cfd->log_number_ before releasing mutex in
LogAndApply.
Test Plan (on devserver):
Then repeat the following multiple times, e.g. 100 after compiling with tsan.