From 28fcfe42feef5ceef22f122cc7bd220f656c08e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Thu, 26 Jun 2025 18:32:30 +0200 Subject: [PATCH 01/21] core: Allow reuse of "Opaque" file id key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, we were creating Opaque objects multiple times for the same file ID of an Inode. Let Inode return the "opaque" key: introduce a "getFileIdKey" method that always returns the same instance. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../java/org/dcache/nfs/v4/FileTracker.java | 10 +++---- .../main/java/org/dcache/nfs/vfs/Inode.java | 27 ++++++++++++++----- .../java/org/dcache/nfs/vfs/VfsCache.java | 4 +-- 3 files changed, 28 insertions(+), 13 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 267a6da2..2367e6a3 100644 --- a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java +++ b/core/src/main/java/org/dcache/nfs/v4/FileTracker.java @@ -242,7 +242,7 @@ public OpenRecord addOpen(NFS4Client client, StateOwner owner, Inode inode, int // client explicitly requested write delegation boolean wantWriteDelegation = (shareAccess & nfs4_prot.OPEN4_SHARE_ACCESS_WANT_WRITE_DELEG) != 0; - Opaque fileId = new Opaque(inode.getFileId()); + Opaque fileId = inode.getFileIdKey(); Lock lock = filesLock.get(fileId); lock.lock(); try { @@ -376,7 +376,7 @@ public OpenRecord addOpen(NFS4Client client, StateOwner owner, Inode inode, int public stateid4 downgradeOpen(NFS4Client client, stateid4 stateid, Inode inode, int shareAccess, int shareDeny) throws ChimeraNFSException { - Opaque fileId = new Opaque(inode.getFileId()); + Opaque fileId = inode.getFileIdKey(); Lock lock = filesLock.get(fileId); lock.lock(); try { @@ -426,7 +426,7 @@ public stateid4 downgradeOpen(NFS4Client client, stateid4 stateid, Inode inode, public void delegationReturn(NFS4Client client, stateid4 stateid, Inode inode) throws ChimeraNFSException { - Opaque fileId = new Opaque(inode.getFileId()); + Opaque fileId = inode.getFileIdKey(); Lock lock = filesLock.get(fileId); lock.lock(); try { @@ -464,7 +464,7 @@ public void delegationReturn(NFS4Client client, stateid4 stateid, Inode inode) public int getShareAccess(NFS4Client client, Inode inode, stateid4 stateid) throws ChimeraNFSException { - Opaque fileId = new Opaque(inode.getFileId()); + Opaque fileId = inode.getFileIdKey(); Lock lock = filesLock.get(fileId); lock.lock(); try { @@ -527,7 +527,7 @@ public int getShareAccess(NFS4Client client, Inode inode, stateid4 stateid) */ void removeOpen(Inode inode, stateid4 stateid) { - Opaque fileId = new Opaque(inode.getFileId()); + Opaque fileId = inode.getFileIdKey(); Lock lock = filesLock.get(fileId); lock.lock(); try { diff --git a/core/src/main/java/org/dcache/nfs/vfs/Inode.java b/core/src/main/java/org/dcache/nfs/vfs/Inode.java index 09f3d079..0136449c 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/Inode.java +++ b/core/src/main/java/org/dcache/nfs/vfs/Inode.java @@ -23,6 +23,8 @@ import java.nio.ByteOrder; import java.util.Arrays; +import org.dcache.nfs.util.Opaque; + import com.google.common.io.BaseEncoding; /** @@ -48,9 +50,10 @@ public class Inode { private final int generation; private final int exportIdx; private final int type; - private final byte[] fs_opaque; private final byte[] nfsHandle; + private final Opaque opaqueKey; + @Deprecated(forRemoval = true) public Inode(FileHandle fh) { this(fh.bytes()); @@ -71,9 +74,9 @@ public Inode(int generation, int exportIdx, int type, byte[] fs_opaque) { this.generation = generation; this.exportIdx = exportIdx; this.type = type; - this.fs_opaque = fs_opaque; + this.opaqueKey = new Opaque(fs_opaque.clone()); - this.nfsHandle = buildNfsHandle(); + this.nfsHandle = buildNfsHandle(fs_opaque); } /** @@ -106,8 +109,9 @@ public Inode(byte[] bytes) { exportIdx = b.getInt(); type = (int) b.get(); int olen = (int) b.get(); - fs_opaque = new byte[olen]; + byte[] fs_opaque = new byte[olen]; b.get(fs_opaque); + this.opaqueKey = new Opaque(fs_opaque); this.nfsHandle = bytes.clone(); } @@ -141,14 +145,25 @@ public static Inode forFile(byte[] bytes) { } public byte[] getFileId() { - return fs_opaque; + return opaqueKey.getOpaque().clone(); + } + + /** + * Returns a key suitable for identifying the underlying inode/file referred to by this instance, providing a + * {@link Object#equals(Object)} and {@link Object#hashCode()} implementation that may or may not be different from + * {@link Inode#equals(Object)} and {@link Inode#hashCode()}. + * + * @return The fileId key. + */ + public Opaque getFileIdKey() { + return opaqueKey; } public byte[] toNfsHandle() { return nfsHandle.clone(); } - private byte[] buildNfsHandle() { + private byte[] buildNfsHandle(byte[] fs_opaque) { int len = fs_opaque.length + MIN_LEN; byte[] bytes = new byte[len]; ByteBuffer b = ByteBuffer.wrap(bytes); diff --git a/core/src/main/java/org/dcache/nfs/vfs/VfsCache.java b/core/src/main/java/org/dcache/nfs/vfs/VfsCache.java index 60bde95b..2ccab223 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/VfsCache.java +++ b/core/src/main/java/org/dcache/nfs/vfs/VfsCache.java @@ -219,7 +219,7 @@ private void updateLookupCache(Inode parent, String path, Inode inode) { * @param inode The inode for which cached state value should be invalidated. */ public void invalidateStatCache(final Inode inode) { - _statCache.invalidate(new Opaque(inode.getFileId())); + _statCache.invalidate(inode.getFileIdKey()); } private void updateParentCache(Inode inode, Inode parent) { @@ -246,7 +246,7 @@ private Inode lookupFromCacheOrLoad(final Inode parent, final String path) throw private Stat statFromCacheOrLoad(final Inode inode) throws IOException { try { - return _statCache.get(new Opaque(inode.getFileId()), () -> _inner.getattr(inode)); + return _statCache.get(inode.getFileIdKey(), () -> _inner.getattr(inode)); } catch (ExecutionException e) { Throwable t = e.getCause(); Throwables.throwIfInstanceOf(t, IOException.class); From e0176ff70fd63f7b94849965e96a81a42d7936d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Thu, 26 Jun 2025 19:44:01 +0200 Subject: [PATCH 02/21] core: Reduce fileId byte[] usage; improve and encourage use of Opaque MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We currently use byte[] in many places where we actually convert them back into Base64 (and sometimes Base16) encoded strings, especially around locking. This is suboptimal. We also pass byte[] directly, which allows direct manipulation, which is also not good. Deprecate direct use of Inode.getFileId. Return an "Opaque" for locking purposes as well (but allow them to be separate from the Opaque fileId key to allow coarser locking). Provide default implementations of the Opaque-variants in LockManager. Changes to AbstractLockManager etc. follow in a separate commit. Change the Base16-encoded file names in FsCache to the same Base64-encoded locking key. Make Opaque immutable-ish (deprecate constructor and getOpaque() method that allows direct manipulation of the underlying byte[] but keep them in for now for backwards compatibility), and add a cached byte[]-to-Base64 string which is constructed only upon demand. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../main/java/org/dcache/nfs/util/Opaque.java | 25 +++++++++++++++++++ .../java/org/dcache/nfs/v4/OperationLOCK.java | 4 +-- .../org/dcache/nfs/v4/OperationLOCKT.java | 2 +- .../org/dcache/nfs/v4/OperationLOCKU.java | 2 +- .../org/dcache/nfs/v4/nlm/LockManager.java | 18 +++++++++++++ .../main/java/org/dcache/nfs/vfs/FsCache.java | 3 +-- .../main/java/org/dcache/nfs/vfs/Inode.java | 19 +++++++++++--- 7 files changed, 64 insertions(+), 9 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 dbf84bb0..7cb1365b 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -21,6 +21,7 @@ import java.io.Serializable; import java.util.Arrays; +import java.util.Base64; import com.google.common.io.BaseEncoding; @@ -32,15 +33,33 @@ public class Opaque implements Serializable { private static final long serialVersionUID = 1532238396149112674L; private final byte[] _opaque; + private String base64 = null; + public static Opaque forBytes(byte[] bytes) { + return new Opaque(bytes.clone()); + } + + @Deprecated(forRemoval = true) public Opaque(byte[] opaque) { _opaque = opaque; } + @Deprecated(forRemoval = true) public byte[] getOpaque() { return _opaque; } + public byte[] asBytes() { + return _opaque.clone(); + } + + public String getBase64() { + if (base64 == null) { + base64 = Base64.getEncoder().withoutPadding().encodeToString(_opaque); + } + return base64; + } + @Override public int hashCode() { return Arrays.hashCode(_opaque); @@ -58,6 +77,12 @@ public boolean equals(Object o) { return Arrays.equals(_opaque, ((Opaque) o)._opaque); } + /** + * Returns a (potentially non-stable) debug string. + * + * @see #getBase64() + */ + @Deprecated @Override public String toString() { StringBuilder sb = new StringBuilder(); 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 f410321a..d3b9575b 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationLOCK.java +++ b/core/src/main/java/org/dcache/nfs/v4/OperationLOCK.java @@ -118,11 +118,11 @@ 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.getFileId(), lock); + context.getLm().lock(inode.getLockKey(), lock); // ensure, that on close locks will be released lock_state.addDisposeListener(s -> { - context.getLm().unlockIfExists(inode.getFileId(), lock); + context.getLm().unlockIfExists(inode.getLockKey(), lock); }); // FIXME: we might run into race condition, thus updating sedid must be fenced! diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationLOCKT.java b/core/src/main/java/org/dcache/nfs/v4/OperationLOCKT.java index d4432b76..fc15e2ec 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationLOCKT.java +++ b/core/src/main/java/org/dcache/nfs/v4/OperationLOCKT.java @@ -85,7 +85,7 @@ public void process(CompoundContext context, nfs_resop4 result) throws IOExcepti NlmLock lock = new NlmLock(lockOwner, _args.oplockt.locktype, _args.oplockt.offset.value, _args.oplockt.length.value); - context.getLm().test(inode.getFileId(), lock); + context.getLm().test(inode.getLockKey(), lock); result.oplockt.status = nfsstat.NFS_OK; diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationLOCKU.java b/core/src/main/java/org/dcache/nfs/v4/OperationLOCKU.java index 601106f5..bbe7223f 100644 --- a/core/src/main/java/org/dcache/nfs/v4/OperationLOCKU.java +++ b/core/src/main/java/org/dcache/nfs/v4/OperationLOCKU.java @@ -76,7 +76,7 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF NlmLock lock = new NlmLock(lockOwner, _args.oplocku.locktype, _args.oplocku.offset.value, _args.oplocku.length.value); try { - context.getLm().unlock(inode.getFileId(), lock); + context.getLm().unlock(inode.getLockKey(), lock); } catch (LockRangeUnavailabeException e) { // posix locks allows unlocking of not locked regions } diff --git a/core/src/main/java/org/dcache/nfs/v4/nlm/LockManager.java b/core/src/main/java/org/dcache/nfs/v4/nlm/LockManager.java index ec3309d2..5899a382 100644 --- a/core/src/main/java/org/dcache/nfs/v4/nlm/LockManager.java +++ b/core/src/main/java/org/dcache/nfs/v4/nlm/LockManager.java @@ -19,6 +19,8 @@ */ package org.dcache.nfs.v4.nlm; +import org.dcache.nfs.util.Opaque; + /** */ public interface LockManager { @@ -33,6 +35,10 @@ public interface LockManager { */ void lock(byte[] objId, NlmLock lock) throws LockException; + default void lock(Opaque key, NlmLock lock) throws LockException { + lock(key.asBytes(), lock); + } + /** * Unlock byte range of an {@code objId}. * @@ -43,6 +49,10 @@ public interface LockManager { */ void unlock(byte[] objId, NlmLock lock) throws LockException; + default void unlock(Opaque key, NlmLock lock) throws LockException { + unlock(key.asBytes(), lock); + } + /** * Test byte range lock existence for an {@code objId}. Same as {@link #lock}, except that a new lock is not * created. @@ -54,6 +64,10 @@ public interface LockManager { */ void test(byte[] objId, NlmLock lock) throws LockException; + default void test(Opaque key, NlmLock lock) throws LockException { + test(key.asBytes(), lock); + } + /** * Like {@link #unlock(byte[], org.dcache.nfs.v4.nlm.NlmLock)}, but does not fail if lock does not exists. * @@ -61,4 +75,8 @@ public interface LockManager { * @param lock */ void unlockIfExists(byte[] objId, NlmLock lock); + + default void unlockIfExists(Opaque key, NlmLock lock) { + unlockIfExists(key.asBytes(), lock); + } } diff --git a/core/src/main/java/org/dcache/nfs/vfs/FsCache.java b/core/src/main/java/org/dcache/nfs/vfs/FsCache.java index 1dbebaf4..3b6a4e59 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/FsCache.java +++ b/core/src/main/java/org/dcache/nfs/vfs/FsCache.java @@ -52,8 +52,7 @@ private static class FileChannelSupplier extends CacheLoader @Override public FileChannel load(Inode inode) throws IOException { - byte[] fid = inode.getFileId(); - String id = BaseEncoding.base16().lowerCase().encode(fid); + String id = inode.getLockKey().getBase64(); File dir = getAndCreateDirectory(id); File f = new File(dir, id); return new RandomAccessFile(f, "rw").getChannel(); diff --git a/core/src/main/java/org/dcache/nfs/vfs/Inode.java b/core/src/main/java/org/dcache/nfs/vfs/Inode.java index 0136449c..52bb7b89 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/Inode.java +++ b/core/src/main/java/org/dcache/nfs/vfs/Inode.java @@ -74,7 +74,7 @@ public Inode(int generation, int exportIdx, int type, byte[] fs_opaque) { this.generation = generation; this.exportIdx = exportIdx; this.type = type; - this.opaqueKey = new Opaque(fs_opaque.clone()); + this.opaqueKey = Opaque.forBytes(fs_opaque); this.nfsHandle = buildNfsHandle(fs_opaque); } @@ -111,7 +111,7 @@ public Inode(byte[] bytes) { int olen = (int) b.get(); byte[] fs_opaque = new byte[olen]; b.get(fs_opaque); - this.opaqueKey = new Opaque(fs_opaque); + this.opaqueKey = Opaque.forBytes(fs_opaque); this.nfsHandle = bytes.clone(); } @@ -144,8 +144,21 @@ public static Inode forFile(byte[] bytes) { return new Inode(0, 0, 0, bytes); } + @Deprecated(forRemoval = true) public byte[] getFileId() { - return opaqueKey.getOpaque().clone(); + return opaqueKey.asBytes(); + } + + /** + * Returns a locking-key referring to the underlying inode/file referred to by this instance, or a superset, + * suitable for {@link org.dcache.nfs.v4.nlm.LockManager} etc. + *

+ * This may or may not be equal to {@link #getFileIdKey()}. + * + * @return The locking key for file referred to by this inode. + */ + public Opaque getLockKey() { + return opaqueKey; } /** From bcb3e17e4138c807dd4632783def7a612c44f860 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Thu, 26 Jun 2025 20:17:00 +0200 Subject: [PATCH 03/21] core: Prepare AbstractLockManager for Opaque support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add "Opaque" methods to AbstractLockManager, which in turn call their byte[]-counterpart. Add a temporary, non-public AbstractLockManager2, which reverses this logic. Also add some simple logging for deprecated calls into AbstractLockManager2 (which will be removed very soon). Mark all byte[]-variants deprecated for removal. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../nfs/v4/nlm/AbstractLockManager.java | 72 +++++- .../nfs/v4/nlm/AbstractLockManager2.java | 227 ++++++++++++++++++ 2 files changed, 297 insertions(+), 2 deletions(-) create mode 100644 core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager2.java diff --git a/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager.java b/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager.java index 97a172f8..c742d78e 100644 --- a/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager.java +++ b/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager.java @@ -26,6 +26,7 @@ import java.util.concurrent.locks.Lock; import java.util.stream.Collectors; +import org.dcache.nfs.util.Opaque; import org.dcache.nfs.v4.xdr.nfs4_prot; /** @@ -42,24 +43,57 @@ public abstract class AbstractLockManager implements LockManager { * @param objId object id. * @return exclusive lock. */ + @Deprecated(forRemoval = true) abstract protected Lock getObjectLock(byte[] objId); + /** + * Get exclusive lock on objects locks. + * + * @param objId object id. + * @return exclusive lock. + */ + protected Lock getObjectLock(Opaque objId) { + return getObjectLock(objId.asBytes()); + } + /** * Get collection of currently used active locks on the object. * * @param objId object id. * @return collection of active locks. */ + @Deprecated(forRemoval = true) abstract protected Collection getActiveLocks(byte[] objId); + /** + * Get collection of currently used active locks on the object. + * + * @param objId object id. + * @return collection of active locks. + */ + protected Collection getActiveLocks(Opaque objId) { + return getActiveLocks(objId.asBytes()); + } + /** * Add {@code lock} to an object. * * @param objId object id. * @param lock lock to add. */ + @Deprecated(forRemoval = true) abstract protected void add(byte[] objId, NlmLock lock); + /** + * Add {@code lock} to an object. + * + * @param objId object id. + * @param lock lock to add. + */ + protected void add(Opaque objId, NlmLock lock) { + add(objId.asBytes(), lock); + } + /** * Remove a lock for the given object. * @@ -67,24 +101,58 @@ public abstract class AbstractLockManager implements LockManager { * @param lock lock to remove. * @return true, if specified lock was removed. */ + @Deprecated(forRemoval = true) abstract protected boolean remove(byte[] objId, NlmLock lock); + /** + * Remove a lock for the given object. + * + * @param objId object id. + * @param lock lock to remove. + * @return true, if specified lock was removed. + */ + protected boolean remove(Opaque objId, NlmLock lock) { + return remove(objId.asBytes(), lock); + } + /** * Add all locks from a given collection of locks * - * @param objId - * @param locks + * @param objId object id. + * @param locks locks to add. */ + @Deprecated(forRemoval = true) abstract protected void addAll(byte[] objId, Collection locks); + /** + * Add all locks from a given collection of locks + * + * @param objId object id. + * @param locks locks to add. + */ + protected void addAll(Opaque objId, Collection locks) { + addAll(objId.asBytes(), locks); + } + /** * Remove all locks specified by {@code locks} associated with the given object. * * @param objId object id. * @param locks collections of locks to remove. */ + @Deprecated(forRemoval = true) abstract protected void removeAll(byte[] objId, Collection locks); + /** + * Remove all locks specified by {@code locks} associated with the given object. + * + * @param objId object id. + * @param locks collections of locks to remove. + */ + protected void removeAll(Opaque objId, Collection locks) { + removeAll(objId.asBytes(), locks); + } + @Override public void lock(byte[] objId, NlmLock lock) throws LockException { Lock dlmLock = getObjectLock(objId); diff --git a/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager2.java b/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager2.java new file mode 100644 index 00000000..8fdc7ede --- /dev/null +++ b/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager2.java @@ -0,0 +1,227 @@ +/* + * Copyright (c) 2017 Deutsches Elektronen-Synchroton, + * Member of the Helmholtz Association, (DESY), HAMBURG, GERMANY + * + * This library is free software; you can redistribute it and/or modify + * it under the terms of the GNU Library General Public License as + * published by the Free Software Foundation; either version 2 of the + * License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Library General Public License for more details. + * + * You should have received a copy of the GNU Library General Public + * License along with this program (see the file COPYING.LIB for more + * details); if not, write to the Free Software Foundation, Inc., + * 675 Mass Ave, Cambridge, MA 02139, USA. + */ +package org.dcache.nfs.v4.nlm; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.locks.Lock; +import java.util.stream.Collectors; + +import org.dcache.nfs.util.Opaque; +import org.dcache.nfs.v4.xdr.nfs4_prot; + +abstract class AbstractLockManager2 extends AbstractLockManager { + private static final boolean DEBUG_DEPRECATED_CALLS = false; + + @Override + @Deprecated(forRemoval = true) + protected final Lock getObjectLock(byte[] objId) { + throw new IllegalStateException(); + } + + @Override + protected abstract Lock getObjectLock(Opaque objId); + + @Override + @Deprecated(forRemoval = true) + protected final Collection getActiveLocks(byte[] objId) { + throw new IllegalStateException(); + } + + @Override + protected abstract Collection getActiveLocks(Opaque objId); + + @Override + @Deprecated(forRemoval = true) + protected final void add(byte[] objId, NlmLock lock) { + throw new IllegalStateException(); + } + + @Override + protected abstract void add(Opaque objId, NlmLock lock); + + @Override + @Deprecated(forRemoval = true) + protected final boolean remove(byte[] objId, NlmLock lock) { + throw new IllegalStateException(); + } + + @Override + protected abstract boolean remove(Opaque objId, NlmLock lock); + + @Override + @Deprecated(forRemoval = true) + protected final void addAll(byte[] objId, Collection locks) { + throw new IllegalStateException(); + } + + @Override + protected abstract void addAll(Opaque objId, Collection locks); + + @Override + @Deprecated(forRemoval = true) + protected final void removeAll(byte[] objId, Collection locks) { + throw new IllegalStateException(); + } + + @Override + protected abstract void removeAll(Opaque objId, Collection locks); + + @Override + @Deprecated(forRemoval = true) + public void lock(byte[] objId, NlmLock lock) throws LockException { + if (DEBUG_DEPRECATED_CALLS) { + new IllegalStateException("Called deprecated method").printStackTrace(); + } + lock(Opaque.forBytes(objId), lock); + } + + @Override + public void lock(Opaque objId, NlmLock lock) throws LockException { + Lock dlmLock = getObjectLock(objId); + dlmLock.lock(); + try { + Collection currentLocks = getActiveLocks(objId); + Optional conflictingLock = currentLocks.stream().filter((NlmLock l) -> l.isConflicting(lock)) + .findAny(); + if (conflictingLock.isPresent()) { + throw new LockDeniedException("object locked", conflictingLock.get()); + } + // no conflicting locks. try to merge existing locks + List toMerge = currentLocks.stream().filter((NlmLock l) -> l.isOverlappingRange(lock)).filter(( + NlmLock l) -> l.isSameOwner(lock)).filter((NlmLock l) -> l.getLockType() == lock.getLockType()) + .collect(Collectors.toList()); + if (toMerge.isEmpty()) { + add(objId, lock); + } else { + // merge overlaping/continues locks + long lockBegin = lock.getOffset(); + long lockEnd = lock.getLength() == nfs4_prot.NFS4_UINT64_MAX ? nfs4_prot.NFS4_UINT64_MAX : (lockBegin + + lock.getLength()); + for (NlmLock l : toMerge) { + lockBegin = Math.min(lockBegin, l.getOffset()); + lockEnd = lockEnd == nfs4_prot.NFS4_UINT64_MAX || l.getLength() == nfs4_prot.NFS4_UINT64_MAX + ? nfs4_prot.NFS4_UINT64_MAX : Math.max(lockEnd, l.getOffset() + l.getLength() - 1); + } + NlmLock mergedLock = new NlmLock(lock.getOwner(), lock.getLockType(), lockBegin, + lockEnd == nfs4_prot.NFS4_UINT64_MAX ? lockEnd : lockEnd - lockBegin); + removeAll(objId, toMerge); + add(objId, mergedLock); + } + } finally { + dlmLock.unlock(); + } + } + + @Override + @Deprecated(forRemoval = true) + public void unlock(byte[] objId, NlmLock lock) throws LockException { + if (DEBUG_DEPRECATED_CALLS) { + new IllegalStateException("Called deprecated method").printStackTrace(); + } + unlock(Opaque.forBytes(objId), lock); + } + + @Override + public void unlock(Opaque objId, NlmLock lock) throws LockException { + Lock dlmLock = getObjectLock(objId); + dlmLock.lock(); + try { + Collection currentLocks = getActiveLocks(objId); + // check for exact match first + if (remove(objId, lock)) { + return; + } + List toRemove = new ArrayList<>(); + List toAdd = new ArrayList<>(); + currentLocks.stream().filter((NlmLock l) -> l.isSameOwner(lock)).filter((NlmLock l) -> l.isOverlappingRange( + lock)).forEach((NlmLock l) -> { + toRemove.add(l); + long l1 = lock.getOffset() - l.getOffset(); + if (l1 > 0) { + NlmLock first = new NlmLock(l.getOwner(), l.getLockType(), l.getOffset(), l1); + toAdd.add(first); + } + if (lock.getLength() != nfs4_prot.NFS4_UINT64_MAX) { + long l2 = l.getLength() - l1 - 1; + if (l2 > 0) { + NlmLock second = new NlmLock(l.getOwner(), l.getLockType(), lock.getOffset() + lock + .getLength(), l2); + toAdd.add(second); + } + } + }); + if (toRemove.isEmpty()) { + throw new LockRangeUnavailabeException("no matching lock"); + } + removeAll(objId, toRemove); + addAll(objId, toAdd); + } finally { + dlmLock.unlock(); + } + } + + @Override + @Deprecated(forRemoval = true) + public void test(byte[] objId, NlmLock lock) throws LockException { + if (DEBUG_DEPRECATED_CALLS) { + new IllegalStateException("Called deprecated method").printStackTrace(); + } + test(Opaque.forBytes(objId), lock); + } + + @Override + public void test(Opaque objId, NlmLock lock) throws LockException { + Lock dlmLock = getObjectLock(objId); + dlmLock.lock(); + try { + Collection currentLocks = getActiveLocks(objId); + Optional conflictingLock = currentLocks.stream().filter((NlmLock l) -> l.isOverlappingRange(lock) + && !l.isSameOwner(lock)).findAny(); + if (conflictingLock.isPresent()) { + throw new LockDeniedException("object locked", conflictingLock.get()); + } + } finally { + dlmLock.unlock(); + } + } + + @Override + @Deprecated(forRemoval = true) + public void unlockIfExists(byte[] objId, NlmLock lock) { + if (DEBUG_DEPRECATED_CALLS) { + new IllegalStateException("Called deprecated method").printStackTrace(); + } + unlockIfExists(Opaque.forBytes(objId), lock); + } + + @Override + public void unlockIfExists(Opaque objId, NlmLock lock) { + Lock dlmLock = getObjectLock(objId); + dlmLock.lock(); + try { + remove(objId, lock); + } finally { + dlmLock.unlock(); + } + } +} From 7334be2b7632ad1a9c225511aff043230fc3e7e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Thu, 26 Jun 2025 20:20:00 +0200 Subject: [PATCH 04/21] core: Make SimpleLm use AbstractLockManager2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SimpleLm used to create new Base64-strings upon toKey(byte[]), which is suboptimal. Use Opaque.getBase64 instead. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../java/org/dcache/nfs/v4/nlm/SimpleLm.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java b/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java index 580fc23b..101d17e8 100644 --- a/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java +++ b/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java @@ -20,13 +20,14 @@ package org.dcache.nfs.v4.nlm; import java.util.ArrayList; -import java.util.Base64; import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.locks.Lock; +import org.dcache.nfs.util.Opaque; + import com.google.common.util.concurrent.Striped; /** @@ -34,7 +35,7 @@ * * @since 0.14 */ -public class SimpleLm extends AbstractLockManager { +public class SimpleLm extends AbstractLockManager2 { /* * Use {@link Striped} here to split synchronized block on file locks into multiple partitions to increase @@ -60,26 +61,26 @@ public SimpleLm(int concurrency) { private final ConcurrentHashMap> locks = new ConcurrentHashMap<>(); @Override - protected Lock getObjectLock(byte[] objId) { + protected Lock getObjectLock(Opaque objId) { String key = toKey(objId); return objLock.get(key); } @Override - protected Collection getActiveLocks(byte[] objId) { + protected Collection getActiveLocks(Opaque objId) { String key = toKey(objId); return locks.getOrDefault(key, Collections.emptyList()); } @Override - protected void add(byte[] objId, NlmLock lock) { + protected void add(Opaque objId, NlmLock lock) { String key = toKey(objId); Collection l = locks.computeIfAbsent(key, k -> new ArrayList<>()); l.add(lock); } @Override - protected boolean remove(byte[] objId, NlmLock lock) { + protected boolean remove(Opaque objId, NlmLock lock) { String key = toKey(objId); Collection l = locks.get(key); boolean isRemoved = false; @@ -93,14 +94,14 @@ protected boolean remove(byte[] objId, NlmLock lock) { } @Override - protected void addAll(byte[] objId, Collection locks) { + protected void addAll(Opaque objId, Collection locks) { String key = toKey(objId); Collection l = this.locks.computeIfAbsent(key, k -> new ArrayList<>()); l.addAll(locks); } @Override - protected void removeAll(byte[] objId, Collection locks) { + protected void removeAll(Opaque objId, Collection locks) { String key = toKey(objId); Collection l = this.locks.get(key); if (l != null) { @@ -111,8 +112,8 @@ protected void removeAll(byte[] objId, Collection locks) { } } - private final String toKey(byte[] objId) { - return Base64.getEncoder().encodeToString(objId); + private final String toKey(Opaque objId) { + return objId.getBase64(); } } From d3a5bcea9ca3d0641121b77cc54dc2bd18a8c8c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Thu, 26 Jun 2025 20:23:58 +0200 Subject: [PATCH 05/21] dlm: Make DistributedLockManager use AbstractLockManager2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DistributedLockManager used to create new Base64-strings upon objIdToKey(byte[]), which is suboptimal. Use Opaque.getBase64 instead. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../nfs/v4/nlm/DistributedLockManager.java | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java b/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java index ae60b7c8..c420006f 100644 --- a/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java +++ b/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java @@ -25,6 +25,8 @@ import java.util.concurrent.locks.Condition; import java.util.concurrent.locks.Lock; +import org.dcache.nfs.util.Opaque; + import com.hazelcast.core.HazelcastInstance; import com.hazelcast.multimap.MultiMap; @@ -45,7 +47,7 @@ * * @since 0.16 */ -public class DistributedLockManager extends AbstractLockManager { +public class DistributedLockManager extends AbstractLockManager2 { private final MultiMap locks; @@ -61,7 +63,7 @@ public DistributedLockManager(HazelcastInstance hz, String name) { } @Override - protected Lock getObjectLock(byte[] objId) { + protected Lock getObjectLock(Opaque objId) { String key = objIdToKey(objId); return new Lock() { @Override @@ -103,40 +105,37 @@ public Condition newCondition() { * @return collection of active locks. */ @Override - protected Collection getActiveLocks(byte[] objId) { + protected Collection getActiveLocks(Opaque objId) { String key = objIdToKey(objId); return locks.get(key); } @Override - protected void add(byte[] objId, NlmLock lock) { + protected void add(Opaque objId, NlmLock lock) { String key = objIdToKey(objId); locks.put(key, lock); } @Override - protected boolean remove(byte[] objId, NlmLock lock) { + protected boolean remove(Opaque objId, NlmLock lock) { String key = objIdToKey(objId); return locks.remove(key, lock); } @Override - protected void addAll(byte[] objId, Collection locks) { + protected void addAll(Opaque objId, Collection locks) { String key = objIdToKey(objId); locks.forEach(l -> this.locks.put(key, l)); } @Override - protected void removeAll(byte[] objId, Collection locks) { + protected void removeAll(Opaque objId, Collection locks) { String key = objIdToKey(objId); locks.forEach(l -> this.locks.remove(key, l)); } - private static String objIdToKey(byte[] objId) { - return Base64 - .getEncoder() - .withoutPadding() - .encodeToString(objId); + private static String objIdToKey(Opaque objId) { + return objId.getBase64(); } } From e1bcf35e6e151f172a3f110c470fc7fb46538b2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Thu, 26 Jun 2025 20:28:06 +0200 Subject: [PATCH 06/21] core: test: Switch SimpleLmTest to use Opaque instead of byte[] keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will change the test to use the new LockManager Opaque API. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../test/java/org/dcache/nfs/v4/nlm/SimpleLmTest.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/core/src/test/java/org/dcache/nfs/v4/nlm/SimpleLmTest.java b/core/src/test/java/org/dcache/nfs/v4/nlm/SimpleLmTest.java index 6bd5dd89..cb4fe772 100644 --- a/core/src/test/java/org/dcache/nfs/v4/nlm/SimpleLmTest.java +++ b/core/src/test/java/org/dcache/nfs/v4/nlm/SimpleLmTest.java @@ -2,6 +2,7 @@ import java.nio.charset.StandardCharsets; +import org.dcache.nfs.util.Opaque; import org.dcache.nfs.v4.StateOwner; import org.dcache.nfs.v4.xdr.clientid4; import org.dcache.nfs.v4.xdr.nfs4_prot; @@ -16,14 +17,14 @@ public class SimpleLmTest { private LockManager nlm; - private byte[] file1; - private byte[] file2; + private Opaque file1; + private Opaque file2; @Before public void setUp() throws Exception { nlm = new SimpleLm(); - file1 = "file1".getBytes(StandardCharsets.UTF_8); - file2 = "file2".getBytes(StandardCharsets.UTF_8); + file1 = Opaque.forBytes("file1".getBytes(StandardCharsets.UTF_8)); + file2 = Opaque.forBytes("file2".getBytes(StandardCharsets.UTF_8)); } @Test From f22dba3dbb9dcc9af0b9c1e4c40cf21bd320339b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Thu, 26 Jun 2025 20:28:54 +0200 Subject: [PATCH 07/21] dlm: test: Switch DistributedLockManagerTest to use Opaque MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will change the test to use the new LockManager Opaque API. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../org/dcache/nfs/v4/nlm/DistributedLockManagerTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dlm/src/test/java/org/dcache/nfs/v4/nlm/DistributedLockManagerTest.java b/dlm/src/test/java/org/dcache/nfs/v4/nlm/DistributedLockManagerTest.java index 1ff45092..92ab5c87 100644 --- a/dlm/src/test/java/org/dcache/nfs/v4/nlm/DistributedLockManagerTest.java +++ b/dlm/src/test/java/org/dcache/nfs/v4/nlm/DistributedLockManagerTest.java @@ -5,6 +5,7 @@ import java.nio.charset.StandardCharsets; import java.util.Arrays; +import org.dcache.nfs.util.Opaque; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -20,12 +21,12 @@ public class DistributedLockManagerTest { private HazelcastInstance hzClient; private LockManager lm1; private LockManager lm2; - private byte[] file1; + private Opaque file1; @Before public void setUp() throws Exception { - file1 = "file1".getBytes(StandardCharsets.UTF_8); + file1 = Opaque.forBytes("file1".getBytes(StandardCharsets.UTF_8)); hzSerrver = Hazelcast.newHazelcastInstance(); From 72b87c94782267604391f2f6cc4d1a06cc2c0785 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Thu, 26 Jun 2025 20:35:52 +0200 Subject: [PATCH 08/21] core: NFS4Client: Use Opaque.forBytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, we were using "new Opaque(bytes[])", which creates a new Opaque instance backed by the given byte[]. Since the Opaque key may be used in a "owners" hashmap, make sure that this information cannot be modified by the caller. Use the immutable "Opaque.forBytes" instead. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/v4/NFS4Client.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 6da6871e..4afbd04b 100644 --- a/core/src/main/java/org/dcache/nfs/v4/NFS4Client.java +++ b/core/src/main/java/org/dcache/nfs/v4/NFS4Client.java @@ -628,7 +628,7 @@ public boolean isCallbackNeede() { public synchronized StateOwner getOrCreateOwner(byte[] owner, seqid4 seq) throws BadSeqidException { StateOwner stateOwner; if (_minorVersion == 0) { - Opaque k = new Opaque(owner); + Opaque k = Opaque.forBytes(owner); stateOwner = _owners.get(k); if (stateOwner == null) { state_owner4 so = new state_owner4(); @@ -655,7 +655,7 @@ public synchronized StateOwner getOrCreateOwner(byte[] owner, seqid4 seq) throws * @param owner client unique state owner */ public synchronized void releaseOwner(byte[] owner) throws StaleClientidException { - Opaque k = new Opaque(owner); + Opaque k = Opaque.forBytes(owner); StateOwner stateOwner = _owners.remove(k); if (stateOwner == null) { throw new StaleClientidException(); From 52f8de734233db5667431648829a2183d34a5951 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 12:14:36 +0200 Subject: [PATCH 09/21] core, dlm: Remove deprecated LockManager methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the now-deprecated byte[]-specific lock methods in favor of using Opaque. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../nfs/v4/nlm/AbstractLockManager.java | 88 +------ .../nfs/v4/nlm/AbstractLockManager2.java | 227 ------------------ .../org/dcache/nfs/v4/nlm/LockManager.java | 26 +- .../java/org/dcache/nfs/v4/nlm/SimpleLm.java | 2 +- .../nfs/v4/nlm/DistributedLockManager.java | 3 +- 5 files changed, 17 insertions(+), 329 deletions(-) delete mode 100644 core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager2.java diff --git a/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager.java b/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager.java index c742d78e..3993065b 100644 --- a/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager.java +++ b/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager.java @@ -43,18 +43,7 @@ public abstract class AbstractLockManager implements LockManager { * @param objId object id. * @return exclusive lock. */ - @Deprecated(forRemoval = true) - abstract protected Lock getObjectLock(byte[] objId); - - /** - * Get exclusive lock on objects locks. - * - * @param objId object id. - * @return exclusive lock. - */ - protected Lock getObjectLock(Opaque objId) { - return getObjectLock(objId.asBytes()); - } + protected abstract Lock getObjectLock(Opaque objId); /** * Get collection of currently used active locks on the object. @@ -62,27 +51,7 @@ protected Lock getObjectLock(Opaque objId) { * @param objId object id. * @return collection of active locks. */ - @Deprecated(forRemoval = true) - abstract protected Collection getActiveLocks(byte[] objId); - - /** - * Get collection of currently used active locks on the object. - * - * @param objId object id. - * @return collection of active locks. - */ - protected Collection getActiveLocks(Opaque objId) { - return getActiveLocks(objId.asBytes()); - } - - /** - * Add {@code lock} to an object. - * - * @param objId object id. - * @param lock lock to add. - */ - @Deprecated(forRemoval = true) - abstract protected void add(byte[] objId, NlmLock lock); + protected abstract Collection getActiveLocks(Opaque objId); /** * Add {@code lock} to an object. @@ -90,19 +59,7 @@ protected Collection getActiveLocks(Opaque objId) { * @param objId object id. * @param lock lock to add. */ - protected void add(Opaque objId, NlmLock lock) { - add(objId.asBytes(), lock); - } - - /** - * Remove a lock for the given object. - * - * @param objId object id. - * @param lock lock to remove. - * @return true, if specified lock was removed. - */ - @Deprecated(forRemoval = true) - abstract protected boolean remove(byte[] objId, NlmLock lock); + protected abstract void add(Opaque objId, NlmLock lock); /** * Remove a lock for the given object. @@ -111,9 +68,7 @@ protected void add(Opaque objId, NlmLock lock) { * @param lock lock to remove. * @return true, if specified lock was removed. */ - protected boolean remove(Opaque objId, NlmLock lock) { - return remove(objId.asBytes(), lock); - } + protected abstract boolean remove(Opaque objId, NlmLock lock); /** * Add all locks from a given collection of locks @@ -121,27 +76,7 @@ protected boolean remove(Opaque objId, NlmLock lock) { * @param objId object id. * @param locks locks to add. */ - @Deprecated(forRemoval = true) - abstract protected void addAll(byte[] objId, Collection locks); - - /** - * Add all locks from a given collection of locks - * - * @param objId object id. - * @param locks locks to add. - */ - protected void addAll(Opaque objId, Collection locks) { - addAll(objId.asBytes(), locks); - } - - /** - * Remove all locks specified by {@code locks} associated with the given object. - * - * @param objId object id. - * @param locks collections of locks to remove. - */ - @Deprecated(forRemoval = true) - abstract protected void removeAll(byte[] objId, Collection locks); + protected abstract void addAll(Opaque objId, Collection locks); /** * Remove all locks specified by {@code locks} associated with the given object. @@ -149,12 +84,10 @@ protected void addAll(Opaque objId, Collection locks) { * @param objId object id. * @param locks collections of locks to remove. */ - protected void removeAll(Opaque objId, Collection locks) { - removeAll(objId.asBytes(), locks); - } + protected abstract void removeAll(Opaque objId, Collection locks); @Override - public void lock(byte[] objId, NlmLock lock) throws LockException { + public void lock(Opaque objId, NlmLock lock) throws LockException { Lock dlmLock = getObjectLock(objId); dlmLock.lock(); try { @@ -191,7 +124,7 @@ public void lock(byte[] objId, NlmLock lock) throws LockException { } @Override - public void unlock(byte[] objId, NlmLock lock) throws LockException { + public void unlock(Opaque objId, NlmLock lock) throws LockException { Lock dlmLock = getObjectLock(objId); dlmLock.lock(); try { @@ -230,7 +163,7 @@ public void unlock(byte[] objId, NlmLock lock) throws LockException { } @Override - public void test(byte[] objId, NlmLock lock) throws LockException { + public void test(Opaque objId, NlmLock lock) throws LockException { Lock dlmLock = getObjectLock(objId); dlmLock.lock(); try { @@ -246,7 +179,7 @@ public void test(byte[] objId, NlmLock lock) throws LockException { } @Override - public void unlockIfExists(byte[] objId, NlmLock lock) { + public void unlockIfExists(Opaque objId, NlmLock lock) { Lock dlmLock = getObjectLock(objId); dlmLock.lock(); try { @@ -255,5 +188,4 @@ public void unlockIfExists(byte[] objId, NlmLock lock) { dlmLock.unlock(); } } - } diff --git a/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager2.java b/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager2.java deleted file mode 100644 index 8fdc7ede..00000000 --- a/core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager2.java +++ /dev/null @@ -1,227 +0,0 @@ -/* - * Copyright (c) 2017 Deutsches Elektronen-Synchroton, - * Member of the Helmholtz Association, (DESY), HAMBURG, GERMANY - * - * This library is free software; you can redistribute it and/or modify - * it under the terms of the GNU Library General Public License as - * published by the Free Software Foundation; either version 2 of the - * License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Library General Public License for more details. - * - * You should have received a copy of the GNU Library General Public - * License along with this program (see the file COPYING.LIB for more - * details); if not, write to the Free Software Foundation, Inc., - * 675 Mass Ave, Cambridge, MA 02139, USA. - */ -package org.dcache.nfs.v4.nlm; - -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; -import java.util.Optional; -import java.util.concurrent.locks.Lock; -import java.util.stream.Collectors; - -import org.dcache.nfs.util.Opaque; -import org.dcache.nfs.v4.xdr.nfs4_prot; - -abstract class AbstractLockManager2 extends AbstractLockManager { - private static final boolean DEBUG_DEPRECATED_CALLS = false; - - @Override - @Deprecated(forRemoval = true) - protected final Lock getObjectLock(byte[] objId) { - throw new IllegalStateException(); - } - - @Override - protected abstract Lock getObjectLock(Opaque objId); - - @Override - @Deprecated(forRemoval = true) - protected final Collection getActiveLocks(byte[] objId) { - throw new IllegalStateException(); - } - - @Override - protected abstract Collection getActiveLocks(Opaque objId); - - @Override - @Deprecated(forRemoval = true) - protected final void add(byte[] objId, NlmLock lock) { - throw new IllegalStateException(); - } - - @Override - protected abstract void add(Opaque objId, NlmLock lock); - - @Override - @Deprecated(forRemoval = true) - protected final boolean remove(byte[] objId, NlmLock lock) { - throw new IllegalStateException(); - } - - @Override - protected abstract boolean remove(Opaque objId, NlmLock lock); - - @Override - @Deprecated(forRemoval = true) - protected final void addAll(byte[] objId, Collection locks) { - throw new IllegalStateException(); - } - - @Override - protected abstract void addAll(Opaque objId, Collection locks); - - @Override - @Deprecated(forRemoval = true) - protected final void removeAll(byte[] objId, Collection locks) { - throw new IllegalStateException(); - } - - @Override - protected abstract void removeAll(Opaque objId, Collection locks); - - @Override - @Deprecated(forRemoval = true) - public void lock(byte[] objId, NlmLock lock) throws LockException { - if (DEBUG_DEPRECATED_CALLS) { - new IllegalStateException("Called deprecated method").printStackTrace(); - } - lock(Opaque.forBytes(objId), lock); - } - - @Override - public void lock(Opaque objId, NlmLock lock) throws LockException { - Lock dlmLock = getObjectLock(objId); - dlmLock.lock(); - try { - Collection currentLocks = getActiveLocks(objId); - Optional conflictingLock = currentLocks.stream().filter((NlmLock l) -> l.isConflicting(lock)) - .findAny(); - if (conflictingLock.isPresent()) { - throw new LockDeniedException("object locked", conflictingLock.get()); - } - // no conflicting locks. try to merge existing locks - List toMerge = currentLocks.stream().filter((NlmLock l) -> l.isOverlappingRange(lock)).filter(( - NlmLock l) -> l.isSameOwner(lock)).filter((NlmLock l) -> l.getLockType() == lock.getLockType()) - .collect(Collectors.toList()); - if (toMerge.isEmpty()) { - add(objId, lock); - } else { - // merge overlaping/continues locks - long lockBegin = lock.getOffset(); - long lockEnd = lock.getLength() == nfs4_prot.NFS4_UINT64_MAX ? nfs4_prot.NFS4_UINT64_MAX : (lockBegin - + lock.getLength()); - for (NlmLock l : toMerge) { - lockBegin = Math.min(lockBegin, l.getOffset()); - lockEnd = lockEnd == nfs4_prot.NFS4_UINT64_MAX || l.getLength() == nfs4_prot.NFS4_UINT64_MAX - ? nfs4_prot.NFS4_UINT64_MAX : Math.max(lockEnd, l.getOffset() + l.getLength() - 1); - } - NlmLock mergedLock = new NlmLock(lock.getOwner(), lock.getLockType(), lockBegin, - lockEnd == nfs4_prot.NFS4_UINT64_MAX ? lockEnd : lockEnd - lockBegin); - removeAll(objId, toMerge); - add(objId, mergedLock); - } - } finally { - dlmLock.unlock(); - } - } - - @Override - @Deprecated(forRemoval = true) - public void unlock(byte[] objId, NlmLock lock) throws LockException { - if (DEBUG_DEPRECATED_CALLS) { - new IllegalStateException("Called deprecated method").printStackTrace(); - } - unlock(Opaque.forBytes(objId), lock); - } - - @Override - public void unlock(Opaque objId, NlmLock lock) throws LockException { - Lock dlmLock = getObjectLock(objId); - dlmLock.lock(); - try { - Collection currentLocks = getActiveLocks(objId); - // check for exact match first - if (remove(objId, lock)) { - return; - } - List toRemove = new ArrayList<>(); - List toAdd = new ArrayList<>(); - currentLocks.stream().filter((NlmLock l) -> l.isSameOwner(lock)).filter((NlmLock l) -> l.isOverlappingRange( - lock)).forEach((NlmLock l) -> { - toRemove.add(l); - long l1 = lock.getOffset() - l.getOffset(); - if (l1 > 0) { - NlmLock first = new NlmLock(l.getOwner(), l.getLockType(), l.getOffset(), l1); - toAdd.add(first); - } - if (lock.getLength() != nfs4_prot.NFS4_UINT64_MAX) { - long l2 = l.getLength() - l1 - 1; - if (l2 > 0) { - NlmLock second = new NlmLock(l.getOwner(), l.getLockType(), lock.getOffset() + lock - .getLength(), l2); - toAdd.add(second); - } - } - }); - if (toRemove.isEmpty()) { - throw new LockRangeUnavailabeException("no matching lock"); - } - removeAll(objId, toRemove); - addAll(objId, toAdd); - } finally { - dlmLock.unlock(); - } - } - - @Override - @Deprecated(forRemoval = true) - public void test(byte[] objId, NlmLock lock) throws LockException { - if (DEBUG_DEPRECATED_CALLS) { - new IllegalStateException("Called deprecated method").printStackTrace(); - } - test(Opaque.forBytes(objId), lock); - } - - @Override - public void test(Opaque objId, NlmLock lock) throws LockException { - Lock dlmLock = getObjectLock(objId); - dlmLock.lock(); - try { - Collection currentLocks = getActiveLocks(objId); - Optional conflictingLock = currentLocks.stream().filter((NlmLock l) -> l.isOverlappingRange(lock) - && !l.isSameOwner(lock)).findAny(); - if (conflictingLock.isPresent()) { - throw new LockDeniedException("object locked", conflictingLock.get()); - } - } finally { - dlmLock.unlock(); - } - } - - @Override - @Deprecated(forRemoval = true) - public void unlockIfExists(byte[] objId, NlmLock lock) { - if (DEBUG_DEPRECATED_CALLS) { - new IllegalStateException("Called deprecated method").printStackTrace(); - } - unlockIfExists(Opaque.forBytes(objId), lock); - } - - @Override - public void unlockIfExists(Opaque objId, NlmLock lock) { - Lock dlmLock = getObjectLock(objId); - dlmLock.lock(); - try { - remove(objId, lock); - } finally { - dlmLock.unlock(); - } - } -} diff --git a/core/src/main/java/org/dcache/nfs/v4/nlm/LockManager.java b/core/src/main/java/org/dcache/nfs/v4/nlm/LockManager.java index 5899a382..253b3390 100644 --- a/core/src/main/java/org/dcache/nfs/v4/nlm/LockManager.java +++ b/core/src/main/java/org/dcache/nfs/v4/nlm/LockManager.java @@ -33,11 +33,7 @@ public interface LockManager { * @throws LockDeniedException if a conflicting lock is detected. * @throws LockException if locking fails. */ - void lock(byte[] objId, NlmLock lock) throws LockException; - - default void lock(Opaque key, NlmLock lock) throws LockException { - lock(key.asBytes(), lock); - } + void lock(Opaque objId, NlmLock lock) throws LockException; /** * Unlock byte range of an {@code objId}. @@ -47,11 +43,7 @@ default void lock(Opaque key, NlmLock lock) throws LockException { * @throws LockRangeUnavailabeException if no matching lock found. * @throws LockException if locking fails. */ - void unlock(byte[] objId, NlmLock lock) throws LockException; - - default void unlock(Opaque key, NlmLock lock) throws LockException { - unlock(key.asBytes(), lock); - } + void unlock(Opaque objId, NlmLock lock) throws LockException; /** * Test byte range lock existence for an {@code objId}. Same as {@link #lock}, except that a new lock is not @@ -62,21 +54,13 @@ default void unlock(Opaque key, NlmLock lock) throws LockException { * @throws LockDeniedException if a conflicting lock is detected. * @throws LockException if locking fails. */ - void test(byte[] objId, NlmLock lock) throws LockException; - - default void test(Opaque key, NlmLock lock) throws LockException { - test(key.asBytes(), lock); - } + void test(Opaque objId, NlmLock lock) throws LockException; /** - * Like {@link #unlock(byte[], org.dcache.nfs.v4.nlm.NlmLock)}, but does not fail if lock does not exists. + * Like {@link #unlock(Opaque, org.dcache.nfs.v4.nlm.NlmLock)}, but does not fail if lock does not exists. * * @param objId * @param lock */ - void unlockIfExists(byte[] objId, NlmLock lock); - - default void unlockIfExists(Opaque key, NlmLock lock) { - unlockIfExists(key.asBytes(), lock); - } + void unlockIfExists(Opaque objId, NlmLock lock); } diff --git a/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java b/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java index 101d17e8..7e0f9f2d 100644 --- a/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java +++ b/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java @@ -35,7 +35,7 @@ * * @since 0.14 */ -public class SimpleLm extends AbstractLockManager2 { +public class SimpleLm extends AbstractLockManager { /* * Use {@link Striped} here to split synchronized block on file locks into multiple partitions to increase diff --git a/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java b/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java index c420006f..410baf8b 100644 --- a/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java +++ b/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java @@ -19,7 +19,6 @@ */ package org.dcache.nfs.v4.nlm; -import java.util.Base64; import java.util.Collection; import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.Condition; @@ -47,7 +46,7 @@ * * @since 0.16 */ -public class DistributedLockManager extends AbstractLockManager2 { +public class DistributedLockManager extends AbstractLockManager { private final MultiMap locks; From a9578611631369202bf2c6ca94cabd3334a7b80e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 12:27:02 +0200 Subject: [PATCH 10/21] core: Opaque: remove deprecated methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This make Opaque completely immutable. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/util/Opaque.java | 8 +------- core/src/main/java/org/dcache/nfs/v4/FileTracker.java | 4 ++-- 2 files changed, 3 insertions(+), 9 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 7cb1365b..2286c48e 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -39,16 +39,10 @@ public static Opaque forBytes(byte[] bytes) { return new Opaque(bytes.clone()); } - @Deprecated(forRemoval = true) - public Opaque(byte[] opaque) { + private Opaque(byte[] opaque) { _opaque = opaque; } - @Deprecated(forRemoval = true) - public byte[] getOpaque() { - return _opaque; - } - public byte[] asBytes() { return _opaque.clone(); } 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 2367e6a3..2f2b1181 100644 --- a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java +++ b/core/src/main/java/org/dcache/nfs/v4/FileTracker.java @@ -565,7 +565,7 @@ void removeOpen(Inode inode, stateid4 stateid) { public Map> getOpenFiles() { return files.entrySet().stream() .collect(Collectors.toMap( - e -> Inode.forFile(e.getKey().getOpaque()), + e -> Inode.forFile(e.getKey().asBytes()), e -> e.getValue().stream().map(OpenState::getClient).collect(Collectors.toSet()))); } @@ -578,7 +578,7 @@ public Map> getOpenFiles() { public Map> getDelegations() { return delegations.entrySet().stream() .collect(Collectors.toMap( - e -> Inode.forFile(e.getKey().getOpaque()), + e -> Inode.forFile(e.getKey().asBytes()), e -> e.getValue().stream().map(DelegationState::client).collect(Collectors.toSet()))); } } From da8f898594f5496de33814e2ea78d978ad6af584 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 12:34:52 +0200 Subject: [PATCH 11/21] core: Inode: Provide helper method create Inode from Opaque fileIdKey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This decouples the last bit of the remaining byte[] logic from Opaque. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/v4/FileTracker.java | 4 ++-- core/src/main/java/org/dcache/nfs/vfs/Inode.java | 4 ++++ 2 files changed, 6 insertions(+), 2 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 2f2b1181..e6e5991c 100644 --- a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java +++ b/core/src/main/java/org/dcache/nfs/v4/FileTracker.java @@ -565,7 +565,7 @@ void removeOpen(Inode inode, stateid4 stateid) { public Map> getOpenFiles() { return files.entrySet().stream() .collect(Collectors.toMap( - e -> Inode.forFile(e.getKey().asBytes()), + e -> Inode.forFileIdKey(e.getKey()), e -> e.getValue().stream().map(OpenState::getClient).collect(Collectors.toSet()))); } @@ -578,7 +578,7 @@ public Map> getOpenFiles() { public Map> getDelegations() { return delegations.entrySet().stream() .collect(Collectors.toMap( - e -> Inode.forFile(e.getKey().asBytes()), + e -> Inode.forFileIdKey(e.getKey()), e -> e.getValue().stream().map(DelegationState::client).collect(Collectors.toSet()))); } } diff --git a/core/src/main/java/org/dcache/nfs/vfs/Inode.java b/core/src/main/java/org/dcache/nfs/vfs/Inode.java index 52bb7b89..b400bdb1 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/Inode.java +++ b/core/src/main/java/org/dcache/nfs/vfs/Inode.java @@ -144,6 +144,10 @@ public static Inode forFile(byte[] bytes) { return new Inode(0, 0, 0, bytes); } + public static Inode forFileIdKey(Opaque key) { + return forFile(key.asBytes()); + } + @Deprecated(forRemoval = true) public byte[] getFileId() { return opaqueKey.asBytes(); From 957551f7a70486a74f6e866b643d8d0ecbe80c07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 12:47:02 +0200 Subject: [PATCH 12/21] core: Opaque: Convert to interface, remove Serializable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... and provide a default implementation inline. This enables certain objects to function as Opaque keys directly. Serializability is not required by the current code, so let's remove it. If required, a new subclass can add support for it. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../main/java/org/dcache/nfs/util/Opaque.java | 100 ++++++++++-------- 1 file changed, 55 insertions(+), 45 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 2286c48e..7b55c387 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -19,68 +19,78 @@ */ package org.dcache.nfs.util; -import java.io.Serializable; import java.util.Arrays; import java.util.Base64; -import com.google.common.io.BaseEncoding; - /** - * A helper class for opaque data manipulations. Enabled opaque date to be used as a key in {@link java.util.Collection} + * Describes something that can be used as a key in {@link java.util.Map} and that can be converted to a {@code byte[]} + * and a Base64 string representation. */ -public class Opaque implements Serializable { +public interface Opaque { + public static Opaque forBytes(byte[] bytes) { + return new OpaqueImpl(bytes.clone()); + } - private static final long serialVersionUID = 1532238396149112674L; + byte[] asBytes(); - private final byte[] _opaque; - private String base64 = null; + String getBase64(); - public static Opaque forBytes(byte[] bytes) { - return new Opaque(bytes.clone()); - } + @Override + int hashCode(); - private Opaque(byte[] opaque) { - _opaque = opaque; - } + @Override + boolean equals(Object o); - public byte[] asBytes() { - return _opaque.clone(); - } + final class OpaqueImpl implements Opaque { + private final byte[] _opaque; + private String base64 = null; - public String getBase64() { - if (base64 == null) { - base64 = Base64.getEncoder().withoutPadding().encodeToString(_opaque); + private OpaqueImpl(byte[] opaque) { + _opaque = opaque; } - return base64; - } - @Override - public int hashCode() { - return Arrays.hashCode(_opaque); - } + @Override + public byte[] asBytes() { + return _opaque.clone(); + } - @Override - public boolean equals(Object o) { - if (o == this) { - return true; + @Override + public String getBase64() { + if (base64 == null) { + base64 = Base64.getEncoder().withoutPadding().encodeToString(_opaque); + } + return base64; } - if (!(o instanceof Opaque)) { - return false; + + @Override + public int hashCode() { + return Arrays.hashCode(_opaque); } - return Arrays.equals(_opaque, ((Opaque) o)._opaque); - } + @Override + public boolean equals(Object o) { + if (o == this) { + return true; + } + if (!(o instanceof Opaque)) { + return false; + } - /** - * Returns a (potentially non-stable) debug string. - * - * @see #getBase64() - */ - @Deprecated - @Override - public String toString() { - StringBuilder sb = new StringBuilder(); - sb.append('[').append(BaseEncoding.base16().lowerCase().encode(_opaque)).append(']'); - return sb.toString(); + if (o instanceof OpaqueImpl) { + return Arrays.equals(_opaque, ((OpaqueImpl) o)._opaque); + } else { + return Arrays.equals(_opaque, ((Opaque) o).asBytes()); + } + } + + /** + * Returns a (potentially non-stable) debug string. + * + * @see #getBase64() + */ + @Override + public String toString() { + return super.toString() + "[" + getBase64() + "]"; + } } } From e74c0871e670312f83ae96bc399cb0625a06ead2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 12:55:31 +0200 Subject: [PATCH 13/21] core, dlm: Opaque: rename methods to properly indicate conversion step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Converting an Opaque to a byte[] or Base64-string may involve an allocation, not just a mere cast/dereference. Therefore, call these methods "toBytes" and toBase64". Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/util/Opaque.java | 14 +++++++------- .../main/java/org/dcache/nfs/v4/nlm/SimpleLm.java | 2 +- core/src/main/java/org/dcache/nfs/vfs/FsCache.java | 2 +- core/src/main/java/org/dcache/nfs/vfs/Inode.java | 4 ++-- .../dcache/nfs/v4/nlm/DistributedLockManager.java | 2 +- 5 files changed, 12 insertions(+), 12 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 7b55c387..940fdb14 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -31,9 +31,9 @@ public static Opaque forBytes(byte[] bytes) { return new OpaqueImpl(bytes.clone()); } - byte[] asBytes(); + byte[] toBytes(); - String getBase64(); + String toBase64(); @Override int hashCode(); @@ -50,12 +50,12 @@ private OpaqueImpl(byte[] opaque) { } @Override - public byte[] asBytes() { + public byte[] toBytes() { return _opaque.clone(); } @Override - public String getBase64() { + public String toBase64() { if (base64 == null) { base64 = Base64.getEncoder().withoutPadding().encodeToString(_opaque); } @@ -79,18 +79,18 @@ public boolean equals(Object o) { if (o instanceof OpaqueImpl) { return Arrays.equals(_opaque, ((OpaqueImpl) o)._opaque); } else { - return Arrays.equals(_opaque, ((Opaque) o).asBytes()); + return Arrays.equals(_opaque, ((Opaque) o).toBytes()); } } /** * Returns a (potentially non-stable) debug string. * - * @see #getBase64() + * @see #toBase64() */ @Override public String toString() { - return super.toString() + "[" + getBase64() + "]"; + return super.toString() + "[" + toBase64() + "]"; } } } diff --git a/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java b/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java index 7e0f9f2d..0d1aa92d 100644 --- a/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java +++ b/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java @@ -113,7 +113,7 @@ protected void removeAll(Opaque objId, Collection locks) { } private final String toKey(Opaque objId) { - return objId.getBase64(); + return objId.toBase64(); } } diff --git a/core/src/main/java/org/dcache/nfs/vfs/FsCache.java b/core/src/main/java/org/dcache/nfs/vfs/FsCache.java index 3b6a4e59..cf8de09b 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/FsCache.java +++ b/core/src/main/java/org/dcache/nfs/vfs/FsCache.java @@ -52,7 +52,7 @@ private static class FileChannelSupplier extends CacheLoader @Override public FileChannel load(Inode inode) throws IOException { - String id = inode.getLockKey().getBase64(); + String id = inode.getLockKey().toBase64(); File dir = getAndCreateDirectory(id); File f = new File(dir, id); return new RandomAccessFile(f, "rw").getChannel(); diff --git a/core/src/main/java/org/dcache/nfs/vfs/Inode.java b/core/src/main/java/org/dcache/nfs/vfs/Inode.java index b400bdb1..0ffc867a 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/Inode.java +++ b/core/src/main/java/org/dcache/nfs/vfs/Inode.java @@ -145,12 +145,12 @@ public static Inode forFile(byte[] bytes) { } public static Inode forFileIdKey(Opaque key) { - return forFile(key.asBytes()); + return forFile(key.toBytes()); } @Deprecated(forRemoval = true) public byte[] getFileId() { - return opaqueKey.asBytes(); + return opaqueKey.toBytes(); } /** diff --git a/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java b/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java index 410baf8b..595ceabd 100644 --- a/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java +++ b/dlm/src/main/java/org/dcache/nfs/v4/nlm/DistributedLockManager.java @@ -134,7 +134,7 @@ protected void removeAll(Opaque objId, Collection locks) { } private static String objIdToKey(Opaque objId) { - return objId.getBase64(); + return objId.toBase64(); } } From b52ab011cd87ac53af33f0db732299164351ab4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 13:06:10 +0200 Subject: [PATCH 14/21] core: Opaque: provide helper method to instantiate from ByteBuffer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We can construct an Opaque instance from the bytes provided in a ByteBuffer. This saves us one clone() in Inode. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../main/java/org/dcache/nfs/util/Opaque.java | 17 +++++++++++++++++ .../src/main/java/org/dcache/nfs/vfs/Inode.java | 4 +--- 2 files changed, 18 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 940fdb14..a37d00e9 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -19,6 +19,7 @@ */ package org.dcache.nfs.util; +import java.nio.ByteBuffer; import java.util.Arrays; import java.util.Base64; @@ -31,6 +32,22 @@ public static Opaque forBytes(byte[] bytes) { return new OpaqueImpl(bytes.clone()); } + /** + * Returns an {@link Opaque} instance based on a copy of the {@code length} bytes from the given {@link ByteBuffer} + * starting from the current {@link ByteBuffer#position()} plus the given {@code offset}. + * + * @param buf The buffer. + * @param offset The offset (relative to the current position). + * @param length The number of bytes. + * @return The {@link Opaque} instance. + */ + public static Opaque forBytes(ByteBuffer buf, int offset, int length) { + byte[] bytes = new byte[length]; + buf.get(bytes, offset, length); + + return new OpaqueImpl(bytes); + } + byte[] toBytes(); String toBase64(); diff --git a/core/src/main/java/org/dcache/nfs/vfs/Inode.java b/core/src/main/java/org/dcache/nfs/vfs/Inode.java index 0ffc867a..e7f2b008 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/Inode.java +++ b/core/src/main/java/org/dcache/nfs/vfs/Inode.java @@ -109,9 +109,7 @@ public Inode(byte[] bytes) { exportIdx = b.getInt(); type = (int) b.get(); int olen = (int) b.get(); - byte[] fs_opaque = new byte[olen]; - b.get(fs_opaque); - this.opaqueKey = Opaque.forBytes(fs_opaque); + this.opaqueKey = Opaque.forBytes(b, 0, olen); this.nfsHandle = bytes.clone(); } From 7a64663f1c245085ee2a60f7528f680c9b5c2ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 13:19:39 +0200 Subject: [PATCH 15/21] core: Opaque: Add javadoc to forBytes method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clarify that this method creates a copy of the specified bytes. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/util/Opaque.java | 6 ++++++ 1 file changed, 6 insertions(+) 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 a37d00e9..ee0c7992 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -28,6 +28,12 @@ * and a Base64 string representation. */ public interface Opaque { + /** + * Returns an {@link Opaque} instance based on a copy of the given bytes. + * + * @param bytes The bytes. + * @return The {@link Opaque} instance. + */ public static Opaque forBytes(byte[] bytes) { return new OpaqueImpl(bytes.clone()); } From ff66e6905863995b8f752584d789b8a8c1009dad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 13:28:57 +0200 Subject: [PATCH 16/21] core: Opaque: Remove position offset from ByteBuffer constructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing the relative offset (which in our case is always 0) will reduce the chance that somebody thinks it's an absolute offset from the beginning of the ByteBuffer... Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/util/Opaque.java | 8 +++----- core/src/main/java/org/dcache/nfs/vfs/Inode.java | 2 +- 2 files changed, 4 insertions(+), 6 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 ee0c7992..6317f40f 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -39,17 +39,15 @@ public static Opaque forBytes(byte[] bytes) { } /** - * Returns an {@link Opaque} instance based on a copy of the {@code length} bytes from the given {@link ByteBuffer} - * starting from the current {@link ByteBuffer#position()} plus the given {@code offset}. + * Returns an {@link Opaque} instance based on a copy of the {@code length} bytes from the given {@link ByteBuffer}. * * @param buf The buffer. - * @param offset The offset (relative to the current position). * @param length The number of bytes. * @return The {@link Opaque} instance. */ - public static Opaque forBytes(ByteBuffer buf, int offset, int length) { + public static Opaque forBytes(ByteBuffer buf, int length) { byte[] bytes = new byte[length]; - buf.get(bytes, offset, length); + buf.get(bytes); return new OpaqueImpl(bytes); } diff --git a/core/src/main/java/org/dcache/nfs/vfs/Inode.java b/core/src/main/java/org/dcache/nfs/vfs/Inode.java index e7f2b008..1da0856a 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/Inode.java +++ b/core/src/main/java/org/dcache/nfs/vfs/Inode.java @@ -109,7 +109,7 @@ public Inode(byte[] bytes) { exportIdx = b.getInt(); type = (int) b.get(); int olen = (int) b.get(); - this.opaqueKey = Opaque.forBytes(b, 0, olen); + this.opaqueKey = Opaque.forBytes(b, olen); this.nfsHandle = bytes.clone(); } From d53f43282da06a99faa31ee01e62bdcdf50334a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 17:24:01 +0200 Subject: [PATCH 17/21] core: Opaque: Clarify behavior of equals/hashCode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's make sure subclasses get that right. We cannot provide a "default" implementation for methods in java.lang.Object, so let's provide static default helpers. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../main/java/org/dcache/nfs/util/Opaque.java | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) 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 6317f40f..cd74de6d 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -52,13 +52,66 @@ public static Opaque forBytes(ByteBuffer buf, int length) { return new OpaqueImpl(bytes); } + /** + * Default implementation for {@link #hashCode()}. + * + * @param obj The instance object. + * @return The hash code. + * @see #hashCode() + */ + public static int defaultHashCode(Opaque obj) { + return Arrays.hashCode(obj.toBytes()); + } + + /** + * Default implementation for {@link #equals(Object)}. + * + * @param obj The instance object. + * @param other The other object. + * @return {@code true} if equal. + * @see #equals(Object) + */ + public static boolean defaultEquals(Opaque obj, Object other) { + if (other == obj) { + return true; + } + if (!(other instanceof Opaque)) { + return false; + } + return Arrays.equals(obj.toBytes(), ((Opaque) other).toBytes()); + } + + /** + * Returns a byte-representation of this opaque object. + * + * @return A new array. + */ byte[] toBytes(); + /** + * Returns a Base64 string representing this opaque object. + * + * @return A Base64 string. + */ String toBase64(); + /** + * Returns the hashCode based on the byte-representation of this instance. + *

+ * This method must behave like {@link #defaultHashCode(Opaque)}, but may be optimized. + * + * @return The hashCode. + */ @Override int hashCode(); + /** + * Compares this object to another one. + *

+ * This method must behave like {@link #defaultEquals(Opaque, Object)}, but may be optimized. + * + * @return {@code true} if both objects are equal. + */ @Override boolean equals(Object o); From d1bc72d1eeec112959388d21d3b911d781f273dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 17:26:54 +0200 Subject: [PATCH 18/21] core: Opaque: Remove unnecessary "public" from "public static" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... not required, since implicit in public interfaces. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/util/Opaque.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 cd74de6d..c8de8de2 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -34,7 +34,7 @@ public interface Opaque { * @param bytes The bytes. * @return The {@link Opaque} instance. */ - public static Opaque forBytes(byte[] bytes) { + static Opaque forBytes(byte[] bytes) { return new OpaqueImpl(bytes.clone()); } @@ -45,7 +45,7 @@ public static Opaque forBytes(byte[] bytes) { * @param length The number of bytes. * @return The {@link Opaque} instance. */ - public static Opaque forBytes(ByteBuffer buf, int length) { + static Opaque forBytes(ByteBuffer buf, int length) { byte[] bytes = new byte[length]; buf.get(bytes); @@ -59,7 +59,7 @@ public static Opaque forBytes(ByteBuffer buf, int length) { * @return The hash code. * @see #hashCode() */ - public static int defaultHashCode(Opaque obj) { + static int defaultHashCode(Opaque obj) { return Arrays.hashCode(obj.toBytes()); } @@ -71,7 +71,7 @@ public static int defaultHashCode(Opaque obj) { * @return {@code true} if equal. * @see #equals(Object) */ - public static boolean defaultEquals(Opaque obj, Object other) { + static boolean defaultEquals(Opaque obj, Object other) { if (other == obj) { return true; } From 8f4c511f1479cb905821448862f4fee907878cf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 18:25:56 +0200 Subject: [PATCH 19/21] core: Opaque: Improve reuse of file id data for PseudoFS innerInode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... and add Opaque#numBytes and #putBytes. This simplifies the dance between public-facing and inner Inode objects, allowing Opaque objects to be reused between these. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- .../main/java/org/dcache/nfs/util/Opaque.java | 26 +++++++++++++++++++ .../main/java/org/dcache/nfs/vfs/Inode.java | 25 +++++++++++++----- .../java/org/dcache/nfs/vfs/PseudoFs.java | 2 +- 3 files changed, 46 insertions(+), 7 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 c8de8de2..119d4fdd 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -88,6 +88,13 @@ static boolean defaultEquals(Opaque obj, Object other) { */ byte[] toBytes(); + /** + * Returns the number of bytes in this opaque object; + * + * @return The number of bytes; + */ + int numBytes(); + /** * Returns a Base64 string representing this opaque object. * @@ -95,6 +102,15 @@ static boolean defaultEquals(Opaque obj, Object other) { */ String toBase64(); + /** + * Writes the bytes of this {@link Opaque} to the given {@link ByteBuffer}. + * + * @param buf The target buffer. + */ + default void putBytes(ByteBuffer buf) { + buf.put(toBytes()); + } + /** * Returns the hashCode based on the byte-representation of this instance. *

@@ -136,6 +152,11 @@ public String toBase64() { return base64; } + @Override + public void putBytes(ByteBuffer buf) { + buf.put(_opaque); + } + @Override public int hashCode() { return Arrays.hashCode(_opaque); @@ -166,5 +187,10 @@ public boolean equals(Object o) { public String toString() { return super.toString() + "[" + toBase64() + "]"; } + + @Override + public int numBytes() { + return _opaque.length; + } } } diff --git a/core/src/main/java/org/dcache/nfs/vfs/Inode.java b/core/src/main/java/org/dcache/nfs/vfs/Inode.java index 1da0856a..110fa21f 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/Inode.java +++ b/core/src/main/java/org/dcache/nfs/vfs/Inode.java @@ -69,14 +69,18 @@ public Inode(FileHandle fh) { */ @Deprecated public Inode(int generation, int exportIdx, int type, byte[] fs_opaque) { + this(generation, exportIdx, type, Opaque.forBytes(fs_opaque)); + } + + private Inode(int generation, int exportIdx, int type, Opaque fileIdKey) { this.version = VERSION; this.magic = MAGIC; this.generation = generation; this.exportIdx = exportIdx; this.type = type; - this.opaqueKey = Opaque.forBytes(fs_opaque); + this.opaqueKey = fileIdKey; - this.nfsHandle = buildNfsHandle(fs_opaque); + this.nfsHandle = buildNfsHandle(); } /** @@ -146,6 +150,10 @@ public static Inode forFileIdKey(Opaque key) { return forFile(key.toBytes()); } + public static Inode innerInode(Inode outerInode) { + return new Inode(0, 0, 0, outerInode.getFileIdKey()); + } + @Deprecated(forRemoval = true) public byte[] getFileId() { return opaqueKey.toBytes(); @@ -178,8 +186,13 @@ public byte[] toNfsHandle() { return nfsHandle.clone(); } - private byte[] buildNfsHandle(byte[] fs_opaque) { - int len = fs_opaque.length + MIN_LEN; + private byte[] buildNfsHandle() { + int opaqueLen = opaqueKey.numBytes(); + if (opaqueLen < 0 || opaqueLen > 255) { + throw new IllegalStateException("Invalid opaque key length"); + } + + int len = MIN_LEN + opaqueLen; byte[] bytes = new byte[len]; ByteBuffer b = ByteBuffer.wrap(bytes); b.order(ByteOrder.BIG_ENDIAN); @@ -188,8 +201,8 @@ private byte[] buildNfsHandle(byte[] fs_opaque) { b.putInt(generation); b.putInt(exportIdx); b.put((byte) type); - b.put((byte) fs_opaque.length); - b.put(fs_opaque); + b.put((byte) opaqueLen); + opaqueKey.putBytes(b); return bytes; } 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 f7ecf18d..0c8ab9d8 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java +++ b/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java @@ -748,6 +748,6 @@ private boolean inheritUidGid(Inode inode) { * @return The {@link Inode} as passed from {@link PseudoFs} to the underlying {@link VirtualFileSystem}. */ private Inode innerInode(Inode inode) { - return Inode.forFile(inode.getFileId()); + return Inode.innerInode(inode); } } From 3ad63dbd8009c8cd597179603f6f473323854a7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 19:33:28 +0200 Subject: [PATCH 20/21] core: Inode: Improve forFileIdKey and innerInode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that we have a Opaque-specific constructor, let's use it instead of converting back and forth from byte[]. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/vfs/Inode.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/dcache/nfs/vfs/Inode.java b/core/src/main/java/org/dcache/nfs/vfs/Inode.java index 110fa21f..788804a7 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/Inode.java +++ b/core/src/main/java/org/dcache/nfs/vfs/Inode.java @@ -147,11 +147,11 @@ public static Inode forFile(byte[] bytes) { } public static Inode forFileIdKey(Opaque key) { - return forFile(key.toBytes()); + return new Inode(0, 0, 0, key); } public static Inode innerInode(Inode outerInode) { - return new Inode(0, 0, 0, outerInode.getFileIdKey()); + return forFileIdKey(outerInode.getFileIdKey()); } @Deprecated(forRemoval = true) From 09e8063234e4b649d3e748474d8d425f53ce64b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 27 Jun 2025 19:34:32 +0200 Subject: [PATCH 21/21] core: PseudoFS: Properly resolve the inner root inode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we try to resolve the inner FS's root inode given a pseudo inode in PseudoFS, we can directly return the inner FS's root inode as returned by VirtualFileSystem#getRootInode(), which is most likely cached, instead of creating new Inode instances all over again. This further reduces the number of Inode allocations. Also fail with BadHandleException for any other pseudo inode because they will by definition be invalid in the underlying file system. Related: https://github.com/dCache/nfs4j/discussions/149 Signed-off-by: Christian Kohlschütter --- core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) 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 0c8ab9d8..3b00a18e 100644 --- a/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java +++ b/core/src/main/java/org/dcache/nfs/vfs/PseudoFs.java @@ -746,8 +746,17 @@ private boolean inheritUidGid(Inode inode) { * * @param inode The {@link Inode} as passed from and to the NFS client. * @return The {@link Inode} as passed from {@link PseudoFs} to the underlying {@link VirtualFileSystem}. + * @throws IOException on error. */ - private Inode innerInode(Inode inode) { + private Inode innerInode(Inode inode) throws IOException { + if (inode.isPseudoInode()) { + Inode innerRootInode = _inner.getRootInode(); + if (innerRootInode.getFileIdKey().equals(inode.getFileIdKey())) { + return innerRootInode; + } else { + throw new BadHandleException(); + } + } return Inode.innerInode(inode); } }