Add clonefile on Windows over ReFS support. #3790
Conversation
kazuki-ma
force-pushed
the
clonefile_windows
branch
14 times, most recently
from
12cdd49
to
1b00158
Compare
kazuki-ma
changed the title
kazuki-ma
force-pushed
the
clonefile_windows
branch
from
1b00158
to
b6884d3
Compare
tools/util_windows.go
Outdated
| // Instructs the file system to copy a range of file bytes on behalf of an application. | ||
| // | ||
| // https://docs.microsoft.com/windows/win32/api/winioctl/ni-winioctl-fsctl_duplicate_extents_to_file | ||
| const _FSCTL_DUPLICATE_EXTENTS_TO_FILE = 623428 |
There was a problem hiding this comment.
Even if Windows names this with a leading underscore, we probably don't want to.
tools/util_windows.go
Outdated
| // Contains parameters for the FSCTL_DUPLICATE_EXTENTS control code that performs the Block Cloning operation. | ||
| // | ||
| // https://docs.microsoft.com/windows/win32/api/winioctl/ns-winioctl-duplicate_extents_data | ||
| type _DUPLICATE_EXTENTS_DATA struct { |
There was a problem hiding this comment.
Same issue about the leading underscore. Since this is not supposed to be accessible outside of this package, we probably want to call it duplicateExtentsData.
Is there a way for us to get the definition of this struct and constant from a library so that we don't have to implement it ourselves?
| "unsafe" | ||
|
|
||
| "github.com/rubyist/tracerx" | ||
| "golang.org/x/sys/windows" |
There was a problem hiding this comment.
Do we need to add this to go.mod, or is it already brought in by the golang.org/x/sys package?
There was a problem hiding this comment.
We don't add entry to go.mod. It's part of golang.org/x/sys.
tools/util_windows.go
Outdated
| } | ||
| defer os.Remove(src.Name()) | ||
|
|
||
| _, err = src.WriteString("TESTING") // Non-empty file is required for correct result. |
There was a problem hiding this comment.
Is a non-empty file required just for this case or for the function itself to work correctly? If the latter, we probably want to check for a non-zero size below.
There was a problem hiding this comment.
Calling FSCTL_DUPLICATE_EXTENTS_TO_FILE to empty file is always success even filesystem don't support it.
To check filesystem support FSCTL_DUPLICATE_EXTENTS_TO_FILE or not, we have to use non-empty file.
I'll write those as comment.
tools/util_windows.go
Outdated
| &bytesReturned, | ||
| &overlapped) | ||
|
|
||
| tracerx.Printf("DeviceIoControl:%+v result:%+v\n", request, err) |
There was a problem hiding this comment.
This also looks like perhaps it's debugging information.
tools/util_windows_test.go
Outdated
| package tools | ||
|
|
||
| import ( | ||
| "crypto/sha1" |
There was a problem hiding this comment.
If we're going to use a hash for comparing two files, let's use SHA-256. SHA-1 is considered weak and we'd like to use less of it in our codebase, and we already use SHA-256 in a lot of places.
There was a problem hiding this comment.
Got it. Thank you.
kazuki-ma
force-pushed
the
clonefile_windows
branch
from
b6884d3
to
6c7aa06
Compare
kazuki-ma
force-pushed
the
clonefile_windows
branch
from
6c7aa06
to
e55bc4c
Compare
|
Updated. Please review again. |
|
Thank you very much for reviewing. |
Windows Server has a reflink feature built onto the dedup-capable ReFS. This is completely irrelevant for client users, but it would still be a good thing to add given how Microsoft loves Nodejs now. The FICLONE_FORCE mode is likely to fail since you have to pay for the server thing to get it to work. Remove the check if the CI is capable. (We still really need someone to test it.) This PR is based on the git-lfs implementation. Ref: git-lfs/git-lfs#3790
In commit e861b79 of git-lfs#5595 the Windows variant of our tools.CheckCloneFileSupported() function was updated to address the problem that it could fail to remove the temporary files it creates to test whether the "file clone" operation (as used by the "git lfs dedup" command) is supported in a given directory. This problem could occur on Windows because we did not close all of the temporary file's open descriptors, and so our deferred call to os.Remove() might not succeed. In a similar vein, the Windows variant of our tools.CloneFileByPath() function opens both the source and destination file paths provided for a "clone" operation but does not close the resultant file descriptors, so we now add deferred calls to ensure we always close both files. (This minor issue dates from the introduction in commit e55bc4c of the support for the "file clone" operation on ReFS in PR git-lfs#3790.)
In commit e861b79 of git-lfs#5595 the Windows variant of our tools.CheckCloneFileSupported() function was updated to address the problem that it could fail to remove the temporary files it creates to test whether the "file clone" operation (as used by the "git lfs dedup" command) is supported in a given directory. This problem could occur on Windows because we did not close all of the temporary file's open descriptors, and so our deferred call to os.Remove() might not succeed. In a similar vein, the Windows variant of our tools.CloneFileByPath() function opens both the source and destination file paths provided for a "clone" operation but does not close the resultant file descriptors, so we now add deferred calls to ensure we always close both files. (This minor issue dates from the introduction in commit e55bc4c of the support for the "file clone" operation on ReFS in PR git-lfs#3790.)
ReFS (wikipedia) is filesystem on Windows OS and it's support copy-on-write / file cloning since v3.2.
ReFS v3.2 is available since Windows Server 2016, Windows Server 2019 and Windows 10 also supports format & read-write.
I think many asset heavy product CI system and developer start using ReFS if git-lfs support dedup on it.
Reference:
This closes #3791