Fix FS moves across Unix mounts #13044

rjmholt · 2020-06-27T18:34:45Z

PR Summary

Currently Move-Item does not support cross-mount moves on Unix, because it doesn't check properly whether given paths lie on the same filesystem.

This PR fixes the check to identify when paths lie on different devices on Unix, so that the move logic can properly copy and delete rather than trying to call the rename API.

NOTE I'm not sure how we can test this, but I would love to if possible.

PR Context

PR Checklist

rjmholt · 2020-06-27T18:35:35Z

Ugh Visual Studio applied some autoformatting...

KalleOlaviNiemitalo · 2020-06-27T19:38:33Z

Would it be reasonable to try the rename call and recognize the EXDEV error? rename(2) says rename will fail with that if you try to rename between two mount points of the same file system; in which case, I think st_dev will be equal.

rjmholt · 2020-06-28T05:49:00Z

Would it be reasonable to try the rename call and recognize the EXDEV error?

We don’t call that directly, it’s a couple of layers down within .NET.

It turns out there’s a similar check done for Windows already with the Move-Item call and I think the best thing is to do something similar here.

PowerShell already has its own stat call, so basically we use that to check whether the device ID of the source and destination are the same.

Implementing that conforms with the existing logic and minimises the code changes required to fix this bug (which we may not be able to test).

KalleOlaviNiemitalo · 2020-06-28T08:27:35Z

A comment in GNU coreutils notes that comparing st_dev to decide whether to rename is not reliable with NFS.

rjmholt · 2020-06-28T14:53:27Z

A comment in GNU coreutils notes that comparing st_dev to decide whether to rename is not reliable with NFS.

Hmmm, yeah that's good information.

I'm also interested by the fact that .NET makes file copying work across drives but not directory copying, making me feel that this might be unintended behaviour. Since their layer is better placed to deal with this, I'll follow up with them before we proceed.

rjmholt · 2020-06-28T18:04:41Z

Given the discussion in dotnet/runtime#31149, I don't think .NET is about to fix this, so continuing with a workaround.

iSazonov · 2020-06-29T04:50:23Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+#else
+            // On Windows, being able to rename vs copy/delete a file
+            // is just a question of the drive
+            if (IsSameWindowsVolume(directory.FullName, destinationPath))


.Net uses MOVEFILE_COPY_ALLOWED flag for MoveFileEx() on Windows.
So I think we can remove the check at all on all platforms but only check catch (IOException e) when (e.HResult == UNIX_ERRNO_EXDEV) on Unix-s.

Another thought is that we can a concern about performance (rename on the same volume is very fast but cross-volume one is a coping that can take many time.) and then we need to add a new parameter Move-Item -AllowCrossVolumeCoping to explicitly allow the behavior for all platforms. It is my preference. But how we will detect cross-volume on Windows if root path can be the same?

.Net uses MOVEFILE_COPY_ALLOWED flag for MoveFileEx() on Windows.

That's very interesting. I did a small experiment with PS 7.1 and got this result though:

