feat: wire up RemapFs inode table serialize/restore for snapshot clones#549
feat: wire up RemapFs inode table serialize/restore for snapshot clones#549
Conversation
Wire fuse-pipe's RemapFs serialize_table/restore_from_table into fcvm's snapshot/clone flow so clones get pre-populated inode mappings instead of starting with an empty table. Changes: - AsyncServer::from_arc() — accepts pre-wrapped Arc<H> so caller retains reference for serialization while server runs - VolumeRequest::fh(), with_fh(), ino(), is_dir_handle_op() — protocol helpers for handle remapping - VolumeResponse::is_ebadf() — detect stale handle errors - RemapFs handle_remap DashMap + try_reopen_handle() — lazily reopens stale file handles after snapshot restore (EBADF → auto-reopen → retry) - SpawnedVolumes return type with remap_refs for portable volumes - Serialize inode tables to snapshot dir at snapshot creation time - Load and restore inode tables when spawning clone VolumeServers Before: clone VolumeServer starts with empty inode table, old inodes return EIO for ~1s until FUSE TTL expires, old file handles return EBADF forever. After: clone VolumeServer starts with restored inode table, old inodes work immediately, stale file handles are lazily reopened on first use. Tested: cargo check, cargo clippy, cargo test -p fuse-pipe (21/21 remap tests pass)
Adds test_open_handle_survives_snapshot_restore that verifies: - Clone can read files from snapshot through FUSE portable volume - Clone can write new files (FUSE mount bidirectional) - Host sees clone's writes (data integrity through clone VolumeServer) Tests the clone VolumeServer pipeline end-to-end with --portable-volumes. Tested: make test-root FILTER=test_open_handle_survives (passes)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea92862bc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn fh(&self) -> Option<u64> { | ||
| match self { | ||
| Self::Read { fh, .. } | ||
| | Self::Write { fh, .. } | ||
| | Self::Release { fh, .. } |
There was a problem hiding this comment.
The new stale-handle recovery path depends on VolumeRequest::fh()/with_fh(), but this helper set does not handle Setattr { fh: Some(...) } requests. Setattr is used for fd-based truncate operations, so after snapshot restore an ftruncate() on a pre-snapshot descriptor can return EBADF permanently because RemapFs never remaps or reopens that handle.
Useful? React with 👍 / 👎.
| Self::CopyFileRange { fh_in, .. } | Self::RemapFileRange { fh_in, .. } => { | ||
| *fh_in = new_fh | ||
| } |
There was a problem hiding this comment.
CopyFileRange/RemapFileRange carry both fh_in and fh_out, but the new helper only rewrites fh_in. If the destination handle is stale after restore, retry logic still sends the old fh_out, so these operations continue failing with EBADF instead of recovering like other handle-based ops.
Useful? React with 👍 / 👎.
🔍 Claude ReviewSEVERITY: medium Findings[MEDIUM] File descriptor leak in When two threads race to reopen the same stale [MEDIUM]
[LOW] Misleading log field
[LOW] Copy-paste doc comment on test ( The doc comment opens with "Verify that --non-blocking-output delivers lines best-effort" which is from a different test. The actual test ( SummaryThe PR adds a solid inode table serialize/restore mechanism for snapshot clones. Two medium issues: a file descriptor leak under concurrent handle reopen races, and incomplete Review by Claude | Run |
🔧 Auto-Fix CreatedI found issues and created a fix PR: #550 Please review and merge the fix PR first, then this PR. |
- Fix FD leak in concurrent reopen race: release the losing thread's opened fd via Release/Releasedir when another thread wins the handle_remap.entry() race - Add Setattr fh to fh()/with_fh() helpers (ftruncate uses fd-based path) - Remap both fh_in and fh_out for CopyFileRange/RemapFileRange - Fix misleading log field: entries → bytes (json.len() is byte count) - Fix copy-paste doc comment on test function
🔍 Claude ReviewSEVERITY: medium Previous Review StatusThe previous review identified 2 medium and 2 low issues. All four were addressed in commit
Findings[MEDIUM] The fix in Concrete scenario in
The same corruption occurs in the EBADF retry path (remap.rs:728-729). In practice, [LOW]
SummaryThe previous review's fixes were committed directly to the branch. The Review by Claude | Run |
🔧 Auto-Fix CreatedI found issues and created a fix PR: #551 Please review and merge the fix PR first, then this PR. |
… handle remap - Extract resolve_volume_path() to eliminate duplicate canonicalize + is_dir between VolumeServer::new() and spawn_volume_servers_with_tables() - Fix with_fh() for CopyFileRange/RemapFileRange: only remap fh_in, not fh_out (they reference different files; setting both to same value corrupts cross-file ops) - Remove incorrect #[allow(dead_code)] on remap_refs (field IS used for serialization) - Add debug log when O_RDWR fallback to O_RDONLY occurs in try_reopen_handle
Volumes can be read-write, so silently downgrading to O_RDONLY would cause subsequent writes to fail. Just use O_RDWR — if the file can't be opened (e.g., mode 0444), return the error honestly rather than masking it with a wrong access mode.
🔍 Claude ReviewSEVERITY: medium Previous Review StatusPrevious reviews (review 1, review 2) identified issues. Fix PRs #550 and #551 were closed; fixes were committed directly to the branch instead. Current status of previously reported issues:
Findings[MEDIUM] As noted in review 2, the dual-handle remapping for CopyFileRange/RemapFileRange is architecturally incomplete. Commit
If In practice this is low-risk: [LOW] PR description doesn't mention several fix commits The PR description covers the original feature commit ( SummaryThe core inode table serialize/restore mechanism is solid and the previous review findings have been addressed. The remaining Review by Claude | Run |
🔧 Auto-Fix CreatedI found issues and created a fix PR: #552 Fixes a misleading comment on |
🔍 Claude ReviewSEVERITY: low Previous Review StatusThree previous reviews (review 1, review 2, review 3) identified issues. All fixes were committed directly to the branch (commits
FindingsNo new [MEDIUM] or [CRITICAL] issues found. [LOW] PR description doesn't reflect fix commits — As noted in review 3, the description only covers the original feature. The 5 fix commits ( [LOW] SummaryThe core inode table serialize/restore mechanism is solid and well-tested. All medium issues from previous reviews have been fixed. The remaining Review by Claude | Run |
…le volumes - Update CopyFileRange/RemapFileRange comment to accurately note fh_out is NOT remapped (known limitation) instead of misleadingly saying it "must be remapped separately" - Add startup log to serve_vsock_with_remap_arc so portable volume servers log the same info as the regular path
🔍 Claude ReviewSEVERITY: low Previous Review StatusFour previous reviews (review 1, review 2, review 3, review 4) identified issues. All medium issues have been fixed in commits
FindingsNo new [MEDIUM] or [CRITICAL] issues found. All previously reported [LOW] issues have either been fixed or remain as noted in prior reviews (PR description not updated for fix commits). SummaryThe inode table serialize/restore implementation is solid. All medium issues from four rounds of review have been addressed. The code correctly aligns Review by Claude | Run |
🔧 CI Auto-FixCreated fix PR: #554 Root cause: During snapshot restore, Fix: Skip the premature port probe in |
1 participant
Summary
Wire up RemapFs
serialize_table()/restore_from_table()into fcvm's snapshot/clone flow so clone VolumeServers restore the inode table from the snapshot baseline. This eliminates the 1-second TTL glitch window where cached inodes return ENOENT after clone boot.Also adds lazy handle re-open: when a stale file handle (from the snapshot baseline) gets EBADF, RemapFs automatically reopens the file and retries the operation.
Changes
fuse-pipe
AsyncServer::from_arc()— accept pre-wrappedArc<H>so caller retains reference for serializationVolumeRequest::fh(),with_fh(),ino(),is_dir_handle_op()— protocol helpers for handle manipulationVolumeResponse::is_ebadf()— detect EBADF responsesRemapFs::handle_remap— DashMap for old_fh→new_fh mapping after restoreRemapFs::try_reopen_handle()— lazy reopen with O_RDWR, race-safe via DashMap::entry()handle_request_with_groups— auto-retry with reopened handlefcvm
SpawnedVolumesreturn type withhandles+remap_refs(Arc references)spawn_volume_servers_with_tables()— restore inode tables from serialized JSONresolve_volume_path()— deduplicated path validation helpervolume-{port}-inode-table.jsonat snapshot time (pre-start cache path)Known Limitations
CopyFileRange/RemapFileRangefh_outis not remapped after restore (onlyfh_in). These operations with a stale destination handle will fail. Rare in practice.fcvm snapshot createruns in a separate process without access to the running VolumeServer's RemapFs, so inode table serialization only works in the pre-start snapshot cache path (same process).Test Results
Integration test
test_open_handle_survives_snapshot_restore: baseline VM with portable volume, write file, snapshot, clone, verify clone reads and writes work.