core: Improve access tracking performance for read/write operations

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 problem not only for scenarios where an explicit open/close is
required, but even for currently implemented scenarios it's 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 call, just as it is when
opening a file descriptor in POSIX.

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:

1. Expose "Opaque" stateids to VirtualFileSystem read/write/commit; add
   open/close for custom FS interop.
2. Rework stateid4, which currently conflates stateid and seqid --
   directly reference stateid4.other Opaque when possible.
3. In PseudoFS, check SharedAccess state from FileTracker and use this
   to determine access during read/write when available.

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 <[email protected]>

Author: kohlschuetter

core/src/main/java/org/dcache/nfs/v4/FileTracker.java

@@ -315,9 +315,9 @@ public OpenRecord addOpen(NFS4Client client, StateOwner owner, Inode inode, int
                         os.shareDenySeen |= 1 << ((shareDeny & nfs4_prot.OPEN4_SHARE_ACCESS_BOTH) - 1);
                     }
 
-                    os.stateid.seqid++;
+                    os.stateid.bumpSeqid();
                     // we need to return copy to avoid modification by concurrent opens
-                    var openStateid = new stateid4(os.stateid.other, os.stateid.seqid);
+                    var openStateid = os.stateid.clone();
 
                     // yet another open from the same client. Let's check if we can delegate.
                     if (canDelegateRead && (os.shareAccess
@@ -341,11 +341,12 @@ public OpenRecord addOpen(NFS4Client client, StateOwner owner, Inode inode, int
             stateid = state.stateid();
             OpenState openState = new OpenState(client, owner, stateid, shareAccess, shareDeny);
             opens.add(openState);
-            state.addDisposeListener(s -> removeOpen(inode, stateid));
-            stateid.seqid++;
+            Opaque fileIdKey = inode.getFileIdKey();
+            state.addDisposeListener(s -> removeOpen(fileIdKey, stateid));
+            stateid.bumpSeqid();
 
             // we need to return copy to avoid modification by concurrent opens
-            var openStateid = new stateid4(stateid.other, stateid.seqid);
+            var openStateid = stateid.clone();
 
             // REVISIT: currently only read-delegations are supported
             if (canDelegateRead && (wantReadDelegation || adlHeuristic.shouldDelegate(client, inode))) {
@@ -408,9 +409,9 @@ public stateid4 downgradeOpen(NFS4Client client, stateid4 stateid, Inode inode,
             os.shareAccess = shareAccess;
             os.shareDeny = shareDeny;
 
-            os.stateid.seqid++;
+            os.stateid.bumpSeqid();
             // we need to return copy to avoid modification by concurrent opens
-            return new stateid4(os.stateid.other, os.stateid.seqid);
+            return os.stateid.clone();
         } finally {
             lock.unlock();
         }
@@ -463,16 +464,21 @@ public void delegationReturn(NFS4Client client, stateid4 stateid, Inode inode)
      */
     public int getShareAccess(NFS4Client client, Inode inode, stateid4 stateid)
             throws ChimeraNFSException {
+        Opaque stateIdOpaque = stateid.getOpaque();
+        return getShareAccess(client, inode, stateIdOpaque);
+    }
 
+    public int getShareAccess(NFS4Client client, Inode inode, Opaque stateid)
+            throws ChimeraNFSException {
         Opaque fileId = inode.getFileIdKey();
         Lock lock = filesLock.get(fileId);
         lock.lock();
         try {
 
-            switch (stateid.other[11]) {
+            switch (stateid.byteAt(11)) {
                 case Stateids.LOCK_STATE_ID:
                     NFS4State lockState = client.state(stateid);
-                    stateid = lockState.getOpenState().stateid();
+                    stateid = lockState.getOpenState().stateid().getOpaque();
                     // fall through
                 case Stateids.OPEN_STATE_ID: {
                     final List<OpenState> opens = files.get(fileId);
@@ -481,10 +487,10 @@ public int getShareAccess(NFS4Client client, Inode inode, stateid4 stateid)
                         throw new BadStateidException("no matching open");
                     }
 
-                    final stateid4 openStateid = stateid;
+                    final Opaque openStateid = stateid;
                     return opens.stream()
                             .filter(s -> client.getId() == s.client.getId())
-                            .filter(s -> s.stateid.equals(openStateid))
+                            .filter(s -> s.stateid.getOpaque().equals(openStateid))
                             .mapToInt(OpenState::getShareAccess)
                             .findAny()
                             .orElseThrow(BadStateidException::new);
@@ -496,11 +502,11 @@ public int getShareAccess(NFS4Client client, Inode inode, stateid4 stateid)
                         throw new BadStateidException("no delegation found");
                     }
 
-                    stateid4 delegationStateid = stateid;
+                    Opaque delegationStateid = stateid;
 
                     var delegation = fileDelegations.stream()
                             .filter(d -> d.client().getId().equals(client.getId()))
-                            .filter(d -> d.delegationStateid().stateid().equals(delegationStateid))
+                            .filter(d -> d.delegationStateid().stateid().getOpaque().equals(delegationStateid))
                             .findAny()
                             .orElseThrow(BadStateidException::new);
 
@@ -526,8 +532,10 @@ public int getShareAccess(NFS4Client client, Inode inode, stateid4 stateid)
      * @param stateid associated with the open.
      */
     void removeOpen(Inode inode, stateid4 stateid) {
+        removeOpen(inode.getFileIdKey(), stateid);
+    }
 
-        Opaque fileId = inode.getFileIdKey();
+    void removeOpen(Opaque fileId, stateid4 stateid) {
         Lock lock = filesLock.get(fileId);
         lock.lock();
         try {

core/src/main/java/org/dcache/nfs/v4/NFS4Client.java

@@ -117,7 +117,7 @@ public class NFS4Client {
      */
     private int _sessionSequence = 1;
 
-    private final Map<stateid4, NFS4State> _clientStates = new ConcurrentHashMap<>();
+    private final Map<Opaque, NFS4State> _clientStates = new ConcurrentHashMap<>();
 
     /**
      * sessions associated with the client
@@ -334,18 +334,20 @@ private NFS4State createState(StateOwner stateOwner, byte type, NFS4State openSt
 
         NFS4State state = new NFS4State(openState, stateOwner, _stateHandler.createStateId(this, type, _stateIdCounter
                 .incrementAndGet()));
+        stateid4 stateId = state.stateid();
+        Opaque opaque = stateId.getOpaque();
         if (openState != null) {
             openState.addDisposeListener(s -> {
                 // remove and dispose derived states.
-                NFS4State nfsState = _clientStates.get(state.stateid());
+                NFS4State nfsState = _clientStates.get(opaque);
                 if (nfsState != null) {
                     _log.debug("removing derived state {}", nfsState);
                     nfsState.tryDispose();
                 }
-                _clientStates.remove(state.stateid());
+                _clientStates.remove(opaque);
             });
         }
-        _clientStates.put(state.stateid(), state);
+        _clientStates.put(opaque, state);
         return state;
     }
 
@@ -410,29 +412,34 @@ public NFS4State createServerSideCopyState(StateOwner stateOwner, NFS4State open
     }
 
     public void releaseState(stateid4 stateid) throws ChimeraNFSException {
+        Opaque opaque = stateid.getOpaque();
 
-        NFS4State state = _clientStates.get(stateid);
+        NFS4State state = _clientStates.get(opaque);
         if (state == null) {
             throw new BadStateidException("State not known to the client: " + stateid);
         }
         state.disposeIgnoreFailures();
-        _clientStates.remove(stateid);
+        _clientStates.remove(opaque);
     }
 
     public void tryReleaseState(stateid4 stateid) throws ChimeraNFSException {
 
-        NFS4State state = _clientStates.get(stateid);
+        NFS4State state = _clientStates.get(stateid.getOpaque());
         if (state == null) {
             throw new BadStateidException("State not known to the client: " + stateid);
         }
         state.tryDispose();
-        _clientStates.remove(stateid);
+        _clientStates.remove(stateid.getOpaque());
     }
 
     public NFS4State state(stateid4 stateid) throws ChimeraNFSException {
-        NFS4State state = _clientStates.get(stateid);
+        return state(stateid.getOpaque());
+    }
+
+    public NFS4State state(Opaque stateidOpaque) throws ChimeraNFSException {
+        NFS4State state = _clientStates.get(stateidOpaque);
         if (state == null) {
-            throw new BadStateidException("State not known to the client: " + stateid);
+            throw new BadStateidException("State not known to the client: " + stateidOpaque);
         }
         return state;
     }
@@ -534,7 +541,7 @@ public boolean hasState() {
      * @param state to attach
      */
     public void attachState(NFS4State state) {
-        _clientStates.put(state.stateid(), state);
+        _clientStates.put(state.stateid().getOpaque(), state);
     }
 
     /**
@@ -543,7 +550,7 @@ public void attachState(NFS4State state) {
      * @param state to detach
      */
     public void detachState(NFS4State state) {
-        _clientStates.remove(state.stateid());
+        _clientStates.remove(state.stateid().getOpaque());
     }
 
     @GuardedBy("this")

core/src/main/java/org/dcache/nfs/v4/NFS4State.java

@@ -68,7 +68,7 @@ public NFS4State(NFS4State openState, StateOwner owner, stateid4 stateid) {
     }
 
     public void bumpSeqid() {
-        ++_stateid.seqid;
+        _stateid.bumpSeqid();
     }
 
     public stateid4 stateid() {

core/src/main/java/org/dcache/nfs/v4/NFSServerV41.java

@@ -134,7 +134,7 @@ public COMPOUND4res NFSPROC4_COMPOUND_4(RpcCall call$, COMPOUND4args arg1) {
             }
             res.resarray = new ArrayList<>(arg1.argarray.length);
 
-            VirtualFileSystem fs = new PseudoFs(_fs, call$, _exportTable);
+            VirtualFileSystem fs = new PseudoFs(_fs, call$, _exportTable, _statHandler);
 
             CompoundContextBuilder builder = new CompoundContextBuilder()
                     .withMinorversion(arg1.minorversion.value)

core/src/main/java/org/dcache/nfs/v4/NFSv4StateHandler.java

@@ -237,13 +237,22 @@ public NFS4Client getClient(clientid4 clientid) throws StaleClientidException {
         }
     }
 
+    public NFS4Client getClientIfExists(long clientId) {
+        _readLock.lock();
+        try {
+            return _clientsByServerId.get(new clientid4(clientId));
+        } finally {
+            _readLock.unlock();
+        }
+    }
+
     public NFS4Client getClientIdByStateId(stateid4 stateId) throws ChimeraNFSException {
 
         _readLock.lock();
         try {
             checkState(_running, "NFS state handler not running");
 
-            clientid4 clientId = new clientid4(Bytes.getLong(stateId.other, 0));
+            clientid4 clientId = new clientid4(stateId.getClientId());
             NFS4Client client = _clientsByServerId.get(clientId);
             if (client == null) {
                 throw new BadStateidException("no client for stateid: " + stateId);
@@ -439,7 +448,7 @@ public int getInstanceId() {
      * @return state hander id.
      */
     public static int getInstanceId(stateid4 stateid) {
-        long clientid = Bytes.getLong(stateid.other, 0);
+        long clientid = stateid.getClientId();
         return (int) (clientid >> 16) & 0xFFFF;
     }
 
@@ -472,7 +481,7 @@ public stateid4 createStateId(NFS4Client client, byte type, int count) {
         // we eat the first 8 bits if the counter, however, we don't expect 16M states be active at the same time,
         // thus the probability of a collision is too low
         Bytes.putInt(other, 8, count << 8 | (type & 0xFF));
-        return new stateid4(other, STATE_INITIAL_SEQUENCE);
+        return stateid4.forBytes(other, STATE_INITIAL_SEQUENCE);
     }
 
     /**

core/src/main/java/org/dcache/nfs/v4/OperationCLOSE.java

@@ -59,6 +59,8 @@ public void process(CompoundContext context, nfs_resop4 result)
         NFS4State nfsState = client.state(stateid);
         Stateids.checkStateId(nfsState.stateid(), stateid);
 
+        context.getFs().close(stateid.getOpaque());
+
         if (context.getMinorversion() == 0) {
             nfsState.getStateOwner().acceptAsNextSequence(_args.opclose.seqid);
             client.updateLeaseTime();
@@ -68,6 +70,5 @@ public void process(CompoundContext context, nfs_resop4 result)
 
         res.open_stateid = Stateids.invalidStateId();
         res.status = nfsstat.NFS_OK;
-
     }
 }

core/src/main/java/org/dcache/nfs/v4/OperationCOMMIT.java

@@ -28,6 +28,7 @@
 import org.dcache.nfs.v4.xdr.nfs_argop4;
 import org.dcache.nfs.v4.xdr.nfs_opnum4;
 import org.dcache.nfs.v4.xdr.nfs_resop4;
+import org.dcache.nfs.v4.xdr.stateid4;
 import org.dcache.nfs.vfs.Inode;
 
 public class OperationCOMMIT extends AbstractNFSv4Operation {
@@ -42,8 +43,11 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF
         final COMMIT4res res = result.opcommit;
         Inode inode = context.currentInode();
 
+        stateid4 stateid = Stateids.getCurrentStateidIfNeeded(context, _args.opwrite.stateid);
+
         _args.opcommit.offset.checkOverflow(_args.opcommit.count.value, "offset + length overflow");
-        context.getFs().commit(inode, _args.opcommit.offset.value, _args.opcommit.count.value);
+        context.getFs().commit(stateid.getOpaque(), inode, _args.opcommit.offset.value,
+                _args.opcommit.count.value);
 
         res.resok4 = new COMMIT4resok();
         res.resok4.writeverf = context.getRebootVerifier();