C:\Users\Robert Holt\Documents\Dev\Microsoft\PSScriptAnalyzer [implement-script-rules +4 ~9 -0 !] > gi .\tools_copy\ | Sv t C:\Users\Robert Holt\Documents\Dev\Microsoft\PSScriptAnalyzer [implement-script-rules +4 ~9 -0 !] > $t.MoveTo('D:\tools') MethodInvocationException: Exception calling "MoveTo" with "1" argument(s): "Source and destination path must have identical roots. Move will not work across volumes." C:\Users\Robert Holt\Documents\Dev\Microsoft\PSScriptAnalyzer [implement-script-rules +4 ~9 -0 !] > get-error Exception : Type : System.Management.Automation.MethodInvocationException ErrorRecord : Exception : Type : System.Management.Automation.ParentContainsErrorRecordException Message : Exception calling "MoveTo" with "1" argument(s): "Source and destination path must have identical roots. Move will not work across volumes." HResult : -2146233087 CategoryInfo : NotSpecified: (:) [], ParentContainsErrorRecordException FullyQualifiedErrorId : IOException InvocationInfo : ScriptLineNumber : 1 OffsetInLine : 1 HistoryId : -1 Line : $t.MoveTo('D:\tools') PositionMessage : At line:1 char:1 + $t.MoveTo('D:\tools') + ~~~~~~~~~~~~~~~~~~~~~ CommandOrigin : Internal ScriptStackTrace : at <ScriptBlock>, <No file>: line 1 TargetSite : Name : ConvertToMethodInvocationException DeclaringType : System.Management.Automation.ExceptionHandlingOps, System.Management.Automation, Version=7.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35 MemberType : Method Module : System.Management.Automation.dll StackTrace : at System.Management.Automation.ExceptionHandlingOps.ConvertToMethodInvocationException(Exception exception, Type typeToThrow, String methodName, Int32 numArgs, MemberInfo memberInfo) at CallSite.Target(Closure , CallSite , Object , String ) at System.Dynamic.UpdateDelegates.UpdateAndExecute2[T0,T1,TRet](CallSite site, T0 arg0, T1 arg1) at System.Management.Automation.Interpreter.DynamicInstruction`3.Run(InterpretedFrame frame) at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame) Message : Exception calling "MoveTo" with "1" argument(s): "Source and destination path must have identical roots. Move will not work across volumes." Data : System.Collections.ListDictionaryInternal InnerException : Type : System.IO.IOException TargetSite : Name : MoveTo DeclaringType : System.IO.DirectoryInfo MemberType : Method Module : System.IO.FileSystem.dll StackTrace : at System.IO.DirectoryInfo.MoveTo(String destDirName) at CallSite.Target(Closure , CallSite , Object , String ) Message : Source and destination path must have identical roots. Move will not work across volumes. Source : System.IO.FileSystem HResult : -2146232800 Source : System.Management.Automation HResult : -2146233087 CategoryInfo : NotSpecified: (:) [], MethodInvocationException FullyQualifiedErrorId : IOException InvocationInfo : ScriptLineNumber : 1 OffsetInLine : 1 HistoryId : -1 Line : $t.MoveTo('D:\tools') PositionMessage : At line:1 char:1 + $t.MoveTo('D:\tools') + ~~~~~~~~~~~~~~~~~~~~~ CommandOrigin : Internal ScriptStackTrace : at <ScriptBlock>, <No file>: line 1

Another thought is that we can a concern about performance

Personally I think user expectation will be for Move-Item to transparently deal with cross-volume moves without needing to specify a flag. mv doesn't require one and nor does .NET on Windows. Throwing an error is unhelpful and technically not performant -- it performs no useful work. I think the best way to get performance here is probably to optimise your mount layout.

If I remember correctly Windows Explorer on XP warned about cross-volume coping but later (Windows 7?) the behavior was changed. Perhaps we could follow this.
Although we should take into account:

Performance - coping will be slow which may be unexpected for the user in an interactive session or it is amazing to see a script slowdown.

Lost security descriptor - after coping permissions are inherited from new root.

Formally it is a breaking change.

Reliability - it is not an atom operation.

"user expectation will be for Move-Item to transparently deal with cross-volume moves without needing to specify a flag"

I can't speak for all users but I can confirm that this is what I would expect as a user. :-)

Regarding *nix specific flags, a bit more context of my scenario....The real value of running PowerShell (my opinion of course) on Linux is no longer having to maintain two separate shell script versions (bash and command prompt).

If we start setting a precedence of having to use special flags for Linux or Microsoft or even differently named cmdlets, (Move-LinuxFile Move-MSFile) then as consumers, we likely have to add in if ($Linux) {#Linux ways} else {#Microsoft ways} at which point we might as maintain separate shell scripts.

For the record, throwing errors should always be avoided from a user frustration standpoint but I would be ok if an error was displayed by default and the CopyAndDelete would happen when -Force is provided.

With that said I like @rjmholt's solution and from one dev to another, I appreciate everyone's effort on this one!

@rjmholt Can you remove the check on Windows and check cross-volume move? I expect it is work.
(Although the question is how this works for reparse points like OneDrive but it seems we don't detect this whatever).
Or you already did? If so it looks like a bug in .Net :-(

KalleOlaviNiemitalo · 2020-06-29T05:51:36Z

Are there already tests for these scenarios:

Move a directory to a subdirectory of itself via a symbolic link
1. a/b/c/d is a file.
2. e is a symbolic link to a/b.
3. Move-Item a e/f must fail because that would move a to a/b/f, and the directory cannot be moved to a subdirectory of itself.
4. After Move-Item has failed, a/b/c/d must still exist.
Move a mount point
1. a is a directory with a file system mounted on it. Thus, the directory cannot be deleted before the file system is unmounted.
2. a/b is a file.
3. c is an empty directory.
4. Move-Item a c/d may succeed or fail.
5. After Move-Item has succeeded, c/d/b must exist.
6. After Move-Item has failed, either c/d/b or a/b must exist.
Another program used to have a bug where it detected the inability to delete a and incorrectly tried to roll back by deleting c/d as well, losing all the files.

rjmholt · 2020-06-29T05:58:04Z

Are there already tests for these scenarios:

I'm not familiar with the Move-Item tests, but I suspect all the tests we have are in the file I've added to here -- so if there's no test for a given scenario in there, there may be no test.

I'd prefer to keep discussion in this PR scoped, but your contribution of either an issue enumerating those tests or test code itself would be very welcome.

rjmholt · 2020-06-29T05:59:09Z

Move a mount point

I'm not currently sure how we would reliably create mount points in our testing infrastructure, especially on the various platforms. I may need help from @adityapatwardhan and @JamesWTruher for that.

KalleOlaviNiemitalo · 2020-06-29T06:56:54Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+#if UNIX
+        // This is the errno returned by the rename() syscall
+        // when an item is attempted to be renamed across filesystem mount boundaries.
+        private const int UNIX_ERRNO_EXDEV = 18;


18 appears to be correct for Linux, macOS, and FreeBSD.

We should check the error code is 18 for all the variants we support especially Raspbian and Alpine, which could have it a bit different.

Confirmed on RPi:

pi@daedalus:~ $ errno -l | grep EXDEV EXDEV 18 Invalid cross-device link

Confirmed on Alpine:

/ # errno -l | grep EXDEV EXDEV 18 Cross-device link

src/System.Management.Automation/namespaces/FileSystemProvider.cs

ghost · 2020-08-17T22:19:55Z

🎉v7.1.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

rjmholt requested a review from anmenaga as a code owner June 27, 2020 18:34

ghost assigned daxian-dbw Jun 27, 2020

rjmholt requested review from JamesWTruher and SteveL-MSFT June 27, 2020 18:35

rjmholt force-pushed the fix-move-unix-mounts branch from 9fec65a to b612197 Compare June 27, 2020 18:36

Fix FS moves across Unix mounts

771c0dd

rjmholt force-pushed the fix-move-unix-mounts branch from b612197 to 771c0dd Compare June 27, 2020 18:37

rjmholt added 2 commits June 27, 2020 12:21

Fix scenarios where files don't exist

d7cdc00

Add test for throwing on non-existent source

ceedf27

rjmholt marked this pull request as draft June 28, 2020 14:53

rjmholt added 2 commits June 28, 2020 11:03

Use try/catch method for rename fallback

c7cba1f

Fix test to actually throw

32932ed

rjmholt marked this pull request as ready for review June 28, 2020 18:04

rjmholt added 3 commits June 28, 2020 11:38

Fix VS's stupid region indentation

5827bda

Undo more autoformatting

f5f30c8

Undo last autoformatting

618aea9

iSazonov mentioned this pull request Jun 29, 2020

System.IO.Directory.Move fails between two mounts on Linux dotnet/runtime#31149

Open

iSazonov reviewed Jun 29, 2020

View reviewed changes

KalleOlaviNiemitalo reviewed Jun 29, 2020

View reviewed changes

adityapatwardhan reviewed Jun 29, 2020

View reviewed changes

src/System.Management.Automation/namespaces/FileSystemProvider.cs Show resolved Hide resolved

Add test and test hook for cross-device moves

7a2c1fb

adityapatwardhan approved these changes Jun 29, 2020

View reviewed changes

daxian-dbw merged commit d3addeb into PowerShell:master Jul 6, 2020

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jul 7, 2020

iSazonov added this to the 7.1.0-preview.6 milestone Jul 7, 2020

rjmholt mentioned this pull request Jul 7, 2020

Move back to .NET's DirectoryInfo.MoveTo code when .NET bug is fixed #13126

Closed

rjmholt deleted the fix-move-unix-mounts branch July 30, 2020 04:33

ghost mentioned this pull request Aug 17, 2020

Move-Item does not work across mounts on Linux #13039

Closed

iSazonov mentioned this pull request Mar 4, 2021

Fix Move-Item for FileSystemProvider to use copy-delete instead of move for DFS paths #14913

Merged

22 tasks

Fix FS moves across Unix mounts #13044

Fix FS moves across Unix mounts #13044

Uh oh!

Conversation

rjmholt commented Jun 27, 2020

PR Summary

PR Context

PR Checklist

Uh oh!

rjmholt commented Jun 27, 2020

Uh oh!

KalleOlaviNiemitalo commented Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjmholt commented Jun 28, 2020

Uh oh!

KalleOlaviNiemitalo commented Jun 28, 2020

Uh oh!

rjmholt commented Jun 28, 2020

Uh oh!

rjmholt commented Jun 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KalleOlaviNiemitalo commented Jun 29, 2020

Uh oh!

rjmholt commented Jun 29, 2020

Uh oh!

rjmholt commented Jun 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Aug 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

KalleOlaviNiemitalo commented Jun 27, 2020 •

edited

Loading

iSazonov Jun 30, 2020 •

edited

Loading