Revised Core/Store cooperation w.r.t. SwapDir map slot locks.

When StoreEntry is deleted, we need to release the SwapDir map slot locks it
holds, if any. This is difficult because SwapDir maintains the locks while
Squid Core maintains the entry swap_status. The Core gets swap_status-related
notifications using async calls so it is easy for swap_status to get out of
sync if SwapDir updates the map slot proactively.

The new code no longer releases the slot lock until the associated StoreEntry
is unlinked or gone, even if the slot is known to be unusable and waiting to
be deleted. We also do not rely on swap_status to guess which lock to release;
we use slot state to determine that instead.

Removed rock-specific code from StoreEntry destructor by introducing a general
SwapDir::disconnect(StoreEntry&) API.

Polished StoreEntry initialization.

Polished and fixed Rock::SwapDir debugging.

diff --git a/src/SwapDir.h b/src/SwapDir.h
index 8e8187c48e8da644ade8744f7e2ecb3e32b408ec..e5a4cddd064fc3882395ceb76eed36307da55d8b 100644 (file)
--- a/src/SwapDir.h
+++ b/src/SwapDir.h
@@ -139,6 +139,9 @@ public:
     /* migrated from store_dir.cc */
     bool objectSizeIsAcceptable(int64_t objsize) const;
 
+    /// called when the entry is about to forget its association with cache_dir
+    virtual void disconnect(StoreEntry &) {}
+
 protected:
     void parseOptions(int reconfiguring);
     void dumpOptions(StoreEntry * e) const;

diff --git a/src/fs/rock/RockDirMap.cc b/src/fs/rock/RockDirMap.cc
index b014bdce5fe63e6f435cf458094a6a0c4571250f..3d716fdc567c32235f4ef07321659d24d47c2421 100644 (file)
--- a/src/fs/rock/RockDirMap.cc
+++ b/src/fs/rock/RockDirMap.cc
@@ -68,6 +68,7 @@ Rock::DirMap::closeForWriting(const sfileno fileno)
     s.switchExclusiveToSharedLock();
 }
 
+/// terminate writing the entry, freeing its slot for others to use
 void
 Rock::DirMap::abortWriting(const sfileno fileno)
 {
@@ -79,6 +80,22 @@ Rock::DirMap::abortWriting(const sfileno fileno)
     freeLocked(s);
 }
 
+void
+Rock::DirMap::abortIo(const sfileno fileno)
+{
+    debugs(79, 5, HERE << " abort I/O for slot at " << fileno <<
+           " in map [" << path << ']');
+    assert(valid(fileno));
+    Slot &s = shared->slots[fileno];
+
+    // The caller is a lock holder. Thus, if we are Writeable, then the
+    // caller must be the writer; otherwise the caller must be the reader.
+    if (s.state == Slot::Writeable)
+        abortWriting(fileno);
+    else
+        closeForReading(fileno);
+}
+
 bool
 Rock::DirMap::putAt(const StoreEntry &e, const sfileno fileno)
 {
@@ -176,6 +193,7 @@ Rock::DirMap::closeForReading(const sfileno fileno)
            "map [" << path << ']');
     assert(valid(fileno));
     Slot &s = shared->slots[fileno];
+    assert(s.state == Slot::Readable);
     s.releaseSharedLock();
     freeIfNeeded(s);
 }

diff --git a/src/fs/rock/RockDirMap.h b/src/fs/rock/RockDirMap.h
index 586800b63e63d1d32f25c652939405c268d2ebcb..fcd48ab4de4cc944ebbe1b26b37088978102dd8c 100644 (file)
--- a/src/fs/rock/RockDirMap.h
+++ b/src/fs/rock/RockDirMap.h
@@ -64,8 +64,6 @@ public:
     StoreEntryBasics *openForWriting(const cache_key *const key, sfileno &fileno);
     /// successfully finish writing the entry, leaving it opened for reading
     void closeForWriting(const sfileno fileno);
-    /// terminate writing the entry, freeing its slot for others to use
-    void abortWriting(const sfileno fileno);
 
     /// stores entry info at the requested slot or returns false
     bool putAt(const StoreEntry &e, const sfileno fileno);
@@ -80,6 +78,9 @@ public:
     /// close slot after reading, decrements read level
     void closeForReading(const sfileno fileno);
 
+    /// called by lock holder to terminate either slot writing or reading
+    void abortIo(const sfileno fileno);
+
     bool full() const; ///< there are no empty slots left
     bool valid(int n) const; ///< whether n is a valid slot coordinate
     int entryCount() const; ///< number of used slots
@@ -100,6 +101,7 @@ private:
     int slotIdx(const cache_key *const key) const;
     Slot &slot(const cache_key *const key);
     const StoreEntryBasics *openForReading(Slot &s);
