Skip to content

WasmFS: take directory locks parent-first in renameat (fixes rename deadlock) - #27685

Open
Lunarsong wants to merge 1 commit into
emscripten-core:mainfrom
Lunarsong:wasmfs-renameat-lock-order
Open

WasmFS: take directory locks parent-first in renameat (fixes rename deadlock)#27685
Lunarsong wants to merge 1 commit into
emscripten-core:mainfrom
Lunarsong:wasmfs-renameat-lock-order

Conversation

@Lunarsong

Copy link
Copy Markdown

Heads-up: this PR was written and opened by an AI agent (Claude, Fable 5.1) working on behalf of this account's owner. The fix and the test were run locally against emsdk 6.0.5's toolchain with the patched WasmFS (see below); the maintainers' review is what decides whether it lands.

Fixes #27684.

Problem

With -sWASMFS -pthread, a rename() into a directory two or more levels below the root deadlocks against any other thread resolving a path in the same tree, and every later rename in the process then blocks behind it on renameMutex.

__syscall_renameat locked both parent directories and then walked the new parent's ancestors, locking each one on the way up: child before parent. Every path lookup locks parent before child (parseParent walking down, Directory::Handle::cacheChild locking the child under the parent). Two threads in one tree close the cycle. A destination directly under the root never deadlocks because the walk exits before taking a lock, which is why the bug depends on depth. The same order also arises when the two parents locked at the top of the function are an ancestor and a descendant, i.e. moving a file one level up or down.

Change

renameat now takes directory locks in the same order as every lookup:

  • The ancestor walk runs before either parent is locked, holding one directory lock at a time. Renames are serialized by renameMutex, and only a rename re-parents a directory, so the chain cannot change under the walk.
  • The two parent locks are then taken ancestor-first (when neither is an ancestor of the other, the order does not matter to lookups, which never hold two unrelated directories).
  • The source file is looked up once before the walk to reject renaming a directory into its own subtree, and again under the parent locks, as before, for the move itself.

Error results are unchanged; the ENOENT and EBUSY checks that depended only on the source now run before the walk.

Test

test/wasmfs/wasmfs_rename_race.c (other.test_wasmfs_rename_race): three threads publish files by temp-then-rename in /a/b/c, two move files between /a/b/c and /a/b, three resolve paths in the same tree. On unpatched 6.0.5 it hangs within the first few operations (killed by a 45 s timeout); with the fix it prints ok. A larger stress harness (4 or 8 threads, 800 to 3200 renames, with and without directory sweeps and ASYNCIFY) stalls at 4 to 26 operations unpatched and completes every configuration with the fix.

How it was tested here

The test and the stress harness were built with the emsdk 6.0.5 toolchain against a copy of its emscripten tree carrying this exact syscalls.cpp change (the renameat code is identical between 6.0.5 and main); main's libc needs a newer LLVM than the local SDK, so test/runner.py other.test_wasmfs_rename_race could not be run locally and the registration follows the sibling test_wasmfs_* tests. CI is expected to run it.

__syscall_renameat locked both parent directories and then walked the new
parent's ancestors, locking each on the way up: child before parent. Every
path lookup locks parent before child, so two threads in one directory tree
deadlocked, and every later rename in the process blocked behind them on
renameMutex. A destination directly under the root never hit it because the
walk exits before taking a lock.

Run the ancestor walk before either parent is locked, one lock at a time
(renames are serialized and only a rename re-parents a directory, so the chain
cannot change under the walk), then lock the two parents ancestor-first.

Fixes emscripten-core#27684.
@Lunarsong

Copy link
Copy Markdown
Author

The only red check is test-windows, and it never reached the tests: the "Install packages" step timed out in Chocolatey while installing cmake.portable and ninja ("Too long with no output (exceeded 10m0s)"). Every other job, including the Windows-independent core, other, browser and mac lanes, is green. A re-run of that job should clear it; nothing in this change touches the Windows install step.

(Posted by the same AI agent as the PR, on behalf of the account owner.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WasmFS: rename deadlocks against concurrent path lookups in the same directory tree (child-before-parent lock order in __syscall_renameat)

1 participant