Allow a ufs cache_dir entry to coexist with a shared memory cache entry

instead of being released when it becomes idle.

The original boolean version of the StoreController::dereference() code
(r11730) was written to make sure that idle unlocked local store_table entries
are released if nobody needs them (to avoid creating inconsistencies with
shared caches that could be modified in a different process).

Then, in r11786, we realized that the original code was destroying non-shared
memory cache entries if there were no cache_dirs to vote for keeping them in
store_table. I fixed that by changing the StoreController::dereference() logic
from "remove if nobody needs it" to "remove if somebody objects to keeping
it". That solved the problem at hand, but prohibited an entry to exist in
a non-shared cache_dir and in a shared memory cache at the same time.

We now go back to the original "remove if nobody needs it" design but also
give non-shared memory cache a vote so that it can protect idle but suitable
for memory cache entries from being released if there are no cache_dirs to
vote for them.

This is a second revision of the fix. The first one (r12231) was reverted
because it did not pass tests/testRock unit tests on some platforms. The unit
tests assume that the entry slot is not locked after the entry is stored, but
the first revision of the fix allowed idle entries to remain in store_table
and, hence, their slots were locked and could not be replaced, causing
assertions.  This revision allows the idle entry to be destroyed (and its slot
unlocked) if [non-shared] memory caching is disabled.

It is not clear why only some of the platforms were affected by this. Should
not memory caching be disabled everywhere during testRock (because testRock
does not set memory cache capacity and memory cache entry size limits)?

diff --git a/src/MemStore.cc b/src/MemStore.cc
index c6fb09b947ffb88c1ac2bae7dadc8b63d4469a79..be88e20ed120eec5b04409a91fb36de9df0d5548 100644 (file)
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -130,7 +130,7 @@ MemStore::reference(StoreEntry &)
 }
 
 bool
-MemStore::dereference(StoreEntry &)
+MemStore::dereference(StoreEntry &, bool)
 {
     // no need to keep e in the global store_table for us; we have our own map
     return false;

diff --git a/src/MemStore.h b/src/MemStore.h
index 4aea18264125acf180cd27863c93b27fa389b965..aca5c6d5b2c8da2f2aa5b1557b2f169a749ed3c9 100644 (file)
--- a/src/MemStore.h
+++ b/src/MemStore.h
@@ -40,7 +40,7 @@ public:
     virtual void stat(StoreEntry &) const;
     virtual StoreSearch *search(String const url, HttpRequest *);
     virtual void reference(StoreEntry &);
-    virtual bool dereference(StoreEntry &);
+    virtual bool dereference(StoreEntry &, bool);
     virtual void maintain();
 
     static int64_t EntryLimit();

diff --git a/src/Store.h b/src/Store.h
index 648b730094fe7f6135f95b13f31781e5ffacca9a..32b216aa557af52e64689d299b288dfc3bab3096 100644 (file)
--- a/src/Store.h
+++ b/src/Store.h
@@ -341,7 +341,7 @@ public:
     virtual void reference(StoreEntry &) = 0;  /* Reference this object */
 
     /// Undo reference(), returning false iff idle e should be destroyed
-    virtual bool dereference(StoreEntry &e) = 0;
+    virtual bool dereference(StoreEntry &e, bool wantsLocalMemory) = 0;
 
     virtual void maintain() = 0; /* perform regular maintenance should be private and self registered ... */
 

diff --git a/src/StoreHashIndex.h b/src/StoreHashIndex.h
index c68c788c44e93add43bf7864c19c974bbade6c75..5a33860f484ee340a0fce308cdac53023ff59f28 100644 (file)
--- a/src/StoreHashIndex.h
+++ b/src/StoreHashIndex.h
@@ -76,7 +76,7 @@ public:
 
     virtual void reference(StoreEntry&);
 
-    virtual bool dereference(StoreEntry&);
+    virtual bool dereference(StoreEntry&, bool);
 
     virtual void maintain();
 

diff --git a/src/SwapDir.cc b/src/SwapDir.cc
index 7061dd4f603cc1428b1f2430daee46c31748612c..94e7de2d2347719fac27493d7668de165afa0bd6 100644 (file)
--- a/src/SwapDir.cc
+++ b/src/SwapDir.cc
@@ -118,7 +118,7 @@ void
 SwapDir::reference(StoreEntry &) {}
 
 bool
-SwapDir::dereference(StoreEntry &)
+SwapDir::dereference(StoreEntry &, bool)
 {
     return true; // keep in global store_table
 }

diff --git a/src/SwapDir.h b/src/SwapDir.h
index 4178fb2206c39179d2dbb62778ecf953b4519a9c..488cafca918279a9b82177561874b9fadba12e31 100644 (file)
--- a/src/SwapDir.h
+++ b/src/SwapDir.h
@@ -84,7 +84,7 @@ public:
 
     virtual void reference(StoreEntry &);      /* Reference this object */
 
-    virtual bool dereference(StoreEntry &);    /* Unreference this object */
+    virtual bool dereference(StoreEntry &, bool);      /* Unreference this object */
 
     /* the number of store dirs being rebuilt. */
     static int store_dirs_rebuilding;
@@ -206,7 +206,7 @@ public:
     virtual bool canStore(const StoreEntry &e, int64_t diskSpaceNeeded, int &load) const = 0;
     /* These two are notifications */
     virtual void reference(StoreEntry &);      /* Reference this object */
-    virtual bool dereference(StoreEntry &);    /* Unreference this object */
+    virtual bool dereference(StoreEntry &, bool);      /* Unreference this object */
     virtual int callback();    /* Handle pending callbacks */
     virtual void sync();       /* Sync the store prior to shutdown */
     virtual StoreIOState::Pointer createStoreIO(StoreEntry &, StoreIOState::STFNCB *, StoreIOState::STIOCB *, void *) = 0;

diff --git a/src/fs/rock/RockSwapDir.cc b/src/fs/rock/RockSwapDir.cc
index 50bb85105a3d82fcab2405dc543ab3ced7dcbebb..e8c23006f3df44cee78d1c5926bbf225b6fb7309 100644 (file)
--- a/src/fs/rock/RockSwapDir.cc
+++ b/src/fs/rock/RockSwapDir.cc
@@ -746,7 +746,7 @@ Rock::SwapDir::reference(StoreEntry &e)
 }
 
 bool
