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)? === modified file 'src/MemStore.cc' --- src/MemStore.cc 2012-09-04 09:10:20 +0000 +++ src/MemStore.cc 2012-10-10 19:04:30 +0000 @@ -113,41 +113,41 @@ } uint64_t MemStore::currentCount() const { return map ? map->entryCount() : 0; } int64_t MemStore::maxObjectSize() const { return Ipc::Mem::PageSize(); } void 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; } int MemStore::callback() { return 0; } StoreSearch * MemStore::search(String const, HttpRequest *) { fatal("not implemented"); return NULL; } StoreEntry * MemStore::get(const cache_key *key) === modified file 'src/MemStore.h' --- src/MemStore.h 2012-10-04 11:10:17 +0000 +++ src/MemStore.h 2012-10-10 19:04:22 +0000 @@ -23,41 +23,41 @@ /// cache the entry or forget about it until the next considerKeeping call void considerKeeping(StoreEntry &e); /// whether e should be kept in local RAM for possible future caching bool keepInLocalMemory(const StoreEntry &e) const; /* Store API */ virtual int callback(); virtual StoreEntry * get(const cache_key *); virtual void get(String const key , STOREGETCLIENT callback, void *cbdata); virtual void init(); virtual uint64_t maxSize() const; virtual uint64_t minSize() const; virtual uint64_t currentSize() const; virtual uint64_t currentCount() const; virtual int64_t maxObjectSize() const; virtual void getStats(StoreInfoStats &stats) const; 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(); protected: bool willFit(int64_t needed) const; void keep(StoreEntry &e); bool copyToShm(StoreEntry &e, MemStoreMap::Extras &extras); bool copyFromShm(StoreEntry &e, const MemStoreMap::Extras &extras); // Ipc::StoreMapCleaner API virtual void cleanReadable(const sfileno fileno); private: MemStoreMap *map; ///< index of mem-cached entries uint64_t theCurrentSize; ///< currently used space in the storage area }; // Why use Store as a base? MemStore and SwapDir are both "caches". === modified file 'src/Store.h' --- src/Store.h 2012-09-22 20:07:31 +0000 +++ src/Store.h 2012-10-10 18:35:48 +0000 @@ -324,41 +324,41 @@ /** * Output stats to the provided store entry. \todo make these calls asynchronous */ virtual void stat(StoreEntry &) const = 0; /** Sync the store prior to shutdown */ virtual void sync(); /** remove a Store entry from the store */ virtual void unlink (StoreEntry &); /* search in the store */ virtual StoreSearch *search(String const url, HttpRequest *) = 0; /* pulled up from SwapDir for migration.... probably do not belong here */ 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 ... */ // XXX: This method belongs to Store::Root/StoreController, but it is here // because test cases use non-StoreController derivatives as Root /// called when the entry is no longer needed by any transaction virtual void handleIdleEntry(StoreEntry &e) {} // XXX: This method belongs to Store::Root/StoreController, but it is here // because test cases use non-StoreController derivatives as Root /// called to get rid of no longer needed entry data in RAM, if any virtual void maybeTrimMemory(StoreEntry &e, const bool preserveSwappable) {} private: static RefCount CurrentRoot; }; /// \ingroup StoreAPI typedef RefCount StorePointer; === modified file 'src/StoreHashIndex.h' --- src/StoreHashIndex.h 2012-09-01 14:38:36 +0000 +++ src/StoreHashIndex.h 2012-10-10 18:35:48 +0000 @@ -59,41 +59,41 @@ virtual void init(); virtual void sync(); virtual uint64_t maxSize() const; virtual uint64_t minSize() const; virtual uint64_t currentSize() const; virtual uint64_t currentCount() const; virtual int64_t maxObjectSize() const; virtual void getStats(StoreInfoStats &stats) const; virtual void stat(StoreEntry&) const; virtual void reference(StoreEntry&); - virtual bool dereference(StoreEntry&); + virtual bool dereference(StoreEntry&, bool); virtual void maintain(); virtual StoreSearch *search(String const url, HttpRequest *); private: /* migration logic */ StorePointer store(int const x) const; SwapDir &dir(int const idx) const; }; class StoreHashIndexEntry : public StoreEntry {}; class StoreSearchHashIndex : public StoreSearch { public: StoreSearchHashIndex(RefCount sd); StoreSearchHashIndex(StoreSearchHashIndex const &); === modified file 'src/SwapDir.cc' --- src/SwapDir.cc 2012-09-04 09:10:20 +0000 +++ src/SwapDir.cc 2012-10-10 18:36:29 +0000 @@ -101,41 +101,41 @@ repl->Stats(repl, &output); } } void SwapDir::statfs(StoreEntry &)const {} void SwapDir::maintain() {} uint64_t SwapDir::minSize() const { return ((maxSize() * Config.Swap.lowWaterMark) / 100); } void SwapDir::reference(StoreEntry &) {} bool -SwapDir::dereference(StoreEntry &) +SwapDir::dereference(StoreEntry &, bool) { return true; // keep in global store_table } int SwapDir::callback() { return 0; } bool SwapDir::canStore(const StoreEntry &e, int64_t diskSpaceNeeded, int &load) const { debugs(47,8, HERE << "cache_dir[" << index << "]: needs " << diskSpaceNeeded << " scanned << '/' << walker->locked); walker->Done(walker); if (full()) { debugs(47, DBG_CRITICAL, "ERROR: Rock cache_dir[" << index << "] " << "is still full after freeing " << freed << " entries. A bug?"); } } void Rock::SwapDir::reference(StoreEntry &e) { debugs(47, 5, HERE << &e << ' ' << e.swap_dirn << ' ' << e.swap_filen); if (repl && repl->Referenced) repl->Referenced(repl, &e, &e.repl); } 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) repl->Dereferenced(repl, &e, &e.repl); // no need to keep e in the global store_table for us; we have our own map return false; } bool Rock::SwapDir::unlinkdUseful() const { // no entry-specific files to unlink return false; } void Rock::SwapDir::unlink(StoreEntry &e) { debugs(47, 5, HERE << e); === 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-10 18:38:25 +0000 @@ -36,41 +36,41 @@ virtual void create(); virtual void parse(int index, char *path); int64_t entryLimitHigh() const { return SwapFilenMax; } ///< Core limit int64_t entryLimitAllowed() const; typedef Ipc::StoreMapWithExtras DirMap; protected: /* protected ::SwapDir API */ virtual bool needsDiskStrand() const; virtual void init(); virtual ConfigOption *getOptionTree() const; virtual bool allowOptionReconfigure(const char *const option) const; virtual bool canStore(const StoreEntry &e, int64_t diskSpaceNeeded, int &load) const; virtual StoreIOState::Pointer createStoreIO(StoreEntry &, StoreIOState::STFNCB *, StoreIOState::STIOCB *, void *); virtual StoreIOState::Pointer openStoreIO(StoreEntry &, StoreIOState::STFNCB *, StoreIOState::STIOCB *, void *); 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; /* IORequestor API */ virtual void ioCompletedNotification(); virtual void closeCompleted(); virtual void readCompleted(const char *buf, int len, int errflag, RefCount< ::ReadRequest>); virtual void writeCompleted(int errflag, size_t len, RefCount< ::WriteRequest>); void parseSize(const bool reconfiguring); ///< parses anonymous cache_dir size option void validateOptions(); ///< warns of configuration problems; may quit bool parseTimeOption(char const *option, const char *value, int reconfiguring); void dumpTimeOption(StoreEntry * e) const; bool parseRateOption(char const *option, const char *value, int reconfiguring); void dumpRateOption(StoreEntry * e) const; void rebuild(); ///< starts loading and validating stored entry metadata ///< used to add entries successfully loaded during rebuild bool addEntry(const int fileno, const DbCellHeader &header, const StoreEntry &from); === 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-10 18:38:16 +0000 @@ -329,41 +329,41 @@ /* If this URL already exists in the cache, does the swap log * appear to have a newer entry? Compare 'lastref' from the * swap log to e->lastref. */ /* is the log entry newer than current entry? */ int disk_entry_newer = currentEntry() ? (swapData.lastref > currentEntry()->lastref ? 1 : 0) : 0; if (used && !disk_entry_newer) { /* log entry is old, ignore it */ ++counts.clashcount; return; } else if (used && currentEntry() && currentEntry()->swap_filen == swapData.swap_filen && currentEntry()->swap_dirn == sd->index) { /* swapfile taken, same URL, newer, update meta */ if (currentEntry()->store_status == STORE_OK) { currentEntry()->lastref = swapData.timestamp; currentEntry()->timestamp = swapData.timestamp; currentEntry()->expires = swapData.expires; 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"); } return; } else if (used) { /* swapfile in use, not by this URL, log entry is newer */ /* This is sorta bad: the log entry should NOT be newer at this * point. If the log is dirty, the filesize check should have * caught this. If the log is clean, there should never be a * newer entry. */ debugs(47, DBG_IMPORTANT, "WARNING: newer swaplog entry for dirno " << sd->index << ", fileno "<< std::setfill('0') << std::hex << std::uppercase << std::setw(8) << swapData.swap_filen); /* I'm tempted to remove the swapfile here just to be safe, * but there is a bad race condition in the NOVM version if * the swapfile has recently been opened for writing, but * not yet opened for reading. Because we can't map * swapfiles back to StoreEntrys, we don't know the state === modified file 'src/fs/ufs/UFSSwapDir.cc' --- src/fs/ufs/UFSSwapDir.cc 2012-10-08 05:35:27 +0000 +++ src/fs/ufs/UFSSwapDir.cc 2012-10-10 18:37:14 +0000 @@ -474,41 +474,41 @@ e->release(); } walker->Done(walker); debugs(47, (removed ? 2 : 3), HERE << path << " removed " << removed << "/" << max_remove << " f=" << std::setprecision(4) << f << " max_scan=" << max_scan); } void Fs::Ufs::UFSSwapDir::reference(StoreEntry &e) { debugs(47, 3, HERE << "referencing " << &e << " " << e.swap_dirn << "/" << e.swap_filen); if (repl->Referenced) repl->Referenced(repl, &e, &e.repl); } 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); if (repl->Dereferenced) repl->Dereferenced(repl, &e, &e.repl); return true; // keep e in the global store_table } StoreIOState::Pointer Fs::Ufs::UFSSwapDir::createStoreIO(StoreEntry &e, StoreIOState::STFNCB * file_callback, StoreIOState::STIOCB * aCallback, void *callback_data) { return IO->create (this, &e, file_callback, aCallback, callback_data); } StoreIOState::Pointer Fs::Ufs::UFSSwapDir::openStoreIO(StoreEntry &e, StoreIOState::STFNCB * file_callback, StoreIOState::STIOCB * aCallback, void *callback_data) { return IO->open (this, &e, file_callback, aCallback, callback_data); === 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-10 18:38:16 +0000 @@ -78,41 +78,41 @@ virtual void unlink(StoreEntry &); virtual void statfs(StoreEntry &)const; virtual void maintain(); /** check whether this filesystem can store the given object * * UFS filesystems will happily store anything as long as * the LRU time isn't too small */ virtual bool canStore(const StoreEntry &e, int64_t diskSpaceNeeded, int &load) const; /** reference an object * * This routine is called whenever an object is referenced, so we can * maintain replacement information within the storage fs. */ virtual void reference(StoreEntry &); /** de-reference an object * * 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(); virtual void closeLog(); virtual int writeCleanStart(); virtual void writeCleanDone(); virtual void logEntry(const StoreEntry & e, int op) const; virtual void parse(int index, char *path); ///parse configuration and setup new SwapDir virtual void reconfigure(); ///reconfigure the SwapDir virtual int callback(); virtual void sync(); virtual void swappedOut(const StoreEntry &e); virtual uint64_t currentSize() const { return cur_size; } virtual uint64_t currentCount() const { return n_disk_objects; } void unlinkFile(sfileno f); // move down when unlink is a virtual method //protected: Fs::Ufs::UFSStrategy *IO; char *fullPath(sfileno, char *) const; === modified file 'src/store_dir.cc' --- src/store_dir.cc 2012-10-08 05:21:11 +0000 +++ src/store_dir.cc 2012-10-10 19:05:23 +0000 @@ -695,61 +695,64 @@ if (EBIT_TEST(e.flags, ENTRY_SPECIAL)) return; /* Notify the fs that we're referencing this object again */ if (e.swap_dirn > -1) swapDir->reference(e); // Notify the memory cache that we're referencing this object again if (memStore && e.mem_status == IN_MEMORY) memStore->reference(e); // TODO: move this code to a non-shared memory cache class when we have it if (e.mem_obj) { if (mem_policy->Referenced) mem_policy->Referenced(mem_policy, &e, &e.mem_obj->repl); } } 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; } StoreEntry * StoreController::get(const cache_key *key) { if (StoreEntry *e = swapDir->get(key)) { // TODO: ignore and maybe handleIdleEntry() unlocked intransit entries // because their backing store slot may be gone already. debugs(20, 3, HERE << "got in-transit entry: " << *e); return e; } if (memStore) { if (StoreEntry *e = memStore->get(key)) { debugs(20, 3, HERE << "got mem-cached entry: " << *e); return e; } @@ -823,43 +826,43 @@ void StoreController::handleIdleEntry(StoreEntry &e) { bool keepInLocalMemory = false; if (EBIT_TEST(e.flags, ENTRY_SPECIAL)) { // Icons (and cache digests?) should stay in store_table until we // have a dedicated storage for them (that would not purge them). // They are not managed [well] by any specific Store handled below. keepInLocalMemory = true; } else if (memStore) { memStore->considerKeeping(e); // leave keepInLocalMemory false; memStore maintains its own cache } else { keepInLocalMemory = keepForLocalMemoryCache(e) && // in good shape and // the local memory cache is not overflowing (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; } debugs(20, 5, HERE << "keepInLocalMemory: " << keepInLocalMemory); // TODO: move this into [non-shared] memory cache class when we have one if (keepInLocalMemory) { e.setMemStatus(IN_MEMORY); e.mem_obj->unlinkRequest(); } else { e.purgeMem(); // may free e } } StoreHashIndex::StoreHashIndex() { if (store_table) abort(); @@ -1060,43 +1063,43 @@ void StoreHashIndex::stat(StoreEntry & output) const { int i; /* Now go through each store, calling its stat routine */ for (i = 0; i < Config.cacheSwap.n_configured; ++i) { storeAppendPrintf(&output, "\n"); store(i)->stat(output); } } void StoreHashIndex::reference(StoreEntry &e) { e.store()->reference(e); } bool -StoreHashIndex::dereference(StoreEntry &e) +StoreHashIndex::dereference(StoreEntry &e, bool wantsLocalMemory) { - return e.store()->dereference(e); + return e.store()->dereference(e, wantsLocalMemory); } void StoreHashIndex::maintain() { int i; /* walk each fs */ for (i = 0; i < Config.cacheSwap.n_configured; ++i) { /* XXX FixMe: This should be done "in parallell" on the different * cache_dirs, not one at a time. */ /* call the maintain function .. */ store(i)->maintain(); } } void StoreHashIndex::sync() { === 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-10 18:37:14 +0000 @@ -11,21 +11,21 @@ MemStore::MemStore() STUB MemStore::~MemStore() STUB bool MemStore::keepInLocalMemory(const StoreEntry &) const STUB_RETVAL(false) void MemStore::considerKeeping(StoreEntry &) STUB void MemStore::reference(StoreEntry &) STUB void MemStore::maintain() STUB void MemStore::cleanReadable(const sfileno) STUB void MemStore::get(String const, STOREGETCLIENT, void *) STUB void MemStore::init() STUB void MemStore::getStats(StoreInfoStats&) const STUB void MemStore::stat(StoreEntry &) const STUB int MemStore::callback() STUB_RETVAL(0) StoreEntry *MemStore::get(const cache_key *) STUB_RETVAL(NULL) uint64_t MemStore::maxSize() const STUB_RETVAL(0) uint64_t MemStore::minSize() const STUB_RETVAL(0) 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) === modified file 'src/tests/testStore.h' --- src/tests/testStore.h 2012-08-28 13:00:30 +0000 +++ src/tests/testStore.h 2012-10-10 19:08:37 +0000 @@ -49,29 +49,29 @@ virtual void init(); virtual void maintain() {}; virtual uint64_t maxSize() const; virtual uint64_t minSize() const; virtual uint64_t currentSize() const; virtual uint64_t currentCount() const; virtual int64_t maxObjectSize() const; virtual void getStats(StoreInfoStats &) const; virtual void stat(StoreEntry &) const; /* output stats to the provided store entry */ 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 *); }; typedef RefCount TestStorePointer; #endif