diff --git a/fs/vfs/vfs.h b/fs/vfs/vfs.h index f655337ea..897e7368f 100644 --- a/fs/vfs/vfs.h +++ b/fs/vfs/vfs.h @@ -159,6 +159,14 @@ void dref(struct dentry *dp); void drele(struct dentry *dp); void dentry_init(void); +#ifdef DEBUG_VFS +/* + * True iff the calling thread holds dentry_hash_lock. Used by vn_lock() to + * assert the VFS lock order (vnode lock -> dentry_hash_lock, never reverse). + */ +bool vfs_dentry_hash_lock_held(void); +#endif + #ifdef DEBUG_VFS void vnode_dump(void); void mount_dump(void); diff --git a/fs/vfs/vfs_dentry.cc b/fs/vfs/vfs_dentry.cc index facd9eaa7..c2fd39064 100644 --- a/fs/vfs/vfs_dentry.cc +++ b/fs/vfs/vfs_dentry.cc @@ -37,16 +37,182 @@ #include #include #include +#include #include #include +#include #include "vfs.h" #define DENTRY_BUCKETS 32 -static LIST_HEAD(dentry_hash_head, dentry) dentry_hash_table[DENTRY_BUCKETS]; -static LIST_HEAD(fake, dentry) fake; -static mutex dentry_hash_lock; +/* + * The "fake" chain that dentry_remove() parks unlinked-but-still-referenced + * dentries on is just one more chain, so give it a slot in the same array and + * it gets a lock by the same rule as every other chain. + */ +#define DENTRY_FAKE_INDEX DENTRY_BUCKETS +#define DENTRY_CHAINS (DENTRY_BUCKETS + 1) + +static LIST_HEAD(dentry_hash_head, dentry) dentry_hash_table[DENTRY_CHAINS]; + +/* + * LOCK ORDERING RULE (whole VFS): + * + * vnode lock (vn_lock) -> any dentry chain lock + * + * and NEVER the reverse. namei() establishes this direction: it holds + * vn_lock(dvp) across dentry_lookup()/dentry_alloc() (vfs_lookup.cc), both of + * which take a chain lock inside. Therefore no code may call anything that + * takes a vnode lock while holding a chain lock. Splitting one lock into + * several does not change this: a per-chain lock inverts against vn_lock + * exactly as the single global one did. + * + * drele() used to violate exactly that: it took the hash lock and then called + * vn_del_name(), which does vn_lock() internally. A namei() on one thread + * (vn_lock held, waiting for the hash lock) against a drele() on another + * (hash lock held, waiting for vn_lock) is an AB-BA deadlock with nothing + * left runnable -- observed as all vCPUs halted in do_idle with an empty + * wakeup mask and >1000 threads parked, while memory was plentiful. + * + * Chain locks are leaf locks. Keep them that way: do not call out to vnode + * code, and do not allocate, while holding one. + * + * MULTI-CHAIN OPERATIONS. Most sites touch exactly one chain, which is the + * point of the split. Two do not, and both are handled explicitly rather + * than assumed independent: + * + * - dentry_move() unlinks dp from the chain for its old path, re-inserts it + * on the chain for the new path, and unlinks every child of dp from + * whatever chains they happen to be on. It therefore takes ALL chain + * locks (dentry_lock_all), in ascending index order. + * - dentry_remove() moves a dentry from its hash chain to the fake chain, + * i.e. two chains, so it locks both, again in ascending index order. + * + * Ascending-index acquisition is what keeps chain-vs-chain lock order safe. + * d_hash_index records which chain a dentry is currently on, so an unlink can + * find the right lock without rehashing a d_path that a rename may already + * have replaced. + */ +static mutex dentry_chain_lock[DENTRY_CHAINS]; + +/* + * OSV_VFS_DENTRY_PERBUCKET=0 : route every chain lock to chain 0's mutex, so + * all chains share one lock and the code behaves like the original single + * global dentry_hash_lock. Same-binary A/B for the lock split. The ordering + * fix in drele() is unconditional and is NOT affected by this switch. + */ +static bool dentry_perbucket_enabled() +{ + static std::atomic cached{-1}; + int v = cached.load(std::memory_order_relaxed); + if (v < 0) { + const char *e = getenv("OSV_VFS_DENTRY_PERBUCKET"); + v = (!e || !e[0]) ? 1 : (e[0] != '0'); + // Proof of binding: say what the flag resolved to, once. The default + // is the non-inert value, so a dropped --env= would otherwise give + // identical behaviour in both arms of an A/B and read as "no effect". + debug_early("VFSFLAG OSV_VFS_DENTRY_PERBUCKET"); + debug_early(e ? "=" : " unset, default "); + debug_early(v ? "1\n" : "0\n"); + cached.store(v, std::memory_order_relaxed); + } + return v != 0; +} + +static mutex *chain_mutex(unsigned index) +{ + return &dentry_chain_lock[dentry_perbucket_enabled() ? index : 0]; +} + +#ifdef DEBUG_VFS +/* + * Teeth for the ordering rule above. When DEBUG_VFS is on, every thread + * tracks how many dentry chain locks it holds; vn_lock() asserts that it + * holds none. This fires on the unfixed drele() and is silent once + * vn_del_name() is moved out of the critical section. + */ +extern "C" bool vfs_dentry_hash_lock_held(void); /* declared in vfs.h */ +static __thread int dentry_hash_lock_depth; +bool vfs_dentry_hash_lock_held(void) +{ + return dentry_hash_lock_depth != 0; +} +static void dentry_hash_lock_enter(void) { dentry_hash_lock_depth++; } +static void dentry_hash_lock_exit(void) { dentry_hash_lock_depth--; } +#else +static inline void dentry_hash_lock_enter(void) {} +static inline void dentry_hash_lock_exit(void) {} +#endif + +/* + * Chain lock accessors. Use these, not mutex_lock/unlock directly, so the + * ordering instrumentation cannot be bypassed by a new call site. + */ +static void dentry_lock(unsigned index) +{ + mutex_lock(chain_mutex(index)); + dentry_hash_lock_enter(); +} + +static void dentry_unlock(unsigned index) +{ + dentry_hash_lock_exit(); + mutex_unlock(chain_mutex(index)); +} + +/* + * Lock every chain, ascending. Used only by dentry_move(), which can touch an + * unbounded set of chains (dp's old chain, its new chain, and one per child). + * With OSV_VFS_DENTRY_PERBUCKET=0 all indices alias chain 0, so take that + * single lock once instead of recursing on it DENTRY_CHAINS times. + * + * ponytail: lock-all is the coarse option for a rare operation (rename of a + * directory with cached children). If rename ever shows up hot, narrow it to + * the union of chains actually touched, collected under d_lock first. + */ +static void dentry_lock_all(void) +{ + if (!dentry_perbucket_enabled()) { + dentry_lock(0); + return; + } + for (unsigned i = 0; i < DENTRY_CHAINS; i++) { + dentry_lock(i); + } +} + +static void dentry_unlock_all(void) +{ + if (!dentry_perbucket_enabled()) { + dentry_unlock(0); + return; + } + for (unsigned i = DENTRY_CHAINS; i > 0; i--) { + dentry_unlock(i - 1); + } +} + +/* + * Lock the chain a dentry is currently on, and return its index. + * + * d_hash_index must be read to know which lock to take, but it is itself only + * stable under that lock, and dentry_move()/dentry_remove() can move a dentry + * between chains. So read, lock, then re-check: if the index changed while we + * were acquiring, we locked the wrong chain -- drop it and retry. Converges + * because a dentry's chain only changes on rename/unlink, not in a loop. + */ +static unsigned dentry_lock_chain_of(struct dentry *dp) +{ + for (;;) { + unsigned index = dp->d_hash_index; + dentry_lock(index); + if (dp->d_hash_index == index) { + return index; + } + dentry_unlock(index); + } +} /* * Get the hash value from the mount point and path name. @@ -95,9 +261,11 @@ dentry_alloc(struct dentry *parent_dp, struct vnode *vp, const char *path) vn_add_name(vp, dp); - mutex_lock(&dentry_hash_lock); - LIST_INSERT_HEAD(&dentry_hash_table[dentry_hash(mp, path)], dp, d_link); - mutex_unlock(&dentry_hash_lock); + unsigned index = dentry_hash(mp, path); + dentry_lock(index); + dp->d_hash_index = index; + LIST_INSERT_HEAD(&dentry_hash_table[index], dp, d_link); + dentry_unlock(index); return dp; }; @@ -106,15 +274,16 @@ dentry_lookup(struct mount *mp, char *path) { struct dentry *dp; - mutex_lock(&dentry_hash_lock); - LIST_FOREACH(dp, &dentry_hash_table[dentry_hash(mp, path)], d_link) { + unsigned index = dentry_hash(mp, path); + dentry_lock(index); + LIST_FOREACH(dp, &dentry_hash_table[index], d_link) { if (dp->d_mount == mp && !strncmp(dp->d_path, path, PATH_MAX)) { dp->d_refcnt++; - mutex_unlock(&dentry_hash_lock); + dentry_unlock(index); return dp; } } - mutex_unlock(&dentry_hash_lock); + dentry_unlock(index); return nullptr; /* not found */ } @@ -128,14 +297,18 @@ static void dentry_children_remove(struct dentry *dp) ASSERT(entry->d_refcnt > 0); LIST_REMOVE(entry, d_link); } - } -} + }} void dentry_move(struct dentry *dp, struct dentry *parent_dp, char *path) { struct dentry *old_pdp = dp->d_parent; char *old_path = dp->d_path; + // Duplicate the new path BEFORE taking dentry_hash_lock. strdup() can + // block in the page allocator, and dentry_hash_lock is a leaf lock held + // by every lookup in the system; sleeping under it stalls all VFS name + // resolution for the duration. Nothing here needs the lock. + char *new_path = strdup(path); if (old_pdp) { WITH_LOCK(old_pdp->d_lock) { @@ -152,18 +325,21 @@ dentry_move(struct dentry *dp, struct dentry *parent_dp, char *path) } } - WITH_LOCK(dentry_hash_lock) { - // Remove all dp's child dentries from the hashtable. - dentry_children_remove(dp); - // Remove dp with outdated hash info from the hashtable. - LIST_REMOVE(dp, d_link); - // Update dp. - dp->d_path = strdup(path); - dp->d_parent = parent_dp; - // Insert dp updated hash info into the hashtable. - LIST_INSERT_HEAD(&dentry_hash_table[dentry_hash(dp->d_mount, path)], - dp, d_link); - } + // dentry_move touches dp's old chain, dp's new chain, and the chain of + // every cached child, so it takes them all (ascending; see the note above). + dentry_lock_all(); + // Remove all dp's child dentries from the hashtable. + dentry_children_remove(dp); + // Remove dp with outdated hash info from the hashtable. + LIST_REMOVE(dp, d_link); + // Update dp with the path duplicated above, outside the lock. + dp->d_path = new_path; + dp->d_parent = parent_dp; + // Insert dp updated hash info into the hashtable. + unsigned index = dentry_hash(dp->d_mount, path); + dp->d_hash_index = index; + LIST_INSERT_HEAD(&dentry_hash_table[index], dp, d_link); + dentry_unlock_all(); if (old_pdp) { drele(old_pdp); @@ -175,11 +351,24 @@ dentry_move(struct dentry *dp, struct dentry *parent_dp, char *path) void dentry_remove(struct dentry *dp) { - mutex_lock(&dentry_hash_lock); + // Two chains: the one dp is on, and the fake chain it moves to. Lock the + // current chain first (re-checking the index), then the fake chain if it + // is a different one, keeping ascending order. + unsigned index = dentry_lock_chain_of(dp); + bool need_fake = dentry_perbucket_enabled() && index != DENTRY_FAKE_INDEX; + if (need_fake) { + // DENTRY_FAKE_INDEX is the highest index, so taking it second is + // already ascending order. + dentry_lock(DENTRY_FAKE_INDEX); + } LIST_REMOVE(dp, d_link); /* put it on a fake list for drele() to work*/ - LIST_INSERT_HEAD(&fake, dp, d_link); - mutex_unlock(&dentry_hash_lock); + LIST_INSERT_HEAD(&dentry_hash_table[DENTRY_FAKE_INDEX], dp, d_link); + dp->d_hash_index = DENTRY_FAKE_INDEX; + if (need_fake) { + dentry_unlock(DENTRY_FAKE_INDEX); + } + dentry_unlock(index); } void @@ -188,9 +377,9 @@ dref(struct dentry *dp) ASSERT(dp); ASSERT(dp->d_refcnt > 0); - mutex_lock(&dentry_hash_lock); + unsigned index = dentry_lock_chain_of(dp); dp->d_refcnt++; - mutex_unlock(&dentry_hash_lock); + dentry_unlock(index); } void @@ -199,15 +388,27 @@ drele(struct dentry *dp) ASSERT(dp); ASSERT(dp->d_refcnt > 0); - mutex_lock(&dentry_hash_lock); + unsigned index = dentry_lock_chain_of(dp); if (--dp->d_refcnt) { - mutex_unlock(&dentry_hash_lock); + dentry_unlock(index); return; } + /* + * Last reference. Unlink from the hash chain while still holding the + * lock -- that is what makes the drop below safe: once dp is off the + * chain, dentry_lookup() can no longer find it, so no other thread can + * resurrect it by taking a new reference. This thread is the sole owner + * of dp from here on, and d_refcnt is 0 and stays 0. + * + * vn_del_name() must NOT be called under a chain lock: it takes the vnode + * lock, which inverts the vn_lock -> chain lock order that namei() + * establishes (see the ordering note at the top of this file). Release + * first, then touch the vnode. + */ LIST_REMOVE(dp, d_link); - vn_del_name(dp->d_vnode, dp); + dentry_unlock(index); - mutex_unlock(&dentry_hash_lock); + vn_del_name(dp->d_vnode, dp); if (dp->d_parent) { WITH_LOCK(dp->d_parent->d_lock) { @@ -228,7 +429,7 @@ dentry_init(void) { int i; - for (i = 0; i < DENTRY_BUCKETS; i++) { + for (i = 0; i < DENTRY_CHAINS; i++) { LIST_INIT(&dentry_hash_table[i]); } } diff --git a/fs/vfs/vfs_vnode.cc b/fs/vfs/vfs_vnode.cc index f4d072d1c..8f65ce64c 100644 --- a/fs/vfs/vfs_vnode.cc +++ b/fs/vfs/vfs_vnode.cc @@ -143,6 +143,16 @@ vn_lock(struct vnode *vp) ASSERT(vp); ASSERT(vp->v_refcnt > 0); +#ifdef DEBUG_VFS + /* + * Lock-order check with teeth (see the ordering note in vfs_dentry.cc): + * the VFS order is vnode lock -> dentry_hash_lock, never the reverse. + * Taking a vnode lock while already holding dentry_hash_lock is the + * AB-BA inversion that deadlocked drele() against namei(). + */ + ASSERT(!vfs_dentry_hash_lock_held()); +#endif + mutex_lock(&vp->v_lock); vp->v_nrlocks++; DPRINTF(VFSDB_VNODE, ("vn_lock: %s\n", vn_path(vp))); diff --git a/include/osv/dentry.h b/include/osv/dentry.h index 4a167fff7..32e4b3270 100644 --- a/include/osv/dentry.h +++ b/include/osv/dentry.h @@ -24,6 +24,13 @@ struct dentry { mutex_t d_lock; LIST_HEAD(, dentry) d_children; LIST_ENTRY(dentry) d_children_link; + /* + * Index of the hash chain d_link currently sits on, so a dentry can be + * unlinked under the correct per-chain lock without recomputing a hash + * from d_path (which may already have been replaced by a rename). + * Maintained by fs/vfs/vfs_dentry.cc; see the locking note there. + */ + unsigned d_hash_index; }; #if defined(__cplusplus) && !defined(USE_C_INTERFACE) diff --git a/modules/tests/Makefile b/modules/tests/Makefile index 9e2ab99a9..f73c4eb3d 100644 --- a/modules/tests/Makefile +++ b/modules/tests/Makefile @@ -148,6 +148,7 @@ tests := tst-iovcnt-guard.so tst-pthread.so misc-ramdisk.so tst-vblk.so tst-mq-s tst-mmap-file.so misc-mmap-big-file.so tst-mmap.so tst-huge.so \ tst-pthread-timedlock.so \ tst-signal-fills.so \ + tst-dentry-lock.so \ tst-epoll-pwait2.so \ tst-splice.so \ tst-fs-syscalls.so \ diff --git a/tests/tst-dentry-lock.cc b/tests/tst-dentry-lock.cc new file mode 100644 index 000000000..e7bff3494 --- /dev/null +++ b/tests/tst-dentry-lock.cc @@ -0,0 +1,190 @@ +/* + * Copyright (C) 2026 Greg Burd + * + * This work is open source software, licensed under the terms of the + * BSD license as described in the LICENSE file in the top-level directory. + */ + +// Concurrency stress for the dentry cache, aimed at the lock-order inversion +// between dentry_hash_lock and the vnode lock. +// +// The wedge this reproduces: namei() holds vn_lock(dvp) while calling +// dentry_lookup()/dentry_alloc(), which take dentry_hash_lock -- so the order +// is vn_lock -> dentry_hash_lock. drele() used to take dentry_hash_lock and +// then call vn_del_name(), which takes vn_lock internally -- the reverse. +// Two threads hitting both orders on the same directory vnode deadlock with +// nothing runnable. +// +// To hit it you need many threads doing open()/close() of DIFFERENT names in +// the SAME directory (so they share the parent directory's vnode but land on +// different hash buckets), plus concurrent rename() and unlink() traffic to +// exercise dentry_move()/dentry_remove() and force dentries onto the fake +// fake list. A single-threaded test never sees it. +// +// On unfixed code this hangs (the harness kills it by timeout). On fixed +// code it completes and reports OK. Under DEBUG_VFS the vn_lock() assertion +// fires on the unfixed path instead of hanging, which is the faster signal. + +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +static std::atomic failures{0}; +static std::atomic ops{0}; + +#define EXPECT(cond, msg) \ + do { \ + if (!(cond)) { \ + std::printf("FAIL %s:%d: %s (errno=%d %s)\n", __FILE__, __LINE__, \ + (msg), errno, std::strerror(errno)); \ + failures++; \ + } \ + } while (0) + +static const char *DIR = "/tmp/tst-dentry-lock"; + +// Open and close the same set of names repeatedly. All of these live in one +// directory, so every lookup takes that directory's vnode lock and then a +// dentry_hash_lock; the names differ so they spread across hash buckets. +static void open_close_worker(int id, int iters, int names) +{ + for (int i = 0; i < iters; i++) { + for (int n = 0; n < names; n++) { + char path[256]; + std::snprintf(path, sizeof(path), "%s/f%d", DIR, (id + n * 7) % names); + int fd = open(path, O_RDONLY); + if (fd >= 0) { + close(fd); + ops++; + } + // A miss is fine and is itself interesting: a failed lookup still + // walks the chain and still drele()s the parent dentry. + } + } +} + +// Stat a deep path so each call walks several components, taking and dropping +// a directory dentry reference at every level -- this is the drele() side. +static void stat_worker(int iters) +{ + for (int i = 0; i < iters; i++) { + struct stat st; + char path[256]; + std::snprintf(path, sizeof(path), "%s/d0/d1/d2", DIR); + if (stat(path, &st) == 0) { + ops++; + } + std::snprintf(path, sizeof(path), "%s/d0/d1", DIR); + if (stat(path, &st) == 0) { + ops++; + } + } +} + +// Rename traffic: exercises dentry_move() and the +// strdup that used to happen under the lock. +static void rename_worker(int id, int iters) +{ + for (int i = 0; i < iters; i++) { + char a[256], b[256]; + std::snprintf(a, sizeof(a), "%s/r%d_%d", DIR, id, i & 1); + std::snprintf(b, sizeof(b), "%s/r%d_%d", DIR, id, (i & 1) ^ 1); + int fd = open(a, O_CREAT | O_RDWR, 0644); + if (fd >= 0) { + close(fd); + if (rename(a, b) == 0) { + ops++; + } + unlink(a); + unlink(b); + } + } +} + +// Create/unlink churn: forces dentry_remove(), which moves dentries onto the +// fake list. +static void churn_worker(int id, int iters) +{ + for (int i = 0; i < iters; i++) { + char path[256]; + std::snprintf(path, sizeof(path), "%s/c%d_%d", DIR, id, i % 4); + int fd = open(path, O_CREAT | O_RDWR, 0644); + if (fd >= 0) { + close(fd); + // Open it again by name so a second dentry reference exists while + // it is being removed. + int fd2 = open(path, O_RDONLY); + unlink(path); + if (fd2 >= 0) { + close(fd2); + } + ops++; + } + } +} + +int main(int argc, char **argv) +{ + // Deliberately more threads than CPUs: the inversion needs two threads to + // interleave inside the two lock acquisitions, so oversubscription helps. + int nthreads = 16; + int iters = 200; + const int names = 32; // == DENTRY_BUCKETS, to spread across all buckets + if (argc > 1) nthreads = std::atoi(argv[1]); + if (argc > 2) iters = std::atoi(argv[2]); + + std::printf("tst-dentry-lock: %d threads x %d iters\n", nthreads, iters); + + mkdir(DIR, 0777); + char p[256]; + std::snprintf(p, sizeof(p), "%s/d0", DIR); mkdir(p, 0777); + std::snprintf(p, sizeof(p), "%s/d0/d1", DIR); mkdir(p, 0777); + std::snprintf(p, sizeof(p), "%s/d0/d1/d2", DIR); mkdir(p, 0777); + for (int n = 0; n < names; n++) { + std::snprintf(p, sizeof(p), "%s/f%d", DIR, n); + int fd = open(p, O_CREAT | O_RDWR, 0644); + EXPECT(fd >= 0, "setup open"); + if (fd >= 0) close(fd); + } + + std::vector ts; + for (int i = 0; i < nthreads; i++) { + switch (i % 4) { + case 0: ts.emplace_back(open_close_worker, i, iters, names); break; + case 1: ts.emplace_back(stat_worker, iters); break; + case 2: ts.emplace_back(rename_worker, i, iters); break; + default: ts.emplace_back(churn_worker, i, iters); break; + } + } + for (auto &t : ts) { + t.join(); + } + + // Cleanup (best effort; failures here are not the point of the test). + for (int n = 0; n < names; n++) { + std::snprintf(p, sizeof(p), "%s/f%d", DIR, n); + unlink(p); + } + std::snprintf(p, sizeof(p), "%s/d0/d1/d2", DIR); rmdir(p); + std::snprintf(p, sizeof(p), "%s/d0/d1", DIR); rmdir(p); + std::snprintf(p, sizeof(p), "%s/d0", DIR); rmdir(p); + rmdir(DIR); + + std::printf("tst-dentry-lock: %ld ops, %d failures\n", ops.load(), + failures.load()); + if (failures) { + std::printf("FAILED\n"); + return 1; + } + // Reaching this line at all is the result: the unfixed kernel never does. + std::printf("OK\n"); + return 0; +}