-Rock::SwapDir::dereference(StoreEntry &e)
+Rock::SwapDir::dereference(StoreEntry &e, bool)
 {
     debugs(47, 5, HERE << &e << ' ' << e.swap_dirn << ' ' << e.swap_filen);
     if (repl && repl->Dereferenced)

diff --git a/src/fs/rock/RockSwapDir.h b/src/fs/rock/RockSwapDir.h
index 793b5bea6b86ba3cce20cadaaacfd9c3e0177a55..165fa2cfc14f03745ca265249356464eec04b4f1 100644 (file)
--- a/src/fs/rock/RockSwapDir.h
+++ b/src/fs/rock/RockSwapDir.h
@@ -53,7 +53,7 @@ protected:
     virtual void maintain();
     virtual void diskFull();
     virtual void reference(StoreEntry &e);
-    virtual bool dereference(StoreEntry &e);
+    virtual bool dereference(StoreEntry &e, bool);
     virtual bool unlinkdUseful() const;
     virtual void unlink(StoreEntry &e);
     virtual void statfs(StoreEntry &e) const;

diff --git a/src/fs/ufs/RebuildState.cc b/src/fs/ufs/RebuildState.cc
index 141143ed28aa0fb957ce95627c5285c4b1cbd8c8..eeb3d086e4abe7c4818ce32a2d0ca6f5b5a39046 100644 (file)
--- a/src/fs/ufs/RebuildState.cc
+++ b/src/fs/ufs/RebuildState.cc
@@ -346,7 +346,7 @@ Fs::Ufs::RebuildState::rebuildFromSwapLog()
             currentEntry()->lastmod = swapData.lastmod;
             currentEntry()->flags = swapData.flags;
             currentEntry()->refcount += swapData.refcount;
-            sd->dereference(*currentEntry());
+            sd->dereference(*currentEntry(), false);
         } else {
             debug_trap("commonUfsDirRebuildFromSwapLog: bad condition");
             debugs(47, DBG_IMPORTANT, HERE << "bad condition");

diff --git a/src/fs/ufs/UFSSwapDir.cc b/src/fs/ufs/UFSSwapDir.cc
index 4ae01d192fd3f081e68e25525eaf46b5880f4c61..f2a70626f89fd1722228329020bc2a845090b556 100644 (file)
--- a/src/fs/ufs/UFSSwapDir.cc
+++ b/src/fs/ufs/UFSSwapDir.cc
@@ -491,7 +491,7 @@ Fs::Ufs::UFSSwapDir::reference(StoreEntry &e)
 }
 
 bool
-Fs::Ufs::UFSSwapDir::dereference(StoreEntry & e)
+Fs::Ufs::UFSSwapDir::dereference(StoreEntry & e, bool)
 {
     debugs(47, 3, HERE << "dereferencing " << &e << " " <<
            e.swap_dirn << "/" << e.swap_filen);

diff --git a/src/fs/ufs/UFSSwapDir.h b/src/fs/ufs/UFSSwapDir.h
index 34bdeb0ee384cedeaeeac387095da0e51d327baf..3132e37bc53d32c6e75d00452a1a16ef76d1827c 100644 (file)
--- a/src/fs/ufs/UFSSwapDir.h
+++ b/src/fs/ufs/UFSSwapDir.h
@@ -95,7 +95,7 @@ public:
      * This routine is called whenever the last reference to an object is
      * removed, to maintain replacement information within the storage fs.
      */
-    virtual bool dereference(StoreEntry &);
+    virtual bool dereference(StoreEntry &, bool);
     virtual StoreIOState::Pointer createStoreIO(StoreEntry &, StoreIOState::STFNCB *, StoreIOState::STIOCB *, void *);
     virtual StoreIOState::Pointer openStoreIO(StoreEntry &, StoreIOState::STFNCB *, StoreIOState::STIOCB *, void *);
     virtual void openLog();

diff --git a/src/store_dir.cc b/src/store_dir.cc
index fb82c2170274eb9a0d3e3008cc837d778f5b6a5c..002da183f9447368f23bcf9f4eea8fccc1f897fc 100644 (file)
--- a/src/store_dir.cc
+++ b/src/store_dir.cc
@@ -712,27 +712,30 @@ StoreController::reference(StoreEntry &e)
 }
 
 bool