+    void abortWriting(const sfileno fileno);
     void freeIfNeeded(Slot &s);
     void freeLocked(Slot &s);
     String sharedMemoryName();

diff --git a/src/fs/rock/RockSwapDir.cc b/src/fs/rock/RockSwapDir.cc
index 6d0b0de56ce861876197d31bd70fd1a91d040cb2..b11c9ba0edec12bc5ac0dbef0daa8152b03f31ed 100644 (file)
--- a/src/fs/rock/RockSwapDir.cc
+++ b/src/fs/rock/RockSwapDir.cc
@@ -76,14 +76,21 @@ Rock::SwapDir::get(const cache_key *key)
     // the disk entry remains open for reading, protected from modifications
 }
 
-void
-Rock::SwapDir::closeForReading(StoreEntry &e)
+void Rock::SwapDir::disconnect(StoreEntry &e)
 {
-    assert(index == e.swap_dirn);
+    assert(e.swap_dirn == index);
     assert(e.swap_filen >= 0);
-    map->closeForReading(e.swap_filen);
+    // cannot have SWAPOUT_NONE entry with swap_filen >= 0
+    assert(e.swap_status != SWAPOUT_NONE);
+
+    // do not rely on e.swap_status here because there is an async delay
+    // before it switches from SWAPOUT_WRITING to SWAPOUT_DONE.
+
+    // since e has swap_filen, its slot is locked for either reading or writing
+    map->abortIo(e.swap_filen);
     e.swap_dirn = -1;
     e.swap_filen = -1;
+    e.swap_status = SWAPOUT_NONE;
 }
 
 // TODO: encapsulate as a tool; identical to CossSwapDir::create()
@@ -475,11 +482,15 @@ Rock::SwapDir::writeCompleted(int errflag, size_t rlen, RefCount< ::WriteRequest
     assert(request->sio !=  NULL);
     IoState &sio = *request->sio;
 
-    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
+    if (errflag == DISK_OK) {
+        // close, assuming we only write once; the entry gets the read lock
+        map->closeForWriting(sio.swap_filen);
+        // and sio.offset_ += rlen;
+    } else {
+        // Do not abortWriting here. The entry should keep the write lock
+        // instead of losing association with the store and confusing core.
+        map->free(sio.swap_filen); // will mark as unusable, just in case
+    }
 
     // TODO: always compute cur_size based on map, do not store it
     cur_size = (HeaderSize + max_objsize * map->entryCount()) >> 10;
@@ -584,15 +595,16 @@ Rock::SwapDir::dereference(StoreEntry &e)
 void
 Rock::SwapDir::unlink(StoreEntry &e)
 {
-    debugs(47, 5, HERE << &e << ' ' << e.swap_dirn << ' ' << e.swap_filen);
+    debugs(47, 5, HERE << e);
     ignoreReferences(e);
     map->free(e.swap_filen);
+    disconnect(e);
 }
 
 void
 Rock::SwapDir::trackReferences(StoreEntry &e)
 {
-    debugs(47, 5, HERE << *e);
+    debugs(47, 5, HERE << e);
     repl->Add(repl, &e, &e.repl);
 }
 
@@ -600,7 +612,7 @@ Rock::SwapDir::trackReferences(StoreEntry &e)
 void
 Rock::SwapDir::ignoreReferences(StoreEntry &e)
 {
-    debugs(47, 5, HERE << *e);
+    debugs(47, 5, HERE << e);
     repl->Remove(repl, &e, &e.repl);
 }
 

diff --git a/src/fs/rock/RockSwapDir.h b/src/fs/rock/RockSwapDir.h
index fb1336898b92ee3158d4da316bbd13f0ee3b1b51..375fd530d1b38a0a66bc3a98038897b6b0d29c0c 100644 (file)
--- a/src/fs/rock/RockSwapDir.h
+++ b/src/fs/rock/RockSwapDir.h
@@ -25,8 +25,7 @@ public:
     virtual void reconfigure(int, char *);
     virtual StoreSearch *search(String const url, HttpRequest *);
     virtual StoreEntry *get(const cache_key *key);
-
-    void closeForReading(StoreEntry &e);
+    virtual void disconnect(StoreEntry &e);
 
 protected:
     /* protected ::SwapDir API */

diff --git a/src/store.cc b/src/store.cc
index 6284558a83ad2f520ef5eade03a49b6284c95c1b..4c70ad6388f6521c41fd90cf6eec0e8b5c5e2595 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -53,7 +53,6 @@
 #include "SquidTime.h"
 #include "swap_log_op.h"
 #include "mgr/StoreIoAction.h"
-#include "fs/rock/RockSwapDir.h"
 
 static STMCB storeWriteComplete;
 
@@ -372,6 +371,7 @@ StoreEntry::StoreEntry():
 
     expires = lastmod = lastref = timestamp = -1;
 
+    swap_status = SWAPOUT_NONE;
     swap_filen = -1;
     swap_dirn = -1;
 }
