Skip to content

vfs: split the global dentry hash lock into per-chain locks - #1503

Open
gburd wants to merge 4 commits into
cloudius-systems:masterfrom
gburd:pr/vfs-dentry-perchain
Open

gburd wants to merge 4 commits into
cloudius-systems:masterfrom
gburd:pr/vfs-dentry-perchain

Conversation

@gburd

@gburd gburd commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

The dentry cache has had 32 hash buckets since it was written:

#define DENTRY_BUCKETS 32
static LIST_HEAD(dentry_hash_head, dentry) dentry_hash_table[DENTRY_BUCKETS];
static mutex dentry_hash_lock;

One mutex guards all of them, taken at 14 sites. Every path lookup in the
system serialises on it, so name resolution does not scale with CPU count no
matter how well the hash distributes. This gives each bucket its own lock.

Builds on #1502, which must come first: that PR fixes an AB-BA inversion
between dentry_hash_lock and the vnode lock, and a per-bucket lock inverts
against the vnode lock in exactly the same way. Splitting the lock without
fixing the order first would preserve the deadlock while making it harder to
reason about. The "chain locks are leaf locks" rule from #1502 still holds and
is documented alongside the lock array.

Not every site touches one bucket

The split does not assume bucket independence, because three call sites do not
have it:

  • dentry_move() unlinks a dentry from the bucket for its old path, inserts it
    on the bucket for its new path, and unlinks every cached child from whatever
    buckets those are on. The child set is unbounded and not known before walking
    it, so this takes all bucket locks, in ascending index order.
  • dentry_remove() moves a dentry from its bucket to the "fake" list that
    exists so drele() keeps working on unlinked dentries. That list is now slot
    32 of the same array, so it is locked by the same rule as any other chain, and
    both locks are taken in ascending order.
  • dref()/drele() must lock the bucket a dentry is currently on, which means
    reading the index before knowing which lock to take. The index is only stable
    under that lock, and dentry_move()/dentry_remove() can move a dentry
    between buckets, so they read it, lock, then re-check and retry if it changed.
    This converges: a dentry changes bucket only on rename or unlink, not
    repeatedly under a spinning reader.

Ascending-index acquisition is what keeps bucket-versus-bucket ordering safe.

d_hash_index is added to struct dentry to record the current bucket, so an
unlink does not have to rehash a d_path that a rename may already have
replaced. Folding the fake list into the array as slot 32 removes the one chain
that previously had no lock of its own.

A/B switch

OSV_VFS_DENTRY_PERBUCKET=0 aliases every bucket to slot 0's mutex, restoring
single-global-lock behaviour in the same binary, so the split can be measured
without comparing two builds. The flag prints what it resolved to on first
read:

VFSFLAG OSV_VFS_DENTRY_PERBUCKET unset, default 1
VFSFLAG OSV_VFS_DENTRY_PERBUCKET=0

so an A/B cannot silently compare an arm against itself, which is easy to do
when the default is the non-inert value and a --env= gets dropped.

Unmeasured

No throughput claim. This is a defect on inspection (one mutex for 32
existing buckets, 14 acquisition sites, contended by every lookup), and the
motivation is a workload where OSv scales negatively with concurrency while
Linux scales positively. But the contribution of this change specifically has
not been measured yet, and the switch above exists precisely so it can be
attributed rather than assumed. If a reviewer would rather wait for numbers
before taking a lock-structure change, that is reasonable and I will follow up
with them.

Correctness testing on x86_64, with the switch in both positions: tst-vfs,
tst-symlink (141 assertions), tst-remove (38), tst-pipe (87), tst-mmap,
tst-dentry-lock, and the fork tests all pass. Compiles clean with
conf_fork=0; no CONF_fork references.

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)
The dentry cache has had DENTRY_BUCKETS 32 hash chains since it was
written, but a single global mutex serialised access to all of them,
taken at 14 sites.  Every path lookup in the system contends on it, so
name resolution does not scale with CPU count no matter how well the
hash distributes.

Give each chain its own mutex.  Preserving the lock order established in
the AB-BA fix is a requirement, not a side effect: a per-chain lock
inverts against the vnode lock exactly as the global one did, so the
"chain locks are leaf locks" rule still holds and is documented with the
array.

Not every site touches a single chain, so the split does not assume
independence:

  - dentry_move() unlinks a dentry from its old chain, inserts it on the
    chain for its new path, and unlinks every cached child from whatever
    chains those are on.  It takes all chain locks, ascending.
  - dentry_remove() moves a dentry from its chain to the "fake" chain.
    That chain is now slot DENTRY_BUCKETS of the same array, so it is
    locked by the same rule; both locks are taken ascending.
  - dref()/drele() must lock the chain a dentry is currently on, which
    means reading d_hash_index before knowing which lock to take.  That
    field is only stable under the lock, so they read it, lock, then
    re-check and retry if it moved.  Converges: a dentry changes chain
    only on rename or unlink.

d_hash_index is added to struct dentry to record the current chain, so an
unlink does not have to rehash a d_path that a rename may already have
replaced.

OSV_VFS_DENTRY_PERBUCKET=0 aliases every chain to slot 0's mutex,
restoring single-global-lock behaviour in the same binary for A/B
measurement.  The AB-BA ordering fix is unconditional and unaffected by
the switch.  The flag prints what it resolved to on first read, so an A/B
cannot silently compare an arm against itself.

Signed-off-by: Greg Burd <greg@burd.me>
(cherry picked from commit a642706)
@gburd gburd changed the title vfs: split the global dentry hash lock into per-bucket locks vfs: split the global dentry hash lock into per-chain locks Sep 10, 2026
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.

1 participant