Check advisory locks in CopyFile

[filipnavara]

Fixes #42455, regression introduced in #37583.

[filipnavara]

The macOS test failure is relevant:

   System.IO.Tests.FileInfo_CopyTo_str_b.CopyFileWithData_MemberData(data: ['6', 0x0080, 'Ç', 'Ø', 0x0018, ...], readOnly: False) [FAIL]
      System.IO.IOException : The process cannot access the file '/var/folders/d9/xhjwf2ls6bd3nkq5gzp900g40000gy/T/FileInfo_CopyTo_str_b_jgu0izd5.gdp/CopyFileWithData_MemberData_115_0c3599b5' because it is being used by another process.
      Stack Trace:
        /_/src/System.Private.CoreLib/shared/System/IO/FileStream.Unix.cs(120,0): at System.IO.FileStream.Init(FileMode mode, FileShare share, String originalPath)
        /_/src/System.Private.CoreLib/shared/System/IO/FileStream.cs(252,0): at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options)
        /_/src/System.Private.CoreLib/shared/System/IO/FileStream.cs(175,0): at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share)
        /_/src/System.IO.FileSystem/src/System/IO/File.cs(259,0): at System.IO.File.OpenRead(String path)
        /_/src/System.IO.FileSystem/tests/File/Copy.cs(136,0): at System.IO.Tests.File_Copy_str_str.CopyFileWithData_MemberData(Char[] data, Boolean readOnly)

[filipnavara]

The Windows build failure is unrelated.

[stephentoub]

@filipnavara, thanks for working to fix this. We're duplicating more and more code now, though.

What would it look like if we reverted the previous change and still opened the FileStream in managed, getting all of these various checks "for free". If we didn't use clonefile, everything else would continue to operate the same. If we did use clonefile, then we would just end up overwriting the FileStream we opened in managed, which already did all of the appropriate access checks and the like, and on the clonefile path we'd just clobber it. It would of course mean an extra file getting opened/closed, but if that approach works it feels much cleaner.

Thoughts?

[filipnavara]

I am also unhappy about the code duplication but the solution is not easy.

It boils down to the fact that clonefile expects the destination file not to exist, while the other code path expects it to exists (or be created by the open call). We cannot blindly open the file using the FileStream logic and then fallback to clonefile because at that point the destination file would always exist and we would not even be able to delete it (short of closing the handle and creating a hacky code path on the managed side to account for that).

Any solution I came up with resulted in pretty bad trade offs in terms of file access API calls and/or number of P/Invokes.

[stephentoub]

we would not even be able to delete it

Why not? Having an open file descriptor to the file shouldn't prevent us from unlinking it.

My suggestion was basically to unlink and then clonefile. The only part of the previous change to managed code we'd need to keep would be changing the DllImport to also pass the destination path (in addition to the destination file descriptor).

[filipnavara]

Why not? Having an open file descriptor to the file shouldn't prevent us from unlinking it.

I can try it when I get home but I was quite convinced the clonefile would fail if we try to do that. Maybe I just come from Windows world and my expectation is incorrect.

[filipnavara]

Also, if we unlink it and clonefile fails then there's no way to fallback to the old code path.

[stephentoub]

Also, if we unlink it and clonefile fails then there's no way to fallback to the old code path.

That's... true. We could do something like:

• only attempt the clonefile if a non-null destination path is specified
• if the clonefile fails, return false back to managed indicating failure
• if the managed code sees a false return, it reopens the destination and passes null for a second attempt

Or something like that. Presumably clonefile failing will be relatively rare and the extra retry cost thus not very impactful.

I'm just trying to keep the code as simple as possible and avoid duplicating all of FileStream's logic in native. Let's explore a few options and see what turns out the best.

[filipnavara]

Presumably clonefile failing will be relatively rare and the extra retry cost thus not very impactful.

I think that's a wrong assumption. Depending on machine configuration and use case it could fail quite often, or not at all. It only works within the same APFS volume. Machines configured with HFS+ will fail every time. Copying on NTFS/FAT will fail every time, and so will copying between different volumes.

I know this sounds bleak and you may even ask whether clonefile is worth it at all? It helps couple of important scenarios. For me, specifically, it's MSBuild copying files stored on my main system volume. This happens really often during development.