@@ -384,6 +384,7 @@ StoreEntry::StoreEntry(const char *aUrl, const char *aLogUrl):
 
     expires = lastmod = lastref = timestamp = -1;
 
+    swap_status = SWAPOUT_NONE;
     swap_filen = -1;
     swap_dirn = -1;
 }
@@ -391,10 +392,8 @@ StoreEntry::StoreEntry(const char *aUrl, const char *aLogUrl):
 StoreEntry::~StoreEntry()
 {
     if (swap_filen >= 0) {
-        // XXX: support cache types other than Rock
-        Rock::SwapDir &rockSwapDir =
-            dynamic_cast<Rock::SwapDir &>(*store());
-        rockSwapDir.closeForReading(*this);
+        SwapDir &sd = dynamic_cast<SwapDir&>(*store());
+        sd.disconnect(*this);
     }
 }
 
@@ -798,9 +797,6 @@ storeCreateEntry(const char *url, const char *log_url, request_flags flags, cons
 
     e->store_status = STORE_PENDING;
     e->setMemStatus(NOT_IN_MEMORY);
-    e->swap_status = SWAPOUT_NONE;
-    e->swap_filen = -1;
-    e->swap_dirn = -1;
     e->refcount = 0;
     e->lastref = squid_curtime;
     e->timestamp = -1;          /* set in StoreEntry::timestampsSet() */
@@ -1279,21 +1275,18 @@ StoreEntry::release()
     storeLog(STORE_LOG_RELEASE, this);
 
     if (swap_filen > -1) {
-        unlink();
 
+        // update size before unlink() below clears swap_status
+        // TODO: the store/SwapDir::unlink should update the size!
         if (swap_status == SWAPOUT_DONE)
             if (EBIT_TEST(flags, ENTRY_VALIDATED))
                 store()->updateSize(swap_file_sz, -1);
 
+        // log before unlink() below clears swap_filen
         if (!EBIT_TEST(flags, KEY_PRIVATE))
             storeDirSwapLog(this, SWAP_LOG_DEL);
 
-#if 0
-        /* From 2.4. I think we do this in storeUnlink? */
-        storeSwapFileNumberSet(this, -1);
-
-#endif
-
+        unlink();
     }
 
     setMemStatus(NOT_IN_MEMORY);
@@ -2013,7 +2006,10 @@ StoreEntry::store() const
 void
 StoreEntry::unlink()
 {
-    store()->unlink(*this);
+    store()->unlink(*this); // implies disconnect()
+    swap_filen = -1;
+    swap_dirn = -1;
+    swap_status = SWAPOUT_NONE;
 }
 
 /*

diff --git a/src/store_swapin.cc b/src/store_swapin.cc
index b6ca9b759f4c8a230e530ceac5be208a8ed19085..d993b62d06d19284bda21ebdce92f5085fc1a7a2 100644 (file)
--- a/src/store_swapin.cc
+++ b/src/store_swapin.cc
@@ -97,6 +97,7 @@ storeSwapInFileNotify(void *data, int errflag, StoreIOState::Pointer self)
            e->swap_dirn << " to " << sc->swapin_sio->swap_filen << "/" <<
            sc->swapin_sio->swap_dirn);
 
+    assert(e->swap_filen < 0); // if this fails, call SwapDir::disconnect(e)
     e->swap_filen = sc->swapin_sio->swap_filen;
     e->swap_dirn = sc->swapin_sio->swap_dirn;
 }

diff --git a/src/store_swapout.cc b/src/store_swapout.cc
index 83971398970c8beea66ee9f51818609fe956ac50..b0981da0716d5d3a6001e2028977e0732eaad215 100644 (file)
--- a/src/store_swapout.cc
+++ b/src/store_swapout.cc
@@ -115,6 +115,7 @@ storeSwapOutFileNotify(void *data, int errflag, StoreIOState::Pointer self)
     assert(mem);
     assert(mem->swapout.sio == self);
     assert(errflag == 0);
+    assert(e->swap_filen < 0); // if this fails, call SwapDir::disconnect(e)
     e->swap_filen = mem->swapout.sio->swap_filen;
     e->swap_dirn = mem->swapout.sio->swap_dirn;
 }
@@ -340,11 +341,7 @@ storeSwapOutFileClosed(void *data, int errflag, StoreIOState::Pointer self)
         if (e->swap_filen > 0)
             e->unlink();
 
-        e->swap_filen = -1;
-
-        e->swap_dirn = -1;
-
-        e->swap_status = SWAPOUT_NONE;
+        assert(e->swap_status == SWAPOUT_NONE);
 
         e->releaseRequest();
     } else {