Repository navigation
dd: ignore a failed cache drop while copying - #14748
ChihweiLHBird wants to merge 1 commit into
Conversation
Merging this PR will not alter performance
|
|
GNU testsuite comparison: |
5b69be5 to
ae1248c
Compare
|
I looked into the CodSpeed report. The regression is real but small, and it comes from compiler register allocation rather than from any added work.
Cause. None of these benchmarks pass I couldn't find a variant of this fix reaches zero instructions increase. Another variant, adding a bunch of |
c77a8fa to
e99a685
Compare
| // Using len=0 in posix_fadvise means "to end of file" | ||
| if i.settings.iflags.nocache { | ||
| i.discard_cache(0, 0); | ||
| let file = i |
There was a problem hiding this comment.
almost the same block twice for input and output, could be a small helper taking the name and the result, no?
There was a problem hiding this comment.
I don't think building a helper for them is a good idea. i and o are different type, and the error messages in translate! macro are also different. Building a helper might make it looks much messy than the current shape
f200407 to
74118f5
Compare
74118f5 to
5650bed
Compare
|
|
||
| // TODO: Is there a way to discard filesystem cache on other targets? | ||
| #[cfg(not(any(target_os = "linux", target_os = "android", target_os = "freebsd")))] | ||
| #[allow( |
There was a problem hiding this comment.
these wrappers are just pass-throughs now, could the non-linux stub live in Source/Dest::discard_cache instead? that would drop both #[allow]
There was a problem hiding this comment.
Moved. But it adds an extra stub inside Source/Dest
5650bed to
8279227
Compare
There was a problem hiding this comment.
🟡 Changes recommended
GNU-source provenance must be clarified and the changed output-side count=0 path needs regression coverage.
2 open findings
What changed in this PR
Updates dd cache-discard handling so advisory failures during copying are ignored while count=0 failures remain fatal.
Changes:
- Return cache-discard errors to callers.
- Preserve diagnostics for
count=0. - Add pipe-copy regression coverage.
| File | Description |
|---|---|
src/uu/dd/src/dd.rs |
Moves cache-error policy to callers. |
src/uu/dd/src/bufferedoutput.rs |
Propagates cache-discard results. |
tests/by-util/test_dd.rs |
Tests silent pipe copying with nocache. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
8279227 to
40f78b8
Compare
40f78b8 to
f457f28
Compare
fe75807 to
ee528b4
Compare
ee528b4 to
cfd839d
Compare
cfd839d to
45218cd
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
This test risks hanging: opening a FIFO for writing typically blocks until a reader is present, and… · New The comment says the parser acceptsnocache/directonly on Linux and Android, but the cfg… · New Same issue asSource::discard_cache: the comment and the cfg platform list are inconsistent… · New
🧠 Review effort: Lite
45218cd to
134b00a
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open findings
The condition guarding the output cache flush checksi.settings.oflags.nocache, but the error… · New This test risks hanging: opening a FIFO for writing typically blocks until a reader is present, and…nocacheis described as supported on both Linux and Android in the PR description, but this… · New Same issue asSource::discard_cache: the comment and the cfg platform list are inconsistent… The comment says the parser acceptsnocache/directonly on Linux and Android, but the cfg…
🧠 Review effort: Lite
134b00a to
1eda9ab
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
The output-side cache flush condition is gated oni.settings.oflags.nocacheinstead of… · New The PR description frames “ignore discard-cache failures while copying” specifically in terms of… · Newnocacheis described as supported on Linux and Android, but this FIFO-output error reporting test… · New
5 resolved since last review
The condition guarding the output cache flush checksi.settings.oflags.nocache, but the error… This test risks hanging: opening a FIFO for writing typically blocks until a reader is present, and…nocacheis described as supported on both Linux and Android in the PR description, but this… Same issue asSource::discard_cache: the comment and the cfg platform list are inconsistent… The comment says the parser acceptsnocache/directonly on Linux and Android, but the cfg…
🧠 Review effort: Lite
| if i.settings.oflags.nocache { | ||
| o.discard_cache(0, 0); | ||
| let file = o | ||
| .settings | ||
| .outfile | ||
| .clone() | ||
| .unwrap_or_else(|| translate!("dd-standard-output")); | ||
| show_if_err!( | ||
| o.discard_cache(0, 0) | ||
| .map_err_context(|| translate!("dd-error-failed-discard-cache", "file" => file)) | ||
| ); | ||
| } |
| // Dropping the cache is best-effort: a failure (a pipe cannot | ||
| // drop its cache at all) must not fail or diagnose a copy that | ||
| // succeeded. Only the `count=0` path reports it. | ||
| if input_nocache { | ||
| i.discard_cache(read_offset, 0); | ||
| let _ = i.discard_cache(read_offset, 0); | ||
| } | ||
| if output_nocache || output_direct { | ||
| o.discard_cache(write_offset.try_into().unwrap(), 0); | ||
| let _ = o.discard_cache(write_offset.try_into().unwrap(), 0); | ||
| } |
|
|
||
| /// Test that discarding system file cache fails for an output FIFO. | ||
| #[test] | ||
| #[cfg(target_os = "linux")] |
Signed-off-by: Zhiwei Liang <zhiwei.liang@zliang.me>
1eda9ab to
f1d005a
Compare
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.



Summary
iflag=nocacheandoflag=nocachediagnosed every failed cache drop, so copying from or to a pipe printedfailed to discard cache for: 'standard input': Illegal seekonce per read and exited 1 even though the copy succeeded. GNU ignores those failures while copying and diagnoses them only withcount=0.Source::discard_cacheandDest::discard_cachenow return the error,InputandOutputpass it through, and the callers decide:dd_copyignores it, thecount=0path reports it and sets the exit status.nocacheis accepted only on Linux and Android, so other targets are unaffected; the platform stub moved fromInput/OutputintoSource/Dest, which lets the wrappers be plain pass-throughs.Part of #14705 (
tests/dd/nocache.sh).Example
dd if=/dev/zero bs=1M count=1 status=none | dd iflag=nocache bs=1M of=/dev/nullBefore: one
failed to discard cacheline per read, exit 1. After: silent, exit 0, as GNU 9.12.count=0still diagnoses the failure: