-
Notifications
You must be signed in to change notification settings - Fork 328
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
v2.6.2 unable to mount on macOS #538
Comments
I think there may be a shadowing bug (or at least an unintended use) here: Line 71 in da0cf03
Since the goroutine is not started till the outer That said: This is not new with that commit, and I don't see an immediate causal relationship to this bug, except insofar as where the channel gets read maybe. |
I did a bit more picking at this, and it appears if control does not return from |
@hanwen Do you have opinions about the right way to address this? It seems to me like there are a couple options:
I'm happy to do some work to plumb it together, but I don't want to start hacking without getting a second opinion. 🙂 |
thanks for the sleuthing. I was afraid something like this might be going on. The solution is to partially revert the commit: the ready channel shouldn't be in the Server struct, but it should be passed to the mount function. Also, it gist of your analysis should be in a comment somewhere. Maybe I should bite the bullet and buy a mac for testing the osx flavor of this. I keep hearing bad things about disallowed kext loading. Is it completely disallowed in the latest versions of macos, or is there a developer switch that enables it? |
Sounds good. I'll dig into it a bit more this weekend and send you a PR to consider.
You can disable SIP, but since I don't work on the FUSE extension itself I just re-approve it each time it gets updated (which is a click-ops step in the system settings and a reboot, ugh). But once the extension is approved you don't need to do extra steps to load and unload it, the approval is just for whether to trust the developer signature. In principle the author could get on the allowlist but Apple charges a lot for that and I suspect he doesn't make enough from working on this to pay for it. |
Google had to convert SrcFS to use NFS, so I think this is not just a matter of paying enough. |
Unfortunate, but it makes sense. I have seen a couple attempts to shim the FUSE interface into an NFS server, but regrettably none that work reliably enough for me to switch to them yet 😞 |
Edit: The below turns out to be some other issue, which went away after a reboot.
|
Hmm, now that I dig a bit further, I think maybe the behaviour seen in the above comment is confounded by some other issues. More investigation is required. |
I bisected again, and it appears that commit e68e570 also interacts with this issue. Reverting it is a little tricky, as there are some later changes built atop it, but as an experiment I checked out at commit b89a90e (the commit just before that in history), reverted f829e36, and the mount still works. When I repeat the same experiment at commit e68e570 (reverting the channel change), I get:
This appears to come from the |
Oh this is interesting, I just noticed:
The INIT opcode is 26, not 19, and indeed opcode 19 is not defined at all. So where is that coming from in this case? |
I have found another interesting thing, in fuse/server.go: req.serializeHeader(req.outPayloadSize())
if req.inHeader().Opcode == _OP_INIT && ms.kernelSettings.Minor <= 22 {
// v8-v22 don't have TimeGran and further fields.
req.outHeader().Length = uint32(sizeOfOutHeader) + 24
} because we serialize the header before making the adjustment, it appears we get the wrong header layout in cases where the minor version is less than 22, which includes Darwin (presently set to 19 in the latest release). I found that if I reorder this code: if req.inHeader().Opcode == _OP_INIT && ms.kernelSettings.Minor <= 22 {
// v8-v22 don't have TimeGran and further fields.
req.outHeader().Length = uint32(sizeOfOutHeader) + 24
}
req.serializeHeader(req.outPayloadSize()) then, combined with a revert of f829e36, I am able to mount a filesystem correctly with macOS off the tip of master. |
I sent |
Older kernel versions do not support all the fields of the current INIT header. Adjust the header size before marshaling the header payload. Updates #538 Change-Id: I06933c281af5c46bde6bfecaf5269274d20b8ad6
More digging reveals that Darwin wants not only the INIT callback, but also the initial STATFS, before it will return control from the Reverting the channel change outright fixes this because it allows the caller to begin serving requests before blocking on the channel. But with my putative fix (where it waits after handleInit returns), it still stalls at startup. @hanwen Sadly, in light of this, I think maybe the original construction—where you had the channel hanging off the Server type directly—might have been a better approach. It's annoying that this is only needed for Darwin, but it definitely is needed there. 😞 |
Not for merging, but I put together https://github.com/creachadair/go-fuse/pull/2 as a prototype of another way we might solve this. If you think this is OK I can turn it into a proper PR on Gerrit. |
hmm. I'm starting to think that the original wasn't so bad, because the |
Yes and no—the meaning of the error is clear, but the semantics of when it has to be closed were never spelled out, and are fiddly to document. My proposal in that draft PR is meant to avoid the need for managing the channel at all in cases where it's not needed. YMMV, though. (That said, simply reverting that change would certainly be simpler to do 🙂) |
see https://review.gerrithub.io/c/hanwen/go-fuse/+/1203281 looks like there is movement on the kext front, osxfuse/osxfuse#1025 |
Oh, that would be excellent. Thank you for the change! |
With v2.6.3 patched, this appears to be resolved, thank you very much for the quick turnaround on this. |
After updating from v2.6.1 to v2.6.2, I am unable to mount a filesystem successfully on macOS, I get the following diagnostic:
It works fine if I revert to v2.6.1, but I have not yet figured out which specific commit is the culprit. I will do some more digging and see if I can isolate it further.
The text was updated successfully, but these errors were encountered: