Skip to content
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

Win32 FS: Rewrite (fix) vfs::host::rename, Fix fs::utime for directories #8894

Merged
merged 5 commits into from
Sep 24, 2020

Conversation

elad335
Copy link
Contributor

@elad335 elad335 commented Sep 11, 2020

I've noticed that when MoveFileEx is called on a directory with opened file handles it returns an access error, it even affects one of my cellSysCache tests which was my testcase for this fix.
"Suspend" opened sys_fs file handles in such case by closing, fs::rename then reopen on the new (if succeeded) or old file path.

Partially addresses #8891 (fixes Battlefieled BC2)

Also: add guest filesystem information in kernel explorer and cellSysCache improvemenrs.

@elad335 elad335 force-pushed the rename branch 7 times, most recently from f53418e to 1bfafc8 Compare September 11, 2020 19:14
@elad335 elad335 changed the title Win32 FS: Rewrite (fix) vfs::host::rename Win32 FS: Rewrite (fix) vfs::host::rename, Fix fs::utime for directories Sep 11, 2020
@elad335
Copy link
Contributor Author

elad335 commented Sep 11, 2020

Added an fs::utime fix for directories based on https://docs.microsoft.com/en-us/windows/win32/fileio/obtaining-a-handle-to-a-directory to fix fs::utime usage on the pr. Requesting @bevanweiss to review the fix. (ERROR_ACCESS_DENIED was returned before)

Utilities/File.cpp Outdated Show resolved Hide resolved
Utilities/File.cpp Outdated Show resolved Hide resolved
@bevanweiss
Copy link
Contributor

I think this might be a slightly bigger problem that this implementation won't resolve.
If a file is currently locked by a real PS3, then that file can be renamed, moved etc and those operations will all occur. The handles will still point at the same file, and all will be well.
Under Windows, I don't believe this is possible (typically). When a file handle is open, the path of that file is locked, and hence renaming parent paths is prevented, and moving the file is prevented.

It does appear that if (all) the files are opened with the FILE_SHARE_DELETE attribute, then renames are allowed, but I'm not sure if that extends to entire path moves, nor what kind of other problems it will raise..
https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea

The policy of always specifying FILE_SHARE_DELETE appears to be supported by Microsoft (in regards to Golang libraries anyway): golang/go#32088 (comment)

It 'might' be a cleaner solution.

@elad335
Copy link
Contributor Author

elad335 commented Sep 12, 2020

We already use this flag though, that's why renaming each file works here.

@bevanweiss
Copy link
Contributor

We already use this flag though.

In fs::file::file() it appears that we don't always set this flag, although admittedly there's not many situations where we don't set it, but it would only take one file to block the move/rename.

DWORD share = 0;
if (!(mode & fs::unread) || !(mode & fs::write))
{
	share |= FILE_SHARE_READ;
}

if (!(mode & (fs::lock + fs::unread)) || !(mode & fs::write))
{
	share |= FILE_SHARE_WRITE | FILE_SHARE_DELETE;
}

@elad335
Copy link
Contributor Author

elad335 commented Sep 12, 2020

I added a commit for it but I already tested all 3 flags always set in fs::file::open, doesnt fix it and renaming each file still works.
Im afraid this problem is related to renaming the entire paths.

@elad335
Copy link
Contributor Author

elad335 commented Sep 15, 2020

Made vfs::host::rename atomic for it and added kernel explorer info on filesystem.

@elad335 elad335 force-pushed the rename branch 2 times, most recently from 06f6edf to 83da09f Compare September 15, 2020 15:54
@elad335
Copy link
Contributor Author

elad335 commented Sep 15, 2020

Can't use cpu_thread::suspend_all... (no-op here) rewritten.

@elad335 elad335 force-pushed the rename branch 2 times, most recently from e031cd5 to 7f66df6 Compare September 16, 2020 00:49
@elad335 elad335 force-pushed the rename branch 3 times, most recently from 5195725 to 9d640db Compare September 17, 2020 22:33
@elad335
Copy link
Contributor Author

elad335 commented Sep 17, 2020

100% accurate now.

@Nekotekina
Copy link
Member

Some conflict

@elad335
Copy link
Contributor Author

elad335 commented Sep 24, 2020

Conflict should be fixed.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants