Do not lock an SMP hit twice for the same transaction (#1134)

Before commit 4310f8b, anchorToCache() (known then as anchorCollapsed())
was only called for not-yet-anchored entries. For anchored entries, we
called updateCollapsed(). In that commit, a new anchorToCache() caller
(i.e. allowSharing() after peek()) started calling anchorToCache() for
all entries, but anchorToCache() code was not updated to skip anchoring
of anchored entries, leading to an extra read lock placed on a peek()ed
hit entry. The extra lock prevented the entry slot from being reused for
other entries when the entry was later purged (i.e. marked for removal),
drastically decreasing hit ratio in some environments.

Fixed anchorToCache() no longer anchors anchored entries. To make sure
we covered all callers/cases, especially in patches, we changed the call
parameters, addressing an old code simplification TODO.

diff --git a/src/MemStore.cc b/src/MemStore.cc
index 9dbd3289889d1d84b3115cf224376b0652c2d67e..68b2585c66481a7d939fb2a55a409a68210d50dc 100644 (file)
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -390,8 +390,11 @@ MemStore::updateHeadersOrThrow(Ipc::StoreMapUpdate &update)
 }
 
 bool
-MemStore::anchorToCache(StoreEntry &entry, bool &inSync)
+MemStore::anchorToCache(StoreEntry &entry)
 {
+    Assure(!entry.hasMemStore());
+    Assure(entry.mem().memCache.io != MemObject::ioDone);
+
     if (!map)
         return false;
 
@@ -402,8 +405,9 @@ MemStore::anchorToCache(StoreEntry &entry, bool &inSync)
         return false;
 
     anchorEntry(entry, index, *slot);
-    inSync = updateAnchoredWith(entry, index, *slot);
-    return true; // even if inSync is false
+    if (!updateAnchoredWith(entry, index, *slot))
+        throw TextException("updateAnchoredWith() failure", Here());
+    return true;
 }
 
 bool

diff --git a/src/MemStore.h b/src/MemStore.h
index 259312d24be992957ae4e3487c75f4b8d0a71949..7c94a33f768add801db7ab2a65834cffda95f74d 100644 (file)
--- a/src/MemStore.h
+++ b/src/MemStore.h
@@ -59,7 +59,7 @@ public:
     virtual bool dereference(StoreEntry &e) override;
     virtual void updateHeaders(StoreEntry *e) override;
     virtual void maintain() override;
-    virtual bool anchorToCache(StoreEntry &e, bool &inSync) override;
+    virtual bool anchorToCache(StoreEntry &) override;
     virtual bool updateAnchored(StoreEntry &) override;
     virtual void evictCached(StoreEntry &) override;
     virtual void evictIfFound(const cache_key *) override;

diff --git a/src/fs/rock/RockSwapDir.cc b/src/fs/rock/RockSwapDir.cc
index c3d7c79d67c42130a94e9b0beb74deb6a9c463bc..3f002bc88d038aa0ca9ba227b0f5d06fd4340a43 100644 (file)
--- a/src/fs/rock/RockSwapDir.cc
+++ b/src/fs/rock/RockSwapDir.cc
@@ -72,8 +72,10 @@ Rock::SwapDir::get(const cache_key *key)
 }
 
 bool
-Rock::SwapDir::anchorToCache(StoreEntry &entry, bool &inSync)
+Rock::SwapDir::anchorToCache(StoreEntry &entry)
 {
+    Assure(!entry.hasDisk());
+
     if (!map || !theFile || !theFile->canRead())
         return false;
 
@@ -84,8 +86,7 @@ Rock::SwapDir::anchorToCache(StoreEntry &entry, bool &inSync)
         return false;
 
     anchorEntry(entry, filen, *slot);
-    inSync = updateAnchoredWith(entry, *slot);
-    return true; // even if inSync is false
+    return true;
 }
 
 bool
@@ -96,13 +97,7 @@ Rock::SwapDir::updateAnchored(StoreEntry &entry)
 
     assert(entry.hasDisk(index));
 
-    const Ipc::StoreMapAnchor &s = map->readableEntry(entry.swap_filen);
-    return updateAnchoredWith(entry, s);
-}
-
-bool
-Rock::SwapDir::updateAnchoredWith(StoreEntry &entry, const Ipc::StoreMapAnchor &anchor)
-{
+    const auto &anchor = map->readableEntry(entry.swap_filen);
     entry.swap_file_sz = anchor.basics.swap_file_sz;
     return true;
 }

diff --git a/src/fs/rock/RockSwapDir.h b/src/fs/rock/RockSwapDir.h
index d0acea59a15e0c200744377192174b27ea36a2ad..7619a9e460fa3a5e63f4cc801746069bcca05e98 100644 (file)
--- a/src/fs/rock/RockSwapDir.h
+++ b/src/fs/rock/RockSwapDir.h
@@ -84,7 +84,7 @@ public:
 
 protected:
     /* Store API */
-    virtual bool anchorToCache(StoreEntry &entry, bool &inSync);
+    virtual bool anchorToCache(StoreEntry &);
     virtual bool updateAnchored(StoreEntry &);
 
     /* protected ::SwapDir API */
@@ -128,7 +128,6 @@ protected:
     StoreIOState::Pointer createUpdateIO(const Ipc::StoreMapUpdate &update, StoreIOState::STFNCB *, StoreIOState::STIOCB *, void *);
 
     void anchorEntry(StoreEntry &e, const sfileno filen, const Ipc::StoreMapAnchor &anchor);
-    bool updateAnchoredWith(StoreEntry &, const Ipc::StoreMapAnchor &);
 
     friend class Rebuild;
     friend class IoState;

diff --git a/src/store/Controlled.h b/src/store/Controlled.h
index 7f4f7be09d6fead652409be6d48f677f4fdc6492..6dea1f29dcdf5e6fb5f4134eeb67faab13bb14ba 100644 (file)
--- a/src/store/Controlled.h
+++ b/src/store/Controlled.h
@@ -34,10 +34,9 @@ public:
     /// make stored metadata and HTTP headers the same as in the given entry
     virtual void updateHeaders(StoreEntry *) {}
 
-    /// If Transients entry cannot be attached to this storage, return false.
-    /// If the entry is not found, return false. Otherwise, return true after
-    /// tying the entry to this cache and setting inSync to updateAnchored().
-    virtual bool anchorToCache(StoreEntry &, bool &/*inSync*/) { return false; }
+    /// tie StoreEntry to this storage if this storage has a matching entry
+    /// \retval true if this storage has a matching entry
+    virtual bool anchorToCache(StoreEntry &) { return false; }
 
     /// Update a local Transients entry with fresh info from this cache (if any).
     /// Return true iff the cache supports Transients entries and

