From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Wed, 2 Feb 2011 19:05:25 +0000 (-0700)
Subject: Fixed Rock MapDir read and write locking:
X-Git-Tag: take02~21
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1adea2a655e4479e2682f5ae157dd19686f6153c;p=thirdparty%2Fsquid.git

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 55ff9881cd..dc01e24b0e 100644
--- 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