Work around SMB lease loss when opening recovery ledger chunks - #8072
Merged
achamayou merged 5 commits intoJul 16, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors and hardens the recovery-ledger rename workaround for an SMB/CIFS client lease-loss bug (affecting Confidential Azure Container Instances), by centralising the optional close/rename/reopen sequence inside LedgerFile::rename() and validating the behaviour with stronger tests and benchmarks.
Changes:
- Add a
close_and_reopenoption toLedgerFile::rename()and enable it specifically for recovery finalisation (complete_recovery()), keeping ordinary commit behaviour unchanged. - Strengthen recovery unit tests to prove the active recovery chunk’s open file description is actually replaced (using a
flocksentinel). - Add a Picobench benchmark for rename-with/without-close+reopen across multiple file sizes, and run it in SNP CI jobs with CSV output in logs.
Custom instructions used:
.github/copilot-instructions.md.github/instructions/reviewing.instructions.md
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/host/ledger.h | Adds LedgerFile::close() helper and extends LedgerFile::rename() to optionally close+reopen for recovery finalisation. |
| src/host/test/ledger.cpp | Adds flock-based assertions to validate close+reopen behaviour during complete_recovery(). |
| src/host/test/ledger_bench.cpp | New Picobench benchmark comparing rename-only vs close+rename+reopen across several fixtures/sizes. |
| CMakeLists.txt | Registers the new ledger_bench picobench target. |
| CHANGELOG.md | Documents the temporary CIFS/SMB workaround and references the PR number. |
| .github/workflows/ci.yml | Runs ledger_bench in SNP CI jobs with verbose output for CSV results. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces #8071.
This keeps the close/rename/reopen workaround for the Linux CIFS lease bug affecting Confidential Azure Container Instances, while restructuring and validating it for easier removal once the platform kernel contains upstream fix
2c7d399e551c.Compared with #8071, this PR:
LedgerFile::rename(). Recovery finalization opts in, while the ordinary commit call site does not. Keeping the workaround behind one flag reduces duplication and makes it straightforward to remove after the kernel rollout.flocksentinel, which rename alone preserves but close/reopen releases; Close and reopen active recovery ledger chunks to preserve SMB write-caching #8071's descriptor-count and post-write checks do not distinguish those behaviors.Only uncommitted recovery chunks are closed and reopened. Committed recovery chunks remain immutable and rename-only. Close, rename, and reopen failures on the recovery-finalization path are fail-stop.
Verification
ledger_test: passedledger_bench: passedBenchmark results
Times are per rename;
overheadis close/reopen divided by rename-only.ACI measurements are from the CSV output in CI run
29490972468.