refactor(reflinking): add windows ReFS filesystem support#1576
Merged
refactor(reflinking): add windows ReFS filesystem support#1576
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Windows ReFS-based reflink support (probe + clone), Windows-specific tests, updates unsupported-platform stubs/tests to treat Windows as runtime-detected, documents Windows ReFS requirements and fallback behavior, and downgrades logging for reflink-unsupported errors in cross-seed service. Changes
Sequence DiagramsequenceDiagram
rect rgba(200,200,255,0.5)
participant Caller
end
rect rgba(200,255,200,0.5)
participant SupportsReflink
participant VolumeValidation
end
rect rgba(255,200,200,0.5)
participant CloneFile
participant DuplicateExtent
participant CopyTail
end
Caller->>SupportsReflink: SupportsReflink(dir)
SupportsReflink->>VolumeValidation: getVolumeRoot / getFilesystemName / getClusterSize
alt ReFS detected
SupportsReflink->>SupportsReflink: create probe files
SupportsReflink->>CloneFile: cloneFile(probeSrc, probeDst)
CloneFile->>VolumeValidation: ensureSameVolume / ensureRefsVolume
loop per-chunk
CloneFile->>DuplicateExtent: duplicateExtent(chunk)
DuplicateExtent-->>CloneFile: OK/Error
end
CloneFile->>CopyTail: copyFileTail(if needed)
CopyTail-->>CloneFile: done
CloneFile-->>SupportsReflink: success
SupportsReflink-->>Caller: (true, "")
else Non-ReFS or error
SupportsReflink-->>Caller: (false, reason)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/reflinktree/reflink_windows.go (1)
281-299: Consider pooling the copy buffer for repeated operations.The 1MB buffer is allocated on every tail copy. If
cloneFileis called frequently (e.g., many files in a reflink tree), pooling viasync.Poolcould reduce GC pressure. This is a minor optimization and not blocking.♻️ Optional: Pool the copy buffer
var copyBufferPool = sync.Pool{ New: func() any { return make([]byte, copyBufferSize) }, } func copyFileTail(srcFile, dstFile *os.File, offset, length int64) error { // ... seek logic unchanged ... buffer := copyBufferPool.Get().([]byte) defer copyBufferPool.Put(buffer) copied, err := io.CopyBuffer(dstFile, io.LimitReader(srcFile, length), buffer) // ... rest unchanged ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reflinktree/reflink_windows.go` around lines 281 - 299, The copyFileTail function allocates a 1MB buffer on each call which can increase GC pressure when cloning many files; introduce a package-level sync.Pool (e.g., copyBufferPool) whose New returns make([]byte, copyBufferSize) and replace the local make call with buffer := copyBufferPool.Get().([]byte) and defer copyBufferPool.Put(buffer) so io.CopyBuffer reuses buffers; keep the existing seek and copy logic and ensure the pool is declared alongside copyBufferSize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/reflinktree/reflink_windows.go`:
- Around line 281-299: The copyFileTail function allocates a 1MB buffer on each
call which can increase GC pressure when cloning many files; introduce a
package-level sync.Pool (e.g., copyBufferPool) whose New returns make([]byte,
copyBufferSize) and replace the local make call with buffer :=
copyBufferPool.Get().([]byte) and defer copyBufferPool.Put(buffer) so
io.CopyBuffer reuses buffers; keep the existing seek and copy logic and ensure
the pool is declared alongside copyBufferSize.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ea89af54-f0c3-4467-a9e5-2dc91a2112fe
📒 Files selected for processing (5)
documentation/docs/features/cross-seed/hardlink-mode.mdpkg/reflinktree/reflink_test.gopkg/reflinktree/reflink_unsupported.gopkg/reflinktree/reflink_windows.gopkg/reflinktree/reflink_windows_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/reflinktree/reflink_windows.go`:
- Around line 84-107: Open the source file before calling os.Stat so the
metadata and subsequent clone operate on the same handle: replace the current
os.Stat(src) call with opening the file first (os.Open) to produce srcFile, then
call srcFile.Stat() to populate srcInfo; keep the existing error handling
semantics (wrap errors with fmt.Errorf like "open source: %w" and "stat source:
%w"), ensure srcFile is closed appropriately, and leave the later calls to
ensureSameVolume and ensureRefsVolume unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9cb5b352-7be8-4bf6-99e7-6e8e80c8f891
📒 Files selected for processing (1)
pkg/reflinktree/reflink_windows.go
Make SupportsReflink write a probe file sized to at least one ReFS cluster plus a tail byte, so the probe cannot pass via tail-copy alone. This ensures the Windows reflink capability check actually invokes FSCTL_DUPLICATE_EXTENTS_TO_FILE instead of returning a false positive for small probe files. Also tighten the Windows probe test to assert both the duplicate-extent call and tail-copy path are exercised.
Resolve Windows source symlinks before cloning so reflinks operate on the real file path instead of the reparse point. Detect sparse ReFS source files, mark destination files sparse, and resize them with SetFilePointerEx/SetEndOfFile before duplicating extents. Add Windows test coverage for symlink resolution, sparse destination preparation, and related error handling.
…tination parents Improve Windows reflink behavior for folder/mountpoint edge cases and clearer fallback semantics. - resolve destination parent path before same-volume/filesystem checks in `cloneFile` so junction/symlink/mountpoint targets are validated against the actual backing path - map `windows.ERROR_NOT_SUPPORTED` from duplicate extents to `ErrReflinkUnsupported` for consistent unsupported-capability handling - downgrade cross-seed reflink-create log severity to `warn` when the error is `ErrReflinkUnsupported` (expected fallback scenario) - add Windows reflink tests for: - resolved destination-parent volume checks - unsupported duplicate-extent error mapping
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/reflinktree/reflink_windows_test.go (1)
35-39: Consider extracting duplicatedcloneCallstruct.The
cloneCallstruct is defined identically in two tests. While acceptable in test code, extracting it to file scope would reduce duplication.♻️ Suggested refactor
+type cloneCall struct { + sourceOffset int64 + targetOffset int64 + byteCount int64 +} + func TestCloneFile_UsesDuplicateExtentsAndCopiesTail(t *testing.T) { // ... - type cloneCall struct { - sourceOffset int64 - targetOffset int64 - byteCount int64 - } var cloneCalls []cloneCallApply similarly in
TestCloneFile_ResolvesSourceSymlinkBeforeClone.Also applies to: 514-518
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reflinktree/reflink_windows_test.go` around lines 35 - 39, Extract the duplicated cloneCall struct into a single package-level (file-scope) declaration so both tests reuse it: remove the local type definitions inside TestCloneFile_ResolvesSourceSymlinkBeforeClone and the other test, add one top-level "type cloneCall struct { sourceOffset int64; targetOffset int64; byteCount int64 }" in reflink_windows_test.go (above the tests), and update the tests to reference that single cloneCall type; ensure the name and field types remain identical so existing test code compiles without other changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/services/crossseed/service.go`:
- Around line 11084-11088: The current logic downgrades logEvent to log.Warn
whenever errors.Is(err, reflinktree.ErrReflinkUnsupported) is true, but that
also matches wrapped errors (e.g., when rollback also failed); change the guard
so you only downgrade when ErrReflinkUnsupported is the sole/terminal error:
check that errors.Is(err, reflinktree.ErrReflinkUnsupported) AND
errors.Unwrap(err) == nil (no further wrapped error) before setting logEvent =
log.Warn(); otherwise keep logEvent as log.Error() so rollback failures stay
logged as errors. Ensure you reference the existing variables err, logEvent and
the constant reflinktree.ErrReflinkUnsupported in the updated condition.
---
Nitpick comments:
In `@pkg/reflinktree/reflink_windows_test.go`:
- Around line 35-39: Extract the duplicated cloneCall struct into a single
package-level (file-scope) declaration so both tests reuse it: remove the local
type definitions inside TestCloneFile_ResolvesSourceSymlinkBeforeClone and the
other test, add one top-level "type cloneCall struct { sourceOffset int64;
targetOffset int64; byteCount int64 }" in reflink_windows_test.go (above the
tests), and update the tests to reference that single cloneCall type; ensure the
name and field types remain identical so existing test code compiles without
other changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 21370664-ed37-4205-b466-4bf5ceaf4467
📒 Files selected for processing (3)
internal/services/crossseed/service.gopkg/reflinktree/reflink_windows.gopkg/reflinktree/reflink_windows_test.go
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
2 participants
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 by CodeRabbit
New Features
Documentation
Bug Fixes / Platform Handling
Tests