Mac ProjFS: Only hydrate/expand when deleting due to rename #1193
Conversation
pmj
force-pushed
the
526-only-hydrate-on-rename-delete
branch
2 times, most recently
from
11482d6
to
90d927a
Compare
|
Review Notes: You'll notice that the first commit in this series, which actually implements the functionality, passes all tests, while the second commit does not - this is the one that kills High Sierra compatibility, and it looks like the Mac Functional tests are still running on a HS machine, so loading the kext there fails. I've run the functional tests locally on a Mojave machine too, and it's all working fine with the new code paths. Personally, as I've said, I'm happy to maintain HS compatibility for the moment - it's quite isolated in the kext, the special casing is extremely straightforward, and I don't foresee anything starting to depend on this optimisation in the near future. I also don't know what the status of the "large build" is with regard to Mojave, perhaps @nickgra can comment on that and what we should do with regard to that in this PR? Edit:
|
pmj
force-pushed
the
526-only-hydrate-on-rename-delete
branch
from
fab8ec2
to
37137b9
Compare
|
|
||
|
|
||
| s_renameLock = SpinLock_Alloc(); | ||
| s_maxPendingRenames = 8; // Arbitrary choice, should be maximum number of expected concurrent threads performing renames, but array will resize on demand |
There was a problem hiding this comment.
I could see some applications doing a bunch of renames (this is how a lot of editors seem to save). Do we know how many threads the Kernel runs with?
There was a problem hiding this comment.
There is a kernel thread for every thread in every process, plus it can create its own. So there are usually thousands. Very few will be between the kauth fileop and vnode authorisation calls in the rename code at any time though, and if there are more than 8, well, that's why allow resizing.
|
Just an FYI, I think our target users are not quite on Mojave although that will be a requirement for go-live. @nickgra for reference |
|
|
||
| SpinLock_Acquire(s_renameLock); | ||
| { | ||
| for (uint32_t i = 0; i < s_maxPendingRenames; ++i) |
There was a problem hiding this comment.
Heads up: I will change this logic slightly so that we don't have to scan the whole array every time. (which is mostly empty most of the time)
That's very much the impression I'm getting. When we talked about this change out-of-band before I embarked on it however, everyone seemed very much against having any backwards compatibility code whatsoever, which is why I'm vocally calling attention to the issue. I don't think we can realistically integrate this change if we don't have explicit HS support, at least initially. I guess one option is to put the rename tracking code in, but not actually do anything with the information so that the kext behaves exactly as before except it's internally aware of the differences between rename-delete and other delete operations. I don't think this is a good idea though because it gives us no information on whether the changes are working as intended. |
pmj
force-pushed
the
526-only-hydrate-on-rename-delete
branch
from
7891180
to
c8af668
Compare
|
This latest version, rebased on master, fixes the first set of review feedback as well as a bunch of bugs that were surfaced by the now-working |
|
/azp run GitHub VFSForGit Mac Functional Tests |
|
No pipelines are associated with this pull request. |
pmj
force-pushed
the
526-only-hydrate-on-rename-delete
branch
from
01472b0
to
83a8aa7
Compare
|
/azp run PR - Mac - Build and Unit Test |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run PR - Mac - Build and Unit Test |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run GitHub VFSForGit Mac Functional Tests |
|
No pipelines are associated with this pull request. |
|
/azp run GitHub VFSForGit Mac Functional Tests - Mojave |
|
No pipelines are associated with this pull request. |
This change moves the vnode eligibility check (file system type, vnode type) from ShouldHandleVnodeOpEvent to HandleVnodeOperation to avoid repeated calls. A unit test has been added to provide test coverage for the new code branch.
This adds an enum scoped in a namespace to be used instead of magic numbers in version checks. The reasons for choosing a classic enum in a namespace are: * The namespace gives us explicit scoping rather than polluting the root namespace. * The using a classic enum vs a modern C++ enum class means the values are implicitly convertible to integers, which is how we actually use them.
Replaces a use of the old mock vnode creation function with the new tree and mountpoint-based one.
This change utilises the KAUTH_FILEOP_WILL_RENAME event which was introduced with Mojave/10.14 to determine if a KAUTH_VNODE_DELETE authorisation check is due to a rename() operation. In this case, the file or directory genuinely needs hydrating, otherwise hydration can be skipped. This change also adds unit tests for the new rename operation tracking code, and adapts existing unit tests so hydration is not expected when a KAUTH_VNODE_DELETE check occurs outside of a rename() operation. Note that this change means the behaviour on macOS 10.13 and earlier versus 10.14 and later subtly diverges.
pmj
force-pushed
the
526-only-hydrate-on-rename-delete
branch
from
83a8aa7
to
369dab5
Compare
|
@jeschu1 mentioned that there would be a conflict between this change and #1215 regarding metadata removal, but it's evidently not triggering any functional test failures. After trying to think it through, I have to admit I still can't quite work out what the problem is, and I've run the functional tests locally on Mojave, and they pass, so they're no help in pinpointing the problem. (the azp pipeline seems to have a codesigning issue) I've archived the commit-by-commit review feedback fixes in this branch: https://github.com/pmj/VFSForGit/tree/526-only-hydrate-on-rename-delete-archived and I'm about to force-push a new version of this PR branch that's squashed and rebased on latest master. If the metadata issue turns out to be a non-issue, I'm happy for it to be merged as-is, otherwise @jeschu1 could you please implement that fix? |
|
/azp run PR - Windows - Extra |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jeschu1
force-pushed
the
526-only-hydrate-on-rename-delete
branch
from
fe5513b
to
63d18aa
Compare
|
/azp run GitHub VFSForGit Mac Functional Tests |
|
No pipelines are associated with this pull request. |
|
/azp run GitHub VFSForGit Mac Functional Tests |
|
No pipelines are associated with this pull request. |
|
Sorry for the late reply @jeschu1 - I did take a quick look at your commits and they all look good to me. Thanks for sorting that out! |
This change utilises the KAUTH_FILEOP_WILL_RENAME event which was introduced with Mojave/10.14 to determine if a KAUTH_VNODE_DELETE authorisation check is due to a rename() operation. In this case, the file or directory genuinely needs hydrating, otherwise hydration can be skipped.
This change also adds unit tests for the new rename operation tracking code, and adapts existing unit tests so hydration is not expected when a KAUTH_VNODE_DELETE check occurs outside of a rename() operation.
This implements #526.