Conversation
__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.
|
The only red check is (Posted by the same AI agent as the PR, on behalf of the account owner.) |
| bool newParentAboveOld = false; | ||
| if (!oldParentAboveNew) { | ||
| for (auto curr = oldParent; curr && curr != root; | ||
| curr = curr->locked().getParent()) { |
There was a problem hiding this comment.
These two loops are taking locks from children to parents aren't they? Isn't that what we should not be doing?
I was expecting this code to go ancestor-first, as in the description. It could follow the code in parseParent which goes ancestor first. Perhaps even add a utility to check if one path is nested in another, and it could be in paths.cpp and share some code there.
Or am I misunderstanding something?
Otherwise this looks great, good find of a real bug!
2 participants
Fixes #27684.
Problem
With
-sWASMFS -pthread, arename()into a directory two or more levels below the root deadlocks against any other thread resolving a path in the same tree, and every laterrenamein the process then blocks behind it onrenameMutex.__syscall_renameatlocked 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 (parseParentwalking down,Directory::Handle::cacheChildlocking 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
renameatnow takes directory locks in the same order as every lookup:renameMutex, and only a rename re-parents a directory, so the chain cannot change under the walk.Error results are unchanged; the
ENOENTandEBUSYchecks 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/cand/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 printsok. 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.cppchange (therenameatcode is identical between 6.0.5 andmain);main's libc needs a newer LLVM than the local SDK, sotest/runner.py other.test_wasmfs_rename_racecould not be run locally and the registration follows the siblingtest_wasmfs_*tests. CI is expected to run it.