Skip to content

Fix atomicity and durability + some small fixes#366

Open
bigerl wants to merge 17 commits into
llnl:developfrom
dice-group:feature/durable-file-handling
Open

Fix atomicity and durability + some small fixes#366
bigerl wants to merge 17 commits into
llnl:developfrom
dice-group:feature/durable-file-handling

Conversation

@bigerl

@bigerl bigerl commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Hi @KIwabuchi

it has been a while since we have been active in metall. We ran lately into some problems with persistence and durability. Here the PR to fix them. @liss-h (See dice-group#17) already reviewed it from our side.

This PR fixes the properly-closed mark, introduces full error handling around files and fixes some minor other issues found along the way. The changes are guarded by regression tests.

Problems with the previous implementation

  • close() ignored errors. It discarded the results of the metadata serialization and the segment sync and created the mark unconditionally. A failed write (ENOSPC, EIO) still produced a datastore that consistent() reported as good.
  • The mark itself was fsynced, but nothing it certified was. Management metadata was written unsynced and in place. After a power loss the mark could exist while the metadata was missing or torn. A crash during close destroyed the previous valid metadata.
  • The mark also served as the lock, and a read-write open deleted it. The removal was not fsynced, so after a power loss a crashed writer's result could be observed as properly closed. Also, once the file was gone there was nothing left to lock: concurrent create() and remove() could clear the directory of a datastore that another process had open.
  • same holds for snapshot() and copy()

New design

Two files with separate roles replace the overloaded mark:

  • A persistent lockfile (<base>/mds_lock). It is never deleted and carries the flock: exclusive for writers, shared for readers. create(), open(), remove(), snapshot(), and copy() all take it, so destroying or replacing an open datastore is refused instead of executed. Existing datastores get the lockfile on first open.
  • Independent properly-closed mark. A read-write open removes it and makes the removal durable (directory fsync) before any data is modified. It is recreated only by an explicit step at the end of a fully successful close. The destructor never writes it, because the state of the datastore is still unknown at that time.

Writes acked by the close mark are now durable and atomic:

  • All management files are written through write_file_atomically that does: temp file in the same directory, fsync, rename, parent directory fsync to make the operation atomic and durable.
  • close() now returns a bool that is only true if all steps were successful.
  • A failed open tries to restore the consumed mark, since a failed open modifies no data.
  • Snapshots and copies are built under <destination>/.tmp_datastore, the whole tree is fsynced, the mark is written inside, and the result is published with a single rename plus directory fsync. A half-written snapshot is never visible under the final path.

Misc Fixes

  • mdtl::munmap(..., call_msync = true) never unmapped. It now syncs and unmaps. This has been there since the initial commit.
  • The per-block fsync after msync(MS_SYNC) is skipped on Linux
  • extend_file_size_manually wrote an uninitialized buffer instead of zeros and ignored pwrite results.
  • fixed sanitizer issues in the tests

Behavior changes

  • manager_kernel::close() returns bool instead of void.
  • A close that cannot persist its data leaves no mark; the next open fails. Previously the datastore was silently marked good while corrupted.
  • create(), remove(), snapshot(), and copy() fail if the datastore or the destination is open elsewhere. Previously they proceeded and could scramble data.
  • New file next to the datastore root: mds_lock.

Tests

@liss-h
liss-h force-pushed the feature/durable-file-handling branch from e757931 to b5df68f Compare July 24, 2026 09:42
@KIwabuchi
KIwabuchi self-requested a review July 25, 2026 23:27
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