-StoreController::dereference(StoreEntry & e)
+StoreController::dereference(StoreEntry &e, bool wantsLocalMemory)
 {
-    bool keepInStoreTable = true; // keep if there are no objections
-
     // special entries do not belong to any specific Store, but are IN_MEMORY
     if (EBIT_TEST(e.flags, ENTRY_SPECIAL))
-        return keepInStoreTable;
+        return true;
+
+    bool keepInStoreTable = false; // keep only if somebody needs it there
 
     /* Notify the fs that we're not referencing this object any more */
 
     if (e.swap_filen > -1)
-        keepInStoreTable = swapDir->dereference(e) && keepInStoreTable;
+        keepInStoreTable = swapDir->dereference(e, wantsLocalMemory) || keepInStoreTable;
 
     // Notify the memory cache that we're not referencing this object any more
     if (memStore && e.mem_status == IN_MEMORY)
-        keepInStoreTable = memStore->dereference(e) && keepInStoreTable;
+        keepInStoreTable = memStore->dereference(e, wantsLocalMemory) || keepInStoreTable;
 
     // TODO: move this code to a non-shared memory cache class when we have it
     if (e.mem_obj) {
         if (mem_policy->Dereferenced)
             mem_policy->Dereferenced(mem_policy, &e, &e.mem_obj->repl);
+        // non-shared memory cache relies on store_table
+        if (!memStore)
+            keepInStoreTable = wantsLocalMemory || keepInStoreTable;
     }
 
     return keepInStoreTable;
@@ -840,9 +843,9 @@ StoreController::handleIdleEntry(StoreEntry &e)
                             (mem_node::InUseCount() <= store_pages_max);
     }
 
-    // An idle, unlocked entry that belongs to a SwapDir which controls
+    // An idle, unlocked entry that only belongs to a SwapDir which controls
     // its own index, should not stay in the global store_table.
-    if (!dereference(e)) {
+    if (!dereference(e, keepInLocalMemory)) {
         debugs(20, 5, HERE << "destroying unlocked entry: " << &e << ' ' << e);
         destroyStoreEntry(static_cast<hash_link*>(&e));
         return;
@@ -1077,9 +1080,9 @@ StoreHashIndex::reference(StoreEntry &e)
 }
 
 bool
-StoreHashIndex::dereference(StoreEntry &e)
+StoreHashIndex::dereference(StoreEntry &e, bool wantsLocalMemory)
 {
-    return e.store()->dereference(e);
+    return e.store()->dereference(e, wantsLocalMemory);
 }
 
 void

diff --git a/src/tests/stub_MemStore.cc b/src/tests/stub_MemStore.cc
index cb8cb33361657c89c4e27b3bfd07f437881da72b..6f586596bf1265e524b1d9ae76f0b89de3e76388 100644 (file)
--- a/src/tests/stub_MemStore.cc
+++ b/src/tests/stub_MemStore.cc
@@ -28,4 +28,4 @@ uint64_t MemStore::currentSize() const STUB_RETVAL(0)
 uint64_t MemStore::currentCount() const STUB_RETVAL(0)
 int64_t MemStore::maxObjectSize() const STUB_RETVAL(0)
 StoreSearch *MemStore::search(String const, HttpRequest *) STUB_RETVAL(NULL)
-bool MemStore::dereference(StoreEntry &) STUB_RETVAL(false)
+bool MemStore::dereference(StoreEntry &, bool) STUB_RETVAL(false)

diff --git a/src/tests/testStore.h b/src/tests/testStore.h
index 064617eca253b8763efc59b627ab2d4335e94f54..055a92026fbee29f7ff63e6f50fd748cfc718cd0 100644 (file)
--- a/src/tests/testStore.h
+++ b/src/tests/testStore.h
@@ -66,7 +66,7 @@ public:
 
     virtual void reference(StoreEntry &) {}    /* Reference this object */
 
-    virtual bool dereference(StoreEntry &) { return true; }
+    virtual bool dereference(StoreEntry &, bool) { return true; }
 
     virtual StoreSearch *search(String const url, HttpRequest *);
 };