Do not release entries that may be stored in memory cache later.

Since r11969, Squid calls trimMemory() for all entries, including
non-swappable, to release unused MemObjects memory.  But it should not
release objects that are or should be stored in local memory cache.
StoreEntry::trimMemory() has a check for IN_MEMORY status for that.  But
IN_MEMORY status is set too late in StoreController::handleIdleEntry(),
after trimMemory() marks entry for release:

  clientReplyContext::removeStoreReference()

    storeUnregister()
      StoreEntry::swapOut()
        StoreEntry::trimMemory()
          StoreEntry::releaseRequest()

    StoreEntry::unlock()
      StoreController::handleIdleEntry() // never get here because entry is
        set IN_MEMORY status             // already marked for release

The patch adds StoreController::keepInLocalMemory() method to determine
if an entry should be kept in memory for later use.  It uses different
checks depending on the configuration: shared memory cache checks are
implemented in MemStore::keepInLocalMemory() and local ones are in
StoreController::keepInLocalMemoryCache().  Both methods use
StoreEntry::memoryCachable() for general checks.  Shared memory
cache-specific checks are moved from StoreEntry::memoryCachable() to
MemStore::keepInLocalMemory().


=== modified file 'src/MemStore.cc'
--- src/MemStore.cc	2011-10-28 01:01:41 +0000
+++ src/MemStore.cc	2012-07-05 21:17:28 +0000
@@ -76,40 +76,51 @@ MemStore::stat(StoreEntry &e) const
         const int limit = map->entryLimit();
         storeAppendPrintf(&e, "Maximum entries: %9d\n", limit);
         if (limit > 0) {
             storeAppendPrintf(&e, "Current entries: %"PRId64" %.2f%%\n",
                               currentCount(), (100.0 * currentCount() / limit));
 
             if (limit < 100) { // XXX: otherwise too expensive to count
                 Ipc::ReadWriteLockStats stats;
                 map->updateStats(stats);
                 stats.dump(e);
             }
         }
     }
 }
 
 void
 MemStore::maintain()
 {
 }
 
+bool
+MemStore::keepInLocalMemory(const StoreEntry &e) const {
+    if (!e.memoryCachable())
+        return false;
+
+    Must(e.mem_obj);
+    const int64_t size =
+        max(e.mem_obj->expectedReplySize(), e.mem_obj->endOffset());
+    return willFit(size);
+}
+
 uint64_t
 MemStore::minSize() const
 {
     return 0; // XXX: irrelevant, but Store parent forces us to implement this
 }
 
 uint64_t
 MemStore::maxSize() const
 {
     return 0; // XXX: make configurable
 }
 
 uint64_t
 MemStore::currentSize() const
 {
     return theCurrentSize;
 }
 
 uint64_t
 MemStore::currentCount() const
@@ -252,41 +263,41 @@ MemStore::copyFromShm(StoreEntry &e, con
 }
 
 void
 MemStore::considerKeeping(StoreEntry &e)
 {
     if (!e.memoryCachable()) {
         debugs(20, 7, HERE << "Not memory cachable: " << e);
         return; // cannot keep due to entry state or properties
     }
 
     assert(e.mem_obj);
     if (!willFit(e.mem_obj->endOffset())) {
         debugs(20, 5, HERE << "No mem-cache space for " << e);
         return; // failed to free enough space
     }
 
     keep(e); // may still fail
 }
 
 bool
