Skip to content

FDB-738: NFS support - #339

Open
mcakircali wants to merge 9 commits into
developfrom
feature/FDB-738-nfs-support
Open

FDB-738: NFS support#339
mcakircali wants to merge 9 commits into
developfrom
feature/FDB-738-nfs-support

Conversation

@mcakircali

@mcakircali mcakircali commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

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) then WRITE at 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 toc file.

Solution

On NFS mounts, at runtime: auto-detect an NFS mount via statfs, and only then apply fcntl byte-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&) uses statfs to detect an NFS mount
    (Linux f_type == NFS_SUPER_MAGIC; macOS f_fstypename == "nfs"). Failure or
    unknown filesystem fails open to "local" (no locking).

  • openForAppend() takes a whole-file write lock — serialises appends and
    neutralises the non-atomic NFS O_APPEND.

  • openForRead() takes a whole-file read lock, correctly integrated with the
    TOC 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 torn
    headers.

  • writeInitRecord() takes a read lock across the "is the TOC already
    initialised?" 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 (an O_RDONLY
    open/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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_SETLKW record 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 receive EDEADLK while 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 tocSize at 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: statfs can transiently fail on an actual NFS mount (for example with EINTR or ESTALE), 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 sets isSubToc_ = 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.

Comment thread src/fdb5/toc/TocHandler.cc Outdated
Comment thread tests/fdb/database/test_nfs.cc Outdated
@mcakircali
mcakircali marked this pull request as ready for review August 31, 2026 09:19
@codecov-commenter

codecov-commenter commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 55.95238% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.44%. Comparing base (ea5c9d3) to head (990e2fc).

Files with missing lines Patch % Lines
src/fdb5/toc/TocHandler.cc 32.72% 37 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

3 participants