Use Opaque for locking instead of byte[], improve reuse of base64 keys #154

kohlschuetter · 2025-06-27T08:17:57Z

Related to discussion #149

The locking infrastructure currently exposes byte[] for keys, yet internally mostly uses base64-representations of that string.

Exposing byte[] is not recommended because the underlying bytes are not immutable, so they could be modified by any involved component after the fact. The base64 representations are currently computed on the fly, which may severely impact performance.

With this PR, we improve Opaque to be more immutable (while keeping backwards compatibility for now, marking unsafe methods deprecated for removal). We also cache the Opaque used from Inode, which further improves reuse.

The change is backwards compatible. We should however remove the deprecated methods as soon as we can.

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: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

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: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

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: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

SimpleLm used to create new Base64-strings upon toKey(byte[]), which is suboptimal. Use Opaque.getBase64 instead. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

DistributedLockManager used to create new Base64-strings upon objIdToKey(byte[]), which is suboptimal. Use Opaque.getBase64 instead. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

This will change the test to use the new LockManager Opaque API. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

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: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

kofemann

Thanks for the contribution. The changes look good. I suggest going with a more aggressive approach and instead of deprication, removing old methods in the LockManager interface.

kofemann · 2025-06-27T09:05:54Z

core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager.java

@@ -42,49 +43,116 @@ public abstract class AbstractLockManager implements LockManager {
     * @param objId object id.
     * @return exclusive lock.
     */
+    @Deprecated(forRemoval = true)


I have strong doubts that there are third-party implementations. I am for removing it right away.

Great!

Should we also remove the now-deprecated Opaque constructor and getOpaque method, or is that being used elsewhere? (including permanently serialized objects, for example)

Ideally, I would want to make Opaque an interface with a default implementation.
For example, in a future environment without PseudoFs involvement we would have Inode implement Opaque directly.

(see new commits below)

kofemann · 2025-06-27T09:08:16Z

core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager2.java

+import org.dcache.nfs.util.Opaque;
+import org.dcache.nfs.v4.xdr.nfs4_prot;
+
+abstract class AbstractLockManager2 extends AbstractLockManager {


Where is the added value of this class?

backwards-compatibility only (for third-party LockManagers).
No longer necessary once we remove the deprecated methods (hence package-level protected)

kofemann · 2025-06-27T09:10:29Z

core/src/main/java/org/dcache/nfs/vfs/Inode.java

    }

    public byte[] toNfsHandle() {
        return nfsHandle.clone();
    }

-    private byte[] buildNfsHandle() {
+    private byte[] buildNfsHandle(byte[] fs_opaque) {


This uses instance attributes (but uses the passed fs_opaque byte[] directly because Opaque would unnecessarily clone() this).

Remove the now-deprecated byte[]-specific lock methods in favor of using Opaque. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

This make Opaque completely immutable. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

This decouples the last bit of the remaining byte[] logic from Opaque. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

... 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: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

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: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

kohlschuetter · 2025-06-27T11:05:32Z

Hang on, a few more changes (low-hanging fruit) coming...

We can construct an Opaque instance from the bytes provided in a ByteBuffer. This saves us one clone() in Inode. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

Clarify that this method creates a copy of the specified bytes. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

kohlschuetter · 2025-06-27T11:21:11Z

@kofemann Please review again, thanks!

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: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

kofemann · 2025-06-27T12:41:09Z

core/src/main/java/org/dcache/nfs/util/Opaque.java

+     * @param length The number of bytes.
+     * @return The {@link Opaque} instance.
+     */
+    public static Opaque forBytes(ByteBuffer buf, int length) {


ByteBuffer has the remaining method. Should do the job. No need to pass a length.

Yes, but we may not want to read the entire remainder

kofemann · 2025-06-27T12:52:26Z

core/src/main/java/org/dcache/nfs/util/Opaque.java

+            if (o instanceof OpaqueImpl) {
+                return Arrays.equals(_opaque, ((OpaqueImpl) o)._opaque);
+            } else {
+                return Arrays.equals(_opaque, ((Opaque) o).toBytes());


This breaks the symmetry requirements:

if x.equals(y), then y.equals(x). Thus, o must be an instanceof OpaqueImpl.

https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Object.html#equals(java.lang.Object)

I'm not sure I follow.

This is the equals implementation for OpaqueImpl, so we know that about our own object.
We should be able to have "equals" be true for all implementations of Opaque (see line 96)
Then we special-case where both objects are OpaqueImpl -- here we can compare the two arrays directly without cloning (lines 100-101)
Lastly (line 102), we compare OpaqueImpl against any other Opaque implementation. Here we know that toBytes gives us the array to compare.

Other implementations of Opaque must behave the same way (i.e., first check instanceof Opaque, bail if not, then potentially optimize).

Is there any particular reason for such implementation?

Apart from the points above: My goal is to have an environment where an Inode is the Opaque file id key.
That works once PseudoFS is out of the equation. (my custom VirtualFileSystem will return custom Inode subclasses that implement Opaque).

Since this may or may not be the default scenario for nfs4j, to keep the current Inode/file-id separation, we need a standard implementation (OpaqueImpl). However, hashCode and equals should ignore the specific implementation details because in the end only the bytes transferred from an NFS client matter (we don't want false-negative matches)

kofemann

The OpaqueImpl#equals looks not compliant with Java spec. Is there any particular reason for such implementation?

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: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

... not required, since implicit in public interfaces. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

kohlschuetter · 2025-06-27T16:26:56Z

one last improvement for this PR coming up in a sec...

... 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: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

kohlschuetter · 2025-06-27T17:32:36Z

I assume you're not reviewing right now — I will add two more changes before I log off.

Now that we have a Opaque-specific constructor, let's use it instead of converting back and forth from byte[]. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

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: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

kohlschuetter added 8 commits June 27, 2025 10:03

core: Make SimpleLm use AbstractLockManager2

7334be2

SimpleLm used to create new Base64-strings upon toKey(byte[]), which is suboptimal. Use Opaque.getBase64 instead. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

dlm: Make DistributedLockManager use AbstractLockManager2

d3a5bce

DistributedLockManager used to create new Base64-strings upon objIdToKey(byte[]), which is suboptimal. Use Opaque.getBase64 instead. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

core: test: Switch SimpleLmTest to use Opaque instead of byte[] keys

e1bcf35

This will change the test to use the new LockManager Opaque API. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

dlm: test: Switch DistributedLockManagerTest to use Opaque

f22dba3

This will change the test to use the new LockManager Opaque API. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

kofemann requested changes Jun 27, 2025

View reviewed changes

kohlschuetter added 5 commits June 27, 2025 12:26

core, dlm: Remove deprecated LockManager methods

52f8de7

Remove the now-deprecated byte[]-specific lock methods in favor of using Opaque. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

core: Opaque: remove deprecated methods

a957861

This make Opaque completely immutable. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

core: Inode: Provide helper method create Inode from Opaque fileIdKey

da8f898

This decouples the last bit of the remaining byte[] logic from Opaque. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

kohlschuetter requested a review from kofemann June 27, 2025 10:58

kohlschuetter added 2 commits June 27, 2025 13:18

core: Opaque: provide helper method to instantiate from ByteBuffer

b52ab01

We can construct an Opaque instance from the bytes provided in a ByteBuffer. This saves us one clone() in Inode. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

core: Opaque: Add javadoc to forBytes method

7a64663

Clarify that this method creates a copy of the specified bytes. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

kofemann reviewed Jun 27, 2025

View reviewed changes

kofemann requested changes Jun 27, 2025

View reviewed changes

kohlschuetter requested a review from kofemann June 27, 2025 15:25

core: Opaque: Remove unnecessary "public" from "public static"

d1bc72d

... not required, since implicit in public interfaces. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

core: Inode: Improve forFileIdKey and innerInode

3ad63db

Now that we have a Opaque-specific constructor, let's use it instead of converting back and forth from byte[]. Related: dCache#149 Signed-off-by: Christian Kohlschütter <[email protected]>

Use Opaque for locking instead of byte[], improve reuse of base64 keys #154

Are you sure you want to change the base?

Use Opaque for locking instead of byte[], improve reuse of base64 keys #154

Uh oh!

Conversation

kohlschuetter commented Jun 27, 2025

Uh oh!

kofemann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kohlschuetter Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kohlschuetter commented Jun 27, 2025

Uh oh!

kohlschuetter commented Jun 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kohlschuetter Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kofemann left a comment

Choose a reason for hiding this comment

Uh oh!

kohlschuetter commented Jun 27, 2025

Uh oh!

kohlschuetter commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kohlschuetter Jun 27, 2025 •

edited

Loading

kohlschuetter Jun 27, 2025 •

edited

Loading

kohlschuetter commented Jun 27, 2025 •

edited

Loading