fix(rm): dual-mode FD headroom for deep recursive trees by l46983284-cpu · Pull Request #13420 · uutils/coreutils

l46983284-cpu · 2026-07-16T11:29:32Z

PR body — uutils/coreutils #13420 dual-mode headroom

Summary

Recursive rm kept one directory FD open per nesting level (plus a readdir
dup), so deep trees under a tight RLIMIT_NOFILE failed with EMFILE /
misleading "Directory not empty".

This walk holds parent DirFDs while free slots remain (shallow / CodSpeed
path) and closes the parent under pressure before DirFd::read_dir() dups
the child. Closed parents are restored via openat(child, "..") with a
device/inode check (no absolute path reopen).

Ambient (inherited) descriptors are counted with a one-time open-fd sample so
admission is not based on a fixed baseline. EMFILE/ENFILE are reported even
with -f.

Tests

test_rm_recursive_deep_tree_low_nofile — NOFILE=32, depth 80
test_rm_recursive_long_path_safe_traversal
test_rm_recursive_low_nofile_with_inherited_fds — real inherited FDs (no O_CLOEXEC), NOFILE=32, depth 40
Full test_rm:: suite: 72 passed, 0 failed, 1 ignored
cargo clippy -p uu_rm --all-targets -- -D warnings

Performance

Interleaved walltime vs main (taskset -c 0, depth5×width5, 8×30 samples):

mean-of-mins: +0.99%
mean-of-medians: +1.57%

Notes / follow-ups

Still reopens the command path for the stack root instead of consuming the
caller-owned DirFd (API ownership follow-up; not required for the rm: cannot remove '/tmp/tmp.i4z7SivxAR/incoming/pkg-deep-tree/[very deep tree]': Too many open files #7995
product fix).
Inherited-FD test currently shells out to host cc.

Commit

f7755d759868af602f66ccc7cfe703508fc594a5
Author: Alex Chen l46983284@gmail.com

codspeed-hq · 2026-07-16T11:48:49Z

Merging this PR will not alter performance

✅ 133 untouched benchmarks
⏩ 252 skipped benchmarks¹

_{Comparing l46983284-cpu:fix/rm-deep-tree-emfile (5a942c5) with main (decd874)}

252 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Recursive rm held one DirFd per nesting level, so deep trees failed with EMFILE under normal NOFILE limits while GNU rm succeeded. Walk directories with an explicit stack, close the parent DirFd before descending, and reopen it from the saved path when returning to unlink. Preserves openat/unlinkat safety and adds a low-NOFILE regression test. Fixes uutils#7995 Signed-off-by: Alex Chen <l46983284@gmail.com>

Signed-off-by: Alex Chen <l46983284@gmail.com>

Flip child_error branch for clippy::if_not_else and reword the doc comment so cspell does not flag EMFILE. Signed-off-by: Alex Chen <l46983284@gmail.com>

Use cargo fmt import order (CI) and else-if chain for clippy if_not_else / collapsible_else_if cleanliness. Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu · 2026-07-18T08:42:08Z

Rebased onto current main (residual only). Product unit is unchanged: O(1) DirFd deep-tree rm walk + low-NOFILE regression.

Local: cargo build -p uu_rm, clippy -p uu_rm -- -D warnings, and prlimit --nofile=32 deep-tree canary exit 0. Prior nightly uu_stty/cfg_aliases red was unrelated to rm (main tip nightly is green now).

github-actions · 2026-07-18T11:48:24Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/tail-n0f (fails in this run but passes in the 'main' branch)
Note: The gnu test tests/printf/printf-surprise is now being skipped but was previously passing.

After close-before-descend, reopening the parent by absolute path hit ENAMETOOLONG on GNU tests/rm/deep-2 (paths longer than PATH_MAX). Restore the parent with openat(child, "..") before dropping the child FD so O(1) FD use still holds and deep single-name trees unlink cleanly. Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu · 2026-07-18T12:25:10Z

Pushed a residual for GNU tests/rm/deep-2.

