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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Improve
File.Copy
by using the block copy operation for ReFS on Windows #88695[WIP] Improve
File.Copy
by using the block copy operation for ReFS on Windows #88695Changes from 3 commits
4711fde
f864fec
306ca00
e1be061
0b9b36a
2a8f650
ee32bef
33b6090
ddb816b
cf8fbf6
29ef3d3
fba6f0f
5b1180d
7f21a4c
a7a74a0
b6ff8b9
f6df015
ce80ab2
b02e4bc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make it so tests (at least some of them) run on a virtual ReFS drive also? That would be ideal, since it runs in the temporary directory currently, and thus we would never be checking if the code works on ReFS in CI or when running it locally. It would also be useful for other OSes like macOS if we could to this, so we can check logic there properly also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the issue here "can we make tests temporarily create a ReFS volume"? I assume one could call Windows API to try to shrink the current volume and create a ReFS volume. Another alternative that's not entirely unreasonable is to create a test that needs manual setup. There are some manual console tests for example. Those only run on a developer's machine, and presumably only when they change relevant code.
On Mac/Linux possibly they could be created in memory? For example, here's a manual test we have that creates an EXFAT partition in memory:
runtime/src/libraries/System.IO.FileSystem/tests/ManualTests/ManualTests.cs
Lines 89 to 94 in 7db092a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just that. I think it would be good to run some of these tests in CI automatically (and potentially locally also). Would that be possible? It would also be useful to test exFAT stuff properly on macOS for example. My understanding is that this stuff is not really tested currently, which isn't great imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could potentially ask infra folks to set up a ReFS partition on the machines. Or we could try and do it at execution time, with shrinking as mentioned (seems slow though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I'd defer worrying about that until we have code we know is good to go...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the pipeline YAML and unit tests for https://github.com/microsoft/CopyOnWrite - there's a pattern there for (a) a dev to specify an env var pointing to a ReFS volume to test on, and (b) when running as admin, create a ReFS VHD and run tests on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it helps, our test automation always runs as admin. Except potentially on a dev machine.
We conventionally mark tests as outer loop if they are potentially disruptive/messy/slow like creating a VHD likely is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reached out to internal eng folks to ask how they'd feel about either updating images to have a ReFS partition, or allowing us to resize as in the yml above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea for the VHD would be to use the dotnet/runtime-assets repo to store the vhd file, which would at least cut the cost of creating the VHD and only leave the cost of mounting and unmounting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test, and the test about checking it's actually cloned should be moved to a new issue. That way we could add it for macOS at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would ideally have a test to ensure it actually does a block copy operation on ReFS. Does anyone know what APIs you can call to check this on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd have to ask someone like @erikmav ? We don't in general have special knowledge of ReFS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. on macOS, there is
fcntl
withF_LOG2PHYS
orF_LOG2PHYS_EXT
, which can be used to check this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been poking around looking for this as e.g. a mode under
fsutil
. Still looking or will ask around.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you have a small test VHD and attempt to duplicate a file that is so large it already occupies more than half of the disk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anyone know if this is meant to fail. And if so, why?
Specifically, copying from an ADS fails, even today. But copying onto one doesn't for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeremyKuhne maybe knows
BTW randomly linking here to the old issue requesting we support alternate data streams. #49604 Not currently planned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do seem to currently "support" ADS btw. Most APIs work fine with them. Specifically copying from one doesn't seem to work, but you seem to be able to read and write to them fine as if they were files for example. I would think if we didn't support them, it would throw an exception instead for all operations (since it's pretty easy to detect), as opposed to only some of them.
That API would be more useful on platforms like macOS where it's done differently (ie. there isn't really a filename you can give the ADS). It would also be useful on Windows probably, but you can already do a number of things with them with .NET already on Windows.