search

facebook / rocksdb

A library that provides an embeddable, persistent key-value store for fast storage.
http://rocksdb.org
GNU General Public License v2.0
27.85k stars 6.2k forks source link

Fix TSAN-reported data race with uncache_aggressiveness #12753

Closed pdillinger closed 3 weeks ago

pdillinger commented 3 weeks ago

Summary: Data race reported on BlockBasedTableReader::Rep::uncache_aggressiveness because apparently a file can be marked obsolete through multiple table cache references in parallel. Using a relaxed atomic should resolve the race quite reasonably, especially considering this is a rare case and the racing writes should be storing the same value anyway.

Test Plan: watch for TSAN crash test results

facebook-github-bot commented 3 weeks ago

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pdillinger commented 3 weeks ago

Crash test failure is a known pre-existing (but undiagnosed) issue.

pdillinger commented 3 weeks ago

IIRC when releasing a handle, the cache can tell the caller if the reference was the last one; is this something we could leverage to mark table readers obsolete in a thread-safe manner?

I think there's a cart before the horse problem: ~BlockBasedTableReader is called by releasing the last reference in the table cache. At that point, we don't have the table reader to help us know what block cache entries to clear up. So because of that, and because destructors don't take parameters, our option is to forewarn the reader about the file being obsolete (about to be deleted) before the destructor is (or might be) called. And it's easier to make that forewarning thread safe than to ensure it happens just once. :)

facebook-github-bot commented 3 weeks ago

@pdillinger merged this pull request in facebook/rocksdb@961468f92e210d9fa5e3b8ca7aef1d5b2c3d4b5c.