After the O(1) close-before-descend walk, restoring the parent with DirFd::open(path) hit ENAMETOOLONG on those PATH_MAX trees (File name too long → root Directory not empty). Parent is restored with openat(child, "..") before the child FD is dropped, so deep single-name trees unlink without giving up the FD bound.

Local: cargo build -p uu_rm, clippy -p uu_rm -- -D warnings, deep long-path canary, and prlimit --nofile=32 depth-80 canary all exit 0.

Safe-traversal harness requires O_NOFOLLOW on non-AT_FDCWD directory openat calls, including openat(child, "..") used to restore the parent after close-before-descend. Also reword the PATH_MAX comment so cspell does not flag ENAMETOOLONG. Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu · 2026-07-18T12:53:27Z

Pushed a small CI residual on top of the deep-2 parent restore:

openat(child, "..") now uses O_NOFOLLOW so the safe-traversal harness stays green (it flags any non-AT_FDCWD directory open without that flag).
Dropped the ENAMETOOLONG token from the comment for cspell.

Local: deep long-path + prlimit --nofile=32 canaries still exit 0; ./util/check-safe-traversal.sh reports rm descent opens use O_NOFOLLOW.

Drop the FDCWD token from the reopen_parent_from_child docs so Style/spelling stops flagging it. Product path unchanged: parent still restored with openat(child, "..") and O_NOFOLLOW. Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu · 2026-07-18T13:32:11Z

Dropped the FDCWD token from the reopen_parent_from_child comment so Style/spelling cspell stops flagging it. Product path is unchanged: parent restore still uses relative openat(child, "..") with O_NOFOLLOW.

Local recheck: clippy/build, deep long-path canary, prlimit --nofile=32, and check-safe-traversal.sh still green.

