mirror of
https://github.com/facebook/rocksdb.git
synced 2026-10-23 08:09:02 +03:00
main
391 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Peter Dillinger
|
539532377d |
Remove Solaris support (#15449)
Summary: Solaris support was contributed by outside users starting with |
||
Anand Ananthabhotla
|
a3b0f187ff |
Add FileSystem::IsPreallocationSupported() to skip WAL truncation on open (#15395)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15395 **Why the change is required:** During `DB::Open`, RocksDB reopens the last WAL file and truncates it to its logical size. This is done even when every column family's log number has already advanced past that WAL, so the file is obsolete and will be deleted later in the same open. The only purpose of the truncation is to release space preallocated beyond the logical end of file (e.g. by `fallocate` with `FALLOC_FL_KEEP_SIZE`); `GetFileSize()` reports the apparent size, so truncating to it can only free preallocated blocks. On a FileSystem that never preallocates, this work is pure overhead: a `GetFileSize`, a `ReopenWritableFile`, a no-op `Truncate` (the default `FSWritableFile::Truncate` returns OK without doing anything), and a `Close` on every open. On remote or append-oriented FileSystems the reopen can be expensive. **Previous behavior:** - If recovery flushed all data, or the WALs contained no data (including when all WALs are older than the min log number to keep), `MaybeFlushFinalMemtableOrRestoreActiveLogFiles()` always called `GetLogSizeAndMaybeTruncate()` on the last WAL. In read-only mode it still issued a `GetFileSize()` whose result was discarded. - With `avoid_flush_during_recovery`, `RestoreAliveLogFiles()` always reopened and truncated the last live WAL. **New behavior:** A FileSystem can report that it never preallocates space by returning false from the new `FileSystem::IsPreallocationSupported()`. When it does, DB open skips reopening and truncating the WAL. In the flushed / no-data path it also skips the unused `GetFileSize()`, so no I/O is issued against the old WAL. The read-only no-data path no longer issues the discarded `GetFileSize()` either. **Implementation:** - `include/rocksdb/file_system.h`: new `virtual bool IsPreallocationSupported()`, defaulting to `true`. `FileSystemWrapper` forwards it to the target. - The policy lives in the two callers in `db_impl_open.cc`; `GetLogSizeAndMaybeTruncate()` is unchanged. `RestoreAliveLogFiles()` still calls `GetFileSize()` because the size is needed for `alive_wal_files_` and `wals_total_size_`. **Safety, compatibility, and limitations:** - The default is `true`, so every existing FileSystem, including ones that override `SupportedOps()` without calling the base class, keeps the current behavior. This is why it is a separate virtual function instead of an opt-in `FSSupportedOps` bit, which would silently disable truncation for those FileSystems. - No built-in FileSystem overrides the default: Posix, Windows, `MockFileSystem`, and `LegacyFileSystemWrapper` all report `true`, so their WAL truncation behavior is unchanged. Posix deliberately reports `true` even in builds without `ROCKSDB_FALLOCATE_PRESENT`: WALs may have been preallocated by a different build running on the same machine, and a build-dependent answer would leave that space unreclaimed. - The function describes what the FileSystem can do, not per-file options such as `allow_fallocate` or build configuration. A WAL may have been preallocated by an earlier process with different options, so gating on the current options could leak space. - A FileSystem that wrongly returns false can only leak preallocated space, in archived WALs (`WAL_ttl_seconds` / `WAL_size_limit_MB`) or across a crash loop before obsolete WALs are deleted. Data is unaffected. Reviewed By: joshkang97 Differential Revision: D125049741 fbshipit-source-id: ac9795f2045c46911cd86b8faacef5aa49043496 |
||
Josh Kang
|
014e1c43c1 |
Apply readability fixes (isolate decls, braces) (#15394)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15394 ## Summary RocksDB's `.clang-tidy` enables several readability checks that existing code still violates: `readability-isolate-declaration` (476 findings), `readability-braces-around-statements` (49), `readability-redundant-string-init` (25), `modernize-use-using` (10), `misc-definitions-in-headers` (3) and `readability-duplicate-include` (1). They keep getting flagged whenever nearby lines change. This diff fixes 553 of the 564; upstream has since fixed the other 11. What changed: - Multi-variable declarations such as `std::string a, b;` are split into one declaration per variable. Pointer and reference declarators are kept per variable. - Braces are added around single-statement `if`/`else`/`for`/`while` bodies, and empty spin loops get an empty `{}` body. - `std::string s = ""` / `member("")` initializers are removed. - Remaining `typedef`s in the touched C++ files become `using`. C headers and `extern "C"` code are untouched. - `inline` is added to non-template function definitions in test headers. - A duplicate `#include "util/udt_util.h"` in `db_impl.cc` is removed. These are clang-tidy's own fix-its, applied mechanically and then clang-formatted on the changed lines. Three kinds of broken fix-it output were repaired by hand. Removing a `member("")` initializer left a dangling comma in constructor initializer lists (`ExternalSstFileInfo`, `ColumnFamilyMetaData` and others). The function-type typedefs in `block_test.cc` came out malformed. Empty `while (cond);` spin loops got a `{;` body. Splitting declarations also exposed two vectors in `db_etc2_test.cc` to `performance-inefficient-vector-operation`, so a `reserve()` was added for each. The lint bot reports every finding in a file a diff touches. This diff splits declarations in `options/configurable_test.cc`, so it also fixes that file's 14 older `cert-err58-cpp` and `cppcoreguidelines-avoid-non-const-global-variables` findings, which come from seven mutable file-level maps. Six are now `const` and built where they are used (function-local statics, or locals in their only user). The unused `imm_struct_option_info` is deleted. Compatibility: no behavior change. In public headers, the only edits are removed redundant `""` initializers, split declarations and added braces. Reviewed By: rban1 Differential Revision: D123186310 fbshipit-source-id: f9c8e88b00e997db0196f8725c7c8d880cf798da |
||
|
Josh Kang
|
4b983da055 |
Spell out special member functions (rule of five) (#15387)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15387 ## Summary RocksDB's `.clang-tidy` enables `cppcoreguidelines-special-member-functions`, `cppcoreguidelines-virtual-class-destructor` and the noexcept-move checks, and clang warns `-Wdeprecated-copy-with-user-provided-dtor`. A full scan reports 476 findings. In most of them, a class declares a destructor (or copy operations) but leaves the rest of copy/move implicit. That has two costs. Readers can't tell whether a class may be copied or moved. And resource-owning classes keep an implicit copy that would double-free if anyone ever used it. This diff resolves 453 of the 476. Approach, picked per class from least to most invasive: 1. An empty user-provided destructor becomes `= default`. This covers most interface classes, such as `virtual ~X() {}` -> `= default`. If the class is still copied, that would only rename the deprecated-copy warning, so the destructor is removed instead when nothing needs it (`Customizable`, `TableFactory` and `InternalKeyComparator` inherit a virtual one; `IteratorWrapperBase` isn't polymorphic), and `MemTableRep::KeyComparator` gets defaulted copy and move. 2. Classes whose copy is already deleted get explicitly deleted move operations too. Move was already unavailable, so this only documents it. 3. Resource-owning classes that are never copied get deleted copy and move operations. Examples: the C API wrapper structs, which run a `destructor_` callback (edited in `tools/c_api_gen/c_base.cc` and regenerated into `db/c.cc`); `ShardedCache`, which frees its shards; iterator wrappers; and RAII handles. 4. Value types that are copied or moved get the missing operations as `= default`, which keeps their current behavior. 5. Exception specifications of existing functions are unchanged, except for `WriteBatch`'s move assignment (below): no other `noexcept` is added or removed. Only the move operations this diff newly declares (`Comparator`, `FileSampledStats` and the thread-local `Entry`) are `noexcept`; they copy only scalars or atomics. `WriteBatch`'s copy and move assignment destroyed `*this` and placement-new'ed a new `WriteBatch` over it. That is undefined behavior when `*this` is a subclass, and if the copy constructor threw, `*this` was left destroyed and was later destroyed again. Move assignment now assigns member-wise and ends in the same state as the move constructor, which leaves `has_key_with_ts_`, the timestamp-size tracking and the other fields it doesn't copy at their defaults. `rep_` is swapped through a temporary, because a plain string move-assignment can keep the old buffer (e.g. a large batch reset with `batch = WriteBatch()` would keep its memory). Nothing in it can throw, so it is now `noexcept`. Copy assignment copies into a temporary and moves it in, so a throwing copy leaves `*this` unchanged. Bugs this exposed and fixed: - `RestoreAfterCopyOrCreateWorkItem` (backup engine): the hand-written move constructor and assignment moved only some fields, so `from_file` and `to_file` were empty after `push_back(std::move(...))`. A restore that hit a checksum mismatch then reported "While restoring -> : ..." without file names. It now uses the compiler-generated moves, which are still `noexcept`. - `ExternalSstFileIngestionJob` lived in a `std::vector`, and its destructor frees raw-owned pointers, so the implicit copy used on vector reallocation would double-free. Its file batches also point into its own inline file storage, so neither a copy nor a move is safe after `Prepare()`. Copy and move are now deleted, and `DBImpl` holds jobs through `std::unique_ptr`. Compatibility: no public class loses an operation users can call. Public interfaces mostly only get `virtual ~X() = default;`. `WriteBatch`'s move assignment becomes `noexcept`, which is not part of the mangled name, so the ABI doesn't change. `SubcompactionJobInfo` and `CompactionJobInfo` intentionally stay C++20 aggregates, so users' aggregate and designated initializers keep compiling. `Configurable`, `CompareInterface`, `WriteBatchBase`, `ObjectLibrary::Entry` and `MemTableRep::KeyComparator` declare defaulted copy and move, and `Customizable` no longer declares a destructor. As a result, `Configurable` and `Customizable` subclasses that declare no destructor or copy operations of their own now move their registered-options list instead of copying it. In-tree, these are the mutable-options adapters, the table factories, the memory allocators and test classes; all are held through smart pointers or used in place, and `Clone()` copies, so none is moved by value. Not changed (23): `IODebugContext` (custom deep copy plus a `std::shared_mutex`), `DBWithColumnFamilies` in db_bench (stored in a `std::vector` with non-movable members), `Snapshot`'s protected virtual destructor (intended), `ManagedSnapshot`, the `SubcompactionJobInfo` and `CompactionJobInfo` aggregates, and the 16 noexcept-move-constructor and noexcept-swap findings on existing functions, including `autovector`'s move assignment (adding `noexcept` would change their exception specifications). Copying `ManagedSnapshot` already double-releases the snapshot. Fixing that needs a real move implementation and is a behavior change, so it is left for a separate diff. Reviewed By: rban1 Differential Revision: D123186309 fbshipit-source-id: 6f8e57e233e12199a007407130bb9a1f6d52e8d1 |
||
|
Josh Kang
|
da82001d81 |
Mark overriding methods and destructors override (#15366)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15366 ## Summary RocksDB's `.clang-tidy` enables `modernize-use-override`, and clang reports `-Winconsistent-missing-destructor-override` when a class marks its other overrides `override` but not its destructor. A full scan finds 190 `modernize-use-override`, 100 `inconsistent-missing-destructor-override` and 2 `suggest-override` findings in existing code. Each one comes back whenever a nearby line is edited. They also leave classes where it isn't obvious which methods are overrides and which introduce new virtuals. This diff marks every overriding member function and destructor `override` and drops the redundant `virtual` on them. It also changes `final override` to plain `final`, following the check's default. Methods that introduce a new virtual keep `virtual`. Most edits are clang-tidy's own fix-its, applied mechanically and then clang-formatted. Compatibility: no signatures change. In public headers the only effect is `override` on existing overrides (mostly destructors, plus `Cache::Name()` and `Cache::GetPrintableOptions()`, whose base declarations are unconditional), so user code that subclasses these types is unaffected. Every touched class now marks all of its overrides consistently. This matters because a partially-marked class would make clang emit `-Winconsistent-missing-override` in user builds. Reviewed By: rban1 Differential Revision: D123186308 fbshipit-source-id: 1f489ea32d938e3ec0faf9abc4f7ff49e042d37a |
||
|
Josh Kang
|
928527b869 |
Fix bugs and false positives from clang-analyzer/bugprone (#15325)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15325 ## Summary RocksDB's `.clang-tidy` enables the `clang-analyzer-*` and `bugprone-*` checks. A full scan of the repo still reports 78 findings from these checks in existing code. Each time a nearby line is edited, they come back as noise, and a few of them are real defects. This diff resolves 73 of the 78; upstream has since fixed one more, and the four left are listed under "Intentionally not changed". Some fixes repair real bugs. The rest make the code's intent explicit to the analyzer without changing behavior. Real bugs fixed: - `port/mmap.cc`: `MemMapping::operator=(MemMapping&&)` ran the destructor on `*this`, `memcpy`'d over a non-trivially-copyable object, then placement-new'd the source. That is undefined behavior, and `*this` can be the base subobject of a `TypedMemMapping`. It now moves the members one by one, and the previous mapping is released through a local `MemMapping`. - `port/mmap.h`: `TypedMemMapping::operator=(MemMapping&&)` had no `return *this;`, so calling it was undefined behavior. - `cache/typed_cache.h`: `if constexpr (sizeof(TCreateContext) > 0)` is always true, so the branch meant for empty create contexts (for example `BlobContentsCreator`) could never run. The check is now `!std::is_empty_v<TCreateContext>`. Existing instantiations still call the same `Create` function. - `cassandra_row_merge_test.cc`: the test reused a vector after `std::move`-ing it into `RowValue::Merge(std::vector<RowValue>&&)`. That only worked because `Merge` happens not to consume its input. The test now rebuilds the input. Behavior-preserving changes: - Virtual calls in constructors and destructors are now explicitly qualified (for example `DBImpl::CloseImpl()` in `~DBImpl`). During construction and destruction, dispatch already resolved to these functions. - Stale `/*name=*/` argument comments now match the parameter names. - `unique_ptr::release()` after a successful cache `Insert` (where the cache takes ownership) uses `[[maybe_unused]] T* released = x.release();`, the pattern already used in `table_cache.cc`. - Removed `class Mutex;` and `class StaticMeta;` forward declarations that sat in the wrong namespace. - Added default member initializers for the `TablePropertiesCollectorFactory::Context` fields that its default constructor left uninitialized. - Smaller cleanups: a redundant `cond ? 3 : 3` in a test, `sizeof(std::string)` in a memory estimate, and an explicit `Cleanable` base move via `up_cast`. Tests that intentionally use moved-from objects now carry `NOLINT(bugprone-use-after-move)`, matching the existing convention. Intentionally not changed: enum casts in tests that check invalid-value handling; three `clang-analyzer-core.NullDereference` reports the analyzer can't prove safe (the C API timestamp out-parameters in `db/c.cc`, which are only written when the caller asked for a timestamp, and the waiter-list walk in `util/async_file_reader.cc`); bit-flag enums in public headers (`FileOpenContract`, `OptionTypeFlags`), where OR-combined values are valid; ownership transfers the analyzer can't see across translation units (`partitioned_index_reader.cc`, `fs_posix.cc`); and the intentional two-separator check in `file/filename.cc`. Reviewed By: rban1 Differential Revision: D123186307 fbshipit-source-id: 623e871286b1c73263b1309cb194692194199e0f |
||
|
Josh Kang
|
dbc2db5228 |
Make external table APIs more consistent with native tables (#15241)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15241 Native `TableFactory` implementations receive the RocksDB `WritableFileWriter` in `NewTableBuilder()` and `RandomAccessFileReader` in `NewTableReader()`. These wrappers provide buffering, checksums, rate limiting, statistics, listeners, tracing, and direct-I/O handling. External table implementations previously received a limited writable-file adapter and reopened readers from a path, bypassing parts of this wrapper logic. This change makes `WritableFileWriter` and `RandomAccessFileReader` implement `FSWritableFile` and `FSRandomAccessFile`, respectively, so the same RocksDB wrappers can be passed to external tables. `ExternalTableFactory::NewTableReader()` now receives the already-opened reader and file size. The filesystem interfaces expose standard append, flush, sync, fsync, file metadata, synchronous reads, asynchronous reads, and batched reads. `RandomAccessFileReader::SubmitReadAsync()` schedules its wrapped coroutine read on the configured filesystem read executor, and the DB table-cache and `SstFileReader` construction paths pass that filesystem through. `SstFileReader` also supplies its configured statistics, listeners, rate limiter, and file temperature, removing the need for a separate external-table metrics filesystem wrapper. For direct I/O, `MultiRead()` checks whether requests are already sorted. When alignment and merging are needed, it constructs a sorted index only for unsorted input, preserving caller request order and result mapping. Fully aligned requests using aligned caller buffers bypass this path and may be supplied in any order. Reviewed By: xingbowang Differential Revision: D121035539 fbshipit-source-id: 27cb897726c31cb1a4dc811c65c8d79e690d82b1 |
||
|
Peter Dillinger
|
8e1569b351 |
Lazy wide-column byte-range (partial) blob reads (#15067)
Summary: Builds on the lazy wide-column read API (DB::GetEntityLazy / DB::MultiGetEntityLazy) to serve true byte-range reads: resolving a sub-range of an uncompressed blob column now reads only the requested bytes from storage instead of fetching and slicing the whole column. This covers blobs in separate blob files and embedded (same-file) blobs in the SST, so a caller that knows a large value's layout can pull just the parts it needs. A partial read skips whole-record checksum verification (a strict sub-range cannot cover the checksum) and does not populate the blob cache (the cache entry holds the whole record). A compressed column, a whole-column read, a cache hit, or a read with LazyColumnReadRequest::force_verify still resolves (and verifies) the whole column; force_verify forces verification even when ReadOptions::verify_checksums is off. Mechanics: new range primitives mirror the whole-value read paths -- BlobFileReader::GetBlobRange, BlobSource::GetBlobRange, Version::GetBlobRange, and VersionBlobFetcherBase::FetchBlobRange for separate-file blobs; ReadSimpleGen2BlobRange, BlobSource::GetSimpleGen2BlobRange, and SameFileBlobReader::GetSameFileBlobRange (implemented by BlockBasedTable) for embedded blobs. ReadPathBlobResolver::ResolveColumnRange owns the partial-vs-whole decision and routes same-file references through the current SST's reader; the whole-column path also gained VersionBlobFetcherBase::FetchBlobForceVerify to honor force_verify. Observability: deferred resolve reads are attributed to a new Env::IOActivity::kLazyResolve (distinct from the kGetEntity / kMultiGetEntity of the initial entity read), and lazy resolution's storage reads are counted by new statistics rocksdb.blobdb.lazy.read.count / .read.bytes (all lazy reads) and rocksdb.blobdb.lazy.partial.read.count / .partial.bytes.saved (the byte-range subset and the bytes it avoided). The whole-column read sites gate recording on kLazyResolve so non-lazy reads are excluded. Also fixes a latent bug in LazyWideColumnsBatch::MultiResolve where an empty/default-constructed batch (null rep_) would accept a foreign standalone column (null owning_batch_rep_) through a nullptr == nullptr comparison instead of returning InvalidArgument. Follow-ups (in a local plan): unifying the blob-read API surface -- folding the whole/range method twins and the verify-policy flags (verify_checksums x force_verify) into one internal BlobReadRequest primitive (a range descriptor plus a 3-valued verify policy), which retires FetchBlobForceVerify and the whole/range method pairs; this is deferred for feature velocity and is not public-API affecting (the public force_verify bool stays). Marked in code with TODO(lazy-blob-resolution-cleanup) in db/blob/blob_fetcher.h and db/wide/read_path_blob_resolver.cc. Also cross-key coalescing (Phase 3) and async execution (Phase 4). Pull Request resolved: https://github.com/facebook/rocksdb/pull/15067 Test Plan: Extended db/wide/db_lazy_entity_test.cc: separate-file and embedded partial reads return the exact sub-span and read only the requested bytes; a partial read does not fill the blob cache and a cache hit slices with no I/O; a compressed column resolves whole and slices; force_verify does a full verified read and is detected via a tampered record even with verify_checksums off; the lazy statistics distinguish whole vs partial reads and are not touched by non-lazy reads; deferred reads carry kLazyResolve (verified with a FileSystem wrapper); and a regression test for the empty-batch ownership check. Added an IOActivityToString case in env/env_test.cc and registered the new tickers in the statistics name map. Crash test: extended the db_stress lazy-vs-eager differential (db_stress_tool/db_stress_test_base.cc) to fuzz per-column byte ranges. Each resolved column now randomly picks a whole-column read or a partial (offset, length) sub-range -- occasionally with an offset past the end or a length beyond the remainder to exercise clamping -- for both GetEntityLazy and MultiGetEntityLazy; when an eager reference is available the resolved bytes are compared against the same clamped sub-range of it. No new flags (reuses --lazy_entity_read_one_in); force_verify fuzzing remains a follow-up. Reviewed By: xingbowang Differential Revision: D115130388 Pulled By: pdillinger fbshipit-source-id: 454368dea8fae069d9de649c2c51611469d3c809 |
||
|
Peter Dillinger
|
af0eae4de3 |
Rename TransactionLogIterator to WalIterator and document its behavior (#15056)
Summary:
`TransactionLogIterator` iterates the write-ahead log. It does not return transactions, does not require `TransactionDB`, and in fact returns `Status::NotSupported()` for two of the three `TransactionDB` write policies (WritePrepared and WriteUnprepared). The name predates `TransactionDB` and comes from "transaction log" being an old synonym for WAL. Everything else in this header was already renamed away from that terminology -- `LogFile` -> `WalFile`, `VectorLogPtr` -> `VectorWalPtr` -- and the C API has long used `rocksdb_wal_iterator_t` and `rocksdb_wal_iter_*`. This finishes the job.
The public class is now `WalIterator` in `include/rocksdb/wal_iterator.h`. `include/rocksdb/transaction_log.h` remains as a forwarding header and `TransactionLogIterator` remains as an alias, both marked deprecated by comment only (not `[[deprecated]]`) so existing users compiling with -Werror are unaffected, matching how `LogFile` and `VectorLogPtr` were retired. `DB::GetUpdatesSince()` keeps its name.
More importantly, the documentation was incomplete in ways that could cost a user data, so this states plainly what the API has always done:
- It only sees the WAL. Writes made with `WriteOptions::disableWAL`, and sequence numbers consumed by `IngestExternalFile()`, advance the DB sequence number without producing a WAL record. They leave permanent holes that end iteration. This was not documented anywhere, so it was unreasonable to expect a user to know those features silently break WAL tailing.
- The three iterator states, in particular that `!Valid()` with an OK `status()` means caught up rather than finished, and that calling `Next()` again is the intended way to tail a DB. The old comment on `Next()` said "REQUIRES: Valid() to be true", which forbids the primary use case.
- `Status::TryAgain`, which a caller must handle but which appeared nowhere in the public headers.
- That `GetUpdatesSince(seq)` silently starts later when `seq` is unavailable. Combined with the above this is the sharp edge: recovering from a spent iterator means calling `GetUpdatesSince()` again, so the recovery step is exactly where data can be skipped unnoticed. A consumer applying these batches to a copy of the DB diverges permanently and undetectably if it misses one, so the header now says such consumers must verify continuity themselves using `BatchResult::sequence` and `WriteBatch::Count()`.
Almost all of this diff is mechanical: renaming the type and its files, updating `#include` paths, rewriting comments, and updating build lists (`src.mk`, `CMakeLists.txt`, `BUCK`, `java/Makefile`, `java/CMakeLists.txt`). `db/c.cc` is regenerated from `tools/c_api_gen/c_base.cc`. Reviewers should focus on the four places that are not:
1. `WalIteratorImpl::UpdateCurrentWriteBatch()` in `db/wal_iterator_impl.cc` (renamed from `db/transaction_log_impl.cc`) drops dead code. On a sequence discontinuity it set `current_status_` to `NotFound` and then called `SeekToStartSequence()`, which returns immediately when `current_status_` is not OK. That guard and the early return in `Next()` were both added by
|
||
|
Josh Kang
|
c64f682946 |
Fix IoUringBackend crash in ReadIOExecutor worker threads (#15020)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15020 Each read executor worker creates its own `IoUringBackend`, and Folly's I/O thread pool creates its worker threads and EventBases lazily. A successful calling-thread probe or `DB::Open()` therefore does not validate every worker. A later read can start a new worker; previously, an `IoUringBackend` initialization exception on that worker could escape startup and terminate the process. The previous 512-submit setup also allowed Folly's capacity retry to reduce the completion queue to 512 while retaining a 1024-entry submission queue, which the kernel rejects with `EINVAL`. Use a 256-submit limit so the 512-entry retry remains valid. Configure the shared read executor during `DB::Open()` through `DBOptions::read_io_executor_threads`, defaulting to one thread. The backend factory catches per-worker io_uring initialization failures and uses Folly's default backend. Each worker's thread-local availability marker starts false and is set true only after `IoUringBackend` construction succeeds. When construction fails, the marker remains false and `SubmitReadAsync()` performs the read synchronously. The executor's maximum thread capacity can be raised or lowered; `db_stress` requests eight threads for async DB APIs. To expose this degradation, `FSRandomAccessFile::SubmitReadAsync()` now reports whether the asynchronous path was used. `RandomAccessFileReader` increments the `FILE_SUBMIT_ASYNC_READ_FALLBACK` ticker for every submission that falls back to synchronous I/O, making the fallback visible through the DB's `Statistics`. Reviewed By: xingbowang Differential Revision: D114031018 fbshipit-source-id: 1b7f6c1cea693e5dd743ea1ada0fca1cf757b3f9 |
||
|
Josh Kang
|
a4645e864d |
Implement Get/MultiGet coroutine (#15014)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15014 Implements coroutine-backed execution behind the callback-based `DB::GetAsync` and `DB::MultiGetAsync` APIs that issue SST block reads asynchronously, so a single application thread can keep many point-lookup reads in flight at once instead of paying one blocking read per thread. The motivation is point-lookup throughput on disk-bound workloads (working set larger than the block cache), where the win comes from queue depth rather than per-read latency. The async capability rests on a small, generic FileSystem API that a `FileSystem` must implement to participate. Every method is virtual with a working synchronous default, so existing and custom `FileSystem` implementations compile and run unchanged (they simply stay synchronous): - `FSRandomAccessFile::SubmitReadAsync(req, opts, cb, dbg)` -- a fire-and-forget read: it starts the read and returns without blocking, then invokes `cb` once `req.result`/`req.status` are filled in (the callback may run on a different thread). The default implementation is synchronous -- a blocking `Read` followed immediately by `cb`. - `FileSystem::GetReadExecutor()` -- returns the `folly::Executor*` that async queries and read completions run on. The default returns `nullptr`, which the query layer treats as "async reads unavailable" and falls back to the synchronous path. It is exposed to users, so they may add additional application logic to run on the same thread pool, thus avoiding expensive context switches. - `FileSystem::SetReadIOExecutorThreads(int)` -- sizes that executor. The default is a no-op. `RandomAccessFileReader::Read`/`MultiRead` are `DEFINE_SYNC_AND_ASYNC` coroutines (new `file/random_access_file_reader_sync_and_async.h`), and `DBImpl::GetAsync`/`MultiGetAsync` bind the query to `GetReadExecutor()` with `co_withExecutor`. For a MultiRead, `SubmitMultiReadAsync` issues all of the block reads through `SubmitReadAsync` back-to-back and then `co_await`s a `folly::coro::Baton`: awaiting the baton suspends the query and releases the worker thread while the reads are in flight, and the callback for the last outstanding read posts the baton to resume it. A MultiGet's per-file `MultiGetFromSST` reads additionally fan out across the current eventbase so overlapping-level lookups run in parallel. This layer works against any executor and any `SubmitReadAsync` implementation. The Posix `FileSystem` is the concrete async implementation, and is where io_uring enters. `GetReadExecutor()` is a `folly::IOThreadPoolExecutor` whose workers are EventBases that each drive an io_uring ring (sized by `SetReadIOExecutorThreads`). `PosixRandomAccessFile::SubmitReadAsync` -- which must be invoked on one of those EventBase workers -- places the read on that worker's submission queue via `IoUringBackend::queueRead` and returns immediately: no blocking `pread`, no parked thread. When the kernel posts the completion the backend runs the callback on that same worker, and a short read re-queues its remainder from inside the completion callback, so even multi-syscall reads stay on the non-blocking path. The net effect is that one worker thread keeps many reads outstanding at once, bounded by io_uring queue depth rather than thread count. ## Thread-local state handling A coroutine suspends at every `co_await` and can resume on a different read-executor worker, and another coroutine can run on that worker in between. Any thread-local state held across a suspension point is therefore unsafe -- it can be observed or mutated by an interleaved coroutine, restored onto the wrong thread, or trip an in-use assertion. The read path handles each thread-local it touches explicitly, and introduces no new thread-local storage of its own: - SuperVersion cache: the thread-local SuperVersion slot is marked `kSVInUse` for the duration of a read, so a second coroutine resuming on the same worker would trip that assertion. The coroutine path instead takes an independent heap reference that releases the slot before any suspension -- `cfd->GetReferencedSuperVersion()` for single Get, and `kExtraSvRef = true` threaded through `MultiCFSnapshot` for MultiGet -- and releases it with `CleanupSuperVersion` (via a `Defer` for single Get). The synchronous path is unchanged and keeps using the thread-local slot (`GetAndRefSuperVersion` / `ReturnAndCleanupSuperVersion`). - perf_context / iostats_context / perf level: the timing guards that save-and-restore or accumulate into these thread-locals across an IO -- `GetPerfLevel`/`SetPerfLevel`, `IOSTATS_TIMER_GUARD(read_nanos)`, `IOSTATS_CPU_TIMER_GUARD(cpu_read_nanos)` -- are compiled out on the coroutine path (`#if defined(WITHOUT_COROUTINES)`), so none of that thread-local timing state spans a `co_await`. The tradeoff is that these perf/IO counters are not populated for coroutine reads (a known limitation of this path). - TLS-free helpers are left as-is: `StopWatch`, for instance, holds only clock/statistics pointers and a start time, so it is safe across a suspension (it can over-count wall time under oversubscription, which is a metrics-accuracy issue, not corruption). ## Benchmarks Benchmarked using 8 threads for both multiget (with/without async_io enabled), and single get on both direct-io enabled and disabled on a ~11 GB DB. Non direct-io benchmarks are not a good representation because my dev machine is capable of paging the entire DB into memory. But, it is still useful to compare against async_io=true, since that feature only benefits non direct-io. Jobs-per-thread (jpt) is the multiplier for the number of tasks being submitted to the thread pool. E.g. a threadpool of 8 threads with jpt = 4 has 32 running tasks on the thread pool. It is important to oversubscribe so that when a task suspends, another one can resume on the thread pool. MultiGet (batch 16): | IO | cache hit % | sync | async_io | coro jpt=1 | coro jpt=2 | coro jpt=5 | coro jpt=10 | | -------- | --- | ---- | -------- | ---------- | ---------- | ---------- | ----------- | | direct | 10% | 65K | 64K | 145K | 151K | 161K | 166K | | direct | 50% | 88K | 80K | 239K | 287K | 327K | 340K | | direct | 90% | 435K | 399K | 732K | 991K | 1,093K | 1,119K | | buffered | 10% | 449K | 561K | 555K | 593K | 572K | 566K | | buffered | 50% | 495K | 682K | 656K | 724K | 700K | 701K | | buffered | 90% | 600K | 878K | 828K | 1,021K | 1,048K | 1,060K | single Get: | IO | cache hit % | sync | coro jpt=1 | coro jpt=2 | coro jpt=5 | coro jpt=10 | | -------- | --- | ---- | ---------- | ---------- | ---------- | ----------- | | direct | 10% | 41K | 37K | 69K | 125K | 144K | | direct | 50% | 74K | 66K | 127K | 238K | 281K | | direct | 90% | 390K | 330K | 540K | 767K | 849K | | buffered | 10% | 403K | 291K | 369K | 447K | 447K | | buffered | 50% | 441K | 337K | 443K | 554K | 527K | | buffered | 90% | 544K | 433K | 606K | 753K | 810K | Example command (MultiGet coroutine, ~50% hit): ``` db_bench -benchmarks=multireadrandomcoroutine -coro_jobs_per_thread=10 -use_existing_db=true -num=2000000 -value_size=1024 -compression_type=none -threads=8 -batch_size=16 -use_direct_reads=true -cache_size=402653184 -reads=320000 -statistics ``` ### Coroutine vs context switch overhead Coroutines are not zero overhead, so we also compare it against the context switching of using many sync threads. Both variants were pinned to the same 8 CPUs and ran 80 concurrent MultiGet operations. | variant | throughput | | ---------------------------------------- | ------------ | | sync, 80 threads | 330.6K ops/s | | coroutine, 8 executor threads / 80 jobs | 446.0K ops/s | Reviewed By: xingbowang Differential Revision: D111116635 fbshipit-source-id: bd0ac052efe79261a552e9e66137e5e70f7cd8a1 |
||
Xingbo Wang
|
214869aacd |
Persist fault injection logs and fail fast on expected-state trace writes (#14651)
Summary: - switch fault injection error recording from an in-memory ring buffer to per-run fixed-record binary logs under `TEST_TMPDIR/fault_injection_logs` (or `/tmp/fault_injection_logs`) so crash paths survive DB reopen cleanup - keep the raw and decoded fault logs for external artifact collection/cleanup, and make `db_crashtest` print consistent blackbox/whitebox summaries after decoding - make expected-state tracing fail fast on trace write failures and document offline trace inspection via `trace_analyzer` - add coverage for binary log persistence/decoding/truncated-tail handling and keep info logs excluded from fault injection Reviewed By: hx235 Differential Revision: D101973626 fbshipit-source-id: fdcb5b6370cf92a046e09b8d3391e80eecb66c23 |
||
|
Xingbo Wang
|
521cd8b31d |
Add parallel gtest flake reproduction runner (#14827)
Summary: - Add `tools/gtest_parallel_repro.py` to reproduce gtest flaky tests that depend on process-level CPU contention and scheduler delays. - Run many fresh gtest processes concurrently, isolate each process with its own `TEST_TMPDIR`, and optionally pin the workload to a smaller CPU set with `taskset`. - Record per-run logs, write `failures.jsonl`, summarize failure keys, support stopping on first failed batch, and optionally rebuild with `COERCE_CONTEXT_SWITCH=1`. - Document when to use the runner in `CLAUDE.md` for CI-style flaky tests that single-process `--gtest_repeat` or coerce mode alone does not reproduce. - Using the new runner, we found and fixed one flaky test and improved c_test harness - Flaky test: EnvPosixTest.ReadAsyncQueueFull simulated a full io_uring submission queue by overwriting the SQE pointer after io_uring_get_sqe() had already consumed a submission slot. That left stale state in the thread-local ring, so later AbortIO tests could process an unexpected completion and crash. Add a skip_io_uring_get_sqe syncpoint before the io_uring_get_sqe() call and use it from the test so the Busy path is exercised without mutating the ring. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14827 Test Plan: - CI Reviewed By: pdillinger Differential Revision: D107758097 Pulled By: xingbowang fbshipit-source-id: 3256612ae1ae98b66901593371743b3de7e5bb53 |
||
zaidoon
|
7affaee1c4 |
Add use_direct_io_for_compaction_reads option (#14743)
Summary: Adds a new `DBOption use_direct_io_for_compaction_reads` (default false). When on, compaction-input SST files are opened with `O_DIRECT` so the sequential read-once data from compaction doesn't pollute the OS page cache and evict the hot user-read working set. User reads keep going through the buffered fast path. This protects user-read tail latency on write-heavy workloads without forcing user reads onto the existing global `use_direct_reads` knob (which pays in throughput and P50 — see the bench below). The interesting bit is that just flipping the FileOptions returned by `FileSystem::OptimizeForCompactionTableRead` doesn't actually trigger `O_DIRECT` at the kernel level. The TableCache (and `FileMetaData::pinned_reader`) is already holding buffered handles opened at flush time or at `DB::Open` via `LoadTableHandlers`. When compaction asks for an iterator, it gets back the cached buffered handle and the kernel never sees the `O_DIRECT` flag. So this PR also adds a small bypass path: - `TableCache::FindTable` / `NewIterator` learn a `open_ephemeral_table_reader` mode. When set, the pinned-reader fast path and the shared cache are skipped, `GetTableReader` is called directly with the caller's FileOptions, and ownership of the freshly opened TableReader is handed back via a `unique_ptr`. The iterator takes ownership via `RegisterCleanup` and frees the reader on destruction. - `VersionSet::MakeInputIterator` and `LevelIterator` plumb the flag through both L0 and L1+ compaction-input paths. - `CompactionJob::ProcessKeyValueCompaction` turns the bypass on when `use_direct_io_for_compaction_reads` is set, the global `use_direct_reads` is off, and `OptimizeForCompactionTableRead` produced `use_direct_reads=true` in the compaction-read FileOptions. The option is opt-in: when off, nothing changes for existing users. When on, only the compaction-input opens take the bypass path; user reads keep hitting the TableCache and the buffered fast path normally. There's also a small db_bench helper in the same PR: a new `--bgwriter_num` flag that lets the writer thread in `readwhilewriting` (and the other "while writing" variants) spread its puts across `[0, bgwriter_num)` instead of `[0, num)`. Without this the readers and writer share a key range and you can't have both a hot read subset and meaningful compaction work — this lets you have both. ### Benchmark Setup: Ubuntu 24.04 (kernel 7.0.5, OrbStack Linux VM on Apple Silicon), 14 vCPUs, virtio-blk disk, btrfs. MGLRU disabled (`echo 0 > /sys/kernel/mm/lru_gen/enabled`) so the kernel uses the classic active/inactive LRU. 14 GB DB (3.5M keys × 4 KB values), no compression. Each measurement run is pinned to a 1 GB cgroup via `systemd-run --scope -p MemoryMax=1G -p MemorySwapMax=0`. Page cache is dropped between configs. db_bench is Release build. Workload: `readwhilewriting` for 120s. 4 reader threads doing random reads over a hot key subset, plus 1 writer thread spreading overwrites across the full 3.5M-key keyspace (via `--bgwriter_num=3500000`) throttled at 200 MB/s, so there's continuous compaction running while the readers go. The size of the hot reader subset relative to available page cache controls how visible the optimization is. The Cassandra blog ([Lightfoot 2026](https://lightfoot.dev/direct-i-o-for-cassandra-compaction-cutting-p99-read-latency-by-5x/)) documented the same thing: biggest wins when the hot set is big enough to actually compete for cache, smaller wins when the hot set trivially fits, neutral when the hot set is way bigger than cache. So I ran two hot-set sizes. #### Small hot set: ~30 MB (~3% of the 1 GB cgroup) — N=5 iterations, mean (CV) `--num=7500`. The hot set is small enough that the page cache holds it without much trouble even under compaction, so the wins here are real but on the modest side. | Config | Throughput (ops/s) | Read P50 (µs) | Read P99 (µs) | Read P99.9 (µs) | Read P99.99 (µs) | |---|---|---|---|---|---| | buffered (default) | 233,477 (8.2%) | 16.09 | 82.24 | 721.0 | 2,102.5 | | direct_compaction_writes_only (existing knob alone) | 287,405 (2.8%) — **+23.1%** | 13.00 (−19.2%) | **66.77 (−18.8%)** | 553.9 (−23.2%) | 1,787.6 (−15.0%) | | direct_compaction_read_only (new knob alone) | 250,669 (2.4%) — +7.4% | 14.16 (−12.0%) | 102.99 (+25.2%) | 689.8 (−4.3%) | 1,801.3 (−14.3%) | | direct_compaction_read_write (new + existing, recommended) | 277,920 (3.3%) — **+19.0%** | **12.99 (−19.3%)** | 84.23 (+2.4%) | 613.4 (−14.9%) | **1,738.2 (−17.3%)** | | use_direct_reads=true (existing global) + write-side | 249,014 (2.5%) — +6.7% | 15.95 (−0.9%) | 68.78 (−16.4%) | **450.8 (−37.5%)** | 1,814.5 (−13.7%) | CV is 2.4–3.3% on the optimized configs (8.2% on buffered), so the deltas are real. With a hot set this small, the existing `use_direct_io_for_flush_and_compaction` knob is already doing most of the work — the new flag's main extra contribution here is P99.99 (combined wins it by ~2 points vs writes-only-alone). Worth noting: the new flag *alone* (without the existing write-side flag) improves P99.99 but regresses P99 by 25% on this small-hot-set workload, because direct compaction reads lose kernel readahead and compaction-output writes are still hitting the page cache. That regression goes away once you combine with the existing write-side flag, or once the hot set is bigger (see next table). So if you're using just one knob, use the existing one. If you're using this PR's flag, pair it with `use_direct_io_for_flush_and_compaction=true`. #### Larger hot set: ~400 MB (~40% of cache) — N=5 iterations, mean (CV) `--num=100000`. This is the case the Cassandra blog calls out — hot set big enough to actually fight compaction for cache. Their analogous setup (1M hot partitions, ~33% hot/cache) reported 1.93× p99 improvement. Numbers here are the headline: | Config | Throughput (ops/s) | Read P50 (µs) | Read P99 (µs) | Read P99.9 (µs) | Read P99.99 (µs) | |---|---|---|---|---|---| | buffered (default) | 68,959 (7.7%) | 44.81 | 541.22 | 2,225.2 | 11,334.5 | | direct_compaction_writes_only (existing knob alone) | 73,973 (10.3%) — +7.3% | 42.22 (−5.8%) | 456.27 (−15.7%) | 2,016.9 (−9.4%) | 9,190.0 (−18.9%) | | direct_compaction_read_only (new knob alone) | 84,337 (2.3%) — +22.3% | 38.66 (−13.7%) | 386.97 (−28.5%) | 1,644.8 (−26.1%) | 4,837.9 (−57.3%, 2.34×) | | direct_compaction_read_write (new + existing, recommended) | **104,923 (8.4%) — +52.2%** | **34.26 (−23.5%)** | **290.97 (−46.2%)** | **1,143.4 (−48.6%)** | **3,080.3 (−72.8%, 3.68×)** | | use_direct_reads=true (existing global) + write-side | 71,598 (9.1%) — +3.8% | 51.33 (+14.5%) | 297.91 (−45.0%) | 1,663.6 (−25.2%) | 6,530.0 (−42.4%) | Combined config gets a 3.68× p99.99 win, 1.86× p99, p50 down 23%, throughput up 52%. Same shape as the Cassandra blog's 1.93× p99 result — the improvement just lands at deeper percentiles for us because RocksDB's baseline data path is roughly 40× faster than Cassandra's (their buffered p99 was 35 ms, ours is 0.54 ms), so the cache-miss tail is further out. A few things worth calling out from this table: - The new flag is doing real work on top of the existing write-side flag here, not just shifting things around. Combined throughput is +42% over `direct_compaction_writes_only` alone, and combined p99.99 is 3× better. The existing knob alone gives a fairly modest +7% throughput / -19% p99.99 in this case — there's a clear gap that the new flag fills. - The new flag *alone* (no existing write-side flag) is also a real improvement here: +22% throughput, p99.99 down 57%. The P99 regression we saw in the small-hot-set case is gone, because the cache-protection effect now dominates the lost-readahead cost. - `use_direct_reads=true` (the existing global flag) actually regresses P50 by 14.5% in this workload — taking user reads off the page cache hurts you when the hot data could have been cached. It also gets the worst throughput of any direct config. It's not an equivalent way to get these gains. ### `compaction_readahead_size` matters when this flag is on Direct I/O bypasses kernel readahead, so RocksDB's own `DBOptions::compaction_readahead_size` becomes the only prefetch the iterator has. The default of 2 MB is enough and real users will get it automatically. **But `db_bench`'s `--compaction_readahead_size` CLI default is 0**, which defeats prefetch and makes direct compaction look slower than it actually is. If you're reproducing the numbers above, pass `--compaction_readahead_size=2097152` (or larger). - Recommended production config is `use_direct_io_for_compaction_reads=true` + `use_direct_io_for_flush_and_compaction=true`. Strongest configuration at every percentile and throughput in both benches. - The new flag is the read-side counterpart to `use_direct_io_for_flush_and_compaction`, which handles compaction-write cache pollution. They address different sources of pollution and compose. The gap between "combined" and "writes-only-alone" is 17 percentage points on p99.99 in the small-hot-set bench and 54 points in the larger one, so the new flag is contributing real value, especially as the hot set grows. - The new flag alone is also a real improvement when the hot set is big enough to compete with cache (+22% throughput, 2.34× p99.99 in the larger-hot-set bench). On a very small hot set it improves p99.99 but regresses p99, so pairing with the existing write-side flag is safer. - The benefit is workload-dependent. Small hot sets get modest tail-latency wins. Hot sets sized to actually compete for cache get the big multi-percentile wins shown above. Hot sets bigger than cache (not benched here but covered in the Cassandra blog) see no change either way — every read misses regardless. ### Reproducing Any Linux host (or a Linux VM on macOS via OrbStack / Multipass / lima): ```bash sudo apt-get install -y build-essential clang cmake git pkg-config \ libgflags-dev libsnappy-dev zlib1g-dev libbz2-dev liblz4-dev libzstd-dev cmake -DCMAKE_BUILD_TYPE=Release -DPORTABLE=1 -DWITH_GFLAGS=1 -DWITH_TESTS=0 .. make -j db_bench echo 0 | sudo tee /sys/kernel/mm/lru_gen/enabled ``` Build the source DB once, unrestricted memory: ```bash ./db_bench --benchmarks=fillrandom,compact,waitforcompaction,stats \ --db=/path/to/source_db --num=3500000 --key_size=16 --value_size=4096 \ --write_buffer_size=16777216 --target_file_size_base=16777216 \ --max_background_jobs=4 --compression_type=none --cache_size=4194304 \ --max_bytes_for_level_base=67108864 --disable_wal=1 --sync=0 ``` For each config, copy `source_db -> scratch_db`, run `sync && echo 3 > /proc/sys/vm/drop_caches`, then: ```bash sudo systemd-run --scope -p MemoryMax=1G -p MemorySwapMax=0 \ ./db_bench --use_existing_db=1 \ --benchmarks=readwhilewriting,stats --db=/path/to/scratch_db \ --threads=5 --duration=120 --statistics=true --histogram=1 \ --num=7500 --bgwriter_num=3500000 \ --key_size=16 --value_size=4096 \ --write_buffer_size=16777216 --target_file_size_base=16777216 \ --max_background_jobs=4 --compression_type=none \ --cache_size=4194304 --open_files=200 \ --skip_stats_update_on_db_open=true \ --max_bytes_for_level_base=67108864 \ --benchmark_write_rate_limit=209715200 \ --compaction_readahead_size=2097152 \ --rate_limiter_bytes_per_sec=0 \ --use_direct_reads={true|false} \ --use_direct_io_for_compaction_reads={true|false} \ --use_direct_io_for_flush_and_compaction={true|false} ``` For the larger hot-set table, change `--num=7500` to `--num=100000`. The five configs in the tables: - `buffered`: all three flags false. - `direct_compaction_writes_only`: `use_direct_io_for_flush_and_compaction=true`, the other two false. This is what users have today without this PR. - `direct_compaction_read_only`: `use_direct_io_for_compaction_reads=true`, the other two false. - `direct_compaction_read_write`: `use_direct_io_for_compaction_reads=true`, `use_direct_io_for_flush_and_compaction=true`, `use_direct_reads=false`. **Recommended.** - `direct_all`: `use_direct_reads=true`, `use_direct_io_for_flush_and_compaction=true`, `use_direct_io_for_compaction_reads=false`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14743 Reviewed By: pdillinger Differential Revision: D108017601 Pulled By: xingbowang fbshipit-source-id: 4039d490d7e77b476db7a477a2f3d24738db6336 |
||
|
Xingbo Wang
|
82085868e2 |
Add read-scoped block buffers for scan reads (#14806)
Summary: Add read-scoped block buffers for scan reads. Introduce an experimental read-scoped block buffer provider API, configured through ReadOptions::read_scoped_block_buffer_provider, so supported block-based table iterator scans and MultiScan data-block reads can use caller-provided read-scoped storage for final data-block contents. When configured, supported provider-backed scan data-block reads bypass the data-block cache while preserving normal index/filter block-cache behavior. Known-uncompressed reads can attach provider cleanup to provider-backed read buffers without copying. Compressed reads decompress directly into provider-backed output, while maybe-compressed reads that turn out to be uncompressed copy once into provider-backed final contents. mmap reads ignore the provider. Extend AlignedBuffer and RandomAccessFileReader direct-I/O paths to support external aligned allocations, then use that support for read-scoped iterator, async I/O, and MultiRead scratch buffers. Centralize read-scoped I/O policy, keep coalesced async reads safe when blocks are released before completion, and validate provider lease contracts. Add focused coverage for read-scoped ownership, compressed and uncompressed blocks, direct I/O, data-block cache bypass behavior, invalid provider leases, async release handling, and stress-test provider invariants. Add public API release notes for read-scoped block buffers. Bonus change: Fixed a flaky test in ReserveThread ## Testing - CI Pull Request resolved: https://github.com/facebook/rocksdb/pull/14806 Reviewed By: anand1976 Differential Revision: D106999951 Pulled By: xingbowang fbshipit-source-id: b1f23d4bab6318b6373ba2ca99a5c4d6a842dc5a |
||
|
Xingbo Wang
|
b5922eab11 |
Add FileSystem SyncFile API (#14739)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14739 Adds public path-level file sync APIs to RocksDB so callers can ask `Env` or `FileSystem` to sync a named file without hand-rolled reopen logic. This diff: - Adds public `Env::SyncFile` and `FileSystem::SyncFile` APIs for syncing or fsyncing a file by name without requiring callers to reopen it directly. - Documents the `SyncFile` contract: filesystems may override it as a no-op when flush/close already provides durability, and RocksDB callers should use it instead of hand-rolled `ReopenWritableFile()+Sync()/Fsync()` so filesystems can reject post-close data-write reopen paths. - Implements the default `Env` and `FileSystem` behavior by reopening the file as writable, calling `Sync` or `Fsync`, closing the handle, returning the sync/fsync error ahead of a close error when both fail, and explicitly consuming the close status on the sync-failure path. - Wires `SyncFile` through the Env/FileSystem bridge and wrapper layers, including `EnvWrapper`, `FileSystemWrapper`, `CompositeEnvWrapper`, `EncryptedFileSystem`, `RemapFileSystem`, read-only file systems, mock file systems, and fault-injection wrappers. - Keeps fault-injection `SyncFile` overrides on the base default implementation so the operation still exercises the wrapper's `ReopenWritableFile`, wrapped-file `Sync`/`Fsync`, and `Close` hooks instead of bypassing them through target forwarding. - Updates external SST ingestion to call `FileSystem::SyncFile` instead of manually reopening an ingested file as writable, while preserving the `NotSupported` skip behavior and failure logging. - Adds release-note coverage for the new public API and unit coverage for default `Env`/`FileSystem` success, reopen failure, close failure, sync-versus-close failure precedence, and external SST sync behavior. There was an earlier version of this API which got reverted (PR #13987) due to internal change broken. Re-apply the change. The internal issue will be resolved in next release. Reviewed By: pdillinger Differential Revision: D104918547 fbshipit-source-id: 3f8d2d127fc962b68b423bbb90de551fe6706224 |
||
|
Xingbo Wang
|
81fa943ca0 |
Avoid retryable io_uring wait_cqe stderr in crash tests (#14779)
Summary: - Suppress retryable `io_uring_wait_cqe()` errors in `PosixFileSystem::Poll()` and `AbortIO()` before they reach stderr during crash-test SIGTERM timeout handling. - Keep terminal `wait_cqe` failures logged and fatal. - Extend the `db_crashtest.py` SIGTERM stderr filter only for retryable `Poll`/`AbortIO` wait_cqe errors and add regression coverage. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14779 Test Plan: CI ## Related - T272682963 Reviewed By: hx235 Differential Revision: D106201579 Pulled By: xingbowang fbshipit-source-id: 2331d5cdca064ad901f6af7341b4e8f15b418663 |
||
|
Peter Dillinger
|
acfa68ccff |
Fix flaky preallocation tests on btrfs, zfs, tmpfs, and overlayfs (#14744)
Summary: Fix flaky test failures in EnvPosixTestWithParam.AllocateTest and DBWALTest.TruncateLastLogAfterRecoverWithFlush that occur when running on filesystems where preallocated space is not reliably reflected in st_blocks. The tests use stat() to check st_blocks and verify that fallocate with FALLOC_FL_KEEP_SIZE actually preallocates disk space. However, on certain filesystems, this check is unreliable: 1. btrfs and zfs: Copy-on-write filesystems where preallocated extents are not reliably reflected in st_blocks, especially under load. 2. tmpfs: Memory filesystem that may not report preallocated blocks in st_blocks. 3. overlayfs: Union filesystem common in containers that may not pass through fallocate properly or report preallocated space. 4. Any filesystem where FALLOC_FL_KEEP_SIZE is not supported: The preallocation will fail silently (error ignored with PermitUncheckedError), leaving only the written data in st_blocks. Changes made: env/env_test.cc: - Added filesystem magic number definitions for TMPFS_MAGIC, OVERLAYFS_SUPER_MAGIC, and ZFS_SUPER_MAGIC - Extended the AllocateTest to skip block count checks on zfs, tmpfs, and overlayfs in addition to btrfs - Added runtime fallback: if st_blocks is less than expected, print a warning and skip the check instead of failing. This handles unknown filesystems or configurations where preallocation isn't supported. db/db_wal_test.cc: - Added includes and filesystem magic number definitions - Added ShouldSkipAllocationCheck() helper function to detect problematic filesystems - Modified TruncateLastLogAfterRecoverWithoutFlush, TruncateLastLogAfterRecoverWithFlush, TruncateLastLogAfterRecoverWALEmpty, and ReadOnlyRecoveryNoTruncate tests to skip allocation checks on problematic filesystems - Added runtime fallback checks similar to env_test.cc These changes make the tests robust against filesystem differences while still validating preallocation behavior on filesystems where it works correctly (ext4, xfs). Pull Request resolved: https://github.com/facebook/rocksdb/pull/14744 Test Plan: Many local 'make -j100 check' runs that would previously fail with good probability. Reviewed By: hx235 Differential Revision: D105331256 Pulled By: pdillinger fbshipit-source-id: 862a1512da1466cb037af15342404939b677c02a |
||
|
Peter Dillinger
|
795f3bd61f |
Fix check-sources.sh non-ASCII check and remove non-ASCII from sources (#14729)
Summary: The non-ASCII character check in check-sources.sh used git grep -P (Perl regex), which requires git compiled with PCRE support. On systems without it, the command fails with exit code 128, which is != 1 (no match), so the check always reported a violation -- effectively dead. Even in CI where git has PCRE2 support, the check was silently broken: git grep -P uses PCRE2 in UTF mode by default, which interprets [\x80-\xFF] as a Unicode codepoint range (U+0080 to U+00FF). Characters like em-dash (U+2014), arrows (U+2192), and math symbols (U+2248, etc.) fall outside that range and were not detected. Only Latin-1 Supplement characters (U+0080-U+00FF) would have been caught. Replace with LC_ALL=C git grep using bash $'[\x80-\xff]' literal byte range, which works with basic regex in the C locale, and replace all non-ASCII characters in non-excluded source files: - em-dash to -- - arrow to -> - math symbols to ASCII equivalents (~=, <=, >=) - box-drawing characters to ASCII art Also exclude .github/ from the check, as scripts there can use non-ascii without disrupting RocksDB builds on non-UTF-8 systems. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14729 Test Plan: manual / CI (make check-sources passes clean) Reviewed By: hx235 Differential Revision: D104692574 Pulled By: pdillinger fbshipit-source-id: 1d884c21056dcd83558b825a04b867f1c08e3f45 |
||
|
Xingbo Wang
|
a9ac4df630 |
env: avoid unused info in TSAN mapping helper (#14597)
Summary: TsanAnnotateMappedMemory() always builds a TsanMappedMemoryInfo payload for TEST_SYNC_POINT_CALLBACK(), but in builds where sync points compile to a no-op the local variable becomes unused and Clang fails with -Werror,-Wunused-variable. Move the explicit (void)info cast so it applies regardless of whether __SANITIZE_THREAD__ is enabled. This keeps the helper warning-free in: - non-TSAN builds - builds where TEST_SYNC_POINT_CALLBACK expands to nothing - TSAN builds that still want the SyncPoint payload for tests This was missed by CI because the warning only appears in the COMPILE_WITH_TSAN=1 + NDEBUG/release configuration. Our CI matrix covers release builds without TSAN and TSAN builds in debug mode, but not the TSAN+NDEBUG combination. No behavior change is intended. The helper still: - exposes the mapping metadata to SyncPoint-based tests - calls AnnotateNewMemory() in TSAN builds - remains a no-op with respect to runtime behavior in non-TSAN builds Pull Request resolved: https://github.com/facebook/rocksdb/pull/14597 Differential Revision: D100333855 Pulled By: xingbowang fbshipit-source-id: 3ff0b0c8a51b4959b7a36984e40ac31e49da82eb |
||
|
Xingbo Wang
|
4254c705a2 |
env: reset TSAN state for recycled mappings (#14594)
Summary: TSAN was reporting false data-race warnings after the kernel reused a virtual address for a new mapping while TSAN still associated that address range with the old mapping. This showed up in two RocksDB paths: - `io_uring` setup on helper threads vs later `io_uring` `MultiRead` access - `io_uring` setup vs file mmap reads when `use_mmap_reads` is enabled ## Changes Introduce `TsanAnnotateMappedMemory()`, which calls `AnnotateNewMemory()` as soon as a fresh mapping exists so TSAN drops any stale shadow state for the recycled address range. Apply the helper to: - io_uring SQ/CQ/SQE mappings created by `CreateIOUring()` - `PosixFileSystem` mmap-read mappings - `PosixFileSystem` raw memory-mapped file buffers - `PosixMmapFile` writable mmap region growth Also add deterministic regression tests that force virtual-address reuse with `MAP_FIXED` for both the io_uring and mmap-read cases. The tests pass mapping metadata over a pipe so teardown/remap ordering is deterministic without introducing TSAN-visible synchronization that would mask the problem. ## Testing - `COMPILE_WITH_TSAN=1 make -j192 env_test` - `env_test --gtest_filter='EnvPosixTest.SupportedOpsNoAsyncIOOnIOUringInitFailure:EnvPosixTest.IOUringAddressReuseNoTsanFalsePositive:EnvPosixTest.MmapReadAddressReuseNoTsanFalsePositive'` Pull Request resolved: https://github.com/facebook/rocksdb/pull/14594 Reviewed By: archang19 Differential Revision: D100235746 Pulled By: xingbowang fbshipit-source-id: 8ef8d85c08a7646540b9c11c3741978898a5af3d |
||
|
Peter Dillinger
|
0186937710 |
Skip AllocateTest block-count checks on btrfs (#14553)
Summary: btrfs accepts fallocate without error but uses copy-on-write, so preallocated extents are not reflected in st_blocks. This caused AllocateTest to fail spuriously on btrfs filesystems. Detect btrfs via statfs and skip only the block-count assertions, keeping the rest of the test (write, flush, close, size checks) intact. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14553 Test Plan: env_test AllocateTest passes on btrfs (skips with message) and would still enforce preallocation checks on ext4. Reviewed By: joshkang97 Differential Revision: D99307962 Pulled By: pdillinger fbshipit-source-id: 8f40c0109aa397e9ca7d9ee4496fa0614e971970 |
||
Omkar Gawde
|
51561fc2cd |
Add explicit Close() call in WriteStringToFile (#14566)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14566 **What**: Adds an explicit `file->Close()` call in `WriteStringToFile()` before the function returns, instead of relying on the `unique_ptr` destructor to close the file. **How**: In `file_system.cc`, after the optional `Sync()` and before the error-cleanup path, calls `file->Close()`. If `Close()` fails, the file is deleted just like any other write failure. Adds two unit tests in `env_test.cc`: - `WriteStringToFileClosesFile`: uses `CountedFileSystem` to verify `Close()` is called and content is written correctly. - `WriteStringToFileCloseFailureDeletesFile`: uses a custom `CloseFailFS` wrapper to inject a `Close()` failure and verify the file is deleted on error. **Why**: The previous implementation never called `Close()` explicitly — it relied on the `unique_ptr` destructor which may silently swallow write errors that occur during close (e.g., flushing buffered data). Explicit `Close()` ensures such errors are detected and propagated to the caller. Reviewed By: archang19 Differential Revision: D99462173 fbshipit-source-id: 525b5489bd0dbfc3159b007a13f9474f84d3c84e |
||
Andrew Chang
|
830347c765 |
Fix null sqe crash in ReadAsync when io_uring submission queue is full (#14521)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14521 ReadAsync calls io_uring_get_sqe() without checking for nullptr. When the io_uring submission queue is full (outstanding completions not yet reaped), io_uring_get_sqe returns NULL and the subsequent io_uring_prep_readv dereferences it, causing a segfault. MultiRead already handles this correctly by using io_uring_sq_space_left() to cap submissions. ReadAsync submits exactly one SQE per call so a simple null check with error return is sufficient. On null sqe, clean up the already-allocated Posix_IOHandle and return IOStatus::Busy so the caller can retry after reaping completions. The io_uring queue depth is kIoUringDepth (256), and each thread gets its own io_uring instance via thread-local storage. In practice the SQ rarely fills because ReadAsync calls io_uring_submit() after each io_uring_get_sqe(), immediately flushing the SQE to the kernel. The null SQE would only occur under unusual kernel backpressure where the kernel cannot consume from the SQ ring fast enough. IOStatus::Busy was chosen (over IOError) because this is a transient condition. The caller has two options: 1. Call Poll() to reap outstanding completions from the CQ, then retry ReadAsync. This mirrors how MultiRead handles queue pressure internally by capping submissions and reaping between batches. 2. Fall back to synchronous Read(). Existing callers (FilePrefetchBuffer, IODispatcher) already have synchronous fallback paths for non-OK ReadAsync status, so IOStatus::Busy naturally triggers that fallback without additional code changes. Given the rarity of this condition, the synchronous fallback is pragmatic and avoids adding retry complexity. Also adds a TEST_SYNC_POINT_CALLBACK on io_uring_get_sqe to enable test injection, and a new ReadAsyncQueueFull unit test that uses SyncPoint to force a null SQE and verifies the Busy return, handle cleanup, and no crash. Reviewed By: xingbowang Differential Revision: D98533853 fbshipit-source-id: f6d181e5c0d5154b570ff6da39e2f52e2a6aea84 |
||
|
Andrew Chang
|
de6cdbb62d |
Fix SupportedOps to verify per-thread io_uring before advertising kAsyncIO (#14514)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14514 SupportedOps previously checked only that the ThreadLocalPtr container existed (non-null), which was set during construction based on a one-time probe on the main thread. However, CreateIOUring() can fail on other threads due to kernel resource limits or flag incompatibilities, causing ReadAsync to hit "failed to init io_uring" at runtime. Now SupportedOps eagerly initializes the thread-local io_uring instance via Get() + CreateIOUring() and only advertises kAsyncIO if it succeeds. Also logs to stderr (with thread id) when io_uring init fails, and adds a TEST_SYNC_POINT_CALLBACK for testing simulated CreateIOUring failures. Reviewed By: mszeszko-meta, xingbowang Differential Revision: D98409140 fbshipit-source-id: efa92d9ac920860e95a46710c4a87e36bacbb466 |
||
Maciej Szeszko
|
b8bbee0f3e |
Log IO uring init failures to stdout (instead of stderr) (#14548)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14548 Reviewed By: archang19 Differential Revision: D98667574 fbshipit-source-id: 4f01c9d8f2ae12051d4b3e4138258091b3c9eb3d |
||
|
Andrew Chang
|
2af38206ad |
Log errno and thread ID on CreateIOUring failure (#14520)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14520 CreateIOUring() silently returns nullptr on failure, discarding the errno from io_uring_queue_init. This makes it impossible to diagnose why io_uring initialization fails on specific threads (e.g. ENOMEM from memlock limits, EINVAL from unsupported flags, EMFILE from fd exhaustion). Add a fprintf(stderr, ...) that logs strerror, errno, and pthread thread ID when io_uring_queue_init fails, so failures are diagnosable from logs without needing to reproduce. Reviewed By: xingbowang Differential Revision: D98526792 fbshipit-source-id: 1eb5042c8b62663c4d24c09f29ef7c55b90032f0 |
||
|
Andrew Chang
|
47de3a3a2c |
Fix io_uring kernel resource leak in DeleteIOUring() (#14518)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14518 When RocksDB creates an io_uring instance via `CreateIOUring()`, it calls `io_uring_queue_init()` under the hood. This function asks the Linux kernel to set up a submission queue and completion queue for async I/O — the kernel allocates file descriptors and memory-mapped ring buffers to make this work. To properly release those kernel resources, you must call `io_uring_queue_exit()` before freeing the `io_uring` struct. This function tells the kernel "I'm done with this io_uring" — it unmaps the shared memory regions and closes the kernel file descriptors. Without it, those resources leak every time an io_uring instance is destroyed. `DeleteIOUring()` (the ThreadLocalPtr destructor callback) was only doing `delete iu` — freeing the C++ struct's heap memory but never telling the kernel to clean up. This meant every thread exit leaked kernel resources. The same bug also existed in the `PosixFileSystem` constructor, which creates a temporary test io_uring to check kernel support and then did `delete new_io_uring` without cleanup. Fix: 1. Add `io_uring_queue_exit(iu)` before `delete iu` in `DeleteIOUring()`. 2. Replace bare `delete new_io_uring` in fs_posix.cc with `DeleteIOUring(new_io_uring)` to reuse the now-correct helper. Note: the error-recovery path in io_posix.cc (~line 907) already correctly calls `io_uring_queue_exit()` before `delete`, confirming this was the intended pattern that was missed in these two spots. Reviewed By: anand1976 Differential Revision: D98500559 fbshipit-source-id: bd6c10c19d9fd67cf537dd7f100ef9dc49bfe77e |
||
|
Omkar Gawde
|
1e1097d8d3 |
Fix close(-1) on failed open in btrfs rename fsync path (#14443)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14443 In `PosixDirectory::FsyncWithDirOptions()`, when handling btrfs file rename syncing, if `open()` fails and `fd` is -1, the code unconditionally calls `close(fd)`. Calling `close(-1)` is undefined behavior per POSIX (returns EBADF on Linux), and overwrites the original meaningful open error with a misleading "While closing file after fsync" error message. Fix: Guard the `close()` call with `fd >= 0`. Reviewed By: xingbowang Differential Revision: D95303407 fbshipit-source-id: 9b64b45a09e6ba41d87d164a1c094b4d22f7c186 |
||
|
Andrew Chang
|
6d6f7d825b |
Check io_uring probe result in SupportedOps (#14355)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14355 SupportedOps advertised kAsyncIO based only on the IsIOUringEnabled() weak symbol check, without verifying that the constructor's io_uring probe actually succeeded. Add a thread_local_async_read_io_urings_ null check so kAsyncIO is only reported when the probe passed. Also update the constructor to probe with the same IORING_SETUP_SINGLE_ISSUER | IORING_SETUP_DEFER_TASKRUN flags that ReadAsync and MultiRead use at runtime. Reviewed By: anand1976 Differential Revision: D93780065 fbshipit-source-id: 6f51f544b267cb39d09b49949a9485f55eeae12e |
||
|
Peter Dillinger
|
d3817f058d |
Remove deprecated DB::Open raw pointer variants (and more) (#14335)
Summary: and remove deprecated DB::MaxMemCompactionLevel(). In the process of pushing through a relatively clean refactoring of uses of the old functions, some other minor public APIs are also migrated from raw DB pointers to unique_ptr. Claude did pretty much all the work, but requiring dozens of prompts to actually push through relatively clean phase out of raw DB pointers from what needed to be touched, and leaving that code in better shape. (Hundreds of `DB*` still remain all over the place even outside C and Java bindings.) Pull Request resolved: https://github.com/facebook/rocksdb/pull/14335 Test Plan: existing tests; no functional changes intended Reviewed By: xingbowang, mszeszko-meta Differential Revision: D93523820 Pulled By: pdillinger fbshipit-source-id: e4ca22ad81cd2cfe91122d7507d7ca34fe03d043 |
||
|
Peter Dillinger
|
871f79d6ef |
Reformat source files (#14331)
Summary: probably something changed, maybe https://github.com/facebook/rocksdb/issues/14311 Full command: ``` git ls-files '*.cc' '*.h' | grep -v '^third-party/' | grep -v 'range_tree' | xargs clang-format -i ``` Pull Request resolved: https://github.com/facebook/rocksdb/pull/14331 Test Plan: CI Reviewed By: mszeszko-meta Differential Revision: D93246992 Pulled By: pdillinger fbshipit-source-id: 6bc5b97978fef8aee52823dadb6daa4bea57343d |
||
|
Peter Dillinger
|
672389fd8c |
Remove obsolete compression code and some .h->.cc movement (#14325)
Summary: In follow-up to https://github.com/facebook/rocksdb/issues/14315 Remove obsolete code replaced by new Compressor/Decompressor interface: * OLD_CompressData and OLD_UncompressData * Individual compression/decompression functions (Snappy_*, Zlib_*, BZip2_*, LZ4_*, LZ4HC_*, XPRESS_*, ZSTD_Compress, ZSTD_Uncompress) * CompressionInfo and UncompressionInfo classes * UncompressionDict class * compression::PutDecompressedSizeInfo and GetDecompressedSizeInfo The only small refactoring in this change that is not pure code removal or movement is in blob_file_builder_test.cc. Move some function implementations etc. from compression.h to compression.cc: * CompressionTypeToString, CompressionTypeFromString, CompressionOptionsToString * ZSTD_TrainDictionary (both overloads), ZSTD_FinalizeDictionary * DecompressorDict::Populate * Most compression library includes Also cleaned up other includes of compression.h, which caused some other files to need new includes. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14325 Test Plan: existing tests Reviewed By: hx235 Differential Revision: D93120580 Pulled By: pdillinger fbshipit-source-id: ab5c50db7379c0387a8c0e379642c9ea2799eae5 |
||
|
Anand Ananthabhotla
|
56cb88ec79 |
Fix racy assertion in AbortIOPartialHandlesBug test (#14319)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14319 The test asserted h1->is_finished and h2->is_finished immediately after AbortIO({H0}), before calling Poll. This is invalid because AbortIO only guarantees that handles in its abort set are finalized. Non-aborted handles' CQEs may or may not be consumed during AbortIO depending on io_uring completion ordering. If H0's two CQEs (original read + cancel) arrive before H1/H2's CQEs, AbortIO breaks out of its wait loop without processing them. Move the H1/H2 is_finished assertions to after Poll, which correctly handles either case. Also remove the racy req_count checks for non-aborted handles since Poll does not increment req_count. Reviewed By: jaykorean Differential Revision: D92848827 fbshipit-source-id: 0c09b44ceada99877e8311cff799fa94f1056545 |
||
|
Anand Ananthabhotla
|
3695cb6767 |
Fix AbortIO consuming completions for non-aborted handles (#14301)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14301 When AbortIO was called with a subset of outstanding async read handles, it would consume io_uring completions for handles NOT in the abort set but fail to finalize them. This caused subsequent Poll calls on those handles to hang forever waiting for completions that had already been consumed. The fix adds an `is_being_aborted` flag to Posix_IOHandle that is set when submitting the cancel request. When processing completions in AbortIO, handles with this flag wait for req_count==2 (original + cancel), while handles without the flag are finalized immediately at req_count==1. Also refactored the completion finalization logic into a shared FinalizeAsyncRead() helper function used by both Poll and AbortIO. Reviewed By: mszeszko-meta, archang19 Differential Revision: D92230883 fbshipit-source-id: e6d11e009a4930e5608459771990f6cf7d46d827 |
||
|
Andrew Chang
|
f84351de98 |
Fix AbortIO hang when aborting multiple io_uring handles (#14252)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14252 Fixed a bug in PosixFileSystem::AbortIO that could cause an infinite hang when aborting multiple concurrent async IO handles. The bug occurred in the completion processing loop: when an io_uring completion arrived for a handle other than the one currently being waited for (io_handles[i]), the code would increment that handle's req_count but only mark it as finished if it also matched io_handles[i]. This meant completions for other handles were consumed but those handles were never marked as finished. Later, when iterating to those handles, the code would enter io_uring_wait_cqe expecting more completions, but they had already been consumed - causing an infinite hang. The fix aligns AbortIO's completion handling with what Poll() already does: mark handles as finished whenever their completions arrive, regardless of which handle we're currently waiting for in the outer loop. Only the break statement remains conditional on matching io_handles[i]. Reviewed By: anand1976 Differential Revision: D91070044 fbshipit-source-id: 47faf5f0df3e26a2aa83444bbac623f43f560933 |
||
|
Maciej Szeszko
|
5a06787a26 |
IO uring improvements (#14158)
Summary: `PosixRandomAccessFile::MultiRead` was introduced in Dec 2019 in https://github.com/facebook/rocksdb/pull/5881. Subsequently, 2 years after, we introduced the `PosixRandomAccessFile::ReadAsync` API in https://github.com/facebook/rocksdb/pull/9578, which was reusing the same `PosixFileSystem` IO ring as `MultiRead` API, consequently writing to the very same ring's submission queue (without waiting!). This 'shared ring' design is problematic, since sequentially interleaving `ReadAsync` and `MultiRead` API calls on the very same thread might result in reading 'unknown' events in `MultiRead` leading to `Bad cqe data` errors (and therefore falsely perceived as a corruption) - which, for some services (running on local flash), in itself is a hard blocker for adopting RocksDB async prefetching ('async IO') that heavily relies on the `ReadAsync` API. This change aims to solve this problem by maintaining separate thread local IO rings for `async reads` and `multi reads` assuring correct execution. In addition, we're adding more robust error handling in form of retries for kernel interrupts and draining the queue when process is experiencing terse memory condition. Separately, we're enhancing the performance aspect by explicitly marking the rings to be written to / read from by a single thread (`IORING_SETUP_SINGLE_ISSUER` [if available]) and defer the task just before the application intends to process completions (`IORING_SETUP_DEFER_TASKRUN` [if available]). See https://man7.org/linux/man-pages/man2/io_uring_setup.2.html for reference. ## Benchmark **TLDR** There's no evident advantage of using `io_uring_submit` (relative to proposed `io_uring_submit_and_wait`) across batches of size 10, 250 and 1000 simulating significantly-less, close-to and 4x-above `kIoUringDepth` batch size. `io_uring_submit` might be more appealing if (at least) one of the IOs is slow (which was NOT the case during the benchmark). More notably, with this PR switching from `io_uring_submit_and_wait` -> `io_uring_submit` can be done with a single line change due to implemented guardrails (we can followup with adding optional config for true ring semantics [if needed]). **Compilation** ``` DEBUG_LEVEL=0 make db_bench ``` **Create DB** ``` ./db_bench \ --db=/db/testdb_2.5m_k100_v6144_16kB_LZ4 \ --benchmarks=fillseq \ --num=2500000 \ --key_size=100 \ --value_size=6144 \ --compression_type=LZ4 \ --block_size=16384 \ --seed=1723056275 ``` **LSM** * L0: 2 files, L1: 5, L2: 49, L3: 79 * Each file is roughly ~35M in size ### MultiReadRandom (with caching disabled) Each run was preceded by OS page cache cleanup with `echo 1 | sudo tee /proc/sys/vm/drop_caches`. ``` ./db_bench \ --use_existing_db=true \ --db=/db/testdb_2.5m_k100_v6144_16kB_LZ4 \ --compression_type=LZ4 \ --benchmarks=multireadrandom \ --num= **<N>** \ --batch_size= **<B>** \ --io_uring_enabled=true \ --async_io=false \ --optimize_multiget_for_io=false \ --threads=4 \ --cache_size=0 \ --use_direct_reads=true \ --use_direct_io_for_flush_and_compaction=true \ --cache_index_and_filter_blocks=false \ --pin_l0_filter_and_index_blocks_in_cache=false \ --pin_top_level_index_and_filter=false \ --prepopulate_block_cache=0 \ --row_cache_size=0 \ --use_blob_cache=false \ --use_compressed_secondary_cache=false ``` | B=10; N=100,000 | B = 250; N=80,000 | B = 1,000; N=20,000 -- | -- | -- | -- baseline | 31.5 (± 0.4) us/op | 17.5 (± 0.5) us/op | 13.5 (± 0.4) us/op io_uring_submit_and_wait | 31.5 (± 0.6) us/op | 17.7 (± 0.4) us/op | 13.6 (± 0.4) us/op io_uring_submit | 31.5 (± 0.6) us/op | 17.5 (± 0.5) us/op | 13.4 (± 0.45) us/op ### Specs | Property | Value -- | -- RocksDB | version 10.9.0 Date | Tue Dec 9 15:57:03 2025 CPU | 56 * Intel Sapphire Rapids (T10 SPR) Kernel version | 6.9.0-0_fbk12_0_g28f2d09ad102 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14158 Reviewed By: anand1976 Differential Revision: D88172809 Pulled By: mszeszko-meta fbshipit-source-id: 5198de3d2f18f76fee661a2ec5f447e79ba06fbd |
||
anand76
|
0eb5b43b4f |
Change PosixWritableFile Truncate to reseek to new end of file (#14088)
Summary: Change PosixWritableFile's Truncate to the new end offset. This ensures that future appends are written with no holes or overwrites. RocksDB doesn't guarantee this in the FileSystem contract, and its left up to the specific implementation. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14088 Reviewed By: cbi42 Differential Revision: D85786398 Pulled By: anand1976 fbshipit-source-id: 3520d9d6336362f5128a17bbf396297d821a5da3 |
||
ngina
|
3687dc4ad3 |
Add prefetch feature enum to FSSupportedOps (#13917)
Summary: **Problem:** RocksDB was making unnecessary prefetch system calls on file systems that don't support prefetch operations, potentially leading to wasted CPU cycles. **Fix:** Add kFSPrefetch to FSSupportedOps enum to allow file systems to indicate prefetch support capability. File systems can now opt out of prefetch calls by not setting this field. **Backwards compatibility:** File systems that don't override SupportedOps() continue to receive prefetch calls exactly as before. Only file systems that explicitly opt out by not setting kFSPrefetch will avoid the calls. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13917 Test Plan: - Added a new test in block_based_table_reader. - Run existing tests: ```make prefetch_test && ./prefetch_test``` Reviewed By: anand1976 Differential Revision: D81607145 Pulled By: nmk70 fbshipit-source-id: 3bbefa05919034e8776ea4e4540cdc695cdc6d3f |
||
Changyu Bi
|
54373ba0e8 |
Revert "Create a new API FileSystem::SyncFile for file sync (#13762)" (#13987)
Summary:
This is causing some internal failure, we decide to revert this for now until we have a proper fix.
This reverts commit
|
||
|
Changyu Bi
|
2620c85638 |
Support async IO for MultiScan (#13932)
Summary: add option MultiScanArgs::use_async_io option and implementation for using ReadAsync() for multiscan. Read requests are submitted during Prepare() and polled during actual scanning. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13932 Test Plan: - updated existing unit test to use async_io. - crash test: `python3 -u ./tools/db_crashtest.py whitebox --iterpercent=60 --prefix_size=-1 --prefixpercent=0 --readpercent=0 --test_batches_snapshots=0 --use_multiscan=1 --read_fault_one_in=0 --kill_random_test=88888 --interval=60 --multiscan_use_async_io=1 --mmap_read=0` Benchmark: - Default multiscan benchmark: ``` Set up: /db_bench --benchmarks="fillseq,compact" --disable_wal=1 --threads=1 --num_levels=1 --compaction_style=2 --fifo_compaction_max_table_files_size_mb=1000 --write_buffer_size=268435456 Without async IO: ./db_bench --db="/tmp/rocksdbtest-543376/dbbench" --use_existing_db=1 --benchmarks=multiscan --disable_auto_compactions=1 --seek_nexts=100 --threads=32 --duration=10 --statistics=1 --use_direct_reads=1 --multiscan_use_async_io=0 multiscan : 415.569 micros/op 75805 ops/sec 10.355 seconds 784968 operations; (multscans:24999) rocksdb.read.async.micros COUNT : 0 With asycn IO: ./db_bench --db="/tmp/rocksdbtest-543376/dbbench" --use_existing_db=1 --benchmarks=multiscan --disable_auto_compactions=1 --seek_nexts=100 --threads=32 --duration=10 --statistics=1 --use_direct_reads=1 --multiscan_use_async_io=1 multiscan : 413.236 micros/op 76044 ops/sec 10.375 seconds 788968 operations; (multscans:24999) rocksdb.read.async.micros COUNT : 3916499 Similar performance. ``` - Larger scan, more scans per multiscan, do not coalesce IO so that async IO can progress while scanning, and use one thread: ``` multiscan_stride = 1000 multiscan_size = 100 seek_nexts = 1000 ./db_bench --db="/tmp/rocksdbtest-543376/dbbench" --use_existing_db=1 --benchmarks=multiscan --disable_auto_compactions=1 --threads=1 --duration=10 --statistics=0 --use_direct_reads=1 --cache_size=2097152 --multiscan_size=100 --multiscan_stride=1000 --seek_nexts=1000 --seed=1 --multiscan_coalesce_threshold=0 --multiscan_use_async_io=0 Without async IO: multiscan : 20495.205 micros/op 48 ops/sec 10.002 seconds 488 operations; (multscans:488) With async IO: multiscan : 18337.883 micros/op 54 ops/sec 10.013 seconds 546 operations; (multscans:546) ~10% improvement in throughput ``` Reviewed By: xingbowang Differential Revision: D82077818 Pulled By: cbi42 fbshipit-source-id: 66e32cf4039183c4841827409286dfbaa6dfbcd8 |
||
|
Andrew Chang
|
d87e598f70 |
Update error logging and status reporting for unsupported iouring (#13936)
Summary: We should add error logging to be able to pinpoint why RocksDB is returning status `NotSupported` for `ReadAsync`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13936 Test Plan: Look at logs (and client logs of error status) Reviewed By: anand1976 Differential Revision: D82141529 Pulled By: archang19 fbshipit-source-id: c71b70967457be35ef5168321d449f96b2b9441d |
||
|
Andrew Chang
|
85f1ba572e |
Add support for custom IOActivity types (#13924)
Summary: There are some internal use cases that do not map cleanly onto the existing `IOActivity` enums. This PR creates new custom IOActivity types that internal users can use as they see fit. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13924 Test Plan: Wrote a simple unit test Reviewed By: pdillinger Differential Revision: D82029992 Pulled By: archang19 fbshipit-source-id: a3e23c360baa96cd2e9adf570e71c6e43947bfc8 |
||
|
Peter Dillinger
|
86bb0c0d1b |
Use C++20 in public API, fix CI (#13915)
Summary: A follow-up to https://github.com/facebook/rocksdb/issues/13904 which was incomplete in updating CI jobs to support C++20 because the C++20 usage was only in tests. Here we add subtle C++20 usage in the public API ("using enum" feature in db.h) to force the issue. A lot of the work for this PR was in updating the Ubuntu22 docker image, for earlier compiler/runtime versions supporting C++20, and generating a new Ubuntu24 docker image, for later compiler/runtime versions. The Ubuntu22 image needed to be updated because there are incompatibilities with clang-13 + c++20 + libstdc++ for gcc 11, seen on these examples ``` #include <chrono> int main(int argc, char *argv[]) { std::chrono::microseconds d = {}; return 0; } ``` and ``` #include <coroutine> int main() { return 0; } ``` The second was causing recurring failures in build-linux-clang-13-asan-ubsan-with-folly, now fixed. So we have to install clang's libc++ to compile with clang-13. I haven't been able to get this to work with some of the libraries like benchmark, glog, and/or gflags, but I'm able to compile core RocksDB with clang-13. On this docker image, an extra compiler parameter is needed to compile with gcc and glog because it's built from source perhaps not perfectly, because the ubuntu package transitively conflicts with libc++. The Ubuntu24 image seems to be low-drama and generally work for testing out newer compiler versions. The mingw build uses Ubuntu24 because the mingw package on Ubuntu22 uses a gcc version that is too old. And the mass of other code changes are trying to work around new warnings, mostly from clang-analyze, which I upgraded to clang-18 in CI. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13915 Test Plan: CI, including temporarily including the nightly jobs in the PR jobs in earlier revisions to test and stabilize Reviewed By: archang19 Differential Revision: D81933067 Pulled By: pdillinger fbshipit-source-id: 7e33823006a79d5f3cf5bc1d625f0a3c08a7d74c |
||
|
Andrew Chang
|
239b06cefb |
Retry on some io_uring_wait_cqe error codes (#13890)
Summary: RocksDB currently aborts whenever `io_uring_wait_cqe` returns an error code. It also does not log what error code was returned. While experimenting with `IO_URING`, my application crashed because of this. I asked the Linux Kernel user group the best way to handle unsuccessful `io_uring_wait_cqe`. It was recommended to retry on `EINTR`, `EAGAIN`, and `ETIME`. `ETIME` only happens when waiting with a timeout, so I am not handling it. I also write to `stderr` so that we have some debugging information if we abort. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13890 Test Plan: Unfortunately this is hard to cover through unit/stress tests. We have to see what sort of errors get encountered in production. Reviewed By: anand1976 Differential Revision: D80639955 Pulled By: archang19 fbshipit-source-id: e3a230bd37552ec0f36be34e6a4e53cfd2a254f1 |
||
Xingbo Wang
|
961880b458 |
Create a new API FileSystem::SyncFile for file sync (#13762)
Summary: Create a new API FileSystem::SyncFile for file sync, so that we could use file sync directly in places where we need to sync file content to file system without any modification. This is mostly used combined with link file. In some file system link file does not guarantee the file content is synced to file system. https://github.com/facebook/rocksdb/issues/13741 Pull Request resolved: https://github.com/facebook/rocksdb/pull/13762 Test Plan: Unit test T229418750 Reviewed By: pdillinger Differential Revision: D78121137 Pulled By: xingbowang fbshipit-source-id: 0ea8a5a3b486e0b61636700400613fed6bbd3faa |
||
|
Xingbo Wang
|
ca5d60fd69 |
Switch back to FSWritableFile in external sst file ingestion job (#13791)
Summary: This patch reverted "NewRandomRWFile" back to "ReopenWritableFile" in external sst file ingestion job when file is linked instead of copied. The reason is that some of the file systems do not support "NewRandomRWFile". A long term fix is being worked in progress. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13791 Test Plan: Unit test Reviewed By: pdillinger Differential Revision: D78697825 Pulled By: xingbowang fbshipit-source-id: d3651223ab1f2369aac34b772bba8049c6c2c628 |
||
|
Xingbo Wang
|
11a259a5f0 |
Support GetFileSize API in FSRandomAccessFile (#13676)
Summary:
Add file size validation in ReadFooterFromFile function.
Deprecate skip_checking_sst_file_sizes_on_db_open option.
This change is used to address this issue
https://github.com/facebook/rocksdb/issues/13619
It supports file size validation in ReadFooterFromFile. In favor of this
change, CheckConsistency function and
skip_checking_sst_file_sizes_on_db_open flag are deprecated.
The CheckConsistency function checks each file size matches what was
recorded in manifest during DB open. Meantime, ReadFooterFromFile was
called for each file in LoadTables function. Since ReadFooterFromFile
always validates file size, the CheckConsistency is redundant.
In addtion, CheckConsistency is executed in a single thread. This could
slow down DB open when a network file system is used. Therefore, the
flag skip_checking_sst_file_sizes_on_db_open was added to skip this
check. After this change, ReadFooterFromFile was executed in parallel
through multiple threads. Therefore, the concern of DB open slowness is
eliminated, and the flag could be deprecated.
When paranoid check flag is set to true, corrupted file will fail to open the DB.
When paranoid check flag is set to false, DB will still be able to open, the
healthy ones can be accessed, while the corrupted ones not.
There is 2 slight concerns of this change.
*If max_open_files is set with smaller value, engine will not open all
the files during DB open. This means if there is a corruption on file
size, it will not be detected during DB open, but rather at a later
time. Since the default is -1, which means open all the files, and it is
rarely overridden and a lot of new features rely on it to be -1, the
risk is very low.
*If FIFO compaction is used, engine could fail to open DB unnecessarily
on the corrupted files that would never be used again. However, this is
a very rare case as well. The error could still be ignored by setting
paranoid_checks operationally. The risk is very low.
To remain backward compatibility. The public facing flag was kept and
marked as no-op internally. Another change is required to fully remove
the flag.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13676
Test Plan:
make check
A new unit test was added to validate file size check API works as
expected.
Reviewed By: pdillinger
Differential Revision: D76168033
Pulled By: xingbowang
fbshipit-source-id: 8ceacf39bcfe02ff7aa289868c341366ee9f3a8e
|
||
|
anand76
|
f340a2eccc |
Port codemod changes from fbcode/rocksdb (#13714)
Summary: Port changes made directly in fbcode in order to facilitate the 10.4 release. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13714 Test Plan: Existing tests Reviewed By: mszeszko-meta Differential Revision: D77038668 Pulled By: anand1976 fbshipit-source-id: 6b9b16d62bccf75923b525c1c24597a59920a948 |
||
Yu Zhang
|
695c653e11 |
Correctly initialize file size for reopened writable file (#13534)
Summary: A reopened writable file's size is not correctly tracked in the `WritableFile`'s internal state. This PR adds a querying to the file system to get the initial file size in the reopen case and use it to populate posix `WritableFile`'s internal state. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13534 Reviewed By: anand1976 Differential Revision: D72756628 Pulled By: jowlyzhang fbshipit-source-id: 6f02b5c5da069fe49055d7b75bec9e7e47d5cd71 |