Add clonefile on Windows over ReFS support. #3790
Conversation
kazuki-ma
force-pushed the
kazuki-ma:clonefile_windows
branch
14 times, most recently
from
12cdd49
to
1b00158
kazuki-ma
changed the title
|
Overall, this looks good. There are a few issues I noted which we may want to fix. |
| // 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 |
bk2204
Aug 26, 2019
Member
Even if Windows names this with a leading underscore, we probably don't want to.
Even if Windows names this with a leading underscore, we probably don't want to.
| // 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 { |
bk2204
Aug 26, 2019
Member
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?
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" |
bk2204
Aug 26, 2019
Member
Do we need to add this to go.mod, or is it already brought in by the golang.org/x/sys package?
Do we need to add this to go.mod, or is it already brought in by the golang.org/x/sys package?
| } | ||
| defer os.Remove(src.Name()) | ||
|
|
||
| _, err = src.WriteString("TESTING") // Non-empty file is required for correct result. |
bk2204
Aug 26, 2019
Member
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.
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.
kazuki-ma
Aug 27, 2019
Author
Contributor
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.
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.
| &bytesReturned, | ||
| &overlapped) | ||
|
|
||
| tracerx.Printf("DeviceIoControl:%+v result:%+v\n", request, err) |
bk2204
Aug 26, 2019
Member
This also looks like perhaps it's debugging information.
This also looks like perhaps it's debugging information.
| package tools | ||
|
|
||
| import ( | ||
| "crypto/sha1" |
bk2204
Aug 26, 2019
Member
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.
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.
kazuki-ma
Aug 27, 2019
Author
Contributor
Got it. Thank you.
Got it. Thank you.
|
Updated. Please review again. |
|
This looks good. Thanks for the patch! |
|
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
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