I'm just trying to keep the code as simple as possible and avoid duplicating all of FileStream's logic in native. Let's explore a few options and see what turns out the best.

The return false to managed code and retry doesn't sound like an improvement to me. At that point you can structure it like this:

using (var src = new FileStream(sourceFullPath, FileMode.Open, FileAccess.Read, FileShare.Read, DefaultBufferSize, FileOptions.None))
{
    if (!CloneFile(src.SafeFileHandle, sourceFullPath, destFullPath, overwrite))
    {
        using (var dst = new FileStream(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, DefaultBufferSize, FileOptions.None))
        {
            Interop.CheckIo(Interop.Sys.CopyFile(src.SafeFileHandle, dst.SafeFileHandle));

The downside is more P/Invokes. The CloneFile would have to do FStat to check the existence of destination file, then do access check and unlink, and finally, call Interop.Sys.CloneFile. For non-macOS it could skip all that and just return false.

[lpereira]

Retrying close(2) in Linux is a race condition (please see the notes section in the man page for details).

[filipnavara]

Thanks for pointing that out. In the last iteration it may not be necessary.

[lpereira]

For Linux, it might be a better idea to use O_TMPFILE to create a temporary file while the copy is being performed, and when it's done, hardlink it with the new name. Then there's no need to use advisory file locking, which isn't really that useful.

[filipnavara]

The advisory locking is used by the managed FileStream so whatever is used it has to be done on both sides and match.

[stephentoub]

I would still like us to try hard to find ways around duplicating all this logic.

[stephentoub]

I know this sounds bleak and you may even ask whether clonefile is worth it at all?

I am asking that ;)

Can you please share the impact of the change on such a scenario? We had been using fcopyfile. What is the perf impact of using clonefile instead?

[filipnavara]

Can you please share the impact of the change on such a scenario? We had been using fcopyfile. What is the perf impact of using clonefile instead?

fcopyfile has no perf improvement over the fallback code whatsoever. I'll try to produce some perf numbers for clonefile. The anecdotal evidence is the following:

• Original Mono PR by Miguel: "On my Mac with APFS, copying a 5 gig file (Xcode) goes from taking
  7.2 seconds to copy on an SSD to 0.5 seconds."
• My MSBuild times on Mono were noticeably better. That may not directly apply to "dotnet build" because deps.json cuts down the number of file copies significantly.

[filipnavara]

The big difference is that clonefile (or reflink on Linux) basically just adds a reference so under ideal conditions it avoids copying the data altogether. The new file becomes a copy-on-write reference.

https://dev.to/robogeek/reflinks-vs-symlinks-vs-hard-links-and-how-they-can-help-machine-learning-projects-1cj4

[stephentoub]

basically just adds a reference

Why would it take half a second then to copy 5gb? Shouldn't it be almost instantaneous "copying" any file, regardless of size?

[filipnavara]

Shouldn't it be almost instantaneous "copying" any file, regardless of size?

Not my number so I cannot comment on it. It should be near instantaneous. I will do the test and post back numbers, no need to speculate :)

[baulig]

The return false to managed code and retry doesn't sound like an improvement to me. At that point you can structure it like this:

using (var src = new FileStream(sourceFullPath, FileMode.Open, FileAccess.Read, FileShare.Read, DefaultBufferSize, FileOptions.None))
{
    if (!CloneFile(src.SafeFileHandle, sourceFullPath, destFullPath, overwrite))
    {
        using (var dst = new FileStream(destFullPath, overwrite ? FileMode.Create : FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None, DefaultBufferSize, FileOptions.None))
        {
            Interop.CheckIo(Interop.Sys.CopyFile(src.SafeFileHandle, dst.SafeFileHandle));

I really dislike the two FileStream approach because you lose the ability of using the

int open(const char *pathname, int flags, mode_t mode);

variant to explicitly specify the file mode of the new file. The advantage of using this system call is that it does the right thing when you don't have permission to actually set the permissions to the ones of the source file. Whereas the fchmod() approach puts you into that awkward spot as to what to do when it fails due to insufficient permissions.

For instance, fchmod() will fail on Android as well in certain scenarios on all Unix-based systems, such as writing to a network share, setgid directories that you don't own etc.

[maryamariyan]

Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime.