Fixed Rock MapDir read and write locking:

The IoState object created by openStoreIO() can be used for many reads. Thus,
incrementing read level at open and decrementing it at [each] readCompleted
leads to negative read levels if the stored object need more than one I/O.

Moreover, the only way core Squid can swap in an entry is if an entry has our
fileno set (by our get()). Thus, the slot is already locked for reading by
get(), with the entry responsible for decreasing the read level upon
destruction. We do not need to open/close for reading in
openStoreIO/readComleted.

When writing fails, invalidate the slot before unlocking it.

diff --git a/src/fs/rock/RockSwapDir.cc b/src/fs/rock/RockSwapDir.cc
index 55ff9881cd76d3cc3120dbcc5fe435c42e913c82..dc01e24b0e07f1ffb652f78981564497e0d76572 100644 (file)
--- a/src/fs/rock/RockSwapDir.cc
+++ b/src/fs/rock/RockSwapDir.cc
@@ -300,6 +300,9 @@ Rock::SwapDir::addEntry(const int fileno, const StoreEntry &from)
        << ", fileno="<< std::setfill('0') << std::hex << std::uppercase <<
        std::setw(8) << fileno);
 
+    // after writing, we close for reading because we do not add this entry to
+    // store_table and, hence, there is nobody to hold the read lock
+
     int idx;
     StoreEntryBasics *const basics = map->openForWriting(key, idx);
     if (!basics) {
@@ -399,13 +402,14 @@ Rock::SwapDir::openStoreIO(StoreEntry &e, StoreIOState::STFNCB *cbFile, StoreIOS
         return NULL;
     }
 
-    if (!map->openForReadingAt(e.swap_filen)) {
-        debugs(47,1, HERE << "bug: dir " << index << " lost locked fileno: " <<
-            std::setfill('0') << std::hex << std::uppercase << std::setw(8) <<
-            e.swap_filen);
+    if (e.swap_filen < 0) { 
+        debugs(47,4, HERE << e);
         return NULL;
     }
 
+    // The only way the entry has swap_filen is if get() locked it for reading
+    // so we do not need to map->openForReadingAt(swap_filen) again here.
+
     IoState *sio = new IoState(this, &e, cbFile, cbIo, data);
 
     sio->swap_dirn = index;
@@ -464,7 +468,6 @@ Rock::SwapDir::readCompleted(const char *buf, int rlen, int errflag, RefCount< :
     ReadRequest *request = dynamic_cast<Rock::ReadRequest*>(r.getRaw());
     assert(request);
     IoState::Pointer sio = request->sio;
-    map->closeForReading(sio->swap_filen);
 
     // do not increment sio->offset_: callers always supply relative offset
 
@@ -483,11 +486,13 @@ Rock::SwapDir::writeCompleted(int errflag, size_t rlen, RefCount< ::WriteRequest
     assert(request);
     assert(request->sio !=  NULL);
     IoState &sio = *request->sio;
-    map->closeForWriting(sio.swap_filen);
+
     if (errflag != DISK_OK)
         map->free(sio.swap_filen); // TODO: test by forcing failure
     // else sio.offset_ += rlen;
 
+    map->closeForWriting(sio.swap_filen); // assume we only write once
+
     // TODO: always compute cur_size based on map, do not store it
     cur_size = (HeaderSize + max_objsize * map->entryCount()) >> 10;
     assert(sio.offset_ <= diskOffsetLimit()); // post-factum check