-MemStore::willFit(int64_t need)
+MemStore::willFit(const int64_t need) const
 {
     // TODO: obey configured maximum entry size (with page-based rounding)
     return need <= static_cast<int64_t>(Ipc::Mem::PageSize());
 }
 
 /// allocates map slot and calls copyToShm to store the entry in shared memory
 void
 MemStore::keep(StoreEntry &e)
 {
     if (!map) {
         debugs(20, 5, HERE << "No map to mem-cache " << e);
         return;
     }
 
     sfileno index = 0;
     Ipc::StoreMapSlot *slot = map->openForWriting(reinterpret_cast<const cache_key *>(e.key), index);
     if (!slot) {
         debugs(20, 5, HERE << "No room in mem-cache map to index " << e);
         return;
     }

=== modified file 'src/MemStore.h'
--- src/MemStore.h	2011-10-14 16:21:48 +0000
+++ src/MemStore.h	2012-07-05 21:12:45 +0000
@@ -26,45 +26,46 @@ public:
 
     /// cache the entry or forget about it until the next considerKeeping call
     void considerKeeping(StoreEntry &e);
 
     /* 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 void maintain();
+    virtual bool keepInLocalMemory(const StoreEntry &e) const;
 
     static int64_t EntryLimit();
 
 protected:
-    bool willFit(int64_t needed);
+    bool willFit(const int64_t need) 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".
 
 // Why not just use a SwapDir API? That would not help much because Store has
 // to check/update memory cache separately from the disk cache. And same API
 // would hurt because we can support synchronous get/put, unlike the disks.
 
 #endif /* SQUID_MEMSTORE_H */

=== modified file 'src/Store.h'
--- src/Store.h	2012-01-13 13:49:26 +0000
+++ src/Store.h	2012-07-05 21:24:31 +0000
@@ -334,40 +334,43 @@ public:
 
     /** 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 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) {}
 
+    /// whether entry should be kept in local memory for later use
+    virtual bool keepInLocalMemory(const StoreEntry &) const { return false; }
+
 private:
     static RefCount<Store> CurrentRoot;
 };
 
 /// \ingroup StoreAPI
 typedef RefCount<Store> StorePointer;
 
 /// \ingroup StoreAPI
 SQUIDCEXTERN size_t storeEntryInUse();
 
 /// \ingroup StoreAPI
 SQUIDCEXTERN const char *storeEntryFlags(const StoreEntry *);
 
 /// \ingroup StoreAPI
 extern void storeEntryReplaceObject(StoreEntry *, HttpReply *);
 
 /// \ingroup StoreAPI
 SQUIDCEXTERN StoreEntry *storeGetPublic(const char *uri, const HttpRequestMethod& method);
 
 /// \ingroup StoreAPI

=== modified file 'src/SwapDir.h'
--- src/SwapDir.h	2011-10-27 23:14:28 +0000
+++ src/SwapDir.h	2012-07-05 20:54:42 +0000
@@ -69,45 +69,48 @@ public:
 
     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 sync();	/* Sync the store prior to shutdown */
 
     virtual StoreSearch *search(String const url, HttpRequest *);
 
     virtual void reference(StoreEntry &);	/* Reference this object */
 
     virtual bool dereference(StoreEntry &);	/* Unreference this object */
 
+    virtual bool keepInLocalMemory(const StoreEntry &e) const;
+
     /* the number of store dirs being rebuilt. */
     static int store_dirs_rebuilding;
 
 private:
     void createOneStore(Store &aStore);
+    bool keepInLocalMemoryCache(const StoreEntry &e) const;
 
     StorePointer swapDir; ///< summary view of all disk caches
     MemStore *memStore; ///< memory cache
 };
 
 /* migrating from the Config based list of swapdirs */
 extern void allocate_new_swapdir(SquidConfig::_cacheSwap *);
 extern void free_cachedir(SquidConfig::_cacheSwap * swap);
 SQUIDCEXTERN OBJH storeDirStats;
 SQUIDCEXTERN char *storeDirSwapLogFile(int, const char *);
 SQUIDCEXTERN char *storeSwapFullPath(int, char *);
 SQUIDCEXTERN char *storeSwapSubSubDir(int, char *);
 SQUIDCEXTERN const char *storeSwapPath(int);
 SQUIDCEXTERN int storeDirWriteCleanLogs(int reopen);
 SQUIDCEXTERN STDIRSELECT *storeDirSelectSwapDir;
 SQUIDCEXTERN int storeVerifySwapDirs(void);
 SQUIDCEXTERN void storeDirCloseSwapLogs(void);
 SQUIDCEXTERN void storeDirCloseTmpSwapLog(int dirn);
 SQUIDCEXTERN void storeDirDiskFull(sdirno);
 SQUIDCEXTERN void storeDirOpenSwapLogs(void);

