core: Reduce fileId byte[] usage; improve and encourage use of Opaque

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

Author: kohlschuetter

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();

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!

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;
 

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
             }

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,11 +64,19 @@ 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.
      *
      * @param objId
      * @param lock
      */
     void unlockIfExists(byte[] objId, NlmLock lock);
+
+    default void unlockIfExists(Opaque key, NlmLock lock) {
+        unlockIfExists(key.asBytes(), lock);
+    }
 }

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

@@ -52,8 +52,7 @@ private static class FileChannelSupplier extends CacheLoader<Inode, FileChannel>
 
         @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();

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.
+     * <p>
+     * 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;
     }
 
     /**