diff --git a/src/store/Controller.cc b/src/store/Controller.cc
index ec48c8e48c92ae8c976953c3b2d4755c42a388ce..ec0049e9a2150fe483e3c2156ef1ee5224fd0200 100644 (file)
--- a/src/store/Controller.cc
+++ b/src/store/Controller.cc
@@ -369,18 +369,13 @@ Store::Controller::find(const cache_key *key)
 void
 Store::Controller::allowSharing(StoreEntry &entry, const cache_key *key)
 {
-    // TODO: refactor to throw on anchorToCache() inSync errors!
-
     // anchorToCache() below and many find() callers expect a registered entry
     addReading(&entry, key);
 
     if (entry.hasTransients()) {
         // store hadWriter before computing `found`; \see Transients::get()
         const auto hadWriter = transients->hasWriter(entry);
-        bool inSync = false;
-        const bool found = anchorToCache(entry, inSync);
-        if (found && !inSync)
-            throw TexcHere("cannot sync");
+        const auto found = anchorToCache(entry);
         if (!found) {
             // !found should imply hittingRequiresCollapsing() regardless of writer presence
             if (!entry.hittingRequiresCollapsing()) {
@@ -811,7 +806,7 @@ Store::Controller::syncCollapsed(const sfileno xitIndex)
     if (sharedMemStore && collapsed->mem_obj->memCache.io == MemObject::ioDone) {
         found = true;
         inSync = true;
-        debugs(20, 7, "fully mem-loaded " << *collapsed);
+        debugs(20, 7, "already handled by memory store: " << *collapsed);
     } else if (sharedMemStore && collapsed->hasMemStore()) {
         found = true;
         inSync = sharedMemStore->updateAnchored(*collapsed);
@@ -820,7 +815,15 @@ Store::Controller::syncCollapsed(const sfileno xitIndex)
         found = true;
         inSync = swapDir->updateAnchored(*collapsed);
     } else {
-        found = anchorToCache(*collapsed, inSync);
+        try {
+            found = anchorToCache(*collapsed);
+            inSync = found;
+        } catch (...) {
+            // TODO: Write an exception handler for the entire method.
+            debugs(20, 3, "anchorToCache() failed for " << *collapsed << ": " << CurrentException);
+            collapsed->abort();
+            return;
+        }
     }
 
     if (entryStatus.waitingToBeFreed && !found) {
@@ -855,49 +858,51 @@ Store::Controller::syncCollapsed(const sfileno xitIndex)
     collapsed->setCollapsingRequirement(true);
 }
 
-/// Called for Transients entries that are not yet anchored to a cache.
+/// If possible and has not been done, associates the entry with its store(s).
 /// \returns false for not-yet-cached entries that we may attach later
 /// \returns true for other entries after synchronizing them with their store
-/// and setting inSync to reflect that synchronization outcome.
 bool
-Store::Controller::anchorToCache(StoreEntry &entry, bool &inSync)
+Store::Controller::anchorToCache(StoreEntry &entry)
 {
     assert(entry.hasTransients());
     assert(transientsReader(entry));
 
+    // TODO: Attach entries to both memory and disk
+
+    // TODO: Reduce code duplication with syncCollapsed()
+    if (sharedMemStore && entry.mem().memCache.io == MemObject::ioDone) {
+        debugs(20, 5, "already handled by memory store: " << entry);
+        return true;
+    } else if (sharedMemStore && entry.hasMemStore()) {
+        debugs(20, 5, "already anchored to memory store: " << entry);
+        return true;
+    } else if (swapDir && entry.hasDisk()) {
+        debugs(20, 5, "already anchored to disk: " << entry);
+        return true;
+    }
+
     debugs(20, 7, "anchoring " << entry);
 
     Transients::EntryStatus entryStatus;
     transients->status(entry, entryStatus);
 
-    inSync = false;
     bool found = false;
     if (sharedMemStore)
-        found = sharedMemStore->anchorToCache(entry, inSync);
+        found = sharedMemStore->anchorToCache(entry);
     if (!found && swapDir)
-        found = swapDir->anchorToCache(entry, inSync);
+        found = swapDir->anchorToCache(entry);
 
-    if (inSync) {
+    if (found) {
         debugs(20, 7, "anchored " << entry);
-        assert(found);
         entry.setCollapsingRequirement(false);
         return true;
     }
 
-    if (found) {
-        debugs(20, 5, "failed to anchor " << entry);
-        return true;
-    }
+    if (entryStatus.waitingToBeFreed)
+        throw TextException("will never be able to anchor to an already marked entry", Here());
 
-    if (entryStatus.waitingToBeFreed) {
-        debugs(20, 5, "failed on marked unattached " << entry);
-        return true;
-    }
-
-    if (!entryStatus.hasWriter) {
-        debugs(20, 5, "failed on abandoned-by-writer " << entry);
-        return true;
-    }
+    if (!entryStatus.hasWriter)
+        throw TextException("will never be able to anchor to an abandoned-by-writer entry", Here());
 
     debugs(20, 7, "skipping not yet cached " << entry);
     entry.setCollapsingRequirement(true);

diff --git a/src/store/Controller.h b/src/store/Controller.h
index ab3471cc3f2bdf9391938bec624c06ee72008b81..02e6450521e406a68f90d16ab881f59e66216c84 100644 (file)
--- a/src/store/Controller.h
+++ b/src/store/Controller.h
@@ -144,7 +144,7 @@ private:
     void memoryEvictCached(StoreEntry &);
     void transientsUnlinkByKeyIfFound(const cache_key *);
     bool keepForLocalMemoryCache(StoreEntry &e) const;
-    bool anchorToCache(StoreEntry &e, bool &inSync);
+    bool anchorToCache(StoreEntry &);
     void checkTransients(const StoreEntry &) const;
     void checkFoundCandidate(const StoreEntry &) const;
 

diff --git a/src/store/Disks.cc b/src/store/Disks.cc
index cc65f1b4c135e6322219e8ad62906a69aa786862..3f0f20efaba3edb35dd5f70815abeb5fdbac6f75 100644 (file)
--- a/src/store/Disks.cc
+++ b/src/store/Disks.cc
@@ -609,8 +609,11 @@ Store::Disks::evictIfFound(const cache_key *key)
 }
 
 bool
-Store::Disks::anchorToCache(StoreEntry &entry, bool &inSync)
+Store::Disks::anchorToCache(StoreEntry &entry)
 {
+    if (entry.hasDisk())
+        return true; // already anchored
+
     if (const int cacheDirs = Config.cacheSwap.n_configured) {
         // ask each cache_dir until the entry is found; use static starting
         // point to avoid asking the same subset of disks more often
@@ -622,7 +625,7 @@ Store::Disks::anchorToCache(StoreEntry &entry, bool &inSync)
             if (!sd.active())
                 continue;
 
-            if (sd.anchorToCache(entry, inSync)) {
+            if (sd.anchorToCache(entry)) {
                 debugs(20, 3, "cache_dir " << idx << " anchors " << entry);
                 return true;
             }

diff --git a/src/store/Disks.h b/src/store/Disks.h
index 8384014091552bf27b1320f083adc59aaa747e5f..e6fd0af1ef68b5f97124a8a262ff86efe050e5fa 100644 (file)
--- a/src/store/Disks.h
+++ b/src/store/Disks.h
@@ -36,7 +36,7 @@ public:
     virtual bool dereference(StoreEntry &e) override;
     virtual void updateHeaders(StoreEntry *) override;
     virtual void maintain() override;
-    virtual bool anchorToCache(StoreEntry &e, bool &inSync) override;
+    virtual bool anchorToCache(StoreEntry &) override;
     virtual bool updateAnchored(StoreEntry &) override;
     virtual void evictCached(StoreEntry &) override;
     virtual void evictIfFound(const cache_key *) override;

diff --git a/src/tests/stub_MemStore.cc b/src/tests/stub_MemStore.cc
index f49a44d50cac391a4ffdd5a8dbed34a4d20b24e4..84535527b066b9404849eeabbb338cf780099646 100644 (file)
--- a/src/tests/stub_MemStore.cc
+++ b/src/tests/stub_MemStore.cc
@@ -36,7 +36,7 @@ int64_t MemStore::maxObjectSize() const STUB_RETVAL(0)
 bool MemStore::dereference(StoreEntry &) STUB_RETVAL(false)
 void MemStore::evictCached(StoreEntry&) STUB
 void MemStore::evictIfFound(const cache_key *) STUB
-bool MemStore::anchorToCache(StoreEntry&, bool&) STUB_RETVAL(false)
+bool MemStore::anchorToCache(StoreEntry&) STUB_RETVAL(false)
 bool MemStore::updateAnchored(StoreEntry&) STUB_RETVAL(false)
 int64_t MemStore::EntryLimit() STUB_RETVAL(0)
 

diff --git a/src/tests/stub_libstore.cc b/src/tests/stub_libstore.cc
index 7a49ee4abb96723883752ee39b707823c7f4a4aa..a2be27a90523ea2f64c15f15563706a92a0d39cc 100644 (file)
--- a/src/tests/stub_libstore.cc
+++ b/src/tests/stub_libstore.cc
@@ -115,7 +115,7 @@ void Disks::reference(StoreEntry &) STUB
 bool Disks::dereference(StoreEntry &) STUB_RETVAL(false)
 void Disks::updateHeaders(StoreEntry *) STUB
 void Disks::maintain() STUB
-bool Disks::anchorToCache(StoreEntry &, bool &) STUB_RETVAL(false)
+bool Disks::anchorToCache(StoreEntry &) STUB_RETVAL(false)
 bool Disks::updateAnchored(StoreEntry &) STUB_RETVAL(false)
 void Disks::evictCached(StoreEntry &) STUB
 void Disks::evictIfFound(const cache_key *) STUB