From e01d9f803398894bd0db217b563cbdff93fd47b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 15:10:24 +0200 Subject: [PATCH 01/12] core: Opaque: add byteAt/longAt support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We sometimes want to check a certain byte or long as part of an Opaque. Let's add helper methods so we don't have to convert to byte[]. Signed-off-by: Christian Kohlschütter --- .../main/java/org/dcache/nfs/util/Opaque.java | 54 +++++++++++++++++-- 1 file changed, 51 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/dcache/nfs/util/Opaque.java b/core/src/main/java/org/dcache/nfs/util/Opaque.java index bec1ddba..344e5d33 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -20,10 +20,13 @@ package org.dcache.nfs.util; import java.nio.ByteBuffer; +import java.nio.ByteOrder; import java.util.Arrays; import java.util.Base64; import java.util.Objects; +import org.dcache.oncrpc4j.util.Bytes; + /** * Describes something that can be used as a key for {@link java.util.HashMap} and that can be converted to a * {@code byte[]} and a Base64 string representation. @@ -85,6 +88,9 @@ static Opaque forBytes(ByteBuffer buf, int length) { * @see #toImmutableOpaque() */ static Opaque forMutableByteBuffer(ByteBuffer buf, int index, int length) { + if (buf.order() != ByteOrder.BIG_ENDIAN) { + buf = buf.duplicate(); + } return new OpaqueBufferImpl(buf, index, length); } @@ -174,10 +180,26 @@ default void putBytes(ByteBuffer buf) { @Override boolean equals(Object o); - class OpaqueImpl implements Opaque { + /** + * Returns the byte stored at the given position. + * + * @param byteOffset The byte offset + * @return The byte. + */ + byte byteAt(int byteOffset); + + /** + * Returns the {@code long} stored at the given position, using big-endian byte order. + * + * @param byteOffset The byte offset + * @return The long. + */ + long longAt(int byteOffset); + + public class OpaqueImpl implements Opaque { final byte[] _opaque; - OpaqueImpl(byte[] opaque) { + protected OpaqueImpl(byte[] opaque) { _opaque = opaque; } @@ -253,6 +275,16 @@ public int numBytes() { public Opaque toImmutableOpaque() { return Opaque.forBytes(_opaque); } + + @Override + public byte byteAt(int position) { + return _opaque[position]; + } + + @Override + public long longAt(int byteOffset) { + return Bytes.getLong(_opaque, byteOffset); + } } final class OpaqueImmutableImpl extends OpaqueImpl { @@ -360,7 +392,13 @@ public boolean equals(Object o) { } return true; } else { - return toImmutableOpaque().equals(o); + Opaque other = (Opaque) o; + for (int i = index, n = index + length, oi = 0; i < n; i++, oi++) { + if (buf.get(i) != other.byteAt(oi)) { + return false; + } + } + return true; } } @@ -368,5 +406,15 @@ public boolean equals(Object o) { public String toString() { return super.toString() + "[" + toBase64() + "]"; } + + @Override + public byte byteAt(int position) { + return buf.get(position); + } + + @Override + public long longAt(int byteOffset) { + return buf.getLong(byteOffset); + } } } From f3ae713d8f1d4931f04f40fa3ce60acf90c62239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 15:15:12 +0200 Subject: [PATCH 02/12] core: OperationLOCK: Limit scope of addDisposeListener lambda MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../src/main/java/org/dcache/nfs/v4/OperationLOCK.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationLOCK.java b/core/src/main/java/org/dcache/nfs/v4/OperationLOCK.java index d3b9575b..dbff6f59 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationLOCK.java +++ b/core/src/main/java/org/dcache/nfs/v4/OperationLOCK.java @@ -24,8 +24,10 @@ import org.dcache.nfs.status.InvalException; import org.dcache.nfs.status.OpenModeException; import org.dcache.nfs.status.ServerFaultException; +import org.dcache.nfs.util.Opaque; import org.dcache.nfs.v4.nlm.LockDeniedException; import org.dcache.nfs.v4.nlm.LockException; +import org.dcache.nfs.v4.nlm.LockManager; import org.dcache.nfs.v4.nlm.NlmLock; import org.dcache.nfs.v4.xdr.LOCK4denied; import org.dcache.nfs.v4.xdr.LOCK4resok; @@ -118,12 +120,12 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF NlmLock lock = new NlmLock(lockOwner, _args.oplock.locktype, _args.oplock.offset.value, _args.oplock.length.value); - context.getLm().lock(inode.getLockKey(), lock); + Opaque lockKey = inode.getLockKey(); + LockManager lm = context.getLm(); + lm.lock(lockKey, lock); // ensure, that on close locks will be released - lock_state.addDisposeListener(s -> { - context.getLm().unlockIfExists(inode.getLockKey(), lock); - }); + lock_state.addDisposeListener(s -> lm.unlockIfExists(lockKey, lock)); // FIXME: we might run into race condition, thus updating sedid must be fenced! lock_state.bumpSeqid(); From b81bcf0378dcf58d19a75ce1f9b9eb5d0bd82bf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 16:04:58 +0200 Subject: [PATCH 03/12] core: Improve access tracking performance for read/write operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../java/org/dcache/nfs/v4/FileTracker.java | 36 ++++++---- .../java/org/dcache/nfs/v4/NFS4Client.java | 31 ++++---- .../java/org/dcache/nfs/v4/NFS4State.java | 2 +- .../java/org/dcache/nfs/v4/NFSServerV41.java | 2 +- .../org/dcache/nfs/v4/NFSv4StateHandler.java | 15 +++- .../org/dcache/nfs/v4/OperationCLOSE.java | 3 +- .../org/dcache/nfs/v4/OperationCOMMIT.java | 6 +- .../java/org/dcache/nfs/v4/OperationOPEN.java | 32 ++++++--- .../java/org/dcache/nfs/v4/OperationREAD.java | 2 +- .../dcache/nfs/v4/OperationTEST_STATEID.java | 2 +- .../org/dcache/nfs/v4/OperationWRITE.java | 3 +- .../main/java/org/dcache/nfs/v4/Stateids.java | 33 ++++----- .../java/org/dcache/nfs/v4/xdr/stateid4.java | 71 +++++++++++++------ .../dcache/nfs/vfs/ForwardingFileSystem.java | 26 ++++++- .../java/org/dcache/nfs/vfs/PseudoFs.java | 51 +++++++++++-- .../org/dcache/nfs/vfs/VirtualFileSystem.java | 68 +++++++++++++++++- .../org/dcache/nfs/v4/OperationREADTest.java | 16 +++-- .../org/dcache/nfs/v4/OperationWRITETest.java | 25 ++++--- .../org/dcache/nfs/v4/xdr/stateid4Test.java | 33 +++------ 19 files changed, 330 insertions(+), 127 deletions(-) diff --git a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java b/core/src/main/java/org/dcache/nfs/v4/FileTracker.java index 1ff0b9a4..7b511ea7 100644 --- a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java +++ b/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 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 { diff --git a/core/src/main/java/org/dcache/nfs/v4/NFS4Client.java b/core/src/main/java/org/dcache/nfs/v4/NFS4Client.java index 4afbd04b..8eff70f3 100644 --- a/core/src/main/java/org/dcache/nfs/v4/NFS4Client.java +++ b/core/src/main/java/org/dcache/nfs/v4/NFS4Client.java @@ -117,7 +117,7 @@ public class NFS4Client { */ private int _sessionSequence = 1; - private final Map _clientStates = new ConcurrentHashMap<>(); + private final Map _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") diff --git a/core/src/main/java/org/dcache/nfs/v4/NFS4State.java b/core/src/main/java/org/dcache/nfs/v4/NFS4State.java index fa506e13..4a7b0c4d 100644 --- a/core/src/main/java/org/dcache/nfs/v4/NFS4State.java +++ b/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() { diff --git a/core/src/main/java/org/dcache/nfs/v4/NFSServerV41.java b/core/src/main/java/org/dcache/nfs/v4/NFSServerV41.java index e1da08e6..85b6588d 100644 --- a/core/src/main/java/org/dcache/nfs/v4/NFSServerV41.java +++ b/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) diff --git a/core/src/main/java/org/dcache/nfs/v4/NFSv4StateHandler.java b/core/src/main/java/org/dcache/nfs/v4/NFSv4StateHandler.java index b7d64a4d..76a9ac64 100644 --- a/core/src/main/java/org/dcache/nfs/v4/NFSv4StateHandler.java +++ b/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); } /** diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationCLOSE.java b/core/src/main/java/org/dcache/nfs/v4/OperationCLOSE.java index ff4e1850..429ff266 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationCLOSE.java +++ b/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; - } } diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationCOMMIT.java b/core/src/main/java/org/dcache/nfs/v4/OperationCOMMIT.java index d534ab26..96b45d12 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationCOMMIT.java +++ b/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(); diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationOPEN.java b/core/src/main/java/org/dcache/nfs/v4/OperationOPEN.java index 790f3b71..d250df4c 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationOPEN.java +++ b/core/src/main/java/org/dcache/nfs/v4/OperationOPEN.java @@ -33,6 +33,7 @@ import org.dcache.nfs.status.NotDirException; import org.dcache.nfs.status.SymlinkException; import org.dcache.nfs.status.WrongTypeException; +import org.dcache.nfs.v4.FileTracker.OpenRecord; import org.dcache.nfs.v4.xdr.OPEN4res; import org.dcache.nfs.v4.xdr.OPEN4resok; import org.dcache.nfs.v4.xdr.aceflag4; @@ -61,6 +62,8 @@ import org.dcache.nfs.v4.xdr.why_no_delegation4; import org.dcache.nfs.vfs.Inode; import org.dcache.nfs.vfs.Stat; +import org.dcache.nfs.vfs.VirtualFileSystem; +import org.dcache.nfs.vfs.VirtualFileSystem.OpenMode; import org.dcache.oncrpc4j.rpc.OncRpcException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -101,6 +104,9 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF res.resok4.cinfo = new change_info4(); res.resok4.cinfo.atomic = true; + Inode inode; + VirtualFileSystem.OpenMode openMode; + switch (_args.opopen.claim.claim) { case open_claim_type4.CLAIM_NULL: @@ -118,7 +124,6 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF String name = NameFilter.convertName(_args.opopen.claim.file.value); _log.debug("regular open for : {}", name); - Inode inode; if (_args.opopen.openhow.opentype == opentype4.OPEN4_CREATE) { boolean exclusive = (_args.opopen.openhow.how.mode == createmode4.EXCLUSIVE4) @@ -205,13 +210,13 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF } } - + openMode = OpenMode.READ_WRITE; } else { // no changes from us, old stat info is still good enough res.resok4.cinfo.after = new changeid4(stat.getGeneration()); inode = context.getFs().lookup(context.currentInode(), name); - checkCanAccess(context, inode, _args.opopen.share_access); + openMode = checkCanAccess(context, inode, _args.opopen.share_access); } context.currentInode(inode); @@ -238,7 +243,7 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF res.resok4.cinfo.after = new changeid4(0); inode = context.currentInode(); - checkCanAccess(context, inode, _args.opopen.share_access); + openMode = checkCanAccess(context, inode, _args.opopen.share_access); break; case open_claim_type4.CLAIM_DELEGATE_CUR: case open_claim_type4.CLAIM_DELEGATE_PREV: @@ -268,15 +273,19 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF * * THis is a perfectly a valid situation as at the end file is created and only one writer is allowed. */ - var openRecord = context + OpenRecord openRecord = context .getStateHandler() .getFileTracker() .addOpen(client, owner, context.currentInode(), _args.opopen.share_access.value, _args.opopen.share_deny.value); - context.currentStateid(openRecord.openStateId()); - res.resok4.stateid = openRecord.openStateId(); + stateid4 openStateId = openRecord.openStateId(); + context.currentStateid(openStateId); + + context.getFs().open(openStateId.getOpaque(), inode, openMode); + + res.resok4.stateid = openStateId; if (openRecord.hasDelegation()) { res.resok4.delegation.delegation_type = open_delegation_type4.OPEN_DELEGATE_READ; res.resok4.delegation.read = new open_read_delegation4(); @@ -294,19 +303,24 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF } - private void checkCanAccess(CompoundContext context, Inode inode, uint32_t share_access) throws IOException { + private VirtualFileSystem.OpenMode checkCanAccess(CompoundContext context, Inode inode, uint32_t share_access) + throws IOException { int accessMode; + VirtualFileSystem.OpenMode openMode; switch (share_access.value & ~nfs4_prot.OPEN4_SHARE_ACCESS_WANT_DELEG_MASK) { case nfs4_prot.OPEN4_SHARE_ACCESS_READ: accessMode = nfs4_prot.ACCESS4_READ; + openMode = VirtualFileSystem.OpenMode.READ_ONLY; break; case nfs4_prot.OPEN4_SHARE_ACCESS_WRITE: accessMode = nfs4_prot.ACCESS4_MODIFY; + openMode = VirtualFileSystem.OpenMode.WRITE_ONLY; break; case nfs4_prot.OPEN4_SHARE_ACCESS_BOTH: accessMode = nfs4_prot.ACCESS4_READ | nfs4_prot.ACCESS4_MODIFY; + openMode = VirtualFileSystem.OpenMode.READ_WRITE; break; default: throw new InvalException("Invalid share_access mode: " + share_access.value); @@ -328,5 +342,7 @@ private void checkCanAccess(CompoundContext context, Inode inode, uint32_t share default: throw context.getMinorversion() == 0 ? new SymlinkException() : new WrongTypeException(); } + + return openMode; } } diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationREAD.java b/core/src/main/java/org/dcache/nfs/v4/OperationREAD.java index 649fe457..748b8c9a 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationREAD.java +++ b/core/src/main/java/org/dcache/nfs/v4/OperationREAD.java @@ -73,7 +73,7 @@ public void process(CompoundContext context, nfs_resop4 result) throws IOExcepti ByteBuffer buf = ByteBuffer.allocate(count); res.resok4 = new READ4resok(); - int bytesRead = context.getFs().read(inode, buf, offset, res.resok4::setEOF); + int bytesRead = context.getFs().read(stateid.getOpaque(), inode, buf, offset, res.resok4::setEOF); if (bytesRead < 0) { buf.clear(); diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationTEST_STATEID.java b/core/src/main/java/org/dcache/nfs/v4/OperationTEST_STATEID.java index 09feca96..44f74da2 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationTEST_STATEID.java +++ b/core/src/main/java/org/dcache/nfs/v4/OperationTEST_STATEID.java @@ -53,7 +53,7 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF stateid4 statid = _args.optest_stateid.ts_stateids[i]; try { NFS4State state = client.state(statid); - if (state.stateid().seqid < statid.seqid) { + if (state.stateid().getSeqId() < statid.getSeqId()) { res.tsr_resok4.tsr_status_codes[i] = nfsstat.NFSERR_OLD_STATEID; } else { res.tsr_resok4.tsr_status_codes[i] = nfsstat.NFS_OK; diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationWRITE.java b/core/src/main/java/org/dcache/nfs/v4/OperationWRITE.java index 27690b02..d1b8237e 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationWRITE.java +++ b/core/src/main/java/org/dcache/nfs/v4/OperationWRITE.java @@ -87,7 +87,8 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF } long offset = _args.opwrite.offset.value; - VirtualFileSystem.WriteResult writeResult = context.getFs().write(context.currentInode(), + VirtualFileSystem.WriteResult writeResult = context.getFs().write( + stateid.getOpaque(), context.currentInode(), _args.opwrite.data, offset, VirtualFileSystem.StabilityLevel.fromStableHow(_args.opwrite.stable)); if (writeResult.getBytesWritten() < 0) { diff --git a/core/src/main/java/org/dcache/nfs/v4/Stateids.java b/core/src/main/java/org/dcache/nfs/v4/Stateids.java index 01349320..1d41bc81 100644 --- a/core/src/main/java/org/dcache/nfs/v4/Stateids.java +++ b/core/src/main/java/org/dcache/nfs/v4/Stateids.java @@ -56,18 +56,18 @@ private Stateids() { } private final static stateid4 CURRENT_STATEID = - new stateid4(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 1); + stateid4.forBytes(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 1); private final static stateid4 INVAL_STATEID = - new stateid4(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, nfs4_prot.NFS4_UINT32_MAX); + stateid4.forBytes(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, nfs4_prot.NFS4_UINT32_MAX); private final static stateid4 ZERO_STATEID = - new stateid4(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 0); + stateid4.forBytes(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 0); - private final static stateid4 ONE_STATEID = new stateid4(new byte[] { + private final static stateid4 ONE_STATEID = stateid4.forBytes(new byte[] { (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff}, nfs4_prot.NFS4_UINT32_MAX); public static stateid4 uptodateOf(stateid4 stateid) { - return new stateid4(stateid.other, 0); + return stateid4.forBytes(stateid.getOpaque().toBytes(), 0); } public static stateid4 currentStateId() { @@ -91,16 +91,17 @@ public static boolean isStateLess(stateid4 stateid) { } public static void checkStateId(stateid4 expected, stateid4 stateid) throws ChimeraNFSException { - if (stateid.seqid == 0) { + int seq = stateid.getSeqId(); + if (seq == 0) { // so called 'most up-to-date seqid', see https://tools.ietf.org/html/rfc5661#section-8.2.2 return; } - if (expected.seqid > stateid.seqid) { - throw new OldStateidException(); - } + int expectedSeq = expected.getSeqId(); - if (expected.seqid < stateid.seqid) { + if (expectedSeq > seq) { + throw new OldStateidException(); + } else if (expectedSeq != seq) { throw new BadStateidException(); } } @@ -114,37 +115,37 @@ public static stateid4 getCurrentStateidIfNeeded(CompoundContext context, statei } public static void checkOpenStateid(stateid4 stateid) throws BadStateidException { - if (stateid.other[11] != OPEN_STATE_ID) { + if (stateid.getType() != OPEN_STATE_ID) { throw new BadStateidException("Not an open stateid"); } } public static void checkLockStateid(stateid4 stateid) throws BadStateidException { - if (stateid.other[11] != LOCK_STATE_ID) { + if (stateid.getType() != LOCK_STATE_ID) { throw new BadStateidException("Not a lock stateid"); } } public static void checkDelegationStateid(stateid4 stateid) throws BadStateidException { - if (stateid.other[11] != DELEGATION_STATE_ID) { + if (stateid.getType() != DELEGATION_STATE_ID) { throw new BadStateidException("Not a delegation stateid"); } } public static void checkDirDelegationStateid(stateid4 stateid) throws BadStateidException { - if (stateid.other[11] != DIR_DELEGATION_STATE_ID) { + if (stateid.getType() != DIR_DELEGATION_STATE_ID) { throw new BadStateidException("Not a directory delegation stateid"); } } public static void checkServerSiderCopyStateid(stateid4 stateid) throws BadStateidException { - if (stateid.other[11] != SSC_STATE_ID) { + if (stateid.getType() != SSC_STATE_ID) { throw new BadStateidException("Not a server-side copy stateid"); } } public static void checkLayoutStateid(stateid4 stateid) throws BadStateidException { - if (stateid.other[11] != LAYOUT_STATE_ID) { + if (stateid.getType() != LAYOUT_STATE_ID) { throw new BadStateidException("Not a layout stateid"); } } diff --git a/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java b/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java index 701775fe..eee985ad 100644 --- a/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java +++ b/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java @@ -21,45 +21,73 @@ import java.io.IOException; import java.io.Serializable; -import java.util.Arrays; +import org.dcache.nfs.util.Opaque; import org.dcache.oncrpc4j.rpc.OncRpcException; import org.dcache.oncrpc4j.xdr.XdrAble; import org.dcache.oncrpc4j.xdr.XdrDecodingStream; import org.dcache.oncrpc4j.xdr.XdrEncodingStream; -import com.google.common.io.BaseEncoding; - -public class stateid4 implements XdrAble, Serializable { +public class stateid4 implements XdrAble, Serializable, Cloneable { static final long serialVersionUID = -6677150504723505919L; - public int seqid; - public byte[] other; + private int seqid; + private final Opaque other; - public stateid4() { + public static stateid4 forBytes(byte[] bytes, int seqid) { + return new stateid4(seqid, Opaque.forBytes(bytes)); } - public stateid4(byte[] other, int seq) { - this.other = other; - seqid = seq; + public stateid4(XdrDecodingStream xdr) throws OncRpcException, IOException { + this.seqid = xdr.xdrDecodeInt(); + this.other = Opaque.forBytes(xdr.xdrDecodeOpaque(12)); } - public stateid4(XdrDecodingStream xdr) - throws OncRpcException, IOException { - xdrDecode(xdr); + @Deprecated(forRemoval = true) + public stateid4(byte[] bytes, int seqid) { + this(seqid, Opaque.forBytes(bytes)); + } + + private stateid4(int seqid, Opaque other) { + this.seqid = seqid; + this.other = other.toImmutableOpaque(); + } + + public Opaque getOpaque() { + return other; + } + + public int getSeqId() { + return seqid; + } + + public long getClientId() { + return other.longAt(0); + } + + public static long getClientId(Opaque stateIdOther) { + return stateIdOther.longAt(0); + } + + public int getType() { + return other.byteAt(11); + } + + @Override + public stateid4 clone() { + return new stateid4(seqid, other); } public void xdrEncode(XdrEncodingStream xdr) throws OncRpcException, IOException { xdr.xdrEncodeInt(seqid); - xdr.xdrEncodeOpaque(other, 12); + xdr.xdrEncodeOpaque(other.toBytes(), 12); } public void xdrDecode(XdrDecodingStream xdr) throws OncRpcException, IOException { - seqid = xdr.xdrDecodeInt(); - other = xdr.xdrDecodeOpaque(12); + throw new UnsupportedOperationException("Use constructor"); } @Override @@ -72,7 +100,7 @@ public boolean equals(Object obj) { final stateid4 other_id = (stateid4) obj; - return Arrays.equals(this.other, other_id.other); + return this.other.equals(other_id.other); } /** @@ -87,12 +115,12 @@ public boolean equalsWithSeq(stateid4 otherState) { return true; } - return otherState.seqid == this.seqid && Arrays.equals(this.other, otherState.other); + return otherState.seqid == this.seqid && this.other.equals(otherState.other); } @Override public int hashCode() { - return Arrays.hashCode(other); + return this.other.hashCode(); } @Override @@ -100,10 +128,13 @@ public String toString() { StringBuilder sb = new StringBuilder(); sb.append("["); - sb.append(BaseEncoding.base16().lowerCase().encode(other)); + sb.append(other); sb.append(", seq: ").append(seqid).append("]"); return sb.toString(); } + public void bumpSeqid() { + ++seqid; + } } // End of stateid4.java diff --git a/core/src/main/java/org/dcache/nfs/vfs/ForwardingFileSystem.java b/core/src/main/java/org/dcache/nfs/vfs/ForwardingFileSystem.java index c22e1108..00c1767b 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/ForwardingFileSystem.java +++ b/core/src/main/java/org/dcache/nfs/vfs/ForwardingFileSystem.java @@ -26,6 +26,7 @@ import javax.security.auth.Subject; +import org.dcache.nfs.util.Opaque; import org.dcache.nfs.v4.NfsIdMapping; import org.dcache.nfs.v4.xdr.nfsace4; import org.dcache.nfs.vfs.Stat.StatAttribute; @@ -107,8 +108,8 @@ public int read(Inode inode, ByteBuffer data, long offset) throws IOException { } @Override - public int read(Inode inode, ByteBuffer data, long offset, Runnable eofReached) throws IOException { - return delegate().read(inode, data, offset, eofReached); + public int read(Opaque stateId, Inode inode, ByteBuffer data, long offset, Runnable eofReached) throws IOException { + return delegate().read(stateId, inode, data, offset, eofReached); } @Override @@ -138,11 +139,22 @@ public WriteResult write(Inode inode, ByteBuffer data, long offset, StabilityLev return delegate().write(inode, data, offset, stabilityLevel); } + @Override + public WriteResult write(Opaque stateId, Inode inode, ByteBuffer data, long offset, StabilityLevel stabilityLevel) + throws IOException { + return delegate().write(stateId, inode, data, offset, stabilityLevel); + } + @Override public void commit(Inode inode, long offset, int count) throws IOException { delegate().commit(inode, offset, count); } + @Override + public void commit(Opaque stateId, Inode inode, long offset, int count) throws IOException { + delegate().commit(stateId, inode, offset, count); + } + @Override public Stat getattr(Inode inode) throws IOException { return delegate().getattr(inode); @@ -222,4 +234,14 @@ public void removeXattr(Inode inode, String attr) throws IOException { public CompletableFuture copyFileRange(Inode src, long srcPos, Inode dst, long dstPos, long len) { return delegate().copyFileRange(src, srcPos, dst, dstPos, len); } + + @Override + public void open(Opaque stateId, Inode inode, OpenMode openMode) { + delegate().open(stateId, inode, openMode); + } + + @Override + public void close(Opaque stateId) { + delegate().close(stateId); + } } diff --git a/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java b/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java index e5e26478..53098596 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java +++ b/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java @@ -71,10 +71,15 @@ import org.dcache.nfs.status.NoEntException; import org.dcache.nfs.status.PermException; import org.dcache.nfs.status.RoFsException; +import org.dcache.nfs.util.Opaque; import org.dcache.nfs.util.SubjectHolder; +import org.dcache.nfs.v4.NFS4Client; +import org.dcache.nfs.v4.NFSv4StateHandler; import org.dcache.nfs.v4.acl.Acls; import org.dcache.nfs.v4.xdr.acemask4; +import org.dcache.nfs.v4.xdr.nfs4_prot; import org.dcache.nfs.v4.xdr.nfsace4; +import org.dcache.nfs.v4.xdr.stateid4; import org.dcache.nfs.vfs.AclCheckable.Access; import org.dcache.nfs.vfs.Stat.StatAttribute; import org.dcache.oncrpc4j.rpc.RpcAuth; @@ -105,6 +110,7 @@ public class PseudoFs extends ForwardingFileSystem { private final VirtualFileSystem _inner; private final ExportTable _exportTable; private final RpcAuth _auth; + private final NFSv4StateHandler _stateHandler; private final static int ACCESS4_MASK = ACCESS4_DELETE | ACCESS4_EXECUTE | ACCESS4_EXTEND @@ -112,11 +118,16 @@ public class PseudoFs extends ForwardingFileSystem { | ACCESS4_XAREAD | ACCESS4_XAWRITE | ACCESS4_XALIST; public PseudoFs(VirtualFileSystem inner, RpcCall call, ExportTable exportTable) { + this(inner, call, exportTable, null); + } + + public PseudoFs(VirtualFileSystem inner, RpcCall call, ExportTable exportTable, NFSv4StateHandler stateHandler) { _inner = inner; _subject = call.getCredential().getSubject(); _auth = call.getCredential(); _inetAddress = call.getTransport().getRemoteSocketAddress(); _exportTable = exportTable; + _stateHandler = stateHandler; } @Override @@ -329,10 +340,29 @@ public int read(Inode inode, ByteBuffer data, long offset) throws IOException { return _inner.read(innerInode(inode), data, offset); } + private void checkAccessReadWriteData(Opaque stateId, Inode inode, boolean write) throws IOException { + if (_stateHandler != null) { + NFS4Client client = _stateHandler.getClientIfExists(stateid4.getClientId(stateId)); + if (client != null) { + int shareAccess; + try { + shareAccess = _stateHandler.getFileTracker().getShareAccess(client, inode, stateId); + if ((shareAccess & (write ? nfs4_prot.OPEN4_SHARE_ACCESS_WRITE + : nfs4_prot.OPEN4_SHARE_ACCESS_READ)) != 0) { + return; // permit + } + } catch (ChimeraNFSException e) { + // ignore; check below and throw proper exception + } + } + } + checkAccess(inode, write ? ACE4_WRITE_DATA : ACE4_READ_DATA); + } + @Override - public int read(Inode inode, ByteBuffer data, long offset, Runnable eofReached) throws IOException { - checkAccess(inode, ACE4_READ_DATA); - return _inner.read(innerInode(inode), data, offset, eofReached); + public int read(Opaque stateId, Inode inode, ByteBuffer data, long offset, Runnable eofReached) throws IOException { + checkAccessReadWriteData(stateId, inode, false); + return _inner.read(stateId, innerInode(inode), data, offset, eofReached); } @Override @@ -383,6 +413,13 @@ public WriteResult write(Inode inode, ByteBuffer data, long offset, StabilityLev return _inner.write(innerInode(inode), data, offset, stabilityLevel); } + @Override + public WriteResult write(Opaque stateId, Inode inode, ByteBuffer data, long offset, StabilityLevel stabilityLevel) + throws IOException { + checkAccessReadWriteData(stateId, inode, true); + return _inner.write(stateId, innerInode(inode), data, offset, stabilityLevel); + } + @Override public Stat getattr(Inode inode) throws IOException { checkAccess(inode, ACE4_READ_ATTRIBUTES); @@ -786,12 +823,12 @@ private boolean inheritUidGid(Inode inode) { } /** - * Convert an {@link Inode} that is used by {@link PseudoFs} to an {@link Inode} that is understood - * by the underlying file system. + * Convert an {@link Inode} that is used by {@link PseudoFs} to an {@link Inode} that is understood by the + * underlying file system. *

* We currently store additional information such as "ExportId" and "PseudoInode" as parts of the Inode. - * {@link VirtualFileSystem}s that store {@link Inode} as a whole (rather than only the "fileId" bit) - * may not recognize such objects, unless we remove this additional information. + * {@link VirtualFileSystem}s that store {@link Inode} as a whole (rather than only the "fileId" bit) may not + * recognize such objects, unless we remove this additional information. *

* Once {@link PseudoFs} handles this configuration internally, we can remove this conversion step. * diff --git a/core/src/main/java/org/dcache/nfs/vfs/VirtualFileSystem.java b/core/src/main/java/org/dcache/nfs/vfs/VirtualFileSystem.java index fd1e778d..229e1880 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/VirtualFileSystem.java +++ b/core/src/main/java/org/dcache/nfs/vfs/VirtualFileSystem.java @@ -29,6 +29,7 @@ import org.dcache.nfs.status.InvalException; import org.dcache.nfs.status.IsDirException; import org.dcache.nfs.status.NotSuppException; +import org.dcache.nfs.util.Opaque; import org.dcache.nfs.v4.NfsIdMapping; import org.dcache.nfs.v4.xdr.nfsace4; import org.dcache.nfs.v4.xdr.stable_how4; @@ -226,6 +227,7 @@ default int read(Inode inode, ByteBuffer data, long offset) throws IOException { * information via {@link #getattr(Inode)}), which may incur a higher, recurring cost than the inconvenience of a * single additional client roundtrip at the end of the file. * + * @param stateId The stateId as obtained via an {@code open} command via NFSv4, or {@code null}. * @param inode inode of the file to read from. * @param data byte array for writing. * @param offset file's position to read from. @@ -234,7 +236,8 @@ default int read(Inode inode, ByteBuffer data, long offset) throws IOException { * @return number of bytes read from the file, possibly zero. -1 if EOF is reached. * @throws IOException */ - default int read(Inode inode, ByteBuffer data, long offset, Runnable eofReached) throws IOException { + default int read(Opaque stateId, Inode inode, ByteBuffer data, long offset, Runnable eofReached) + throws IOException { Stat stat = getattr(inode); Stat.Type statType = stat.type(); @@ -317,6 +320,22 @@ default WriteResult write(Inode inode, ByteBuffer data, long offset, StabilityLe return write(inode, bytes, offset, count, stabilityLevel); } + /** + * Write provided {@code data} into inode with a given stability level. + * + * @param stateId The stateId as obtained via an {@code open} command via NFSv4, or {@code null}. + * @param inode inode of the file to write. + * @param data data to be written. + * @param offset the file position to begin writing at. + * @param stabilityLevel data stability level. + * @return write result. + * @throws IOException + */ + default WriteResult write(Opaque stateId, Inode inode, ByteBuffer data, long offset, StabilityLevel stabilityLevel) + throws IOException { + return write(inode, data, offset, stabilityLevel); + } + /** * Flush data in {@code dirty} state to the stable storage. Typically follows * {@link #write(Inode, ByteBuffer, long, StabilityLevel)} operation. @@ -328,6 +347,20 @@ default WriteResult write(Inode inode, ByteBuffer data, long offset, StabilityLe */ void commit(Inode inode, long offset, int count) throws IOException; + /** + * Flush data in {@code dirty} state to the stable storage. Typically follows + * {@link #write(Inode, ByteBuffer, long, StabilityLevel)} operation. + * + * @param stateId The stateId as obtained via an {@code open} command via NFSv4, or {@code null}. + * @param inode inode of the file to commit. + * @param offset the file position to start commit at. + * @param count number of bytes to commit. + * @throws IOException + */ + default void commit(Opaque stateId, Inode inode, long offset, int count) throws IOException { + commit(inode, offset, count); + } + /** * Get file system object's attributes. * @@ -444,6 +477,18 @@ public StabilityLevel getStabilityLevel() { } } + enum OpenMode { + READ_ONLY, WRITE_ONLY, READ_WRITE; + + boolean canRead() { + return this == READ_ONLY || this == READ_WRITE; + } + + boolean canWrite() { + return this == WRITE_ONLY || this == READ_WRITE; + } + } + // NOTE - stability values and ordinals are the same for nfs 3 and 4 /** * Write stability level. @@ -588,4 +633,25 @@ default CompletableFuture copyFileRange(Inode src, long srcPos, Inode dst, default Inode inodeForNfsHandle(byte[] bytes) throws IOException { return Inode.forNfsHandle(bytes); } + + /** + * Notifies the filing system about an explicit open operation to an {@link Inode}, using the given open mode and + * the specified stateId as a reference for {@link #read(Opaque, Inode, ByteBuffer, long, Runnable)}, + * {@link #write(Opaque, Inode, ByteBuffer, long, StabilityLevel)} and {@link #commit(Opaque, Inode, long, int)}. + * + * @param stateId The stateId. + * @param inode The {@link Inode}. + * @param openMode The open mode. + */ + default void open(Opaque stateId, Inode inode, OpenMode openMode) { + } + + /** + * Notifies the filing system about an explicit close operation corresponding to the given stateId. + * + * @param stateId The stateId. + * @see #open(Opaque, Inode, OpenMode) + */ + default void close(Opaque stateId) { + } } diff --git a/core/src/test/java/org/dcache/nfs/v4/OperationREADTest.java b/core/src/test/java/org/dcache/nfs/v4/OperationREADTest.java index 7d4f47fd..12dbed83 100644 --- a/core/src/test/java/org/dcache/nfs/v4/OperationREADTest.java +++ b/core/src/test/java/org/dcache/nfs/v4/OperationREADTest.java @@ -1,8 +1,16 @@ package org.dcache.nfs.v4; -import static org.dcache.nfs.v4.NfsTestUtils.*; +import static org.dcache.nfs.v4.NfsTestUtils.execute; import static org.dcache.nfs.v4.NfsTestUtils.generateRpcCall; -import static org.mockito.Mockito.*; +import static org.dcache.nfs.v4.NfsTestUtils.generateStateId; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import java.io.IOException; import java.net.UnknownHostException; @@ -54,7 +62,7 @@ public void testLeaseUpdateForV40Client() throws UnknownHostException, ChimeraNF FileTracker fileTracker = mock(FileTracker.class); when(stateHandler.getFileTracker()).thenReturn(fileTracker); - when(fileTracker.getShareAccess(any(), any(), any())).thenReturn(nfs4_prot.OPEN4_SHARE_ACCESS_READ); + when(fileTracker.getShareAccess(any(), any(), (stateid4) any())).thenReturn(nfs4_prot.OPEN4_SHARE_ACCESS_READ); when(stateHandler.getClientIdByStateId(any())).thenReturn(client); when(session.getClient()).thenReturn(client); when(stateHandler.getClientIdByStateId(any())).thenReturn(client); @@ -90,7 +98,7 @@ public void testNoLeaseUpdateForV41Client() throws UnknownHostException, Chimera FileTracker fileTracker = mock(FileTracker.class); when(stateHandler.getFileTracker()).thenReturn(fileTracker); - when(fileTracker.getShareAccess(any(), any(), any())).thenReturn(nfs4_prot.OPEN4_SHARE_ACCESS_READ); + when(fileTracker.getShareAccess(any(), any(), (stateid4) any())).thenReturn(nfs4_prot.OPEN4_SHARE_ACCESS_READ); when(stateHandler.getClientIdByStateId(any())).thenReturn(client); when(session.getClient()).thenReturn(client); when(stateHandler.getClientIdByStateId(any())).thenReturn(client); diff --git a/core/src/test/java/org/dcache/nfs/v4/OperationWRITETest.java b/core/src/test/java/org/dcache/nfs/v4/OperationWRITETest.java index b02bfa48..35afe6b2 100644 --- a/core/src/test/java/org/dcache/nfs/v4/OperationWRITETest.java +++ b/core/src/test/java/org/dcache/nfs/v4/OperationWRITETest.java @@ -1,9 +1,16 @@ package org.dcache.nfs.v4; -import static org.dcache.nfs.v4.NfsTestUtils.*; +import static org.dcache.nfs.v4.NfsTestUtils.execute; import static org.dcache.nfs.v4.NfsTestUtils.generateRpcCall; -import static org.junit.Assert.*; -import static org.mockito.Mockito.*; +import static org.dcache.nfs.v4.NfsTestUtils.generateStateId; +import static org.junit.Assert.assertSame; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import java.io.IOException; import java.net.UnknownHostException; @@ -57,14 +64,14 @@ public void testLeaseUpdateForV40Client() throws UnknownHostException, ChimeraNF FileTracker fileTracker = mock(FileTracker.class); when(stateHandler.getFileTracker()).thenReturn(fileTracker); - when(fileTracker.getShareAccess(any(), any(), any())).thenReturn(nfs4_prot.OPEN4_SHARE_ACCESS_WRITE); + when(fileTracker.getShareAccess(any(), any(), (stateid4) any())).thenReturn(nfs4_prot.OPEN4_SHARE_ACCESS_WRITE); when(stateHandler.getClientIdByStateId(any())).thenReturn(client); when(session.getClient()).thenReturn(client); when(stateHandler.getClientIdByStateId(any())).thenReturn(client); when(vfs.getattr(any())).thenReturn(fileStat); when(vfs.getattr(any(), any())).thenCallRealMethod(); - when(vfs.write(any(), any(), anyLong(), any())) + when(vfs.write(any(), any(), any(), anyLong(), any())) .thenReturn(new VirtualFileSystem.WriteResult(VirtualFileSystem.StabilityLevel.UNSTABLE, 1)); COMPOUND4args writeArgs = new CompoundBuilder() @@ -94,14 +101,14 @@ public void testNoLeaseUpdateForV41Client() throws UnknownHostException, Chimera FileTracker fileTracker = mock(FileTracker.class); when(stateHandler.getFileTracker()).thenReturn(fileTracker); - when(fileTracker.getShareAccess(any(), any(), any())).thenReturn(nfs4_prot.OPEN4_SHARE_ACCESS_WRITE); + when(fileTracker.getShareAccess(any(), any(), (stateid4) any())).thenReturn(nfs4_prot.OPEN4_SHARE_ACCESS_WRITE); when(stateHandler.getClientIdByStateId(any())).thenReturn(client); when(session.getClient()).thenReturn(client); when(stateHandler.getClientIdByStateId(any())).thenReturn(client); when(vfs.getattr(any())).thenReturn(fileStat); when(vfs.getattr(any(), any())).thenCallRealMethod(); - when(vfs.write(any(), any(), anyLong(), any())) + when(vfs.write(any(), any(), any(), anyLong(), any())) .thenReturn(new VirtualFileSystem.WriteResult(VirtualFileSystem.StabilityLevel.UNSTABLE, 1)); COMPOUND4args writeArgs = new CompoundBuilder() @@ -131,7 +138,7 @@ public void testReturnWriteVerifier() throws UnknownHostException, ChimeraNFSExc FileTracker fileTracker = mock(FileTracker.class); when(stateHandler.getFileTracker()).thenReturn(fileTracker); - when(fileTracker.getShareAccess(any(), any(), any())).thenReturn(nfs4_prot.OPEN4_SHARE_ACCESS_WRITE); + when(fileTracker.getShareAccess(any(), any(), (stateid4) any())).thenReturn(nfs4_prot.OPEN4_SHARE_ACCESS_WRITE); when(stateHandler.getClientIdByStateId(any())).thenReturn(client); when(session.getClient()).thenReturn(client); when(stateHandler.getClientIdByStateId(any())).thenReturn(client); @@ -140,7 +147,7 @@ public void testReturnWriteVerifier() throws UnknownHostException, ChimeraNFSExc when(vfs.getattr(any())).thenReturn(fileStat); when(vfs.getattr(any(), any())).thenCallRealMethod(); - when(vfs.write(any(), any(), anyLong(), any())) + when(vfs.write(any(), any(), any(), anyLong(), any())) .thenReturn(new VirtualFileSystem.WriteResult(VirtualFileSystem.StabilityLevel.UNSTABLE, 1)); COMPOUND4args writeArgs = new CompoundBuilder() diff --git a/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java b/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java index 61e0bd05..a2694bd4 100644 --- a/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java +++ b/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java @@ -19,10 +19,9 @@ */ package org.dcache.nfs.v4.xdr; -import static org.junit.Assert.*; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; -import org.dcache.nfs.v4.xdr.stateid4; -import org.dcache.nfs.v4.xdr.uint32_t; import org.junit.Test; public class stateid4Test { @@ -30,13 +29,9 @@ public class stateid4Test { @Test public void testEqualsTrue() { - stateid4 stateidA = new stateid4(); - stateidA.seqid = 1; - stateidA.other = "state".getBytes(); + stateid4 stateidA = stateid4.forBytes("state".getBytes(), 1); - stateid4 stateidB = new stateid4(); - stateidB.seqid = 1; - stateidB.other = "state".getBytes(); + stateid4 stateidB = stateid4.forBytes("state".getBytes(), 1); assertTrue("equal keys not equal", stateidA.equals(stateidB)); assertTrue("equal, but different hashCode", stateidA.hashCode() == stateidB.hashCode()); @@ -46,9 +41,7 @@ public void testEqualsTrue() { @Test public void testEqualsSame() { - stateid4 stateidA = new stateid4(); - stateidA.seqid = 1; - stateidA.other = "state".getBytes(); + stateid4 stateidA = stateid4.forBytes("state".getBytes(), 1); assertTrue("equal keys not equal", stateidA.equals(stateidA)); } @@ -56,13 +49,9 @@ public void testEqualsSame() { @Test public void testDifferSequence() { - stateid4 stateidA = new stateid4(); - stateidA.seqid = 1; - stateidA.other = "state".getBytes(); + stateid4 stateidA = stateid4.forBytes("state".getBytes(), 1); - stateid4 stateidB = new stateid4(); - stateidB.seqid = 2; - stateidB.other = "state".getBytes(); + stateid4 stateidB = stateid4.forBytes("state".getBytes(), 2); assertTrue("differ by sequence should still be equal", stateidA.equals(stateidB)); assertFalse("differ by sequence can't be equal", stateidA.equalsWithSeq(stateidB)); @@ -71,13 +60,9 @@ public void testDifferSequence() { @Test public void testDifferOther() { - stateid4 stateidA = new stateid4(); - stateidA.seqid = 1; - stateidA.other = "stateA".getBytes(); + stateid4 stateidA = stateid4.forBytes("stateA".getBytes(), 1); - stateid4 stateidB = new stateid4(); - stateidB.seqid = 1; - stateidB.other = "stateB".getBytes(); + stateid4 stateidB = stateid4.forBytes("stateB".getBytes(), 1); assertFalse("differ by other not detected", stateidA.equals(stateidB)); } From 3007d90c7f7f5ef210c55b3a9967de54b57c302a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 16:14:15 +0200 Subject: [PATCH 04/12] core: Fix NPE in OperationCLOSE when running maven tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit context.getFs() may be null upon close... Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/v4/OperationCLOSE.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationCLOSE.java b/core/src/main/java/org/dcache/nfs/v4/OperationCLOSE.java index 429ff266..82504ed8 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationCLOSE.java +++ b/core/src/main/java/org/dcache/nfs/v4/OperationCLOSE.java @@ -29,6 +29,7 @@ import org.dcache.nfs.v4.xdr.nfs_resop4; import org.dcache.nfs.v4.xdr.stateid4; import org.dcache.nfs.vfs.Inode; +import org.dcache.nfs.vfs.VirtualFileSystem; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -59,7 +60,10 @@ public void process(CompoundContext context, nfs_resop4 result) NFS4State nfsState = client.state(stateid); Stateids.checkStateId(nfsState.stateid(), stateid); - context.getFs().close(stateid.getOpaque()); + VirtualFileSystem fs = context.getFs(); + if (fs != null) { + fs.close(stateid.getOpaque()); + } if (context.getMinorversion() == 0) { nfsState.getStateOwner().acceptAsNextSequence(_args.opclose.seqid); From 6d67cd226ca6706aa1601b41ec5043c8688ace98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 16:44:45 +0200 Subject: [PATCH 05/12] core: stateid4: Restore Serialization compatibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apparently stateid4 needs to be serialized for dcache, so let's make sure this works again. Signed-off-by: Christian Kohlschütter --- .../java/org/dcache/nfs/v4/xdr/stateid4.java | 43 +++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java b/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java index eee985ad..e5ce87a3 100644 --- a/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java +++ b/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java @@ -20,6 +20,8 @@ package org.dcache.nfs.v4.xdr; import java.io.IOException; +import java.io.ObjectInputStream.GetField; +import java.io.ObjectOutputStream.PutField; import java.io.Serializable; import org.dcache.nfs.util.Opaque; @@ -33,7 +35,9 @@ public class stateid4 implements XdrAble, Serializable, Cloneable { static final long serialVersionUID = -6677150504723505919L; private int seqid; - private final Opaque other; + @SuppressWarnings("unused") + private byte[] other; // only declared for Java Serialization + private transient Opaque opaque; public static stateid4 forBytes(byte[] bytes, int seqid) { return new stateid4(seqid, Opaque.forBytes(bytes)); @@ -41,7 +45,7 @@ public static stateid4 forBytes(byte[] bytes, int seqid) { public stateid4(XdrDecodingStream xdr) throws OncRpcException, IOException { this.seqid = xdr.xdrDecodeInt(); - this.other = Opaque.forBytes(xdr.xdrDecodeOpaque(12)); + this.opaque = Opaque.forBytes(xdr.xdrDecodeOpaque(12)); } @Deprecated(forRemoval = true) @@ -51,11 +55,11 @@ public stateid4(byte[] bytes, int seqid) { private stateid4(int seqid, Opaque other) { this.seqid = seqid; - this.other = other.toImmutableOpaque(); + this.opaque = other.toImmutableOpaque(); } public Opaque getOpaque() { - return other; + return opaque; } public int getSeqId() { @@ -63,7 +67,7 @@ public int getSeqId() { } public long getClientId() { - return other.longAt(0); + return opaque.longAt(0); } public static long getClientId(Opaque stateIdOther) { @@ -71,18 +75,18 @@ public static long getClientId(Opaque stateIdOther) { } public int getType() { - return other.byteAt(11); + return opaque.byteAt(11); } @Override public stateid4 clone() { - return new stateid4(seqid, other); + return new stateid4(seqid, opaque); } public void xdrEncode(XdrEncodingStream xdr) throws OncRpcException, IOException { xdr.xdrEncodeInt(seqid); - xdr.xdrEncodeOpaque(other.toBytes(), 12); + xdr.xdrEncodeOpaque(opaque.toBytes(), 12); } public void xdrDecode(XdrDecodingStream xdr) @@ -100,7 +104,7 @@ public boolean equals(Object obj) { final stateid4 other_id = (stateid4) obj; - return this.other.equals(other_id.other); + return this.opaque.equals(other_id.opaque); } /** @@ -115,12 +119,12 @@ public boolean equalsWithSeq(stateid4 otherState) { return true; } - return otherState.seqid == this.seqid && this.other.equals(otherState.other); + return otherState.seqid == this.seqid && this.opaque.equals(otherState.opaque); } @Override public int hashCode() { - return this.other.hashCode(); + return this.opaque.hashCode(); } @Override @@ -128,7 +132,7 @@ public String toString() { StringBuilder sb = new StringBuilder(); sb.append("["); - sb.append(other); + sb.append(opaque); sb.append(", seq: ").append(seqid).append("]"); return sb.toString(); } @@ -136,5 +140,20 @@ public String toString() { public void bumpSeqid() { ++seqid; } + + private void writeObject(java.io.ObjectOutputStream out) + throws IOException { + PutField pf = out.putFields(); + pf.put("seqid", this.seqid); + pf.put("other", this.opaque.toBytes()); + out.writeFields(); + } + + private void readObject(java.io.ObjectInputStream in) + throws IOException, ClassNotFoundException { + GetField gf = in.readFields(); + this.seqid = gf.get("seqid", 0); + this.opaque = Opaque.forBytes((byte[]) gf.get("other", new byte[0])); + } } // End of stateid4.java From 5db35a09344338738a08f7853b990d1d64d01f60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 16:47:38 +0200 Subject: [PATCH 06/12] core: stateid4: Bring back xdrDecode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As per reviewer comment, this may still be used somewhere. Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java b/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java index e5ce87a3..1d9aca82 100644 --- a/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java +++ b/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java @@ -91,7 +91,8 @@ public void xdrEncode(XdrEncodingStream xdr) public void xdrDecode(XdrDecodingStream xdr) throws OncRpcException, IOException { - throw new UnsupportedOperationException("Use constructor"); + seqid = xdr.xdrDecodeInt(); + opaque = Opaque.forBytes(xdr.xdrDecodeOpaque(12)); } @Override From db14099dc534096bdb0e53bf4482e36f433c3f21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 17:02:50 +0200 Subject: [PATCH 07/12] core: FileTracker: Use stateid4.getType instead of byteAt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the introduction of stateid4.getType, let's not make any further assumptions about how the type is stored in the Opaque. Add a static getType variant in stateid4 and call that instead of Opaque.byteAt. Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/v4/FileTracker.java | 2 +- core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java b/core/src/main/java/org/dcache/nfs/v4/FileTracker.java index 7b511ea7..daec213d 100644 --- a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java +++ b/core/src/main/java/org/dcache/nfs/v4/FileTracker.java @@ -475,7 +475,7 @@ public int getShareAccess(NFS4Client client, Inode inode, Opaque stateid) lock.lock(); try { - switch (stateid.byteAt(11)) { + switch (stateid4.getType(stateid)) { case Stateids.LOCK_STATE_ID: NFS4State lockState = client.state(stateid); stateid = lockState.getOpenState().stateid().getOpaque(); diff --git a/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java b/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java index 1d9aca82..c675b659 100644 --- a/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java +++ b/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java @@ -78,6 +78,10 @@ public int getType() { return opaque.byteAt(11); } + public static int getType(Opaque stateIdOther) { + return stateIdOther.byteAt(11); + } + @Override public stateid4 clone() { return new stateid4(seqid, opaque); From dd506c6754b139175f6dd5176499c9bbdf7579e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 17:35:33 +0200 Subject: [PATCH 08/12] core: Undo stateid4.forBytes change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to reduce the amount of changes in #162 Signed-off-by: Christian Kohlschütter --- .../java/org/dcache/nfs/v4/NFSv4StateHandler.java | 2 +- core/src/main/java/org/dcache/nfs/v4/Stateids.java | 10 +++++----- .../main/java/org/dcache/nfs/v4/xdr/stateid4.java | 5 ----- .../java/org/dcache/nfs/v4/xdr/stateid4Test.java | 14 +++++++------- 4 files changed, 13 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/dcache/nfs/v4/NFSv4StateHandler.java b/core/src/main/java/org/dcache/nfs/v4/NFSv4StateHandler.java index 76a9ac64..afc62a6a 100644 --- a/core/src/main/java/org/dcache/nfs/v4/NFSv4StateHandler.java +++ b/core/src/main/java/org/dcache/nfs/v4/NFSv4StateHandler.java @@ -481,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 stateid4.forBytes(other, STATE_INITIAL_SEQUENCE); + return new stateid4(other, STATE_INITIAL_SEQUENCE); } /** diff --git a/core/src/main/java/org/dcache/nfs/v4/Stateids.java b/core/src/main/java/org/dcache/nfs/v4/Stateids.java index 1d41bc81..51e8fa07 100644 --- a/core/src/main/java/org/dcache/nfs/v4/Stateids.java +++ b/core/src/main/java/org/dcache/nfs/v4/Stateids.java @@ -56,18 +56,18 @@ private Stateids() { } private final static stateid4 CURRENT_STATEID = - stateid4.forBytes(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 1); + new stateid4(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 1); private final static stateid4 INVAL_STATEID = - stateid4.forBytes(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, nfs4_prot.NFS4_UINT32_MAX); + new stateid4(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, nfs4_prot.NFS4_UINT32_MAX); private final static stateid4 ZERO_STATEID = - stateid4.forBytes(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 0); + new stateid4(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 0); - private final static stateid4 ONE_STATEID = stateid4.forBytes(new byte[] { + private final static stateid4 ONE_STATEID = new stateid4(new byte[] { (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff}, nfs4_prot.NFS4_UINT32_MAX); public static stateid4 uptodateOf(stateid4 stateid) { - return stateid4.forBytes(stateid.getOpaque().toBytes(), 0); + return new stateid4(stateid.getOpaque().toBytes(), 0); } public static stateid4 currentStateId() { diff --git a/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java b/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java index c675b659..90422848 100644 --- a/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java +++ b/core/src/main/java/org/dcache/nfs/v4/xdr/stateid4.java @@ -39,16 +39,11 @@ public class stateid4 implements XdrAble, Serializable, Cloneable { private byte[] other; // only declared for Java Serialization private transient Opaque opaque; - public static stateid4 forBytes(byte[] bytes, int seqid) { - return new stateid4(seqid, Opaque.forBytes(bytes)); - } - public stateid4(XdrDecodingStream xdr) throws OncRpcException, IOException { this.seqid = xdr.xdrDecodeInt(); this.opaque = Opaque.forBytes(xdr.xdrDecodeOpaque(12)); } - @Deprecated(forRemoval = true) public stateid4(byte[] bytes, int seqid) { this(seqid, Opaque.forBytes(bytes)); } diff --git a/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java b/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java index a2694bd4..f81b19ca 100644 --- a/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java +++ b/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java @@ -29,9 +29,9 @@ public class stateid4Test { @Test public void testEqualsTrue() { - stateid4 stateidA = stateid4.forBytes("state".getBytes(), 1); + stateid4 stateidA = new stateid4("state".getBytes(), 1); - stateid4 stateidB = stateid4.forBytes("state".getBytes(), 1); + stateid4 stateidB = new stateid4("state".getBytes(), 1); assertTrue("equal keys not equal", stateidA.equals(stateidB)); assertTrue("equal, but different hashCode", stateidA.hashCode() == stateidB.hashCode()); @@ -41,7 +41,7 @@ public void testEqualsTrue() { @Test public void testEqualsSame() { - stateid4 stateidA = stateid4.forBytes("state".getBytes(), 1); + stateid4 stateidA = new stateid4("state".getBytes(), 1); assertTrue("equal keys not equal", stateidA.equals(stateidA)); } @@ -49,9 +49,9 @@ public void testEqualsSame() { @Test public void testDifferSequence() { - stateid4 stateidA = stateid4.forBytes("state".getBytes(), 1); + stateid4 stateidA = new stateid4("state".getBytes(), 1); - stateid4 stateidB = stateid4.forBytes("state".getBytes(), 2); + stateid4 stateidB = new stateid4("state".getBytes(), 2); assertTrue("differ by sequence should still be equal", stateidA.equals(stateidB)); assertFalse("differ by sequence can't be equal", stateidA.equalsWithSeq(stateidB)); @@ -60,9 +60,9 @@ public void testDifferSequence() { @Test public void testDifferOther() { - stateid4 stateidA = stateid4.forBytes("stateA".getBytes(), 1); + stateid4 stateidA = new stateid4("stateA".getBytes(), 1); - stateid4 stateidB = stateid4.forBytes("stateB".getBytes(), 1); + stateid4 stateidB = new stateid4("stateB".getBytes(), 1); assertFalse("differ by other not detected", stateidA.equals(stateidB)); } From 5d2e8437c35004f040a98f62866a478908091fba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 17:40:00 +0200 Subject: [PATCH 09/12] core: Revert unnecessary linewrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit to reduce the amount of changes in #162 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java b/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java index 53098596..697e3f7f 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java +++ b/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java @@ -823,12 +823,12 @@ private boolean inheritUidGid(Inode inode) { } /** - * Convert an {@link Inode} that is used by {@link PseudoFs} to an {@link Inode} that is understood by the - * underlying file system. + * Convert an {@link Inode} that is used by {@link PseudoFs} to an {@link Inode} that is understood + * by the underlying file system. *

* We currently store additional information such as "ExportId" and "PseudoInode" as parts of the Inode. - * {@link VirtualFileSystem}s that store {@link Inode} as a whole (rather than only the "fileId" bit) may not - * recognize such objects, unless we remove this additional information. + * {@link VirtualFileSystem}s that store {@link Inode} as a whole (rather than only the "fileId" bit) + * may not recognize such objects, unless we remove this additional information. *

* Once {@link PseudoFs} handles this configuration internally, we can remove this conversion step. * From c4510cf3daaa6988957390b17b4cb4347adad68a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 17:43:55 +0200 Subject: [PATCH 10/12] core: Undo some changes to Opaque MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit to reduce the amount of changes in #162 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/util/Opaque.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/dcache/nfs/util/Opaque.java b/core/src/main/java/org/dcache/nfs/util/Opaque.java index 344e5d33..41dc21f9 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -199,7 +199,7 @@ default void putBytes(ByteBuffer buf) { public class OpaqueImpl implements Opaque { final byte[] _opaque; - protected OpaqueImpl(byte[] opaque) { + OpaqueImpl(byte[] opaque) { _opaque = opaque; } @@ -392,13 +392,7 @@ public boolean equals(Object o) { } return true; } else { - Opaque other = (Opaque) o; - for (int i = index, n = index + length, oi = 0; i < n; i++, oi++) { - if (buf.get(i) != other.byteAt(oi)) { - return false; - } - } - return true; + return toImmutableOpaque().equals(o); } } From 02708e4136584d2e0581d4ea44a87ca90ed05e05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 18:13:27 +0200 Subject: [PATCH 11/12] core: Undo optimization in FileTracker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit to reduce the amount of changes in #162 We may also need the reference to Inode should we decide to call Vfs.close from here. Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/v4/FileTracker.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java b/core/src/main/java/org/dcache/nfs/v4/FileTracker.java index daec213d..81d31558 100644 --- a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java +++ b/core/src/main/java/org/dcache/nfs/v4/FileTracker.java @@ -341,8 +341,7 @@ 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); - Opaque fileIdKey = inode.getFileIdKey(); - state.addDisposeListener(s -> removeOpen(fileIdKey, stateid)); + state.addDisposeListener(s -> removeOpen(inode, stateid)); stateid.bumpSeqid(); // we need to return copy to avoid modification by concurrent opens @@ -532,10 +531,8 @@ public int getShareAccess(NFS4Client client, Inode inode, Opaque stateid) * @param stateid associated with the open. */ void removeOpen(Inode inode, stateid4 stateid) { - removeOpen(inode.getFileIdKey(), stateid); - } - void removeOpen(Opaque fileId, stateid4 stateid) { + Opaque fileId = inode.getFileIdKey(); Lock lock = filesLock.get(fileId); lock.lock(); try { From f5c9b9f6c9c5b1bdb995216593bebb9a5f04c831 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Mon, 14 Jul 2025 18:19:08 +0200 Subject: [PATCH 12/12] core: test: Undo some organize-imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit to reduce the amount of changes in #162 Signed-off-by: Christian Kohlschütter --- .../java/org/dcache/nfs/v4/OperationREADTest.java | 12 ++---------- .../java/org/dcache/nfs/v4/OperationWRITETest.java | 13 +++---------- .../java/org/dcache/nfs/v4/xdr/stateid4Test.java | 3 +-- 3 files changed, 6 insertions(+), 22 deletions(-) diff --git a/core/src/test/java/org/dcache/nfs/v4/OperationREADTest.java b/core/src/test/java/org/dcache/nfs/v4/OperationREADTest.java index 12dbed83..5c8cd101 100644 --- a/core/src/test/java/org/dcache/nfs/v4/OperationREADTest.java +++ b/core/src/test/java/org/dcache/nfs/v4/OperationREADTest.java @@ -1,16 +1,8 @@ package org.dcache.nfs.v4; -import static org.dcache.nfs.v4.NfsTestUtils.execute; +import static org.dcache.nfs.v4.NfsTestUtils.*; import static org.dcache.nfs.v4.NfsTestUtils.generateRpcCall; -import static org.dcache.nfs.v4.NfsTestUtils.generateStateId; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyInt; -import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; import java.io.IOException; import java.net.UnknownHostException; diff --git a/core/src/test/java/org/dcache/nfs/v4/OperationWRITETest.java b/core/src/test/java/org/dcache/nfs/v4/OperationWRITETest.java index 35afe6b2..90d07b77 100644 --- a/core/src/test/java/org/dcache/nfs/v4/OperationWRITETest.java +++ b/core/src/test/java/org/dcache/nfs/v4/OperationWRITETest.java @@ -1,16 +1,9 @@ package org.dcache.nfs.v4; -import static org.dcache.nfs.v4.NfsTestUtils.execute; +import static org.dcache.nfs.v4.NfsTestUtils.*; import static org.dcache.nfs.v4.NfsTestUtils.generateRpcCall; -import static org.dcache.nfs.v4.NfsTestUtils.generateStateId; -import static org.junit.Assert.assertSame; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.junit.Assert.*; +import static org.mockito.Mockito.*; import java.io.IOException; import java.net.UnknownHostException; diff --git a/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java b/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java index f81b19ca..b8989e3f 100644 --- a/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java +++ b/core/src/test/java/org/dcache/nfs/v4/xdr/stateid4Test.java @@ -19,8 +19,7 @@ */ package org.dcache.nfs.v4.xdr; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; import org.junit.Test;