=== modified file 'src/store.cc'
--- src/store.cc	2012-01-13 13:49:26 +0000
+++ src/store.cc	2012-07-05 21:25:34 +0000
@@ -1435,47 +1435,44 @@ storeConfigure(void)
     store_swap_low = (long) (((float) Store::Root().maxSize() *
                               (float) Config.Swap.lowWaterMark) / (float) 100);
     store_pages_max = Config.memMaxSize / sizeof(mem_node);
 }
 
 bool
 StoreEntry::memoryCachable() const
 {
     if (mem_obj == NULL)
         return 0;
 
     if (mem_obj->data_hdr.size() == 0)
         return 0;
 
     if (mem_obj->inmem_lo != 0)
         return 0;
 
     if (!Config.onoff.memory_cache_first && swap_status == SWAPOUT_DONE && refcount == 1)
         return 0;
 
-    if (Config.memShared && IamWorkerProcess()) {
-        const int64_t expectedSize = mem_obj->expectedReplySize();
-        // objects of unknown size are not allowed into memory cache, for now
-        if (expectedSize < 0 ||
-                expectedSize > static_cast<int64_t>(Config.Store.maxInMemObjSize))
-            return 0;
-    }
+    const int64_t size =
+        max(mem_obj->expectedReplySize(), mem_obj->endOffset());
+    if (size > static_cast<int64_t>(Config.Store.maxInMemObjSize))
+        return 0;
 
     return 1;
 }
 
 int
 StoreEntry::checkNegativeHit() const
 {
     if (!EBIT_TEST(flags, ENTRY_NEGCACHED))
         return 0;
 
     if (expires <= squid_curtime)
         return 0;
 
     if (store_status != STORE_OK)
         return 0;
 
     return 1;
 }
 
 /**
@@ -1885,40 +1882,43 @@ StoreEntry::getSerialisedMetaData()
     StoreMeta *tlv_list = storeSwapMetaBuild(this);
     int swap_hdr_sz;
     char *result = storeSwapMetaPack(tlv_list, &swap_hdr_sz);
     storeSwapTLVFree(tlv_list);
     assert (swap_hdr_sz >= 0);
     mem_obj->swap_hdr_sz = (size_t) swap_hdr_sz;
     return result;
 }
 
 void
 StoreEntry::trimMemory(const bool preserveSwappable)
 {
     /*
      * DPW 2007-05-09
      * Bug #1943.  We must not let go any data for IN_MEMORY
      * objects.  We have to wait until the mem_status changes.
      */
     if (mem_status == IN_MEMORY)
         return;
 
+    if (Store::Root().keepInLocalMemory(*this))
+        return; // keep entry in memory for later use
+
     if (!preserveSwappable) {
         if (mem_obj->policyLowestOffsetToKeep(0) == 0) {
             /* Nothing to do */
             return;
         }
         /*
          * Its not swap-able, and we're about to delete a chunk,
          * so we must make it PRIVATE.  This is tricky/ugly because
          * for the most part, we treat swapable == cachable here.
          */
         releaseRequest();
         mem_obj->trimUnSwappable ();
     } else {
         mem_obj->trimSwappable ();
     }
 }
 
 bool
 StoreEntry::modifiedSince(HttpRequest * request) const
 {

=== modified file 'src/store_dir.cc'
--- src/store_dir.cc	2011-10-14 16:21:48 +0000
+++ src/store_dir.cc	2012-07-05 21:02:36 +0000
@@ -707,40 +707,57 @@ StoreController::dereference(StoreEntry
     bool keepInStoreTable = true; // keep if there are no objections
 
     /* Notify the fs that we're not referencing this object any more */
 
     if (e.swap_filen > -1)
         keepInStoreTable = swapDir->dereference(e) && 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;
 
     // 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);
     }
 
     return keepInStoreTable;
 }
 
+bool
+StoreController::keepInLocalMemory(const StoreEntry &e) const
+{
+    return memStore ? memStore->keepInLocalMemory(e) :
+        keepInLocalMemoryCache(e);
+}
+
+// whether entry should be kept in local memory cache
+bool
+StoreController::keepInLocalMemoryCache(const StoreEntry &e) const
+{
+    Must(!memStore); // Should(), really
+    return e.memoryCachable() && // entry is in good shape and
+        // the local memory cache is not overflowing
+        (mem_node::InUseCount() <= store_pages_max);
+}
+
 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;
         }
     }
 
     // TODO: this disk iteration is misplaced; move to StoreHashIndex when
     // the global store_table is no longer used for in-transit objects.
     if (const int cacheDirs = Config.cacheSwap.n_configured) {
@@ -763,45 +780,42 @@ StoreController::get(const cache_key *ke
     }
 
     debugs(20, 4, HERE << "none of " << Config.cacheSwap.n_configured <<
            " cache_dirs have " << storeKeyText(key));
     return NULL;
 }
 
 void
 StoreController::get(String const key, STOREGETCLIENT aCallback, void *aCallbackData)
 {
     fatal("not implemented");
 }
 
 void
 StoreController::handleIdleEntry(StoreEntry &e)
 {
     bool keepInLocalMemory = false;
     if (memStore) {
         memStore->considerKeeping(e);
         // leave keepInLocalMemory false; memStore maintains its own cache
-    } else {
-        keepInLocalMemory = e.memoryCachable() && // entry is in good shape and
-                            // the local memory cache is not overflowing
-                            (mem_node::InUseCount() <= store_pages_max);
-    }
+    } else
+        keepInLocalMemory = keepInLocalMemoryCache(e);
 
     // An idle, unlocked entry that belongs to a SwapDir which controls
     // its own index, should not stay in the global store_table.
     if (!dereference(e)) {
         debugs(20, 5, HERE << "destroying unlocked entry: " << &e << ' ' << e);
         destroyStoreEntry(static_cast<hash_link*>(&e));
         return;
     }
 
     // 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()
 {