From: Alex Rousskov Date: Wed, 17 Oct 2012 01:02:52 +0000 (-0600) Subject: Allow a ufs cache_dir entry to coexist with a shared memory cache entry X-Git-Tag: SQUID_3_2_3~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e64bbf1ba768a748c62c128f6f769e3e78f91420;p=thirdparty%2Fsquid.git 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 eb5d155bec..afd86c6a3a 100644 --- 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 b4a03ac8e1..39db40e4c0 100644 --- 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 968d1337d1..433ef1387f 100644 --- 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 936219dc92..39b5920477 100644 --- 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 0e20ae5964..a41e474150 100644 --- 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 e1d7030adc..0a445f2b53 100644 --- 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 d624ec7b01..78417bfcb9 100644 --- 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 f427c57fff..0c6eacb002 100644 --- 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 ef183af471..77bcf7ea38 100644 --- 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 c4af21183a..897b7c88b5 100644 --- 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 0ddde25967..cbbf962cc6 100644 --- 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 ace530e68b..c8462e6d07 100644 --- 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(&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 08a1b7c16e..cc822a72c0 100644 --- 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 39309c9c36..30c0f6e220 100644 --- 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 *); };