Hash & dedupe files whose absolute path exceeds PATH_MAX (#117) - #124
Conversation
I would think that these would be fairly trivial to fix after these changes. Trivial enough to include in this PR. |
Introduce src/longpath.{c,h}: longpath_open() and longpath_stat() reach a
file whose absolute path exceeds PATH_MAX by opening a reachable ancestor
and openat-walking the remaining components in <=PATH_MAX chunks. Paths that
fit in PATH_MAX take the plain open()/stat() fast path unchanged, so callers
on the hot per-file path stay unaffected.
Add unit tests (src/tests.c) that build a >PATH_MAX directory chain on tmpfs
and cover open+read, stat, deep-directory listing, missing final component
and missing intermediate (ENOENT, not ENAMETOOLONG), and short-path
equivalence.
Also commit the implementation & test plan under docs/plans/.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wire the longpath helper into every place oans touches a file by its stored path, so files under a directory chain longer than PATH_MAX are now hashed, deduped, rescanned and pruned instead of skipped with a warning (the #115 follow-through). * struct file.filename becomes a heap-owned char* (was char[PATH_MAX+1]), managed via file_set_filename()/file_cleanup() so an over-PATH_MAX path round-trips through scan and change-detection intact. The files.filename column is already TEXT, so no schema change / DB_FILE_MINOR bump. * The walk (process_dir) reaches a deep directory via longpath_open + fdopendir, stats children relative to the directory fd, and drops the old length skip-guard; its child buffer is now sized to the prefix plus one NAME_MAX component instead of a fixed PATH_MAX. * Hashing (subvol probe + csum_whole_file), dedupe (filerec_open) and the stat-based prune all go through longpath_open/longpath_stat. Each keeps the plain open()/stat() fast path for in-range paths, so the hot single-consumer scan path is unchanged for ordinary trees. * Display buffers (progress status line, dedupe report) copy paths NUL-safe and tail-elide anything over the buffer; fixes a strncpy no-terminator. * Correct the single-syscall length bound to PATH_MAX-1 (PATH_MAX counts the NUL), so a path of exactly PATH_MAX chars takes the openat chain. Tests: rewrite test_long_path.py to assert the deep file is hashed, dedupes (verified via a dir-fd-relative FIEMAP so the test can reach it too), no-op rescans, and prunes selectively without ENAMETOOLONG-pruning its siblings; cover both the deep-chain and long-basename-in-range shapes. Add fd-based FIEMAP helpers to the harness. Full suite + integration-valgrind clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Post-review cleanup of the longpath module and its call sites (no behaviour
change):
* open_ancestor walks one path component at a time instead of packing
components into <=PATH_MAX openat() chunks. A component is bounded by
NAME_MAX, so the chunk accumulator, its flush blocks and the chunk[]
buffer were needless machinery on a cold (over-PATH_MAX) path.
* Replace the with_parent_dirfd() function-pointer dispatch (act_open/
act_stat trampolines + void* args) with a plain open_parent_dir()
accessor returning (dirfd, basename); each public entry point does its
own openat/fstatat/fdopendir. Collapse the repeated save/close/restore-
errno idiom into close_keep_errno(), and the duplicated fast-path guards
into fits_one_syscall().
* Add longpath_opendir() and longpath_lstat() so every call site routes
length-gating through the module: file_scan.c's opendir_maybe_long()
(which re-derived the PATH_MAX threshold) is gone, and is_file_renamed()
now uses longpath_lstat() instead of a plain lstat() that would
ENAMETOOLONG on a stored over-PATH_MAX path.
Extend the longpath unit test to cover longpath_opendir/longpath_lstat.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#117) Follow-up to the review of this branch. The headline bug: check_file()'s btrfs fs-identity probe still resolved the *path* (open + statfs + statx), so a nested subvolume below PATH_MAX depth was rejected with ENAMETOOLONG and its entire subtree silently dropped -- the exact failure #117 exists to remove, one layer up. Every other syscall on the walk had been converted. Correctness: - probe_fs() derives uuid + is_btrfs + supported from ONE longpath_open() fd (fstatfs, statx AT_EMPTY_PATH), replacing get_uuid()/is_btrfs()/ is_fs_supported(). Deletes the path-based is_btrfs() so it cannot come back. Verified: pre-fix 2 rows + "File name too long", post-fix 4 rows and 256 KiB reclaimed on the same tree. - print_dupes_table() sizes its buffer from the filename instead of PATH_MAX. Two distinct group members used to print as identical 4096-char strings naming their parent directory. - for_each_stdin_line() no longer bounds every consumer: -R is a SQL DELETE with no length limit, so `oans -L | oans -R -` could not remove rows the scan had just created. scan_file() keeps its own limit. - progress_copy_path() no longer underflows cap - 2 at small caps, and reuses ellipsize_path(), so a shortened path keeps its real head and basename behind a single marker instead of two different ones. - An over-PATH_MAX scan root now says so, and NOTES in the man page documents that only the root is limited, not the depth below it. Performance: - open_ancestor() advances by the longest run of components that fits one syscall argument (memrchr) instead of one openat per component: 22 -> 5 opens for a 4620-byte path. Scan perf neutral (12.76 vs 12.78 s median, 175k files). Guarding the invariant: - scripts/lint-longpath.py, wired into `make lint` / `make check`: flags any path-taking syscall in src/ that is not dirfd-relative and not waived with a `longpath-ok: <why>` comment. Verified it catches the exact regression it exists to prevent. - The invariant, its allow-list and its tests are stated in longpath.h. - New integration test with a mixed deep+shallow tree -- every other long-path test scans only long paths, so a silently dropped subtree looked like a pass. That shape is what hid the probe_fs bug. Tests: C unit test for progress_copy_path (incl. degenerate caps) and the two-stage render; test_longpath tears down unconditionally so a failed assertion cannot strand the cwd in an unnameable directory. Co-Authored-By: Claude <noreply@anthropic.com>
45bb612 to
9f65d7d
Compare
| errno, strerror(errno), path); | ||
| return; | ||
| } | ||
| if (child == NULL) { |
There was a problem hiding this comment.
This if should be moved above the if block above it. This way if dirp==NULL we don't potentially leak child's memory.
| * Open the directory named by the range [begin, end) (an absolute path prefix, | ||
| * possibly longer than PATH_MAX), returning an O_PATH directory fd suitable as | ||
| * a dirfd for openat()/fstatat()/fdopendir(). Walks from "/" one component at a | ||
| * time (each is bounded by NAME_MAX, well within a single syscall argument). |
There was a problem hiding this comment.
This increases openat syscalls on paths with length greater than PATH_MAX by quite a bit. You've avoided
A more efficient algorithm would be to break up the path starting at begin into maximum sized path chunks less than PATH_MAX. Then do the openat loop on each path chunk.
For the case where the file path is less than PATH_MAX, the number of syscalls will be O(1) with respect to the old code. For file paths greater than PATH_MAX, this implementation in the PR will have at least 8 times the number of openat syscalls and as much as ~2047 times the number of openat syscalls per PATH_MAX path chunk!
This also allows us rid of the fits_one_syscall checks below, which are currently needed to prevent this change from blowing up the number of syscalls for paths that are less of length less than PATH_MAX.
It would be interesting to know how much slower in wall time would be in the best worst case, absolute worst case, and my suggestions (ie. how cheap is openat).
| int dfd, fd; | ||
|
|
||
| if (fits_one_syscall(abspath)) | ||
| return open(abspath, flags); |
There was a problem hiding this comment.
This would not be necessary with my suggestion above. Nor would fits_one_syscall be needed.`
| if (dfd < 0) | ||
| return NULL; | ||
| fd = openat(dfd, base, O_RDONLY | O_DIRECTORY | O_CLOEXEC); | ||
| close_keep_errno(dfd); |
There was a problem hiding this comment.
nearly all lines from this line up can be replaced with something like:
fd = longpath_open(abspath, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
| dst[0] = '~'; | ||
| memcpy(dst + 1, src + (len - (cap - 2)), cap - 2); | ||
| dst[cap - 1] = '\0'; | ||
| } |
There was a problem hiding this comment.
Is there a reason not to simplify the above by using strcpy instead of memcpy?
Something like:
void progress_copy_path(char *dst, size_t cap, const char *src)
{
size_t len;
if (cap == 0)
return;
len = strlen(src);
strncpy(dst, src + (len - (cap - 1)), cap - 1);
if (len >= cap)
dst[0] = '~';
}
All pre-existing, all found reviewing #124: places that still assume a path fits PATH_MAX even though nothing there talks to the kernel, or that could be reached with a path that no longer does. - add_exclude_pattern() built the cwd expansion in a fixed buffer and refused anything over PATH_MAX ("cannot prepend cwd to ..."). An exclude pattern only ever reaches fnmatch()/strcmp(), never a syscall, so the bound was inherited cargo -- and it meant you could not exclude the deep subtrees #117 had just made scannable, which is the natural workaround for the one remaining over-PATH_MAX root limit. Built with get_current_dir_name() + asprintf() now, and a strdup/asprintf failure is reported instead of dereferenced. - storage_detect() did stat() + statfs() + open() on the path. One longpath_open() plus fstat()/fstatfs() gives the same three answers with one path lookup, and stops this being another thing in the way if the root limit is ever lifted. - persist_scan_config() dropped a root it could not realpath() with no message. That config is what a bare `oans --hashfile=X` replays, so an omitted root means a later scheduled run quietly covers less ground -- and the "all roots gone" guard cannot fire for a root that was never stored. It warns now. Tests: test_relative_exclude_longer_than_path_max builds a ~3000-char cwd and a 1400-char relative pattern (expansion ~4.4 KB). It fails on the old binary with "cannot prepend cwd" and passes here. harness: DUPEREMOVE is resolved to an absolute path at import. `make integration` passes a relative ./oans, so any test that chdir()s -- like the one above, which has to, to use a relative pattern -- could not find the binary. Co-Authored-By: Claude <noreply@anthropic.com>
…128) * Wait for the block-hash search pool before freeing its filerecs (#123) find_additional_dedupe() could return while its workers were still running, and the caller frees the filerecs those workers are reading. The search runs on a persistent pool, so it cannot be waited on by freeing the pool, and the only thing that looked like a join was psearch_join() -- a *progress* concern. Outside the dedupe phase that happens to work: it joins the progress printer, which loops until every worker has bumped the processed count. Inside the phase there is no printer, so psearch_join() resets two counters and returns immediately: void psearch_join(void) { if (pdd.phase) { search_total = 0; search_processed = 0; return; /* waits for nothing */ } g_thread_join(printer); } stream_load_batch() (--dedupe-options=partial) then moved on to the next batch, and dedupe_drain() -> reap_ready_locked() -> free_batch() freed filerecs out from under the live workers. A worker would dereference one in dbfile_load_nondupe_file_extents(). Correctness was resting on whether a display thread happened to exist. find_additional_dedupe() now waits on its own completion counter, which is independent of the progress module and also covers the two error paths that previously returned without waiting at all. Guarding it: - free_batch() asserts extents_search_idle() before dropping filerec refs, so this class of bug aborts at the point of the violation instead of silently reading freed memory. - DUPEREMOVE_SEARCH_DELAY_MS holds each worker back so the producer wins the race every time, turning a few-percent flake into a deterministic failure. New test_partial_search_waits_for_its_workers uses it: it aborts at run_dedupe.c's assertion on the unfixed code and passes on the fixed one, without needing valgrind or a sanitizer. Also: assertDmOk() now checks the exit status, not just the output. A crash prints everything up to the point it died, so an output-only check read a SIGABRT as a clean run -- which is exactly how this regression test first failed, on a downstream assertion with a misleading message. Verified: the valgrind repro loop over test_streaming_dedupe goes from 7 findings / 144 runs on master to 0 / 162 here. Full make integration-valgrind clean; 119 tests pass. Co-Authored-By: Claude <noreply@anthropic.com> * CLAUDE.md: record the search-pool lifetime rule (#123) The partial-mode section documented the drain but not why the search has to wait for its own workers, which is the invariant #123 violated. Co-Authored-By: Claude <noreply@anthropic.com> * Remove three PATH_MAX leftovers the long-path work missed All pre-existing, all found reviewing #124: places that still assume a path fits PATH_MAX even though nothing there talks to the kernel, or that could be reached with a path that no longer does. - add_exclude_pattern() built the cwd expansion in a fixed buffer and refused anything over PATH_MAX ("cannot prepend cwd to ..."). An exclude pattern only ever reaches fnmatch()/strcmp(), never a syscall, so the bound was inherited cargo -- and it meant you could not exclude the deep subtrees #117 had just made scannable, which is the natural workaround for the one remaining over-PATH_MAX root limit. Built with get_current_dir_name() + asprintf() now, and a strdup/asprintf failure is reported instead of dereferenced. - storage_detect() did stat() + statfs() + open() on the path. One longpath_open() plus fstat()/fstatfs() gives the same three answers with one path lookup, and stops this being another thing in the way if the root limit is ever lifted. - persist_scan_config() dropped a root it could not realpath() with no message. That config is what a bare `oans --hashfile=X` replays, so an omitted root means a later scheduled run quietly covers less ground -- and the "all roots gone" guard cannot fire for a root that was never stored. It warns now. Tests: test_relative_exclude_longer_than_path_max builds a ~3000-char cwd and a 1400-char relative pattern (expansion ~4.4 KB). It fails on the old binary with "cannot prepend cwd" and passes here. harness: DUPEREMOVE is resolved to an absolute path at import. `make integration` passes a relative ./oans, so any test that chdir()s -- like the one above, which has to, to use a relative pattern -- could not find the binary. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
|
Ok, everything in review of the old code has been checked and does not apply for the new changes. So they can all be closed. |
…gure (#151) The README was last substantively updated in #118, two releases ago, so it was missing everything shipped since and carried one number that no longer matched the benchmark doc. Fix a wrong figure: the path-hash row claimed "41 vs 73 MiB on the benchmark tree". Neither number appears in docs/benchmarks.md, which measures 39.7 vs 70.9 MiB -- and that figure comes from the larger-than-RAM tree, not the 2.07M-file tree the surrounding table describes. Correct both the numbers and the attribution, and reword the table intro, which named only one of the two benchmarks the rows actually draw from. Add the missing user-facing work: - paths beyond PATH_MAX are hashed and deduped (#117/#124/#128) - the streaming dedupe pipeline (#116), which had no bullet at all - the O(extents^2) fragmented-file scan fix (#134) - the two dedupe-phase races (#123, #129) - clang ASAN/UBSAN/TSAN CI legs, warnings-as-errors, make check-all - --cpu-threads, absent from both CLI lists despite being in --help - progress polish: scan-phase throughput (#120), idle workers (#143) Tighten for readability: drop the standalone larger-than-RAM NOTE, which stated the same ~13x claim a third time; its unique content (RSS, hashfile size) moves into the speedups table where the reader is already comparing figures. Mark upstream issue references as "upstream #NNN" throughout -- bare markfasheh#331/markfasheh#374/markfasheh#376/markfasheh#387 now read as oans issues, since oans has its own numbers in that range. Docs-only; no code touched. Co-authored-by: Claude <noreply@anthropic.com>
What
Actually hash and deduplicate files whose absolute path exceeds
PATH_MAX(4096), instead of skipping them with a warning. This is the follow-through to #108/#115 (which only made the skip visible).Closes #117.
Why it was non-trivial
The blocker is the kernel:
open()/statx()/stat()reject any single pathname argument longer thanPATH_MAXwithENAMETOOLONG. The only way to reach such a file is to open a reachable ancestor andopenat-walk the remaining components, keeping every argument in range.A useful finding shaped the design: the storage layer was already length-agnostic — the
files.filenamecolumn isTEXT, the store bind uses the full string,struct filerec.filenameis a heapchar*, and bothfilerec_open()and the prunestat()act on the full string. So the full path is available at every access point, and theopenat-split can be derived on demand from the flat string — no schema change and noDB_FILE_MINORbump.How
src/longpath.{c,h}—longpath_open/longpath_opendir/longpath_stat/longpath_lstat. Each reaches an over-PATH_MAXpath by opening a reachable ancestor andopenat-walking the rest, advancing by the longest run of components that fits one syscall argument (memrchr) rather than one component per call: a 4620-byte path costs 5 opens instead of 22. In-range paths take the plainopen()/opendir()/stat()fast path internally, so callers never gate on length and the hot path is untouched.struct file.filenamebecomes a heap-ownedchar*(waschar[PATH_MAX+1]), managed viafile_set_filename()/file_cleanup()so an over-PATH_MAXpath round-trips through scan and change-detection intact.process_dir): reaches deep directories vialongpath_opendir+fdopendir, stats children relative to the directory fd, and drops the old length skip-guard.probe_fs): derives fs UUID, btrfs-ness and supported-ness from a singlelongpath_open()fd (fstatfs+statx(AT_EMPTY_PATH)), replacing the separate path-basedget_uuid()/is_btrfs()/is_fs_supported()calls. The path-basedis_btrfs()is deleted so it cannot be reintroduced.longpath_*helpers.PATH_MAX: the dedupe report printed two distinct group members as identical 4096-char strings naming their parent directory. The progress slot elides via the renderer's ownellipsize_path(), so a shortened path keeps its real head and basename behind one marker.for_each_stdin_line()no longer bounds every consumer.-Ris a SQLDELETEwith no length limit, sooans -L | oans -R -previously could not remove rows the scan had just created;scan_file()keeps its own limit, where therealpath()buffer actually needs one.Guarding the invariant
The failure mode this feature has is nasty: a reintroduced path-argument syscall gets
ENAMETOOLONG, the caller reads it as "gone", the file is dropped — and the run still exits 0. Three layers guard against that:scripts/lint-longpath.py, wired intomake lintandmake check(and its own CI step). It flags any path-taking syscall insrc/that is neither dirfd-relative nor waived with alongpath-ok: <why>comment next to the code. The 23 legitimate sites (hashfile,/proc, device nodes, scan roots, the autotune sampler) each carry a one-line reason. Verified it catches the exact regression it exists to prevent.longpath.h, where anyone touching the API reads it.probe_fsbug below.Tests
>PATH_MAXdirectory chain on tmpfs and coversopen+read,opendir,stat/lstat, missing final component and missing intermediate (ENOENT, notENAMETOOLONG), and short-path equivalence. Teardown is unconditional, so a failed assertion cannot strand the process cwd in a directory it cannot name.progress_copy_path(including degenerate caps) and for the two-stage render, pinning that a shortened path carries exactly one elision marker and keeps both ends.tests/integration/test_long_path.py: the deep file is hashed, dedupes (verified via a dir-fd-relative FIEMAP so the test can reach it too), no-op rescans, prunes selectively, and does not derail its in-range siblings. Covers both the deep-chain and long-basename-in-range shapes.Verification
make lint,scripts/verify.sh— build (no warnings), fullmake check, valgrind smoke: all pass.make integration-valgrind: clean for this change. (An unrelated, pre-existing flaky UAF in the--dedupe-options=partialdrain path is filed as Flaky use-after-free in --dedupe-options=partial drain (find_dupes reads a filerec freed by dedupe_drain) #123 — measured at 7/144 findings on unmodified master and 7/144 with this branch, i.e. identical, and this PR touches neither the free nor the read side of it.)Cannot open …: File name too longand stored 0 of its files while exiting 0.master, warm, interleaved, 175k-file real tree): scan perf-neutral — 12.76 s vs 12.78 s median.Notes / out of scope (documented)
PATH_MAX(deep trees under a short root are fully supported). Now reported with an explicit, actionable message instead of a bareENAMETOOLONG, and documented under NOTES in the man page.PATH_MAX, and the--autotunesampler, which feeds its paths back as roots and so inherits the same limit (it now counts and reports what it skipped rather than dropping it silently).Lifting that last limit needs a
longpath_realpath()(openat the leafO_PATH,readlink /proc/self/fd/N), which would also unblock--file-listand the sampler. Deliberately left for a follow-up.🤖 Generated with Claude Code