options: expose direct-IO setters for the SST writer and reader - #856
options: expose direct-IO setters for the SST writer and reader#856sanketkedia wants to merge 1 commit into
Conversation
TiKV's SST importer writes ingested SSTs buffered and then reads each one back in full, also buffered, to verify checksums. During a large ADD INDEX ingest that streams the entire index through the OS page cache twice and evicts the foreground read working set: reads that were page-cache hits (~50us) become device reads (~400us), degrading foreground p99 by ~2x. See tikv/tikv#19917 for the full analysis. Fixing that in TiKV needs two knobs this crate does not currently expose: - EnvOptions::set_use_direct_writes, so SstFileWriter can write without populating the page cache. RocksDB uses O_DIRECT on Linux and fcntl(F_NOCACHE) on macOS. - ColumnFamilyOptions::set_use_direct_reads, so a standalone SstFileReader can read the same way. use_direct_reads is a DB-level RocksDB option and DBOptions already exposes it here, but SstFileReader::new only accepts ColumnFamilyOptions; in crocksdb both wrap the same underlying Options object, so the setter is added there as well. Neither setter changes any default: both are false unless explicitly set. The test writes an SST with direct writes, reads it back with direct reads, and then reads it again buffered to confirm the alignment padding O_DIRECT requires does not leak into the file format. Ref: tikv/tikv#19917 Signed-off-by: Sanket Kedia <kediasanket11121993@gmail.com>
ti-chi-bot
Bot
added
dco-signoff: yes
|
Hi @sanketkedia. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
the
first-time-contributor
|
Welcome @sanketkedia! |
ti-chi-bot
Bot
added
the
size/M
📝 WalkthroughWalkthroughAdds direct read and write option setters to the Rust RocksDB API through new C and Rust FFI bindings. A test verifies direct-I/O SST creation, checksum validation, iteration, and compatibility with non-direct readers. ChangesDirect I/O option support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rocksdb_options.rs`:
- Around line 1492-1501: Update the public documentation for
set_use_direct_reads and the corresponding DBOptions setter to replace
platform-specific “O_DIRECT” wording with “direct I/O,” while preserving the
existing explanation of bypassing the OS page cache.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b84651c-38cf-40e3-8bac-60966ef818dc
📒 Files selected for processing (5)
librocksdb_sys/crocksdb/c.cclibrocksdb_sys/crocksdb/crocksdb/c.hlibrocksdb_sys/src/lib.rssrc/rocksdb_options.rstests/cases/test_ingest_external_file.rs
| /// Read this options' files with O_DIRECT, bypassing the OS page cache. | ||
| /// Useful for a standalone SstFileReader (e.g. ingest checksum verification) | ||
| /// so a one-shot read does not populate the page cache. `DBOptions` exposes | ||
| /// the same setter; both write to the shared underlying options object. | ||
| pub fn set_use_direct_reads(&mut self, v: bool) { | ||
| unsafe { | ||
| crocksdb_ffi::crocksdb_options_set_use_direct_reads(self.inner, v); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use platform-neutral direct-I/O wording in the public docs.
The implementation uses O_DIRECT on Linux but F_NOCACHE on macOS. Replace the literal O_DIRECT references with “direct I/O” so the API documentation remains accurate across supported platforms.
Also applies to: 2379-2386
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rocksdb_options.rs` around lines 1492 - 1501, Update the public
documentation for set_use_direct_reads and the corresponding DBOptions setter to
replace platform-specific “O_DIRECT” wording with “direct I/O,” while preserving
the existing explanation of bypassing the OS page cache.
AndreMouche
left a comment
AndreMouche
left a comment
There was a problem hiding this comment.
LGTM @Connor1996 Please take a look
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AndreMouche The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Although the internal RocksDB read/write APIs are designed to guarantee forward and backward compatibility when writes use 1. Buffered write -> Direct I/O readThe current test covers "direct-write" -> "buffered-read", which is the important “disable Direct I/O after writing” compatibility case. Direct I/O is an I/O mode rather than an SST format attribute, so this should work without any format metadata or migration. Adding this case would make the compatibility matrix explicit and protect both directions when users toggle the option. 2. Direct I/O write -> ingest -> normal DB readCould we also extend the test to ingest a directly-written SST into a DB and read the ingested keys through the normal buffered DB read path? Reopening the external SST with a buffered |
3 participants
What is changed and how it works?
See tikv/tikv#19917 for full context. Exposes two direct-IO setters that TiKV needs in order to stop its SST importer from polluting the OS page cache:
EnvOptions::set_use_direct_writes— letsSstFileWriterwrite an SST withoutpopulating the page cache. Adds the missing
crocksdb_envoptions_set_use_direct_writesshim; RocksDB already supports the underlying
EnvOptions::use_direct_writes, usingO_DIRECTon Linux andfcntl(F_NOCACHE)on macOS.ColumnFamilyOptions::set_use_direct_reads— lets a standaloneSstFileReaderread anSST the same way. Rust-only wrapper; it reuses the existing
crocksdb_options_set_use_direct_readsFFI, so there is no new C code on this path.Neither setter changes any default — both are
falseunless explicitly set, so existingusers are unaffected.
Check List
Tests
test_sst_direct_iointests/cases/test_ingest_external_file.rs: writesan SST with direct writes, reads it back with direct reads and verifies checksum +
contents, then reads it once more buffered to confirm the alignment padding
O_DIRECTrequires does not leak into the file format.Side effects
No behaviour change unless a caller opts in.
Related changes
tikv/tikvimplementing the importer fix; that PR is blockedon this one landing, since TiKV pins
rocksdbby git.Summary by CodeRabbit
New Features
Tests