Conversation
drele() took dentry_hash_lock and then called vn_del_name(), which takes
the vnode lock internally. namei() establishes the opposite order: it
holds vn_lock(dvp) across dentry_lookup() and dentry_alloc() (see
fs/vfs/vfs_lookup.cc:178 and :284), both of which take dentry_hash_lock.
A namei() on one thread (holding vn_lock, waiting for dentry_hash_lock)
racing a drele() on another (holding dentry_hash_lock, waiting for
vn_lock) leaves neither able to proceed and nothing runnable. Observed
on a 32-vCPU guest under concurrent open()/close() traffic from a
forking server: every vCPU halted in do_idle with an empty wakeup mask
and >1000 threads parked, while ~27 GiB of memory was still free. It is
reachable whenever the dentry being released is a directory, which
namei() itself does on every path component, so it is not theoretical.
Establish and document a single order for the VFS:
vnode lock -> dentry_hash_lock, never the reverse
and make drele() conform. The unlink from the hash chain stays inside
the critical section, which is what makes releasing early safe: once the
dentry is off the chain, dentry_lookup() cannot find it and no thread can
take a new reference, so the releasing thread is its sole owner and the
refcount is 0 and stays 0. vn_del_name() then runs outside the lock.
dentry_hash_lock is now a leaf lock: it calls out to no other subsystem.
Route every acquisition through dentry_hash_lock_acquire()/_release() so
a future call site cannot quietly bypass the rule, and under DEBUG_VFS
track per-thread ownership so vn_lock() can assert the order directly.
That assertion fires on the unfixed drele() and is silent once
vn_del_name() moves out of the critical section.
Signed-off-by: Greg Burd <greg@burd.me>
(cherry picked from commit 6e250b6)
dentry_move() called strdup() inside the dentry_hash_lock critical section. That allocation can block in the page allocator, and dentry_hash_lock serialises every name lookup in the system, so a rename that happens to hit a slow allocation stalls all VFS path resolution until it completes. Duplicate the path before acquiring the lock. Nothing in the copy needs the lock held; only the list surgery does. No functional change beyond the shorter critical section. This is a latent defect on the rename path, separate from the lock-order inversion fixed in the previous commit. Signed-off-by: Greg Burd <greg@burd.me> (cherry picked from commit 48d2ea0)
Reproducer for the AB-BA deadlock between the dentry chain locks and the vnode lock. Hitting it needs many threads opening and closing DIFFERENT names in the SAME directory, so they contend on one directory vnode while landing on different hash chains, plus concurrent rename() and unlink() traffic to drive dentry_move() and dentry_remove(). A single-threaded test never sees it, which is why the defect survived this long. On an unfixed kernel this wedges with nothing runnable and the harness kills it by timeout. On a fixed kernel it completes and prints OK. Built with DEBUG_VFS the vn_lock() lock-order assertion fires on the unfixed path instead of hanging, which is a faster and more specific signal than a hang. Signed-off-by: Greg Burd <greg@burd.me> (cherry picked from commit 6467455)
|
End-to-end confirmation on a real workload, in addition to the lock-order assertion already in the PR. A PostgreSQL-on-ZFS HammerDB run on a base WITHOUT this fix wedged the guest hard at 32 virtual users: Rebuilding the identical image with this fix applied and re-running the same 32-VU cells: no wedge, three cells completed at ~62,400 NOPM, with ~18 of 32 vCPUs busy (vs 0 in the wedge). So the fix removes the deadlock end-to-end under a real database workload, not just under the lock-order assertion. To be clear about scope: clearing the deadlock lets the workload run at 32 VU, but throughput there is still well below Linux on the same hardware-that ceiling is a separate WAL-commit-completion serialization issue, not this deadlock, and is not something this PR claims to address. This PR's claim is narrower and now has both forms of proof: the lock-order assertion fires on the unfixed path, and the workload that deadlocked without it completes with it. |
The VFS has an implicit lock order that
namei()establishes anddrele()violated. This series states the rule, adds an assertion that enforces it, and
makes
drele()conform.The rule
fs/vfs/vfs_lookup.ccholds the directory vnode lock across the dentry cachelookup, at two sites (
:178and:284):So the order is vnode lock, then
dentry_hash_lock. Nothing may take avnode lock while holding
dentry_hash_lock.drele()did exactly that. It tookdentry_hash_lockand then calledvn_del_name(), which takes the vnode lock internally(
fs/vfs/vfs_vnode.cc:531):A
namei()holdingvn_lockand waiting fordentry_hash_lock, against adrele()holdingdentry_hash_lockand waiting forvn_lock, is an AB-BAdeadlock in which neither side can proceed.
Evidence
The first commit adds
ASSERT(!vfs_dentry_hash_lock_held())tovn_lock()under
DEBUG_VFS, so the rule is checked rather than asserted in prose. Onunfixed code it fires immediately, during
vfs_init, with this stack:On fixed code, same test and same configuration, the assertion is silent and
the test passes:
Note what the stack shows:
fdrop()is the ordinaryclose()path(
fs/vfs/main.cc:154,:159,:194). The illegal order is therefore taken byevery
close()that drops a last dentry reference. It is not a rareinterleaving; the lock cycle is constructed constantly, and the only missing
ingredient for the deadlock is a concurrent
namei()on the same vnode.That ingredient was supplied in production. On a 32-vCPU guest running a
forking server with 184 concurrent backends, the system wedged with
dentry_hash_lockheld, every vCPU halted indo_idlewith an empty wakeupmask, and over a thousand threads parked, while ~27 GiB of memory was free.
The included stress test passes on unfixed code. 48 threads x 400
iterations, 172,800 operations, 16 vCPUs under TCG, and it does not wedge:
OSv's mutex hold times here are short, and closing the cycle needs genuine
parallelism plus a same-vnode collision. A reviewer who runs it on a laptop and
sees it pass should not conclude the defect is fictional; the assertion above
is the reliable signal, and the hang itself was observed on 32 real vCPUs. The
test is included because it exercises the paths (concurrent open/close in one
directory, plus rename and unlink churn) and would catch a regression that
reintroduces a blocking call under the lock.
The fix
Release
dentry_hash_lockbefore callingvn_del_name(). Unlinking from thehash chain stays inside the critical section, and that is what makes the early
release safe: once the dentry is off the chain,
dentry_lookup()cannot findit, so no thread can take a new reference. The releasing thread is its sole
owner and the refcount is 0 and stays 0.
dentry_hash_lockbecomes a leaf lock: it calls out to no other subsystem.Every acquisition is routed through accessors so a new call site cannot quietly
bypass the rule, and the per-thread ownership tracking that the assertion needs
costs nothing when
DEBUG_VFSis off.Commits
vfs: fix AB-BA deadlock between dentry_hash_lock and the vnode lock- theordering fix, the documented rule, and the assertion.
vfs: do not allocate while holding dentry_hash_lock in dentry_move()- anindependent latent defect found while auditing the other call sites.
dentry_move()calledstrdup()inside the critical section; thatallocation can block in the page allocator while every lookup in the system
waits. Hoisted above the lock. Not the deadlock, and on the
rename()pathrather than the
open()path, but the same class of mistake.tests: concurrency stress for the dentry cache lock order- the test,with the caveat above recorded in its header comment.
Notes
Not fork-specific.
forksupplies the concurrency that closes the cycle, butthe inversion is present regardless: there are no
CONF_forkreferences inthis series, and
vfs_dentry.ccandvfs_vnode.cccompile clean withconf_fork=0.A separate change splitting the global
dentry_hash_lockinto per-bucket locksis deliberately not in this PR. The 32 hash buckets have always shared one
mutex taken at 14 sites, which is a scalability defect, but it is independent of
this correctness fix and a reviewer may reasonably want one without the other.
It also has to come after this fix, because a per-bucket lock inverts against
the vnode lock exactly as the global one does. Its throughput effect is
currently unmeasured and no claim is made about it.
Tested on x86_64: 12 of 12 relevant tests pass (
tst-vfs,tst-symlink141assertions,
tst-remove38,tst-pipe87,tst-mmap,tst-condvar,tst-yield,tst-dentry-lock, and three fork tests).tst-openatreports onepre-existing failure that reproduces identically on unpatched master.