------------------------------------------------------------ revno: 12365 revision-id: squid3@treenet.co.nz-20121016234354-zo8ojneglp4t3bj0 parent: squid3@treenet.co.nz-20121016234132-e3wjkgbfhdscydei author: Alex Rousskov committer: Amos Jeffries branch nick: 3.3 timestamp: Tue 2012-10-16 17:43:54 -0600 message: 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)? ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20121016234354-zo8ojneglp4t3bj0 # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.3 # testament_sha1: b433d715192e04cde3ad3e274b6fcb8063f0ce5d # timestamp: 2012-10-16 23:44:15 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.3 # base_revision_id: squid3@treenet.co.nz-20121016234132-\ # e3wjkgbfhdscydei # # Begin patch === modified file 'src/MemStore.cc' --- src/MemStore.cc 2012-09-04 09:10:20 +0000 +++ src/MemStore.cc 2012-10-16 23:43:54 +0000 @@ -130,7 +130,7 @@ } 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; === modified file 'src/MemStore.h' --- src/MemStore.h 2012-10-04 11:10:17 +0000 +++ src/MemStore.h 2012-10-16 23:43:54 +0000 @@ -40,7 +40,7 @@ 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(); === modified file 'src/Store.h' --- src/Store.h 2012-09-22 20:07:31 +0000 +++ src/Store.h 2012-10-16 23:43:54 +0000 @@ -341,7 +341,7 @@ 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 ... */ === modified file 'src/StoreHashIndex.h' --- src/StoreHashIndex.h 2012-09-01 14:38:36 +0000 +++ src/StoreHashIndex.h 2012-10-16 23:43:54 +0000 @@ -76,7 +76,7 @@ virtual void reference(StoreEntry&); - virtual bool dereference(StoreEntry&); + virtual bool dereference(StoreEntry&, bool); virtual void maintain(); === modified file 'src/SwapDir.cc' --- src/SwapDir.cc 2012-09-04 09:10:20 +0000 +++ src/SwapDir.cc 2012-10-16 23:43:54 +0000 @@ -118,7 +118,7 @@ SwapDir::reference(StoreEntry &) {} bool -SwapDir::dereference(StoreEntry &) +SwapDir::dereference(StoreEntry &, bool) { return true; // keep in global store_table } === modified file 'src/SwapDir.h' --- src/SwapDir.h 2012-09-21 14:57:30 +0000 +++ src/SwapDir.h 2012-10-16 23:43:54 +0000 @@ -84,7 +84,7 @@ 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 @@ 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; === modified file 'src/fs/rock/RockSwapDir.cc' --- src/fs/rock/RockSwapDir.cc 2012-09-04 09:10:20 +0000 +++ src/fs/rock/RockSwapDir.cc 2012-10-16 23:43:54 +0000 @@ -746,7 +746,7 @@ } 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) === modified file 'src/fs/rock/RockSwapDir.h' --- src/fs/rock/RockSwapDir.h 2012-08-28 13:00:30 +0000 +++ src/fs/rock/RockSwapDir.h 2012-10-16 23:43:54 +0000 @@ -53,7 +53,7 @@ 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; === modified file 'src/fs/ufs/RebuildState.cc' --- src/fs/ufs/RebuildState.cc 2012-09-04 09:10:20 +0000 +++ src/fs/ufs/RebuildState.cc 2012-10-16 23:43:54 +0000 @@ -346,7 +346,7 @@ 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"); === modified file 'src/fs/ufs/UFSSwapDir.cc' --- src/fs/ufs/UFSSwapDir.cc 2012-09-04 09:10:20 +0000 +++ src/fs/ufs/UFSSwapDir.cc 2012-10-16 23:43:54 +0000 @@ -491,7 +491,7 @@ } 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); === modified file 'src/fs/ufs/UFSSwapDir.h' --- src/fs/ufs/UFSSwapDir.h 2012-08-10 06:56:49 +0000 +++ src/fs/ufs/UFSSwapDir.h 2012-10-16 23:43:54 +0000 @@ -95,7 +95,7 @@ * 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(); === modified file 'src/store_dir.cc' --- src/store_dir.cc 2012-09-04 09:10:20 +0000 +++ src/store_dir.cc 2012-10-16 23:43:54 +0000 @@ -712,27 +712,30 @@ } 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 @@ (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; @@ -1077,9 +1080,9 @@ } bool -StoreHashIndex::dereference(StoreEntry &e) +StoreHashIndex::dereference(StoreEntry &e, bool wantsLocalMemory) { - return e.store()->dereference(e); + return e.store()->dereference(e, wantsLocalMemory); } void === modified file 'src/tests/stub_MemStore.cc' --- src/tests/stub_MemStore.cc 2012-09-01 14:38:36 +0000 +++ src/tests/stub_MemStore.cc 2012-10-16 23:43:54 +0000 @@ -28,4 +28,4 @@ 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) === modified file 'src/tests/testStore.h' --- src/tests/testStore.h 2012-08-28 13:00:30 +0000 +++ src/tests/testStore.h 2012-10-16 23:43:54 +0000 @@ -66,7 +66,7 @@ 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 *); };