FDB-738: NFS support - #339
Conversation
There was a problem hiding this comment.
Pull request overview
Adds runtime NFS detection and synchronization intended to protect TOC reads and appends from non-atomic NFS behavior.
Changes:
- Detects NFS mounts on Linux and macOS.
- Adds TOC file locking, cache refresh, and directory synchronization.
- Adds NFS detection and TOC round-trip tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/fdb5/toc/TocHandler.cc |
Implements NFS detection and synchronization. |
src/fdb5/toc/TocHandler.h |
Exposes detection and caches NFS status. |
tests/fdb/database/test_nfs.cc |
Adds detection and round-trip tests. |
tests/fdb/database/CMakeLists.txt |
Registers the new test target. |
Suppressed comments (5)
src/fdb5/toc/TocHandler.cc:299
- POSIX
F_SETLKWrecord locks are process-associated, so two writer threads in this process acquire this lock without blocking each other; closing any descriptor for the TOC can also release the process's locks. FDB explicitly supports one-FDB-per-thread concurrent archives (tests/fdb/concurrent/test_thread.cc:88-95), so NFS appends can still race. Add process-local per-TOC serialization for the same lifetime as the interprocess lock (or use suitable open-file-description locks where available).
if (onNFS() && !isSubToc_) {
tocFileLock(fd_, F_WRLCK);
}
src/fdb5/toc/TocHandler.cc:1119
- A shared read lock does not serialize racing creators. Two clients can both hold
F_RDLCK, both observe an empty TOC, and then deadlock or receiveEDEADLKwhile upgrading; because lock errors are currently ignored, duplicate init records can be written. Take the exclusive lock before inspecting the TOC so the check-and-initialize sequence is atomic.
// Hold a lock to serialise racing creators. Released on close().
if (nfs) {
tocFileLock(fd_, F_RDLCK);
src/fdb5/toc/TocHandler.cc:343
- The cached read obtains
tocSizeat line 327 before taking this lock. If an NFS writer is mid-append, that unlocked size can cut through a record; after waiting for the writer, the reader still copies only that partial length into the cache. Acquire the read lock first and measure the size while it is held.
if (nfsLock) {
tocFileLock(fd_, F_RDLCK);
}
src/fdb5/toc/TocHandler.cc:250
- Failing open is unsafe here:
statfscan transiently fail on an actual NFS mount (for example withEINTRorESTALE), after which the cached result permanently disables locking for this handler. Retry interruptible failures and otherwise throw or conservatively enable NFS handling; do not continue with unprotected appends when the filesystem cannot be identified.
if (::statfs(path.localPath(), &buf) != 0) {
return false; // fail-open: treat as local filesystem
tests/fdb/database/test_nfs.cc:43
- This overload is the sub-TOC/diagnostic constructor (
TocHandler.cc:183-190), so it setsisSubToc_ = true; every new synchronization branch is guarded by!isSubToc_. Combined with running only on the ordinary test filesystem, this round-trip never exercises NFS locking, cache refresh, or concurrent creators/appends. Add an NFS-backed or injectable multi-process/multi-thread test that constructs a main TOC and verifies complete records under contention.
fdb5::TocHandler writer(tocPath, fdb5::Key{});
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #339 +/- ##
===========================================
- Coverage 77.51% 77.44% -0.08%
===========================================
Files 411 412 +1
Lines 27553 27637 +84
Branches 2769 2778 +9
===========================================
+ Hits 21358 21403 +45
- Misses 6195 6234 +39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This PR ensures FDB's TOC append/read are safe on NFS mounts at runtime.
Problem
FDB's TOC (metadata) uses POSIX O_APPEND, which is a file status flag passed to the open() system call that forces every write() operation to move the file pointer to the end of the file immediately before writing.
While on a local filesystem, append is atomic at the OS kernel level, NFS's POSIX O_APPEND is non-atomic. Over NFS the client turns this into two RPCs:
GETATTR(fetch size) thenWRITEat that offset — so two clients can fetch the same EOF and overwrite each other.The only multi-writer contention in the TOC backend is the shared
tocfile.Solution
On NFS mounts, at runtime: auto-detect an NFS mount via
statfs, and only then applyfcntlbyte-range locking + NFS attribute-cache invalidation + directory sync. On local filesystems, the old behavior (no lock overhead) is unchanged.All NFS-specific behavior is gated on
onNFS() && !isSubToc_.New
TocHandler::onNFS(const PathName&)usesstatfsto detect an NFS mount(Linux
f_type == NFS_SUPER_MAGIC; macOSf_fstypename == "nfs"). Failure orunknown filesystem fails open to "local" (no locking).
openForAppend()takes a whole-file write lock — serialises appends andneutralises the non-atomic NFS
O_APPEND.openForRead()takes a whole-file read lock, correctly integrated with theTOC read-cache path: the lock is held across the copy into memory and released
when the descriptor is closed. This protects the
databaseKey()/openForRead()/readNextInternal()read path that previously observed tornheaders.
writeInitRecord()takes a read lock across the "is the TOC alreadyinitialised?" check and upgrades to a write lock before appending
TOC_INIT,closing the race between processes creating the same DB.
Before initialising a TOC, refresh the DB directory handle cache
(
eckit::StdDir) and flush the TOC file's attribute cache (anO_RDONLYopen/close), so a TOC created concurrently by another client is detected.
syncParentDirectory()is called after publishing namespace changes(TOC creation, subtoc pointer records, and on write-close) so directory-entry
visibility does not depend on cache expiry.
Subtoc pointer records are additionally
fdatasync'd before the directory sync.🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/fdb/pull-requests/PR-339