core: Improve access tracking performance for read/write#163
Open
kohlschuetter wants to merge 2 commits intodCache:masterfrom
Open
core: Improve access tracking performance for read/write#163kohlschuetter wants to merge 2 commits intodCache:masterfrom
kohlschuetter wants to merge 2 commits intodCache:masterfrom
Conversation
When specifying lambdas for cleanup operations, it is good practice to
reduce the scope of instances referenced ("pinned") in the lambda scope.
Right now, we reference unnecessary objects, adding to heap
fragmentation.
Reduce the scope of referenced objects by accessing the required
references before entering the dispose listener lambda.
Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
Currently, read and write operations to the VirtualFileSystem are "stateless" in the sense that there is no corresponding "open" and "close" exposed. At the NFS4 level, this exists, and nfs4j tracks these internally already (stateid4.opaque byte sequences), but they're not exposed to VirtualFileSystem. This is a performance problem: PseudoFs calls checkAccess(inode, ACE4_READ_DATA/ACE4_WRITE_DATA) for each read/write, which in turn triggers a Stat for each read/write operation. This incurs an unnecessary performance penalty of more than 20%. The access check is unnecessary because a successful check upon "open" will be valid for the entire lifetime of the client-side file descriptor, just as it is when opening a file descriptor on other POSIX file systems. In order to properly track granted access, we can leverage data stored in stateid4 and the existing work in FileTracker. Since stateid4 exists in NFSv4 only, we make sure there is no performance degradation in NFSv3 and no exposure of NFSv4-only internals in the VirtualFileSystem API: Expose stateids as OpenHandle to VirtualFileSystem read/write, defaulting to existing read/write methods for backwards compatibility. In PseudoFS, check SharedAccess state from FileTracker and use this to determine access during read/write when available. Rework stateid4 to implement OpenHandle, and expose ClientId (the first 8 bytes of the opaque) so we can look up the information in NFSv4StateHandler. With the change, for sustained reads I'm seeing 854 MB/s instead of 712 MB/s on LocalFileSystem, and 2000 MB/s instead of 1666 MB/s on a custom implementation. Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
Member
|
Consider the following scenario: sequenceDiagram
Client->>Server: OPEN
Server ->> Client: OK, open_stateid
Client->>Server: READ(open_stateid)
Server ->> Client: OK, bytes
Client->>Server: CLOSE(open_stateid)
Server ->> Server: invalidate open_stateid
Server ->> Client: OK
Client->>Server: OPEN
Server ->> Client: OK, open_stateid, delegation_stateid
Client->>Server: CLOSE (open_stateid)
Server ->> Server: invalidate open_stateid
Server ->> Client: OK
Client->>Server: READ(delegation_stateid)
Server ->> Client: OK, bytes
With your approach, the filesystem implementation should track all stateids. var myListeningFs = ...;
var stateHandler = new NFSv4StateHandler();
stateHandler.getFileTracker().addListener(myListeningFs); |
Contributor
Author
|
I think right now, the We could expose With the patch, I'm getting 20% better performance without sacrificing PseudoFs, so that's good enough for me. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently, read and write operations to the VirtualFileSystem are
"stateless" in the sense that there is no corresponding "open" and
"close" exposed. At the NFS4 level, this exists, and nfs4j tracks these
internally already (stateid4.opaque byte sequences), but they're not
exposed to VirtualFileSystem.
This is a performance problem: PseudoFs calls checkAccess(inode,
ACE4_READ_DATA/ACE4_WRITE_DATA) for each read/write, which in turn
triggers a Stat for each read/write operation.
This incurs an unnecessary performance penalty of more than 20%.
The access check is unnecessary because a successful check upon "open"
will be valid for the entire lifetime of the client-side file
descriptor, just as it is when opening a file descriptor on other POSIX
file systems.
In order to properly track granted access, we can leverage data stored
in stateid4 and the existing work in FileTracker.
Since stateid4 exists in NFSv4 only, we make sure there is no
performance degradation in NFSv3 and no exposure of NFSv4-only internals
in the VirtualFileSystem API:
Expose stateids as OpenHandle to VirtualFileSystem read/write,
defaulting to existing read/write methods for backwards compatibility.
In PseudoFS, check SharedAccess state from FileTracker and use this to
determine access during read/write when available. Rework stateid4 to
implement OpenHandle, and expose ClientId (the first 8 bytes of the
opaque) so we can look up the information in NFSv4StateHandler.
With the change, for sustained reads I'm seeing 854 MB/s instead of 712
MB/s on LocalFileSystem, and 2000 MB/s instead of 1666 MB/s on a custom
implementation.
Also add a small change to limit the scope of addDisposeListener lambdas