Skip to content

Close and reopen active recovery ledger chunks to preserve SMB write-caching - #8071

Closed
andpiccione wants to merge 5 commits into
microsoft:mainfrom
andpiccione:fix/rename-smb-lease-break-fix
Closed

Close and reopen active recovery ledger chunks to preserve SMB write-caching#8071
andpiccione wants to merge 5 commits into
microsoft:mainfrom
andpiccione:fix/rename-smb-lease-break-fix

Conversation

@andpiccione

Copy link
Copy Markdown
Member

When a node starts from a snapshot - a backup joining a network, or a node recovering - the host ledger enters recovery mode (init_as_backup() -> ledger->init(idx, recovery_start_idx)). Entries replicated/appended after the snapshot point are written to temporary ledger_X.recovery chunks. Once the service is observed OPEN, complete_recovery() -> LedgerFile::open() renames each ledger_X.recovery to its final ledger_X name while the active chunk's write handle is still open.

On Azure Files (SMB) with a CIFS client lacking the upstream fix 2c7d399e551c (in Linux 6.6.32, not backported to 6.1.y, so current C-VN2 at 6.1.x is affected), renaming a file with an open write handle breaks that handle's write-caching lease. The handle never regains write caching, so every later ledger write becomes a synchronous server round-trip (~2000x slower), which starves the ledger writer and can stall a joining or recovering node.

The proposed simple fix is to close the handle before the rename and reopen the renamed file so it gets a fresh lease, for chunks that can still be written:

  • Uncommitted chunks (!committed): fclose -> files::rename -> fopen("r+b"). These are still appended to, or can be truncated by a rollback and rewritten.
  • Committed chunks: unchanged (rename-only) - they are immutable and dropped from the active list immediately.

The predicate is committed (not completed) because a completed chunk can be un-completed by a post-recovery truncate and then written again. Failures are fail-stop (fclose/fopen checked, file nulled before throw, throwing rename), and the .recovery crash-marker semantics are unchanged. The change is confined to the single finalize-rename site (LedgerFile::open()), so it covers every path that produces .recovery chunks (join and recovery alike). No format, API, or platform-detection change.

I verified these changes on C-VN2, and the results are as expected. Slow writes are no longer appearing when starting a fresh network because the write degradation is gone.

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

This PR adjusts host-ledger recovery finalization so that writable (uncommitted) .recovery ledger chunks are closed before being renamed to their final names and then reopened, avoiding SMB/CIFS client lease/write-caching degradation after rename on some Linux kernels/filesystems. It also adds regression tests around recovery completion and documents the fix in the changelog.

Changes:

  • Update LedgerFile::open() to close → rename → reopen uncommitted recovery chunks when completing recovery.
  • Add host-ledger unit tests asserting recovery completion preserves open-FD counts and that post-recovery writes/truncation still work.
  • Add a changelog entry describing the Azure Files/SMB latency regression and the mitigation.

Custom instructions used:

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md
  • .github/instructions/changelog.instructions.md

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/host/ledger.h Implements close/rename/reopen flow for uncommitted recovery chunks during complete_recovery().
src/host/test/ledger.cpp Adds recovery-focused tests validating FD stability and continued write/truncate behavior after recovery completion.
CHANGELOG.md Documents the SMB/Azure Files recovery-ledger write-latency fix.

Comment thread src/host/ledger.h Outdated

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@andpiccione
andpiccione force-pushed the fix/rename-smb-lease-break-fix branch from aee831c to d6765a0 Compare July 16, 2026 03:55
@andpiccione
andpiccione marked this pull request as ready for review July 16, 2026 05:07
@andpiccione
andpiccione requested a review from a team as a code owner July 16, 2026 05:07
@achamayou

Copy link
Copy Markdown
Member

Replaced by #8072

@achamayou achamayou closed this Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bench-ab run-long-test Run Long Test job

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants