nfs4: bump open-stateid sequence only in FileTracker#addOpen

Motivation: To avoid race conditions, the updating of the open-stateid must happen only under lock, which is guaranteed by FileTracker#addOpen Modification: Update open-stateid sequence only in FileTracker#addOpen Result: Fix of the race condition Fixes: #120 Acked-by: Paul Millar Target: master, 0.24, 0.23 (cherry picked from commit 42ed2e0) Signed-off-by: Tigran Mkrtchyan <[email protected]>
dCache · Mar 28, 2023 · 056ffe3 · 056ffe3
1 parent 91ad32f
commit 056ffe3
Show file tree

Hide file tree

Showing 3 changed files with 7 additions and 5 deletions.
diff --git a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java b/core/src/main/java/org/dcache/nfs/v4/FileTracker.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017 - 2020 Deutsches Elektronen-Synchroton,
+ * Copyright (c) 2017 - 2023 Deutsches Elektronen-Synchroton,
  * Member of the Helmholtz Association, (DESY), HAMBURG, GERMANY
  *
  * This library is free software; you can redistribute it and/or modify
@@ -120,14 +120,15 @@ public stateid4 addOpen(NFS4Client client, StateOwner owner, Inode inode, int sh
                     throw new ShareDeniedException("Conflicting share");
             }
 
-            // if there is an another open from the same client we must merge
+            // if there is another open from the same client we must merge
             // access mode and return the same stateid as required by rfc5661#18.16.3
 
             for (OpenState os : opens) {
                 if (os.client.getId() == client.getId() &&
                         os.getOwner().equals(owner)) {
                         os.shareAccess |= shareAccess;
                         os.shareDeny |= shareDeny;
+                        os.stateid.seqid++;
                         return os.stateid;
                 }
             }
@@ -137,6 +138,7 @@ public stateid4 addOpen(NFS4Client client, StateOwner owner, Inode inode, int sh
             OpenState openState = new OpenState(client, owner, stateid, shareAccess, shareDeny);
             opens.add(openState);
             state.addDisposeListener(s -> removeOpen(inode, stateid));
+            stateid.seqid++;
             return stateid;
         } finally {
             lock.unlock();

diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationLOCK.java b/core/src/main/java/org/dcache/nfs/v4/OperationLOCK.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009 - 2018 Deutsches Elektronen-Synchroton,
+ * Copyright (c) 2009 - 2023 Deutsches Elektronen-Synchroton,
  * Member of the Helmholtz Association, (DESY), HAMBURG, GERMANY
  *
  * This library is free software; you can redistribute it and/or modify
@@ -122,6 +122,7 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF
                 context.getLm().unlockIfExists(inode.getFileId(), lock);
             });
 
+            // FIXME: we might run into race condition, thus updating sedid must be fenced!
             lock_state.bumpSeqid();
             context.currentStateid(lock_state.stateid());
             result.oplock.status = nfsstat.NFS_OK;

diff --git a/core/src/main/java/org/dcache/nfs/v4/OperationOPEN.java b/core/src/main/java/org/dcache/nfs/v4/OperationOPEN.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009 - 2020 Deutsches Elektronen-Synchroton,
+ * Copyright (c) 2009 - 2023 Deutsches Elektronen-Synchroton,
  * Member of the Helmholtz Association, (DESY), HAMBURG, GERMANY
  *
  * This library is free software; you can redistribute it and/or modify
@@ -271,7 +271,6 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF
                 _args.opopen.share_access.value,
                 _args.opopen.share_deny.value);
 
-        stateid.seqid++;
         context.currentStateid(stateid);
         res.resok4.stateid = stateid;
         res.status = nfsstat.NFS_OK;