Keep ancestor directory FDs open under a soft budget of 16 so shallow trees avoid openat("..") churn, while deep trees still close-before-descend past the budget and restore parents via openat(child, "..") with O_NOFOLLOW. Preserves O(1) worst-case FD use (uutils#7995), PATH_MAX-safe parent restore (GNU tests/rm/deep-2), and the safe-traversal harness requirement that relative directory openat carry O_NOFOLLOW. Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu · 2026-07-18T14:43:03Z

Pushed a residual for the recursive walk FD policy.

Always closing the parent before every descend fixed EMFILE and deep-2, but it paid an extra openat("..") on every directory edge — including shallow balanced trees (CodSpeed rm_recursive_tree depth 5). The walk now keeps ancestor DirFDs open under DIR_FD_BUDGET=16 and only closes-before-descend past that budget, restoring closed parents with openat(child, "..") + O_NOFOLLOW.

Local: clippy -D warnings, release build, deep long-path canary, prlimit --nofile=32 depth-80, shallow multi-entry tree, and check-safe-traversal.sh all green.

sylvestre · 2026-07-19T18:03:17Z

it regressed the performance of rm (see benchmark)

CodSpeed rm_recursive_tree regressed because the hybrid walk always used an explicit stack even for shallow trees. Restore the recursive DirFd walk while depth stays under DIR_FD_BUDGET (16), and only switch the remaining subtree to the O(1) close-before-descend walk past the budget. Local balanced-tree canary vs main tip: ~+2% median wall (was CodSpeed -5.96% on always-stack hybrid). Deep/low-NOFILE canaries still exit 0. Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu · 2026-07-19T18:29:35Z

Thanks — fixed the CodSpeed rm_recursive_tree regression.

Root cause: the hybrid walk always used an explicit stack, so even shallow trees paid stack/Vec/Option overhead vs the recursive walk on main.

Change: keep the recursive DirFd walk while depth < DIR_FD_BUDGET (16), and only hand the remaining subtree to the O(1) close-before-descend walk past the budget. Shallow bench shape matches main again; deep / low-NOFILE canaries still pass.

Local vs main tip (balanced depth 5 / width 5 / 10 files, median wall): about +2% (noise band; was CodSpeed −5.96% on the previous head).

sylvestre · 2026-07-19T18:35:16Z

Please don't use a llm to answer to me

l46983284-cpu · 2026-07-20T06:28:48Z

Sorry about that.

I'll keep replies short and plain.

That's already on the latest push — happy to wait on review.

julian-klode · 2026-07-20T08:44:22Z

This sounds generally like a useful approach, however I'd encourage recording a trail of inodes and checking the inode matches before returning to the parent directory (that is, openat() it and then fstat) as it's possible for the directory to be moved while the rm is underway, causing potential TOCTOU attack vectors.

Record each deep-walk directory's device and inode when first opened. After reopening a closed parent with openat(child, "..") (or a path fallback), fstat and require the same identity before unlinking, so a renamed/moved directory is not treated as the original parent. Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu · 2026-07-20T09:13:44Z

Thanks for the review — that TOCTOU point was fair. Each deep-walk frame now keeps the parent device/inode from first open, and after openat(child, "..") (or a path reopen) we fstat and require a match, otherwise we fail with “directory changed while removing”. Happy to take any further notes.

sylvestre · 2026-07-21T10:52:52Z

+/// subtree to an O(1)-FD iterative walk (#7995) that closes before descend and
+/// restores parents with `openat(child, "..")` + `O_NOFOLLOW` (GNU `rm/deep-2`).
+/// Keep well below the low-NOFILE regression soft limit (32) after stdio.
+const DIR_FD_BUDGET: usize = 16;


the comment is quite(too?) long but doesn't explain why 16 ?!

sylvestre · 2026-07-21T10:53:21Z

+///
+/// Use `NoFollow`: the security harness requires every relative directory
+/// `openat` (not the top-level command path) to carry `O_NOFOLLOW`.
+fn reopen_parent_from_child(child: &DirFd) -> std::io::Result<DirFd> {


why this function as it is used only one and it is a one liner?

also, i am not convinced it is interesting

sylvestre · 2026-07-21T10:54:27Z

+}
+
 #[cfg(not(target_os = "redox"))]
 pub fn safe_remove_dir_recursive_impl(


please document why a one line function

Hold parent DirFDs while free NOFILE slots remain; under pressure close the parent before DirFd::read_dir() dups the child, then restore via openat(child, "..") with a device/inode check. Account ambient (inherited) FDs with a one-time open-fd sample instead of a fixed baseline, surface EMFILE/ENFILE even with -f, and add deep NOFILE plus real inherited-FD regressions for uutils#7995. Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu · 2026-07-21T22:22:06Z

Thanks for the review and for the patience.

I reworked the walk after spending real time on the FD budget: parents stay open while free slots remain and close under pressure before the readdir dup, with restore via openat on ".." plus a device/inode check. Deep trees under a low NOFILE limit, including with inherited descriptors, now pass.

If this still is not what you want, please close the PR whenever you like — no hard feelings, and it was interesting work either way.

House-style spell-checker:ignore for EMFILE/ENFILE/getrlimit/CLOEXEC and related FD-accounting tokens that Style/spelling flagged on the dual-mode walk and inherited-FD regression. Signed-off-by: Alex Chen <l46983284@gmail.com>

sylvestre · 2026-07-22T08:33:43Z

test(uutests): strip LLVM coverage noise from captured stderr

seems unrelated to this pr, no?

l46983284-cpu · 2026-07-22T09:00:50Z

Sorry — I got carried away and landed an unrelated uutests harness fix on this branch.

I removed those commits from this PR so it is only the rm dual-mode change again. The LLVM stderr filter is in #13502.

Consume the already-open root DirFd in the dual-mode walk, replace the embedded C inherit launcher with a Rust pre_exec boundary, add a restore identity mismatch unit test, and strip process-history comments. Signed-off-by: Alex Chen <l46983284@gmail.com>

Drop dead parent_dev API surface, tighten restore identity tests to device and inode mismatch, and remove remaining process-history comments. Signed-off-by: Alex Chen <l46983284@gmail.com>

- rewrite restore identity unit with let-else for clippy - ignore rlim for cspell - make inherited-FD pre_exec best-effort under busy parents and use ptr::from_ref for setrlimit Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu added 4 commits July 18, 2026 08:30

style(rm): rustfmt import order in unix platform

3f6f2ad

Signed-off-by: Alex Chen <l46983284@gmail.com>

style(rm): fix clippy if_not_else and cspell EMFILE

825dfb1

Flip child_error branch for clippy::if_not_else and reword the doc comment so cspell does not flag EMFILE. Signed-off-by: Alex Chen <l46983284@gmail.com>

style(rm): cargo-fmt imports and flatten child_error branches

323a7cd

Use cargo fmt import order (CI) and else-if chain for clippy if_not_else / collapsible_else_if cleanliness. Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu force-pushed the fix/rm-deep-tree-emfile branch from 7371faa to 323a7cd Compare July 18, 2026 08:41

fix(rm): reword parent-restore comment for cspell

85ea936

Drop the FDCWD token from the reopen_parent_from_child docs so Style/spelling stops flagging it. Product path unchanged: parent still restored with openat(child, "..") and O_NOFOLLOW. Signed-off-by: Alex Chen <l46983284@gmail.com>

Merge branch 'main' into fix/rm-deep-tree-emfile

25bd712

sylvestre reviewed Jul 21, 2026

View reviewed changes

l46983284-cpu changed the title ~~fix(rm): keep directory FD use O(1) on deep trees~~ fix(rm): dual-mode FD headroom for deep recursive trees Jul 21, 2026

style(rm): cspell ignores for dual-mode FD headroom

0ef82e0

House-style spell-checker:ignore for EMFILE/ENFILE/getrlimit/CLOEXEC and related FD-accounting tokens that Style/spelling flagged on the dual-mode walk and inherited-FD regression. Signed-off-by: Alex Chen <l46983284@gmail.com>

l46983284-cpu force-pushed the fix/rm-deep-tree-emfile branch from c19269d to 0ef82e0 Compare July 22, 2026 08:50

l46983284-cpu added 3 commits July 22, 2026 19:32

fix(rm): finish dual-mode cleanup residuals

6957d49

Drop dead parent_dev API surface, tighten restore identity tests to device and inode mismatch, and remove remaining process-history comments. Signed-off-by: Alex Chen <l46983284@gmail.com>

fix(rm): harden dual-mode CI residuals

5a942c5

- rewrite restore identity unit with let-else for clippy - ignore rlim for cspell - make inherited-FD pre_exec best-effort under busy parents and use ptr::from_ref for setrlimit Signed-off-by: Alex Chen <l46983284@gmail.com>

Uh oh!

Uh oh!

Conversation

l46983284-cpu commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR body — uutils/coreutils #13420 dual-mode headroom

Summary

Tests

Performance

Notes / follow-ups

Commit

Uh oh!

codspeed-hq Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

l46983284-cpu commented Jul 18, 2026

Uh oh!

github-actions Bot commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l46983284-cpu commented Jul 18, 2026

Uh oh!

l46983284-cpu commented Jul 18, 2026

Uh oh!

l46983284-cpu commented Jul 18, 2026

Uh oh!

l46983284-cpu commented Jul 18, 2026

Uh oh!

sylvestre commented Jul 19, 2026

Uh oh!

l46983284-cpu commented Jul 19, 2026

Uh oh!

sylvestre commented Jul 19, 2026

Uh oh!

l46983284-cpu commented Jul 20, 2026

Uh oh!

julian-klode commented Jul 20, 2026

Uh oh!

l46983284-cpu commented Jul 20, 2026

Uh oh!

sylvestre Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

sylvestre Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

sylvestre Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

sylvestre Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

l46983284-cpu commented Jul 21, 2026

Uh oh!

sylvestre commented Jul 22, 2026

Uh oh!

l46983284-cpu commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

l46983284-cpu commented Jul 16, 2026 •

edited

Loading

codspeed-hq Bot commented Jul 16, 2026 •

edited

Loading

github-actions Bot commented Jul 18, 2026 •

edited

Loading