pool: Avoid lock contention when opening files and setting sticky flags

Motivation:

To avoid races between the change notifications during file opening/setting
sticky flags and repository initialization, the repository acquires an
exclusive lock around the notification logic. This is a contention point
when opening several files concurrently, in particular when opening the
file requires reads from the meta DB.

Modification:

Replace the lock with a read write lock to allow open/set sticky operations to
be executed concurrently.

The patch is deliberately minimalistic to allow backporting to 2.13 and 2.14.
Bigger changes may be made on master in separate patches.

Result:

Resolved lock contention when opening files or changing sticky flags on pools.

Target: trunk
Request: 2.14
Request: 2.13
Require-notes: yes
Require-book: no
Acked-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
Patch: https://rb.dcache.org/r/8924/
(cherry picked from commit 0d9b3b1)

modules/dcache/src/main/java/org/dcache/pool/repository/v5/CacheRepositoryV5.java

@@ -21,6 +21,8 @@
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import diskCacheV111.util.CacheException;
 import diskCacheV111.util.DiskErrorCacheException;
@@ -86,6 +88,7 @@ public class CacheRepositoryV5
      * The following order must be observed when synchronizing:
      *
      *  - this
+     *  - _stateLock
      *  - entries (only one)
      *  - _account
      *
@@ -127,7 +130,7 @@ public class CacheRepositoryV5
     /**
      * Meta data about files in the pool.
      */
-    private MetaDataStore _store;
+    private volatile MetaDataStore _store;
 
     /**
      * Current state of the repository.
@@ -144,6 +147,16 @@ enum State {
 
     private volatile State _state = State.UNINITIALIZED;
 
+    /**
+     * Lock for the _state field.
+     *
+     * The _state field itself is volatile and may be accessed without locking
+     * by threads that are only interested in the current state, however updates
+     * to _state must obtain a write lock and threads that want to block state
+     * changes in a critical region must obtain a read lock.
+     */
+    private final ReadWriteLock _stateLock = new ReentrantReadWriteLock();
+
     /**
      * Initialization progress between 0 and 1.
      */
@@ -314,24 +327,28 @@ public void init()
         assert _account != null : "Account must be set";
         assert _allocator != null : "Account must be set";
 
