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 eb5d155beca1430c0d9087e9beb9e0291e42b104..afd86c6a3abe331de474574ca119e4f2a4d09d3e 100644 (file)
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -129,7 +129,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 b4a03ac8e171891c8e3babfd1c880b8b5e9d19e4..39db40e4c0a165ea65697867ae8581dacffbcd9f 100644 (file)
--- a/src/MemStore.h
+++ b/src/MemStore.h
@@ -44,7 +44,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 968d1337d1d9a146f84f9d9ede17fb1e9632c178..433ef1387f3a039f987928075f466f4b71da791b 100644 (file)
--- a/src/Store.h
+++ b/src/Store.h
@@ -342,7 +342,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 936219dc92c4030c87f98334fec2b2ba67948264..39b5920477dd07e15164f7245d22aab1bee508f0 100644 (file)
--- a/src/StoreHashIndex.h
+++ b/src/StoreHashIndex.h
@@ -78,7 +78,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 0e20ae5964fb5bfcf9d78bbdab7b5b844d94af0e..a41e474150955f1cdda8f7449cf3914ab8782321 100644 (file)
--- a/src/SwapDir.cc
+++ b/src/SwapDir.cc
@@ -115,7 +115,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 e1d7030adca440b18d75254b2fc73b2b4f0b3db8..0a445f2b5333e6be1b8badea7001fc3f375a952d 100644 (file)
--- a/src/SwapDir.h
+++ b/src/SwapDir.h
@@ -85,7 +85,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;
@@ -207,7 +207,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 d624ec7b01bd6edd563420d24199e497e393fa1b..78417bfcb9046d1185aa3a3dc98cb0b5e9f9ff61 100644 (file)
--- a/src/fs/rock/RockSwapDir.cc
+++ b/src/fs/rock/RockSwapDir.cc
@@ -739,7 +739,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 f427c57fff42e8a4765a2c3c9f6ee9fab57d86c7..0c6eacb002b72421181af7b9b3c7037da391c0e4 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/store_dir_ufs.cc b/src/fs/ufs/store_dir_ufs.cc
index ef183af47105350d9ba5c1add6947f1454f4ad74..77bcf7ea3890248472a662386ad9b3a15a7d1f71 100644 (file)
--- a/src/fs/ufs/store_dir_ufs.cc
+++ b/src/fs/ufs/store_dir_ufs.cc
@@ -427,7 +427,7 @@ UFSSwapDir::reference(StoreEntry &e)
  * removed, to maintain replacement information within the storage fs.
  */
 bool
-UFSSwapDir::dereference(StoreEntry & e)
+UFSSwapDir::dereference(StoreEntry & e, bool)
 {
     debugs(47, 3, "UFSSwapDir::dereference: referencing " << &e << " " << e.swap_dirn << "/" << e.swap_filen);
 

diff --git a/src/fs/ufs/ufscommon.cc b/src/fs/ufs/ufscommon.cc
index c4af21183affba4d1c43c03285d6f8aa56f90deb..897b7c88b50b29c0d368b683cc7d27ea2f374d6e 100644 (file)
--- a/src/fs/ufs/ufscommon.cc
+++ b/src/fs/ufs/ufscommon.cc
@@ -628,7 +628,7 @@ 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, 1, "\tSee " << __FILE__ << ":" << __LINE__);

diff --git a/src/fs/ufs/ufscommon.h b/src/fs/ufs/ufscommon.h
index 0ddde259673183ed504a18203848f3ab35bc3df2..cbbf962cc6f5975bfe19c4efeb3f04686951805e 100644 (file)
--- a/src/fs/ufs/ufscommon.h
+++ b/src/fs/ufs/ufscommon.h
@@ -66,7 +66,7 @@ public:
     virtual void maintain();
     virtual bool canStore(const StoreEntry &e, int64_t diskSpaceNeeded, int &load) const;
     virtual void reference(StoreEntry &);
-    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 ace530e68b89689f7cabb2672609b62657e934d1..c8462e6d07801221c7f0e25e1a05d137a0b97113 100644 (file)
--- a/src/store_dir.cc
+++ b/src/store_dir.cc
@@ -706,27 +706,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;
@@ -834,9 +837,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;
@@ -1071,9 +1074,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 08a1b7c16eee49d75964e4aea4ea6f7cc79029f7..cc822a72c0f0de443e971b750ebdc77ec55ea5f3 100644 (file)
--- a/src/tests/stub_MemStore.cc
+++ b/src/tests/stub_MemStore.cc
@@ -30,4 +30,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 39309c9c36d74519ddf3f29bdc67b2f8b726b3a1..30c0f6e22084f22082d9086223c8e9aca7fc3065 100644 (file)
--- a/src/tests/testStore.h
+++ b/src/tests/testStore.h
@@ -71,7 +71,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 *);
 };