Skip to content

Commit ac71ad7

Browse files
authored
Merge pull request #154 from kohlschuetter/ck/Inode2
Use Opaque for locking instead of byte[], improve reuse of base64 keys
2 parents 37cf57c + 09e8063 commit ac71ad7

File tree

16 files changed

+281
-98
lines changed

16 files changed

+281
-98
lines changed

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

Lines changed: 155 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -19,49 +19,178 @@
1919
*/
2020
package org.dcache.nfs.util;
2121

22-
import java.io.Serializable;
22+
import java.nio.ByteBuffer;
2323
import java.util.Arrays;
24-
25-
import com.google.common.io.BaseEncoding;
24+
import java.util.Base64;
2625

2726
/**
28-
* A helper class for opaque data manipulations. Enabled opaque date to be used as a key in {@link java.util.Collection}
27+
* Describes something that can be used as a key in {@link java.util.Map} and that can be converted to a {@code byte[]}
28+
* and a Base64 string representation.
2929
*/
30-
public class Opaque implements Serializable {
31-
32-
private static final long serialVersionUID = 1532238396149112674L;
33-
34-
private final byte[] _opaque;
35-
36-
public Opaque(byte[] opaque) {
37-
_opaque = opaque;
30+
public interface Opaque {
31+
/**
32+
* Returns an {@link Opaque} instance based on a copy of the given bytes.
33+
*
34+
* @param bytes The bytes.
35+
* @return The {@link Opaque} instance.
36+
*/
37+
static Opaque forBytes(byte[] bytes) {
38+
return new OpaqueImpl(bytes.clone());
3839
}
3940

40-
public byte[] getOpaque() {
41-
return _opaque;
41+
/**
42+
* Returns an {@link Opaque} instance based on a copy of the {@code length} bytes from the given {@link ByteBuffer}.
43+
*
44+
* @param buf The buffer.
45+
* @param length The number of bytes.
46+
* @return The {@link Opaque} instance.
47+
*/
48+
static Opaque forBytes(ByteBuffer buf, int length) {
49+
byte[] bytes = new byte[length];
50+
buf.get(bytes);
51+
52+
return new OpaqueImpl(bytes);
4253
}
4354

44-
@Override
45-
public int hashCode() {
46-
return Arrays.hashCode(_opaque);
55+
/**
56+
* Default implementation for {@link #hashCode()}.
57+
*
58+
* @param obj The instance object.
59+
* @return The hash code.
60+
* @see #hashCode()
61+
*/
62+
static int defaultHashCode(Opaque obj) {
63+
return Arrays.hashCode(obj.toBytes());
4764
}
4865

49-
@Override
50-
public boolean equals(Object o) {
51-
if (o == this) {
66+
/**
67+
* Default implementation for {@link #equals(Object)}.
68+
*
69+
* @param obj The instance object.
70+
* @param other The other object.
71+
* @return {@code true} if equal.
72+
* @see #equals(Object)
73+
*/
74+
static boolean defaultEquals(Opaque obj, Object other) {
75+
if (other == obj) {
5276
return true;
5377
}
54-
if (!(o instanceof Opaque)) {
78+
if (!(other instanceof Opaque)) {
5579
return false;
5680
}
81+
return Arrays.equals(obj.toBytes(), ((Opaque) other).toBytes());
82+
}
5783

58-
return Arrays.equals(_opaque, ((Opaque) o)._opaque);
84+
/**
85+
* Returns a byte-representation of this opaque object.
86+
*
87+
* @return A new array.
88+
*/
89+
byte[] toBytes();
90+
91+
/**
92+
* Returns the number of bytes in this opaque object;
93+
*
94+
* @return The number of bytes;
95+
*/
96+
int numBytes();
97+
98+
/**
99+
* Returns a Base64 string representing this opaque object.
100+
*
101+
* @return A Base64 string.
102+
*/
103+
String toBase64();
104+
105+
/**
106+
* Writes the bytes of this {@link Opaque} to the given {@link ByteBuffer}.
107+
*
108+
* @param buf The target buffer.
109+
*/
110+
default void putBytes(ByteBuffer buf) {
111+
buf.put(toBytes());
59112
}
60113

114+
/**
115+
* Returns the hashCode based on the byte-representation of this instance.
116+
* <p>
117+
* This method must behave like {@link #defaultHashCode(Opaque)}, but may be optimized.
118+
*
119+
* @return The hashCode.
120+
*/
121+
@Override
122+
int hashCode();
123+
124+
/**
125+
* Compares this object to another one.
126+
* <p>
127+
* This method must behave like {@link #defaultEquals(Opaque, Object)}, but may be optimized.
128+
*
129+
* @return {@code true} if both objects are equal.
130+
*/
61131
@Override
62-
public String toString() {
63-
StringBuilder sb = new StringBuilder();
64-
sb.append('[').append(BaseEncoding.base16().lowerCase().encode(_opaque)).append(']');
65-
return sb.toString();
132+
boolean equals(Object o);
133+
134+
final class OpaqueImpl implements Opaque {
135+
private final byte[] _opaque;
136+
private String base64 = null;
137+
138+
private OpaqueImpl(byte[] opaque) {
139+
_opaque = opaque;
140+
}
141+
142+
@Override
143+
public byte[] toBytes() {
144+
return _opaque.clone();
145+
}
146+
147+
@Override
148+
public String toBase64() {
149+
if (base64 == null) {
150+
base64 = Base64.getEncoder().withoutPadding().encodeToString(_opaque);
151+
}
152+
return base64;
153+
}
154+
155+
@Override
156+
public void putBytes(ByteBuffer buf) {
157+
buf.put(_opaque);
158+
}
159+
160+
@Override
161+
public int hashCode() {
162+
return Arrays.hashCode(_opaque);
163+
}
164+
165+
@Override
166+
public boolean equals(Object o) {
167+
if (o == this) {
168+
return true;
169+
}
170+
if (!(o instanceof Opaque)) {
171+
return false;
172+
}
173+
174+
if (o instanceof OpaqueImpl) {
175+
return Arrays.equals(_opaque, ((OpaqueImpl) o)._opaque);
176+
} else {
177+
return Arrays.equals(_opaque, ((Opaque) o).toBytes());
178+
}
179+
}
180+
181+
/**
182+
* Returns a (potentially non-stable) debug string.
183+
*
184+
* @see #toBase64()
185+
*/
186+
@Override
187+
public String toString() {
188+
return super.toString() + "[" + toBase64() + "]";
189+
}
190+
191+
@Override
192+
public int numBytes() {
193+
return _opaque.length;
194+
}
66195
}
67196
}

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,7 @@ public OpenRecord addOpen(NFS4Client client, StateOwner owner, Inode inode, int
242242
// client explicitly requested write delegation
243243
boolean wantWriteDelegation = (shareAccess & nfs4_prot.OPEN4_SHARE_ACCESS_WANT_WRITE_DELEG) != 0;
244244

245-
Opaque fileId = new Opaque(inode.getFileId());
245+
Opaque fileId = inode.getFileIdKey();
246246
Lock lock = filesLock.get(fileId);
247247
lock.lock();
248248
try {
@@ -376,7 +376,7 @@ public OpenRecord addOpen(NFS4Client client, StateOwner owner, Inode inode, int
376376
public stateid4 downgradeOpen(NFS4Client client, stateid4 stateid, Inode inode, int shareAccess, int shareDeny)
377377
throws ChimeraNFSException {
378378

379-
Opaque fileId = new Opaque(inode.getFileId());
379+
Opaque fileId = inode.getFileIdKey();
380380
Lock lock = filesLock.get(fileId);
381381
lock.lock();
382382
try {
@@ -426,7 +426,7 @@ public stateid4 downgradeOpen(NFS4Client client, stateid4 stateid, Inode inode,
426426
public void delegationReturn(NFS4Client client, stateid4 stateid, Inode inode)
427427
throws ChimeraNFSException {
428428

429-
Opaque fileId = new Opaque(inode.getFileId());
429+
Opaque fileId = inode.getFileIdKey();
430430
Lock lock = filesLock.get(fileId);
431431
lock.lock();
432432
try {
@@ -464,7 +464,7 @@ public void delegationReturn(NFS4Client client, stateid4 stateid, Inode inode)
464464
public int getShareAccess(NFS4Client client, Inode inode, stateid4 stateid)
465465
throws ChimeraNFSException {
466466

467-
Opaque fileId = new Opaque(inode.getFileId());
467+
Opaque fileId = inode.getFileIdKey();
468468
Lock lock = filesLock.get(fileId);
469469
lock.lock();
470470
try {
@@ -527,7 +527,7 @@ public int getShareAccess(NFS4Client client, Inode inode, stateid4 stateid)
527527
*/
528528
void removeOpen(Inode inode, stateid4 stateid) {
529529

530-
Opaque fileId = new Opaque(inode.getFileId());
530+
Opaque fileId = inode.getFileIdKey();
531531
Lock lock = filesLock.get(fileId);
532532
lock.lock();
533533
try {
@@ -565,7 +565,7 @@ void removeOpen(Inode inode, stateid4 stateid) {
565565
public Map<Inode, Collection<NFS4Client>> getOpenFiles() {
566566
return files.entrySet().stream()
567567
.collect(Collectors.toMap(
568-
e -> Inode.forFile(e.getKey().getOpaque()),
568+
e -> Inode.forFileIdKey(e.getKey()),
569569
e -> e.getValue().stream().map(OpenState::getClient).collect(Collectors.toSet())));
570570
}
571571

@@ -578,7 +578,7 @@ public Map<Inode, Collection<NFS4Client>> getOpenFiles() {
578578
public Map<Inode, Collection<NFS4Client>> getDelegations() {
579579
return delegations.entrySet().stream()
580580
.collect(Collectors.toMap(
581-
e -> Inode.forFile(e.getKey().getOpaque()),
581+
e -> Inode.forFileIdKey(e.getKey()),
582582
e -> e.getValue().stream().map(DelegationState::client).collect(Collectors.toSet())));
583583
}
584584
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -628,7 +628,7 @@ public boolean isCallbackNeede() {
628628
public synchronized StateOwner getOrCreateOwner(byte[] owner, seqid4 seq) throws BadSeqidException {
629629
StateOwner stateOwner;
630630
if (_minorVersion == 0) {
631-
Opaque k = new Opaque(owner);
631+
Opaque k = Opaque.forBytes(owner);
632632
stateOwner = _owners.get(k);
633633
if (stateOwner == null) {
634634
state_owner4 so = new state_owner4();
@@ -655,7 +655,7 @@ public synchronized StateOwner getOrCreateOwner(byte[] owner, seqid4 seq) throws
655655
* @param owner client unique state owner
656656
*/
657657
public synchronized void releaseOwner(byte[] owner) throws StaleClientidException {
658-
Opaque k = new Opaque(owner);
658+
Opaque k = Opaque.forBytes(owner);
659659
StateOwner stateOwner = _owners.remove(k);
660660
if (stateOwner == null) {
661661
throw new StaleClientidException();

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,11 +118,11 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF
118118

119119
NlmLock lock = new NlmLock(lockOwner, _args.oplock.locktype, _args.oplock.offset.value,
120120
_args.oplock.length.value);
121-
context.getLm().lock(inode.getFileId(), lock);
121+
context.getLm().lock(inode.getLockKey(), lock);
122122

123123
// ensure, that on close locks will be released
124124
lock_state.addDisposeListener(s -> {
125-
context.getLm().unlockIfExists(inode.getFileId(), lock);
125+
context.getLm().unlockIfExists(inode.getLockKey(), lock);
126126
});
127127

128128
// FIXME: we might run into race condition, thus updating sedid must be fenced!

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ public void process(CompoundContext context, nfs_resop4 result) throws IOExcepti
8585

8686
NlmLock lock = new NlmLock(lockOwner, _args.oplockt.locktype, _args.oplockt.offset.value,
8787
_args.oplockt.length.value);
88-
context.getLm().test(inode.getFileId(), lock);
88+
context.getLm().test(inode.getLockKey(), lock);
8989

9090
result.oplockt.status = nfsstat.NFS_OK;
9191

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF
7676
NlmLock lock = new NlmLock(lockOwner, _args.oplocku.locktype, _args.oplocku.offset.value,
7777
_args.oplocku.length.value);
7878
try {
79-
context.getLm().unlock(inode.getFileId(), lock);
79+
context.getLm().unlock(inode.getLockKey(), lock);
8080
} catch (LockRangeUnavailabeException e) {
8181
// posix locks allows unlocking of not locked regions
8282
}

0 commit comments

Comments
 (0)