-        synchronized (this) {
+        _stateLock.writeLock().lock();
+        try {
             if (_state != State.UNINITIALIZED) {
                 throw new IllegalStateException("Can only initialize repository once.");
             }
-
             _state = State.INITIALIZING;
+        } finally {
+            _stateLock.writeLock().unlock();
         }
 
-        /* Instantiating the cache causes the listing to be
-         * generated to prepopulate the cache. That may take some
-         * time. Therefore we do this outside the synchronization.
+        /* Instantiating the cache causes the listing to be generated to
+         * populate the cache. This may take some time and therefore we
+         * do this outside the synchronization.
          */
-        _log.warn("Reading inventory from " + _store);
-        MetaDataCache cache = new MetaDataCache(_store);
+        _log.warn("Reading inventory from {}", _store);
+        _store = new MetaDataCache(_store);
 
-        synchronized (this) {
-            _store = cache;
+        _stateLock.writeLock().lock();
+        try {
             _state = State.INITIALIZED;
+        } finally {
+            _stateLock.writeLock().unlock();
         }
     }
 
@@ -341,11 +358,14 @@ public void load()
                InterruptedException
     {
         try {
-            synchronized (this) {
+            _stateLock.writeLock().lock();
+            try {
                 if (_state != State.INITIALIZED) {
                     throw new IllegalStateException("Can only load repository after initialization and only once.");
                 }
                 _state = State.LOADING;
+            } finally {
+                _stateLock.writeLock().unlock();
             }
 
             List<PnfsId> ids = new ArrayList<>(_store.index());
@@ -389,7 +409,12 @@ public void load()
              * repository is loading. We synchronize to ensure a clean
              * switch from the LOADING state to the OPEN state.
              */
-            synchronized (this) {
+            _stateLock.writeLock().lock();
+            try {
+                if (_state != State.LOADING) {
+                    throw new IllegalStateException("Repository was closed during loading.");
+                }
+
                 /* Register with event listeners in LRU order. The
                  * sweeper relies on the LRU order.
                  */
@@ -406,6 +431,8 @@ public void load()
                           usedDataSpace, _account.getFree());
 
                 _state = State.OPEN;
+            } finally {
+                _stateLock.writeLock().unlock();
             }
 
             /* Register sticky timeouts.
@@ -419,10 +446,13 @@ public void load()
                 }
             }
         } finally {
-            synchronized (this) {
+            _stateLock.writeLock().lock();
+            try {
                 if (_state != State.OPEN) {
                     _state = State.FAILED;
                 }
+            } finally {
+                _stateLock.writeLock().unlock();
             }
         }
 
@@ -537,7 +567,8 @@ public ReplicaDescriptor openEntry(PnfsId id, Set<OpenFlags> flags)
             }
 
             if (!flags.contains(OpenFlags.NOATIME)) {
-                synchronized (this) {
+                _stateLock.readLock().lock();
+                try {
                     /* Don't notify listeners until we are done
                      * loading; at the end of the load method
                      * listeners are informed about all entries.
@@ -550,6 +581,8 @@ public ReplicaDescriptor openEntry(PnfsId id, Set<OpenFlags> flags)
                             accessTimeChanged(oldEntry, newEntry);
                         }
                     }
+                } finally {
+                    _stateLock.readLock().unlock();
                 }
             }
 
@@ -630,10 +663,14 @@ public void setSticky(PnfsId id, String owner,
             throw e;
         }
 
-        /* We synchronize on 'this' because setSticky below is
-         * synchronized; and 'this' has to be locked before entry.
+        /* We acquire the state lock to avoid conflicts with the
+         * load method; at the end of load method expiration tasks are
+         * scheduled. For that reason this method does not generate
+         * sticky changed notification and does not schedule
+         * expiration tasks if the repository is not OPEN.
          */
-        synchronized (this) {
+        _stateLock.readLock().lock();
+        try {
             synchronized (entry) {
                 switch (entry.getState()) {
                 case NEW:
@@ -650,8 +687,23 @@ public void setSticky(PnfsId id, String owner,
                     break;
                 }
 
-                setSticky(entry, owner, expire, overwrite);
+                try {
+                    CacheEntry oldEntry = new CacheEntryImpl(entry);
+                    if (entry.setSticky(owner, expire, overwrite) && _state == State.OPEN) {
+                        CacheEntryImpl newEntry = new CacheEntryImpl(entry);
+                        stickyChanged(oldEntry, newEntry, new StickyRecord(owner, expire));
+                        scheduleExpirationTask(entry);
+                    }
+                } catch (CacheException e) {
+                    fail(FaultAction.READONLY, "Internal repository error", e);
+                    throw new RuntimeException("Internal repository error", e);
+                } catch (RuntimeException e) {
+                    fail(FaultAction.DEAD, "Internal repository error", e);
+                    throw e;
+                }
             }
+        } finally {
+            _stateLock.readLock().unlock();
         }
     }
 
@@ -823,11 +875,16 @@ public void getInfo(PrintWriter pw)
         pw.println("    Runtime configured   : " + UnitInteger.toUnitString(_runtimeMaxSize));
     }
 
-    public synchronized void shutdown()
+    public void shutdown()
     {
-        _stateChangeListeners.stop();
-        _state = State.CLOSED;
-        _store.close();
+        _stateLock.writeLock().lock();
+        try {
+            _stateChangeListeners.stop();
+            _state = State.CLOSED;
+            _store.close();
+        } finally {
+            _stateLock.writeLock().unlock();
+        }
     }
 
     // Operations on MetaDataRecord ///////////////////////////////////////
@@ -942,16 +999,17 @@ void setState(MetaDataRecord entry, EntryState state)
     /**
      * Package local method for changing sticky records of an entry.
      */
-    synchronized void setSticky(MetaDataRecord entry, String owner,
+    void setSticky(MetaDataRecord entry, String owner,
                    long expire, boolean overwrite)
         throws IllegalArgumentException
     {
-        /* This method is synchronized to avoid conflicts with the
+        /* We acquire the state lock to avoid conflicts with the
          * load method; at the end of load method expiration tasks are
          * scheduled. For that reason this method does not generate
          * sticky changed notification and does not schedule
          * expiration tasks if the repository is not OPEN.
          */
+        _stateLock.readLock().lock();
         try {
             synchronized (entry) {
                 CacheEntry oldEntry = new CacheEntryImpl(entry);
@@ -967,6 +1025,8 @@ synchronized void setSticky(MetaDataRecord entry, String owner,
         } catch (RuntimeException e) {
             fail(FaultAction.DEAD, "Internal repository error", e);
             throw e;
+        } finally {
+            _stateLock.readLock().unlock();
         }
     }