Applying a different madvise after PopulateRead cancels its effect#6714
Applying a different madvise after PopulateRead cancels its effect#6714
Conversation
📝 WalkthroughWalkthroughThe code was updated to change when the ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/common/memory/src/mmap_ops.rs (1)
71-75: Replicate the advice / error-handling fixes for the mutable mappingEverything said for the read-only path applies to
open_write_mmapas well—ignoring the caller’s advice and the populate error may cause inconsistent behaviour between read and write code paths. Consider applying the same refactor here to keep the APIs symmetrical.
🧹 Nitpick comments (1)
lib/common/memory/src/mmap_ops.rs (1)
55-56:mmap.populate()result is ignored
memmap2::Mmap::{populate, populate_mut}returnio::Result<()>, yet the code drops the result.
Silent failures (e.g.,ENOMEM,EIO) will go unnoticed and may lead to subtle runtime issues.- mmap.populate(); + mmap.populate()?; // propagate error to the callerIf best-effort population is acceptable, at least log the error via
tracing::warn!to aid diagnostics.Also applies to: 71-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/common/memory/src/mmap_ops.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
lib/common/memory/src/mmap_ops.rs (1)
Learnt from: generall
PR: qdrant/qdrant#6323
File: lib/segment/src/vector_storage/quantized/quantized_mmap_storage.rs:12-16
Timestamp: 2025-04-07T23:31:22.515Z
Learning: The `populate()` method in `QuantizedMmapStorage` correctly returns void instead of a `Result` because it directly implements the `Madviseable` trait which defines `populate(&self)` without a return type. Adding an unnecessary `Ok(())` return would trigger Clippy warnings about unnecessary Result wrapping.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-low-resources
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: test-consensus-compose
- GitHub Check: integration-tests-consensus
- GitHub Check: rust-tests (windows-latest)
| if populate { | ||
| mmap.populate(); | ||
| } else { | ||
| madvise::madvise(&mmap, advice.resolve())?; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Advice hint is silently dropped when populate == true
With the new else‐branch, any non-default AdviceSetting provided by the caller is ignored whenever populate is requested.
Call sites that relied on open_*_mmap(path, AdviceSetting::Sequential /* or Random */, true) will now receive the default kernel advice, potentially hurting read-ahead behaviour and cache locality.
A minimal fix that preserves both semantics and the intended kernel behaviour is to apply the advice before populating:
if populate {
- mmap.populate();
+ madvise::madvise(&mmap, advice.resolve())?;
+ mmap.populate(); // MADV_WILLNEED comes last, no cancellation
} else {
madvise::madvise(&mmap, advice.resolve())?;
}This keeps PopulateRead effective while still honouring the caller’s hint.
Please double-check the micro-benchmarks and update the documentation/comments accordingly.
Advice hint is silently dropped when populate == true
With the new else‐branch, any non-default AdviceSetting provided by the caller is ignored whenever populate is requested.
Call sites that relied on open_*_mmap(path, AdviceSetting::Sequential /* or Random */, true) will now receive the default kernel advice, potentially hurting read-ahead behaviour and cache locality.
A minimal fix that preserves both semantics and the intended kernel behaviour is to apply the advice before populating:
if populate {
- mmap.populate();
+ madvise::madvise(&mmap, advice.resolve())?;
+ mmap.populate(); // MADV_WILLNEED comes last, no cancellation
} else {
madvise::madvise(&mmap, advice.resolve())?;
}This keeps PopulateRead effective while still honouring the caller’s hint.
Please double-check the micro-benchmarks and update the documentation/comments accordingly.
🤖 Prompt for AI Agents
In lib/common/memory/src/mmap_ops.rs around lines 55 to 59, the current code
only applies the madvise advice when populate is false, causing the advice hint
to be ignored when populate is true. To fix this, call madvise with the resolved
advice before calling mmap.populate() so that the kernel advice is always
applied regardless of the populate flag. This preserves the intended read-ahead
behavior and cache locality. Also, verify micro-benchmarks and update comments
or documentation to reflect this change.
|
We found a different reason for the IO load discrepancy which is unrelated to I still think that our usage of Closing for the time being. |
Based on my manual testing, setting a different madvise right after setting
PopulateReadcancels its effect.It appears the kernel discards the pages loaded.
When using a vector storage
on_disk = falsewith this patch:read_bytesfromproc/pid/iois much higher on startup thandevperf stat -e major-faults -p $(pidof qdrant)unlikedev